Re: [PATCH v9 06/10] usb: dwc3: qcom: Add support to read IRQ's related to multiport

[Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>]

On 7/3/2023 12:29 AM, Krishna Kurapati PSSNV wrote:
> 
> 
> On 6/27/2023 8:01 PM, Johan Hovold wrote:
>> On Wed, Jun 21, 2023 at 10:06:24AM +0530, Krishna Kurapati wrote:
>>> Add support to read Multiport IRQ's related to quad port controller
>>> of SA8295 Device.
>>
>> Please use a more descriptive summary and commit message; "read" is to
>> vague. You're looking up interrupts from the devicetree. Also this
>> should not just be about SA8295.
>>
>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>> ---
>>>   drivers/usb/dwc3/dwc3-qcom.c | 108 +++++++++++++++++++++++++++++------
>>>   1 file changed, 91 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
>>> index 3de43df6bbe8..3ab48a6925fe 100644
>>> --- a/drivers/usb/dwc3/dwc3-qcom.c
>>> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>>> @@ -74,9 +74,9 @@ struct dwc3_qcom {
>>>       struct reset_control    *resets;
>>>       int            hs_phy_irq;
>>> -    int            dp_hs_phy_irq;
>>> -    int            dm_hs_phy_irq;
>>> -    int            ss_phy_irq;
>>> +    int            dp_hs_phy_irq[4];
>>> +    int            dm_hs_phy_irq[4];
>>> +    int            ss_phy_irq[2];
>>
>> As has already been pointed out, you should use a define for these. And
>> you already have DWC3_MAX_PORTS.
>>
>> The driver should not be hardcoding the fact that there are only two SS
>> ports on this particular SoC that you're interested in.
>>
>>>       enum usb_device_speed    usb2_speed;
>>>       struct extcon_dev    *edev;
>>
>>> @@ -535,6 +535,80 @@ static int dwc3_qcom_get_irq(struct 
>>> platform_device *pdev,
>>>       return ret;
>>>   }
>>> +static int dwc3_qcom_setup_mp_irq(struct platform_device *pdev)
>>> +{
>>> +    struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
>>> +    char irq_name[15];
>>> +    int irq;
>>> +    int ret;
>>> +    int i;
>>> +
>>> +    for (i = 0; i < 4; i++) {
>>
>> DWC3_MAX_PORTS here too and similar below.
>>
>>> +        if (qcom->dp_hs_phy_irq[i])
>>> +            continue;
>>
>> This is not very nice. You should try to integrate the current lookup
>> code as I told you to do with the PHY lookups. That is, use a single
>> loop for all HS/SS IRQs, and pick the legacy name if the number of ports
>> is 1.
>>
>> Of course, you added the xhci capability parsing to the core driver so
>> that information is not yet available, but you need it in the glue
>> driver also...
>>
>> As I mentioned earlier, you can infer the number of ports from the
>> interrupt names. Alternatively, you can infer it from the compatible
>> string. In any case, you should not need to ways to determine the same
>> information in the glue driver, then in the core part, and then yet
>> again in the xhci driver...
>>
> Hi Johan,
> 
>   The reason why I didn't integrate this with the original function was 
> the ACPI stuff. The MP devices have no ACPI variant. And I think for 
> clarity sake its best to keep these two functions separated.
> 
> Regards,
> Krishna,
> 
>>> +
>>> +        sprintf(irq_name, "dp%d_hs_phy_irq", i+1);
>>
>> Spaces around binary operators. Does not checkpatch warn about that?
>>
>>> +        irq = dwc3_qcom_get_irq(pdev, irq_name, -1);
>>> +        if (irq > 0) {
>>> +            irq_set_status_flags(irq, IRQ_NOAUTOEN);
>>> +            ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
>>> +                    qcom_dwc3_resume_irq,
>>> +                    IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
>>> +                    irq_name, qcom);
>>> +            if (ret) {
>>> +                dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret);
>>> +                return ret;
>>> +            }
>>> +        }
>>> +
>>> +        qcom->dp_hs_phy_irq[i] = irq;
>>> +    }
>>> +
>>> +    for (i = 0; i < 4; i++) {
>>> +        if (qcom->dm_hs_phy_irq[i])
>>> +            continue;
>>> +
>>> +        sprintf(irq_name, "dm%d_hs_phy_irq", i+1);
>>> +        irq = dwc3_qcom_get_irq(pdev, irq_name, -1);
>>> +        if (irq > 0) {
>>> +            irq_set_status_flags(irq, IRQ_NOAUTOEN);
>>> +            ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
>>> +                    qcom_dwc3_resume_irq,
>>> +                    IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
>>> +                    irq_name, qcom);
>>> +            if (ret) {
>>> +                dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret);
>>> +                return ret;
>>> +            }
>>> +        }
>>> +
>>> +        qcom->dm_hs_phy_irq[i] = irq;
>>> +    }
>>> +
>>> +    for (i = 0; i < 2; i++) {
>>> +        if (qcom->ss_phy_irq[i])
>>> +            continue;
>>> +
>>> +        sprintf(irq_name, "ss%d_phy_irq", i+1);
>>> +        irq = dwc3_qcom_get_irq(pdev, irq_name, -1);
>>> +        if (irq > 0) {
>>> +            irq_set_status_flags(irq, IRQ_NOAUTOEN);
>>> +            ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
>>> +                    qcom_dwc3_resume_irq,
>>> +                    IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
>>> +                    irq_name, qcom);
>>> +            if (ret) {
>>> +                dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret);
>>> +                return ret;
>>> +            }
>>> +        }
>>> +
>>> +        qcom->ss_phy_irq[i] = irq;
>>> +    }
>>
>> So the above should all be merged in either a single helper looking up
>> all the interrupts for a port and resused for the non-MP case.
>>
> I agree, Will merge all under some common helper function.
> 

