| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <cabf24d0-8eea-4eb5-8205-bf7fe6017ec2@quicinc.com>
Date: Mon, 23 Oct 2023 16:54:11 +0530
From: Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
To: Johan Hovold <johan@...nel.org>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Philipp Zabel <p.zabel@...gutronix.de>,
"Andy Gross" <agross@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
"Konrad Dybcio" <konrad.dybcio@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Felipe Balbi <balbi@...nel.org>,
Wesley Cheng <quic_wcheng@...cinc.com>,
<linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
<quic_pkondeti@...cinc.com>, <quic_ppratap@...cinc.com>,
<quic_jackp@...cinc.com>, <ahalaney@...hat.com>,
<quic_shazhuss@...cinc.com>
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM
Glue driver
On 10/23/2023 2:51 PM, Johan Hovold wrote:
> On Mon, Oct 23, 2023 at 12:11:45AM +0530, Krishna Kurapati PSSNV wrote:
>> On 10/20/2023 6:53 PM, Johan Hovold wrote:
>>> On Sat, Oct 07, 2023 at 09:18:01PM +0530, Krishna Kurapati wrote:
>
>>>> +#define NUM_PHY_IRQ 4
>>>> +
>>>> +enum dwc3_qcom_ph_index {
>>>
>>> "phy_index"
>>>
>>>> + DP_HS_PHY_IRQ_INDEX = 0,
>>>> + DM_HS_PHY_IRQ_INDEX,
>>>> + SS_PHY_IRQ_INDEX,
>>>> + HS_PHY_IRQ_INDEX,
>>>> +};
>>>> +
>>>> struct dwc3_acpi_pdata {
>>>> u32 qscratch_base_offset;
>>>> u32 qscratch_base_size;
>>>> u32 dwc3_core_base_size;
>>>> + /*
>>>> + * The phy_irq_index corresponds to ACPI indexes of (in order) DP/DM/SS
>>>> + * IRQ's respectively.
>>>> + */
>>>> + int phy_irq_index[NUM_PHY_IRQ - 1];
>>>> int hs_phy_irq_index;
>>>> - int dp_hs_phy_irq_index;
>>>> - int dm_hs_phy_irq_index;
>>>> - int ss_phy_irq_index;
>>>> bool is_urs;
>>>> };
>>>>
>>>> @@ -73,10 +84,12 @@ struct dwc3_qcom {
>>>> int num_clocks;
>>>> struct reset_control *resets;
>>>>
>>>> + /*
>>>> + * The phy_irq corresponds to IRQ's registered for (in order) DP/DM/SS
>>>> + * respectively.
>>>> + */
>>>> + int phy_irq[NUM_PHY_IRQ - 1][DWC3_MAX_PORTS];
>>>> int hs_phy_irq;
>>>> - int dp_hs_phy_irq;
>>>> - int dm_hs_phy_irq;
>>>> - int ss_phy_irq;
>>>
>>> I'm not sure using arrays like this is a good idea (and haven't you
>>> switched the indexes above?).
>>>
>>> Why not add a port structure instead?
>>>
>>> struct dwc3_qcom_port {
>>> int hs_phy_irq;
>>> int dp_hs_phy_irq;
>>> int dm_hs_phy_irq;
>>> int ss_phy_irq;
>>> };
>>>
>>> and then have
>>>
>>> struct dwc3_qcom_port ports[DWC3_MAX_PORTS];
>>>
>>> in dwc3_qcom. The port structure can the later also be amended with
>>> whatever other additional per-port data there is need for.
>>>
>>> This should make the implementation cleaner.
>>>
>>> I also don't like the special handling of hs_phy_irq; if this is really
>>> just another name for the pwr_event_irq then this should be cleaned up
>>> before making the code more complicated than it needs to be.
>>>
>>> Make sure to clarify this before posting a new revision.
>>
>> hs_phy_irq is different from pwr_event_irq.
>
> How is it different and how are they used?
>
>> AFAIK, there is only one of this per controller.
>
> But previous controllers were all single port so this interrupt is
> likely also per-port, even if your comment below seems to suggest even
> SC8280XP has one, which is unexpected (and not described in the updated
> binding):
>
> Yes, all targets have the same IRQ's. Just that MP one's have
> multiple IRQ's of each type. But hs-phy_irq is only one in
> SC8280 as well.
>
> https://lore.kernel.org/lkml/70b2495f-1305-05b1-2039-9573d171fe24@quicinc.com/
>
> Please clarify.
>
For sure pwr_event_irq and hs_phy_irq are different. I assumed it was
per-controller and not per-phy because I took reference from software
code we have on downstream and hs_phy for multiport is not used
anywhere. I don't see any functionality implemented in downstream for
that IRQ. And it is only one for single port controllers.
But I got the following info from HW page and these are all the
interrupts (on apss processor) for multiport (extra details removed):
u_usb31_scnd_mvs_pipe_wrapper_usb31_power_event_irq_0 SYS_apcsQgicSPI[130]
u_usb31_scnd_mvs_pipe_wrapper_usb31_power_event_irq_1 SYS_apcsQgicSPI[135]
u_usb31_scnd_mvs_pipe_wrapper_usb31_power_event_irq_3 SYS_apcsQgicSPI[856]
u_usb31_scnd_mvs_pipe_wrapper_usb31_power_event_irq_2 SYS_apcsQgicSPI[857]
u_usb31_scnd_mvs_pipe_wrapper_usb31_ctrl_irq[0] SYS_apcsQgicSPI[133]
u_usb31_scnd_mvs_pipe_wrapper_usb31_ctrl_irq[1] SYS_apcsQgicSPI[134]
u_cm_usb3_uni_wrapper_mp0_usb3phy_debug_irq SYS_apcsQgicSPI[668]
u_usb31_scnd_mvs_pipe_wrapper_usb31_bam_irq[0] SYS_apcsQgicSPI[830]
u_cm_usb3_uni_wrapper_mp1_usb3phy_debug_irq SYS_apcsQgicSPI[855]
u_usb31_scnd_mvs_pipe_wrapper_usb31_hs_phy_irq_0 SYS_apcsQgicSPI[131]
u_usb31_scnd_mvs_pipe_wrapper_usb31_hs_phy_irq_1 SYS_apcsQgicSPI[136]
u_usb31_scnd_mvs_pipe_wrapper_usb31_hs_phy_irq_3 SYS_apcsQgicSPI[859]
u_usb31_scnd_mvs_pipe_wrapper_usb31_hs_phy_irq_2 SYS_apcsQgicSPI[860]
u_cm_dwc_usb2_hs0_usb2_dpse apps_pdc_irq_out[127]
u_cm_dwc_usb2_hs0_usb2_dmse apps_pdc_irq_out[126]
u_cm_dwc_usb2_hs1_usb2_dpse apps_pdc_irq_out[129]
u_cm_dwc_usb2_hs1_usb2_dmse apps_pdc_irq_out[128]
u_cm_dwc_usb2_hs2_usb2_dpse apps_pdc_irq_out[131]
u_cm_dwc_usb2_hs2_usb2_dmse apps_pdc_irq_out[130]
u_cm_dwc_usb2_hs3_usb2_dpse apps_pdc_irq_out[133]
u_cm_dwc_usb2_hs3_usb2_dmse apps_pdc_irq_out[132]
u_cm_usb3_uni_wrapper_mp0_qmp_usb3_lfps_rxterm_irq apps_pdc_irq_out[16]
u_cm_usb3_uni_wrapper_mp1_qmp_usb3_lfps_rxterm_irq apps_pdc_irq_out[17]
Seems like there are 4 IRQ's for HS.
Regards,
Krishna,
Powered by blists - more mailing lists