Re: [PATCH v10 06/11] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 9.08.2023 08:06, Krishna Kurapati PSSNV wrote:
> 
> 
> On 8/8/2023 5:20 PM, Konrad Dybcio wrote:
>> On 8.08.2023 10:32, Krishna Kurapati PSSNV wrote:
>>>   +
>>>>> +enum dwc3_qcom_phy_irq_identifier {
>>>>> +    HS_PHY_IRQ = 0,
>>>>> +    DP_HS_PHY_IRQ,
>>>>> +    DM_HS_PHY_IRQ,
>>>>> +    SS_PHY_IRQ,
>>>>>    };
>>>>
>>>> This enum is unused.
>>>>
>>>
>>> Hi Bjorn,
>>>
>>>   I didn't use the enum directly, but used its members in the get_port_irq call below.
>>>
>>>> [..]
>>>>> +static int dwc3_get_acpi_index(const struct dwc3_acpi_pdata *pdata, int irq_index)
>>>>> +{
>>>>> +    int acpi_index = -1;
>>>>> +
>>>>> +    if (!pdata)
>>>>> +        return -1;
>>>>> +
>>>>> +    if (irq_index == DP_HS_PHY_IRQ)
>>>>> +        acpi_index = pdata->dp_hs_phy_irq_index;
>>>>> +    else if (irq_index == DM_HS_PHY_IRQ)
>>>>> +        acpi_index = pdata->dm_hs_phy_irq_index;
>>>>> +    else if (irq_index == SS_PHY_IRQ)
>>>>> +        acpi_index = pdata->ss_phy_irq_index;
>>>>
>>>> It looks favourable to put these in an array, instead of having to pull
>>>> them out of 4 different variables conditionally.
>>>>
>>>>> +
>>>>> +    return acpi_index;
>>>>> +}
>>>>> +
>>>>> +static int dwc3_get_port_irq(struct platform_device *pdev, u8 port_index)
>>>>> +{
>>>>> +    struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
>>>>> +    bool is_mp_supported = (qcom->data->num_ports > 1) ? true : false;
>>>>> +    const struct dwc3_acpi_pdata *pdata = qcom->acpi_pdata;
>>>>> +    char *disp_name;
>>>>> +    int acpi_index;
>>>>> +    char *dt_name;
>>>>> +    int ret;
>>>>> +    int irq;
>>>>> +    int i;
>>>>> +
>>>>> +    /*
>>>>> +     * We need to read only DP/DM/SS IRQ's here.
>>>>> +     * So loop over from 1->3 and accordingly modify respective phy_irq[].
>>>>> +     */
>>>>> +    for (i = 1; i < MAX_PHY_IRQ; i++) {
>>>>> +
>>>>> +        if (!is_mp_supported && (port_index == 0)) {
>>>>> +            if (i == DP_HS_PHY_IRQ) {
>>>>> +                dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
>>>>> +                    "dp_hs_phy_irq");
>>>>> +                disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
>>>>> +                    "qcom_dwc3 DP_HS");
>>>>> +            } else if (i == DM_HS_PHY_IRQ) {
>>>>> +                dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
>>>>> +                    "dm_hs_phy_irq");
>>>>> +                disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
>>>>> +                    "qcom_dwc3 DM_HS");
>>>>> +            } else if (i == SS_PHY_IRQ) {
>>>>> +                dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
>>>>> +                    "ss_phy_irq");
>>>>> +                disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
>>>>> +                    "qcom_dwc3 SS");
>>> Bjorn, Konrad,
>>>
>>> If we are to remove this repetitive loops, we might need to make a 2D array for all of Dp/Dm/Ss interrutps and make a global array of names to be used for irq lookup and use them to reduce the if-else-if stuff here. If that is fine, I can make those changes, else I would like to stick to this approach for now because if we don't add the global array of names, prepping them seperately for dp/dm/ss would again lead us to making if-else loops like above.
>>>
>>> Please let me know your thoughts on this.
>> Can we not just reuse the associated interrupt-names from the devicetree
>> if present?
>>
> Hi Konrad,
> 
>  Thanks for the comments but one more confirmation.
> We can read the interrupts from DT but I believe the compatible would still need to stay. We need the num_ports information not just for registering interrupts but for modifying the pwr_event_irq registers during suspend/resume. If we rely on the interrupts to find the number of ports, the user is free to remove any IRQ and we might end up in a situation where glue and core are not having same view of how many number of ports present. So I believe its best to keep the compatible and get num_ports info from there and rely on reading interrupt-names to get interrupts cleanly. Can you let me know your view on the same.
So is "is it okay to add SoC-specific compatibles and add the port number in
match data" what you're asking?

If so, that doesn't seem right.

The user should not "feel free to remove any IRQ", modifying the devicetree to
depict a subset of the hardware is not something we want to support. The driver
has to work with the "full" description in accordance with the bindings.

Konrad