diff mbox series

[v4,3/4] usb: dwc3: imx8mp: Add support for setting SOC specific flags

Message ID 20220126141340.234125-4-alexander.stein@ew.tq-group.com
State Accepted
Commit 9d52107185b646d449b682e10e95e6b3037b79cf
Headers show
Series i.MX8MP: more USB3 glue layer feature support | expand

Commit Message

Alexander Stein Jan. 26, 2022, 2:13 p.m. UTC
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@ew.tq-group.com>
---
 drivers/usb/dwc3/dwc3-imx8mp.c | 62 ++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

Comments

Jun Li Feb. 18, 2022, 6:57 a.m. UTC | #1
> -----Original Message-----
> From: Alexander Stein <alexander.stein@ew.tq-group.com>
> Sent: Wednesday, January 26, 2022 10:14 PM
> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring
> <robh+dt@kernel.org>; Shawn Guo <shawnguo@kernel.org>; Sascha Hauer
> <s.hauer@pengutronix.de>; Fabio Estevam <festevam@gmail.com>
> Cc: Alexander Stein <alexander.stein@ew.tq-group.com>; dl-linux-imx
> <linux-imx@nxp.com>; linux-usb@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Jun Li
> <jun.li@nxp.com>
> 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@ew.tq-group.com>
> ---
>  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.

> +
> +	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.

>  	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.

thanks
Li Jun

>  	if (dwc3_imx->wakeup_pending) {
>  		dwc3_imx->wakeup_pending = false;
>  		if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE) {
> --
> 2.25.1
Alexander Stein Feb. 18, 2022, 9:24 a.m. UTC | #2
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@ew.tq-group.com>
> > Sent: Wednesday, January 26, 2022 10:14 PM
> > To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring
> > <robh+dt@kernel.org>; Shawn Guo <shawnguo@kernel.org>; Sascha Hauer
> > <s.hauer@pengutronix.de>; Fabio Estevam <festevam@gmail.com>
> > Cc: Alexander Stein <alexander.stein@ew.tq-group.com>; dl-linux-imx
> > <linux-imx@nxp.com>; linux-usb@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Jun Li
> > <jun.li@nxp.com>
> > 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@ew.tq-group.com>
> > ---
> > 
> >  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
diff mbox series

Patch

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;
+
+	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;
 	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);
+
 	if (dwc3_imx->wakeup_pending) {
 		dwc3_imx->wakeup_pending = false;
 		if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE) {