| Message ID | 20210427170857.21273-1-tharvey@gateworks.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | [1/3] usb: ehci-mx6: move mode set/detect to probe | expand |
On 4/27/21 7:08 PM, Tim Harvey wrote: > There is no need to set and/or detect mode in of_to_plat and > accessing phy registers at that point before device power domain and > clock are enabled will cause hangs on platforms such as IMX8M Mini. > > Move the mode set/detect from of_to_plat into the probe and remove > the unnecessary of_to_plat. > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > --- > drivers/usb/host/ehci-mx6.c | 42 ++++++++++++++----------------------- > 1 file changed, 16 insertions(+), 26 deletions(-) > > diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c > index 06be9deaaa..c2dfe49012 100644 > --- a/drivers/usb/host/ehci-mx6.c > +++ b/drivers/usb/host/ehci-mx6.c > @@ -539,28 +539,6 @@ static int ehci_usb_phy_mode(struct udevice *dev) > return 0; > } > > -static int ehci_usb_of_to_plat(struct udevice *dev) > -{ > - struct usb_plat *plat = dev_get_plat(dev); > - enum usb_dr_mode dr_mode; > - > - dr_mode = usb_get_dr_mode(dev_ofnode(dev)); > - > - switch (dr_mode) { > - case USB_DR_MODE_HOST: > - plat->init_type = USB_INIT_HOST; > - break; > - case USB_DR_MODE_PERIPHERAL: > - plat->init_type = USB_INIT_DEVICE; > - break; > - case USB_DR_MODE_OTG: > - case USB_DR_MODE_UNKNOWN: > - return ehci_usb_phy_mode(dev); > - }; > - > - return 0; > -} > - [...] > @@ -764,7 +755,6 @@ U_BOOT_DRIVER(usb_mx6) = { > .name = "ehci_mx6", > .id = UCLASS_USB, > .of_match = mx6_usb_ids, > - .of_to_plat = ehci_usb_of_to_plat, I wonder why it was implemented in of_to_plat originally , maybe there is some reason for that ?
On Tue, Apr 27, 2021 at 10:45 AM Marek Vasut <marex@denx.de> wrote: > > On 4/27/21 7:08 PM, Tim Harvey wrote: > > There is no need to set and/or detect mode in of_to_plat and > > accessing phy registers at that point before device power domain and > > clock are enabled will cause hangs on platforms such as IMX8M Mini. > > > > Move the mode set/detect from of_to_plat into the probe and remove > > the unnecessary of_to_plat. > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > --- > > drivers/usb/host/ehci-mx6.c | 42 ++++++++++++++----------------------- > > 1 file changed, 16 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c > > index 06be9deaaa..c2dfe49012 100644 > > --- a/drivers/usb/host/ehci-mx6.c > > +++ b/drivers/usb/host/ehci-mx6.c > > @@ -539,28 +539,6 @@ static int ehci_usb_phy_mode(struct udevice *dev) > > return 0; > > } > > > > -static int ehci_usb_of_to_plat(struct udevice *dev) > > -{ > > - struct usb_plat *plat = dev_get_plat(dev); > > - enum usb_dr_mode dr_mode; > > - > > - dr_mode = usb_get_dr_mode(dev_ofnode(dev)); > > - > > - switch (dr_mode) { > > - case USB_DR_MODE_HOST: > > - plat->init_type = USB_INIT_HOST; > > - break; > > - case USB_DR_MODE_PERIPHERAL: > > - plat->init_type = USB_INIT_DEVICE; > > - break; > > - case USB_DR_MODE_OTG: > > - case USB_DR_MODE_UNKNOWN: > > - return ehci_usb_phy_mode(dev); > > - }; > > - > > - return 0; > > -} > > - > > [...] > > > @@ -764,7 +755,6 @@ U_BOOT_DRIVER(usb_mx6) = { > > .name = "ehci_mx6", > > .id = UCLASS_USB, > > .of_match = mx6_usb_ids, > > - .of_to_plat = ehci_usb_of_to_plat, > > I wonder why it was implemented in of_to_plat originally , maybe there > is some reason for that ? Marek, Looking back the commit that added the ehci_usb_ofdata_to_platdata was: cccbddc38c43 ("usb: ehci-mx6: implement ofdata_to_platdata") Before that there was a board-specific function that would set the usb_plat->init_type. The only reason to set usb_plat->init_type in of_to_plat would be so that drivers/usb/host/usb-uclass.c would have knowledge of it but I only see that it is set there in usb_setup_ehci_gadget. I added Peng, Stefano, and Simon to the thread to see if they see an issue with doing away with of_to_plat setting the usb_plat->init_type prior to probe. Tim
On 4/28/21 3:51 AM, Tim Harvey wrote: > On Tue, Apr 27, 2021 at 10:45 AM Marek Vasut <marex@denx.de> wrote: >> >> On 4/27/21 7:08 PM, Tim Harvey wrote: >>> There is no need to set and/or detect mode in of_to_plat and >>> accessing phy registers at that point before device power domain and >>> clock are enabled will cause hangs on platforms such as IMX8M Mini. >>> >>> Move the mode set/detect from of_to_plat into the probe and remove >>> the unnecessary of_to_plat. >>> >>> Signed-off-by: Tim Harvey <tharvey@gateworks.com> >>> --- >>> drivers/usb/host/ehci-mx6.c | 42 ++++++++++++++----------------------- >>> 1 file changed, 16 insertions(+), 26 deletions(-) >>> >>> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c >>> index 06be9deaaa..c2dfe49012 100644 >>> --- a/drivers/usb/host/ehci-mx6.c >>> +++ b/drivers/usb/host/ehci-mx6.c >>> @@ -539,28 +539,6 @@ static int ehci_usb_phy_mode(struct udevice *dev) >>> return 0; >>> } >>> >>> -static int ehci_usb_of_to_plat(struct udevice *dev) >>> -{ >>> - struct usb_plat *plat = dev_get_plat(dev); >>> - enum usb_dr_mode dr_mode; >>> - >>> - dr_mode = usb_get_dr_mode(dev_ofnode(dev)); >>> - >>> - switch (dr_mode) { >>> - case USB_DR_MODE_HOST: >>> - plat->init_type = USB_INIT_HOST; >>> - break; >>> - case USB_DR_MODE_PERIPHERAL: >>> - plat->init_type = USB_INIT_DEVICE; >>> - break; >>> - case USB_DR_MODE_OTG: >>> - case USB_DR_MODE_UNKNOWN: >>> - return ehci_usb_phy_mode(dev); >>> - }; >>> - >>> - return 0; >>> -} >>> - >> >> [...] >> >>> @@ -764,7 +755,6 @@ U_BOOT_DRIVER(usb_mx6) = { >>> .name = "ehci_mx6", >>> .id = UCLASS_USB, >>> .of_match = mx6_usb_ids, >>> - .of_to_plat = ehci_usb_of_to_plat, >> >> I wonder why it was implemented in of_to_plat originally , maybe there >> is some reason for that ? > > Marek, > > Looking back the commit that added the ehci_usb_ofdata_to_platdata was: > cccbddc38c43 ("usb: ehci-mx6: implement ofdata_to_platdata") > > Before that there was a board-specific function that would set the > usb_plat->init_type. > > The only reason to set usb_plat->init_type in of_to_plat would be so > that drivers/usb/host/usb-uclass.c would have knowledge of it but I > only see that it is set there in usb_setup_ehci_gadget. So interaction with Gadget, that's what I was afraid of. > I added Peng, Stefano, and Simon to the thread to see if they see an > issue with doing away with of_to_plat setting the usb_plat->init_type > prior to probe. I added Lukasz too.
On Tue, Apr 27, 2021 at 6:59 PM Marek Vasut <marex@denx.de> wrote: > > On 4/28/21 3:51 AM, Tim Harvey wrote: > > On Tue, Apr 27, 2021 at 10:45 AM Marek Vasut <marex@denx.de> wrote: > >> > >> On 4/27/21 7:08 PM, Tim Harvey wrote: > >>> There is no need to set and/or detect mode in of_to_plat and > >>> accessing phy registers at that point before device power domain and > >>> clock are enabled will cause hangs on platforms such as IMX8M Mini. > >>> > >>> Move the mode set/detect from of_to_plat into the probe and remove > >>> the unnecessary of_to_plat. > >>> > >>> Signed-off-by: Tim Harvey <tharvey@gateworks.com> > >>> --- > >>> drivers/usb/host/ehci-mx6.c | 42 ++++++++++++++----------------------- > >>> 1 file changed, 16 insertions(+), 26 deletions(-) > >>> > >>> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c > >>> index 06be9deaaa..c2dfe49012 100644 > >>> --- a/drivers/usb/host/ehci-mx6.c > >>> +++ b/drivers/usb/host/ehci-mx6.c > >>> @@ -539,28 +539,6 @@ static int ehci_usb_phy_mode(struct udevice *dev) > >>> return 0; > >>> } > >>> > >>> -static int ehci_usb_of_to_plat(struct udevice *dev) > >>> -{ > >>> - struct usb_plat *plat = dev_get_plat(dev); > >>> - enum usb_dr_mode dr_mode; > >>> - > >>> - dr_mode = usb_get_dr_mode(dev_ofnode(dev)); > >>> - > >>> - switch (dr_mode) { > >>> - case USB_DR_MODE_HOST: > >>> - plat->init_type = USB_INIT_HOST; > >>> - break; > >>> - case USB_DR_MODE_PERIPHERAL: > >>> - plat->init_type = USB_INIT_DEVICE; > >>> - break; > >>> - case USB_DR_MODE_OTG: > >>> - case USB_DR_MODE_UNKNOWN: > >>> - return ehci_usb_phy_mode(dev); > >>> - }; > >>> - > >>> - return 0; > >>> -} > >>> - > >> > >> [...] > >> > >>> @@ -764,7 +755,6 @@ U_BOOT_DRIVER(usb_mx6) = { > >>> .name = "ehci_mx6", > >>> .id = UCLASS_USB, > >>> .of_match = mx6_usb_ids, > >>> - .of_to_plat = ehci_usb_of_to_plat, > >> > >> I wonder why it was implemented in of_to_plat originally , maybe there > >> is some reason for that ? > > > > Marek, > > > > Looking back the commit that added the ehci_usb_ofdata_to_platdata was: > > cccbddc38c43 ("usb: ehci-mx6: implement ofdata_to_platdata") > > > > Before that there was a board-specific function that would set the > > usb_plat->init_type. > > > > The only reason to set usb_plat->init_type in of_to_plat would be so > > that drivers/usb/host/usb-uclass.c would have knowledge of it but I > > only see that it is set there in usb_setup_ehci_gadget. > > So interaction with Gadget, that's what I was afraid of. > > > I added Peng, Stefano, and Simon to the thread to see if they see an > > issue with doing away with of_to_plat setting the usb_plat->init_type > > prior to probe. > > I added Lukasz too. Marek, Is there something you want me to change here? I am happy to test gadget support as my board has an OTG connector. Could you give me some pointers on how to configure a gadget in U-Boot in order to test? Best regards, Tim
Hi Tim. On Thu, Jun 3, 2021 at 12:50 PM Tim Harvey <tharvey@gateworks.com> wrote: > Marek, > > Is there something you want me to change here? > > I am happy to test gadget support as my board has an OTG connector. > Could you give me some pointers on how to configure a gadget in U-Boot > in order to test? One quick way to test gadget support is via the "ums 0 mmc 0" command. This will mount the eMMC content via USB mass storage device on the PC. You can check configs/warp7_defconfig for an example on how to select the USB gadget options. I applied this one and tested on a imx7s-warp board via "ums 0 mmc 0". Tested-by: Fabio Estevam <festevam@gmail.com> I am interested in getting USB gadget support in imx8mm too. Thanks
On Thu, Jul 1, 2021 at 5:41 PM Fabio Estevam <festevam@gmail.com> wrote: > > Hi Tim. > > On Thu, Jun 3, 2021 at 12:50 PM Tim Harvey <tharvey@gateworks.com> wrote: > > > Marek, > > > > Is there something you want me to change here? > > > > I am happy to test gadget support as my board has an OTG connector. > > Could you give me some pointers on how to configure a gadget in U-Boot > > in order to test? > > One quick way to test gadget support is via the "ums 0 mmc 0" command. > > This will mount the eMMC content via USB mass storage device on the PC. > > You can check configs/warp7_defconfig for an example on how to select > the USB gadget options. > > I applied this one and tested on a imx7s-warp board via "ums 0 mmc 0". > > Tested-by: Fabio Estevam <festevam@gmail.com> > > I am interested in getting USB gadget support in imx8mm too. > Fabio, Right - I finally remembered that earlier today and just sent a v2 of this series showing that I indeed tested that on both imx6q and imx8mm. When you get a chance please show your Tested-by on that thread. Tim
diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c index 06be9deaaa..c2dfe49012 100644 --- a/drivers/usb/host/ehci-mx6.c +++ b/drivers/usb/host/ehci-mx6.c @@ -539,28 +539,6 @@ static int ehci_usb_phy_mode(struct udevice *dev) return 0; } -static int ehci_usb_of_to_plat(struct udevice *dev) -{ - struct usb_plat *plat = dev_get_plat(dev); - enum usb_dr_mode dr_mode; - - dr_mode = usb_get_dr_mode(dev_ofnode(dev)); - - switch (dr_mode) { - case USB_DR_MODE_HOST: - plat->init_type = USB_INIT_HOST; - break; - case USB_DR_MODE_PERIPHERAL: - plat->init_type = USB_INIT_DEVICE; - break; - case USB_DR_MODE_OTG: - case USB_DR_MODE_UNKNOWN: - return ehci_usb_phy_mode(dev); - }; - - return 0; -} - static int mx6_parse_dt_addrs(struct udevice *dev) { #if !defined(CONFIG_PHY) @@ -623,7 +601,6 @@ static int ehci_usb_probe(struct udevice *dev) struct usb_plat *plat = dev_get_plat(dev); struct usb_ehci *ehci = dev_read_addr_ptr(dev); struct ehci_mx6_priv_data *priv = dev_get_priv(dev); - enum usb_init_type type = plat->init_type; struct ehci_hccr *hccr; struct ehci_hcor *hcor; int ret; @@ -641,7 +618,6 @@ static int ehci_usb_probe(struct udevice *dev) return ret; priv->ehci = ehci; - priv->init_type = type; #if CONFIG_IS_ENABLED(CLK) ret = clk_get_by_index(dev, 0, &priv->clk); @@ -657,6 +633,21 @@ static int ehci_usb_probe(struct udevice *dev) mdelay(1); #endif + switch (usb_get_dr_mode(dev_ofnode(dev))) { + case USB_DR_MODE_HOST: + plat->init_type = USB_INIT_HOST; + break; + case USB_DR_MODE_PERIPHERAL: + plat->init_type = USB_INIT_DEVICE; + break; + case USB_DR_MODE_OTG: + case USB_DR_MODE_UNKNOWN: + ret = ehci_usb_phy_mode(dev); + if (ret) + return ret; + }; + priv->init_type = plat->init_type; + #if CONFIG_IS_ENABLED(DM_REGULATOR) ret = device_get_supply_regulator(dev, "vbus-supply", &priv->vbus_supply); @@ -680,7 +671,7 @@ static int ehci_usb_probe(struct udevice *dev) #if CONFIG_IS_ENABLED(DM_REGULATOR) if (priv->vbus_supply) { ret = regulator_set_enable(priv->vbus_supply, - (type == USB_INIT_DEVICE) ? + (priv->init_type == USB_INIT_DEVICE) ? false : true); if (ret && ret != -ENOSYS) { printf("Error enabling VBUS supply (ret=%i)\n", ret); @@ -764,7 +755,6 @@ U_BOOT_DRIVER(usb_mx6) = { .name = "ehci_mx6", .id = UCLASS_USB, .of_match = mx6_usb_ids, - .of_to_plat = ehci_usb_of_to_plat, .probe = ehci_usb_probe, .remove = ehci_usb_remove, .ops = &ehci_usb_ops,
There is no need to set and/or detect mode in of_to_plat and accessing phy registers at that point before device power domain and clock are enabled will cause hangs on platforms such as IMX8M Mini. Move the mode set/detect from of_to_plat into the probe and remove the unnecessary of_to_plat. Signed-off-by: Tim Harvey <tharvey@gateworks.com> --- drivers/usb/host/ehci-mx6.c | 42 ++++++++++++++----------------------- 1 file changed, 16 insertions(+), 26 deletions(-)