musb: sunxi: Few fixes/improvements

PATCH Superseded Jagan Teki 6 participants 18 messages Jul 9, 2018 – Jul 17, 2018

Subsystems (3)

USB Arm/Allwinner sunXi SoC support Generic PHY framework

lore

Share & tools

lore

Jagan Teki

author

2018-07-09 19:17 UTC #0 lore raw

cover musb: sunxi: Few fixes/improvements

All these changes mostly related to musb/sunxi.c and msub core files with few fixes on sunxi front and some code improvements on musb while handling shutdown code.

One patch mark it as 'RFC' about "including UCLASS_USB_DEV_GENERIC into shutdown caller" so expecting some inputs on the same.

patch 1, fix improper musb host pointer

patch 2, musb-new/sunxi allocate phy in private

patch 3, use phy_passby even for PHY#0

patch 4, don't set usb_clk_cfg in PHY probe

patch 5, update PHY#4 rst_mask for H3_H5

patch 6, shutdown UCLASS_USB_DEV_GENERIC

patch 7, musb-new/sunxi add proper exit

patch 8, call musb_platform_exit from musb_stop

Jagan Teki (8):
usb: musb-new: Fix improper musb host pointer
usb: musb-new: sunxi: Allocate struct phy in private
phy: sun4i-usb: Call phy_passby even for PHY#0
phy: sun4i-usb: Remove usb_clk_cfg set in probe
phy: sun4i-usb: Update PHY#3 rst_mask only for H3_H5
dm: usb: Add UCLASS_USB_DEV_GENERIC shutdown
usb: musb-new: sunxi: Add proper musb exit support
usb: musb-new: core: Call musb_platform_exit from musb_stop

 board/compulab/cm_t3517/cm_t3517.c    |  4 +-
 drivers/phy/allwinner/phy-sun4i-usb.c |  7 +--
 drivers/usb/host/usb-uclass.c         | 43 +++++++++++++++
 drivers/usb/musb-new/musb_core.c      |  1 +
 drivers/usb/musb-new/musb_uboot.c     | 10 ++--
 drivers/usb/musb-new/pic32.c          |  6 ++-
 drivers/usb/musb-new/sunxi.c          | 76 +++++++++++++++------------
 include/linux/usb/musb.h              |  4 +-
 8 files changed, 101 insertions(+), 50 deletions(-)

Signature · 1 line

2.17.1

Jagan Teki

author

2018-07-09 19:17 UTC #1 lore raw

PATCH 1/8 usb: musb-new: Fix improper musb host pointer

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.

Signed-off-by Jagan Teki <[email protected]>

---
 board/compulab/cm_t3517/cm_t3517.c |  4 ++--
 drivers/usb/musb-new/musb_uboot.c  | 10 +++++-----
 drivers/usb/musb-new/pic32.c       |  6 ++++--
 drivers/usb/musb-new/sunxi.c       |  8 +++++---
 include/linux/usb/musb.h           |  4 ++--
 5 files changed, 18 insertions(+), 14 deletions(-)

board/compulab/cm_t3517/cm_t3517.c +2 -2

index 09cb27def0..668bb7631a 100644
@@ -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

drivers/usb/musb-new/musb_uboot.c +5 -5

index 2b04fbd046..3cd89793c9 100644
@@ -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 NULL;
 	}
 
 	*musbp = musb_init_controller(plat, (struct device *)bdata, ctl_regs);
 	if (!*musbp) {
 		printf("Failed to init the controller\n");
-		return -EIO;
+		return NULL;
 	}
 
-	return 0;
+	return *musbp;
 }

drivers/usb/musb-new/pic32.c +4 -2

index f04719d7af..3a19900e21 100644
@@ -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;

drivers/usb/musb-new/sunxi.c +5 -3

index 08de9c69c7..b846afb094 100644
@@ -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;

include/linux/usb/musb.h +2 -2

index 9104414cf0..a31ce67a81 100644
@@ -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 */

Signature · 1 line

2.17.1

Julian Calaby

2018-07-17 12:45 UTC #2 lore raw

PATCH 1/8 usb: musb-new: Fix improper musb host pointer

Hi Jagan,

