[PATCH v2] phy: renesas: rcar-gen3-usb2: fix SError happen if DEBUG_SHIRQ is enabled

[PATCH v2] phy: renesas: rcar-gen3-usb2: fix SError happen if DEBUG_SHIRQ is enabled

[Yoshihiro Shimoda]

If CONFIG_DEBUG_SHIRQ was enabled, r8a77951-salvator-xs could boot
correctly. If we appended "earlycon keep_bootcon" to the kernel
command like, we could get kernel log like below.

    SError Interrupt on CPU0, code 0xbf000002 -- SError
    CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.8.0-rc3-salvator-x-00505-g6c843129e6faaf01 #785
    Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT)
    pstate: 60400085 (nZCv daIf +PAN -UAO BTYPE=--)
    pc : rcar_gen3_phy_usb2_irq+0x14/0x54
    lr : free_irq+0xf4/0x27c

This means free_irq() calls the interrupt handler while PM runtime
is not getting if DEBUG_SHIRQ is enabled and rcar_gen3_phy_usb2_probe()
failed. To fix the issue, move the irq registration place to
rcar_gen3_phy_usb2_init() which is ready to handle the interrupts.

Note that after the commit 549b6b55b005 ("phy: renesas: rcar-gen3-usb2:
enable/disable independent irqs") which is merged into v5.2, since this
driver creates multiple phy instances, needs to check whether one of
phy instances is initialized. However, if we backport this patch to v5.1
or less, we don't need to check it because such kernel have single
phy instance.

Reported-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
Fixes: 9f391c574efc ("phy: rcar-gen3-usb2: add runtime ID/VBUS pin detection")
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 Changes from v1:
 - Move the irq registration to rcar_gen3_phy_usb2_init() instead of
   add a condition into the irq handler.
 https://patchwork.kernel.org/patch/11654097/

 drivers/phy/renesas/phy-rcar-gen3-usb2.c | 62 +++++++++++++++++---------------
 1 file changed, 34 insertions(+), 28 deletions(-)

diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
index bfb22f8..6978f86 100644
--- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
+++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
@@ -111,6 +111,7 @@ struct rcar_gen3_chan {
 	struct work_struct work;
 	struct mutex lock;	/* protects rphys[...].powered */
 	enum usb_dr_mode dr_mode;
+	int irq;
 	bool extcon_host;
 	bool is_otg_channel;
 	bool uses_otg_pins;
@@ -389,12 +390,39 @@ static void rcar_gen3_init_otg(struct rcar_gen3_chan *ch)
 	rcar_gen3_device_recognition(ch);
 }
 
+static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch)
+{
+	struct rcar_gen3_chan *ch = _ch;
+	void __iomem *usb2_base = ch->base;
+	u32 status = readl(usb2_base + USB2_OBINTSTA);
+	irqreturn_t ret = IRQ_NONE;
+
+	if (status & USB2_OBINT_BITS) {
+		dev_vdbg(ch->dev, "%s: %08x\n", __func__, status);
+		writel(USB2_OBINT_BITS, usb2_base + USB2_OBINTSTA);
+		rcar_gen3_device_recognition(ch);
+		ret = IRQ_HANDLED;
+	}
+
+	return ret;
+}
+
 static int rcar_gen3_phy_usb2_init(struct phy *p)
 {
 	struct rcar_gen3_phy *rphy = phy_get_drvdata(p);
 	struct rcar_gen3_chan *channel = rphy->ch;
 	void __iomem *usb2_base = channel->base;
 	u32 val;
+	int ret;
+
+	if (!rcar_gen3_is_any_rphy_initialized(channel) && channel->irq >= 0) {
+		INIT_WORK(&channel->work, rcar_gen3_phy_usb2_work);
+		ret = request_irq(channel->irq, rcar_gen3_phy_usb2_irq,
+				  IRQF_SHARED, dev_name(channel->dev), channel);
+		if (ret < 0)
+			dev_err(channel->dev, "No irq handler (%d)\n",
+				channel->irq);
+	}
 
 	/* Initialize USB2 part */
 	val = readl(usb2_base + USB2_INT_ENABLE);
@@ -433,6 +461,9 @@ static int rcar_gen3_phy_usb2_exit(struct phy *p)
 		val &= ~USB2_INT_ENABLE_UCOM_INTEN;
 	writel(val, usb2_base + USB2_INT_ENABLE);
 
+	if (channel->irq >= 0 && !rcar_gen3_is_any_rphy_initialized(channel))
+		free_irq(channel->irq, channel);
+
 	return 0;
 }
 
