| Message ID | 20190611135842.8396-1-narmstrong@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: dwc3: meson-g12a: Add support for IRQ based OTG switching | expand |
Hi Neil, On Tue, Jun 11, 2019 at 3:58 PM Neil Armstrong <narmstrong@baylibre.com> wrote: > > Add support for the OTG ID change interrupt to switch between Host > and Device mode. > > Tested on the Hardkernel Odroid-N2 board. > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> with the three questions/comments below answered/addressed: Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > drivers/usb/dwc3/dwc3-meson-g12a.c | 32 ++++++++++++++++++++++++++++-- > 1 file changed, 30 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c > index 2aec31a2eacb..e5c5ad0d529e 100644 > --- a/drivers/usb/dwc3/dwc3-meson-g12a.c > +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c the comment block at the start of the driver file also contains a "TOFIX" which points to the missing IRQ handling can you please also drop that TOFIX comment in lines 15/16? > @@ -348,6 +348,22 @@ static enum usb_role dwc3_meson_g12a_role_get(struct device *dev) > USB_ROLE_HOST : USB_ROLE_DEVICE; > } > > +static irqreturn_t dwc3_meson_g12a_irq_thread(int irq, void *data) > +{ > + struct dwc3_meson_g12a *priv = data; > + enum phy_mode otg_id; > + > + otg_id = dwc3_meson_g12a_get_id(priv); > + if (otg_id != priv->otg_phy_mode) { > + if (dwc3_meson_g12a_otg_mode_set(priv, otg_id)) > + dev_warn(priv->dev, "Failed to switch OTG mode\n"); > + } > + > + regmap_update_bits(priv->regmap, USB_R5, USB_R5_ID_DIG_IRQ, 0); > + > + return IRQ_HANDLED; > +} > + > static struct device *dwc3_meson_g12_find_child(struct device *dev, > const char *compatible) > { > @@ -374,7 +390,7 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) > void __iomem *base; > struct resource *res; > enum phy_mode otg_id; > - int ret, i; > + int ret, i, irq; > > priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > if (!priv) > @@ -436,6 +452,19 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) > /* Get dr_mode */ > priv->otg_mode = usb_get_dr_mode(dev); > > + if (priv->otg_mode == USB_DR_MODE_OTG) { > + /* Ack irq before registering */ > + regmap_update_bits(priv->regmap, USB_R5, > + USB_R5_ID_DIG_IRQ, 0); I assume that either the IRQ line is: - always enabled - enabled when (USB_R5_ID_DIG_EN_0 | USB_R5_ID_DIG_EN_1 | USB_R5_ID_DIG_TH_MASK) are set (which we already do in dwc3_meson_g12a_usb_init) > + irq = platform_get_irq(pdev, 0); do we need to check the IRQ before trying to request it? drivers/gpu/drm/meson/meson_dw_hdmi.c and drivers/usb/dwc3/host.c for example error out if irq number is lower than 0 (it's great to see that this only required a small patch to make it work :)) Martin
On 11/06/2019 20:08, Martin Blumenstingl wrote: > Hi Neil, > > On Tue, Jun 11, 2019 at 3:58 PM Neil Armstrong <narmstrong@baylibre.com> wrote: >> >> Add support for the OTG ID change interrupt to switch between Host >> and Device mode. >> >> Tested on the Hardkernel Odroid-N2 board. >> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > with the three questions/comments below answered/addressed: > Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > >> --- >> drivers/usb/dwc3/dwc3-meson-g12a.c | 32 ++++++++++++++++++++++++++++-- >> 1 file changed, 30 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c >> index 2aec31a2eacb..e5c5ad0d529e 100644 >> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c >> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c > the comment block at the start of the driver file also contains a > "TOFIX" which points to the missing IRQ handling > can you please also drop that TOFIX comment in lines 15/16? Indeed, thanks for pointing that ! > >> @@ -348,6 +348,22 @@ static enum usb_role dwc3_meson_g12a_role_get(struct device *dev) >> USB_ROLE_HOST : USB_ROLE_DEVICE; >> } >> >> +static irqreturn_t dwc3_meson_g12a_irq_thread(int irq, void *data) >> +{ >> + struct dwc3_meson_g12a *priv = data; >> + enum phy_mode otg_id; >> + >> + otg_id = dwc3_meson_g12a_get_id(priv); >> + if (otg_id != priv->otg_phy_mode) { >> + if (dwc3_meson_g12a_otg_mode_set(priv, otg_id)) >> + dev_warn(priv->dev, "Failed to switch OTG mode\n"); >> + } >> + >> + regmap_update_bits(priv->regmap, USB_R5, USB_R5_ID_DIG_IRQ, 0); >> + >> + return IRQ_HANDLED; >> +} >> + >> static struct device *dwc3_meson_g12_find_child(struct device *dev, >> const char *compatible) >> { >> @@ -374,7 +390,7 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) >> void __iomem *base; >> struct resource *res; >> enum phy_mode otg_id; >> - int ret, i; >> + int ret, i, irq; >> >> priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >> if (!priv) >> @@ -436,6 +452,19 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) >> /* Get dr_mode */ >> priv->otg_mode = usb_get_dr_mode(dev); >> >> + if (priv->otg_mode == USB_DR_MODE_OTG) { >> + /* Ack irq before registering */ >> + regmap_update_bits(priv->regmap, USB_R5, >> + USB_R5_ID_DIG_IRQ, 0); > I assume that either the IRQ line is: > - always enabled > - enabled when (USB_R5_ID_DIG_EN_0 | USB_R5_ID_DIG_EN_1 | > USB_R5_ID_DIG_TH_MASK) are set (which we already do in > dwc3_meson_g12a_usb_init) Can't say... I suspect the (USB_R5_ID_DIG_EN_0 | USB_R5_ID_DIG_EN_1 | > USB_R5_ID_DIG_TH_MASK) enables the detection. The regmap_update_bits(USB_R5_ID_DIG_IRQ) is only here to make sure the "current" irq event is masked, whatever the previous init. Or I misunderstood question ? > >> + irq = platform_get_irq(pdev, 0); > do we need to check the IRQ before trying to request it? > drivers/gpu/drm/meson/meson_dw_hdmi.c and drivers/usb/dwc3/host.c for > example error out if irq number is lower than 0 No, devm_request_threaded_irq() will fail if -1 is given, I've using this scheme for a while now ! Neil > > (it's great to see that this only required a small patch to make it work :)) yeah, I was impressed when I wrote it, I expected much more work > > > Martin Thanks, Neil >
Hi Neil, On Wed, Jun 12, 2019 at 5:13 PM Neil Armstrong <narmstrong@baylibre.com> wrote: [...] > >> @@ -436,6 +452,19 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) > >> /* Get dr_mode */ > >> priv->otg_mode = usb_get_dr_mode(dev); > >> > >> + if (priv->otg_mode == USB_DR_MODE_OTG) { > >> + /* Ack irq before registering */ > >> + regmap_update_bits(priv->regmap, USB_R5, > >> + USB_R5_ID_DIG_IRQ, 0); > > I assume that either the IRQ line is: > > - always enabled > > - enabled when (USB_R5_ID_DIG_EN_0 | USB_R5_ID_DIG_EN_1 | > > USB_R5_ID_DIG_TH_MASK) are set (which we already do in > > dwc3_meson_g12a_usb_init) > > Can't say... I suspect the (USB_R5_ID_DIG_EN_0 | USB_R5_ID_DIG_EN_1 | > > USB_R5_ID_DIG_TH_MASK) enables the detection. > The regmap_update_bits(USB_R5_ID_DIG_IRQ) is only here to make sure the "current" > irq event is masked, whatever the previous init. > > Or I misunderstood question ? that perfectly answers my question - thank you! > > > >> + irq = platform_get_irq(pdev, 0); > > do we need to check the IRQ before trying to request it? > > drivers/gpu/drm/meson/meson_dw_hdmi.c and drivers/usb/dwc3/host.c for > > example error out if irq number is lower than 0 > > No, devm_request_threaded_irq() will fail if -1 is given, I've using this scheme > for a while now ! OK, it wasn't obvious to me when I looked at devm_request_threaded_irq. thank you for clarifying this Martin
diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c index 2aec31a2eacb..e5c5ad0d529e 100644 --- a/drivers/usb/dwc3/dwc3-meson-g12a.c +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c @@ -348,6 +348,22 @@ static enum usb_role dwc3_meson_g12a_role_get(struct device *dev) USB_ROLE_HOST : USB_ROLE_DEVICE; } +static irqreturn_t dwc3_meson_g12a_irq_thread(int irq, void *data) +{ + struct dwc3_meson_g12a *priv = data; + enum phy_mode otg_id; + + otg_id = dwc3_meson_g12a_get_id(priv); + if (otg_id != priv->otg_phy_mode) { + if (dwc3_meson_g12a_otg_mode_set(priv, otg_id)) + dev_warn(priv->dev, "Failed to switch OTG mode\n"); + } + + regmap_update_bits(priv->regmap, USB_R5, USB_R5_ID_DIG_IRQ, 0); + + return IRQ_HANDLED; +} + static struct device *dwc3_meson_g12_find_child(struct device *dev, const char *compatible) { @@ -374,7 +390,7 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) void __iomem *base; struct resource *res; enum phy_mode otg_id; - int ret, i; + int ret, i, irq; priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); if (!priv) @@ -436,6 +452,19 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) /* Get dr_mode */ priv->otg_mode = usb_get_dr_mode(dev); + if (priv->otg_mode == USB_DR_MODE_OTG) { + /* Ack irq before registering */ + regmap_update_bits(priv->regmap, USB_R5, + USB_R5_ID_DIG_IRQ, 0); + + irq = platform_get_irq(pdev, 0); + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, + dwc3_meson_g12a_irq_thread, + IRQF_ONESHOT, pdev->name, priv); + if (ret) + return ret; + } + dwc3_meson_g12a_usb_init(priv); /* Init PHYs */ @@ -460,7 +489,6 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) /* Setup OTG mode corresponding to the ID pin */ if (priv->otg_mode == USB_DR_MODE_OTG) { - /* TOFIX Handle ID mode toggling via IRQ */ otg_id = dwc3_meson_g12a_get_id(priv); if (otg_id != priv->otg_phy_mode) { if (dwc3_meson_g12a_otg_mode_set(priv, otg_id))
Add support for the OTG ID change interrupt to switch between Host and Device mode. Tested on the Hardkernel Odroid-N2 board. Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- drivers/usb/dwc3/dwc3-meson-g12a.c | 32 ++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-)