SPI devices and OF

SPI devices and OF

[Sascha Hauer]

Hi,

I'm currently writing a driver for the mpc5200 spi controller (the
dedicated one, not the PSC ones).
The driver has the form of a of_platform_driver. My problem is that I
don't know how to define the spi devices on the spi bus. My current
approach is having something like this in the OF tree:


	spi@f00 {
		device_type = "spi";
		compatible = "mpc5200b-spi\0mpc5200-spi";
		reg = <f00 20>;
		interrupts = <2 d 0 2 e 0>;
		interrupt-parent = <500>;
		mmc@0 {
			device_type = "mmc_spi";
			compatible = "mmc_spi";
		};
	};

I can then parse the children in my spi driver with

	while( (child = of_get_next_child(odev->node, child))) {
		struct spi_board_info info;

		info.max_speed_hz = 
		info.bus_num = 
		info.chip_select = 
		...
		spi_register_board_info(&info, 1);
	}

I think it will work this way but I found no way getting the
platform_data for the spi devices.

I could also define the spi devices in a board specific c file but there
I would not know which spi bus is which if there is more than one.

Any thoughts on this or am I completely on the wrong track?

Sascha

Re: SPI devices and OF

[Vitaly Wool]

On 4/4/07, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> Hi,
>
> I'm currently writing a driver for the mpc5200 spi controller (the
> dedicated one, not the PSC ones).
> The driver has the form of a of_platform_driver. My problem is that I
> don't know how to define the spi devices on the spi bus. My current
> approach is having something like this in the OF tree:
>
>
>         spi@f00 {
>                 device_type = "spi";
>                 compatible = "mpc5200b-spi\0mpc5200-spi";
>                 reg = <f00 20>;
>                 interrupts = <2 d 0 2 e 0>;
>                 interrupt-parent = <500>;
>                 mmc@0 {
>                         device_type = "mmc_spi";
>                         compatible = "mmc_spi";
>                 };
>         };
>
> I can then parse the children in my spi driver with
>
>         while( (child = of_get_next_child(odev->node, child))) {
>                 struct spi_board_info info;
>
>                 info.max_speed_hz =
>                 info.bus_num =
>                 info.chip_select =
>                 ...
>                 spi_register_board_info(&info, 1);
>         }
>
> I think it will work this way but I found no way getting the
> platform_data for the spi devices.

I think that it's worth extending the current SPI core with OF
support. Maybe I'll manage to get to it :)
If you have something ready, please email to
spi-devel-general@lists.sourceforge.net.

Thanks,
   Vitaly

Re: SPI devices and OF

[Kumar Gala]

On Apr 4, 2007, at 6:51 AM, Vitaly Wool wrote:

> On 4/4/07, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> Hi,
>>
>> I'm currently writing a driver for the mpc5200 spi controller (the
>> dedicated one, not the PSC ones).
>> The driver has the form of a of_platform_driver. My problem is that I
>> don't know how to define the spi devices on the spi bus. My current
>> approach is having something like this in the OF tree:
>>
>>
>>         spi@f00 {
>>                 device_type = "spi";
>>                 compatible = "mpc5200b-spi\0mpc5200-spi";
>>                 reg = <f00 20>;
>>                 interrupts = <2 d 0 2 e 0>;
>>                 interrupt-parent = <500>;
>>                 mmc@0 {
>>                         device_type = "mmc_spi";
>>                         compatible = "mmc_spi";
>>                 };
>>         };
>>
>> I can then parse the children in my spi driver with
>>
>>         while( (child = of_get_next_child(odev->node, child))) {
>>                 struct spi_board_info info;
>>
>>                 info.max_speed_hz =
>>                 info.bus_num =
>>                 info.chip_select =
>>                 ...
>>                 spi_register_board_info(&info, 1);
>>         }
>>
>> I think it will work this way but I found no way getting the
>> platform_data for the spi devices.
>
> I think that it's worth extending the current SPI core with OF
> support. Maybe I'll manage to get to it :)
> If you have something ready, please email to
> spi-devel-general@lists.sourceforge.net.

