| Message ID | 1551438348-22119-3-git-send-email-fabrizio.castro@bp.renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add USB-HOST support to cat874 | expand |
On 01/03/19 4:35 PM, Fabrizio Castro wrote: > As stated in the comment found on top of rcar_gen3_phy_usb2_work, > we should not be registering the IRQ if !otg_channel || !uses_otg_pins. > On top of that, we should also make sure that extcon has been > allocated before requesting the irq as rcar_gen3_phy_usb2_work > uses it, hence the patch. > > Fixes: 2b38543c8db1 ("phy: rcar-gen3-usb2: add extcon support") > Fixes: 73801b90a38f ("phy: renesas: rcar-gen3-usb2: change a condition "dr_mode"") > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> merged, thanks. -Kishon > --- > drivers/phy/renesas/phy-rcar-gen3-usb2.c | 26 +++++++++++++------------- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > index 0a34782..be1a392 100644 > --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c > +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > @@ -447,20 +447,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(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); > - } > - > channel->dr_mode = of_usb_get_dr_mode_by_phy(dev->of_node, 0); > - if (channel->dr_mode != USB_DR_MODE_UNKNOWN) { > - int ret; > - > + if (channel->dr_mode == USB_DR_MODE_OTG) { > channel->is_otg_channel = true; > channel->uses_otg_pins = !of_property_read_bool(dev->of_node, > "renesas,no-otg-pins"); > @@ -476,6 +464,18 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev) > } > } > > + if (channel->is_otg_channel && channel->uses_otg_pins) { > + /* call request_irq for OTG */ > + irq = platform_get_irq(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); > + } > + } > + > /* > * devm_phy_create() will call pm_runtime_enable(&phy->dev); > * And then, phy-core will manage runtime pm for this device. >
Hi Kishon, Fabrizio, > From: Kishon Vijay Abraham I, Sent: Monday, April 1, 2019 9:26 PM > > On 01/03/19 4:35 PM, Fabrizio Castro wrote: > > As stated in the comment found on top of rcar_gen3_phy_usb2_work, > > we should not be registering the IRQ if !otg_channel || !uses_otg_pins. > > On top of that, we should also make sure that extcon has been > > allocated before requesting the irq as rcar_gen3_phy_usb2_work > > uses it, hence the patch. > > > > Fixes: 2b38543c8db1 ("phy: rcar-gen3-usb2: add extcon support") > > Fixes: 73801b90a38f ("phy: renesas: rcar-gen3-usb2: change a condition "dr_mode"") > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > merged, thanks. I'm very sorry, I didn't notice this patch... I'd like to fix this patch because this patch breaks for R-Car D3. In this case, should I submit an incremental patch? > -Kishon > > --- > > drivers/phy/renesas/phy-rcar-gen3-usb2.c | 26 +++++++++++++------------- > > 1 file changed, 13 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > > index 0a34782..be1a392 100644 > > --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c > > +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > > @@ -447,20 +447,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(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); > > - } > > - > > channel->dr_mode = of_usb_get_dr_mode_by_phy(dev->of_node, 0); > > - if (channel->dr_mode != USB_DR_MODE_UNKNOWN) { > > - int ret; > > - > > + if (channel->dr_mode == USB_DR_MODE_OTG) { R-Car D3 (r8a77995-draak.dts) has dr_mode with "host". So, the board can use host mode as default and "role" mode to switch the role for peripheral later. However, after we applied this patch, the R-Car D3 USB2.0 cannot switch to the peripheral mode. Best regards, Yoshihiro Shimoda > > channel->is_otg_channel = true; > > channel->uses_otg_pins = !of_property_read_bool(dev->of_node, > > "renesas,no-otg-pins"); > > @@ -476,6 +464,18 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev) > > } > > } > > > > + if (channel->is_otg_channel && channel->uses_otg_pins) { > > + /* call request_irq for OTG */ > > + irq = platform_get_irq(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); > > + } > > + } > > + > > /* > > * devm_phy_create() will call pm_runtime_enable(&phy->dev); > > * And then, phy-core will manage runtime pm for this device. > >
Hello Yoshihiro-san, Kishon > From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > Sent: 02 April 2019 03:11 > Subject: RE: [PATCH 2/4] phy: renesas: rcar-gen3-usb2: No need to request IRQ for non-OTG > > Hi Kishon, Fabrizio, > > > From: Kishon Vijay Abraham I, Sent: Monday, April 1, 2019 9:26 PM > > > > On 01/03/19 4:35 PM, Fabrizio Castro wrote: > > > As stated in the comment found on top of rcar_gen3_phy_usb2_work, > > > we should not be registering the IRQ if !otg_channel || !uses_otg_pins. > > > On top of that, we should also make sure that extcon has been > > > allocated before requesting the irq as rcar_gen3_phy_usb2_work > > > uses it, hence the patch. > > > > > > Fixes: 2b38543c8db1 ("phy: rcar-gen3-usb2: add extcon support") > > > Fixes: 73801b90a38f ("phy: renesas: rcar-gen3-usb2: change a condition "dr_mode"") > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > > > merged, thanks. > > I'm very sorry, I didn't notice this patch... > I'd like to fix this patch because this patch breaks for R-Car D3. > In this case, should I submit an incremental patch? Kishon, is it too late to drop the patch? > > > -Kishon > > > --- > > > drivers/phy/renesas/phy-rcar-gen3-usb2.c | 26 +++++++++++++------------- > > > 1 file changed, 13 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > > > index 0a34782..be1a392 100644 > > > --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c > > > +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > > > @@ -447,20 +447,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(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); > > > - } > > > - > > > channel->dr_mode = of_usb_get_dr_mode_by_phy(dev->of_node, 0); > > > - if (channel->dr_mode != USB_DR_MODE_UNKNOWN) { > > > - int ret; > > > - > > > + if (channel->dr_mode == USB_DR_MODE_OTG) { > > R-Car D3 (r8a77995-draak.dts) has dr_mode with "host". So, the board can use host mode > as default and "role" mode to switch the role for peripheral later. However, after we > applied this patch, the R-Car D3 USB2.0 cannot switch to the peripheral mode. I understand, I am sorry for breaking it. The probing function still needs some fixing, devm_request_irq uses rcar_gen3_phy_usb2_irq, rcar_gen3_phy_usb2_irq calls rcar_gen3_device_recognition, rcar_gen3_device_recognition may call either rcar_gen3_init_for_host or rcar_gen3_init_for_peri, both rcar_gen3_init_for_host and rcar_gen3_init_for_peri schedule work for the work queue that calls into rcar_gen3_phy_usb2_work, rcar_gen3_phy_usb2_work uses extcon, extcon gets initialized after devm_request_irq, which means we should move devm_request_irq after extcon gets initialized,. Also, we may register the irq, and never deal with extcon if dr_mode == USB_DR_MODE_UNKNOWN. Would you like to come up with a patch to address those points? Thanks, Fab > > Best regards, > Yoshihiro Shimoda > > > > channel->is_otg_channel = true; > > > channel->uses_otg_pins = !of_property_read_bool(dev->of_node, > > > "renesas,no-otg-pins"); > > > @@ -476,6 +464,18 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev) > > > } > > > } > > > > > > + if (channel->is_otg_channel && channel->uses_otg_pins) { > > > + /* call request_irq for OTG */ > > > + irq = platform_get_irq(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); > > > + } > > > + } > > > + > > > /* > > > * devm_phy_create() will call pm_runtime_enable(&phy->dev); > > > * And then, phy-core will manage runtime pm for this device. > > >
Hi, On 03/04/19 3:54 PM, Fabrizio Castro wrote: > Hello Yoshihiro-san, Kishon > >> From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> >> Sent: 02 April 2019 03:11 >> Subject: RE: [PATCH 2/4] phy: renesas: rcar-gen3-usb2: No need to request IRQ for non-OTG >> >> Hi Kishon, Fabrizio, >> >>> From: Kishon Vijay Abraham I, Sent: Monday, April 1, 2019 9:26 PM >>> >>> On 01/03/19 4:35 PM, Fabrizio Castro wrote: >>>> As stated in the comment found on top of rcar_gen3_phy_usb2_work, >>>> we should not be registering the IRQ if !otg_channel || !uses_otg_pins. >>>> On top of that, we should also make sure that extcon has been >>>> allocated before requesting the irq as rcar_gen3_phy_usb2_work >>>> uses it, hence the patch. >>>> >>>> Fixes: 2b38543c8db1 ("phy: rcar-gen3-usb2: add extcon support") >>>> Fixes: 73801b90a38f ("phy: renesas: rcar-gen3-usb2: change a condition "dr_mode"") >>>> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> >>> >>> merged, thanks. >> >> I'm very sorry, I didn't notice this patch... >> I'd like to fix this patch because this patch breaks for R-Car D3. >> In this case, should I submit an incremental patch? > > Kishon, is it too late to drop the patch? I have to send a new pul request. I'll drop this patch then. Thanks for letting me know. -Kishon > >> >>> -Kishon >>>> --- >>>> drivers/phy/renesas/phy-rcar-gen3-usb2.c | 26 +++++++++++++------------- >>>> 1 file changed, 13 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c >>>> index 0a34782..be1a392 100644 >>>> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c >>>> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c >>>> @@ -447,20 +447,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(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); >>>> - } >>>> - >>>> channel->dr_mode = of_usb_get_dr_mode_by_phy(dev->of_node, 0); >>>> - if (channel->dr_mode != USB_DR_MODE_UNKNOWN) { >>>> - int ret; >>>> - >>>> + if (channel->dr_mode == USB_DR_MODE_OTG) { >> >> R-Car D3 (r8a77995-draak.dts) has dr_mode with "host". So, the board can use host mode >> as default and "role" mode to switch the role for peripheral later. However, after we >> applied this patch, the R-Car D3 USB2.0 cannot switch to the peripheral mode. > > I understand, I am sorry for breaking it. > The probing function still needs some fixing, devm_request_irq uses rcar_gen3_phy_usb2_irq, > rcar_gen3_phy_usb2_irq calls rcar_gen3_device_recognition, rcar_gen3_device_recognition may > call either rcar_gen3_init_for_host or rcar_gen3_init_for_peri, both rcar_gen3_init_for_host and > rcar_gen3_init_for_peri schedule work for the work queue that calls into rcar_gen3_phy_usb2_work, > rcar_gen3_phy_usb2_work uses extcon, extcon gets initialized after devm_request_irq, which > means we should move devm_request_irq after extcon gets initialized,. > Also, we may register the irq, and never deal with extcon if dr_mode == USB_DR_MODE_UNKNOWN. > > Would you like to come up with a patch to address those points? > > Thanks, > Fab > >> >> Best regards, >> Yoshihiro Shimoda >> >>>> channel->is_otg_channel = true; >>>> channel->uses_otg_pins = !of_property_read_bool(dev->of_node, >>>> "renesas,no-otg-pins"); >>>> @@ -476,6 +464,18 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev) >>>> } >>>> } >>>> >>>> + if (channel->is_otg_channel && channel->uses_otg_pins) { >>>> + /* call request_irq for OTG */ >>>> + irq = platform_get_irq(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); >>>> + } >>>> + } >>>> + >>>> /* >>>> * devm_phy_create() will call pm_runtime_enable(&phy->dev); >>>> * And then, phy-core will manage runtime pm for this device. >>>>
diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c index 0a34782..be1a392 100644 --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c @@ -447,20 +447,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(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); - } - channel->dr_mode = of_usb_get_dr_mode_by_phy(dev->of_node, 0); - if (channel->dr_mode != USB_DR_MODE_UNKNOWN) { - int ret; - + if (channel->dr_mode == USB_DR_MODE_OTG) { channel->is_otg_channel = true; channel->uses_otg_pins = !of_property_read_bool(dev->of_node, "renesas,no-otg-pins"); @@ -476,6 +464,18 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev) } } + if (channel->is_otg_channel && channel->uses_otg_pins) { + /* call request_irq for OTG */ + irq = platform_get_irq(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); + } + } + /* * devm_phy_create() will call pm_runtime_enable(&phy->dev); * And then, phy-core will manage runtime pm for this device.
As stated in the comment found on top of rcar_gen3_phy_usb2_work, we should not be registering the IRQ if !otg_channel || !uses_otg_pins. On top of that, we should also make sure that extcon has been allocated before requesting the irq as rcar_gen3_phy_usb2_work uses it, hence the patch. Fixes: 2b38543c8db1 ("phy: rcar-gen3-usb2: add extcon support") Fixes: 73801b90a38f ("phy: renesas: rcar-gen3-usb2: change a condition "dr_mode"") Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> --- drivers/phy/renesas/phy-rcar-gen3-usb2.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)