| Message ID | 1272384698-4359-2-git-send-email-agust@denx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Grant Likely |
| Headers | show |
On Tue, Apr 27, 2010 at 10:11 AM, Anatolij Gustschin <agust@denx.de> wrote: > Factor out common code for registering a FSL EHCI platform > device into new fsl_usb2_register_device() function. This > is done to avoid code duplication while adding code for > instantiating of MPC5121 dual role USB platform devices. > Then, the subsequent patch can use > for_each_compatible_node(np, NULL, "fsl,mpc5121-usb2-dr") { > ... > fsl_usb2_register_device(); > } > > Signed-off-by: Anatolij Gustschin <agust@denx.de> > Cc: Kumar Gala <galak@kernel.crashing.org> > Cc: Grant Likely <grant.likely@secretlab.ca> > --- > arch/powerpc/sysdev/fsl_soc.c | 231 +++++++++++++++++++--------------------- Hi Anatolij, Thanks for this work. However, I've got concerns. Forgive me for ragging on code that you didn't write, but this fsl_soc.c code for registering the USB device really doesn't belong here anymore. It should be part of the drivers/usb/host/ehci-fsl.c and the driver should do of-style binding (Which should be a lot easier if I manage to get the merge of platform bus and of_platform bus into 2.6.35). This patch series makes the fsl_soc.c code even more complicated, and scatters what is essentially driver code over even more places in the arch/powerpc tree. I'm really not keen on it being merged in this form. g.
Hi Grant, On Tue, 27 Apr 2010 10:51:21 -0600 Grant Likely <grant.likely@secretlab.ca> wrote: > On Tue, Apr 27, 2010 at 10:11 AM, Anatolij Gustschin <agust@denx.de> wrote: > > Factor out common code for registering a FSL EHCI platform > > device into new fsl_usb2_register_device() function. This > > is done to avoid code duplication while adding code for > > instantiating of MPC5121 dual role USB platform devices. > > Then, the subsequent patch can use > > for_each_compatible_node(np, NULL, "fsl,mpc5121-usb2-dr") { > > ... > > fsl_usb2_register_device(); > > } > > > > Signed-off-by: Anatolij Gustschin <agust@denx.de> > > Cc: Kumar Gala <galak@kernel.crashing.org> > > Cc: Grant Likely <grant.likely@secretlab.ca> > > --- > > arch/powerpc/sysdev/fsl_soc.c | 231 +++++++++++++++++++--------------------- > > Hi Anatolij, > > Thanks for this work. However, I've got concerns. > > Forgive me for ragging on code that you didn't write, but this > fsl_soc.c code for registering the USB device really doesn't belong > here anymore. It should be part of the drivers/usb/host/ehci-fsl.c > and the driver should do of-style binding (Which should be a lot > easier if I manage to get the merge of platform bus and of_platform > bus into 2.6.35). Now I moved the USB devices registration code to drivers/usb/host/ehci-fsl.c and added of-style binding there. It works for one platform driver based on your test-devicetree branch. It seems I can't bind more than one driver to the device described in the tree. But I need to bind at least 2 drivers, ehci-hcd and fsl-usb2-udc. For USB OTG support I need additional one to be bound to the same USB dual role device (also tree different drivers simultaneously). I also can't register UDC device in the ehci-fsl.c since registering of the UDC device should also be possible independent of the EHCI-HDC driver (for USB device support we do not need host controller driver). Is it possible to bind more than one driver to the same device simultaneously? If so, how can I implement this? > This patch series makes the fsl_soc.c code even more complicated, and > scatters what is essentially driver code over even more places in the > arch/powerpc tree. I'm really not keen on it being merged in this > form. Now I see one reason why it has been originally implemented this way. Thanks, Anatolij
On Thu, May 6, 2010 at 1:18 PM, Anatolij Gustschin <agust@denx.de> wrote: > Hi Grant, > > On Tue, 27 Apr 2010 10:51:21 -0600 > Grant Likely <grant.likely@secretlab.ca> wrote: > >> On Tue, Apr 27, 2010 at 10:11 AM, Anatolij Gustschin <agust@denx.de> wrote: >> > Factor out common code for registering a FSL EHCI platform >> > device into new fsl_usb2_register_device() function. This >> > is done to avoid code duplication while adding code for >> > instantiating of MPC5121 dual role USB platform devices. >> > Then, the subsequent patch can use >> > for_each_compatible_node(np, NULL, "fsl,mpc5121-usb2-dr") { >> > ... >> > fsl_usb2_register_device(); >> > } >> > >> > Signed-off-by: Anatolij Gustschin <agust@denx.de> >> > Cc: Kumar Gala <galak@kernel.crashing.org> >> > Cc: Grant Likely <grant.likely@secretlab.ca> >> > --- >> > arch/powerpc/sysdev/fsl_soc.c | 231 +++++++++++++++++++--------------------- >> >> Hi Anatolij, >> >> Thanks for this work. However, I've got concerns. >> >> Forgive me for ragging on code that you didn't write, but this >> fsl_soc.c code for registering the USB device really doesn't belong >> here anymore. It should be part of the drivers/usb/host/ehci-fsl.c >> and the driver should do of-style binding (Which should be a lot >> easier if I manage to get the merge of platform bus and of_platform >> bus into 2.6.35). > > Now I moved the USB devices registration code to > drivers/usb/host/ehci-fsl.c and added of-style binding there. It > works for one platform driver based on your test-devicetree branch. > It seems I can't bind more than one driver to the device described > in the tree. But I need to bind at least 2 drivers, ehci-hcd and > fsl-usb2-udc. For USB OTG support I need additional one to be bound > to the same USB dual role device (also tree different drivers > simultaneously). > I also can't register UDC device in the ehci-fsl.c since registering > of the UDC device should also be possible independent of the EHCI-HDC > driver (for USB device support we do not need host controller driver). > Is it possible to bind more than one driver to the same device > simultaneously? If so, how can I implement this? No, the linux driver model can bind exactly one driver to a struct device. However, that doesn't mean you can't have multiple struct devices (in whatever form they come) to tell the Linux kernel about all the details of a single hardware device. >> This patch series makes the fsl_soc.c code even more complicated, and >> scatters what is essentially driver code over even more places in the >> arch/powerpc tree. I'm really not keen on it being merged in this >> form. > > Now I see one reason why it has been originally implemented > this way. Should be a solvable problem. The fsl_soc.c stuff was originally written simply because the infrastructure wasn't there for doing it any other way. We're long past that point now. I don't have time to dig into the details now (merge window and all), but ping me in a few weeks and I'll take another look to see if I can help you. g.
On Wed, 19 May 2010 14:47:47 -0600 Grant Likely <grant.likely@secretlab.ca> wrote: > On Thu, May 6, 2010 at 1:18 PM, Anatolij Gustschin <agust@denx.de> wrote: > > Hi Grant, > > > > On Tue, 27 Apr 2010 10:51:21 -0600 > > Grant Likely <grant.likely@secretlab.ca> wrote: > > > >> On Tue, Apr 27, 2010 at 10:11 AM, Anatolij Gustschin <agust@denx.de> wrote: > >> > Factor out common code for registering a FSL EHCI platform > >> > device into new fsl_usb2_register_device() function. This > >> > is done to avoid code duplication while adding code for > >> > instantiating of MPC5121 dual role USB platform devices. > >> > Then, the subsequent patch can use > >> > for_each_compatible_node(np, NULL, "fsl,mpc5121-usb2-dr") { > >> > ... > >> > fsl_usb2_register_device(); > >> > } > >> > > >> > Signed-off-by: Anatolij Gustschin <agust@denx.de> > >> > Cc: Kumar Gala <galak@kernel.crashing.org> > >> > Cc: Grant Likely <grant.likely@secretlab.ca> > >> > --- > >> > arch/powerpc/sysdev/fsl_soc.c | 231 +++++++++++++++++++--------------------- > >> > >> Hi Anatolij, > >> > >> Thanks for this work. However, I've got concerns. > >> > >> Forgive me for ragging on code that you didn't write, but this > >> fsl_soc.c code for registering the USB device really doesn't belong > >> here anymore. It should be part of the drivers/usb/host/ehci-fsl.c > >> and the driver should do of-style binding (Which should be a lot > >> easier if I manage to get the merge of platform bus and of_platform > >> bus into 2.6.35). > > > > Now I moved the USB devices registration code to > > drivers/usb/host/ehci-fsl.c and added of-style binding there. It > > works for one platform driver based on your test-devicetree branch. > > It seems I can't bind more than one driver to the device described > > in the tree. But I need to bind at least 2 drivers, ehci-hcd and > > fsl-usb2-udc. For USB OTG support I need additional one to be bound > > to the same USB dual role device (also tree different drivers > > simultaneously). > > I also can't register UDC device in the ehci-fsl.c since registering > > of the UDC device should also be possible independent of the EHCI-HDC > > driver (for USB device support we do not need host controller driver). > > Is it possible to bind more than one driver to the same device > > simultaneously? If so, how can I implement this? > > No, the linux driver model can bind exactly one driver to a struct > device. However, that doesn't mean you can't have multiple struct > devices (in whatever form they come) to tell the Linux kernel about > all the details of a single hardware device. > > >> This patch series makes the fsl_soc.c code even more complicated, and > >> scatters what is essentially driver code over even more places in the > >> arch/powerpc tree. I'm really not keen on it being merged in this > >> form. > > > > Now I see one reason why it has been originally implemented > > this way. > > Should be a solvable problem. The fsl_soc.c stuff was originally > written simply because the infrastructure wasn't there for doing it > any other way. We're long past that point now. I don't have time to > dig into the details now (merge window and all), but ping me in a few > weeks and I'll take another look to see if I can help you. Hi Grant, Ping. Do you have any suggestions how to realize this using current infrastructure? Thanks! Anatolij
On Fri, Jun 11, 2010 at 5:24 AM, Anatolij Gustschin <agust@denx.de> wrote: > On Wed, 19 May 2010 14:47:47 -0600 > Grant Likely <grant.likely@secretlab.ca> wrote: > >> On Thu, May 6, 2010 at 1:18 PM, Anatolij Gustschin <agust@denx.de> wrote: >> > Hi Grant, >> > >> > On Tue, 27 Apr 2010 10:51:21 -0600 >> > Grant Likely <grant.likely@secretlab.ca> wrote: >> > >> >> On Tue, Apr 27, 2010 at 10:11 AM, Anatolij Gustschin <agust@denx.de> wrote: >> >> > Factor out common code for registering a FSL EHCI platform >> >> > device into new fsl_usb2_register_device() function. This >> >> > is done to avoid code duplication while adding code for >> >> > instantiating of MPC5121 dual role USB platform devices. >> >> > Then, the subsequent patch can use >> >> > for_each_compatible_node(np, NULL, "fsl,mpc5121-usb2-dr") { >> >> > ... >> >> > fsl_usb2_register_device(); >> >> > } >> >> > >> >> > Signed-off-by: Anatolij Gustschin <agust@denx.de> >> >> > Cc: Kumar Gala <galak@kernel.crashing.org> >> >> > Cc: Grant Likely <grant.likely@secretlab.ca> >> >> > --- >> >> > arch/powerpc/sysdev/fsl_soc.c | 231 +++++++++++++++++++--------------------- >> >> >> >> Hi Anatolij, >> >> >> >> Thanks for this work. However, I've got concerns. >> >> >> >> Forgive me for ragging on code that you didn't write, but this >> >> fsl_soc.c code for registering the USB device really doesn't belong >> >> here anymore. It should be part of the drivers/usb/host/ehci-fsl.c >> >> and the driver should do of-style binding (Which should be a lot >> >> easier if I manage to get the merge of platform bus and of_platform >> >> bus into 2.6.35). >> > >> > Now I moved the USB devices registration code to >> > drivers/usb/host/ehci-fsl.c and added of-style binding there. It >> > works for one platform driver based on your test-devicetree branch. >> > It seems I can't bind more than one driver to the device described >> > in the tree. But I need to bind at least 2 drivers, ehci-hcd and >> > fsl-usb2-udc. For USB OTG support I need additional one to be bound >> > to the same USB dual role device (also tree different drivers >> > simultaneously). >> > I also can't register UDC device in the ehci-fsl.c since registering >> > of the UDC device should also be possible independent of the EHCI-HDC >> > driver (for USB device support we do not need host controller driver). >> > Is it possible to bind more than one driver to the same device >> > simultaneously? If so, how can I implement this? >> >> No, the linux driver model can bind exactly one driver to a struct >> device. However, that doesn't mean you can't have multiple struct >> devices (in whatever form they come) to tell the Linux kernel about >> all the details of a single hardware device. >> >> >> This patch series makes the fsl_soc.c code even more complicated, and >> >> scatters what is essentially driver code over even more places in the >> >> arch/powerpc tree. I'm really not keen on it being merged in this >> >> form. >> > >> > Now I see one reason why it has been originally implemented >> > this way. >> >> Should be a solvable problem. The fsl_soc.c stuff was originally >> written simply because the infrastructure wasn't there for doing it >> any other way. We're long past that point now. I don't have time to >> dig into the details now (merge window and all), but ping me in a few >> weeks and I'll take another look to see if I can help you. > > Hi Grant, > > Ping. Do you have any suggestions how to realize this using current > infrastructure? Thanks! It sounds like the USB OTG controller is effectively a compound device with one host controller and one device controller plus some logic to switch between the two. I'm not a USB expert, so correct me if I'm wrong here. You've got 2 choices for implementing this: A) create a 'master' of_driver which registers 2 platform devices it it's probe routine. B) Rework the drivers so that both fsl-ehci and fsl-usb2-udc bits are called directly (essentially one driver handles both modes) Option A probably makes the most sense as it has the least amount of impact on current code. Essentially, you create an of_driver and put into it's probe hook the code that is currently in fsl_soc.c. Then it will bind in the normal of_platform_bus_type manner and can register the appropriate platform devices for each platform. Some important points though; when you create the platform devices, make sure you set the parent pointer correctly so the new devices are registered as children of the of_device. Second, you can eliminate the call to platform_device_add_data() by setting the of_node pointer in the platform device (the of_node is now part of struct device). Make the driver look up its own data from the device tree instead of passing it in an anonymous data structure. On that note, the current code looks racy anyway because it registers the device before attaching the platform_data. It's probably okay because of how early it is run and no drivers are registered yet, but it is still wrong. It should be: allocate; add data; then register. But I digress. Eliminating the platform data makes this problem go away. I hope this helps. g.
diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c index b91f7ac..976218c 100644 --- a/arch/powerpc/sysdev/fsl_soc.c +++ b/arch/powerpc/sysdev/fsl_soc.c @@ -209,6 +209,49 @@ static int __init of_add_fixed_phys(void) arch_initcall(of_add_fixed_phys); #endif /* CONFIG_FIXED_PHY */ +struct fsl_usb2_dev_data { + char *dr_mode; /* controller mode */ + char *drivers[3]; /* drivers to instantiate for this mode */ + enum fsl_usb2_operating_modes op_mode; /* operating mode */ +}; + +struct fsl_usb2_dev_data dr_data[] __initdata = { + { + "host", + { "fsl-ehci", NULL, NULL, }, + FSL_USB2_DR_HOST, + }, + { + "otg", + { "fsl-ehci", "fsl-usb2-udc", "fsl-usb2-otg", }, + FSL_USB2_DR_OTG, + }, + { + "periferal", + { "fsl-usb2-udc", NULL, NULL, }, + FSL_USB2_DR_DEVICE, + }, +}; + +struct fsl_usb2_dev_data mph_data[] __initdata = { + { NULL, { "fsl-ehci", NULL, }, FSL_USB2_MPH_HOST, }, +}; + +struct fsl_usb2_dev_data * __init get_dr_data(struct device_node *np) +{ + const unsigned char *prop; + int i; + + prop = of_get_property(np, "dr_mode", NULL); + if (prop) { + for (i = 0; i < ARRAY_SIZE(dr_data); i++) { + if (!strcmp(prop, dr_data[i].dr_mode)) + return &dr_data[i]; + } + } + return &dr_data[0]; /* mode not specified, use host */ +} + static enum fsl_usb2_phy_modes determine_usb_phy(const char *phy_type) { if (!phy_type) @@ -225,151 +268,97 @@ static enum fsl_usb2_phy_modes determine_usb_phy(const char *phy_type) return FSL_USB2_PHY_NONE; } -static int __init fsl_usb_of_init(void) +int __init fsl_usb2_register_device(struct device_node *np, + struct fsl_usb2_dev_data *dev_data, + struct fsl_usb2_platform_data *pdata, + unsigned int idx) { - struct device_node *np; - unsigned int i = 0; - struct platform_device *usb_dev_mph = NULL, *usb_dev_dr_host = NULL, - *usb_dev_dr_client = NULL; + struct platform_device *usb_dev; + const unsigned char *prop; + struct resource r[2]; + int registered = 0; int ret; + int i; + + if (!(dev_data && pdata)) + return -EINVAL; + + memset(&r, 0, sizeof(r)); + ret = of_address_to_resource(np, 0, &r[0]); + if (ret) + return -ENODEV; + + ret = of_irq_to_resource(np, 0, &r[1]); + if (ret == NO_IRQ) + return -ENODEV; + + pdata->operating_mode = dev_data->op_mode; + + prop = of_get_property(np, "phy_type", NULL); + pdata->phy_mode = determine_usb_phy(prop); + ret = 0; + for (i = 0; i < ARRAY_SIZE(dev_data->drivers); i++) { + if (!dev_data->drivers[i]) + break; + usb_dev = platform_device_register_simple( + dev_data->drivers[i], idx, r, 2); + if (IS_ERR(usb_dev)) { + ret = PTR_ERR(usb_dev); + goto out; + } - for_each_compatible_node(np, NULL, "fsl-usb2-mph") { - struct resource r[2]; - struct fsl_usb2_platform_data usb_data; - const unsigned char *prop = NULL; - - memset(&r, 0, sizeof(r)); - memset(&usb_data, 0, sizeof(usb_data)); - - ret = of_address_to_resource(np, 0, &r[0]); + usb_dev->dev.coherent_dma_mask = 0xffffffffUL; + usb_dev->dev.dma_mask = &usb_dev->dev.coherent_dma_mask; + ret = platform_device_add_data(usb_dev, pdata, + sizeof(struct fsl_usb2_platform_data)); if (ret) - goto err; - - of_irq_to_resource(np, 0, &r[1]); - - usb_dev_mph = - platform_device_register_simple("fsl-ehci", i, r, 2); - if (IS_ERR(usb_dev_mph)) { - ret = PTR_ERR(usb_dev_mph); - goto err; - } + platform_device_unregister(usb_dev); + else + registered++; + } +out: + if (!registered) + irq_dispose_mapping(r[1].end); - usb_dev_mph->dev.coherent_dma_mask = 0xffffffffUL; - usb_dev_mph->dev.dma_mask = &usb_dev_mph->dev.coherent_dma_mask; + return ret; +} - usb_data.operating_mode = FSL_USB2_MPH_HOST; +static int __init fsl_usb_of_init(void) +{ + struct device_node *np; + const unsigned char *prop; + struct fsl_usb2_platform_data pdata; + unsigned int idx = 0; + int ret; + for_each_compatible_node(np, NULL, "fsl-usb2-mph") { + memset(&pdata, 0, sizeof(pdata)); prop = of_get_property(np, "port0", NULL); if (prop) - usb_data.port_enables |= FSL_USB2_PORT0_ENABLED; + pdata.port_enables |= FSL_USB2_PORT0_ENABLED; prop = of_get_property(np, "port1", NULL); if (prop) - usb_data.port_enables |= FSL_USB2_PORT1_ENABLED; + pdata.port_enables |= FSL_USB2_PORT1_ENABLED; - prop = of_get_property(np, "phy_type", NULL); - usb_data.phy_mode = determine_usb_phy(prop); - - ret = - platform_device_add_data(usb_dev_mph, &usb_data, - sizeof(struct - fsl_usb2_platform_data)); + ret = fsl_usb2_register_device(np, mph_data, &pdata, idx++); if (ret) - goto unreg_mph; - i++; + return ret; } for_each_compatible_node(np, NULL, "fsl-usb2-dr") { - struct resource r[2]; - struct fsl_usb2_platform_data usb_data; - const unsigned char *prop = NULL; - if (!of_device_is_available(np)) continue; - memset(&r, 0, sizeof(r)); - memset(&usb_data, 0, sizeof(usb_data)); - - ret = of_address_to_resource(np, 0, &r[0]); + memset(&pdata, 0, sizeof(pdata)); + ret = fsl_usb2_register_device(np, get_dr_data(np), + &pdata, idx++); if (ret) - goto unreg_mph; - - of_irq_to_resource(np, 0, &r[1]); - - prop = of_get_property(np, "dr_mode", NULL); - - if (!prop || !strcmp(prop, "host")) { - usb_data.operating_mode = FSL_USB2_DR_HOST; - usb_dev_dr_host = platform_device_register_simple( - "fsl-ehci", i, r, 2); - if (IS_ERR(usb_dev_dr_host)) { - ret = PTR_ERR(usb_dev_dr_host); - goto err; - } - } else if (prop && !strcmp(prop, "peripheral")) { - usb_data.operating_mode = FSL_USB2_DR_DEVICE; - usb_dev_dr_client = platform_device_register_simple( - "fsl-usb2-udc", i, r, 2); - if (IS_ERR(usb_dev_dr_client)) { - ret = PTR_ERR(usb_dev_dr_client); - goto err; - } - } else if (prop && !strcmp(prop, "otg")) { - usb_data.operating_mode = FSL_USB2_DR_OTG; - usb_dev_dr_host = platform_device_register_simple( - "fsl-ehci", i, r, 2); - if (IS_ERR(usb_dev_dr_host)) { - ret = PTR_ERR(usb_dev_dr_host); - goto err; - } - usb_dev_dr_client = platform_device_register_simple( - "fsl-usb2-udc", i, r, 2); - if (IS_ERR(usb_dev_dr_client)) { - ret = PTR_ERR(usb_dev_dr_client); - goto err; - } - } else { - ret = -EINVAL; - goto err; - } - - prop = of_get_property(np, "phy_type", NULL); - usb_data.phy_mode = determine_usb_phy(prop); - - if (usb_dev_dr_host) { - usb_dev_dr_host->dev.coherent_dma_mask = 0xffffffffUL; - usb_dev_dr_host->dev.dma_mask = &usb_dev_dr_host-> - dev.coherent_dma_mask; - if ((ret = platform_device_add_data(usb_dev_dr_host, - &usb_data, sizeof(struct - fsl_usb2_platform_data)))) - goto unreg_dr; - } - if (usb_dev_dr_client) { - usb_dev_dr_client->dev.coherent_dma_mask = 0xffffffffUL; - usb_dev_dr_client->dev.dma_mask = &usb_dev_dr_client-> - dev.coherent_dma_mask; - if ((ret = platform_device_add_data(usb_dev_dr_client, - &usb_data, sizeof(struct - fsl_usb2_platform_data)))) - goto unreg_dr; - } - i++; + return ret; } - return 0; -unreg_dr: - if (usb_dev_dr_host) - platform_device_unregister(usb_dev_dr_host); - if (usb_dev_dr_client) - platform_device_unregister(usb_dev_dr_client); -unreg_mph: - if (usb_dev_mph) - platform_device_unregister(usb_dev_mph); -err: - return ret; + return 0; } - arch_initcall(fsl_usb_of_init); #if defined(CONFIG_FSL_SOC_BOOKE) || defined(CONFIG_PPC_86xx)
Factor out common code for registering a FSL EHCI platform device into new fsl_usb2_register_device() function. This is done to avoid code duplication while adding code for instantiating of MPC5121 dual role USB platform devices. Then, the subsequent patch can use for_each_compatible_node(np, NULL, "fsl,mpc5121-usb2-dr") { ... fsl_usb2_register_device(); } Signed-off-by: Anatolij Gustschin <agust@denx.de> Cc: Kumar Gala <galak@kernel.crashing.org> Cc: Grant Likely <grant.likely@secretlab.ca> --- arch/powerpc/sysdev/fsl_soc.c | 231 +++++++++++++++++++--------------------- 1 files changed, 110 insertions(+), 121 deletions(-)