| Message ID | 1405385792-4469-7-git-send-email-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
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, ...) struct spi_slave would be a prime candidate to have in child->parentdata (which should only be accessed by the parent IIUC) regards Pavel Herrmann
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. 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. Regards, Simon
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. 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. regards Pavel Herrmann
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. > > 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
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
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@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. 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
Hi Pavel, On 17 July 2014 12:01, Pavel Herrmann <morpheus.ibis@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@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? 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
Hi Pavel, On 17 July 2014 12:29, Pavel Herrmann <morpheus.ibis@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@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. > > 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
Hi Simon, some nitpicking On 15.07.2014 02:56, 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> > --- > > common/exports.c | 4 +- > drivers/spi/Makefile | 4 + > drivers/spi/spi-uclass.c | 253 +++++++++++++++++++++++++++++++++++++++++++++++ > include/dm/uclass-id.h | 1 + > include/spi.h | 140 ++++++++++++++++++++++++++ > 5 files changed, 401 insertions(+), 1 deletion(-) > create mode 100644 drivers/spi/spi-uclass.c ... > --- /dev/null > +++ b/drivers/spi/spi-uclass.c > @@ -0,0 +1,253 @@ > +/* > + * Copyright (c) 2014 Google, Inc > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +#include <common.h> > +#include <dm.h> > +#include <errno.h> > +#include <fdtdec.h> > +#include <spi.h> > +#include <dm/device-internal.h> > +#include <dm/uclass-internal.h> > +#include <dm/root.h> > +#include <dm/lists.h> > + > +DECLARE_GLOBAL_DATA_PTR; > + > +static int spi_set_speed_mode(struct udevice *bus, int speed, int mode) > +{ > + struct dm_spi_ops *ops; > + int ret; > + > + ops = spi_get_ops(bus); > + if (ops->set_speed) > + ret = (*ops->set_speed)(bus, speed); ret = ops->set_speed(bus, speed); > + else > + ret = -EINVAL; > + if (ret) { > + printf("Cannot set speed (err=%d)\n", ret); > + return ret; > + } > + > + ops = spi_get_ops(bus); redundant assignment > + if (ops->set_mode) > + ret = (*ops->set_mode)(bus, mode); ret = ops->set_mode(bus, mode); > + else > + ret = -EINVAL; > + if (ret) { > + printf("Cannot set mode (err=%d)\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +int spi_claim_bus(struct spi_slave *slave) > +{ > + struct udevice *dev = slave->dev; > + struct udevice *bus = dev->parent; > + struct dm_spi_ops *ops = spi_get_ops(bus); > + struct dm_spi_bus *spi = bus->uclass_priv; > + int speed; > + int ret; > + > + speed = slave->max_hz; > + if (spi->max_hz) { > + if (speed) > + speed = min(speed, spi->max_hz); > + else > + speed = spi->max_hz; > + } > + if (!speed) > + speed = 100000; > + ret = spi_set_speed_mode(bus, speed, slave->mode); > + if (ret) > + return ret; > + > + return ops->claim_bus ? ops->claim_bus(bus) : 0; > +} > + > +void spi_release_bus(struct spi_slave *slave) > +{ > + struct udevice *dev = slave->dev; > + struct udevice *bus = dev->parent; > + struct dm_spi_ops *ops = spi_get_ops(bus); > + > + if (ops->release_bus) > + spi_get_ops(bus)->release_bus(bus); ops->release_bus(bus); > +} > + > +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; > + > + if (bus->uclass->uc_drv->id != UCLASS_SPI) > + return -EOPNOTSUPP; > + > + return spi_get_ops(bus)->xfer(bus, dev, bitlen, dout, din, flags); > +} > + > +int spi_post_bind(struct udevice *dev) > +{ > + /* Scan the bus for devices */ > + return dm_scan_fdt_node(dev, gd->fdt_blob, dev->of_offset, false); > +} > + > +int spi_post_probe(struct udevice *dev) > +{ > + struct dm_spi_bus *spi = dev->uclass_priv; > + > + spi->max_hz = fdtdec_get_int(gd->fdt_blob, dev->of_offset, > + "spi-max-frequency", 0); > + > + return 0; > +} > + > +int spi_bind_device(struct udevice *bus, int cs, const char *drv_name, > + const char *dev_name, struct udevice **slavep) > +{ > + struct driver *drv; > + int ret; > + > + drv = lists_driver_lookup_name(drv_name); > + if (!drv) { > + puts("Cannot find spi_flash_std driver\n"); > + return -ENOENT; > + } > + ret = device_bind(bus, drv, dev_name, NULL, -1, slavep); > + if (ret) { > + printf("Cannot create device named '%s' (err=%d)\n", > + dev_name, ret); > + return ret; > + } > + (*slavep)->req_seq = cs; > + > + return 0; > +} > + > +int spi_find_bus_and_cs(int busnum, int cs, struct udevice **busp, > + struct udevice **devp) > +{ > + struct udevice *bus, *dev; > + int ret; > + > + ret = uclass_find_device_by_seq(UCLASS_SPI, busnum, false, &bus); > + if (ret) > + return ret; > + ret = device_find_child_by_seq(bus, cs, false, &dev); > + if (ret) > + return ret; > + *busp = bus; > + *devp = dev; > + > + return ret; > +} > + > +int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, > + const char *drv_name, const char *dev_name, > + struct udevice **devp, struct spi_slave **slavep) > +{ > + struct udevice *bus, *dev; > + int ret; > + > + ret = uclass_get_device_by_seq(UCLASS_SPI, busnum, &bus); > + if (ret) { > + printf("Invalid bus %d (err=%d)\n", busnum, ret); > + return ret; > + } > + ret = device_get_child_by_seq(bus, cs, &dev); > + > + /** > + * If there is no such device, create one automatically. This means > + * that we don't need a device tree node or platform data for the > + * SPI flash chip - we will bind to the correct driver. > + */ > + if (ret == -ENODEV && drv_name) { > + ret = spi_bind_device(bus, cs, drv_name, dev_name, &dev); > + if (ret) > + return ret; > + } > + if (ret) { > + printf("Invalid chip select %d:%d (err=%d)\n", busnum, cs, > + ret); > + return ret; > + } > + > + ret = spi_set_speed_mode(bus, speed, mode); > + if (ret) > + return ret; > + > + *devp = bus; > + *slavep = dev_get_parentdata(dev); > + > + return 0; > +} > + > +/* Compatibility function - to be removed */ > +struct spi_slave *spi_setup_slave_fdt(const void *blob, int node, > + int bus_node) > +{ > + struct udevice *bus, *dev; > + int ret; > + > + ret = uclass_get_device_by_of_offset(UCLASS_SPI, bus_node, &bus); > + if (ret) > + return NULL; > + ret = device_get_child_by_of_offset(bus, node, &dev); > + if (ret) > + return NULL; > + return dev_get_parentdata(dev); > +} > + > +/* Compatibility function - to be removed */ > +struct spi_slave *spi_setup_slave(unsigned int busnum, unsigned int cs, > + unsigned int speed, unsigned int mode) > +{ > + struct spi_slave *slave; > + struct udevice *dev; > + int ret; > + > + ret = spi_get_bus_and_cs(busnum, cs, speed, mode, NULL, 0, &dev, > + &slave); > + if (ret) > + return NULL; > + > + return slave; > +} > + > +void spi_free_slave(struct spi_slave *slave) > +{ > + device_remove(slave->dev); > + slave->dev = NULL; > +} > + > +int spi_ofdata_to_platdata(const void *blob, int node, > + struct spi_slave *spi) > +{ > + int mode = 0; > + > + spi->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency", 0); > + if (fdtdec_get_bool(blob, node, "spi-cpol")) > + mode |= SPI_CPOL; > + if (fdtdec_get_bool(blob, node, "spi-cpha")) > + mode |= SPI_CPHA; > + if (fdtdec_get_bool(blob, node, "spi-cs-high")) > + mode |= SPI_CS_HIGH; > + if (fdtdec_get_bool(blob, node, "spi-half-duplex")) > + mode |= SPI_PREAMBLE; > + spi->mode = mode; > + > + return 0; > +} > + > +UCLASS_DRIVER(spi) = { > + .id = UCLASS_SPI, > + .name = "spi", > + .post_bind = spi_post_bind, > + .post_probe = spi_post_probe, > + .per_device_auto_alloc_size = sizeof(struct dm_spi_bus), > +};
On 15 July 2014 06:26, Simon Glass <sjg@chromium.org> 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> > --- > > common/exports.c | 4 +- > drivers/spi/Makefile | 4 + > drivers/spi/spi-uclass.c | 253 +++++++++++++++++++++++++++++++++++++++++++++++ > include/dm/uclass-id.h | 1 + > include/spi.h | 140 ++++++++++++++++++++++++++ > 5 files changed, 401 insertions(+), 1 deletion(-) > create mode 100644 drivers/spi/spi-uclass.c > > diff --git a/common/exports.c b/common/exports.c > index b97ca48..88fcfc8 100644 > --- a/common/exports.c > +++ b/common/exports.c > @@ -27,10 +27,12 @@ unsigned long get_version(void) > # define i2c_write dummy > # define i2c_read dummy > #endif > -#ifndef CONFIG_CMD_SPI > +#if !defined(CONFIG_CMD_SPI) || defined(CONFIG_DM_SPI) > # define spi_init dummy > # define spi_setup_slave dummy > # define spi_free_slave dummy > +#endif > +#ifndef CONFIG_CMD_SPI > # define spi_claim_bus dummy > # define spi_release_bus dummy > # define spi_xfer dummy > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile > index f02c35a..d1f1dd0 100644 > --- a/drivers/spi/Makefile > +++ b/drivers/spi/Makefile > @@ -6,7 +6,11 @@ > # > > # There are many options which enable SPI, so make this library available > +ifdef CONFIG_DM_SPI > +obj-y += spi-uclass.o > +else > obj-y += spi.o > +endif > > obj-$(CONFIG_EP93XX_SPI) += ep93xx_spi.o > obj-$(CONFIG_ALTERA_SPI) += altera_spi.o > diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c > new file mode 100644 > index 0000000..4057bce > --- /dev/null > +++ b/drivers/spi/spi-uclass.c > @@ -0,0 +1,253 @@ > +/* > + * Copyright (c) 2014 Google, Inc > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +#include <common.h> > +#include <dm.h> > +#include <errno.h> > +#include <fdtdec.h> > +#include <spi.h> > +#include <dm/device-internal.h> > +#include <dm/uclass-internal.h> > +#include <dm/root.h> > +#include <dm/lists.h> > + > +DECLARE_GLOBAL_DATA_PTR; > + > +static int spi_set_speed_mode(struct udevice *bus, int speed, int mode) > +{ > + struct dm_spi_ops *ops; > + int ret; > + > + ops = spi_get_ops(bus); > + if (ops->set_speed) > + ret = (*ops->set_speed)(bus, speed); > + else > + ret = -EINVAL; > + if (ret) { > + printf("Cannot set speed (err=%d)\n", ret); > + return ret; > + } > + > + ops = spi_get_ops(bus); > + if (ops->set_mode) > + ret = (*ops->set_mode)(bus, mode); > + else > + ret = -EINVAL; > + if (ret) { > + printf("Cannot set mode (err=%d)\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +int spi_claim_bus(struct spi_slave *slave) > +{ > + struct udevice *dev = slave->dev; > + struct udevice *bus = dev->parent; > + struct dm_spi_ops *ops = spi_get_ops(bus); > + struct dm_spi_bus *spi = bus->uclass_priv; > + int speed; > + int ret; > + > + speed = slave->max_hz; > + if (spi->max_hz) { > + if (speed) > + speed = min(speed, spi->max_hz); > + else > + speed = spi->max_hz; > + } > + if (!speed) > + speed = 100000; > + ret = spi_set_speed_mode(bus, speed, slave->mode); > + if (ret) > + return ret; > + > + return ops->claim_bus ? ops->claim_bus(bus) : 0; > +} > + > +void spi_release_bus(struct spi_slave *slave) > +{ > + struct udevice *dev = slave->dev; > + struct udevice *bus = dev->parent; > + struct dm_spi_ops *ops = spi_get_ops(bus); > + > + if (ops->release_bus) > + spi_get_ops(bus)->release_bus(bus); > +} > + > +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; > + > + if (bus->uclass->uc_drv->id != UCLASS_SPI) > + return -EOPNOTSUPP; > + > + return spi_get_ops(bus)->xfer(bus, dev, bitlen, dout, din, flags); > +} Cleared all these calls, as individual drivers/spi* will setup their ops and fills the priv data. So this uclass will call accordingly - correct? add if I'm missing anything. > + > +int spi_post_bind(struct udevice *dev) > +{ > + /* Scan the bus for devices */ > + return dm_scan_fdt_node(dev, gd->fdt_blob, dev->of_offset, false); > +} > + > +int spi_post_probe(struct udevice *dev) > +{ > + struct dm_spi_bus *spi = dev->uclass_priv; > + > + spi->max_hz = fdtdec_get_int(gd->fdt_blob, dev->of_offset, > + "spi-max-frequency", 0); > + > + return 0; > +} Anyway each spi driver will fills the data from dtb - like spi-max-frequency, why the spi core (uclass) will do the same. Can't we reuse the spi_slave, why dm_spi_bus? > + > +int spi_bind_device(struct udevice *bus, int cs, const char *drv_name, > + const char *dev_name, struct udevice **slavep) > +{ > + struct driver *drv; > + int ret; > + > + drv = lists_driver_lookup_name(drv_name); > + if (!drv) { > + puts("Cannot find spi_flash_std driver\n"); > + return -ENOENT; > + } > + ret = device_bind(bus, drv, dev_name, NULL, -1, slavep); > + if (ret) { > + printf("Cannot create device named '%s' (err=%d)\n", > + dev_name, ret); > + return ret; > + } > + (*slavep)->req_seq = cs; > + > + return 0; > +} > + > +int spi_find_bus_and_cs(int busnum, int cs, struct udevice **busp, > + struct udevice **devp) > +{ > + struct udevice *bus, *dev; > + int ret; > + > + ret = uclass_find_device_by_seq(UCLASS_SPI, busnum, false, &bus); > + if (ret) > + return ret; > + ret = device_find_child_by_seq(bus, cs, false, &dev); > + if (ret) > + return ret; > + *busp = bus; > + *devp = dev; > + > + return ret; > +} > + > +int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, > + const char *drv_name, const char *dev_name, > + struct udevice **devp, struct spi_slave **slavep) > +{ > + struct udevice *bus, *dev; > + int ret; > + > + ret = uclass_get_device_by_seq(UCLASS_SPI, busnum, &bus); > + if (ret) { > + printf("Invalid bus %d (err=%d)\n", busnum, ret); > + return ret; > + } > + ret = device_get_child_by_seq(bus, cs, &dev); > + > + /** > + * If there is no such device, create one automatically. This means > + * that we don't need a device tree node or platform data for the > + * SPI flash chip - we will bind to the correct driver. > + */ > + if (ret == -ENODEV && drv_name) { > + ret = spi_bind_device(bus, cs, drv_name, dev_name, &dev); > + if (ret) > + return ret; > + } > + if (ret) { > + printf("Invalid chip select %d:%d (err=%d)\n", busnum, cs, > + ret); > + return ret; > + } > + > + ret = spi_set_speed_mode(bus, speed, mode); > + if (ret) > + return ret; > + > + *devp = bus; > + *slavep = dev_get_parentdata(dev); > + > + return 0; > +} I guess this is one of the replacement for spi_setup_slave() in spi drivers, so how bus and cs member will populate to drivers. Usually bus and cs members from spi_slave will populate to drivers through "sf probe" so spi_cs_activate will set the respective chip-select and along with reg_base in driver select through bus value. spi_setup_slave() { ... zslave->base = get_zynq_spi_base(bus); ... } How these will handling with dm? > + > +/* Compatibility function - to be removed */ > +struct spi_slave *spi_setup_slave_fdt(const void *blob, int node, > + int bus_node) > +{ > + struct udevice *bus, *dev; > + int ret; > + > + ret = uclass_get_device_by_of_offset(UCLASS_SPI, bus_node, &bus); > + if (ret) > + return NULL; > + ret = device_get_child_by_of_offset(bus, node, &dev); > + if (ret) > + return NULL; > + return dev_get_parentdata(dev); > +} > + > +/* Compatibility function - to be removed */ > +struct spi_slave *spi_setup_slave(unsigned int busnum, unsigned int cs, > + unsigned int speed, unsigned int mode) > +{ > + struct spi_slave *slave; > + struct udevice *dev; > + int ret; > + > + ret = spi_get_bus_and_cs(busnum, cs, speed, mode, NULL, 0, &dev, > + &slave); > + if (ret) > + return NULL; > + > + return slave; > +} > + > +void spi_free_slave(struct spi_slave *slave) > +{ > + device_remove(slave->dev); > + slave->dev = NULL; > +} > + > +int spi_ofdata_to_platdata(const void *blob, int node, > + struct spi_slave *spi) > +{ > + int mode = 0; > + > + spi->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency", 0); > + if (fdtdec_get_bool(blob, node, "spi-cpol")) > + mode |= SPI_CPOL; > + if (fdtdec_get_bool(blob, node, "spi-cpha")) > + mode |= SPI_CPHA; > + if (fdtdec_get_bool(blob, node, "spi-cs-high")) > + mode |= SPI_CS_HIGH; > + if (fdtdec_get_bool(blob, node, "spi-half-duplex")) > + mode |= SPI_PREAMBLE; > + spi->mode = mode; > + > + return 0; > +} > + > +UCLASS_DRIVER(spi) = { > + .id = UCLASS_SPI, > + .name = "spi", > + .post_bind = spi_post_bind, > + .post_probe = spi_post_probe, > + .per_device_auto_alloc_size = sizeof(struct dm_spi_bus), > +}; > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h > index 7f0e37b..8207483 100644 > --- a/include/dm/uclass-id.h > +++ b/include/dm/uclass-id.h > @@ -22,6 +22,7 @@ enum uclass_id { > /* U-Boot uclasses start here */ > UCLASS_GPIO, /* Bank of general-purpose I/O pins */ > UCLASS_SERIAL, /* Serial UART */ > + UCLASS_SPI, /* SPI bus */ > > UCLASS_COUNT, > UCLASS_INVALID = -1, > diff --git a/include/spi.h b/include/spi.h > index b673be2..b5e9347 100644 > --- a/include/spi.h > +++ b/include/spi.h > @@ -54,11 +54,19 @@ > > #define SPI_DEFAULT_WORDLEN 8 > > +#ifdef CONFIG_DM_SPI > +struct dm_spi_bus { > + uint max_hz; > +}; > + > +#endif /* CONFIG_DM_SPI */ > + No idea why this should require, max_hz is already a part of spi_slave and handling of these like getting from dtb and process and fill on priv data should be part of individual drivers and filling spi_slave member should be part of uclass - based on my understanding. Any comments? > /** > * struct spi_slave - Representation of a SPI slave > * > * Drivers are expected to extend this with controller-specific data. > * > + * @dev: SPI slave device Missing comments for max_hz and mode > * @bus: ID of the bus that the slave is attached to. > * @cs: ID of the chip select connected to the slave. > * @op_mode_rx: SPI RX operation mode. > @@ -71,8 +79,14 @@ > * @flags: Indication of SPI flags. > */ > struct spi_slave { > +#ifdef CONFIG_DM_SPI > + struct udevice *dev; /* struct spi_slave is dev->parentdata */ > + uint max_hz; > + uint mode; > +#else > unsigned int bus; > unsigned int cs; > +#endif > u8 op_mode_rx; > u8 op_mode_tx; > unsigned int wordlen; > @@ -220,6 +234,8 @@ int spi_set_wordlen(struct spi_slave *slave, unsigned int wordlen); > int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout, > void *din, unsigned long flags); > > +#ifndef CONFIG_DM_SPI > + > /** > * Determine if a SPI chipselect is valid. > * This function is provided by the board if the low-level SPI driver > @@ -255,6 +271,7 @@ void spi_cs_deactivate(struct spi_slave *slave); > * @hz: The transfer speed > */ > void spi_set_speed(struct spi_slave *slave, uint hz); > +#endif > > /** > * Write 8 bits, then read 8 bits. > @@ -305,4 +322,127 @@ struct spi_slave *spi_setup_slave_fdt(const void *blob, int slave_node, > struct spi_slave *spi_base_setup_slave_fdt(const void *blob, int busnum, > int node); > > +#ifdef CONFIG_DM_SPI > + > +/** > + * struct struct dm_spi_ops - Driver model SPI operations > + * > + * The uclass interface is implemented by all SPI devices which use > + * driver model. > + */ > +struct dm_spi_ops { > + /** > + * Claim the bus and prepare it for communication. > + * > + * The device privided is the slave device. It's parent controller > + * will be used to provide the communication. > + * > + * This must be called before doing any transfers with a SPI slave. It > + * will enable and initialize any SPI hardware as necessary, and make > + * sure that the SCK line is in the correct idle state. It is not > + * allowed to claim the same bus for several slaves without releasing > + * the bus in between. > + * > + * @device: The SPI slave > + * > + * Returns: 0 if the bus was claimed successfully, or a negative value > + * if it wasn't. > + */ > + int (*claim_bus)(struct udevice *device); > + > + /** > + * Release the SPI bus > + * > + * This must be called once for every call to spi_claim_bus() after > + * all transfers have finished. It may disable any SPI hardware as > + * appropriate. > + * > + * @device: The SPI slave > + */ > + int (*release_bus)(struct udevice *device); > + > + /** > + * Set the word length for SPI transactions > + * > + * Set the word length (number of bits per word) for SPI transactions. > + * > + * @device: The SPI slave > + * @wordlen: The number of bits in a word > + * > + * Returns: 0 on success, -ve on failure. > + */ > + int (*set_wordlen)(struct udevice *deiuce, unsigned int wordlen); > + > + /** > + * SPI transfer > + * > + * This writes "bitlen" bits out the SPI MOSI port and simultaneously > + * clocks "bitlen" bits in the SPI MISO port. That's just the way SPI > + * works. > + * > + * The source of the outgoing bits is the "dout" parameter and the > + * destination of the input bits is the "din" parameter. Note that > + * "dout" and "din" can point to the same memory location, in which > + * case the input data overwrites the output data (since both are > + * buffered by temporary variables, this is OK). > + * > + * spi_xfer() interface: > + * @device: The SPI bus which will be sending/receiving the data. > + * @slave: The slave device to communicate with > + * @bitlen: How many bits to write and read. > + * @dout: Pointer to a string of bits to send out. The bits are > + * held in a byte array and are sent MSB first. > + * @din: Pointer to a string of bits that will be filled in. > + * @flags: A bitwise combination of SPI_XFER_* flags. > + * > + * Returns: 0 on success, not -1 on failure > + */ > + int (*xfer)(struct udevice *device, struct udevice *slave, > + unsigned int bitlen, const void *dout, void *din, > + unsigned long flags); > + > + /** > + * Set transfer speed. > + * This sets a new speed to be applied for next spi_xfer(). > + * @slave: The SPI slave > + * @hz: The transfer speed > + * @return 0 if OK, -ve on error > + */ > + int (*set_speed)(struct udevice *device, uint hz); > + > + /** > + * Set the SPI mode/flags > + * > + * It is unclear if we want to set speed and mode together instead > + * of separately. > + * > + * @slave: The SPI slave > + * @mode: Requested SPI mode (SPI_... flags) > + * @return 0 if OK, -ve on error > + */ > + int (*set_mode)(struct udevice *device, uint mode); > +}; > + > +int spi_find_bus_and_cs(int busnum, int cs, struct udevice **busp, > + struct udevice **devp); > + > +int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, > + const char *drv_name, const char *dev_name, > + struct udevice **devp, struct spi_slave **slavep); > + > +int spi_bind_device(struct udevice *bus, int cs, const char *drv_name, > + const char *dev_name, struct udevice **slavep); > + > +int spi_ofdata_to_platdata(const void *blob, int node, > + struct spi_slave *spi); > + > +struct sandbox_state; > +int sandbox_spi_get_emul(struct sandbox_state *state, > + struct udevice *bus, struct udevice *slave, > + struct udevice **emulp); > + > +/* Access the serial operations for a device */ > +#define spi_get_ops(dev) ((struct dm_spi_ops *)(dev)->driver->ops) > +#endif /* CONFIG_DM_SPI */ > + > #endif /* _SPI_H_ */ > -- > 2.0.0.526.g5318336 > thanks!
Hi Jagan, On 28 August 2014 02:58, Jagan Teki <jagannadh.teki@gmail.com> wrote: > On 15 July 2014 06:26, Simon Glass <sjg@chromium.org> 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> >> --- >> >> common/exports.c | 4 +- >> drivers/spi/Makefile | 4 + >> drivers/spi/spi-uclass.c | 253 +++++++++++++++++++++++++++++++++++++++++++++++ >> include/dm/uclass-id.h | 1 + >> include/spi.h | 140 ++++++++++++++++++++++++++ >> 5 files changed, 401 insertions(+), 1 deletion(-) >> create mode 100644 drivers/spi/spi-uclass.c >> >> diff --git a/common/exports.c b/common/exports.c >> index b97ca48..88fcfc8 100644 >> --- a/common/exports.c >> +++ b/common/exports.c >> @@ -27,10 +27,12 @@ unsigned long get_version(void) >> # define i2c_write dummy >> # define i2c_read dummy >> #endif >> -#ifndef CONFIG_CMD_SPI >> +#if !defined(CONFIG_CMD_SPI) || defined(CONFIG_DM_SPI) >> # define spi_init dummy >> # define spi_setup_slave dummy >> # define spi_free_slave dummy >> +#endif >> +#ifndef CONFIG_CMD_SPI >> # define spi_claim_bus dummy >> # define spi_release_bus dummy >> # define spi_xfer dummy >> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile >> index f02c35a..d1f1dd0 100644 >> --- a/drivers/spi/Makefile >> +++ b/drivers/spi/Makefile >> @@ -6,7 +6,11 @@ >> # >> >> # There are many options which enable SPI, so make this library available >> +ifdef CONFIG_DM_SPI >> +obj-y += spi-uclass.o >> +else >> obj-y += spi.o >> +endif >> >> obj-$(CONFIG_EP93XX_SPI) += ep93xx_spi.o >> obj-$(CONFIG_ALTERA_SPI) += altera_spi.o >> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c >> new file mode 100644 >> index 0000000..4057bce >> --- /dev/null >> +++ b/drivers/spi/spi-uclass.c >> @@ -0,0 +1,253 @@ >> +/* >> + * Copyright (c) 2014 Google, Inc >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#include <common.h> >> +#include <dm.h> >> +#include <errno.h> >> +#include <fdtdec.h> >> +#include <spi.h> >> +#include <dm/device-internal.h> >> +#include <dm/uclass-internal.h> >> +#include <dm/root.h> >> +#include <dm/lists.h> >> + >> +DECLARE_GLOBAL_DATA_PTR; >> + >> +static int spi_set_speed_mode(struct udevice *bus, int speed, int mode) >> +{ >> + struct dm_spi_ops *ops; >> + int ret; >> + >> + ops = spi_get_ops(bus); >> + if (ops->set_speed) >> + ret = (*ops->set_speed)(bus, speed); >> + else >> + ret = -EINVAL; >> + if (ret) { >> + printf("Cannot set speed (err=%d)\n", ret); >> + return ret; >> + } >> + >> + ops = spi_get_ops(bus); >> + if (ops->set_mode) >> + ret = (*ops->set_mode)(bus, mode); >> + else >> + ret = -EINVAL; >> + if (ret) { >> + printf("Cannot set mode (err=%d)\n", ret); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +int spi_claim_bus(struct spi_slave *slave) >> +{ >> + struct udevice *dev = slave->dev; >> + struct udevice *bus = dev->parent; >> + struct dm_spi_ops *ops = spi_get_ops(bus); >> + struct dm_spi_bus *spi = bus->uclass_priv; >> + int speed; >> + int ret; >> + >> + speed = slave->max_hz; >> + if (spi->max_hz) { >> + if (speed) >> + speed = min(speed, spi->max_hz); >> + else >> + speed = spi->max_hz; >> + } >> + if (!speed) >> + speed = 100000; >> + ret = spi_set_speed_mode(bus, speed, slave->mode); >> + if (ret) >> + return ret; >> + >> + return ops->claim_bus ? ops->claim_bus(bus) : 0; >> +} >> + >> +void spi_release_bus(struct spi_slave *slave) >> +{ >> + struct udevice *dev = slave->dev; >> + struct udevice *bus = dev->parent; >> + struct dm_spi_ops *ops = spi_get_ops(bus); >> + >> + if (ops->release_bus) >> + spi_get_ops(bus)->release_bus(bus); >> +} >> + >> +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; >> + >> + if (bus->uclass->uc_drv->id != UCLASS_SPI) >> + return -EOPNOTSUPP; >> + >> + return spi_get_ops(bus)->xfer(bus, dev, bitlen, dout, din, flags); >> +} > > Cleared all these calls, as individual drivers/spi* will setup their > ops and fills > the priv data. So this uclass will call accordingly - correct? add if > I'm missing anything. Yes so far as I understand you, that is correct. > >> + >> +int spi_post_bind(struct udevice *dev) >> +{ >> + /* Scan the bus for devices */ >> + return dm_scan_fdt_node(dev, gd->fdt_blob, dev->of_offset, false); >> +} >> + >> +int spi_post_probe(struct udevice *dev) >> +{ >> + struct dm_spi_bus *spi = dev->uclass_priv; >> + >> + spi->max_hz = fdtdec_get_int(gd->fdt_blob, dev->of_offset, >> + "spi-max-frequency", 0); >> + >> + return 0; >> +} > > Anyway each spi driver will fills the data from dtb - like > spi-max-frequency, why the spi > core (uclass) will do the same. Because the overall SPI peripheral has a maximum speed too. We should allow it to run at a nominal speed, but slow down for particular peripherals if needed. > > Can't we reuse the spi_slave, why dm_spi_bus? > >> + >> +int spi_bind_device(struct udevice *bus, int cs, const char *drv_name, >> + const char *dev_name, struct udevice **slavep) >> +{ >> + struct driver *drv; >> + int ret; >> + >> + drv = lists_driver_lookup_name(drv_name); >> + if (!drv) { >> + puts("Cannot find spi_flash_std driver\n"); >> + return -ENOENT; >> + } >> + ret = device_bind(bus, drv, dev_name, NULL, -1, slavep); >> + if (ret) { >> + printf("Cannot create device named '%s' (err=%d)\n", >> + dev_name, ret); >> + return ret; >> + } >> + (*slavep)->req_seq = cs; >> + >> + return 0; >> +} >> + >> +int spi_find_bus_and_cs(int busnum, int cs, struct udevice **busp, >> + struct udevice **devp) >> +{ >> + struct udevice *bus, *dev; >> + int ret; >> + >> + ret = uclass_find_device_by_seq(UCLASS_SPI, busnum, false, &bus); >> + if (ret) >> + return ret; >> + ret = device_find_child_by_seq(bus, cs, false, &dev); >> + if (ret) >> + return ret; >> + *busp = bus; >> + *devp = dev; >> + >> + return ret; >> +} >> + >> +int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, >> + const char *drv_name, const char *dev_name, >> + struct udevice **devp, struct spi_slave **slavep) >> +{ >> + struct udevice *bus, *dev; >> + int ret; >> + >> + ret = uclass_get_device_by_seq(UCLASS_SPI, busnum, &bus); >> + if (ret) { >> + printf("Invalid bus %d (err=%d)\n", busnum, ret); >> + return ret; >> + } >> + ret = device_get_child_by_seq(bus, cs, &dev); >> + >> + /** >> + * If there is no such device, create one automatically. This means >> + * that we don't need a device tree node or platform data for the >> + * SPI flash chip - we will bind to the correct driver. >> + */ >> + if (ret == -ENODEV && drv_name) { >> + ret = spi_bind_device(bus, cs, drv_name, dev_name, &dev); >> + if (ret) >> + return ret; >> + } >> + if (ret) { >> + printf("Invalid chip select %d:%d (err=%d)\n", busnum, cs, >> + ret); >> + return ret; >> + } >> + >> + ret = spi_set_speed_mode(bus, speed, mode); >> + if (ret) >> + return ret; >> + >> + *devp = bus; >> + *slavep = dev_get_parentdata(dev); >> + >> + return 0; >> +} > > I guess this is one of the replacement for spi_setup_slave() in spi drivers, so > how bus and cs member will populate to drivers. > > Usually bus and cs members from spi_slave will populate to drivers through > "sf probe" so spi_cs_activate will set the respective chip-select and > along with > reg_base in driver select through bus value. > > spi_setup_slave() { > ... > zslave->base = get_zynq_spi_base(bus); > ... > } > > How these will handling with dm? The base should be in the device tree, or in platform data. The bus number should not be visible to the driver. > >> + >> +/* Compatibility function - to be removed */ >> +struct spi_slave *spi_setup_slave_fdt(const void *blob, int node, >> + int bus_node) >> +{ >> + struct udevice *bus, *dev; >> + int ret; >> + >> + ret = uclass_get_device_by_of_offset(UCLASS_SPI, bus_node, &bus); >> + if (ret) >> + return NULL; >> + ret = device_get_child_by_of_offset(bus, node, &dev); >> + if (ret) >> + return NULL; >> + return dev_get_parentdata(dev); >> +} >> + >> +/* Compatibility function - to be removed */ >> +struct spi_slave *spi_setup_slave(unsigned int busnum, unsigned int cs, >> + unsigned int speed, unsigned int mode) >> +{ >> + struct spi_slave *slave; >> + struct udevice *dev; >> + int ret; >> + >> + ret = spi_get_bus_and_cs(busnum, cs, speed, mode, NULL, 0, &dev, >> + &slave); >> + if (ret) >> + return NULL; >> + >> + return slave; >> +} >> + >> +void spi_free_slave(struct spi_slave *slave) >> +{ >> + device_remove(slave->dev); >> + slave->dev = NULL; >> +} >> + >> +int spi_ofdata_to_platdata(const void *blob, int node, >> + struct spi_slave *spi) >> +{ >> + int mode = 0; >> + >> + spi->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency", 0); >> + if (fdtdec_get_bool(blob, node, "spi-cpol")) >> + mode |= SPI_CPOL; >> + if (fdtdec_get_bool(blob, node, "spi-cpha")) >> + mode |= SPI_CPHA; >> + if (fdtdec_get_bool(blob, node, "spi-cs-high")) >> + mode |= SPI_CS_HIGH; >> + if (fdtdec_get_bool(blob, node, "spi-half-duplex")) >> + mode |= SPI_PREAMBLE; >> + spi->mode = mode; >> + >> + return 0; >> +} >> + >> +UCLASS_DRIVER(spi) = { >> + .id = UCLASS_SPI, >> + .name = "spi", >> + .post_bind = spi_post_bind, >> + .post_probe = spi_post_probe, >> + .per_device_auto_alloc_size = sizeof(struct dm_spi_bus), >> +}; >> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h >> index 7f0e37b..8207483 100644 >> --- a/include/dm/uclass-id.h >> +++ b/include/dm/uclass-id.h >> @@ -22,6 +22,7 @@ enum uclass_id { >> /* U-Boot uclasses start here */ >> UCLASS_GPIO, /* Bank of general-purpose I/O pins */ >> UCLASS_SERIAL, /* Serial UART */ >> + UCLASS_SPI, /* SPI bus */ >> >> UCLASS_COUNT, >> UCLASS_INVALID = -1, >> diff --git a/include/spi.h b/include/spi.h >> index b673be2..b5e9347 100644 >> --- a/include/spi.h >> +++ b/include/spi.h >> @@ -54,11 +54,19 @@ >> >> #define SPI_DEFAULT_WORDLEN 8 >> >> +#ifdef CONFIG_DM_SPI >> +struct dm_spi_bus { >> + uint max_hz; >> +}; >> + >> +#endif /* CONFIG_DM_SPI */ >> + > > No idea why this should require, max_hz is already a part of spi_slave > and handling of these like getting from dtb and process and fill on priv > data should be part of individual drivers and filling spi_slave member should > be part of uclass - based on my understanding. > > Any comments? See above - the bus has its own maximum speed in many cases. > > >> /** >> * struct spi_slave - Representation of a SPI slave >> * >> * Drivers are expected to extend this with controller-specific data. >> * >> + * @dev: SPI slave device > > Missing comments for max_hz and mode > >> * @bus: ID of the bus that the slave is attached to. >> * @cs: ID of the chip select connected to the slave. >> * @op_mode_rx: SPI RX operation mode. >> @@ -71,8 +79,14 @@ >> * @flags: Indication of SPI flags. >> */ >> struct spi_slave { >> +#ifdef CONFIG_DM_SPI >> + struct udevice *dev; /* struct spi_slave is dev->parentdata */ >> + uint max_hz; >> + uint mode; >> +#else >> unsigned int bus; >> unsigned int cs; >> +#endif >> u8 op_mode_rx; >> u8 op_mode_tx; >> unsigned int wordlen; >> @@ -220,6 +234,8 @@ int spi_set_wordlen(struct spi_slave *slave, unsigned int wordlen); >> int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout, >> void *din, unsigned long flags); >> >> +#ifndef CONFIG_DM_SPI >> + >> /** >> * Determine if a SPI chipselect is valid. >> * This function is provided by the board if the low-level SPI driver >> @@ -255,6 +271,7 @@ void spi_cs_deactivate(struct spi_slave *slave); >> * @hz: The transfer speed >> */ >> void spi_set_speed(struct spi_slave *slave, uint hz); >> +#endif >> >> /** >> * Write 8 bits, then read 8 bits. >> @@ -305,4 +322,127 @@ struct spi_slave *spi_setup_slave_fdt(const void *blob, int slave_node, >> struct spi_slave *spi_base_setup_slave_fdt(const void *blob, int busnum, >> int node); >> >> +#ifdef CONFIG_DM_SPI >> + >> +/** >> + * struct struct dm_spi_ops - Driver model SPI operations >> + * >> + * The uclass interface is implemented by all SPI devices which use >> + * driver model. >> + */ >> +struct dm_spi_ops { >> + /** >> + * Claim the bus and prepare it for communication. >> + * >> + * The device privided is the slave device. It's parent controller >> + * will be used to provide the communication. >> + * >> + * This must be called before doing any transfers with a SPI slave. It >> + * will enable and initialize any SPI hardware as necessary, and make >> + * sure that the SCK line is in the correct idle state. It is not >> + * allowed to claim the same bus for several slaves without releasing >> + * the bus in between. >> + * >> + * @device: The SPI slave >> + * >> + * Returns: 0 if the bus was claimed successfully, or a negative value >> + * if it wasn't. >> + */ >> + int (*claim_bus)(struct udevice *device); >> + >> + /** >> + * Release the SPI bus >> + * >> + * This must be called once for every call to spi_claim_bus() after >> + * all transfers have finished. It may disable any SPI hardware as >> + * appropriate. >> + * >> + * @device: The SPI slave >> + */ >> + int (*release_bus)(struct udevice *device); >> + >> + /** >> + * Set the word length for SPI transactions >> + * >> + * Set the word length (number of bits per word) for SPI transactions. >> + * >> + * @device: The SPI slave >> + * @wordlen: The number of bits in a word >> + * >> + * Returns: 0 on success, -ve on failure. >> + */ >> + int (*set_wordlen)(struct udevice *deiuce, unsigned int wordlen); >> + >> + /** >> + * SPI transfer >> + * >> + * This writes "bitlen" bits out the SPI MOSI port and simultaneously >> + * clocks "bitlen" bits in the SPI MISO port. That's just the way SPI >> + * works. >> + * >> + * The source of the outgoing bits is the "dout" parameter and the >> + * destination of the input bits is the "din" parameter. Note that >> + * "dout" and "din" can point to the same memory location, in which >> + * case the input data overwrites the output data (since both are >> + * buffered by temporary variables, this is OK). >> + * >> + * spi_xfer() interface: >> + * @device: The SPI bus which will be sending/receiving the data. >> + * @slave: The slave device to communicate with >> + * @bitlen: How many bits to write and read. >> + * @dout: Pointer to a string of bits to send out. The bits are >> + * held in a byte array and are sent MSB first. >> + * @din: Pointer to a string of bits that will be filled in. >> + * @flags: A bitwise combination of SPI_XFER_* flags. >> + * >> + * Returns: 0 on success, not -1 on failure >> + */ >> + int (*xfer)(struct udevice *device, struct udevice *slave, >> + unsigned int bitlen, const void *dout, void *din, >> + unsigned long flags); >> + >> + /** >> + * Set transfer speed. >> + * This sets a new speed to be applied for next spi_xfer(). >> + * @slave: The SPI slave >> + * @hz: The transfer speed >> + * @return 0 if OK, -ve on error >> + */ >> + int (*set_speed)(struct udevice *device, uint hz); >> + >> + /** >> + * Set the SPI mode/flags >> + * >> + * It is unclear if we want to set speed and mode together instead >> + * of separately. >> + * >> + * @slave: The SPI slave >> + * @mode: Requested SPI mode (SPI_... flags) >> + * @return 0 if OK, -ve on error >> + */ >> + int (*set_mode)(struct udevice *device, uint mode); >> +}; >> + >> +int spi_find_bus_and_cs(int busnum, int cs, struct udevice **busp, >> + struct udevice **devp); >> + >> +int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, >> + const char *drv_name, const char *dev_name, >> + struct udevice **devp, struct spi_slave **slavep); >> + >> +int spi_bind_device(struct udevice *bus, int cs, const char *drv_name, >> + const char *dev_name, struct udevice **slavep); >> + >> +int spi_ofdata_to_platdata(const void *blob, int node, >> + struct spi_slave *spi); >> + >> +struct sandbox_state; >> +int sandbox_spi_get_emul(struct sandbox_state *state, >> + struct udevice *bus, struct udevice *slave, >> + struct udevice **emulp); >> + >> +/* Access the serial operations for a device */ >> +#define spi_get_ops(dev) ((struct dm_spi_ops *)(dev)->driver->ops) >> +#endif /* CONFIG_DM_SPI */ >> + >> #endif /* _SPI_H_ */ >> -- >> 2.0.0.526.g5318336 >> > > thanks! > -- > Jagan. Regards, Simon
diff --git a/common/exports.c b/common/exports.c index b97ca48..88fcfc8 100644 --- a/common/exports.c +++ b/common/exports.c @@ -27,10 +27,12 @@ unsigned long get_version(void) # define i2c_write dummy # define i2c_read dummy #endif -#ifndef CONFIG_CMD_SPI +#if !defined(CONFIG_CMD_SPI) || defined(CONFIG_DM_SPI) # define spi_init dummy # define spi_setup_slave dummy # define spi_free_slave dummy +#endif +#ifndef CONFIG_CMD_SPI # define spi_claim_bus dummy # define spi_release_bus dummy # define spi_xfer dummy diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index f02c35a..d1f1dd0 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -6,7 +6,11 @@ # # There are many options which enable SPI, so make this library available +ifdef CONFIG_DM_SPI +obj-y += spi-uclass.o +else obj-y += spi.o +endif obj-$(CONFIG_EP93XX_SPI) += ep93xx_spi.o obj-$(CONFIG_ALTERA_SPI) += altera_spi.o diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c new file mode 100644 index 0000000..4057bce --- /dev/null +++ b/drivers/spi/spi-uclass.c @@ -0,0 +1,253 @@ +/* + * Copyright (c) 2014 Google, Inc + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <dm.h> +#include <errno.h> +#include <fdtdec.h> +#include <spi.h> +#include <dm/device-internal.h> +#include <dm/uclass-internal.h> +#include <dm/root.h> +#include <dm/lists.h> + +DECLARE_GLOBAL_DATA_PTR; + +static int spi_set_speed_mode(struct udevice *bus, int speed, int mode) +{ + struct dm_spi_ops *ops; + int ret; + + ops = spi_get_ops(bus); + if (ops->set_speed) + ret = (*ops->set_speed)(bus, speed); + else + ret = -EINVAL; + if (ret) { + printf("Cannot set speed (err=%d)\n", ret); + return ret; + } + + ops = spi_get_ops(bus); + if (ops->set_mode) + ret = (*ops->set_mode)(bus, mode); + else + ret = -EINVAL; + if (ret) { + printf("Cannot set mode (err=%d)\n", ret); + return ret; + } + + return 0; +} + +int spi_claim_bus(struct spi_slave *slave) +{ + struct udevice *dev = slave->dev; + struct udevice *bus = dev->parent; + struct dm_spi_ops *ops = spi_get_ops(bus); + struct dm_spi_bus *spi = bus->uclass_priv; + int speed; + int ret; + + speed = slave->max_hz; + if (spi->max_hz) { + if (speed) + speed = min(speed, spi->max_hz); + else + speed = spi->max_hz; + } + if (!speed) + speed = 100000; + ret = spi_set_speed_mode(bus, speed, slave->mode); + if (ret) + return ret; + + return ops->claim_bus ? ops->claim_bus(bus) : 0; +} + +void spi_release_bus(struct spi_slave *slave) +{ + struct udevice *dev = slave->dev; + struct udevice *bus = dev->parent; + struct dm_spi_ops *ops = spi_get_ops(bus); + + if (ops->release_bus) + spi_get_ops(bus)->release_bus(bus); +} + +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; + + if (bus->uclass->uc_drv->id != UCLASS_SPI) + return -EOPNOTSUPP; + + return spi_get_ops(bus)->xfer(bus, dev, bitlen, dout, din, flags); +} + +int spi_post_bind(struct udevice *dev) +{ + /* Scan the bus for devices */ + return dm_scan_fdt_node(dev, gd->fdt_blob, dev->of_offset, false); +} + +int spi_post_probe(struct udevice *dev) +{ + struct dm_spi_bus *spi = dev->uclass_priv; + + spi->max_hz = fdtdec_get_int(gd->fdt_blob, dev->of_offset, + "spi-max-frequency", 0); + + return 0; +} + +int spi_bind_device(struct udevice *bus, int cs, const char *drv_name, + const char *dev_name, struct udevice **slavep) +{ + struct driver *drv; + int ret; + + drv = lists_driver_lookup_name(drv_name); + if (!drv) { + puts("Cannot find spi_flash_std driver\n"); + return -ENOENT; + } + ret = device_bind(bus, drv, dev_name, NULL, -1, slavep); + if (ret) { + printf("Cannot create device named '%s' (err=%d)\n", + dev_name, ret); + return ret; + } + (*slavep)->req_seq = cs; + + return 0; +} + +int spi_find_bus_and_cs(int busnum, int cs, struct udevice **busp, + struct udevice **devp) +{ + struct udevice *bus, *dev; + int ret; + + ret = uclass_find_device_by_seq(UCLASS_SPI, busnum, false, &bus); + if (ret) + return ret; + ret = device_find_child_by_seq(bus, cs, false, &dev); + if (ret) + return ret; + *busp = bus; + *devp = dev; + + return ret; +} + +int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, + const char *drv_name, const char *dev_name, + struct udevice **devp, struct spi_slave **slavep) +{ + struct udevice *bus, *dev; + int ret; + + ret = uclass_get_device_by_seq(UCLASS_SPI, busnum, &bus); + if (ret) { + printf("Invalid bus %d (err=%d)\n", busnum, ret); + return ret; + } + ret = device_get_child_by_seq(bus, cs, &dev); + + /** + * If there is no such device, create one automatically. This means + * that we don't need a device tree node or platform data for the + * SPI flash chip - we will bind to the correct driver. + */ + if (ret == -ENODEV && drv_name) { + ret = spi_bind_device(bus, cs, drv_name, dev_name, &dev); + if (ret) + return ret; + } + if (ret) { + printf("Invalid chip select %d:%d (err=%d)\n", busnum, cs, + ret); + return ret; + } + + ret = spi_set_speed_mode(bus, speed, mode); + if (ret) + return ret; + + *devp = bus; + *slavep = dev_get_parentdata(dev); + + return 0; +} + +/* Compatibility function - to be removed */ +struct spi_slave *spi_setup_slave_fdt(const void *blob, int node, + int bus_node) +{ + struct udevice *bus, *dev; + int ret; + + ret = uclass_get_device_by_of_offset(UCLASS_SPI, bus_node, &bus); + if (ret) + return NULL; + ret = device_get_child_by_of_offset(bus, node, &dev); + if (ret) + return NULL; + return dev_get_parentdata(dev); +} + +/* Compatibility function - to be removed */ +struct spi_slave *spi_setup_slave(unsigned int busnum, unsigned int cs, + unsigned int speed, unsigned int mode) +{ + struct spi_slave *slave; + struct udevice *dev; + int ret; + + ret = spi_get_bus_and_cs(busnum, cs, speed, mode, NULL, 0, &dev, + &slave); + if (ret) + return NULL; + + return slave; +} + +void spi_free_slave(struct spi_slave *slave) +{ + device_remove(slave->dev); + slave->dev = NULL; +} + +int spi_ofdata_to_platdata(const void *blob, int node, + struct spi_slave *spi) +{ + int mode = 0; + + spi->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency", 0); + if (fdtdec_get_bool(blob, node, "spi-cpol")) + mode |= SPI_CPOL; + if (fdtdec_get_bool(blob, node, "spi-cpha")) + mode |= SPI_CPHA; + if (fdtdec_get_bool(blob, node, "spi-cs-high")) + mode |= SPI_CS_HIGH; + if (fdtdec_get_bool(blob, node, "spi-half-duplex")) + mode |= SPI_PREAMBLE; + spi->mode = mode; + + return 0; +} + +UCLASS_DRIVER(spi) = { + .id = UCLASS_SPI, + .name = "spi", + .post_bind = spi_post_bind, + .post_probe = spi_post_probe, + .per_device_auto_alloc_size = sizeof(struct dm_spi_bus), +}; diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 7f0e37b..8207483 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -22,6 +22,7 @@ enum uclass_id { /* U-Boot uclasses start here */ UCLASS_GPIO, /* Bank of general-purpose I/O pins */ UCLASS_SERIAL, /* Serial UART */ + UCLASS_SPI, /* SPI bus */ UCLASS_COUNT, UCLASS_INVALID = -1, diff --git a/include/spi.h b/include/spi.h index b673be2..b5e9347 100644 --- a/include/spi.h +++ b/include/spi.h @@ -54,11 +54,19 @@ #define SPI_DEFAULT_WORDLEN 8 +#ifdef CONFIG_DM_SPI +struct dm_spi_bus { + uint max_hz; +}; + +#endif /* CONFIG_DM_SPI */ + /** * struct spi_slave - Representation of a SPI slave * * Drivers are expected to extend this with controller-specific data. * + * @dev: SPI slave device * @bus: ID of the bus that the slave is attached to. * @cs: ID of the chip select connected to the slave. * @op_mode_rx: SPI RX operation mode. @@ -71,8 +79,14 @@ * @flags: Indication of SPI flags. */ struct spi_slave { +#ifdef CONFIG_DM_SPI + struct udevice *dev; /* struct spi_slave is dev->parentdata */ + uint max_hz; + uint mode; +#else unsigned int bus; unsigned int cs; +#endif u8 op_mode_rx; u8 op_mode_tx; unsigned int wordlen; @@ -220,6 +234,8 @@ int spi_set_wordlen(struct spi_slave *slave, unsigned int wordlen); int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout, void *din, unsigned long flags); +#ifndef CONFIG_DM_SPI + /** * Determine if a SPI chipselect is valid. * This function is provided by the board if the low-level SPI driver @@ -255,6 +271,7 @@ void spi_cs_deactivate(struct spi_slave *slave); * @hz: The transfer speed */ void spi_set_speed(struct spi_slave *slave, uint hz); +#endif /** * Write 8 bits, then read 8 bits. @@ -305,4 +322,127 @@ struct spi_slave *spi_setup_slave_fdt(const void *blob, int slave_node, struct spi_slave *spi_base_setup_slave_fdt(const void *blob, int busnum, int node); +#ifdef CONFIG_DM_SPI + +/** + * struct struct dm_spi_ops - Driver model SPI operations + * + * The uclass interface is implemented by all SPI devices which use + * driver model. + */ +struct dm_spi_ops { + /** + * Claim the bus and prepare it for communication. + * + * The device privided is the slave device. It's parent controller + * will be used to provide the communication. + * + * This must be called before doing any transfers with a SPI slave. It + * will enable and initialize any SPI hardware as necessary, and make + * sure that the SCK line is in the correct idle state. It is not + * allowed to claim the same bus for several slaves without releasing + * the bus in between. + * + * @device: The SPI slave + * + * Returns: 0 if the bus was claimed successfully, or a negative value + * if it wasn't. + */ + int (*claim_bus)(struct udevice *device); + + /** + * Release the SPI bus + * + * This must be called once for every call to spi_claim_bus() after + * all transfers have finished. It may disable any SPI hardware as + * appropriate. + * + * @device: The SPI slave + */ + int (*release_bus)(struct udevice *device); + + /** + * Set the word length for SPI transactions + * + * Set the word length (number of bits per word) for SPI transactions. + * + * @device: The SPI slave + * @wordlen: The number of bits in a word + * + * Returns: 0 on success, -ve on failure. + */ + int (*set_wordlen)(struct udevice *deiuce, unsigned int wordlen); + + /** + * SPI transfer + * + * This writes "bitlen" bits out the SPI MOSI port and simultaneously + * clocks "bitlen" bits in the SPI MISO port. That's just the way SPI + * works. + * + * The source of the outgoing bits is the "dout" parameter and the + * destination of the input bits is the "din" parameter. Note that + * "dout" and "din" can point to the same memory location, in which + * case the input data overwrites the output data (since both are + * buffered by temporary variables, this is OK). + * + * spi_xfer() interface: + * @device: The SPI bus which will be sending/receiving the data. + * @slave: The slave device to communicate with + * @bitlen: How many bits to write and read. + * @dout: Pointer to a string of bits to send out. The bits are + * held in a byte array and are sent MSB first. + * @din: Pointer to a string of bits that will be filled in. + * @flags: A bitwise combination of SPI_XFER_* flags. + * + * Returns: 0 on success, not -1 on failure + */ + int (*xfer)(struct udevice *device, struct udevice *slave, + unsigned int bitlen, const void *dout, void *din, + unsigned long flags); + + /** + * Set transfer speed. + * This sets a new speed to be applied for next spi_xfer(). + * @slave: The SPI slave + * @hz: The transfer speed + * @return 0 if OK, -ve on error + */ + int (*set_speed)(struct udevice *device, uint hz); + + /** + * Set the SPI mode/flags + * + * It is unclear if we want to set speed and mode together instead + * of separately. + * + * @slave: The SPI slave + * @mode: Requested SPI mode (SPI_... flags) + * @return 0 if OK, -ve on error + */ + int (*set_mode)(struct udevice *device, uint mode); +}; + +int spi_find_bus_and_cs(int busnum, int cs, struct udevice **busp, + struct udevice **devp); + +int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, + const char *drv_name, const char *dev_name, + struct udevice **devp, struct spi_slave **slavep); + +int spi_bind_device(struct udevice *bus, int cs, const char *drv_name, + const char *dev_name, struct udevice **slavep); + +int spi_ofdata_to_platdata(const void *blob, int node, + struct spi_slave *spi); + +struct sandbox_state; +int sandbox_spi_get_emul(struct sandbox_state *state, + struct udevice *bus, struct udevice *slave, + struct udevice **emulp); + +/* Access the serial operations for a device */ +#define spi_get_ops(dev) ((struct dm_spi_ops *)(dev)->driver->ops) +#endif /* CONFIG_DM_SPI */ + #endif /* _SPI_H_ */
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> --- common/exports.c | 4 +- drivers/spi/Makefile | 4 + drivers/spi/spi-uclass.c | 253 +++++++++++++++++++++++++++++++++++++++++++++++ include/dm/uclass-id.h | 1 + include/spi.h | 140 ++++++++++++++++++++++++++ 5 files changed, 401 insertions(+), 1 deletion(-) create mode 100644 drivers/spi/spi-uclass.c