Re: [PATCH] of: add bus-number specification to spi_mpc8xxx

Re: [PATCH] of: add bus-number specification to spi_mpc8xxx

[Grant Likely]

[cc'd spi-devel-generial too]

On Tue, Feb 16, 2010 at 8:08 AM, Ernst Schwab <eschwab-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org> wrote:
> Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
>
>> No, unless you can provide a really compelling reason to do so, the
>> goal is to *not* specify Linux implementation detail things like bus
>> numbers in the device tree.
>>
>> Instead, your code should be using spi device child nodes from the
>> bus, or finding the spi bus node and decoding the dynamically assigned
>> bus number from there.  Don't hard code spi bus numbers.
>
> Ok, I understand that no operating system specific stuff should be
> specified in the device tree. But: the bus numbers may be specified
> by the hardware design or the used microcontroller, and I think
> there should be a way to propagate the bus name/number present in
> the hardware design to the operating system. Maybe by adding a
> 'label' property, or by parsing the node name?

Add a property to the /chosen node to assign short labels to devices.

> Documentation/spi/spi-summary tells us to use fixed numbers for SOC
> systems, but unfortunately, we have no method to specify them with
> the device tree:

That document predates SPI device tree bindings, and assumes that
platform data structures will be used to populate SPI busses.
Platform data structures require fixed numbers to line up data with
bus instances.  That problem doesn't exist when using the device tree.

Unless you're trying to line up disparate data structure, the actually
number assigned to a bus really doesn't matter.  It is better to let
Linux dynamically assign than to manually maintain the assigned bus
numbers for each machine.  Assuming dynamic assignment also protects
against breaking userspace applications when, for whatever reason, the
bus numbers get shuffled on a platform.

g.

>
> "
> Bus numbering is important, since that's how Linux identifies a given
> SPI bus (shared SCK, MOSI, MISO).  Valid bus numbers start at zero.  On
> SOC systems, the bus numbers should match the numbers defined by the chip
> manufacturer.  For example, hardware controller SPI2 would be bus number 2,
> and spi_board_info for devices connected to it would use that number.
>
> If you don't have such hardware-assigned bus number, and for some reason
> you can't just assign them, then provide a negative bus number.  That will
> then be replaced by a dynamically assigned number. You'd then need to treat
> this as a non-static configuration (see above).
> "
>
> Ernst
>
> P.S:
> Unfortunately, the proposed patch wasn't sent to the mailing list by mistake,
> so here it is again for reference.
>
> ---
> From: Ernst Schwab <eschwab-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org>
>
> Added devicetree parsing for SPI bus number. If no bus number is specified,
> a dynamically assigned bus number will be used (typically 32766).
>
> Signed-off-by: Ernst Schwab <eschwab-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org>
> ---
> diff -upr a/Documentation/powerpc/dts-bindings/fsl/spi.txt b/Documentation/powerpc/dts-bindings/fsl/spi.txt
> --- a/Documentation/powerpc/dts-bindings/fsl/spi.txt
> +++ b/Documentation/powerpc/dts-bindings/fsl/spi.txt
> @@ -4,6 +4,8 @@ Required properties:
>  - cell-index : SPI controller index.
>  - compatible : should be "fsl,spi".
>  - mode : the SPI operation mode, it can be "cpu" or "cpu-qe".
> +- bus-number : number of the SPI bus. If unspecified, a dynamically
> +  assigned bus number will be used.
>  - reg : Offset and length of the register set for the device
>  - interrupts : <a b> where a is the interrupt number and b is a
>   field that represents an encoding of the sense and level
> @@ -21,4 +23,5 @@ Example:
>                interrupts = <82 0>;
>                interrupt-parent = <700>;
>                mode = "cpu";
> +               bus-number = <0>;
>        };
> diff -upr a/drivers/spi/spi_mpc8xxx.c b/drivers/spi/spi_mpc8xxx.c
> --- a/drivers/spi/spi_mpc8xxx.c
> +++ b/drivers/spi/spi_mpc8xxx.c
> @@ -1230,6 +1230,8 @@ static int __devinit of_mpc8xxx_spi_prob
>        struct resource mem;
>        struct resource irq;
>        const void *prop;
> +       const int *bus_num;
> +       int len;
>        int ret = -ENOMEM;
>
>        pinfo = kzalloc(sizeof(*pinfo), GFP_KERNEL);
> @@ -1239,8 +1241,16 @@ static int __devinit of_mpc8xxx_spi_prob
>        pdata = &pinfo->pdata;
>        dev->platform_data = pdata;
>
> -       /* Allocate bus num dynamically. */
> -       pdata->bus_num = -1;
> +
> +       bus_num = of_get_property(np, "bus-number", &len);
> +       if (bus_num && len == sizeof(*bus_num)) {
> +               /* bus number is specified in node */
> +               pdata->bus_num = *bus_num;
> +       } else {
> +               /* Allocate bus num dynamically. */
> +               pdata->bus_num = -1;
> +       }
> +
>
>        /* SPI controller is either clocked from QE or SoC clock. */
>        pdata->sysclk = get_brgfreq();
>
>
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] of: add bus-number specification to spi_mpc8xxx

