Message-ID: <6f84d21f-1093-185f-dd70-b470bfa9c7da@baylibre.com>
Date:   Wed, 13 Mar 2019 14:07:34 +0100
From:   Neil Armstrong <narmstrong@...libre.com>
To:     Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Cc:     gregkh@...uxfoundation.org, hminas@...opsys.com, balbi@...nel.org,
        kishon@...com, linux-amlogic@...ts.infradead.org,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 8/8] usb: dwc3: Add Amlogic G12A DWC3 glue

On 11/03/2019 22:56, Martin Blumenstingl wrote:
> Hi Neil,
> 
> On Mon, Mar 4, 2019 at 11:41 AM Neil Armstrong <narmstrong@...libre.com> wrote:
> [...]
>> +static void dwc3_meson_g12a_usb_init_mode(struct dwc3_meson_g12a *priv)
>> +{
>> +       if (priv->otg_phy_mode == PHY_MODE_USB_DEVICE) {
>> +               regmap_update_bits(priv->regmap, USB_R0,
>> +                               USB_R0_U2D_ACT, USB_R0_U2D_ACT);
>> +               regmap_update_bits(priv->regmap, USB_R0,
>> +                               USB_R0_U2D_SS_SCALEDOWN_MODE_MASK, 0);
>> +               regmap_update_bits(priv->regmap, USB_R4,
>> +                               USB_R4_P21_SLEEP_M0, USB_R4_P21_SLEEP_M0);
>> +       } else {
>> +               regmap_update_bits(priv->regmap, USB_R0,
>> +                               USB_R0_U2D_ACT, 0);
>> +               regmap_update_bits(priv->regmap, USB_R4,
>> +                               USB_R4_P21_SLEEP_M0, 0);
>> +       }
>> +}
> I was already confused by the name of this function in v1.
> do you think "dwc3_meson_g12a_usb_otg_apply_mode" is a suitable name?

Yep changed

