| Message ID | 20250219160749.1750797-3-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | phy: renesas: rcar-gen3-usb2: Fixes for Renesas RZ/G3S | expand |
Hello Claudiu-san, > From: Claudiu, Sent: Thursday, February 20, 2025 1:08 AM > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Commit 08b0ad375ca6 ("phy: renesas: rcar-gen3-usb2: move IRQ registration > to init") moved the IRQ request operation from probe to I don't know why the checkpatch.pl said the following ERROR though, as I sent my Reviewed-by on the top of this email thread, this patch is good, I think. --- ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'Commit 08b0ad375ca6 ("phy: renesas: rcar-gen3-usb2: move irq registration to init")' #6: Commit 08b0ad375ca6 ("phy: renesas: rcar-gen3-usb2: move IRQ registration to init") moved the IRQ request operation from probe to --- Best regards, Yoshihiro Shimoda > struct phy_ops::phy_init API to avoid triggering interrupts (which lead to > register accesses) while the PHY clocks (enabled through runtime PM APIs) > are not active. If this happens, it results in a synchronous abort. > > One way to reproduce this issue is by enabling CONFIG_DEBUG_SHIRQ, which > calls free_irq() on driver removal. > > Move the IRQ request and free operations back to probe, and take the > runtime PM state into account in IRQ handler. This commit is preparatory > for the subsequent fixes in this series. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > drivers/phy/renesas/phy-rcar-gen3-usb2.c | 46 +++++++++++++----------- > 1 file changed, 26 insertions(+), 20 deletions(-) > > diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > index 46afba2fe0dc..826c9c4dd4c0 100644 > --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c > +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > @@ -120,7 +120,6 @@ struct rcar_gen3_chan { > struct work_struct work; > struct mutex lock; /* protects rphys[...].powered */ > enum usb_dr_mode dr_mode; > - int irq; > u32 obint_enable_bits; > bool extcon_host; > bool is_otg_channel; > @@ -428,16 +427,25 @@ 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); > + struct device *dev = ch->dev; > irqreturn_t ret = IRQ_NONE; > + u32 status; > > + pm_runtime_get_noresume(dev); > + > + if (pm_runtime_suspended(dev)) > + goto rpm_put; > + > + status = readl(usb2_base + USB2_OBINTSTA); > if (status & ch->obint_enable_bits) { > - dev_vdbg(ch->dev, "%s: %08x\n", __func__, status); > + dev_vdbg(dev, "%s: %08x\n", __func__, status); > writel(ch->obint_enable_bits, usb2_base + USB2_OBINTSTA); > rcar_gen3_device_recognition(ch); > ret = IRQ_HANDLED; > } > > +rpm_put: > + pm_runtime_put_noidle(dev); > return ret; > } > > @@ -447,17 +455,6 @@ static int rcar_gen3_phy_usb2_init(struct phy *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); > - return ret; > - } > - } > > /* Initialize USB2 part */ > val = readl(usb2_base + USB2_INT_ENABLE); > @@ -490,9 +487,6 @@ 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; > } > > @@ -698,7 +692,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev) > struct device *dev = &pdev->dev; > struct rcar_gen3_chan *channel; > struct phy_provider *provider; > - int ret = 0, i; > + int ret = 0, i, irq; > > if (!dev->of_node) { > dev_err(dev, "This driver needs device tree\n"); > @@ -714,8 +708,6 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev) > return PTR_ERR(channel->base); > > channel->obint_enable_bits = USB2_OBINT_BITS; > - /* 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) { > channel->is_otg_channel = true; > @@ -784,6 +776,20 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev) > channel->vbus = NULL; > } > > + irq = platform_get_irq_optional(pdev, 0); > + if (irq == -EPROBE_DEFER) { > + ret = -EPROBE_DEFER; > + goto error; > + } else if (irq >= 0) { > + INIT_WORK(&channel->work, rcar_gen3_phy_usb2_work); > + ret = devm_request_irq(dev, irq, rcar_gen3_phy_usb2_irq, > + IRQF_SHARED, dev_name(dev), channel); > + if (ret < 0) { > + dev_err(dev, "Failed to request irq (%d)\n", irq); > + goto error; > + } > + } > + > provider = devm_of_phy_provider_register(dev, rcar_gen3_phy_usb2_xlate); > if (IS_ERR(provider)) { > dev_err(dev, "Failed to register PHY provider\n"); > -- > 2.43.0
Hi Shimoda-san, On Fri, 21 Feb 2025 at 03:11, Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > From: Claudiu, Sent: Thursday, February 20, 2025 1:08 AM > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > Commit 08b0ad375ca6 ("phy: renesas: rcar-gen3-usb2: move IRQ registration > > to init") moved the IRQ request operation from probe to > > I don't know why the checkpatch.pl said the following ERROR though, > as I sent my Reviewed-by on the top of this email thread, this patch is good, > I think. > --- > ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'Commit 08b0ad375ca6 ("phy: renesas: rcar-gen3-usb2: move irq registration to init")' > #6: > Commit 08b0ad375ca6 ("phy: renesas: rcar-gen3-usb2: move IRQ registration > to init") moved the IRQ request operation from probe to Checkpatch complains because the quoted commit description (the part between '("' and '")') is split across two lines. As this is in flowed text, and not in tag, it is a false positive, and can be ignored. Gr{oetje,eeting}s, Geert
diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c index 46afba2fe0dc..826c9c4dd4c0 100644 --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c @@ -120,7 +120,6 @@ struct rcar_gen3_chan { struct work_struct work; struct mutex lock; /* protects rphys[...].powered */ enum usb_dr_mode dr_mode; - int irq; u32 obint_enable_bits; bool extcon_host; bool is_otg_channel; @@ -428,16 +427,25 @@ 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); + struct device *dev = ch->dev; irqreturn_t ret = IRQ_NONE; + u32 status; + pm_runtime_get_noresume(dev); + + if (pm_runtime_suspended(dev)) + goto rpm_put; + + status = readl(usb2_base + USB2_OBINTSTA); if (status & ch->obint_enable_bits) { - dev_vdbg(ch->dev, "%s: %08x\n", __func__, status); + dev_vdbg(dev, "%s: %08x\n", __func__, status); writel(ch->obint_enable_bits, usb2_base + USB2_OBINTSTA); rcar_gen3_device_recognition(ch); ret = IRQ_HANDLED; } +rpm_put: + pm_runtime_put_noidle(dev); return ret; } @@ -447,17 +455,6 @@ static int rcar_gen3_phy_usb2_init(struct phy *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); - return ret; - } - } /* Initialize USB2 part */ val = readl(usb2_base + USB2_INT_ENABLE); @@ -490,9 +487,6 @@ 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; } @@ -698,7 +692,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct rcar_gen3_chan *channel; struct phy_provider *provider; - int ret = 0, i; + int ret = 0, i, irq; if (!dev->of_node) { dev_err(dev, "This driver needs device tree\n"); @@ -714,8 +708,6 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev) return PTR_ERR(channel->base); channel->obint_enable_bits = USB2_OBINT_BITS; - /* 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) { channel->is_otg_channel = true; @@ -784,6 +776,20 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev) channel->vbus = NULL; } + irq = platform_get_irq_optional(pdev, 0); + if (irq == -EPROBE_DEFER) { + ret = -EPROBE_DEFER; + goto error; + } else if (irq >= 0) { + INIT_WORK(&channel->work, rcar_gen3_phy_usb2_work); + ret = devm_request_irq(dev, irq, rcar_gen3_phy_usb2_irq, + IRQF_SHARED, dev_name(dev), channel); + if (ret < 0) { + dev_err(dev, "Failed to request irq (%d)\n", irq); + goto error; + } + } + provider = devm_of_phy_provider_register(dev, rcar_gen3_phy_usb2_xlate); if (IS_ERR(provider)) { dev_err(dev, "Failed to register PHY provider\n");