Quoted 21 lines · Jagan Teki

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.
Signed-off-by Jagan Teki <[email protected]>
---
 board/compulab/cm_t3517/cm_t3517.c |  4 ++--
 drivers/usb/musb-new/musb_uboot.c  | 10 +++++-----
 drivers/usb/musb-new/pic32.c       |  6 ++++--
 drivers/usb/musb-new/sunxi.c       |  8 +++++---
 include/linux/usb/musb.h           |  4 ++--
 5 files changed, 18 insertions(+), 14 deletions(-)
drivers/usb/musb-new/musb_uboot.c +0 -0
index 2b04fbd046..3cd89793c9 100644

drivers/usb/musb-new/musb_uboot.c int musb_register(struct musb_hdrc_platform_data *plat, void *bdata, · line 439

Quoted 5 lines

drivers/usb/musb-new/musb_uboot.c +1 -1

@@ -436,14 +436,14 @@ int musb_register(struct musb_hdrc_platform_data *plat, void *bdata,
                break;
 #endif
        default:
-               return -EINVAL;
+               return NULL;

These error constants appear to have been chosen specifically, so would it be nicer to preserve them and use ERR_PTR() and friends to return them?

(I know nothing checks them today, but they might be useful in the future)

drivers/usb/musb-new/musb_uboot.c int musb_register(struct musb_hdrc_platform_data *plat, void *bdata, · line 449

Quoted 12 lines

drivers/usb/musb-new/musb_uboot.c +2 -2

        }
 
        *musbp = musb_init_controller(plat, (struct device *)bdata, ctl_regs);
        if (!*musbp) {
                printf("Failed to init the controller\n");
-               return -EIO;
+               return NULL;
        }
 
-       return 0;
+       return *musbp;
 }

Thanks,

Signature · 4 lines

Julian Calaby

Email: julian.calaby at gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Julian Calaby

2018-07-17 12:48 UTC #3 lore raw

PATCH 1/8 usb: musb-new: Fix improper musb host pointer

Hi,

Sorry, please ignore this, I didn't realise this patch was for u-boot.

Quoted 35 lines · Julian Calaby

Hi Jagan,
Quoted 21 lines · Jagan Teki
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.
Signed-off-by Jagan Teki <[email protected]>
---
 board/compulab/cm_t3517/cm_t3517.c |  4 ++--
 drivers/usb/musb-new/musb_uboot.c  | 10 +++++-----
 drivers/usb/musb-new/pic32.c       |  6 ++++--
 drivers/usb/musb-new/sunxi.c       |  8 +++++---
 include/linux/usb/musb.h           |  4 ++--
 5 files changed, 18 insertions(+), 14 deletions(-)
drivers/usb/musb-new/musb_uboot.c +0 -0
index 2b04fbd046..3cd89793c9 100644
drivers/usb/musb-new/musb_uboot.c int musb_register(struct musb_hdrc_platform_data *plat, void *bdata, · line 439
Quoted 5 lines
drivers/usb/musb-new/musb_uboot.c +1 -1
@@ -436,14 +436,14 @@ int musb_register(struct musb_hdrc_platform_data *plat, void *bdata,
                break;
 #endif
        default:
-               return -EINVAL;
+               return NULL;
These error constants appear to have been chosen specifically, so would it be nicer to preserve them and use ERR_PTR() and friends to return them?

Quoted 14 lines

(I know nothing checks them today, but they might be useful in the future)

Quoted 12 lines

drivers/usb/musb-new/musb_uboot.c +2 -2

        }
 
        *musbp = musb_init_controller(plat, (struct device *)bdata, ctl_regs);
        if (!*musbp) {
                printf("Failed to init the controller\n");
-               return -EIO;
+               return NULL;
        }
 
-       return 0;
+       return *musbp;
 }

Thanks,

Signature · 4 lines

Julian Calaby

Email: julian.calaby at gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Jagan Teki

author

2018-07-09 19:17 UTC #4 lore raw

PATCH 2/8 usb: musb-new: sunxi: Allocate struct phy in private

In reply to Jagan Teki

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 <[email protected]>

---
 drivers/usb/musb-new/sunxi.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

drivers/usb/musb-new/sunxi.c +11 ~~-13~~

index b846afb094..aa2880eeb9 100644
@@ -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;

Signature · 1 line

2.17.1

Jagan Teki

author

2018-07-09 19:17 UTC #5 lore raw

PATCH 3/8 phy: sun4i-usb: Call phy_passby even for PHY#0

In reply to Jagan Teki

phy_passby is not calling for phy id 0 in legacy code, which has no information on why it restricted. Since Linux phy driver doesn't restrict like this, so call it directly with out any condition

Signed-off-by Jagan Teki <[email protected]>

---
 drivers/phy/allwinner/phy-sun4i-usb.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/phy/allwinner/phy-sun4i-usb.c +1 -2

index 2b3cf48025..01f585a283 100644
@@ -300,8 +300,7 @@ static int sun4i_usb_phy_init(struct phy *phy)
 				    data->cfg->disc_thresh, PHY_DISCON_TH_LEN);
 	}
 
