Hi Li Jun,
thanks for your feedback.
Am Freitag, 18. Februar 2022, 07:57:33 CET schrieb Jun Li:
> > -----Original Message-----
> > From: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
> > Sent: Wednesday, January 26, 2022 10:14 PM
> > To: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Rob Herring
> > <robh+dt@xxxxxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>; Sascha Hauer
> > <s.hauer@xxxxxxxxxxxxxx>; Fabio Estevam <festevam@xxxxxxxxx>
> > Cc: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>; dl-linux-imx
> > <linux-imx@xxxxxxx>; linux-usb@xxxxxxxxxxxxxxx;
> > devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Jun Li
> > <jun.li@xxxxxxx>
> > Subject: [PATCH v4 3/4] usb: dwc3: imx8mp: Add support for setting SOC
> > specific flags
> >
> > The i.MX8MP glue layer has support for the following flags:
> > * over-current polarity
> > * PWR pad polarity
> > * controlling PPC flag in HCCPARAMS register
> > * permanent port attach for usb2 & usb3 port
> >
> > Allow setting these flags by supporting specific flags in the glue node.
> > In order to get this to work an additional IORESOURCE_MEM and clock is
> > necessary. For backward compatibility this is purely optional.
> >
> > Signed-off-by: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
> > ---
> >
> > drivers/usb/dwc3/dwc3-imx8mp.c | 62 ++++++++++++++++++++++++++++++++++
> > 1 file changed, 62 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/dwc3-imx8mp.c
> > b/drivers/usb/dwc3/dwc3-imx8mp.c
> > index 1c8fe657b3a9..5a96b66bf237 100644
> > --- a/drivers/usb/dwc3/dwc3-imx8mp.c
> > +++ b/drivers/usb/dwc3/dwc3-imx8mp.c
> > @@ -36,10 +36,22 @@
> >
> > #define USB_WAKEUP_EN_MASK GENMASK(5, 0)
> >
> > +/* USB glue registers */
> > +#define USB_CTRL0 0x00
> > +#define USB_CTRL1 0x04
> > +
> > +#define USB_CTRL0_PORTPWR_EN BIT(12) /* 1 - PPC enabled (default) */
> > +#define USB_CTRL0_USB3_FIXED BIT(22) /* 1 - USB3 permanent attached */
> > +#define USB_CTRL0_USB2_FIXED BIT(23) /* 1 - USB2 permanent attached */
> > +
> > +#define USB_CTRL1_OC_POLARITY BIT(16) /* 0 - HIGH / 1 - LOW */
> > +#define USB_CTRL1_PWR_POLARITY BIT(17) /* 0 - HIGH / 1 - LOW */
> > +
> >
> > struct dwc3_imx8mp {
> >
> > struct device *dev;
> > struct platform_device *dwc3;
> > void __iomem *hsio_blk_base;
> >
> > + void __iomem *glue_base;
> >
> > struct clk *hsio_clk;
> > struct clk *suspend_clk;
> > int irq;
> >
> > @@ -47,6 +59,42 @@ struct dwc3_imx8mp {
> >
> > bool wakeup_pending;
> >
> > };
> >
> > +static void imx8mp_configure_glue(struct dwc3_imx8mp *dwc3_imx)
> > +{
> > + struct device *dev = dwc3_imx->dev;
> > + u32 value;
> > +
> > + if (!dwc3_imx->glue_base)
> > + return;
> > +
> > + value = readl(dwc3_imx->glue_base + USB_CTRL0);
> > +
> > + if (device_property_read_bool(dev, "fsl,permanently-attached"))
> > + value |= (USB_CTRL0_USB2_FIXED | USB_CTRL0_USB3_FIXED);
> > + else
> > + value &= ~(USB_CTRL0_USB2_FIXED | USB_CTRL0_USB3_FIXED);
> > +
> > + if (device_property_read_bool(dev,
> > "fsl,disable-port-power-control"))
> > + value &= ~(USB_CTRL0_PORTPWR_EN);
> > + else
> > + value |= USB_CTRL0_PORTPWR_EN;
> > +
> > + writel(value, dwc3_imx->glue_base + USB_CTRL0);
> > +
> > + value = readl(dwc3_imx->glue_base + USB_CTRL1);
> > + if (device_property_read_bool(dev, "fsl,over-current-active-low"))
> > + value |= USB_CTRL1_OC_POLARITY;
> > + else
> > + value &= ~USB_CTRL1_OC_POLARITY;
> > +
> > + if (device_property_read_bool(dev, "fsl,power-active-low"))
> > + value |= USB_CTRL1_PWR_POLARITY;
> > + else
> > + value &= ~USB_CTRL1_PWR_POLARITY;
>
> I think you don't need a value update&write if property is not present.
I get what you intend here, but this assumes some (sane) default which might
be true in every case. But I prefer configuring this without any assumption
about previous settings.
> > +
> > + writel(value, dwc3_imx->glue_base + USB_CTRL1);
> > +}
> > +
> >
> > static void dwc3_imx8mp_wakeup_enable(struct dwc3_imx8mp *dwc3_imx)
> > {
> >
> > struct dwc3 *dwc3 = platform_get_drvdata(dwc3_imx->dwc3);
> >
> > @@ -100,6 +148,7 @@ static int dwc3_imx8mp_probe(struct platform_device
> > *pdev)
> >
> > struct device *dev = &pdev->dev;
> > struct device_node *dwc3_np, *node = dev->of_node;
> > struct dwc3_imx8mp *dwc3_imx;
> >
> > + struct resource *res;
>
> Looks like this is not used anywhere, remove it.
It actually is, please refer to the next hunk. Both is inside
dwc3_imx8mp_probe(). Probably the context of the diff is a bit misleading.
> > int err, irq;
> >
> > if (!node) {
> >
> > @@ -119,6 +168,15 @@ static int dwc3_imx8mp_probe(struct platform_device
> > *pdev)
> >
> > if (IS_ERR(dwc3_imx->hsio_blk_base))
> >
> > return PTR_ERR(dwc3_imx->hsio_blk_base);
> >
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > + if (!res) {
> > + dev_warn(dev, "Base address for glue layer missing.
Continuing
> > without, some features are missing though.");
> > + } else {
> > + dwc3_imx->glue_base = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(dwc3_imx->glue_base))
> > + return PTR_ERR(dwc3_imx->glue_base);
> > + }
> > +
> >
> > dwc3_imx->hsio_clk = devm_clk_get(dev, "hsio");
> > if (IS_ERR(dwc3_imx->hsio_clk)) {
> >
> > err = PTR_ERR(dwc3_imx->hsio_clk);
> >
> > @@ -152,6 +210,8 @@ static int dwc3_imx8mp_probe(struct platform_device
> > *pdev)
> >
> > }
> > dwc3_imx->irq = irq;
> >
> > + imx8mp_configure_glue(dwc3_imx);
> > +
> >
> > pm_runtime_set_active(dev);
> > pm_runtime_enable(dev);
> > err = pm_runtime_get_sync(dev);
> >
> > @@ -252,6 +312,8 @@ static int __maybe_unused dwc3_imx8mp_resume(struct
> > dwc3_imx8mp *dwc3_imx,
> >
> > dwc3_imx8mp_wakeup_disable(dwc3_imx);
> > dwc3_imx->pm_suspended = false;
> >
> > + imx8mp_configure_glue(dwc3_imx);
> > +
>
> I guess this is to restore the settings if we experienced a power lost,
> Please add some comments.
You are right, makes sense. I'll add a comment.
Regards,
Alexander
> > if (dwc3_imx->wakeup_pending) {
> >
> > dwc3_imx->wakeup_pending = false;
> > if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE) {
> >
> > --
> > 2.25.1
