Hi Jagan,

On Tue, Jul 10, 2018 at 5:17 AM Jagan Teki jagan@amarulasolutions.com wrote:

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 jagan@amarulasolutions.com

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(-)

diff --git a/drivers/usb/musb-new/musb_uboot.c b/drivers/usb/musb-new/musb_uboot.c index 2b04fbd046..3cd89793c9 100644 --- a/drivers/usb/musb-new/musb_uboot.c +++ b/drivers/usb/musb-new/musb_uboot.c @@ -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)

    }

    *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,

On Tue, Jul 10, 2018 at 3:17 AM, Jagan Teki jagan@amarulasolutions.com wrote:

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

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

Signed-off-by: Jagan Teki jagan@amarulasolutions.com

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

diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c index 2b3cf48025..01f585a283 100644 --- a/drivers/phy/allwinner/phy-sun4i-usb.c +++ b/drivers/phy/allwinner/phy-sun4i-usb.c @@ -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

On Wed, Jul 11, 2018 at 12:23 PM, Vasily Khoruzhick anarsoul@gmail.com wrote:

On Mon, Jul 9, 2018 at 12:17 PM, Jagan Teki jagan@amarulasolutions.com wrote:

...

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

Signed-off-by: Jagan Teki jagan@amarulasolutions.com

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

diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c index 3096f12c1c..81fcd1f910 100644 --- a/drivers/phy/allwinner/phy-sun4i-usb.c +++ b/drivers/phy/allwinner/phy-sun4i-usb.c @@ -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

On Wed, Jul 11, 2018 at 12:44 PM, Maxime Ripard maxime.ripard@bootlin.com wrote:

On Wed, Jul 11, 2018 at 12:37:04PM +0530, Jagan Teki wrote:

...

On Wed, Jul 11, 2018 at 12:23 PM, Vasily Khoruzhick anarsoul@gmail.com wrote:

...

On Mon, Jul 9, 2018 at 12:17 PM, Jagan Teki jagan@amarulasolutions.com wrote:

...

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

Signed-off-by: Jagan Teki jagan@amarulasolutions.com

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

diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c index 3096f12c1c..81fcd1f910 100644 --- a/drivers/phy/allwinner/phy-sun4i-usb.c +++ b/drivers/phy/allwinner/phy-sun4i-usb.c @@ -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).

Says who?

Me with USB clocks and resets.

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.

On Mon, Jul 9, 2018 at 12:17 PM, Jagan Teki jagan@amarulasolutions.com wrote:

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

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

diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c index aa2880eeb9..9f71b84fd1 100644 --- a/drivers/usb/musb-new/sunxi.c +++ b/drivers/usb/musb-new/sunxi.c @@ -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

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

•   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@lists.denx.de https://lists.denx.de/listinfo/u-boot