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

[Simon Glass]

Hi Pavel,

On 17 July 2014 12:01, Pavel Herrmann <morpheus.ibis at gmail.com> 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?

As mentioned spi_slave is something of a special case to retain some
sort of code compatibility. We use the same structure with an without
driver model.

Possibly we should unify this. I did think of allowing a child to
support both parent-defined data and parent-uclass-defined data. It is
an obvious step in some ways. However, I am concerned that the
complexity of all of this is getting out of hand. My approach so far
with driver model has been to build things as they are needed. I agree
that this requires a certain about of refactoring effort, but I am
much more worried about creating a framework that looks lovely but no
one understands :-)

>
>> > 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.
>
> One would also need to relax the semantics a bit, so that host bus controller
> can fill the parentdata for a udevice that is not its direct child.
>
> I'm not saying your code is wrong or anything, I just think you should have a
> look at a more complex bus like USB or PCI, and design the "bus uclass
> interface template" in a way that would support such a bus. Otherwise you
> might end up either reworking your simpler busses later, or having
> inconsistent bus uclass interfaces (which we did when we tried this the first
> time).
>
> I understand that API compatibility is an issue, and I agree that it is fine to
> cut some corners at this point, but we need to keep in mind to fix them once
> all the drivers have been converted.

Right, so you almost have me convinced that we should add a separate
parent parameter. However I would really like to see how USB hubs are
implemented before we do this. We are actually talking about a
different uclass, and there is no requirement that different uclasses
follow similar semantics. I agree that this is desirable trait and I'm
not really worried about the inefficiency of passing a redundant
parameter in the case of SPI since the cost is pretty small.

So how about we see how USB hubs are implemented in practice, and keep
this in mind? For that reason it would be useful to get USB going
quickly.

Regards,
Simon