I don't think this is a good idea for SPI devices.  The effort vs  
reward isn't worth it.  The simple fact that the 'chip select'  
mechanism ends up being board specific is too much of a pain to  
figure out how to deal with in the device tree.  I think its ok if we  
put information about the controller in the tree, but trying to do  
the devices as well at this point doesn't seem like its much of a win.

- k

Re: SPI devices and OF

[Arnd Bergmann]

On Wednesday 04 April 2007, Kumar Gala wrote:
> I don't think this is a good idea for SPI devices. =A0The effort vs =A0
> reward isn't worth it. =A0The simple fact that the 'chip select' =A0
> mechanism ends up being board specific is too much of a pain to =A0
> figure out how to deal with in the device tree. =A0I think its ok if we =
=A0
> put information about the controller in the tree, but trying to do =A0
> the devices as well at this point doesn't seem like its much of a win.
>=20

I'm not following your argumentation. If the chip select is board specific,
isn't the device tree for that board _exactly_ the place where that
information should be? Where else would you get it from?

	Arnd <><

Re: SPI devices and OF

[Kumar Gala]

On Apr 4, 2007, at 12:03 PM, Arnd Bergmann wrote:

> On Wednesday 04 April 2007, Kumar Gala wrote:
>> I don't think this is a good idea for SPI devices.  The effort vs
>> reward isn't worth it.  The simple fact that the 'chip select'
>> mechanism ends up being board specific is too much of a pain to
>> figure out how to deal with in the device tree.  I think its ok if we
>> put information about the controller in the tree, but trying to do
>> the devices as well at this point doesn't seem like its much of a  
>> win.
>>
>
> I'm not following your argumentation. If the chip select is board  
> specific,
> isn't the device tree for that board _exactly_ the place where that
> information should be? Where else would you get it from?

 From explicitly board code like we do today.  I mean the mechanism  
can very so greatly that trying to decided and come up with all  
possible cases and somehow encoding that in the device tree isn't  
worth the effort.  Additionally you'll still need code to handle the  
actual chip select and I don't see how you make that generic at all.

A board designer could use I2C, GPIO, or something off an FPGA.  I  
just dont see trying to 'encode' this in the device tree as providing  
any real value.

- k

Re: SPI devices and OF

[Benjamin Herrenschmidt]

>  From explicitly board code like we do today.  I mean the mechanism  
> can very so greatly that trying to decided and come up with all  
> possible cases and somehow encoding that in the device tree isn't  
> worth the effort.  Additionally you'll still need code to handle the  
> actual chip select and I don't see how you make that generic at all.
> 
> A board designer could use I2C, GPIO, or something off an FPGA.  I  
> just dont see trying to 'encode' this in the device tree as providing  
> any real value.

Or board designers can use board specific device-tree bits and board
specific code to udnerstand them :-) That works too and can be handy if
you have for example several versions of a board with small differences
that you want to expose that way in the device-tree.

That is, the devive-tree -can- be used to put proprietary stuff, though
if you do so, you should try to use prefixes on your properties, like
mycompany,xxxx

Ben.

Re: SPI devices and OF

[Milton Miller]

Ben Herrenschmidt wrote:
> Kumar Gala wrote:
>>  From explicitly board code like we do today.  I mean the mechanism
>> can very so greatly that trying to decided and come up with all
>> possible cases and somehow encoding that in the device tree isn't
>> worth the effort.  Additionally you'll still need code to handle the
>> actual chip select and I don't see how you make that generic at all.

At this point I don't think we are trying to make a generic master.  At
most we are trying to expose the spi device in a manner that can be
reused by board-specific master drivers.

