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

[Pavel Herrmann]

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.

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