[U-Boot] [PATCH v2 0/6] musb-new: Improve shutdown code
This is v2 for previous series[1], by excluding sunxi phy changes.
One patch mark it as 'RFC' about "including UCLASS_USB_DEV_GENERIC into shutdown caller" so expecting some inputs on the same
[1] https://patchwork.ozlabs.org/cover/941588/
Jagan Teki (6): usb: musb-new: Fix improper musb host pointer usb: musb-new: sunxi: Allocate struct phy in private dm: usb: Add UCLASS_USB_DEV_GENERIC shutdown musb-new: sunxi: Access ahb_reset0_cfg via ccm offset usb: musb-new: sunxi: Add proper musb exit support usb: musb-new: Call musb_platform_exit from musb_stop
board/compulab/cm_t3517/cm_t3517.c | 4 +- drivers/usb/host/usb-uclass.c | 43 ++++++++++++ drivers/usb/musb-new/musb_core.c | 1 + drivers/usb/musb-new/musb_uboot.c | 12 ++-- drivers/usb/musb-new/pic32.c | 6 +- drivers/usb/musb-new/sunxi.c | 107 ++++++++++++++++++----------- include/linux/usb/musb.h | 4 +- 7 files changed, 123 insertions(+), 54 deletions(-)
When MUSB is operating in peripheral mode, probe registering musb core using musb_register which intern return int value for validation. so there is no scope to preserve struct musb pointer but the same can be used in .remove musb_stop. So fix this by return musb_register with struct musb pointer.
Cc: Igor Grinberg grinberg@compulab.co.il Cc: Purna Chandra Mandal purna.mandal@microchip.com Signed-off-by: Jagan Teki jagan@amarulasolutions.com --- Changes for v2: - use proper error codes for musb_register
board/compulab/cm_t3517/cm_t3517.c | 4 ++-- drivers/usb/musb-new/musb_uboot.c | 12 ++++++------ drivers/usb/musb-new/pic32.c | 6 ++++-- drivers/usb/musb-new/sunxi.c | 8 +++++--- include/linux/usb/musb.h | 4 ++-- 5 files changed, 19 insertions(+), 15 deletions(-)
diff --git a/board/compulab/cm_t3517/cm_t3517.c b/board/compulab/cm_t3517/cm_t3517.c index 09cb27def0..668bb7631a 100644 --- a/board/compulab/cm_t3517/cm_t3517.c +++ b/board/compulab/cm_t3517/cm_t3517.c @@ -74,8 +74,8 @@ static void cm_t3517_musb_init(void) CONF2_REFFREQ_13MHZ | CONF2_SESENDEN | CONF2_VBDTCTEN | CONF2_DATPOL);
- if (musb_register(&cm_t3517_musb_pdata, &cm_t3517_musb_board_data, - (void *)AM35XX_IPSS_USBOTGSS_BASE)) + if (!musb_register(&cm_t3517_musb_pdata, &cm_t3517_musb_board_data, + (void *)AM35XX_IPSS_USBOTGSS_BASE)) printf("Failed initializing AM35x MUSB!\n"); } #else diff --git a/drivers/usb/musb-new/musb_uboot.c b/drivers/usb/musb-new/musb_uboot.c index 2b04fbd046..2bf918eab4 100644 --- a/drivers/usb/musb-new/musb_uboot.c +++ b/drivers/usb/musb-new/musb_uboot.c @@ -419,8 +419,8 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver) } #endif /* CONFIG_USB_MUSB_GADGET */
-int musb_register(struct musb_hdrc_platform_data *plat, void *bdata, - void *ctl_regs) +struct musb *musb_register(struct musb_hdrc_platform_data *plat, void *bdata, + void *ctl_regs) { struct musb **musbp;
@@ -436,14 +436,14 @@ int musb_register(struct musb_hdrc_platform_data *plat, void *bdata, break; #endif default: - return -EINVAL; + return ERR_PTR(-EINVAL); }
*musbp = musb_init_controller(plat, (struct device *)bdata, ctl_regs); - if (!*musbp) { + if (IS_ERR(*musbp)) { printf("Failed to init the controller\n"); - return -EIO; + return ERR_CAST(*musbp); }
- return 0; + return *musbp; } diff --git a/drivers/usb/musb-new/pic32.c b/drivers/usb/musb-new/pic32.c index f04719d7af..3a19900e21 100644 --- a/drivers/usb/musb-new/pic32.c +++ b/drivers/usb/musb-new/pic32.c @@ -251,9 +251,11 @@ static int musb_usb_probe(struct udevice *dev) ret = musb_lowlevel_init(mdata); #else pic32_musb_plat.mode = MUSB_PERIPHERAL; - ret = musb_register(&pic32_musb_plat, &pdata->dev, mregs); + mdata->host = musb_register(&pic32_musb_plat, &pdata->dev, mregs); + if (!mdata->host) + return -EIO; #endif - if (ret == 0) + if ((ret == 0) && mdata->host) printf("PIC32 MUSB OTG\n");
return ret; diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c index 08de9c69c7..b846afb094 100644 --- a/drivers/usb/musb-new/sunxi.c +++ b/drivers/usb/musb-new/sunxi.c @@ -444,9 +444,11 @@ static int musb_usb_probe(struct udevice *dev) printf("Allwinner mUSB OTG (Host)\n"); #else pdata.mode = MUSB_PERIPHERAL; - ret = musb_register(&pdata, &glue->dev, base); - if (!ret) - printf("Allwinner mUSB OTG (Peripheral)\n"); + host->host = musb_register(&pdata, &glue->dev, base); + if (!host->host) + return -EIO; + + printf("Allwinner mUSB OTG (Peripheral)\n"); #endif
return ret; diff --git a/include/linux/usb/musb.h b/include/linux/usb/musb.h index 9104414cf0..a31ce67a81 100644 --- a/include/linux/usb/musb.h +++ b/include/linux/usb/musb.h @@ -150,7 +150,7 @@ extern int tusb6010_platform_retime(unsigned is_refclk); /* * U-Boot specfic stuff */ -int musb_register(struct musb_hdrc_platform_data *plat, void *bdata, - void *ctl_regs); +struct musb *musb_register(struct musb_hdrc_platform_data *plat, void *bdata, + void *ctl_regs);
#endif /* __LINUX_USB_MUSB_H */
Allocate struct phy in private structure instead of allocating locally and assign it to a pointer. This eventually fix miss alignment phy which is used in another functions.
Signed-off-by: Jagan Teki jagan@amarulasolutions.com --- Changes for v2: - none
drivers/usb/musb-new/sunxi.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c index b846afb094..aa2880eeb9 100644 --- a/drivers/usb/musb-new/sunxi.c +++ b/drivers/usb/musb-new/sunxi.c @@ -87,7 +87,7 @@ struct sunxi_glue { struct sunxi_ccm_reg *ccm; struct sunxi_musb_config *cfg; struct device dev; - struct phy *phy; + struct phy phy; }; #define to_sunxi_glue(d) container_of(d, struct sunxi_glue, dev)
@@ -235,19 +235,19 @@ static int sunxi_musb_enable(struct musb *musb) musb_writeb(musb->mregs, USBC_REG_o_VEND0, 0);
if (is_host_enabled(musb)) { - ret = sun4i_usb_phy_vbus_detect(glue->phy); + ret = sun4i_usb_phy_vbus_detect(&glue->phy); if (ret == 1) { printf("A charger is plugged into the OTG: "); return -ENODEV; }
- ret = sun4i_usb_phy_id_detect(glue->phy); + ret = sun4i_usb_phy_id_detect(&glue->phy); if (ret == 1) { printf("No host cable detected: "); return -ENODEV; }
- ret = generic_phy_power_on(glue->phy); + ret = generic_phy_power_on(&glue->phy); if (ret) { pr_err("failed to power on USB PHY\n"); return ret; @@ -271,7 +271,7 @@ static void sunxi_musb_disable(struct musb *musb) return;
if (is_host_enabled(musb)) { - ret = generic_phy_power_off(glue->phy); + ret = generic_phy_power_off(&glue->phy); if (ret) { pr_err("failed to power off USB PHY\n"); return; @@ -291,7 +291,7 @@ static int sunxi_musb_init(struct musb *musb)
pr_debug("%s():\n", __func__);
- ret = generic_phy_init(glue->phy); + ret = generic_phy_init(&glue->phy); if (ret) { pr_err("failed to init USB PHY\n"); return ret; @@ -330,14 +330,14 @@ static void sunxi_musb_pre_root_reset_end(struct musb *musb) { struct sunxi_glue *glue = to_sunxi_glue(musb->controller);
- sun4i_usb_phy_set_squelch_detect(glue->phy, false); + sun4i_usb_phy_set_squelch_detect(&glue->phy, false); }
static void sunxi_musb_post_root_reset_end(struct musb *musb) { struct sunxi_glue *glue = to_sunxi_glue(musb->controller);
- sun4i_usb_phy_set_squelch_detect(glue->phy, true); + sun4i_usb_phy_set_squelch_detect(&glue->phy, true); }
static const struct musb_platform_ops sunxi_musb_ops = { @@ -405,7 +405,6 @@ static int musb_usb_probe(struct udevice *dev) struct usb_bus_priv *priv = dev_get_uclass_priv(dev); struct musb_hdrc_platform_data pdata; void *base = dev_read_addr_ptr(dev); - struct phy phy; int ret;
if (!base) @@ -419,13 +418,12 @@ static int musb_usb_probe(struct udevice *dev) if (IS_ERR(glue->ccm)) return PTR_ERR(glue->ccm);
- ret = generic_phy_get_by_name(dev, "usb", &phy); + ret = generic_phy_get_by_name(dev, "usb", &glue->phy); if (ret) { pr_err("failed to get usb PHY\n"); return ret; }
- glue->phy = &phy; priv->desc_before_addr = true;
memset(&pdata, 0, sizeof(pdata)); @@ -460,8 +458,8 @@ static int musb_usb_remove(struct udevice *dev) struct musb_host_data *host = &glue->mdata; int ret;
- if (generic_phy_valid(glue->phy)) { - ret = generic_phy_exit(glue->phy); + if (generic_phy_valid(&glue->phy)) { + ret = generic_phy_exit(&glue->phy); if (ret) { pr_err("failed to exit %s USB PHY\n", dev->name); return ret;
Some OTG controllers which operates on Peripheral mode are registered as UCLASS_USB_DEV_GENERIC.
So add support to shutdown them as well. shutdown happened during 'usb reset' and U-Boot handoff code for Linux boot.
controller restarting in 'usb reset' like probing again is still missing for this type of UCLASS.
Cc: Simon Glass sjg@chromium.org Signed-off-by: Jagan Teki jagan@amarulasolutions.com --- Note: I tried of traversing for multiple UCLASS in removal code in usb_stop, but couldn't come with proper solutions. I don't think any other area of code need a requirement like this, or may be I'm missing. any help would appreciate.
Changes for v2: - none
drivers/usb/host/usb-uclass.c | 43 +++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)
diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c index 611ea97a72..99cf3d2b49 100644 --- a/drivers/usb/host/usb-uclass.c +++ b/drivers/usb/host/usb-uclass.c @@ -158,6 +158,45 @@ int usb_get_max_xfer_size(struct usb_device *udev, size_t *size) return ops->get_max_xfer_size(bus, size); }
+int __usb_stop(void) +{ + struct udevice *bus; + struct udevice *rh; + struct uclass *uc; + struct usb_uclass_priv *uc_priv; + int err = 0, ret; + + /* De-activate any devices that have been activated */ + ret = uclass_get(UCLASS_USB_DEV_GENERIC, &uc); + if (ret) + return ret; + + uc_priv = uc->priv; + + uclass_foreach_dev(bus, uc) { + ret = device_remove(bus, DM_REMOVE_NORMAL); + if (ret && !err) + err = ret; + + /* Locate root hub device */ + device_find_first_child(bus, &rh); + if (rh) { + /* + * All USB devices are children of root hub. + * Unbinding root hub will unbind all of its children. + */ + ret = device_unbind(rh); + if (ret && !err) + err = ret; + } + } + + uc_priv->companion_device_count = 0; + usb_started = 0; + + return err; +} + int usb_stop(void) { struct udevice *bus; @@ -166,6 +205,10 @@ int usb_stop(void) struct usb_uclass_priv *uc_priv; int err = 0, ret;
+ ret = __usb_stop(); + if (ret) + return ret; + /* De-activate any devices that have been activated */ ret = uclass_get(UCLASS_USB, &uc); if (ret)
Hi Jagan,
On 20 July 2018 at 01:13, Jagan Teki jagan@amarulasolutions.com wrote:
Some OTG controllers which operates on Peripheral mode are registered as UCLASS_USB_DEV_GENERIC.
So add support to shutdown them as well. shutdown happened during 'usb reset' and U-Boot handoff code for Linux boot.
controller restarting in 'usb reset' like probing again is still missing for this type of UCLASS.
Cc: Simon Glass sjg@chromium.org Signed-off-by: Jagan Teki jagan@amarulasolutions.com
Note: I tried of traversing for multiple UCLASS in removal code in usb_stop, but couldn't come with proper solutions. I don't think any other area of code need a requirement like this, or may be I'm missing. any help would appreciate.
Changes for v2:
- none
drivers/usb/host/usb-uclass.c | 43 +++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)
diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c index 611ea97a72..99cf3d2b49 100644 --- a/drivers/usb/host/usb-uclass.c +++ b/drivers/usb/host/usb-uclass.c @@ -158,6 +158,45 @@ int usb_get_max_xfer_size(struct usb_device *udev, size_t *size) return ops->get_max_xfer_size(bus, size); }
+int __usb_stop(void)
Why the __ ? I think it should be something like usb_remove_and_unbind_all()
+{
struct udevice *bus;struct udevice *rh;struct uclass *uc;struct usb_uclass_priv *uc_priv;int err = 0, ret;/* De-activate any devices that have been activated */ret = uclass_get(UCLASS_USB_DEV_GENERIC, &uc);if (ret)return ret;uc_priv = uc->priv;uclass_foreach_dev(bus, uc) {ret = device_remove(bus, DM_REMOVE_NORMAL);if (ret && !err)err = ret;/* Locate root hub device */device_find_first_child(bus, &rh);if (rh) {/** All USB devices are children of root hub.* Unbinding root hub will unbind all of its children.*/ret = device_unbind(rh);if (ret && !err)err = ret;}}uc_priv->companion_device_count = 0;usb_started = 0;return err;+}
int usb_stop(void) { struct udevice *bus; @@ -166,6 +205,10 @@ int usb_stop(void) struct usb_uclass_priv *uc_priv; int err = 0, ret;
ret = __usb_stop();if (ret)return ret;
This looks like the same code that appears below here, or very similar. Why is this needed?
/* De-activate any devices that have been activated */ ret = uclass_get(UCLASS_USB, &uc); if (ret)-- 2.17.1
Regards, Simon
On Tue, Jul 24, 2018 at 5:18 AM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On 20 July 2018 at 01:13, Jagan Teki jagan@amarulasolutions.com wrote:
Some OTG controllers which operates on Peripheral mode are registered as UCLASS_USB_DEV_GENERIC.
So add support to shutdown them as well. shutdown happened during 'usb reset' and U-Boot handoff code for Linux boot.
controller restarting in 'usb reset' like probing again is still missing for this type of UCLASS.
Cc: Simon Glass sjg@chromium.org Signed-off-by: Jagan Teki jagan@amarulasolutions.com
Note: I tried of traversing for multiple UCLASS in removal code in usb_stop, but couldn't come with proper solutions. I don't think any other area of code need a requirement like this, or may be I'm missing. any help would appreciate.
Changes for v2:
- none
drivers/usb/host/usb-uclass.c | 43 +++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)
diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c index 611ea97a72..99cf3d2b49 100644 --- a/drivers/usb/host/usb-uclass.c +++ b/drivers/usb/host/usb-uclass.c @@ -158,6 +158,45 @@ int usb_get_max_xfer_size(struct usb_device *udev, size_t *size) return ops->get_max_xfer_size(bus, size); }
+int __usb_stop(void)
Why the __ ? I think it should be something like usb_remove_and_unbind_all()
+{
struct udevice *bus;struct udevice *rh;struct uclass *uc;struct usb_uclass_priv *uc_priv;int err = 0, ret;/* De-activate any devices that have been activated */ret = uclass_get(UCLASS_USB_DEV_GENERIC, &uc);if (ret)return ret;uc_priv = uc->priv;uclass_foreach_dev(bus, uc) {ret = device_remove(bus, DM_REMOVE_NORMAL);if (ret && !err)err = ret;/* Locate root hub device */device_find_first_child(bus, &rh);if (rh) {/** All USB devices are children of root hub.* Unbinding root hub will unbind all of its children.*/ret = device_unbind(rh);if (ret && !err)err = ret;}}uc_priv->companion_device_count = 0;usb_started = 0;return err;+}
int usb_stop(void) { struct udevice *bus; @@ -166,6 +205,10 @@ int usb_stop(void) struct usb_uclass_priv *uc_priv; int err = 0, ret;
ret = __usb_stop();if (ret)return ret;This looks like the same code that appears below here, or very similar. Why is this needed?
Here the usage-case it to remove/unbind UCLASS_USB and UCLASS_USB_DEV_GENERIC and same code will use to do that, any suggestions?
Hi Jagan,
On 7 August 2018 at 01:03, Jagan Teki jagan@amarulasolutions.com wrote:
On Tue, Jul 24, 2018 at 5:18 AM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On 20 July 2018 at 01:13, Jagan Teki jagan@amarulasolutions.com wrote:
Some OTG controllers which operates on Peripheral mode are registered as UCLASS_USB_DEV_GENERIC.
So add support to shutdown them as well. shutdown happened during 'usb reset' and U-Boot handoff code for Linux boot.
controller restarting in 'usb reset' like probing again is still missing for this type of UCLASS.
Cc: Simon Glass sjg@chromium.org Signed-off-by: Jagan Teki jagan@amarulasolutions.com
Note: I tried of traversing for multiple UCLASS in removal code in usb_stop, but couldn't come with proper solutions. I don't think any other area of code need a requirement like this, or may be I'm missing. any help would appreciate.
Changes for v2:
- none
drivers/usb/host/usb-uclass.c | 43 +++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)
diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c index 611ea97a72..99cf3d2b49 100644 --- a/drivers/usb/host/usb-uclass.c +++ b/drivers/usb/host/usb-uclass.c @@ -158,6 +158,45 @@ int usb_get_max_xfer_size(struct usb_device *udev, size_t *size) return ops->get_max_xfer_size(bus, size); }
+int __usb_stop(void)
Why the __ ? I think it should be something like usb_remove_and_unbind_all()
+{
struct udevice *bus;struct udevice *rh;struct uclass *uc;struct usb_uclass_priv *uc_priv;int err = 0, ret;/* De-activate any devices that have been activated */ret = uclass_get(UCLASS_USB_DEV_GENERIC, &uc);if (ret)return ret;uc_priv = uc->priv;uclass_foreach_dev(bus, uc) {ret = device_remove(bus, DM_REMOVE_NORMAL);if (ret && !err)err = ret;/* Locate root hub device */device_find_first_child(bus, &rh);if (rh) {/** All USB devices are children of root hub.* Unbinding root hub will unbind all of its children.*/ret = device_unbind(rh);if (ret && !err)err = ret;}}uc_priv->companion_device_count = 0;usb_started = 0;return err;+}
int usb_stop(void) { struct udevice *bus; @@ -166,6 +205,10 @@ int usb_stop(void) struct usb_uclass_priv *uc_priv; int err = 0, ret;
ret = __usb_stop();if (ret)return ret;This looks like the same code that appears below here, or very similar. Why is this needed?
Here the usage-case it to remove/unbind UCLASS_USB and UCLASS_USB_DEV_GENERIC and same code will use to do that, any suggestions?
I'm just wondering why you appear to be duplicating the exact code that is already there? Maybe I am missing something, iwc please can you explain it?
Thanks, Simon
Hi Simon,
On Fri, Aug 17, 2018 at 6:18 PM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On 7 August 2018 at 01:03, Jagan Teki jagan@amarulasolutions.com wrote:
On Tue, Jul 24, 2018 at 5:18 AM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On 20 July 2018 at 01:13, Jagan Teki jagan@amarulasolutions.com wrote:
Some OTG controllers which operates on Peripheral mode are registered as UCLASS_USB_DEV_GENERIC.
So add support to shutdown them as well. shutdown happened during 'usb reset' and U-Boot handoff code for Linux boot.
controller restarting in 'usb reset' like probing again is still missing for this type of UCLASS.
Cc: Simon Glass sjg@chromium.org Signed-off-by: Jagan Teki jagan@amarulasolutions.com
Note: I tried of traversing for multiple UCLASS in removal code in usb_stop, but couldn't come with proper solutions. I don't think any other area of code need a requirement like this, or may be I'm missing. any help would appreciate.
Changes for v2:
- none
drivers/usb/host/usb-uclass.c | 43 +++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)
diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c index 611ea97a72..99cf3d2b49 100644 --- a/drivers/usb/host/usb-uclass.c +++ b/drivers/usb/host/usb-uclass.c @@ -158,6 +158,45 @@ int usb_get_max_xfer_size(struct usb_device *udev, size_t *size) return ops->get_max_xfer_size(bus, size); }
+int __usb_stop(void)
Why the __ ? I think it should be something like usb_remove_and_unbind_all()
+{
struct udevice *bus;struct udevice *rh;struct uclass *uc;struct usb_uclass_priv *uc_priv;int err = 0, ret;/* De-activate any devices that have been activated */ret = uclass_get(UCLASS_USB_DEV_GENERIC, &uc);if (ret)return ret;uc_priv = uc->priv;uclass_foreach_dev(bus, uc) {ret = device_remove(bus, DM_REMOVE_NORMAL);if (ret && !err)err = ret;/* Locate root hub device */device_find_first_child(bus, &rh);if (rh) {/** All USB devices are children of root hub.* Unbinding root hub will unbind all of its children.*/ret = device_unbind(rh);if (ret && !err)err = ret;}}uc_priv->companion_device_count = 0;usb_started = 0;return err;+}
int usb_stop(void) { struct udevice *bus; @@ -166,6 +205,10 @@ int usb_stop(void) struct usb_uclass_priv *uc_priv; int err = 0, ret;
ret = __usb_stop();if (ret)return ret;This looks like the same code that appears below here, or very similar. Why is this needed?
Here the usage-case it to remove/unbind UCLASS_USB and UCLASS_USB_DEV_GENERIC and same code will use to do that, any suggestions?
I'm just wondering why you appear to be duplicating the exact code that is already there? Maybe I am missing something, iwc please can you explain it?
Yes, the code is duplicate. Here, I'm looking for suggestion to unbind/remove multiple UCLASS's like UCLASS_USB and ULASS_USB_DEV_GENERIC in this use-case. do you have any? or am I missing something?
Jagan.
Hi Jagan,
On 17 August 2018 at 07:37, Jagan Teki jagan@amarulasolutions.com wrote:
Hi Simon,
On Fri, Aug 17, 2018 at 6:18 PM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On 7 August 2018 at 01:03, Jagan Teki jagan@amarulasolutions.com wrote:
On Tue, Jul 24, 2018 at 5:18 AM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On 20 July 2018 at 01:13, Jagan Teki jagan@amarulasolutions.com wrote:
Some OTG controllers which operates on Peripheral mode are registered as UCLASS_USB_DEV_GENERIC.
So add support to shutdown them as well. shutdown happened during 'usb reset' and U-Boot handoff code for Linux boot.
controller restarting in 'usb reset' like probing again is still missing for this type of UCLASS.
Cc: Simon Glass sjg@chromium.org Signed-off-by: Jagan Teki jagan@amarulasolutions.com
Note: I tried of traversing for multiple UCLASS in removal code in usb_stop, but couldn't come with proper solutions. I don't think any other area of code need a requirement like this, or may be I'm missing. any help would appreciate.
Changes for v2:
- none
drivers/usb/host/usb-uclass.c | 43 +++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)
diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c index 611ea97a72..99cf3d2b49 100644 --- a/drivers/usb/host/usb-uclass.c +++ b/drivers/usb/host/usb-uclass.c @@ -158,6 +158,45 @@ int usb_get_max_xfer_size(struct usb_device *udev, size_t *size) return ops->get_max_xfer_size(bus, size); }
+int __usb_stop(void)
Why the __ ? I think it should be something like usb_remove_and_unbind_all()
+{
struct udevice *bus;struct udevice *rh;struct uclass *uc;struct usb_uclass_priv *uc_priv;int err = 0, ret;/* De-activate any devices that have been activated */ret = uclass_get(UCLASS_USB_DEV_GENERIC, &uc);if (ret)return ret;uc_priv = uc->priv;uclass_foreach_dev(bus, uc) {ret = device_remove(bus, DM_REMOVE_NORMAL);if (ret && !err)err = ret;/* Locate root hub device */device_find_first_child(bus, &rh);if (rh) {/** All USB devices are children of root hub.* Unbinding root hub will unbind all of its children.*/ret = device_unbind(rh);if (ret && !err)err = ret;}}uc_priv->companion_device_count = 0;usb_started = 0;return err;+}
int usb_stop(void) { struct udevice *bus; @@ -166,6 +205,10 @@ int usb_stop(void) struct usb_uclass_priv *uc_priv; int err = 0, ret;
ret = __usb_stop();if (ret)return ret;This looks like the same code that appears below here, or very similar. Why is this needed?
Here the usage-case it to remove/unbind UCLASS_USB and UCLASS_USB_DEV_GENERIC and same code will use to do that, any suggestions?
I'm just wondering why you appear to be duplicating the exact code that is already there? Maybe I am missing something, iwc please can you explain it?
Yes, the code is duplicate. Here, I'm looking for suggestion to unbind/remove multiple UCLASS's like UCLASS_USB and ULASS_USB_DEV_GENERIC in this use-case. do you have any? or am I missing something?
But if you unbind the bus, doesn't this automatically unbind its children?
At the very least, it seems like there is some common code here that you should factor out.
Regards, Simon
reset0 is not available for sun4i, 5i and 7i so access the reset0 offset from ccm via driver data for relevant Allwinner SoC. this will eventually drop the existing ifdef for SUN6I.
Signed-off-by: Jagan Teki jagan@amarulasolutions.com --- Changes for v2: - new patch
drivers/usb/musb-new/sunxi.c | 40 +++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c index aa2880eeb9..d909b9ea53 100644 --- a/drivers/usb/musb-new/sunxi.c +++ b/drivers/usb/musb-new/sunxi.c @@ -76,15 +76,20 @@ * From usbc/usbc.c ******************************************************************************/
+#define OFF_SUN6I_AHB_RESET0 0x2c0 + struct sunxi_musb_config { struct musb_hdrc_config *config; + bool has_reset; u8 rst_bit; u8 clkgate_bit; + u32 off_reset0; };
struct sunxi_glue { struct musb_host_data mdata; struct sunxi_ccm_reg *ccm; + u32 *reg_reset0; struct sunxi_musb_config *cfg; struct device dev; struct phy phy; @@ -303,12 +308,12 @@ static int sunxi_musb_init(struct musb *musb) if (glue->cfg->clkgate_bit) setbits_le32(&glue->ccm->ahb_gate0, BIT(glue->cfg->clkgate_bit)); -#ifdef CONFIG_SUNXI_GEN_SUN6I - setbits_le32(&glue->ccm->ahb_reset0_cfg, BIT(AHB_GATE_OFFSET_USB0)); + + if (glue->cfg->has_reset) + setbits_le32(glue->reg_reset0, BIT(AHB_GATE_OFFSET_USB0)); + if (glue->cfg->rst_bit) - setbits_le32(&glue->ccm->ahb_reset0_cfg, - BIT(glue->cfg->rst_bit)); -#endif + setbits_le32(glue->reg_reset0, BIT(glue->cfg->rst_bit));
USBC_ConfigFIFO_Base(); USBC_EnableDpDmPullUp(musb->mregs); @@ -418,6 +423,8 @@ static int musb_usb_probe(struct udevice *dev) if (IS_ERR(glue->ccm)) return PTR_ERR(glue->ccm);
+ glue->reg_reset0 = (void *)glue->ccm + glue->cfg->off_reset0; + ret = generic_phy_get_by_name(dev, "usb", &glue->phy); if (ret) { pr_err("failed to get usb PHY\n"); @@ -468,12 +475,12 @@ static int musb_usb_remove(struct udevice *dev)
musb_stop(host->host);
-#ifdef CONFIG_SUNXI_GEN_SUN6I - clrbits_le32(&glue->ccm->ahb_reset0_cfg, BIT(AHB_GATE_OFFSET_USB0)); + if (glue->cfg->has_reset) + clrbits_le32(glue->reg_reset0, BIT(AHB_GATE_OFFSET_USB0)); + if (glue->cfg->rst_bit) - clrbits_le32(&glue->ccm->ahb_reset0_cfg, - BIT(glue->cfg->rst_bit)); -#endif + clrbits_le32(glue->reg_reset0, BIT(glue->cfg->rst_bit)); + clrbits_le32(&glue->ccm->ahb_gate0, BIT(AHB_GATE_OFFSET_USB0)); if (glue->cfg->clkgate_bit) clrbits_le32(&glue->ccm->ahb_gate0, @@ -487,21 +494,30 @@ static int musb_usb_remove(struct udevice *dev)
static const struct sunxi_musb_config sun4i_a10_cfg = { .config = &musb_config, + .has_reset = false, +}; + +static const struct sunxi_musb_config sun6i_a31_cfg = { + .config = &musb_config, + .has_reset = true, + .off_reset0 = OFF_SUN6I_AHB_RESET0, };
static const struct sunxi_musb_config sun8i_h3_cfg = { .config = &musb_config_h3, + .has_reset = true, .rst_bit = 23, .clkgate_bit = 23, + .off_reset0 = OFF_SUN6I_AHB_RESET0, };
static const struct udevice_id sunxi_musb_ids[] = { { .compatible = "allwinner,sun4i-a10-musb", .data = (ulong)&sun4i_a10_cfg }, { .compatible = "allwinner,sun6i-a31-musb", - .data = (ulong)&sun4i_a10_cfg }, + .data = (ulong)&sun6i_a31_cfg }, { .compatible = "allwinner,sun8i-a33-musb", - .data = (ulong)&sun4i_a10_cfg }, + .data = (ulong)&sun6i_a31_cfg }, { .compatible = "allwinner,sun8i-h3-musb", .data = (ulong)&sun8i_h3_cfg }, { }
musb have platform ops to do proper graceful exit, so add the exit call and move musb platform exit code instead of keeping it in driver remove. This make proper shutdown of musb where .remove will call disable, exit serially via musb_stop.
Signed-off-by: Jagan Teki jagan@amarulasolutions.com --- Changes for v2: - use driver data for sun6i changes
drivers/usb/musb-new/sunxi.c | 49 ++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 21 deletions(-)
diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c index d909b9ea53..6cf9826cda 100644 --- a/drivers/usb/musb-new/sunxi.c +++ b/drivers/usb/musb-new/sunxi.c @@ -331,6 +331,33 @@ static int sunxi_musb_init(struct musb *musb) return 0; }
+static int sunxi_musb_exit(struct musb *musb) +{ + struct sunxi_glue *glue = to_sunxi_glue(musb->controller); + int ret = 0; + + if (generic_phy_valid(&glue->phy)) { + ret = generic_phy_exit(&glue->phy); + if (ret) { + dev_err(dev, "failed to power off usb phy\n"); + return ret; + } + } + + if (glue->cfg->has_reset) + clrbits_le32(glue->reg_reset0, BIT(AHB_GATE_OFFSET_USB0)); + + if (glue->cfg->rst_bit) + clrbits_le32(glue->reg_reset0, BIT(glue->cfg->rst_bit)); + + clrbits_le32(&glue->ccm->ahb_gate0, BIT(AHB_GATE_OFFSET_USB0)); + if (glue->cfg->clkgate_bit) + clrbits_le32(&glue->ccm->ahb_gate0, + BIT(glue->cfg->clkgate_bit)); + + return 0; +} + static void sunxi_musb_pre_root_reset_end(struct musb *musb) { struct sunxi_glue *glue = to_sunxi_glue(musb->controller); @@ -347,6 +374,7 @@ static void sunxi_musb_post_root_reset_end(struct musb *musb)
static const struct musb_platform_ops sunxi_musb_ops = { .init = sunxi_musb_init, + .exit = sunxi_musb_exit, .enable = sunxi_musb_enable, .disable = sunxi_musb_disable, .pre_root_reset_end = sunxi_musb_pre_root_reset_end, @@ -463,29 +491,8 @@ static int musb_usb_remove(struct udevice *dev) { struct sunxi_glue *glue = dev_get_priv(dev); struct musb_host_data *host = &glue->mdata; - int ret; - - if (generic_phy_valid(&glue->phy)) { - ret = generic_phy_exit(&glue->phy); - if (ret) { - pr_err("failed to exit %s USB PHY\n", dev->name); - return ret; - } - }
musb_stop(host->host); - - if (glue->cfg->has_reset) - clrbits_le32(glue->reg_reset0, BIT(AHB_GATE_OFFSET_USB0)); - - if (glue->cfg->rst_bit) - clrbits_le32(glue->reg_reset0, BIT(glue->cfg->rst_bit)); - - clrbits_le32(&glue->ccm->ahb_gate0, BIT(AHB_GATE_OFFSET_USB0)); - if (glue->cfg->clkgate_bit) - clrbits_le32(&glue->ccm->ahb_gate0, - BIT(glue->cfg->clkgate_bit)); - free(host->host); host->host = NULL;
musb stop is musb core call during unregister or shutting down gadget or host musb. For graceful exit add musb_platform_exit on musb_stop so-that it can exit the musb platform driver as well.
Signed-off-by: Jagan Teki jagan@amarulasolutions.com --- Changes for v2: - update proper commit message
drivers/usb/musb-new/musb_core.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/usb/musb-new/musb_core.c b/drivers/usb/musb-new/musb_core.c index 8fec6f38ad..afea9fbcef 100644 --- a/drivers/usb/musb-new/musb_core.c +++ b/drivers/usb/musb-new/musb_core.c @@ -1007,6 +1007,7 @@ void musb_stop(struct musb *musb) * - ... */ musb_platform_try_idle(musb, 0); + musb_platform_exit(musb); }
#ifndef __UBOOT__
On 07/20/2018 09:13 AM, Jagan Teki wrote:
This is v2 for previous series[1], by excluding sunxi phy changes.
One patch mark it as 'RFC' about "including UCLASS_USB_DEV_GENERIC into shutdown caller" so expecting some inputs on the same
[1] https://patchwork.ozlabs.org/cover/941588/
Jagan Teki (6): usb: musb-new: Fix improper musb host pointer usb: musb-new: sunxi: Allocate struct phy in private dm: usb: Add UCLASS_USB_DEV_GENERIC shutdown musb-new: sunxi: Access ahb_reset0_cfg via ccm offset usb: musb-new: sunxi: Add proper musb exit support usb: musb-new: Call musb_platform_exit from musb_stop
board/compulab/cm_t3517/cm_t3517.c | 4 +- drivers/usb/host/usb-uclass.c | 43 ++++++++++++ drivers/usb/musb-new/musb_core.c | 1 + drivers/usb/musb-new/musb_uboot.c | 12 ++-- drivers/usb/musb-new/pic32.c | 6 +- drivers/usb/musb-new/sunxi.c | 107 ++++++++++++++++++----------- include/linux/usb/musb.h | 4 +- 7 files changed, 123 insertions(+), 54 deletions(-)
+CC Vasily, I want his AB/TB on this. Please keep him CCed on those USB patches.
On Fri, Jul 20, 2018 at 3:13 PM, Jagan Teki jagan@amarulasolutions.com wrote:
This is v2 for previous series[1], by excluding sunxi phy changes.
One patch mark it as 'RFC' about "including UCLASS_USB_DEV_GENERIC into shutdown caller" so expecting some inputs on the same
[1] https://patchwork.ozlabs.org/cover/941588/
Jagan Teki (6): usb: musb-new: Fix improper musb host pointer usb: musb-new: sunxi: Allocate struct phy in private dm: usb: Add UCLASS_USB_DEV_GENERIC shutdown musb-new: sunxi: Access ahb_reset0_cfg via ccm offset usb: musb-new: sunxi: Add proper musb exit support usb: musb-new: Call musb_platform_exit from musb_stop
board/compulab/cm_t3517/cm_t3517.c | 4 +- drivers/usb/host/usb-uclass.c | 43 ++++++++++++ drivers/usb/musb-new/musb_core.c | 1 + drivers/usb/musb-new/musb_uboot.c | 12 ++-- drivers/usb/musb-new/pic32.c | 6 +- drivers/usb/musb-new/sunxi.c | 107 ++++++++++++++++++----------- include/linux/usb/musb.h | 4 +- 7 files changed, 123 insertions(+), 54 deletions(-)
-- 2.17.1
Tested-by: Chen-Yu Tsai wens@csie.org
on A33-OlinuXino. This fixes the abort when starting the boot sequence.
On Mon, Aug 13, 2018 at 4:33 PM, Chen-Yu Tsai wens@csie.org wrote:
On Fri, Jul 20, 2018 at 3:13 PM, Jagan Teki jagan@amarulasolutions.com wrote:
This is v2 for previous series[1], by excluding sunxi phy changes.
One patch mark it as 'RFC' about "including UCLASS_USB_DEV_GENERIC into shutdown caller" so expecting some inputs on the same
[1] https://patchwork.ozlabs.org/cover/941588/
Jagan Teki (6): usb: musb-new: Fix improper musb host pointer usb: musb-new: sunxi: Allocate struct phy in private dm: usb: Add UCLASS_USB_DEV_GENERIC shutdown musb-new: sunxi: Access ahb_reset0_cfg via ccm offset usb: musb-new: sunxi: Add proper musb exit support usb: musb-new: Call musb_platform_exit from musb_stop
board/compulab/cm_t3517/cm_t3517.c | 4 +- drivers/usb/host/usb-uclass.c | 43 ++++++++++++ drivers/usb/musb-new/musb_core.c | 1 + drivers/usb/musb-new/musb_uboot.c | 12 ++-- drivers/usb/musb-new/pic32.c | 6 +- drivers/usb/musb-new/sunxi.c | 107 ++++++++++++++++++----------- include/linux/usb/musb.h | 4 +- 7 files changed, 123 insertions(+), 54 deletions(-)
-- 2.17.1
Tested-by: Chen-Yu Tsai wens@csie.org
on A33-OlinuXino. This fixes the abort when starting the boot sequence.
Does it fixes w/o 3/6? because it an RFC and still in discussion
On Fri, Jul 20, 2018 at 12:43 PM, Jagan Teki jagan@amarulasolutions.com wrote:
This is v2 for previous series[1], by excluding sunxi phy changes.
One patch mark it as 'RFC' about "including UCLASS_USB_DEV_GENERIC into shutdown caller" so expecting some inputs on the same
[1] https://patchwork.ozlabs.org/cover/941588/
Jagan Teki (6): usb: musb-new: Fix improper musb host pointer usb: musb-new: sunxi: Allocate struct phy in private dm: usb: Add UCLASS_USB_DEV_GENERIC shutdown musb-new: sunxi: Access ahb_reset0_cfg via ccm offset usb: musb-new: sunxi: Add proper musb exit support usb: musb-new: Call musb_platform_exit from musb_stop
I'm planning to apply by excluding RFC patch, let me know if anyone has any inputs.
Jagan.
On Tue, Aug 21, 2018 at 9:04 PM Jagan Teki jagan@amarulasolutions.com wrote:
On Fri, Jul 20, 2018 at 12:43 PM, Jagan Teki jagan@amarulasolutions.com wrote:
This is v2 for previous series[1], by excluding sunxi phy changes.
One patch mark it as 'RFC' about "including UCLASS_USB_DEV_GENERIC into shutdown caller" so expecting some inputs on the same
[1] https://patchwork.ozlabs.org/cover/941588/
Jagan Teki (6): usb: musb-new: Fix improper musb host pointer usb: musb-new: sunxi: Allocate struct phy in private dm: usb: Add UCLASS_USB_DEV_GENERIC shutdown musb-new: sunxi: Access ahb_reset0_cfg via ccm offset usb: musb-new: sunxi: Add proper musb exit support usb: musb-new: Call musb_platform_exit from musb_stop
I'm planning to apply by excluding RFC patch, let me know if anyone has any inputs.
Boots cleanly without the RFC patch on the Bananapi M2 Magic, with musb-new built in both host and gadget mode.
Curiously, both modes can be enabled at the same time in Kconfig, but host mode takes precedence in the actual operation.
ChenYu
On Wed, Aug 22, 2018 at 8:11 AM, Chen-Yu Tsai wens@csie.org wrote:
On Tue, Aug 21, 2018 at 9:04 PM Jagan Teki jagan@amarulasolutions.com wrote:
On Fri, Jul 20, 2018 at 12:43 PM, Jagan Teki jagan@amarulasolutions.com wrote:
This is v2 for previous series[1], by excluding sunxi phy changes.
One patch mark it as 'RFC' about "including UCLASS_USB_DEV_GENERIC into shutdown caller" so expecting some inputs on the same
[1] https://patchwork.ozlabs.org/cover/941588/
Jagan Teki (6): usb: musb-new: Fix improper musb host pointer usb: musb-new: sunxi: Allocate struct phy in private dm: usb: Add UCLASS_USB_DEV_GENERIC shutdown musb-new: sunxi: Access ahb_reset0_cfg via ccm offset usb: musb-new: sunxi: Add proper musb exit support usb: musb-new: Call musb_platform_exit from musb_stop
I'm planning to apply by excluding RFC patch, let me know if anyone has any inputs.
Boots cleanly without the RFC patch on the Bananapi M2 Magic, with musb-new built in both host and gadget mode.
Thanks.
Collected Tested-by and Applied to u-boot-sunxi/master
Curiously, both modes can be enabled at the same time in Kconfig, but host mode takes precedence in the actual operation.
Look like Yes. I think we can grab dr_mode to set respective mode.
participants (4)
-
Chen-Yu Tsai -
Jagan Teki -
Marek Vasut -
Simon Glass