-	if (usb_phy->id != 0)
-		sun4i_usb_phy_passby(phy, true);
+	sun4i_usb_phy_passby(phy, true);
 
 	sun4i_usb_phy0_reroute(data, true);

Signature · 1 line

2.17.1

Chen-Yu Tsai

2018-07-10 02:30 UTC #6 lore raw

PATCH 3/8 phy: sun4i-usb: Call phy_passby even for PHY#0

Quoted 4 lines · Jagan Teki

phy_passby is not calling for phy id 0 in legacy code, which has no information on why it restricted. Since Linux phy driver doesn't restrict like this, so call it directly with out any condition

See

https://github.com/wens/u-boot-sunxi/commit/b1d3cb2973e75097bbee1835efdc07afb8a7ef2f

for a better commit message. This is an older version. I tried rebasing on the latest version, but apparently there are merge conflicts between master and sunxi/next. Even worse, it fails to boot properly.

Please try to understand why the code is what it is. The reason we started out with not calling it for PHY 0 is because in the past USB0 did not have EHCI/OHCI host pairs, which in turn means there's no "pmu" (whatever that is) register associated with that PHY.

In time, the check for the pointer to that register was moved into sun4i_usb_phy_passby() itself, which directly returns if that pointer is not set. And the PHY driver only sets that pointer if a "pmuX" resource for X PHY is provided.

ChenYu

Quoted 22 lines

Signed-off-by Jagan Teki <[email protected]>

---
 drivers/phy/allwinner/phy-sun4i-usb.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/phy/allwinner/phy-sun4i-usb.c +1 -3

index 2b3cf48025..01f585a283 100644
@@ -300,8 +300,7 @@ static int sun4i_usb_phy_init(struct phy *phy)
                                    data->cfg->disc_thresh, PHY_DISCON_TH_LEN);
        }
 
-       if (usb_phy->id != 0)
-               sun4i_usb_phy_passby(phy, true);
+       sun4i_usb_phy_passby(phy, true);
 
        sun4i_usb_phy0_reroute(data, true);
 
--

2.17.1

Jagan Teki

author

2018-07-09 19:17 UTC #7 lore raw

PATCH 4/8 phy: sun4i-usb: Remove usb_clk_cfg set in probe

In reply to Jagan Teki

usb_clk_cfg is setting CTRL_PHYGATE bit value in probe which is BIT 0 for sun4i, 6i and 8 for a83t but all these were handling in phy ops init exit calls.

Signed-off-by Jagan Teki <[email protected]>

---
 drivers/phy/allwinner/phy-sun4i-usb.c | 2 --
 1 file changed, 2 deletions(-)

drivers/phy/allwinner/phy-sun4i-usb.c +0 -2

index 01f585a283..3096f12c1c 100644
@@ -462,8 +462,6 @@ static int sun4i_usb_phy_probe(struct udevice *dev)
 		phy->rst_mask = info->rst_mask;
 	};
 
-	setbits_le32(&data->ccm->usb_clk_cfg, CCM_USB_CTRL_PHYGATE);
-
 	debug("Allwinner Sun4I USB PHY driver loaded\n");
 	return 0;
 }

Signature · 1 line

2.17.1

Jagan Teki

author

2018-07-09 19:17 UTC #8 lore raw

PATCH 5/8 phy: sun4i-usb: Update PHY#3 rst_mask only for H3_H5

In reply to Jagan Teki

Only H3 and H5 have 4 PHYS so restrict rst_mask only for them.

Signed-off-by Jagan Teki <[email protected]>

---
 drivers/phy/allwinner/phy-sun4i-usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/phy/allwinner/phy-sun4i-usb.c +1 -1

