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

[Pavel Herrmann <morpheus.ibis@gmail.com>]

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

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