>> A board designer could use I2C, GPIO, or something off an FPGA.  I
>> just dont see trying to 'encode' this in the device tree as providing
>> any real value.
>
> Or board designers can use board specific device-tree bits and board
> specific code to udnerstand them :-) That works too and can be handy if
> you have for example several versions of a board with small differences
> that you want to expose that way in the device-tree.
>
> That is, the devive-tree -can- be used to put proprietary stuff, though
> if you do so, you should try to use prefixes on your properties, like
> mycompany,xxxx

I agree with Ben here.

Asserting the chip select is the responsibility of the master driver,
and that is selected by the compatable property in the device tree
to a specific driver.

The reg property in the device node can be useful to the master driver
wihtout expressing how to assert it in the master node.

milton

Re: SPI devices and OF

[Sascha Hauer]

On Wed, Apr 04, 2007 at 11:34:11AM -0500, Kumar Gala wrote:
> 
> On Apr 4, 2007, at 6:51 AM, Vitaly Wool wrote:
> 
> 
> I don't think this is a good idea for SPI devices.  The effort vs  
> reward isn't worth it.  The simple fact that the 'chip select'  
> mechanism ends up being board specific is too much of a pain to  
> figure out how to deal with in the device tree.  I think its ok if we  
> put information about the controller in the tree, but trying to do  
> the devices as well at this point doesn't seem like its much of a win.

It would be much easier to provide some board specific functions which
registers the spi devices instead of doing it in the device tree.
But there is one problem with that: If there is more than one spi
controller in the tree, how do you differentiate them?


Sascha

Re: SPI devices and OF

[Milton Miller]

On Wed Apr 4 21:09:16 EST 2007, Sascha Hauer wrote:

> I'm currently writing a driver for the mpc5200 spi controller (the
> dedicated one, not the PSC ones).
>
> The driver has the form of a of_platform_driver. My problem is that I
> don't know how to define the spi devices on the spi bus. My current
> approach is having something like this in the OF tree:
>
>
>         spi at f00 {
>                 device_type = "spi";
>                 compatible = "mpc5200b-spi\0mpc5200-spi";
>                 reg = <f00 20>;
>                 interrupts = <2 d 0 2 e 0>;
>                 interrupt-parent = <500>;
>                 mmc at 0 {
>                         device_type = "mmc_spi";
>                         compatible = "mmc_spi";

Why is the device_type mmc_spi?   Is this a MMC adapter?
The type of a network card is "network", not "pci_network".


>                 };
>         };
>
> I can then parse the children in my spi driver with
>
>         while( (child = of_get_next_child(odev->node, child))) {
>                 struct spi_board_info info;
                 (fill out info) ...
>                 spi_register_board_info(&info, 1);
>         }
>
> I think it will work this way but I found no way getting the
> platform_data for the spi devices.

Why do you need platform_data for the devices?  I understand for
the master.   Actually, spi_device_info has a nice platform_info 
pointer.

> I could also define the spi devices in a board specific c file but 
> there
> I would not know which spi bus is which if there is more than one.

The device tree is definately preferred.

> Any thoughts on this or am I completely on the wrong track?
>
> Sascha

First of all, I have no knowledge of SPI.

However, looking at drivers/spi/spi.c, it looks
spi_register_board_info is for staticly determined tables that are
known at _init and the spi master busses are enumerated in some
platform-dtermined way.  Since your proposed binding doesn't have
an enumeration of the spi master, that does not match.  I guess you
could register them with sequential numbers as you encounter the
masters in the device tree.  However, if this is code is in your
driver, it seems to be the wrong point in time, you need something
_init_or_module.

Looking through the code, It seems you want to use spi_new_device()
after calling spi_alloc_master() and spi_register_master().

However

Re: SPI devices and OF

[Sascha Hauer]

