[U-Boot] [PATCH 06/25] dm: spi: Add a uclass for SPI

Mon Jul 21 04:17:21 CEST 2014

Hi Pavel,

On 17 July 2014 12:29, Pavel Herrmann <morpheus.ibis at gmail.com> wrote:
> On Thursday 17 of July 2014 20:01:29 Pavel Herrmann wrote:
>> Hi
>>
>> On Thursday 17 of July 2014 09:26:47 Simon Glass wrote:
>> > Hi Pavel,
>> >
>> > On 17 July 2014 01:57, Pavel Herrmann <morpheus.ibis at gmail.com> wrote:
>> > > Hi
>> > >
>> > > On Wednesday 16 of July 2014 23:39:44 Simon Glass wrote:
>> > >> Hi Pavel,
>> > >>
>> > >> On 15 July 2014 02:26, Pavel Herrmann <morpheus.ibis at gmail.com> wrote:
>> > >> > Hi
>> > >> >
>> > >> > On Monday 14 of July 2014 18:56:13 Simon Glass wrote:
>> > >> > > Add a uclass which provides access to SPI buses and includes
>> > >> > > operations
>> > >> > > required by SPI.
>> > >> > >
>> > >> > > For a time driver model will need to co-exist with the legacy SPI
>> > >> > > interface
>> > >> > > so some parts of the header file are changed depending on which is
>> > >> > > in
>> > >> > > use.
>> > >> > > The exports are adjusted also since some functions are not
>> > >> > > available
>> > >> > > with
>> > >> > > driver model.
>> > >> > >
>> > >> > > Boards must define CONFIG_DM_SPI to use driver model for SPI.
>> > >> > >
>> > >> > > Signed-off-by: Simon Glass <sjg at chromium.org>
>> > >> > > ---
>> > >> > >
>> > >> > > ...
>> > >> > > +int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
>> > >> > > +          const void *dout, void *din, unsigned long flags)
>> > >> > > +{
>> > >> > > +     struct udevice *dev = slave->dev;
>> > >> > > +     struct udevice *bus = dev->parent;
>> > >> >
>> > >> > is this the best interface here?
>> > >> > I think it would be cleaner if bus drivers had interfaces which
>> > >> > follow
>> > >> > a
>> > >> > certain template, such as
>> > >> > bus_ops(struct udevice *bus, struct udevice *child, ...)
>> > >>
>> > >> Thanks for your comments.
>> > >>
>> > >> Well I thought about that long and hard. Clearly in a pure
>> > >> driver-model work we would pass a udevice, not a spi_slave.
>> > >>
>> > >> > struct spi_slave would be a prime candidate to have in
>> > >> > child->parentdata
>> > >> > (which should only be accessed by the parent IIUC)
>> > >>
>> > >> Yes, it is. In other words, 'struct spi_slave' is the child's parent
>> > >> data. The only reason that spi_xfer() is passed a spi_slave rather
>> > >> than a udevice is to maintain compatibility with the existing SPI
>> > >> system. I tried various other approachs, such as '#define spi_slave
>> > >> udevice' and the like. At some point we can change it, but it is
>> > >> really painful to have two completely different APIs co-existing in
>> > >> U-Boot.
>> > >>
>> > >> This way, driver model becomes a fairly soft transition, the amount of
>> > >> rewriting needed is reduced, and I think it is much more likely that
>> > >> people will use it. As an example, one of the reasons that the generic
>> > >> board change has been relatively painless is that people can just
>> > >> define CONFIG_SYS_GENERIC_BOARD in their board header file, and in
>> > >> most cases it just works. If we require people to rewrite things it
>> > >> will take longer, etc. There is already enough rewriting required in
>> > >> individual drivers to keep people busy. It will be a relatively easy
>> > >> change to do in the future if we want to.
>> > >
>> > > OK, that reason I understand.
>> > >
>> > > However, what you are doing now is limiting what parentdata a SPI bus
>> > > controller can use.
>> > > This means that each bus driver has its parentdata defined by what
>> > > uclass
>> > > it belongs to. Are you sure this won't be a limitation? I mean that you
>> > > could end up with uclasses that have the same calls but a slightly
>> > > different parentdata.
>> >
>> > No it is defined by the driver of the bus. Since it is possible to
>> > have (for example) 3 different USB drivers in a system, each can have
>> > its own idea of what child data it wants. I definitely agree that
>> > setting the parentdata by the child's uclass, or even the bus's uclass
>> > would be limiting. In the case of SPI I have elected to use struct
>> > spi_slave for reasons I explained earlier.
>>
>> OK, so you would have some bus uclasses that pass typecasted parentdata,
>> because it is the same for all bus drivers, and some that pass the childs
>> udevice*, because the parentdata type is not known beforehands? what happens
>> if suddenly you need to add a bus controller that needs a little more per-
>> child data than the existing ones?
>>
>> wouldn't it be better to unify this somehow?
>>
>> > > Yes, you could have a shared parentdata from the uclass (that makes
>> > > sense
>> > > for all controllers, because of the way the bus works), and a
>> > > controller-specific parentdata as an extension (read "void *private" at
>> > > the end of the parentdata structure)
>> > >
>> > >> Re passing both the bus and the device, really, what's the point? The
>> > >> only valid bus to pass to the function is dev->parent. If you pass
>> > >> anything else it is an error. It is redundant and therefore just
>> > >> introduces the possibility of error.
>> > >
>> > > Well, yes, sort of.
>> > >
>> > > Consider a bus with transparent bridges, like USB or PCI.
>> > >
>> > > If you were to represent these bridges as udevices, you would end up
>> > > with
>> > > something in between the actual devices and the bus controller, that
>> > > forwards requests with no or minimal change.
>> > > in case of USB specifically (IIRC), hubs are visible during
>> > > initialization, but when the device is up it has its own descriptor that
>> > > is used to
>> > > in case of PCI, the device address actually contains the bus number, but
>> > > the device itself doesn't really care about it, and is only used to
>> > > select which bus the command goes to.
>> > >
>> > > consider the following scenario:
>> > > ------     ----------     ---------     ---------     ------------
>> > >
>> > > |root| --- |ehci_hcd| --- |usb_hub| --- |usb_hub| --- |usb_device|
>> > >
>> > > ------     ----------     ---------     ---------     ------------
>> > >
>> > > If you were to flatten the bus, the udevice tree would not really
>> > > correspond to how the bus looks like in reality, and it might give you
>> > > some hurdles with initialization.
>> > >
>> > > note that in these cases you cannot pass a child udevice to the bus
>> > > controller as a part of the upcall, since it is not necessarily its
>> > > child. this is in contradiction to what I wrote previously, simply
>> > > because I didn't think hard enough to find these counter examples.
>> >
>> > I think you are referring to setting up a bus such that it becomes
>> > transparent. But even when it is, I'm not sure that driver model needs
>> > to rearrange its data structures to suit. Why would you flatten the
>> > bus? If we keep the data structures the same (strict parent-child
>> > relationships) then we always have a parent for the child, rather than
>> > trying to be too clever. Still, the child can obtain direct bus access
>> > through some mechanism delegated by the parent if we like. In that
>> > case there is even less reason to access the parent udevice IMO.
>> >
>> > I think I'm missing something from your example, so please let me know.
>>
>> ok, lets try some code examples
>>
>> lets say a usb_hub is implemented as a USB bus that itself sits in a USB
>> bus. lets say USB bus uclass has a function usb_bulk_msg()
>> usb_hub's implementation of such function would look like this:
>>
>> usb_bulk_msg(udevice *hub, usb_descriptor *child, ...)
>> {
>>       return usb_bulk_msg(hub->parent, child, ...);
>> }
>>
>> your USB device would then call
>> usb_bulk_msg(my_dev->parent, my_desc,...)
>>
>> this would work recursively through all hubs, and the end result you would
>> like is to call usb_bulk_msg() of the host bus controller, without changing
>> any of the parameters. (except the first, which is used for recursion)
>>
>> However, if you got rid of the first parameter, you would need a different
>> variable to control your recursion, or you would need the device descriptor
>> to hold pointer to the udevice of the host controller, which would
>> effectively flatten the bus.
>>
>> this is the case where I believe the best way to implement is to have a
>> shared parentdata based on parent uclass (the usb_descriptor in this case),
>> with a driver-specific extension, in case the bus driver need something
>> extra over what the "standard" is. the shared parentdata would then be used
>> as a device descriptor/address/whatever-you-call-that in the bus calls.
>
> I would like to add that passing the childs udevice* should work equally as
> well, if you relax the bus == child->parent assumption.

OK. But at least in the case of USB I think we should be able to
display the bus as a tree - hubs included. If we start re-stitching
children to different parents when a bus is active I worry that things
are going to get complicated.

Regards,
Simon