Re: [PATCH 1/5] Add the explanation and sample of RapidIO DTS sector to the document of booting-without-of.txt file.

[Segher Boessenkool <segher@kernel.crashing.org>]

> > > +    - device_type : Should be "rapidio"
> >
> > There is no OF binding, so no.
>
> So, we need to define it.

If you want to.  Until that has been done, don't use
a "device_type".  Linux won't use it, anyway.

> > > +    - compatible : Should be "fsl,rapidio-v0.0" or
> > "fsl,rapidio-v1.0"
> > > +      and so on. The version number is got from IP Block Revision
> > > +      Register of RapidIO controller.
> >
> > It's better to use real device names, just like everyone
> > else.
>
> Some silicons of Freescale processor are the same RapidIO controller,
> such as mpc8540/mpc8560 are the same (v0.0), mpc8548/mpc8641 are the
> same (v1.0). For v1.0 RapidIO controller, should we use mpc8548 or
> mpc8641? Those will make people confused.

Not at all.  On an 8641 it could be

	compatible = "fsl,mpc8641-rapidio" "fsl,mpc8548-rapidio";

which states "this is the 8641 thing and it is compatible
to the 8548 thing".  Perfectly clear.

> Using IP Block Revision is a
> clear choice.

I don't think so.  For one thing, it describes a version of
a cell design, not a version of an actual device.  For another
thing, if I hear "8641" I know what you're talking about (sort
of, anyway), but I draw a blank stare if you say "v1.0".  I'm
sure I'm not the only one.  Concrete names are good.

> > > +    - #address-cells : Address representation for
> > "rapidio" devices.
> > > +      This field represents the number of cells needed to represent
> > > +      the RapidIO address of the registers.  For
> > supporting more than
> > > +      36-bits RapidIO address, this field should be <2>.
> >
> > More than 32 bit?
>
> Yes, RapidIO bus address width is 34 bits.

You said "more than 36 bit", I tried to ask if that is a typo
perhaps.

> > No.  The format of an "interrupts" entry is defined by
> > the interrupt domain this device sits in, not by the
> > device itself.
> Do you misunderstand the meaning of 'interrupts'?

Hahaha.  No, I don't misunderstand what the "interrupts" property
means.  Perhaps you do?

> These interrupts is
> issued from the RapidIO controller to the pic controller for tx, rx,
> err, doorbell and message.

But the rapidio node doesn't know or care what the interrupts
are connected to, and neither should it.  That's what the
interrupt mapping recommended practice is for.

> > > For this sector, interrupts order should be
> > > +      <err_irq bell_outb_irq bell_inb_irq msg1_tx_irq msg1_rx_irq
> > > +      msg2_tx_irq msg2_rx_irq ... msgN_tx_irq msgN_rx_irq>.
> >
> > That's to be defined in the binding for your specific device,
> > not in a more generic rapidio binding.
>
> These description is just for compatible="fsl,rapidio-v*.*" rapidio
> controller.

Okay, good.  Please make that way more obvious then :-)

> > > +		#address-cells = <2>;
> >
> > You want a #size-cells as well.
>
> The size is not used in this sector, so no defined.

The size _is_ used; in the "ranges" property in this node,
for example.  It is also needed to describe the "reg" for
any child node of this node.

A non-existant "#size-cells" means 1, and "#address-cells"
means 2, so in principle you could do without these
properties; but Linux doesn't parse the tree correctly in
that case (which reminds me, I have some more patches to
send).

> Thanks!

My pleasure,


Segher

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/