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

[Simon Glass]

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.

>
> 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.

Regards,
Simon