@@ -503,23 +534,6 @@ static const struct phy_ops rz_g1c_phy_usb2_ops = {
 	.owner		= THIS_MODULE,
 };
 
-static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch)
-{
-	struct rcar_gen3_chan *ch = _ch;
-	void __iomem *usb2_base = ch->base;
-	u32 status = readl(usb2_base + USB2_OBINTSTA);
-	irqreturn_t ret = IRQ_NONE;
-
-	if (status & USB2_OBINT_BITS) {
-		dev_vdbg(ch->dev, "%s: %08x\n", __func__, status);
-		writel(USB2_OBINT_BITS, usb2_base + USB2_OBINTSTA);
-		rcar_gen3_device_recognition(ch);
-		ret = IRQ_HANDLED;
-	}
-
-	return ret;
-}
-
 static const struct of_device_id rcar_gen3_phy_usb2_match_table[] = {
 	{
 		.compatible = "renesas,usb2-phy-r8a77470",
@@ -598,7 +612,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
 	struct phy_provider *provider;
 	struct resource *res;
 	const struct phy_ops *phy_usb2_ops;
-	int irq, ret = 0, i;
+	int ret = 0, i;
 
 	if (!dev->of_node) {
 		dev_err(dev, "This driver needs device tree\n");
@@ -614,16 +628,8 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
 	if (IS_ERR(channel->base))
 		return PTR_ERR(channel->base);
 
-	/* call request_irq for OTG */
-	irq = platform_get_irq_optional(pdev, 0);
-	if (irq >= 0) {
-		INIT_WORK(&channel->work, rcar_gen3_phy_usb2_work);
-		irq = devm_request_irq(dev, irq, rcar_gen3_phy_usb2_irq,
-				       IRQF_SHARED, dev_name(dev), channel);
-		if (irq < 0)
-			dev_err(dev, "No irq handler (%d)\n", irq);
-	}
-
+	/* get irq number here and request_irq for OTG in phy_init */
+	channel->irq = platform_get_irq_optional(pdev, 0);
 	channel->dr_mode = rcar_gen3_get_dr_mode(dev->of_node);
 	if (channel->dr_mode != USB_DR_MODE_UNKNOWN) {
 		int ret;
-- 
2.7.4

Re: [PATCH v2] phy: renesas: rcar-gen3-usb2: fix SError happen if DEBUG_SHIRQ is enabled

[Vinod Koul]

hello Yoshihiro,

On 13-07-20, 21:11, Yoshihiro Shimoda wrote:

Please consider revising patch subject. It tell me you are fixing an
error but it doesnt tell me what this patch is about :)

Perhpas :move irq registration to init" maybe a better title which
describes the changes this patch brings in

> If CONFIG_DEBUG_SHIRQ was enabled, r8a77951-salvator-xs could boot
> correctly. If we appended "earlycon keep_bootcon" to the kernel
> command like, we could get kernel log like below.
> 
>     SError Interrupt on CPU0, code 0xbf000002 -- SError
>     CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.8.0-rc3-salvator-x-00505-g6c843129e6faaf01 #785
>     Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT)
>     pstate: 60400085 (nZCv daIf +PAN -UAO BTYPE=--)
>     pc : rcar_gen3_phy_usb2_irq+0x14/0x54
>     lr : free_irq+0xf4/0x27c
> 
> This means free_irq() calls the interrupt handler while PM runtime
> is not getting if DEBUG_SHIRQ is enabled and rcar_gen3_phy_usb2_probe()
> failed. To fix the issue, move the irq registration place to
> rcar_gen3_phy_usb2_init() which is ready to handle the interrupts.
> 
> Note that after the commit 549b6b55b005 ("phy: renesas: rcar-gen3-usb2:
> enable/disable independent irqs") which is merged into v5.2, since this
> driver creates multiple phy instances, needs to check whether one of
> phy instances is initialized. However, if we backport this patch to v5.1
> or less, we don't need to check it because such kernel have single
> phy instance.
> 
> Reported-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Fixes: 9f391c574efc ("phy: rcar-gen3-usb2: add runtime ID/VBUS pin detection")
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  Changes from v1:
>  - Move the irq registration to rcar_gen3_phy_usb2_init() instead of
>    add a condition into the irq handler.
>  https://patchwork.kernel.org/patch/11654097/
> 
>  drivers/phy/renesas/phy-rcar-gen3-usb2.c | 62 +++++++++++++++++---------------
>  1 file changed, 34 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> index bfb22f8..6978f86 100644
> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> @@ -111,6 +111,7 @@ struct rcar_gen3_chan {
>  	struct work_struct work;
>  	struct mutex lock;	/* protects rphys[...].powered */
>  	enum usb_dr_mode dr_mode;
> +	int irq;
>  	bool extcon_host;
>  	bool is_otg_channel;
>  	bool uses_otg_pins;
> @@ -389,12 +390,39 @@ static void rcar_gen3_init_otg(struct rcar_gen3_chan *ch)
>  	rcar_gen3_device_recognition(ch);
>  }
>  
> +static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch)
> +{
> +	struct rcar_gen3_chan *ch = _ch;
> +	void __iomem *usb2_base = ch->base;
> +	u32 status = readl(usb2_base + USB2_OBINTSTA);
> +	irqreturn_t ret = IRQ_NONE;
> +
> +	if (status & USB2_OBINT_BITS) {
> +		dev_vdbg(ch->dev, "%s: %08x\n", __func__, status);
> +		writel(USB2_OBINT_BITS, usb2_base + USB2_OBINTSTA);
> +		rcar_gen3_device_recognition(ch);
> +		ret = IRQ_HANDLED;
> +	}
> +
> +	return ret;
> +}
> +
>  static int rcar_gen3_phy_usb2_init(struct phy *p)
>  {
>  	struct rcar_gen3_phy *rphy = phy_get_drvdata(p);
>  	struct rcar_gen3_chan *channel = rphy->ch;
>  	void __iomem *usb2_base = channel->base;
>  	u32 val;
> +	int ret;
> +
> +	if (!rcar_gen3_is_any_rphy_initialized(channel) && channel->irq >= 0) {
> +		INIT_WORK(&channel->work, rcar_gen3_phy_usb2_work);
> +		ret = request_irq(channel->irq, rcar_gen3_phy_usb2_irq,
> +				  IRQF_SHARED, dev_name(channel->dev), channel);
> +		if (ret < 0)
> +			dev_err(channel->dev, "No irq handler (%d)\n",
> +				channel->irq);

This could be in a single line :)

Should we continue on error here?

> +	}
>  
>  	/* Initialize USB2 part */
>  	val = readl(usb2_base + USB2_INT_ENABLE);
> @@ -433,6 +461,9 @@ static int rcar_gen3_phy_usb2_exit(struct phy *p)
>  		val &= ~USB2_INT_ENABLE_UCOM_INTEN;
>  	writel(val, usb2_base + USB2_INT_ENABLE);
>  
> +	if (channel->irq >= 0 && !rcar_gen3_is_any_rphy_initialized(channel))
> +		free_irq(channel->irq, channel);
> +
>  	return 0;
>  }
>  
> @@ -503,23 +534,6 @@ static const struct phy_ops rz_g1c_phy_usb2_ops = {
>  	.owner		= THIS_MODULE,
>  };
>  
> -static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch)
> -{
> -	struct rcar_gen3_chan *ch = _ch;
> -	void __iomem *usb2_base = ch->base;
> -	u32 status = readl(usb2_base + USB2_OBINTSTA);
> -	irqreturn_t ret = IRQ_NONE;
> -
> -	if (status & USB2_OBINT_BITS) {
> -		dev_vdbg(ch->dev, "%s: %08x\n", __func__, status);
> -		writel(USB2_OBINT_BITS, usb2_base + USB2_OBINTSTA);
> -		rcar_gen3_device_recognition(ch);
> -		ret = IRQ_HANDLED;
> -	}
> -
> -	return ret;
> -}
> -
>  static const struct of_device_id rcar_gen3_phy_usb2_match_table[] = {
>  	{
>  		.compatible = "renesas,usb2-phy-r8a77470",
> @@ -598,7 +612,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
>  	struct phy_provider *provider;
>  	struct resource *res;
>  	const struct phy_ops *phy_usb2_ops;
> -	int irq, ret = 0, i;
> +	int ret = 0, i;
>  
>  	if (!dev->of_node) {
>  		dev_err(dev, "This driver needs device tree\n");
> @@ -614,16 +628,8 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
>  	if (IS_ERR(channel->base))
>  		return PTR_ERR(channel->base);
>  
> -	/* call request_irq for OTG */
> -	irq = platform_get_irq_optional(pdev, 0);
> -	if (irq >= 0) {
> -		INIT_WORK(&channel->work, rcar_gen3_phy_usb2_work);
> -		irq = devm_request_irq(dev, irq, rcar_gen3_phy_usb2_irq,
> -				       IRQF_SHARED, dev_name(dev), channel);
> -		if (irq < 0)
> -			dev_err(dev, "No irq handler (%d)\n", irq);
> -	}
> -
> +	/* get irq number here and request_irq for OTG in phy_init */
> +	channel->irq = platform_get_irq_optional(pdev, 0);
>  	channel->dr_mode = rcar_gen3_get_dr_mode(dev->of_node);
>  	if (channel->dr_mode != USB_DR_MODE_UNKNOWN) {
>  		int ret;
> -- 
> 2.7.4

-- 
~Vinod

RE: [PATCH v2] phy: renesas: rcar-gen3-usb2: fix SError happen if DEBUG_SHIRQ is enabled

[Yoshihiro Shimoda]

Hello Vinod,

> From: Vinod Koul, Sent: Friday, July 17, 2020 3:39 PM
> 
> hello Yoshihiro,
> 
> On 13-07-20, 21:11, Yoshihiro Shimoda wrote:
> 
> Please consider revising patch subject. It tell me you are fixing an
> error but it doesnt tell me what this patch is about :)
> 
> Perhpas :move irq registration to init" maybe a better title which
> describes the changes this patch brings in

Thank you for your suggestion! I also think your suggestion is better.
So, I will fix it.

<snip>
> > @@ -389,12 +390,39 @@ static void rcar_gen3_init_otg(struct rcar_gen3_chan *ch)
> >  	rcar_gen3_device_recognition(ch);
> >  }
> >
> > +static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch)
> > +{
> > +	struct rcar_gen3_chan *ch = _ch;
> > +	void __iomem *usb2_base = ch->base;
> > +	u32 status = readl(usb2_base + USB2_OBINTSTA);
> > +	irqreturn_t ret = IRQ_NONE;
> > +
> > +	if (status & USB2_OBINT_BITS) {
> > +		dev_vdbg(ch->dev, "%s: %08x\n", __func__, status);
> > +		writel(USB2_OBINT_BITS, usb2_base + USB2_OBINTSTA);
> > +		rcar_gen3_device_recognition(ch);
> > +		ret = IRQ_HANDLED;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  static int rcar_gen3_phy_usb2_init(struct phy *p)
> >  {
> >  	struct rcar_gen3_phy *rphy = phy_get_drvdata(p);
> >  	struct rcar_gen3_chan *channel = rphy->ch;
> >  	void __iomem *usb2_base = channel->base;
> >  	u32 val;
> > +	int ret;
> > +
> > +	if (!rcar_gen3_is_any_rphy_initialized(channel) && channel->irq >= 0) {
> > +		INIT_WORK(&channel->work, rcar_gen3_phy_usb2_work);
> > +		ret = request_irq(channel->irq, rcar_gen3_phy_usb2_irq,
> > +				  IRQF_SHARED, dev_name(channel->dev), channel);
> > +		if (ret < 0)
> > +			dev_err(channel->dev, "No irq handler (%d)\n",
> > +				channel->irq);
> 
> This could be in a single line :)

Yes. We could be 80 over characters in a line now :)
I'll fix it.

> Should we continue on error here?

Hmm, maybe it's better if the request_irq() failed because
it can avoid unexpected behaviors. But, original code continued on error.
In this case, should I make a separated incremental patch to exit on error?

Best regards,
Yoshihiro Shimoda

Re: [PATCH v2] phy: renesas: rcar-gen3-usb2: fix SError happen if DEBUG_SHIRQ is enabled

[Vinod Koul]

On 17-07-20, 08:16, Yoshihiro Shimoda wrote:
> Hello Vinod,
> 
> > From: Vinod Koul, Sent: Friday, July 17, 2020 3:39 PM
> > 
> > hello Yoshihiro,
> > 
> > On 13-07-20, 21:11, Yoshihiro Shimoda wrote:
> > 
> > Please consider revising patch subject. It tell me you are fixing an
> > error but it doesnt tell me what this patch is about :)
> > 
> > Perhpas :move irq registration to init" maybe a better title which
> > describes the changes this patch brings in
> 
> Thank you for your suggestion! I also think your suggestion is better.
> So, I will fix it.
> 
> <snip>
> > > @@ -389,12 +390,39 @@ static void rcar_gen3_init_otg(struct rcar_gen3_chan *ch)
> > >  	rcar_gen3_device_recognition(ch);
> > >  }
> > >
> > > +static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch)
> > > +{
> > > +	struct rcar_gen3_chan *ch = _ch;
> > > +	void __iomem *usb2_base = ch->base;
> > > +	u32 status = readl(usb2_base + USB2_OBINTSTA);
> > > +	irqreturn_t ret = IRQ_NONE;
> > > +
> > > +	if (status & USB2_OBINT_BITS) {
> > > +		dev_vdbg(ch->dev, "%s: %08x\n", __func__, status);
> > > +		writel(USB2_OBINT_BITS, usb2_base + USB2_OBINTSTA);
> > > +		rcar_gen3_device_recognition(ch);
> > > +		ret = IRQ_HANDLED;
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >  static int rcar_gen3_phy_usb2_init(struct phy *p)
> > >  {
> > >  	struct rcar_gen3_phy *rphy = phy_get_drvdata(p);
> > >  	struct rcar_gen3_chan *channel = rphy->ch;
> > >  	void __iomem *usb2_base = channel->base;
> > >  	u32 val;
> > > +	int ret;
> > > +
> > > +	if (!rcar_gen3_is_any_rphy_initialized(channel) && channel->irq >= 0) {
> > > +		INIT_WORK(&channel->work, rcar_gen3_phy_usb2_work);
> > > +		ret = request_irq(channel->irq, rcar_gen3_phy_usb2_irq,
> > > +				  IRQF_SHARED, dev_name(channel->dev), channel);
> > > +		if (ret < 0)
> > > +			dev_err(channel->dev, "No irq handler (%d)\n",
> > > +				channel->irq);
> > 
> > This could be in a single line :)
> 
> Yes. We could be 80 over characters in a line now :)
> I'll fix it.
> 
> > Should we continue on error here?
> 
> Hmm, maybe it's better if the request_irq() failed because
> it can avoid unexpected behaviors. But, original code continued on error.
> In this case, should I make a separated incremental patch to exit on error?

Yes that would be better :), Always, a patch per change

-- 
~Vinod

RE: [PATCH v2] phy: renesas: rcar-gen3-usb2: fix SError happen if DEBUG_SHIRQ is enabled

[Yoshihiro Shimoda]

> From: Vinod Koul, Sent: Friday, July 17, 2020 5:46 PM
<snip>
> > > >  static int rcar_gen3_phy_usb2_init(struct phy *p)
> > > >  {
> > > >  	struct rcar_gen3_phy *rphy = phy_get_drvdata(p);
> > > >  	struct rcar_gen3_chan *channel = rphy->ch;
> > > >  	void __iomem *usb2_base = channel->base;
> > > >  	u32 val;
> > > > +	int ret;
> > > > +
> > > > +	if (!rcar_gen3_is_any_rphy_initialized(channel) && channel->irq >= 0) {
> > > > +		INIT_WORK(&channel->work, rcar_gen3_phy_usb2_work);
> > > > +		ret = request_irq(channel->irq, rcar_gen3_phy_usb2_irq,
> > > > +				  IRQF_SHARED, dev_name(channel->dev), channel);
> > > > +		if (ret < 0)
> > > > +			dev_err(channel->dev, "No irq handler (%d)\n",
> > > > +				channel->irq);
> > >
> > > This could be in a single line :)
> >
> > Yes. We could be 80 over characters in a line now :)
> > I'll fix it.
> >
> > > Should we continue on error here?
> >
> > Hmm, maybe it's better if the request_irq() failed because
> > it can avoid unexpected behaviors. But, original code continued on error.
> > In this case, should I make a separated incremental patch to exit on error?
> 
> Yes that would be better :), Always, a patch per change

Thank you for the reply. I got it :)

Best regards,
Yoshihiro Shimoda