[Ernst Schwab]

Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:

> Add a property to the /chosen node to assign short labels to devices.

I'm not familiar with this and will check - does anyone know of an
existing example for this? 

> Unless you're trying to line up disparate data structure, the actually
> number assigned to a bus really doesn't matter.  It is better to let
> Linux dynamically assign than to manually maintain the assigned bus
> numbers for each machine.  Assuming dynamic assignment also protects
> against breaking userspace applications when, for whatever reason, the
> bus numbers get shuffled on a platform.

Hm. What if we have two identical devices, lets say EEPROMs at25,
connected to two SPI controllers. These are set up as 
/sys/bus/spi/devices/spi32766.0/eeprom and 
/sys/bus/spi/devices/spi32765.0/eeprom. 
How can userspace code determine which one it should 
use if it needs to use the one connected to the first spi controller?
Can this be solved with the /chosen mechanism?

Regards
Ernst

------------------------------------------------------------------------------
SOLARIS 10 is the OS for Data Centers - provides features such as DTrace,
Predictive Self Healing and Award Winning ZFS. Get Solaris 10 NOW
http://p.sf.net/sfu/solaris-dev2dev

Re: [PATCH] of: add bus-number specification to spi_mpc8xxx

[Grant Likely]

On Tue, Feb 16, 2010 at 11:59 AM, Ernst Schwab <eschwab-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org> wrote:
> Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
>
>> Add a property to the /chosen node to assign short labels to devices.
>
> I'm not familiar with this and will check - does anyone know of an
> existing example for this?

Oops, sorry.  I meant the 'aliases' node.  'chosen' is for something else.

Properties in the aliases node are simply name = "/path/to/node";
pairs.  Do "grep -A 6 arch/powerpc/boot/dts/" to see lots of examples.
dtc expands the <name> = &label; construct into the full path to the
node.

>> Unless you're trying to line up disparate data structure, the actually
>> number assigned to a bus really doesn't matter.  It is better to let
>> Linux dynamically assign than to manually maintain the assigned bus
>> numbers for each machine.  Assuming dynamic assignment also protects
>> against breaking userspace applications when, for whatever reason, the
>> bus numbers get shuffled on a platform.
>
> Hm. What if we have two identical devices, lets say EEPROMs at25,
> connected to two SPI controllers. These are set up as
> /sys/bus/spi/devices/spi32766.0/eeprom and
> /sys/bus/spi/devices/spi32765.0/eeprom.
> How can userspace code determine which one it should
> use if it needs to use the one connected to the first spi controller?
> Can this be solved with the /chosen mechanism?

Use the alias to find the spi bus you care about in /sys/devices/.
Then whichever eeprom has that bus as its parent is the device you
want.  You can read the expanded device tree in /proc/device-tree.
Unfortunately I don't have sample code that shows how to do this; I
haven't had to do so personally yet.

g.

Re: [PATCH] of: add bus-number specification to spi_mpc8xxx

[Ernst Schwab]

On Tue, 16 Feb 2010 14:23:49 -0700
Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:

> On Tue, Feb 16, 2010 at 11:59 AM, Ernst Schwab <eschwab-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org> wrote:
> > Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> >
> >> Add a property to the /chosen node to assign short labels to devices.
> >
> > I'm not familiar with this and will check - does anyone know of an
> > existing example for this?
> 
> Oops, sorry.  I meant the 'aliases' node.  'chosen' is for something else.
> 
> Properties in the aliases node are simply name = "/path/to/node";
> pairs.  Do "grep -A 6 arch/powerpc/boot/dts/" to see lots of examples.
> dtc expands the <name> = &label; construct into the full path to the
> node.

Ok, as I did the grep with "chosen" there wasn't anything, "aliases" 
looks much better. I was aware of this node, but had no idea how
to use it. Actually, I still do not exactly know how to use it ;-).

> Use the alias to find the spi bus you care about in /sys/devices/.
> Then whichever eeprom has that bus as its parent is the device you
> want.  You can read the expanded device tree in /proc/device-tree.
> Unfortunately I don't have sample code that shows how to do this; I
> haven't had to do so personally yet.

I created 
	aliases { ...
		spi0 = &spi;
		eepmain   = &eepmain;
	};

	spi:spi@7000 { ...
		eepmain:eeprom@8 { ...
		};
	};

The aliases are visible at /proc/device-tree/aliases/:
eepmain   : contains /immr@e0000000/spi@7000/eeprom@8
spi0      : contains /immr@e0000000/spi@7000

The EEPROM can be accessed under 
	/sys/bus/spi/devices/spi32766.8/eeprom

The alias names do not appear in /sys as far as I can see.
The paths (e.g. /immr@e0000000/spi@7000) seems not to be useable
to address something in the /sys directory directly.

Does this mean that software which wants to access 'eepmain' 
has to reformat the alias value to construct a path in the sysfs
to access the EEPROM contents?

Regards
Ernst