On Wed, Apr 04, 2007 at 10:57:28AM -0500, Milton Miller wrote:
> On Wed Apr 4 21:09:16 EST 2007, Sascha Hauer wrote:
> 
> >I'm currently writing a driver for the mpc5200 spi controller (the
> >dedicated one, not the PSC ones).
> >
> >The driver has the form of a of_platform_driver. My problem is that I
> >don't know how to define the spi devices on the spi bus. My current
> >approach is having something like this in the OF tree:
> >
> >
> >        spi at f00 {
> >                device_type = "spi";
> >                compatible = "mpc5200b-spi\0mpc5200-spi";
> >                reg = <f00 20>;
> >                interrupts = <2 d 0 2 e 0>;
> >                interrupt-parent = <500>;
> >                mmc at 0 {
> >                        device_type = "mmc_spi";
> >                        compatible = "mmc_spi";
> 
> Why is the device_type mmc_spi?   Is this a MMC adapter?
> The type of a network card is "network", not "pci_network".

MMC cards have a SPI mode, so mmc_spi ist meant as "MMC over SPI". This
is the name the linux mmc over spi driver matches on.

> 
> 
> >                };
> >        };
> >
> >I can then parse the children in my spi driver with
> >
> >        while( (child = of_get_next_child(odev->node, child))) {
> >                struct spi_board_info info;
>                 (fill out info) ...
> >                spi_register_board_info(&info, 1);
> >        }
> >
> >I think it will work this way but I found no way getting the
> >platform_data for the spi devices.
> 
> Why do you need platform_data for the devices?  I understand for
> the master.   Actually, spi_device_info has a nice platform_info 
> pointer.

For example the AT25 eeprom driver needs the page_size and some other bits in
the platform_data.

> 
> However, looking at drivers/spi/spi.c, it looks
> spi_register_board_info is for staticly determined tables that are
> known at _init and the spi master busses are enumerated in some
> platform-dtermined way.  Since your proposed binding doesn't have
> an enumeration of the spi master, that does not match.  I guess you
> could register them with sequential numbers as you encounter the
> masters in the device tree.  However, if this is code is in your
> driver, it seems to be the wrong point in time, you need something
> _init_or_module.
> 
> Looking through the code, It seems you want to use spi_new_device()
> after calling spi_alloc_master() and spi_register_master().

Yes, you're right. I mixed that up. Anyway, it was only meant as pseudo
code to show what I'm trying to do.

Sascha

Re: SPI devices and OF

[Milton Miller]

On Apr 5, 2007, at 3:50 AM, Sascha Hauer wrote:

> On Wed, Apr 04, 2007 at 10:57:28AM -0500, Milton Miller wrote:
>> On Wed Apr 4 21:09:16 EST 2007, Sascha Hauer wrote:
>>
>>> I'm currently writing a driver for the mpc5200 spi controller (the
>>> dedicated one, not the PSC ones).
...
>>> I think it will work this way but I found no way getting the
>>> platform_data for the spi devices.
>>
>> Why do you need platform_data for the devices?  I understand for
>> the master.   Actually, spi_device_info has a nice platform_info
>> pointer.
>
> For example the AT25 eeprom driver needs the page_size and some other 
> bits in
> the platform_data.

>> Looking through the code, It seems you want to use spi_new_device()
>> after calling spi_alloc_master() and spi_register_master().
>
> Yes, you're right. I mixed that up. Anyway, it was only meant as pseudo
> code to show what I'm trying to do.

ok, so what is the problem with the platform data?   You kzalloc the
data structure that the device needs, fill it in, and fill out
the pointer in the auto _info variable that gets copied by
register_new_device.

Am I missing something?

milton

Re: SPI devices and OF

[Sascha Hauer]

On Thu, Apr 05, 2007 at 09:44:13AM -0500, Milton Miller wrote:
> 
> 
> ok, so what is the problem with the platform data?

Not a problem of programming technique but a problem of where to
actually get the information in the platform from. device tree or
board setup code?

> You kzalloc the
> data structure that the device needs, fill it in, and fill out
> the pointer in the auto _info variable that gets copied by
> register_new_device.

My point is that we have two possibilities to handle spi devices. a) We
put the information about all devices connected to a spi bus into the
device tree in which case we must also put the data described in the
platform_data (e.g. the mentioned page_size for at25 eeproms) into the
tree. b) would be to put only the spi masters into the tree (well they
already are).

