Hi,
On 03/04/19 3:54 PM, Fabrizio Castro wrote:
> Hello Yoshihiro-san, Kishon
>
>> From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
>> 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@xxxxxxxxxxxxxx>
>>>
>>> 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.
>>>>
