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@xxxxxxxxxxx> 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