It seems b) is the prefered way. If I understand Ben correctly I can
then put some proprietary tag into the tree to differentiate between
different spi controllers so that the board code knows which spi
controller is which. So my tree could look like:

spi@f00 {
	device_type = "spi";
	compatible = "mpc5200b-spi\0mpc5200-spi";
	reg = <f00 20>;
	interrupts = <2 d 0 2 e 0>;
	interrupt-parent = <500>;
	identifier = <0>;
}

spi@xxx {
	device_type = "spi";
	...
	identifier = <1>;
}

I can then use the board code to populate the spi devices depending on
the identifier. Perhaps there is a better name instead of 'identifier'?

Sascha

Re: SPI devices and OF

[Arnd Bergmann]

On Thursday 05 April 2007, Sascha Hauer wrote:
> Not a problem of programming technique but a problem of where to
> actually get the information in the platform from. device tree or
> board setup code?

There should not need to be board specific setup code in many cases,
so it should be in the device tree.

> > You kzalloc the
> > data structure that the device needs, fill it in, and fill out
> > the pointer in the auto _info variable that gets copied by
> > register_new_device.
>=20
> My point is that we have two possibilities to handle spi devices. a) We
> put the information about all devices connected to a spi bus into the
> device tree in which case we must also put the data described in the
> platform_data (e.g. the mentioned page_size for at25 eeproms) into the
> tree. b) would be to put only the spi masters into the tree (well they
> already are).

If all the information you need can be probed through SPI commands, you
don't need to have a device tree layout for it. That is how we deal
with PCI devices as well. However, if there are things you need to know
in the specific driver for an SPI attached device, it should be in
the tree.

> It seems b) is the prefered way. If I understand Ben correctly I can
> then put some proprietary tag into the tree to differentiate between
> different spi controllers so that the board code knows which spi
> controller is which. So my tree could look like:
>=20
> spi@f00 {
> =A0=A0=A0=A0=A0=A0=A0=A0device_type =3D "spi";
> =A0=A0=A0=A0=A0=A0=A0=A0compatible =3D "mpc5200b-spi\0mpc5200-spi";
> =A0=A0=A0=A0=A0=A0=A0=A0reg =3D <f00 20>;
> =A0=A0=A0=A0=A0=A0=A0=A0interrupts =3D <2 d 0 2 e 0>;
> =A0=A0=A0=A0=A0=A0=A0=A0interrupt-parent =3D <500>;
> =A0=A0=A0=A0=A0=A0=A0=A0identifier =3D <0>;
> }
>=20
> spi@xxx {
> =A0=A0=A0=A0=A0=A0=A0=A0device_type =3D "spi";
> =A0=A0=A0=A0=A0=A0=A0=A0...
> =A0=A0=A0=A0=A0=A0=A0=A0identifier =3D <1>;
> }
>=20
> I can then use the board code to populate the spi devices depending on
> the identifier. Perhaps there is a better name instead of 'identifier'?

I think this is backwards. The board code should not need to know
about it, but you identify the chip select based on the "compatible"
property. Your of_platform_driver probe will then look more or less like
this:

struct of_device_id of_spi_ids[] =3D {
	{ .type =3D "spi", .compatible =3D "mpc5200b-spi",
			 .data =3D SPI_PROBE_MPC5200b },
	{ .type =3D "spi", .compatible =3D "mpc52foo-spi",
			 .data =3D SPI_PROBE_MPC52FOO },
	...
};

static int of_spi_probe(struct of_device* dev,
	const struct of_device_id *match)
{
	int ret;

	/* common initialization */
	...

	/* board specific initialition */
	switch (match->data) {
	case SPI_PROBE_MPC5200b:
		ret =3D of_spi_probe_mpc5200b(dev);
		break;
	case SPI_PROBE_MPC5200b:
		ret =3D of_spi_probe_mpc5200b(dev);
		break;
	...
	}
=09
	/* register with spi layer */
	...

	return ret;
}

	Arnd <><