Hi Johan,

  One more thought. To make one single function read all the interrupts 
(Single port or multi port), we need to provide proper inputs to get_irq 
call (i.e., "dp_hs_phy_irq" or dp_hs_phy_irq_X" and for that we need to 
know if device is multiport capable or not.

Either of the following ways needs to be done:

1. Try getting all interrupts (MP or single port) and accordingly save 
it in qcom struct like done in this patch in which case setup_irq and 
get_mp_irq being seperated is the best option and we don't need to 
bother about whether we are MP capable or not.

2. Else, we need to find out if we are MP capable and read number of 
ports and accordingly get the IRQ's which will just complicate the code 
because of_platform_populate is done after setup_irq. Even if I move 
setup_irq to after of_platform_populate, what dwc3 probe was deferred or 
failed, we would not know what IRQ's to read.

3. If we want to know whether we are MP capable or not in dwc3-qcom even 
before of_platform_populate, we need to find out a way to get port info 
which will just be duplication of code or re-implementation of code done 
in core.c [1].

4. One more option would be to defer qcom probe if dwc3 probe is not 
done and move setup_irq to be called after of_platform_populate. This 
way we can be sure we are not dereferencing dwc3->drvdata without 
knowing if it is NULL or not. Since qcom probe happened, we are sure 
dwc3 probe too happened. We would know if we are MP capable or not while 
setting up IRQ and we can read IRQ's appropriately.

Logically, dwc3-qcom is nothing without dwc3 core getting probed and 
becoming active. So it would be better IMO to defer qcom probe if dwc3 
probe doesn't happen and that way even the layering violations too would 
go away. I hope this idea appeals to the issues we are dealing with.

[1]: 
https://lore.kernel.org/all/20230621043628.21485-4-quic_kriskura@quicinc.com/

Adding Bjorn and Konrad as well to know their thoughts on Point-4.

Regards,
Krishna,


>>> +    return 0;
>>> +}
>>> +
>>>   static int dwc3_qcom_setup_irq(struct platform_device *pdev)
>>>   {
>>>       struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
>>> @@ -570,7 +644,7 @@ static int dwc3_qcom_setup_irq(struct 
>>> platform_device *pdev)
>>>               dev_err(qcom->dev, "dp_hs_phy_irq failed: %d\n", ret);
>>>               return ret;
>>>           }
>>> -        qcom->dp_hs_phy_irq = irq;
>>> +        qcom->dp_hs_phy_irq[0] = irq;
>>>       }
>>>       irq = dwc3_qcom_get_irq(pdev, "dm_hs_phy_irq",
>>> @@ -585,7 +659,7 @@ static int dwc3_qcom_setup_irq(struct 
>>> platform_device *pdev)
>>>               dev_err(qcom->dev, "dm_hs_phy_irq failed: %d\n", ret);
>>>               return ret;
>>>           }
>>> -        qcom->dm_hs_phy_irq = irq;
>>> +        qcom->dm_hs_phy_irq[0] = irq;
>>>       }
>>>       irq = dwc3_qcom_get_irq(pdev, "ss_phy_irq",
>>> @@ -600,10 +674,10 @@ static int dwc3_qcom_setup_irq(struct 
>>> platform_device *pdev)
>>>               dev_err(qcom->dev, "ss_phy_irq failed: %d\n", ret);
>>>               return ret;
>>>           }
>>> -        qcom->ss_phy_irq = irq;
>>> +        qcom->ss_phy_irq[0] = irq;
>>>       }
>>> -    return 0;
>>> +    return dwc3_qcom_setup_mp_irq(pdev);;
>>
>> Stray ;
>>
>>>   }
>>>   static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count)
>>
>> Johan