| Message ID | 20190322164429.28637-1-jjhiblot@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [U-Boot] dm: Add a No-op uclass | expand |
On Fri, 22 Mar 2019, Jean-Jacques Hiblot wrote: It is probably the right solution, just have one suggestion -- why wouldn't we make it UCLASS_GLUE instead? NOP is too generic, IMHO and it is just NOP. There is definitely a place for such thing but we might want to add some specific functionality for glues and NOP is not a very good place to do so... Just my $.25... BTW, there is yet another thing with USB gadgets bound by a glue. The usb_gadget_initialize() in udc-uclass.c calls uclass_get_device_by_seq() that requires either already probed device or an alias. If neither is found it fails so it is not possible to get USB Glue gadget subnode to come up automagically. Replacing uclass_get_device_by_seq() with plain uclass_get_device() solves this problem -- it probes the device without a need for an alias. Tested here on a custom imx8mq board. Yet another $.25 :) > This uclass is intended for devices that do not need any features from the > uclass, including binding children. > This will typically be used by devices that are used to bind child devices > but do not use dm_scan_fdt_dev() to do it. > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> > --- > drivers/core/uclass.c | 5 +++++ > include/dm/uclass-id.h | 1 + > 2 files changed, 6 insertions(+) > > diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c > index fc3157de39..dc9eb62893 100644 > --- a/drivers/core/uclass.c > +++ b/drivers/core/uclass.c > @@ -757,3 +757,8 @@ int uclass_pre_remove_device(struct udevice *dev) > return 0; > } > #endif > + > +UCLASS_DRIVER(nop) = { > + .id = UCLASS_NOP, > + .name = "nop", > +}; > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h > index 86e59781b0..3797cd48f6 100644 > --- a/include/dm/uclass-id.h > +++ b/include/dm/uclass-id.h > @@ -61,6 +61,7 @@ enum uclass_id { > UCLASS_MMC, /* SD / MMC card or chip */ > UCLASS_MOD_EXP, /* RSA Mod Exp device */ > UCLASS_MTD, /* Memory Technology Device (MTD) device */ > + UCLASS_NOP, /* No-op devices */ > UCLASS_NORTHBRIDGE, /* Intel Northbridge / SDRAM controller */ > UCLASS_NVME, /* NVM Express device */ > UCLASS_PANEL, /* Display panel, such as an LCD */ > -- > 2.17.1 > --- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
Hi Sergey, On 22/03/2019 18:39, Sergey Kubushyn wrote: > On Fri, 22 Mar 2019, Jean-Jacques Hiblot wrote: > > It is probably the right solution, just have one suggestion -- why > wouldn't > we make it UCLASS_GLUE instead? NOP is too generic, IMHO and it is > just NOP. > There is definitely a place for such thing but we might want to add some > specific functionality for glues and NOP is not a very good place to do > so... GLUE is the usual term used for USB glue layers, but there are quite a few MISC users that are not at all related to USB that would benefit a UCLASS that really does nothing by itself (at91 clk system for example). If a need for USB specific features arises then a UCLASS_GLUE will make sense. Until then I don't think we should add it. > > Just my $.25... > > BTW, there is yet another thing with USB gadgets bound by a glue. The > usb_gadget_initialize() in udc-uclass.c calls uclass_get_device_by_seq() > that requires either already probed device or an alias. If neither is > found > it fails so it is not possible to get USB Glue gadget subnode to come up > automagically. > > Replacing uclass_get_device_by_seq() with plain uclass_get_device() > solves > this problem -- it probes the device without a need for an alias. Tested > here on a custom imx8mq board. I see your point and will post a fix for this. But using uclass_get_device_by_seq() has its benefit as it allows to set the index of the device using an alias in DT (usually the USB port number). JJ > > Yet another $.25 :) >> This uclass is intended for devices that do not need any features >> from the >> uclass, including binding children. >> This will typically be used by devices that are used to bind child >> devices >> but do not use dm_scan_fdt_dev() to do it. >> >> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> >> --- >> drivers/core/uclass.c | 5 +++++ >> include/dm/uclass-id.h | 1 + >> 2 files changed, 6 insertions(+) >> >> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c >> index fc3157de39..dc9eb62893 100644 >> --- a/drivers/core/uclass.c >> +++ b/drivers/core/uclass.c >> @@ -757,3 +757,8 @@ int uclass_pre_remove_device(struct udevice *dev) >> return 0; >> } >> #endif >> + >> +UCLASS_DRIVER(nop) = { >> + .id = UCLASS_NOP, >> + .name = "nop", >> +}; >> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h >> index 86e59781b0..3797cd48f6 100644 >> --- a/include/dm/uclass-id.h >> +++ b/include/dm/uclass-id.h >> @@ -61,6 +61,7 @@ enum uclass_id { >> UCLASS_MMC, /* SD / MMC card or chip */ >> UCLASS_MOD_EXP, /* RSA Mod Exp device */ >> UCLASS_MTD, /* Memory Technology Device (MTD) device */ >> + UCLASS_NOP, /* No-op devices */ >> UCLASS_NORTHBRIDGE, /* Intel Northbridge / SDRAM controller */ >> UCLASS_NVME, /* NVM Express device */ >> UCLASS_PANEL, /* Display panel, such as an LCD */ >> -- >> 2.17.1 >> > > --- > ****************************************************************** > * KSI@home KOI8 Net < > The impossible we do immediately. * > * Las Vegas NV, USA < > Miracles require 24-hour notice. * > ****************************************************************** >
Hi Jean-Jacques, On Fri, 22 Mar 2019 at 10:44, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote: > > This uclass is intended for devices that do not need any features from the > uclass, including binding children. > This will typically be used by devices that are used to bind child devices > but do not use dm_scan_fdt_dev() to do it. > Can you expand this motivation a little? I am not sure why calling dm_scan_fdt_dev() would be problematic. Also if you do add a new uclass it should have a sandbox driver and test. Regards, Simon
Hi Simon, On 30/03/2019 22:18, Simon Glass wrote: > Hi Jean-Jacques, > > On Fri, 22 Mar 2019 at 10:44, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote: >> This uclass is intended for devices that do not need any features from the >> uclass, including binding children. >> This will typically be used by devices that are used to bind child devices >> but do not use dm_scan_fdt_dev() to do it. >> > Can you expand this motivation a little? I am not sure why calling > dm_scan_fdt_dev() would be problematic. In the case of the USB wrappers, there are 2 children nodes: one for peripheral and one for host. The wrapper binds only one of them. dm_scan_fdt_dev() would bind both. > > Also if you do add a new uclass it should have a sandbox driver and test. There isn't much to test, except to check that the uclass is present. The UCLASS itself does not provide any feature. JJ > > Regards, > Simon >
Hi Jean-Jacques, On Mon, 1 Apr 2019 at 05:32, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote: > > Hi Simon, > > On 30/03/2019 22:18, Simon Glass wrote: > > Hi Jean-Jacques, > > > > On Fri, 22 Mar 2019 at 10:44, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote: > >> This uclass is intended for devices that do not need any features from the > >> uclass, including binding children. > >> This will typically be used by devices that are used to bind child devices > >> but do not use dm_scan_fdt_dev() to do it. > >> > > Can you expand this motivation a little? I am not sure why calling > > dm_scan_fdt_dev() would be problematic. > > In the case of the USB wrappers, there are 2 children nodes: one for > peripheral and one for host. The wrapper binds only one of them. > dm_scan_fdt_dev() would bind both. OK, please add this to the commit message and your uclass header file (or wherever else you can add docs for this uclass). > > > > > Also if you do add a new uclass it should have a sandbox driver and test. > > There isn't much to test, except to check that the uclass is present. > The UCLASS itself does not provide any feature. Well, one feature is that it does not scan its children. So you could add a child node in test.dts and test that it does not get bound. Regards, Simon
diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c index fc3157de39..dc9eb62893 100644 --- a/drivers/core/uclass.c +++ b/drivers/core/uclass.c @@ -757,3 +757,8 @@ int uclass_pre_remove_device(struct udevice *dev) return 0; } #endif + +UCLASS_DRIVER(nop) = { + .id = UCLASS_NOP, + .name = "nop", +}; diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 86e59781b0..3797cd48f6 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -61,6 +61,7 @@ enum uclass_id { UCLASS_MMC, /* SD / MMC card or chip */ UCLASS_MOD_EXP, /* RSA Mod Exp device */ UCLASS_MTD, /* Memory Technology Device (MTD) device */ + UCLASS_NOP, /* No-op devices */ UCLASS_NORTHBRIDGE, /* Intel Northbridge / SDRAM controller */ UCLASS_NVME, /* NVM Express device */ UCLASS_PANEL, /* Display panel, such as an LCD */
This uclass is intended for devices that do not need any features from the uclass, including binding children. This will typically be used by devices that are used to bind child devices but do not use dm_scan_fdt_dev() to do it. Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> --- drivers/core/uclass.c | 5 +++++ include/dm/uclass-id.h | 1 + 2 files changed, 6 insertions(+)