| Message ID | 1577428606-69855-5-git-send-email-hanjie.lin@amlogic.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: meson: Add support for USB on Amlogic A1 | expand |
Hello Hanjie, sorry that it took me so long to look at this you can find my comments below On Fri, Dec 27, 2019 at 7:37 AM Hanjie Lin <hanjie.lin@amlogic.com> wrote: [...] > +static const struct clk_bulk_data meson_g12a_clocks[] = { > + { .id = NULL}, > +}; > + > +static const struct clk_bulk_data meson_a1_clocks[] = { > + { .id = "usb_ctrl"}, > + { .id = "usb_bus"}, > + { .id = "xtal_usb_phy"}, > + { .id = "xtal_usb_ctrl"}, > +}; nit-pick: the values in meson_g12a_clocks and meson_a1_clocks all have a space after the opening "{" but no space before the closing "}" we should be consistent here (personally I prefer the variant with space after "{" and before "}", but having no space in both cases is fine for me too) [...] > static void dwc3_meson_g12a_usb2_set_mode(struct dwc3_meson_g12a *priv, > @@ -138,10 +156,13 @@ static int dwc3_meson_g12a_usb2_init(struct dwc3_meson_g12a *priv) > { > int i; > > - if (priv->otg_mode == USB_DR_MODE_PERIPHERAL) > - priv->otg_phy_mode = PHY_MODE_USB_DEVICE; > - else > - priv->otg_phy_mode = PHY_MODE_USB_HOST; > + /* only G12A supports otg mode */ > + if (priv->soc_id == MESON_SOC_G12A) { > + if (priv->otg_mode == USB_DR_MODE_PERIPHERAL) > + priv->otg_phy_mode = PHY_MODE_USB_DEVICE; > + else > + priv->otg_phy_mode = PHY_MODE_USB_HOST; > + } can you comment on future Amlogic SoCs and how this code will look in the future? I would like to avoid having to adjust this "if" for every new SoC, but I don't know if the majority of the SoCs will have OTG support also one idea that just came to my mind: you could define in the .yaml binding that for A1 only dr_mode = "host" is allowed then you may not need extra logic in the driver at all [...] > - if (i == USB2_OTG_PHY) { > + if (priv->soc_id == MESON_SOC_G12A && i == USB2_OTG_PHY) { on GXL we have two PHYs (0 and 1), the second one is OTG capable on GXM we have three PHYs (0..2), the second one is OTG capable on G12A/G12B we have two PHYs (0 and 1), the second one is OTG capable you already wrote that there is only one USB2 PHY on the A1 SoC is really only the second PHY port ("usb2-phy1" instead of "usb2-phy0") used on A1? if "usb2-phy0" is correct then you don't need these checks (there are more checks like this below) [...] > - usb_role_switch_unregister(priv->role_switch); > + if (priv->soc_id == MESON_SOC_G12A) > + usb_role_switch_unregister(priv->role_switch); I didn't expect this because in _probe usb_role_switch_register is still called on A1 we now call usb_role_switch_register() but we never call usb_role_switch_unregister() Martin
On 2019/12/28 0:38, Martin Blumenstingl wrote: > Hello Hanjie, > > sorry that it took me so long to look at this > you can find my comments below > > On Fri, Dec 27, 2019 at 7:37 AM Hanjie Lin <hanjie.lin@amlogic.com> wrote: > [...] >> +static const struct clk_bulk_data meson_g12a_clocks[] = { >> + { .id = NULL}, >> +}; >> + >> +static const struct clk_bulk_data meson_a1_clocks[] = { >> + { .id = "usb_ctrl"}, >> + { .id = "usb_bus"}, >> + { .id = "xtal_usb_phy"}, >> + { .id = "xtal_usb_ctrl"}, >> +}; > nit-pick: the values in meson_g12a_clocks and meson_a1_clocks all have > a space after the opening "{" but no space before the closing "}" > we should be consistent here (personally I prefer the variant with > space after "{" and before "}", but having no space in both cases is > fine for me too) > Right, I will fix it. > [...] >> static void dwc3_meson_g12a_usb2_set_mode(struct dwc3_meson_g12a *priv, >> @@ -138,10 +156,13 @@ static int dwc3_meson_g12a_usb2_init(struct dwc3_meson_g12a *priv) >> { >> int i; >> >> - if (priv->otg_mode == USB_DR_MODE_PERIPHERAL) >> - priv->otg_phy_mode = PHY_MODE_USB_DEVICE; >> - else >> - priv->otg_phy_mode = PHY_MODE_USB_HOST; >> + /* only G12A supports otg mode */ >> + if (priv->soc_id == MESON_SOC_G12A) { >> + if (priv->otg_mode == USB_DR_MODE_PERIPHERAL) >> + priv->otg_phy_mode = PHY_MODE_USB_DEVICE; >> + else >> + priv->otg_phy_mode = PHY_MODE_USB_HOST; >> + } > can you comment on future Amlogic SoCs and how this code will look in > the future? > I would like to avoid having to adjust this "if" for every new SoC, > but I don't know if the majority of the SoCs will have OTG support > > also one idea that just came to my mind: > you could define in the .yaml binding that for A1 only dr_mode = > "host" is allowed > then you may not need extra logic in the driver at all > Good idea this different SoC extra logic could avoided by add constraints to .yaml, also code will be more elegant. I will do this in next version. > [...] >> - if (i == USB2_OTG_PHY) { >> + if (priv->soc_id == MESON_SOC_G12A && i == USB2_OTG_PHY) { > on GXL we have two PHYs (0 and 1), the second one is OTG capable > on GXM we have three PHYs (0..2), the second one is OTG capable > on G12A/G12B we have two PHYs (0 and 1), the second one is OTG capable > > you already wrote that there is only one USB2 PHY on the A1 SoC > is really only the second PHY port ("usb2-phy1" instead of > "usb2-phy0") used on A1? > if "usb2-phy0" is correct then you don't need these checks (there are > more checks like this below) Actually, A1 have same phys("usb2-phy0", "usb2-phy1", "usb3-phy0") and register base with G12A. But A1 driver is designed to support host mode with usb2-phy1 only. > > [...] >> - usb_role_switch_unregister(priv->role_switch); >> + if (priv->soc_id == MESON_SOC_G12A) >> + usb_role_switch_unregister(priv->role_switch); > I didn't expect this because in _probe usb_role_switch_register is still called > on A1 we now call usb_role_switch_register() but we never call > usb_role_switch_unregister() > Actually, usb_role_switch_register() can be called only in G12A. dwc3_meson_g12a_probe() ... if (priv->soc_id != MESON_SOC_G12A) goto setup_pm_runtime; Same with second suggestion, this different SoC extra logic could avoided by add constraints to .yaml. I will do this in next version. Thanks, Hanjie > > Martin > > _______________________________________________ > linux-amlogic mailing list > linux-amlogic@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-amlogic > > . >
Hello Hanjie, On Thu, Jan 2, 2020 at 1:30 AM Hanjie Lin <hanjie.lin@amlogic.com> wrote: [...] > >> - if (i == USB2_OTG_PHY) { > >> + if (priv->soc_id == MESON_SOC_G12A && i == USB2_OTG_PHY) { > > on GXL we have two PHYs (0 and 1), the second one is OTG capable > > on GXM we have three PHYs (0..2), the second one is OTG capable > > on G12A/G12B we have two PHYs (0 and 1), the second one is OTG capable > > > > you already wrote that there is only one USB2 PHY on the A1 SoC > > is really only the second PHY port ("usb2-phy1" instead of > > "usb2-phy0") used on A1? > > if "usb2-phy0" is correct then you don't need these checks (there are > > more checks like this below) > > Actually, A1 have same phys("usb2-phy0", "usb2-phy1", "usb3-phy0") and register base with G12A. > But A1 driver is designed to support host mode with usb2-phy1 only. OK, thank you for clarifying this interesting decision made by the HW team ...] > >> - usb_role_switch_unregister(priv->role_switch); > >> + if (priv->soc_id == MESON_SOC_G12A) > >> + usb_role_switch_unregister(priv->role_switch); > > I didn't expect this because in _probe usb_role_switch_register is still called > > on A1 we now call usb_role_switch_register() but we never call > > usb_role_switch_unregister() > > > > Actually, usb_role_switch_register() can be called only in G12A. > > dwc3_meson_g12a_probe() > ... > if (priv->soc_id != MESON_SOC_G12A) > goto setup_pm_runtime; I completely missed that, thank you for clarifying it > > Same with second suggestion, this different SoC extra logic could avoided by add constraints > to .yaml. > I will do this in next version. that would be awesome if it works out! Martin
diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c index 8a3ec1a..916a200 100644 --- a/drivers/usb/dwc3/dwc3-meson-g12a.c +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c @@ -96,6 +96,11 @@ #define USB_R5_ID_DIG_TH_MASK GENMASK(15, 8) #define USB_R5_ID_DIG_CNT_MASK GENMASK(23, 16) +enum meson_soc_id { + MESON_SOC_G12A = 0, + MESON_SOC_A1, +}; + enum { USB2_HOST_PHY = 0, USB2_OTG_PHY, @@ -107,10 +112,22 @@ static const char *phy_names[PHY_COUNT] = { "usb2-phy0", "usb2-phy1", "usb3-phy0", }; +static const struct clk_bulk_data meson_g12a_clocks[] = { + { .id = NULL}, +}; + +static const struct clk_bulk_data meson_a1_clocks[] = { + { .id = "usb_ctrl"}, + { .id = "usb_bus"}, + { .id = "xtal_usb_phy"}, + { .id = "xtal_usb_ctrl"}, +}; + struct dwc3_meson_g12a { struct device *dev; struct regmap *regmap; - struct clk *clk; + struct clk_bulk_data *clks; + int num_clks; struct reset_control *reset; struct phy *phys[PHY_COUNT]; enum usb_dr_mode otg_mode; @@ -120,6 +137,7 @@ struct dwc3_meson_g12a { struct regulator *vbus; struct usb_role_switch_desc switch_desc; struct usb_role_switch *role_switch; + int soc_id; }; static void dwc3_meson_g12a_usb2_set_mode(struct dwc3_meson_g12a *priv, @@ -138,10 +156,13 @@ static int dwc3_meson_g12a_usb2_init(struct dwc3_meson_g12a *priv) { int i; - if (priv->otg_mode == USB_DR_MODE_PERIPHERAL) - priv->otg_phy_mode = PHY_MODE_USB_DEVICE; - else - priv->otg_phy_mode = PHY_MODE_USB_HOST; + /* only G12A supports otg mode */ + if (priv->soc_id == MESON_SOC_G12A) { + if (priv->otg_mode == USB_DR_MODE_PERIPHERAL) + priv->otg_phy_mode = PHY_MODE_USB_DEVICE; + else + priv->otg_phy_mode = PHY_MODE_USB_HOST; + } for (i = 0 ; i < USB3_HOST_PHY ; ++i) { if (!priv->phys[i]) @@ -151,7 +172,7 @@ static int dwc3_meson_g12a_usb2_init(struct dwc3_meson_g12a *priv) U2P_R0_POWER_ON_RESET, U2P_R0_POWER_ON_RESET); - if (i == USB2_OTG_PHY) { + if (priv->soc_id == MESON_SOC_G12A && i == USB2_OTG_PHY) { regmap_update_bits(priv->regmap, U2P_R0 + (U2P_REG_SIZE * i), U2P_R0_ID_PULLUP | U2P_R0_DRV_VBUS, @@ -295,7 +316,8 @@ static int dwc3_meson_g12a_otg_mode_set(struct dwc3_meson_g12a *priv, { int ret; - if (!priv->phys[USB2_OTG_PHY]) + /* only G12A supports otg mode */ + if (priv->soc_id != MESON_SOC_G12A || !priv->phys[USB2_OTG_PHY]) return -EINVAL; if (mode == PHY_MODE_USB_HOST) @@ -409,17 +431,32 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) priv->vbus = NULL; } - priv->clk = devm_clk_get(dev, NULL); - if (IS_ERR(priv->clk)) - return PTR_ERR(priv->clk); + priv->soc_id = (enum meson_soc_id)of_device_get_match_data(&pdev->dev); + + if (priv->soc_id == MESON_SOC_G12A) { + priv->clks = devm_kmemdup(dev, meson_g12a_clocks, + sizeof(meson_g12a_clocks), + GFP_KERNEL); + priv->num_clks = ARRAY_SIZE(meson_g12a_clocks); + } else if (priv->soc_id == MESON_SOC_A1) { + priv->clks = devm_kmemdup(dev, meson_a1_clocks, + sizeof(meson_a1_clocks), + GFP_KERNEL); + priv->num_clks = ARRAY_SIZE(meson_a1_clocks); + } else { + return -EINVAL; + } + + if (!priv->clks) + return -ENOMEM; - ret = clk_prepare_enable(priv->clk); + ret = devm_clk_bulk_get(dev, priv->num_clks, priv->clks); if (ret) return ret; - devm_add_action_or_reset(dev, - (void(*)(void *))clk_disable_unprepare, - priv->clk); + ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks); + if (ret) + return ret; platform_set_drvdata(pdev, priv); priv->dev = dev; @@ -433,22 +470,23 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) ret = reset_control_reset(priv->reset); if (ret) - return ret; + goto err_disable_clks; ret = dwc3_meson_g12a_get_phys(priv); if (ret) - return ret; + goto err_disable_clks; if (priv->vbus) { ret = regulator_enable(priv->vbus); if (ret) - return ret; + goto err_disable_clks; } /* Get dr_mode */ priv->otg_mode = usb_get_dr_mode(dev); - if (priv->otg_mode == USB_DR_MODE_OTG) { + if (priv->soc_id == MESON_SOC_G12A && + priv->otg_mode == USB_DR_MODE_OTG) { /* Ack irq before registering */ regmap_update_bits(priv->regmap, USB_R5, USB_R5_ID_DIG_IRQ, 0); @@ -458,7 +496,7 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) dwc3_meson_g12a_irq_thread, IRQF_ONESHOT, pdev->name, priv); if (ret) - return ret; + goto err_disable_clks; } dwc3_meson_g12a_usb_init(priv); @@ -467,7 +505,7 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) for (i = 0 ; i < PHY_COUNT ; ++i) { ret = phy_init(priv->phys[i]); if (ret) - return ret; + goto err_disable_clks; } /* Set PHY Power */ @@ -478,10 +516,11 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) } ret = of_platform_populate(np, NULL, NULL, dev); - if (ret) { - clk_disable_unprepare(priv->clk); + if (ret) goto err_phys_power; - } + + if (priv->soc_id != MESON_SOC_G12A) + goto setup_pm_runtime; /* Setup OTG mode corresponding to the ID pin */ if (priv->otg_mode == USB_DR_MODE_OTG) { @@ -504,6 +543,7 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) if (IS_ERR(priv->role_switch)) dev_warn(dev, "Unable to register Role Switch\n"); +setup_pm_runtime: pm_runtime_set_active(dev); pm_runtime_enable(dev); pm_runtime_get_sync(dev); @@ -518,6 +558,9 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) for (i = 0 ; i < PHY_COUNT ; ++i) phy_exit(priv->phys[i]); +err_disable_clks: + clk_bulk_disable_unprepare(priv->num_clks, priv->clks); + return ret; } @@ -527,7 +570,8 @@ static int dwc3_meson_g12a_remove(struct platform_device *pdev) struct device *dev = &pdev->dev; int i; - usb_role_switch_unregister(priv->role_switch); + if (priv->soc_id == MESON_SOC_G12A) + usb_role_switch_unregister(priv->role_switch); of_platform_depopulate(dev); @@ -547,7 +591,7 @@ static int __maybe_unused dwc3_meson_g12a_runtime_suspend(struct device *dev) { struct dwc3_meson_g12a *priv = dev_get_drvdata(dev); - clk_disable(priv->clk); + clk_bulk_disable_unprepare(priv->num_clks, priv->clks); return 0; } @@ -556,7 +600,7 @@ static int __maybe_unused dwc3_meson_g12a_runtime_resume(struct device *dev) { struct dwc3_meson_g12a *priv = dev_get_drvdata(dev); - return clk_enable(priv->clk); + return clk_bulk_prepare_enable(priv->num_clks, priv->clks); } static int __maybe_unused dwc3_meson_g12a_suspend(struct device *dev) @@ -619,7 +663,14 @@ static const struct dev_pm_ops dwc3_meson_g12a_dev_pm_ops = { }; static const struct of_device_id dwc3_meson_g12a_match[] = { - { .compatible = "amlogic,meson-g12a-usb-ctrl" }, + { + .compatible = "amlogic,meson-g12a-usb-ctrl", + .data = (void *)MESON_SOC_G12A, + }, + { + .compatible = "amlogic,meson-a1-usb-ctrl", + .data = (void *)MESON_SOC_A1, + }, { /* Sentinel */ } }; MODULE_DEVICE_TABLE(of, dwc3_meson_g12a_match);