> 
> [...]
>> +static int dwc3_meson_g12a_otg_mode_set(struct dwc3_meson_g12a *priv,
>> +                                       enum phy_mode mode)
>> +{
>> +       int ret;
>> +
>> +       if (!priv->phys[USB2_OTG_PHY])
>> +               return -EINVAL;
>> +
>> +       if (mode == PHY_MODE_USB_HOST)
>> +               dev_info(priv->dev, "switching to Host Mode\n");
>> +       else
>> +               dev_info(priv->dev, "switching to Device Mode\n");
>> +
>> +       if (priv->vbus) {
>> +               if (mode == PHY_MODE_USB_DEVICE)
>> +                       ret = regulator_disable(priv->vbus);
>> +               else
>> +                       ret = regulator_enable(priv->vbus);
> do we need to track the regulator status (whether it's enabled or not)?
> the regulator framework WARN()s if it detects "unbalanced disables for
> <regulator>"
> (I haven't tested this on one of my boards yet, so maybe it works
> because the callers of dwc3_meson_g12a_otg_mode_set() protect against
> this by not calling this function if the mode doesn't change)

I handled this case by enabling the regulator _before_, as we default in
Host/OTG mode in probe, then eventually switch to device.

In this case, we don't have to track the status, and further calls
to dwc3_meson_g12a_otg_mode_set() will only change the regulator
on mode change.

> 
>> +               if (ret)
>> +                       return ret;
>> +               }
>> +
>> +               priv->otg_phy_mode = mode;
>> +
>> +               dwc3_meson_g12a_usb2_set_mode(priv, USB2_OTG_PHY, mode);
>> +
>> +               dwc3_meson_g12a_usb_init_mode(priv);
>> +
>> +               return phy_set_mode(priv->phys[USB2_OTG_PHY], mode);
>> + }
> this is the only place where phy_set_mode is called
> I'm fine with keeping it but then it should be consistent at least for
> all USB2 PHYs.
> I suggest to either move the phy_set_mode call to
> dwc3_meson_g12a_usb2_set_mode or to remove it.

I will remove it, it's useless for now.

> 
> [...]
>> +static int dwc3_meson_g12a_role_set(struct device *dev, enum usb_role role)
>> +{
>> +       struct dwc3_meson_g12a *priv = dev_get_drvdata(dev);
>> +       enum phy_mode mode;
>> +
>> +       if (role == USB_ROLE_NONE)
>> +               return 0;
>> +
>> +       mode = role == USB_ROLE_HOST ? PHY_MODE_USB_HOST : PHY_MODE_USB_DEVICE;
> (without surrounding parens I find the "role == USB_ROLE_HOST" part
> hard to distinguish from the "mode =" operation. if more people think
> this way then please speak up - otherwise it's probably just my
> personal taste)

Yep, I added parenthesis and changed indentation to make it clearer.

> 
> [...]
>> +static struct device *dwc3_meson_g12_find_child(struct device *dev,
>> +                                               const char *compatible)
>> +{
>> +       struct platform_device *pdev;
>> +       struct device_node *np;
>> +
>> +       np = of_find_compatible_node(dev->of_node, NULL, compatible);
>> +       if (!np)
>> +               return NULL;
>> +
>> +       pdev = of_find_device_by_node(np);
> maybe switch to of_get_compatible_child() here? This was done for the
> MMC driver in c483a5cc9d09f4 ("mmc: meson-mx-sdio: fix OF child-node
> lookup"), but I'm not sure if the problem described there also applies
> to dwc3_meson_g12_find_child

of_get_compatible_child() is new to me, switching to it !

> 
> [...]
>> +static int dwc3_meson_g12a_probe(struct platform_device *pdev)
>> +{
>> +       struct dwc3_meson_g12a  *priv;
>> +       struct device           *dev = &pdev->dev;
>> +       struct device_node      *np = dev->of_node;
>> +       void __iomem *base;
>> +       struct resource *res;
>> +       enum phy_mode otg_id;
>> +       int ret, i;
>> +
>> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +       if (!priv)
>> +               return -ENOMEM;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       base = devm_ioremap_resource(dev, res);
>> +       if (IS_ERR(base))
>> +               return PTR_ERR(base);
>> +
>> +       priv->regmap = devm_regmap_init_mmio(dev, base,
>> +                                            &phy_meson_g12a_usb3_regmap_conf);
>> +       if (IS_ERR(priv->regmap))
>> +               return PTR_ERR(priv->regmap);
>> +
>> +       priv->vbus = devm_regulator_get_optional(dev, "vbus");
>> +       if (IS_ERR(priv->vbus)) {
>> +               if (PTR_ERR(priv->vbus) == -EPROBE_DEFER)
>> +                       return PTR_ERR(priv->vbus);
>> +               priv->vbus = NULL;
>> +       }
>> +
>> +       priv->clk = devm_clk_get(dev, NULL);
>> +       if (IS_ERR(priv->clk))
>> +               return PTR_ERR(priv->clk);
>> +
>> +       ret = clk_prepare_enable(priv->clk);
>> +       if (ret)
>> +               return ret;
>> +
>> +       platform_set_drvdata(pdev, priv);
>> +       priv->dev = dev;
>> +
>> +       priv->reset = devm_reset_control_get(dev, NULL);
>> +       if (IS_ERR(priv->reset)) {
>> +               ret = PTR_ERR(priv->reset);
>> +               dev_err(dev, "failed to get device reset, err=%d\n", ret);
>> +               return ret;
>> +       }
> clk_prepare_enable is called a few lines above but this (and a few
> more) error-paths don't call clk_disable_unprepare.
> Jerome suggested in the dwmac-meson8b driver that I use something
> like, which will even allow you to drop the clk_disable_unprepare call
> from dwc3_meson_g12a_remove and catch all error cases in
> dwc3_meson_g12a_probe at the same time:
>   devm_add_action_or_reset(dev, (void(*)(void
> *))clk_disable_unprepare, priv->clk);
> (if you go with this then you also need to remove the
> clk_disable_unprepare after of_platform_populate)

Indeed I totally forgot this, thanks !

> 
> [...]
>> +static int dwc3_meson_g12a_remove(struct platform_device *pdev)
>> +{
>> +       struct dwc3_meson_g12a *priv = platform_get_drvdata(pdev);
>> +       struct device *dev = &pdev->dev;
>> +       int i;
>> +
>> +       usb_role_switch_unregister(priv->role_switch);
>> +
>> +       of_platform_depopulate(dev);
>> +
>> +       for (i = 0 ; i < PHY_COUNT ; ++i) {
>> +               phy_power_off(priv->phys[i]);
>> +               phy_exit(priv->phys[i]);
>> +               phy_put(priv->phys[i]);
>> +       }
>> +
>> +       clk_disable_unprepare(priv->clk);
>> +       clk_put(priv->clk);
> priv->clk is obtained with devm_clk_get so the common clock framework
> will call clk_put for us automatically

Right

> 
> [...]
>> +static int __maybe_unused dwc3_meson_g12a_suspend(struct device *dev)
>> +{
>> +       struct dwc3_meson_g12a *priv = dev_get_drvdata(dev);
>> +       int i;
>> +
>> +       for (i = 0 ; i < PHY_COUNT ; ++i)
>> +               if (priv->phys[i])
>> +                       phy_exit(priv->phys[i]);
> phy_init is NULL-safe, so you can drop the NULL-check above

Forgot this one


> 
>> +
>> +       reset_control_assert(priv->reset);
>> +
>> +       return 0;
>> +}
>> +
>> +static int __maybe_unused dwc3_meson_g12a_resume(struct device *dev)
>> +{
>> +       struct dwc3_meson_g12a *priv = dev_get_drvdata(dev);
>> +       int i, ret;
>> +
>> +       reset_control_deassert(priv->reset);
>> +
>> +       dwc3_meson_g12a_usb_init(priv);
>> +
>> +       /* Init PHYs */
>> +       for (i = 0 ; i < PHY_COUNT ; ++i) {
>> +               if (priv->phys[i]) {
>> +                       ret = phy_init(priv->phys[i]);
> phy_init is NULL-safe, so you can drop the NULL-check above

Good catch...

> 
> 
> Regards
> Martin
> 

Thanks !!

Neil

Powered by blists - more mailing lists