Re: [PATCH v3 4/6] usb: dwc3: Add Amlogic A1 DWC3 glue
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date: 2019-12-27 16:38:20
Also in:
linux-amlogic, linux-arm-kernel, linux-usb
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 [off-list ref] 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)
[...]quoted hunk ↗ jump to hunk
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