index 3096f12c1c..81fcd1f910 100644
@@ -117,7 +117,7 @@ struct sun4i_usb_phy_info {
 		.gpio_vbus = CONFIG_USB3_VBUS_PIN,
 		.gpio_vbus_det = NULL,
 		.gpio_id_det = NULL,
-#ifdef CONFIG_MACH_SUN6I
+#ifdef CONFIG_MACH_SUNXI_H3_H5
 		.rst_mask = (CCM_USB_CTRL_PHY3_RST | CCM_USB_CTRL_PHY3_CLK),
 #endif
 	},

Signature · 1 line

2.17.1

Vasily Khoruzhick

2018-07-11 06:53 UTC #9 lore raw

PATCH 5/8 phy: sun4i-usb: Update PHY#3 rst_mask only for H3_H5

Quoted 12 lines · Jagan Teki

Only H3 and H5 have 4 PHYS so restrict rst_mask only for them.
Signed-off-by Jagan Teki <[email protected]>
---
 drivers/phy/allwinner/phy-sun4i-usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/phy/allwinner/phy-sun4i-usb.c +0 -0
index 3096f12c1c..81fcd1f910 100644

drivers/phy/allwinner/phy-sun4i-usb.c struct sun4i_usb_phy_info { · line 120

Quoted 5 lines

drivers/phy/allwinner/phy-sun4i-usb.c +1 -1

@@ -117,7 +117,7 @@ struct sun4i_usb_phy_info {
                .gpio_vbus = CONFIG_USB3_VBUS_PIN,
                .gpio_vbus_det = NULL,
                .gpio_id_det = NULL,
-#ifdef CONFIG_MACH_SUN6I
+#ifdef CONFIG_MACH_SUNXI_H3_H5

It should be handled by compatibles, not ifdefs. Phy driver is proper DM driver, so you can use .data in device ids array.

Quoted 10 lines

drivers/phy/allwinner/phy-sun4i-usb.c +0 -1
                .rst_mask = (CCM_USB_CTRL_PHY3_RST | CCM_USB_CTRL_PHY3_CLK),
 #endif
        },
--
2.17.1
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
https://lists.denx.de/listinfo/u-boot

Jagan Teki

2018-07-11 07:07 UTC #10 lore raw

PATCH 5/8 phy: sun4i-usb: Update PHY#3 rst_mask only for H3_H5

Quoted 8 lines · Vasily Khoruzhick

Quoted 7 lines · Jagan Teki
Only H3 and H5 have 4 PHYS so restrict rst_mask only for them.
Signed-off-by Jagan Teki <[email protected]>
---
 drivers/phy/allwinner/phy-sun4i-usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 13 lines

drivers/phy/allwinner/phy-sun4i-usb.c struct sun4i_usb_phy_info { · line 120
Quoted 10 lines
drivers/phy/allwinner/phy-sun4i-usb.c +1 -1
index 3096f12c1c..81fcd1f910 100644
@@ -117,7 +117,7 @@ struct sun4i_usb_phy_info {
                .gpio_vbus = CONFIG_USB3_VBUS_PIN,
                .gpio_vbus_det = NULL,
                .gpio_id_det = NULL,
-#ifdef CONFIG_MACH_SUN6I
+#ifdef CONFIG_MACH_SUNXI_H3_H5
It should be handled by compatibles, not ifdefs. Phy driver is proper DM driver, so you can use .data in device ids array.

True, but since it' already have ifdef and more over this code will drop very soon, we are planning to support CLK and RST[1] (max of v2018.11).

[1] https://github.com/amarula/u-boot-amarula/commits/sun-dev

Maxime Ripard

2018-07-11 07:14 UTC #11 lore raw

PATCH 5/8 phy: sun4i-usb: Update PHY#3 rst_mask only for H3_H5

Quoted 23 lines · Jagan Teki

Quoted 8 lines · Vasily Khoruzhick
Quoted 7 lines · Jagan Teki
Only H3 and H5 have 4 PHYS so restrict rst_mask only for them.
Signed-off-by Jagan Teki <[email protected]>
---
 drivers/phy/allwinner/phy-sun4i-usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 13 lines
drivers/phy/allwinner/phy-sun4i-usb.c struct sun4i_usb_phy_info { · line 120
Quoted 10 lines
drivers/phy/allwinner/phy-sun4i-usb.c +1 -1
index 3096f12c1c..81fcd1f910 100644
@@ -117,7 +117,7 @@ struct sun4i_usb_phy_info {
                .gpio_vbus = CONFIG_USB3_VBUS_PIN,
                .gpio_vbus_det = NULL,
                .gpio_id_det = NULL,
-#ifdef CONFIG_MACH_SUN6I
+#ifdef CONFIG_MACH_SUNXI_H3_H5
It should be handled by compatibles, not ifdefs. Phy driver is proper DM driver, so you can use .data in device ids array.

Quoted 3 lines

True, but since it' already have ifdef and more over this code will drop very soon, we are planning to support CLK and RST[1] (max of v2018.11).

Says who?

I'd really prefer to have something when it's ready, rather than trying to push to meet a deadline that we enforced on ourselves, and end up with a tons of bugs to fix. The latest PHY patches should be a pretty good example on why we *shouldn't* do that.

Maxime

Signature · 10 lines

Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180711/05fbe719/attachment.sig>

Jagan Teki

2018-07-11 07:22 UTC #12 lore raw

PATCH 5/8 phy: sun4i-usb: Update PHY#3 rst_mask only for H3_H5

Quoted 24 lines · Maxime Ripard

Quoted 23 lines · Jagan Teki
Quoted 21 lines · Vasily Khoruzhick
Quoted 12 lines · Jagan Teki
Only H3 and H5 have 4 PHYS so restrict rst_mask only for them.
Signed-off-by Jagan Teki <[email protected]>
---
 drivers/phy/allwinner/phy-sun4i-usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/phy/allwinner/phy-sun4i-usb.c +0 -0
index 3096f12c1c..81fcd1f910 100644
drivers/phy/allwinner/phy-sun4i-usb.c struct sun4i_usb_phy_info { · line 120
Quoted 5 lines
drivers/phy/allwinner/phy-sun4i-usb.c +1 -1
@@ -117,7 +117,7 @@ struct sun4i_usb_phy_info {
                .gpio_vbus = CONFIG_USB3_VBUS_PIN,
                .gpio_vbus_det = NULL,
                .gpio_id_det = NULL,
-#ifdef CONFIG_MACH_SUN6I
+#ifdef CONFIG_MACH_SUNXI_H3_H5
It should be handled by compatibles, not ifdefs. Phy driver is proper DM driver, so you can use .data in device ids array.

Quoted 5 lines

Quoted 3 lines
True, but since it' already have ifdef and more over this code will drop very soon, we are planning to support CLK and RST[1] (max of v2018.11).
Says who?

Me with USB clocks and resets.

Quoted 5 lines

I'd really prefer to have something when it's ready, rather than trying to push to meet a deadline that we enforced on ourselves, and end up with a tons of bugs to fix. The latest PHY patches should be a pretty good example on why we *shouldn't* do that.

These series fixes not certainly related to sunxi because of recent changes, were there in musb shutdown code and PHY changes were there before with legacy.

Maxime Ripard

2018-07-11 09:59 UTC #13 lore raw

PATCH 5/8 phy: sun4i-usb: Update PHY#3 rst_mask only for H3_H5

Quoted 34 lines · Jagan Teki

Quoted 24 lines · Maxime Ripard
Quoted 23 lines · Jagan Teki
Quoted 21 lines · Vasily Khoruzhick
Quoted 12 lines · Jagan Teki
Only H3 and H5 have 4 PHYS so restrict rst_mask only for them.
Signed-off-by Jagan Teki <[email protected]>
---
 drivers/phy/allwinner/phy-sun4i-usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/phy/allwinner/phy-sun4i-usb.c +0 -0
index 3096f12c1c..81fcd1f910 100644
drivers/phy/allwinner/phy-sun4i-usb.c struct sun4i_usb_phy_info { · line 120
Quoted 5 lines
drivers/phy/allwinner/phy-sun4i-usb.c +1 -1
@@ -117,7 +117,7 @@ struct sun4i_usb_phy_info {
                .gpio_vbus = CONFIG_USB3_VBUS_PIN,
                .gpio_vbus_det = NULL,
                .gpio_id_det = NULL,
-#ifdef CONFIG_MACH_SUN6I
+#ifdef CONFIG_MACH_SUNXI_H3_H5
It should be handled by compatibles, not ifdefs. Phy driver is proper DM driver, so you can use .data in device ids array.
Quoted 5 lines
Quoted 3 lines
True, but since it' already have ifdef and more over this code will drop very soon, we are planning to support CLK and RST[1] (max of v2018.11).
Says who?
Me with USB clocks and resets.

Quoted 8 lines

Quoted 4 lines
I'd really prefer to have something when it's ready, rather than trying to push to meet a deadline that we enforced on ourselves, and end up with a tons of bugs to fix. The latest PHY patches should be a pretty good example on why we *shouldn't* do that.
These series fixes not certainly related to sunxi because of recent changes, were there in musb shutdown code and PHY changes were there before with legacy.

Well, all the fallout with EHCI being broken surely wasn't there before. It took us a year or so to have support for all the SoCs we support in Linux and have a somewhat robust, complete clock support. Thinking that it can be done and merged within 3 monthes is unreasonable.

Maxime

Signature · 10 lines

Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180711/484a8e98/attachment.sig>

Jagan Teki

author

2018-07-09 19:17 UTC #14 lore raw

RFC PATCH 6/8 dm: usb: Add UCLASS_USB_DEV_GENERIC shutdown

In reply to Jagan Teki

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 <[email protected]>

Signed-off-by Jagan Teki <[email protected]>

---
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.

 drivers/usb/host/usb-uclass.c | 43 +++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

drivers/usb/host/usb-uclass.c +43 -0

index 611ea97a72..99cf3d2b49 100644
@@ -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)

Signature · 1 line

2.17.1

Jagan Teki

author

2018-07-09 19:17 UTC #15 lore raw

PATCH 7/8 usb: musb-new: sunxi: Add proper musb exit support

In reply to Jagan Teki

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 <[email protected]>

---
 drivers/usb/musb-new/sunxi.c | 48 +++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 20 deletions(-)

drivers/usb/musb-new/sunxi.c +28 ~~-20~~

index aa2880eeb9..9f71b84fd1 100644
@@ -326,6 +326,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;
+		}
+	}
+
+#ifdef CONFIG_SUNXI_GEN_SUN6I
+	clrbits_le32(&glue->ccm->ahb_reset0_cfg, 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->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);

@@ -342,6 +369,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,

@@ -456,29 +484,9 @@ 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);
 
-#ifdef CONFIG_SUNXI_GEN_SUN6I
-	clrbits_le32(&glue->ccm->ahb_reset0_cfg, 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->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;

Signature · 1 line

2.17.1

Vasily Khoruzhick

2018-07-11 06:57 UTC #16 lore raw

PATCH 7/8 usb: musb-new: sunxi: Add proper musb exit support

Quoted 23 lines · Jagan Teki

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 <[email protected]>
---
 drivers/usb/musb-new/sunxi.c | 48 +++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 20 deletions(-)
drivers/usb/musb-new/sunxi.c +4 -0
index aa2880eeb9..9f71b84fd1 100644
@@ -326,6 +326,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;

drivers/usb/musb-new/sunxi.c static int sunxi_musb_init(struct musb *musb) · line 342

Quoted 10 lines

drivers/usb/musb-new/sunxi.c +10 -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;
+               }
+       }
+
+#ifdef CONFIG_SUNXI_GEN_SUN6I

Same here, since you're refactoring this code, it would be appropriate to use compatibles and get rid of this ifdef.

Quoted 61 lines

drivers/usb/musb-new/sunxi.c +14 ~~-21~~

+       clrbits_le32(&glue->ccm->ahb_reset0_cfg, 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->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);

@@ -342,6 +369,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,

@@ -456,29 +484,9 @@ 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);
 
-#ifdef CONFIG_SUNXI_GEN_SUN6I
-       clrbits_le32(&glue->ccm->ahb_reset0_cfg, 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->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;
 
--

2.17.1

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
https://lists.denx.de/listinfo/u-boot

Jagan Teki

author

2018-07-09 19:17 UTC #17 lore raw

PATCH 8/8 usb: musb-new: core: Call musb_platform_exit from musb_stop

In reply to Jagan Teki

musb_stop is shutdown code for musb where it call platform and generic disable. So add platform exit, for proper graceful shutdown.

Signed-off-by Jagan Teki <[email protected]>

---
 drivers/usb/musb-new/musb_core.c | 1 +
 1 file changed, 1 insertion(+)

drivers/usb/musb-new/musb_core.c +1 -0

index 8fec6f38ad..afea9fbcef 100644
@@ -1007,6 +1007,7 @@ void musb_stop(struct musb *musb)
 	 *  - ...
 	 */
 	musb_platform_try_idle(musb, 0);
+	musb_platform_exit(musb);
 }
 
 #ifndef __UBOOT__

Signature · 1 line

2.17.1