| Message ID | 20231007154806.605-6-quic_kriskura@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add multiport support for DWC3 controllers | expand |
First, drop "QCOM Glue driver" from Subject, you already have the "qcom" prefix. On Sat, Oct 07, 2023 at 09:18:01PM +0530, Krishna Kurapati wrote: > Refactor setup_irq call to facilitate reading multiport IRQ's along "IRQs" or just "interrupts" > with non mulitport ones. Read through the interrupt-names property "multiport" Please spell check all your patches (commit messages and code) before posting, it's not the reviewers job. > to figure out, the type of interrupt (DP/DM/HS/SS) and to which port > it belongs. Also keep track of port index to calculate port count > based on interrupts provided as input in DT. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/dwc3-qcom.c | 210 +++++++++++++++++++++++++---------- > 1 file changed, 154 insertions(+), 56 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index ef2006db7601..863892284146 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -53,14 +53,25 @@ > #define APPS_USB_AVG_BW 0 > #define APPS_USB_PEAK_BW MBps_to_icc(40) > > +#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. > enum usb_device_speed usb2_speed; > > struct extcon_dev *edev; > if (qcom->usb2_speed == USB_SPEED_LOW) { > - dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq); > + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]); For example, this would become dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq); which is much more readable. > -static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, char *irq_name, > - char *disp_name, int irq) > +static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, const char *irq_name, > + const char *disp_name, int irq) Ok, here you did drop the second name parameter, but without renaming the first and hidden in a long diff without any mention anywhere. > +static int dwc3_qcom_get_port_index(const char *irq_name, int irq_index) > +{ > + int port_index = -1; > + > + switch (irq_index) { > + case DP_HS_PHY_IRQ_INDEX: > + if (strcmp(irq_name, "dp_hs_phy_irq") == 0) > + port_index = 1; > + else > + sscanf(irq_name, "dp_hs_phy_%d", &port_index); > + break; > + No need for newlines after break. > + case DM_HS_PHY_IRQ_INDEX: > + if (strcmp(irq_name, "dm_hs_phy_irq") == 0) > + port_index = 1; > + else > + sscanf(irq_name, "dm_hs_phy_%d", &port_index); > + break; > + > + case SS_PHY_IRQ_INDEX: > + if (strcmp(irq_name, "ss_phy_irq") == 0) > + port_index = 1; > + else > + sscanf(irq_name, "ss_phy_%d", &port_index); > + break; > + > + case HS_PHY_IRQ_INDEX: > + port_index = 1; > + break; > + } > + > + if (port_index > DWC3_MAX_PORTS) > + port_index = -1; > + > + return port_index; > +} > static int dwc3_qcom_setup_irq(struct platform_device *pdev) > { > struct dwc3_qcom *qcom = platform_get_drvdata(pdev); > - const struct dwc3_acpi_pdata *pdata = qcom->acpi_pdata; > + struct device_node *np = pdev->dev.of_node; > + const char **irq_names; > + int port_index; > + int acpi_index; > + int irq_count; > + int irq_index; > int irq; > int ret; > + int i; > > - irq = dwc3_qcom_get_irq(pdev, "hs_phy_irq", > - pdata ? pdata->hs_phy_irq_index : -1); > - if (irq > 0) { > - ret = dwc3_qcom_prep_irq(qcom, "hs_phy_irq", "qcom_dwc3 HS", irq); > - if (ret) > - return ret; > - qcom->hs_phy_irq = irq; > - } > + irq_count = of_property_count_strings(np, "interrupt-names"); of_property_count_strings() can return negative errnos and you don't have any sanity checks for the return value... Please slow down, and also make sure to get your patches reviewed internally before posting new revisions. > + irq_names = devm_kzalloc(&pdev->dev, sizeof(*irq_names) * irq_count, GFP_KERNEL); devm_kcalloc() > + if (!irq_names) > + return -ENOMEM; > > - irq = dwc3_qcom_get_irq(pdev, "dp_hs_phy_irq", > - pdata ? pdata->dp_hs_phy_irq_index : -1); > - if (irq > 0) { > - ret = dwc3_qcom_prep_irq(qcom, "dp_hs_phy_irq", "qcom_dwc3 DP_HS", irq); > - if (ret) > - return ret; > - qcom->dp_hs_phy_irq = irq; > - } > + ret = of_property_read_string_array(np, "interrupt-names", > + irq_names, irq_count); No sanity check here either? > + for (i = 0; i < irq_count; i++) { > + irq_index = dwc3_qcom_get_irq_index(irq_names[i]); > + if (irq_index == -1) { > + dev_dbg(&pdev->dev, "Invalid IRQ not handled"); > + continue; > + } I'll just stop reviewing here. This is a waste of my time. Johan
On 10/20/2023 6:53 PM, Johan Hovold wrote: > First, drop "QCOM Glue driver" from Subject, you already have the "qcom" > prefix. > > On Sat, Oct 07, 2023 at 09:18:01PM +0530, Krishna Kurapati wrote: >> Refactor setup_irq call to facilitate reading multiport IRQ's along > > "IRQs" or just "interrupts" > >> with non mulitport ones. Read through the interrupt-names property > > "multiport" > > Please spell check all your patches (commit messages and code) before > posting, it's not the reviewers job. > >> to figure out, the type of interrupt (DP/DM/HS/SS) and to which port >> it belongs. Also keep track of port index to calculate port count >> based on interrupts provided as input in DT. >> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/dwc3-qcom.c | 210 +++++++++++++++++++++++++---------- >> 1 file changed, 154 insertions(+), 56 deletions(-) >> >> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c >> index ef2006db7601..863892284146 100644 >> --- a/drivers/usb/dwc3/dwc3-qcom.c >> +++ b/drivers/usb/dwc3/dwc3-qcom.c >> @@ -53,14 +53,25 @@ >> #define APPS_USB_AVG_BW 0 >> #define APPS_USB_PEAK_BW MBps_to_icc(40) >> >> +#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. AFAIK, there is only one of this per controller. >> -static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, char *irq_name, >> - char *disp_name, int irq) >> +static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, const char *irq_name, >> + const char *disp_name, int irq) > > Ok, here you did drop the second name parameter, but without renaming > the first and hidden in a long diff without any mention anywhere. > I didn't understand the comment. Can you please elaborate. I didn't drop the second parameter. In the usage of this call, I passed same value to both irq_name and disp_name: "dwc3_qcom_prep_irq(qcom, irq_names[i], irq_names[i], irq);" I mentioned in cover-letter that I am using same name as obtained from DT to register the interrupts as well. Should've mentioned that in commit text of this patch. Will do it in next version. >> +static int dwc3_qcom_get_port_index(const char *irq_name, int irq_index) >> +{ >> + int port_index = -1; >> + >> + switch (irq_index) { >> + case DP_HS_PHY_IRQ_INDEX: >> + if (strcmp(irq_name, "dp_hs_phy_irq") == 0) >> + port_index = 1; >> + else >> + sscanf(irq_name, "dp_hs_phy_%d", &port_index); >> + break; >> + > > No need for newlines after break. > >> + case DM_HS_PHY_IRQ_INDEX: >> + if (strcmp(irq_name, "dm_hs_phy_irq") == 0) >> + port_index = 1; >> + else >> + sscanf(irq_name, "dm_hs_phy_%d", &port_index); >> + break; >> + >> + case SS_PHY_IRQ_INDEX: >> + if (strcmp(irq_name, "ss_phy_irq") == 0) >> + port_index = 1; >> + else >> + sscanf(irq_name, "ss_phy_%d", &port_index); >> + break; >> + >> + case HS_PHY_IRQ_INDEX: >> + port_index = 1; >> + break; >> + } >> + >> + if (port_index > DWC3_MAX_PORTS) >> + port_index = -1; >> + >> + return port_index; >> +} > >> static int dwc3_qcom_setup_irq(struct platform_device *pdev) >> { >> struct dwc3_qcom *qcom = platform_get_drvdata(pdev); >> - const struct dwc3_acpi_pdata *pdata = qcom->acpi_pdata; >> + struct device_node *np = pdev->dev.of_node; >> + const char **irq_names; >> + int port_index; >> + int acpi_index; >> + int irq_count; >> + int irq_index; >> int irq; >> int ret; >> + int i; >> >> - irq = dwc3_qcom_get_irq(pdev, "hs_phy_irq", >> - pdata ? pdata->hs_phy_irq_index : -1); >> - if (irq > 0) { >> - ret = dwc3_qcom_prep_irq(qcom, "hs_phy_irq", "qcom_dwc3 HS", irq); >> - if (ret) >> - return ret; >> - qcom->hs_phy_irq = irq; >> - } >> + irq_count = of_property_count_strings(np, "interrupt-names"); > > of_property_count_strings() can return negative errnos and you don't > have any sanity checks for the return value... > > Please slow down, and also make sure to get your patches reviewed > internally before posting new revisions. > It did go through internal review but probably went un-noticed. Will add sanity checks here and below. >> + irq_names = devm_kzalloc(&pdev->dev, sizeof(*irq_names) * irq_count, GFP_KERNEL); > > devm_kcalloc() > >> + if (!irq_names) >> + return -ENOMEM; >> >> - irq = dwc3_qcom_get_irq(pdev, "dp_hs_phy_irq", >> - pdata ? pdata->dp_hs_phy_irq_index : -1); >> - if (irq > 0) { >> - ret = dwc3_qcom_prep_irq(qcom, "dp_hs_phy_irq", "qcom_dwc3 DP_HS", irq); >> - if (ret) >> - return ret; >> - qcom->dp_hs_phy_irq = irq; >> - } >> + ret = of_property_read_string_array(np, "interrupt-names", >> + irq_names, irq_count); > > No sanity check here either? > >> + for (i = 0; i < irq_count; i++) { >> + irq_index = dwc3_qcom_get_irq_index(irq_names[i]); >> + if (irq_index == -1) { >> + dev_dbg(&pdev->dev, "Invalid IRQ not handled"); >> + continue; >> + } > > I'll just stop reviewing here. This is a waste of my time. > Regards, Krishna,
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. > >> -static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, char *irq_name, > >> - char *disp_name, int irq) > >> +static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, const char *irq_name, > >> + const char *disp_name, int irq) > > > > Ok, here you did drop the second name parameter, but without renaming > > the first and hidden in a long diff without any mention anywhere. > > > I didn't understand the comment. Can you please elaborate. > I didn't drop the second parameter. In the usage of this call, I passed > same value to both irq_name and disp_name: > > "dwc3_qcom_prep_irq(qcom, irq_names[i], irq_names[i], irq);" > > I mentioned in cover-letter that I am using same name as obtained from > DT to register the interrupts as well. Should've mentioned that in > commit text of this patch. Will do it in next version. Ah, sorry I misread the diff. You never drop the second name even though it is now redundant as I pointed on in a comment to one of the earlier patches. Johan
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,
On Mon, Oct 23, 2023 at 04:54:11PM +0530, Krishna Kurapati PSSNV wrote: > 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: > >>> 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] This second core interrupt is also missing in the updated binding... It is defined in the ACPI tables so presumably it is needed for the multiport controller. Do you have any more details on this one? > 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] Ok, so at least we know hs_phy_irq and pwr_event_irq are distinct and both per-port. The ACPI tables do not seem to include these, but yeah, that doesn't say much more than that the Windows implementation doesn't currently use them either. > 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. Right. And I assume there are hs_phy_irqs also for the first two USB controllers on sc8280xp? Can you find out anything more about what hs_phy_irq is used for? It appears to be an HS wakeup interrupt like the dp/dm ones, but there are not really any details on how it is supposed to be used. Johan
On 10/23/2023 7:37 PM, Johan Hovold wrote: > On Mon, Oct 23, 2023 at 04:54:11PM +0530, Krishna Kurapati PSSNV wrote: >> 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: > >>>>> 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] > > This second core interrupt is also missing in the updated binding... It > is defined in the ACPI tables so presumably it is needed for the > multiport controller. > > Do you have any more details on this one? > >> 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] > > Ok, so at least we know hs_phy_irq and pwr_event_irq are distinct and > both per-port. > > The ACPI tables do not seem to include these, but yeah, that doesn't say > much more than that the Windows implementation doesn't currently use > them either. > >> 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. > > Right. And I assume there are hs_phy_irqs also for the first two USB > controllers on sc8280xp? Hi Johan, There are, I can dig through and find out. Atleast in downstream I don't see any use of them. > > Can you find out anything more about what hs_phy_irq is used for? It > appears to be an HS wakeup interrupt like the dp/dm ones, but there are > not really any details on how it is supposed to be used. > This IRQ is really not used in downstream controllers. Not sure if its a good idea to add driver code for that. I did some digging and I got the reason why I first said that there is only one hs_phy_irq for tertiary port of controller. The hardware programming sequence doesn't specify usage of these 4 IRQ's but the hw specifics mention that there are 4 of them. Adding driver support for these IRQ's is not a good idea (atleast at this point because they are not used in downstream and I am not sure what would be the side effect). For now I suggest we can add them in bindings and DT and not handle the 4 hs_phy_irq's in the driver code (meaning not add the hs_phy_irq to port structure we plan on adding to dwc3_qcom). Also I plan on splitting the patchset into 4 parts (essentially 4 diff series): 1. Bindings update for hs_phy_irq's 2. DT patches for MP controller and platform specific files 3. Core driver update for supporting multiport 4. QCOM driver update for supporting wakeup/suspend/resume This is in accordance to [1] and that way qcom code won't block core driver changes from getting merged. Core driver changes are independent and are sufficient to get multiport working. [1]: https://lore.kernel.org/all/d4663197-8295-4967-a4f5-6cc91638fc0d@linaro.org/ Regards, Krishna,
On Mon, Oct 23, 2023 at 10:42:31PM +0530, Krishna Kurapati PSSNV wrote: > On 10/23/2023 7:37 PM, Johan Hovold wrote: > > Right. And I assume there are hs_phy_irqs also for the first two USB > > controllers on sc8280xp? > There are, I can dig through and find out. Atleast in downstream I don't > see any use of them. Yes, please do post how these are wired as well for completeness. > > Can you find out anything more about what hs_phy_irq is used for? It > > appears to be an HS wakeup interrupt like the dp/dm ones, but there are > > not really any details on how it is supposed to be used. > > This IRQ is really not used in downstream controllers. Not sure if its > a good idea to add driver code for that. I did some digging and I got > the reason why I first said that there is only one hs_phy_irq for > tertiary port of controller. The hardware programming sequence doesn't > specify usage of these 4 IRQ's but the hw specifics mention that there > are 4 of them. Adding driver support for these IRQ's is not a good idea > (atleast at this point because they are not used in downstream and I am > not sure what would be the side effect). For now I suggest we can add > them in bindings and DT and not handle the 4 hs_phy_irq's in the driver > code (meaning not add the hs_phy_irq to port structure we plan on adding > to dwc3_qcom). But there is already support for these interrupts in the driver. You work for Qualcomm who built the thing so surely you can figure how they intended these to be used? You need to provide this information so that we can determine what the binding should look like. The implementation would also be simplified if we don't have to add random hacks to it just because we don't know why the vendor driver you refer does not use it currently on this particular platform. > Also I plan on splitting the patchset into 4 parts (essentially 4 diff > series): > > 1. Bindings update for hs_phy_irq's > 2. DT patches for MP controller and platform specific files > 3. Core driver update for supporting multiport > 4. QCOM driver update for supporting wakeup/suspend/resume > > This is in accordance to [1] and that way qcom code won't block core > driver changes from getting merged. Core driver changes are independent > and are sufficient to get multiport working. No, you clearly did not understand [1] at all. And stop trying to game the upstreaming process. Bindings and driver patches go together. The devicetree changes can be sent separately in case of USB but only *after* the first set has been merged. If the code had been in good shape from the start it would have been merged by now. Just learn from your mistakes and next time things will be smoother. > [1]: > https://lore.kernel.org/all/d4663197-8295-4967-a4f5-6cc91638fc0d@linaro.org/ Johan
On 10/24/2023 12:26 PM, Johan Hovold wrote: > > You need to provide this information so that we can determine what the > binding should look like. The implementation would also be simplified if > we don't have to add random hacks to it just because we don't know why > the vendor driver you refer does not use it currently on this particular > platform. > >> Also I plan on splitting the patchset into 4 parts (essentially 4 diff >> series): >> >> 1. Bindings update for hs_phy_irq's >> 2. DT patches for MP controller and platform specific files >> 3. Core driver update for supporting multiport >> 4. QCOM driver update for supporting wakeup/suspend/resume >> >> This is in accordance to [1] and that way qcom code won't block core >> driver changes from getting merged. Core driver changes are independent >> and are sufficient to get multiport working. > > No, you clearly did not understand [1] at all. And stop trying to game > the upstreaming process. Bindings and driver patches go together. The > devicetree changes can be sent separately in case of USB but only > *after* the first set has been merged. > > If the code had been in good shape from the start it would have been > merged by now. Just learn from your mistakes and next time things will > be smoother. > I agree that bindings should go first. My point is core bindings are already approved and merged and just wanted to check if core driver changes can be merged without glue blocking them. Core driver changes have nothing to do with interrupt handling in glue. If we get the core changes merged separately after fixing the nits mentioned, we can take up this interrupt handling in glue in parallel. I am just trying to see if we can start merging independent portions of code. I agree that my glue driver changes are still not upto mark. But that has nothing to do with core driver changes. Please let me know if that is appropriate because I think functionality intended by changes in core is independent of glue driver and glue bindings. If anything glue is partially dependent on core changes (like the MAX_PORTS macro etc., but not the other way around). Regards, Krishna,
On Tue, Oct 24, 2023 at 02:23:57PM +0530, Krishna Kurapati PSSNV wrote: > On 10/24/2023 12:26 PM, Johan Hovold wrote: > > No, you clearly did not understand [1] at all. And stop trying to game > > the upstreaming process. Bindings and driver patches go together. The > > devicetree changes can be sent separately in case of USB but only > > *after* the first set has been merged. > > > > If the code had been in good shape from the start it would have been > > merged by now. Just learn from your mistakes and next time things will > > be smoother. > > I agree that bindings should go first. My point is core bindings are > already approved and merged and just wanted to check if core driver > changes can be merged without glue blocking them. Core driver changes > have nothing to do with interrupt handling in glue. If we get the core > changes merged separately after fixing the nits mentioned, we can take > up this interrupt handling in glue in parallel. I am just trying to see > if we can start merging independent portions of code. I agree that my > glue driver changes are still not upto mark. But that has nothing to do > with core driver changes. Again, no. The dwc3 glue and core bits are not independent, and ideally the bindings should not have been merged either before having the implementation in a decent shape either (e.g. as the messy implementation suggested that the bindings were incomplete). You're again trying to sneak in an incomplete implementation. Qualcomm has a terrible track record of doing just that and leaving others with the task to clean up their mess. This should go in as one series, when it's ready, and not before. And we may even consider reverting the updated bindings as it appears they are still not correct. Johan
On Tue, Oct 24, 2023 at 11:18:26AM +0200, Johan Hovold wrote: > On Tue, Oct 24, 2023 at 02:23:57PM +0530, Krishna Kurapati PSSNV wrote: > > On 10/24/2023 12:26 PM, Johan Hovold wrote: > > > > No, you clearly did not understand [1] at all. And stop trying to game > > > the upstreaming process. Bindings and driver patches go together. The > > > devicetree changes can be sent separately in case of USB but only > > > *after* the first set has been merged. > > > > > > If the code had been in good shape from the start it would have been > > > merged by now. Just learn from your mistakes and next time things will > > > be smoother. > > > > I agree that bindings should go first. My point is core bindings are > > already approved and merged and just wanted to check if core driver > > changes can be merged without glue blocking them. Core driver changes > > have nothing to do with interrupt handling in glue. If we get the core > > changes merged separately after fixing the nits mentioned, we can take > > up this interrupt handling in glue in parallel. I am just trying to see > > if we can start merging independent portions of code. I agree that my > > glue driver changes are still not upto mark. But that has nothing to do > > with core driver changes. > > Again, no. The dwc3 glue and core bits are not independent, and ideally > the bindings should not have been merged either before having the > implementation in a decent shape either (e.g. as the messy > implementation suggested that the bindings were incomplete). > > You're again trying to sneak in an incomplete implementation. Qualcomm > has a terrible track record of doing just that and leaving others with > the task to clean up their mess. > > This should go in as one series, when it's ready, and not before. > > And we may even consider reverting the updated bindings as it appears > they are still not correct. If you can tell me what the git ids of them are, I'll be glad to do so right now, sorry for taking them "early". thanks, greg k-h
On Tue, Oct 24, 2023 at 11:23:19AM +0200, Greg Kroah-Hartman wrote: > On Tue, Oct 24, 2023 at 11:18:26AM +0200, Johan Hovold wrote: > > And we may even consider reverting the updated bindings as it appears > > they are still not correct. > > If you can tell me what the git ids of them are, I'll be glad to do so > right now, sorry for taking them "early". That's ca58c4ae75b6 ("dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport") and eb3f1d9e42b1 ("dt-bindings: usb: Add bindings for multiport properties on DWC3 controller") It's probably best to just revert them now. Thanks. Johan
On Tue, Oct 24, 2023 at 11:29:17AM +0200, Johan Hovold wrote: > On Tue, Oct 24, 2023 at 11:23:19AM +0200, Greg Kroah-Hartman wrote: > > On Tue, Oct 24, 2023 at 11:18:26AM +0200, Johan Hovold wrote: > > > > And we may even consider reverting the updated bindings as it appears > > > they are still not correct. > > > > If you can tell me what the git ids of them are, I'll be glad to do so > > right now, sorry for taking them "early". > > That's > > ca58c4ae75b6 ("dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport") > > and > > eb3f1d9e42b1 ("dt-bindings: usb: Add bindings for multiport properties on DWC3 controller") > > It's probably best to just revert them now. Now reverted, thanks. greg k-h
On 10/24/2023 12:26 PM, Johan Hovold wrote: > On Mon, Oct 23, 2023 at 10:42:31PM +0530, Krishna Kurapati PSSNV wrote: >> On 10/23/2023 7:37 PM, Johan Hovold wrote: > >>> Right. And I assume there are hs_phy_irqs also for the first two USB >>> controllers on sc8280xp? > >> There are, I can dig through and find out. Atleast in downstream I don't >> see any use of them. > > Yes, please do post how these are wired as well for completeness. > >>> Can you find out anything more about what hs_phy_irq is used for? It >>> appears to be an HS wakeup interrupt like the dp/dm ones, but there are >>> not really any details on how it is supposed to be used. >> >> This IRQ is really not used in downstream controllers. Not sure if its >> a good idea to add driver code for that. I did some digging and I got >> the reason why I first said that there is only one hs_phy_irq for >> tertiary port of controller. The hardware programming sequence doesn't >> specify usage of these 4 IRQ's but the hw specifics mention that there >> are 4 of them. Adding driver support for these IRQ's is not a good idea >> (atleast at this point because they are not used in downstream and I am >> not sure what would be the side effect). For now I suggest we can add >> them in bindings and DT and not handle the 4 hs_phy_irq's in the driver >> code (meaning not add the hs_phy_irq to port structure we plan on adding >> to dwc3_qcom). > > But there is already support for these interrupts in the driver. You > work for Qualcomm who built the thing so surely you can figure how they > intended these to be used? > > You need to provide this information so that we can determine what the > binding should look like. The implementation would also be simplified if > we don't have to add random hacks to it just because we don't know why > the vendor driver you refer does not use it currently on this particular > platform. > Hi Johan, Regarding the points of discussion we had last week on [1], here are some clarifications: 1. We do have hs_phy_irq 1/2/3/4 for tertiary port of Sc8280 as mentioned. Why do we need them and would we use it in multiport targets ? DPSE and DMSE are single ended line state of DP and DM lines. The DP line and DM line stay in steady High or Low during suspend and they flip when there is a RESUME or REMOTE WAKE. This is what we do/check in dwc3_qcom_enable_interrupts call for dp/dm irq's based on usb2_speed. Initially in QUSB2 targets, the interrupts were enabled and configured in phy and the wakeup was interrupt was read on hs_phy_irq vector - [2]. In that case, we modify DP/DM interrupts in phy registers, specifically QUSB2PHY_INTR_CTRL and when wakeup signal comes in, hs_phy_irq is triggered. But in femto targets, this is done via DP/DM interrupts and there is no use of hs_phy_irq. Even hw folks confirmed they dont use hs_ph_irq in femto phy targets. As an experiment, I tried to test wakeup by pressing buttons on connected keyboard when in suspend state or connecting/disconnecting keyboard in suspended state on different ports and only see dp/dm IRQ's getting fired although we register for hs_phy_irq as well: / # cat /proc/interrupts |grep phy_ 171: 1 0 0 0 0 0 0 0 PDC 127 Edge dp_hs_phy_1 172: 2 0 0 0 0 0 0 0 PDC 126 Edge dm_hs_phy_1 173: 3 0 0 0 0 0 0 0 PDC 129 Edge dp_hs_phy_2 174: 4 0 0 0 0 0 0 0 PDC 128 Edge dm_hs_phy_2 175: 0 0 0 0 0 0 0 0 PDC 131 Edge dp_hs_phy_3 176: 2 0 0 0 0 0 0 0 PDC 130 Edge dm_hs_phy_3 177: 2 0 0 0 0 0 0 0 PDC 133 Edge dp_hs_phy_4 178: 5 0 0 0 0 0 0 0 PDC 132 Edge dm_hs_phy_4 179: 0 0 0 0 0 0 0 0 PDC 16 Level ss_phy_1 180: 0 0 0 0 0 0 0 0 PDC 17 Level ss_phy_2 181: 0 0 0 0 0 0 0 0 GICv3 163 Level hs_phy_1 182: 0 0 0 0 0 0 0 0 GICv3 168 Level hs_phy_2 183: 0 0 0 0 0 0 0 0 GICv3 892 Level hs_phy_3 184: 0 0 0 0 0 0 0 0 GICv3 891 Level hs_phy_4 Since the hs_phy_irq is applicable only for qusb2 targets, do we still need to add it to DT. 2. BAM Irq usage (u_usb31_scnd_mvs_pipe_wrapper_usb31_bam_irq[0]): BAM IRQ is not needed in host-only controller. It was just added in process of porting/deriving code from DRD controllers and is non-functional (confirmed by HW team here). We can skip this from DT of multiport. 3. ctrl_irq[1] usage: This is a feature of SNPS controller, not qcom glue wrapper, and is present on all targets (non-QC as well probably). As mentioned before on [3], this is used for HW acceleration. In host mode, XHCI spec does allow for multiple interrupters when multiple event rings are used. A possible usage is multiple execution environments something like what we are doing on mobile with ADSP audio offload [4]. Another possibility could be some of virtualization where host/hyp would manage the first interrupter and could allow a guest to operate only with the second (though current design does not go far enough to offer true isolation for real VM type workloads). The additional interrupts (ones other than ctrl_irq[0]) are either for virtualization use cases, or for our various “hw offload” features. In device mode, these are used for offloading tethering functionality to IPA FW. Since the DeviceTree passed to the OS, should describe the hardware to the OS, and should represent the hardware from the point-of-view of the OS, adding one interrupt (ctrl_irq[0]) might be sufficient as Linux would not use the other interrupts. Furthermore AFAIK even UEFI/Windows also use only ctrl_irq[0] for host mode in their execution environment today. Do we still need to add this to bindings and DT ? [1]: https://lore.kernel.org/all/ZTJ_T1UL8-s2cgNz@hovoldconsulting.com/ [2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/qualcomm/phy-qcom-qusb2.c?h=v6.6#n626 [3]: https://lore.kernel.org/all/ZTduh5LULBMYf3wq@hovoldconsulting.com/ [4]: https://lore.kernel.org/all/20231017200109.11407-1-quic_wcheng@quicinc.com/ Regards, Krishna,
>> >> But there is already support for these interrupts in the driver. You >> work for Qualcomm who built the thing so surely you can figure how they >> intended these to be used? >> >> You need to provide this information so that we can determine what the >> binding should look like. The implementation would also be simplified if >> we don't have to add random hacks to it just because we don't know why >> the vendor driver you refer does not use it currently on this particular >> platform. >> > > Hi Johan, > > Regarding the points of discussion we had last week on [1], here are > some clarifications: > > 1. We do have hs_phy_irq 1/2/3/4 for tertiary port of Sc8280 as > mentioned. Why do we need them and would we use it in multiport targets ? > > DPSE and DMSE are single ended line state of DP and DM lines. The DP > line and DM line stay in steady High or Low during suspend and they flip > when there is a RESUME or REMOTE WAKE. This is what we do/check in > dwc3_qcom_enable_interrupts call for dp/dm irq's based on usb2_speed. > > Initially in QUSB2 targets, the interrupts were enabled and configured > in phy and the wakeup was interrupt was read on hs_phy_irq vector - [2]. > In that case, we modify DP/DM interrupts in phy registers, specifically > QUSB2PHY_INTR_CTRL and when wakeup signal comes in, hs_phy_irq is > triggered. But in femto targets, this is done via DP/DM interrupts and > there is no use of hs_phy_irq. Even hw folks confirmed they dont use > hs_ph_irq in femto phy targets. > > As an experiment, I tried to test wakeup by pressing buttons on > connected keyboard when in suspend state or connecting/disconnecting > keyboard in suspended state on different ports and only see dp/dm IRQ's > getting fired although we register for hs_phy_irq as well: > > / # cat /proc/interrupts |grep phy_ > 171: 1 0 0 0 0 0 0 0 PDC 127 Edge dp_hs_phy_1 > 172: 2 0 0 0 0 0 0 0 PDC 126 Edge dm_hs_phy_1 > 173: 3 0 0 0 0 0 0 0 PDC 129 Edge dp_hs_phy_2 > 174: 4 0 0 0 0 0 0 0 PDC 128 Edge dm_hs_phy_2 > 175: 0 0 0 0 0 0 0 0 PDC 131 Edge dp_hs_phy_3 > 176: 2 0 0 0 0 0 0 0 PDC 130 Edge dm_hs_phy_3 > 177: 2 0 0 0 0 0 0 0 PDC 133 Edge dp_hs_phy_4 > 178: 5 0 0 0 0 0 0 0 PDC 132 Edge dm_hs_phy_4 > 179: 0 0 0 0 0 0 0 0 PDC 16 Level ss_phy_1 > 180: 0 0 0 0 0 0 0 0 PDC 17 Level ss_phy_2 > 181: 0 0 0 0 0 0 0 0 GICv3 163 Level hs_phy_1 > 182: 0 0 0 0 0 0 0 0 GICv3 168 Level hs_phy_2 > 183: 0 0 0 0 0 0 0 0 GICv3 892 Level hs_phy_3 > 184: 0 0 0 0 0 0 0 0 GICv3 891 Level hs_phy_4 > > Since the hs_phy_irq is applicable only for qusb2 targets, do we still > need to add it to DT. > > 2. BAM Irq usage (u_usb31_scnd_mvs_pipe_wrapper_usb31_bam_irq[0]): > > BAM IRQ is not needed in host-only controller. It was just added in > process of porting/deriving code from DRD controllers and is > non-functional (confirmed by HW team here). We can skip this from DT of > multiport. > > 3. ctrl_irq[1] usage: > > This is a feature of SNPS controller, not qcom glue wrapper, and is > present on all targets (non-QC as well probably). As mentioned before on > [3], this is used for HW acceleration. > > In host mode, XHCI spec does allow for multiple interrupters when > multiple event rings are used. A possible usage is multiple execution > environments something like what we are doing on mobile with ADSP audio > offload [4]. Another possibility could be some of virtualization where > host/hyp would manage the first interrupter and could allow a guest to > operate only with the second (though current design does not go far > enough to offer true isolation for real VM type workloads). The > additional interrupts (ones other than ctrl_irq[0]) are either for > virtualization use cases, or for our various “hw offload” features. In > device mode, these are used for offloading tethering functionality to > IPA FW. > > Since the DeviceTree passed to the OS, should describe the hardware to > the OS, and should represent the hardware from the point-of-view of the > OS, adding one interrupt (ctrl_irq[0]) might be sufficient as Linux > would not use the other interrupts. Furthermore AFAIK even UEFI/Windows > also use only ctrl_irq[0] for host mode in their execution environment > today. Do we still need to add this to bindings and DT ? > > [1]: https://lore.kernel.org/all/ZTJ_T1UL8-s2cgNz@hovoldconsulting.com/ > [2]: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/qualcomm/phy-qcom-qusb2.c?h=v6.6#n626 > [3]: https://lore.kernel.org/all/ZTduh5LULBMYf3wq@hovoldconsulting.com/ > [4]: > https://lore.kernel.org/all/20231017200109.11407-1-quic_wcheng@quicinc.com/ > Hi Johan, Can you help provide your comments on the above mentioned points so that we can take the discussion forward and finalize changes for v14 patches. And thanks for all the reviews so far on previous revisions. Regards, Krishna,
On Fri, Nov 03, 2023 at 03:34:52PM +0530, Krishna Kurapati PSSNV wrote: > On 10/24/2023 12:26 PM, Johan Hovold wrote: > > On Mon, Oct 23, 2023 at 10:42:31PM +0530, Krishna Kurapati PSSNV wrote: > >> On 10/23/2023 7:37 PM, Johan Hovold wrote: > > > >>> Right. And I assume there are hs_phy_irqs also for the first two USB > >>> controllers on sc8280xp? > > > >> There are, I can dig through and find out. Atleast in downstream I don't > >> see any use of them. > > > > Yes, please do post how these are wired as well for completeness. Did you find these two interrupts as well? > >>> Can you find out anything more about what hs_phy_irq is used for? It > >>> appears to be an HS wakeup interrupt like the dp/dm ones, but there are > >>> not really any details on how it is supposed to be used. > >> > >> This IRQ is really not used in downstream controllers. Not sure if its > >> a good idea to add driver code for that. I did some digging and I got > >> the reason why I first said that there is only one hs_phy_irq for > >> tertiary port of controller. The hardware programming sequence doesn't > >> specify usage of these 4 IRQ's but the hw specifics mention that there > >> are 4 of them. Adding driver support for these IRQ's is not a good idea > >> (atleast at this point because they are not used in downstream and I am > >> not sure what would be the side effect). For now I suggest we can add > >> them in bindings and DT and not handle the 4 hs_phy_irq's in the driver > >> code (meaning not add the hs_phy_irq to port structure we plan on adding > >> to dwc3_qcom). > > > > But there is already support for these interrupts in the driver. You > > work for Qualcomm who built the thing so surely you can figure how they > > intended these to be used? > > > > You need to provide this information so that we can determine what the > > binding should look like. The implementation would also be simplified if > > we don't have to add random hacks to it just because we don't know why > > the vendor driver you refer does not use it currently on this particular > > platform. > Regarding the points of discussion we had last week on [1], here are > some clarifications: > > 1. We do have hs_phy_irq 1/2/3/4 for tertiary port of Sc8280 as > mentioned. Why do we need them and would we use it in multiport targets ? > > DPSE and DMSE are single ended line state of DP and DM lines. The DP > line and DM line stay in steady High or Low during suspend and they flip > when there is a RESUME or REMOTE WAKE. This is what we do/check in > dwc3_qcom_enable_interrupts call for dp/dm irq's based on usb2_speed. Right, this bit is clear. > Initially in QUSB2 targets, the interrupts were enabled and configured > in phy and the wakeup was interrupt was read on hs_phy_irq vector - [2]. > In that case, we modify DP/DM interrupts in phy registers, specifically > QUSB2PHY_INTR_CTRL and when wakeup signal comes in, hs_phy_irq is > triggered. But in femto targets, this is done via DP/DM interrupts and > there is no use of hs_phy_irq. Even hw folks confirmed they dont use > hs_ph_irq in femto phy targets. Ok, thanks for pointing to QUSB2. The same mechanism is apparently used in phy-qcom-usb-hs-28nm.c as well (even if the dtsi currently does not define the wakeup interrupts). Furthermore, that implementation is broken and has never worked due to another half-arsed, incomplete Qualcomm implementation. Specifically, no one is changing the PHY mode based on the current speed before suspend as commits like 3b3cd24ae61b ("phy: Add USB speed related PHY modes") and 891a96f65ac3 ("phy: qcom-qusb2: Add support for runtime PM") depend on. Guess I should go revert that mess too... > As an experiment, I tried to test wakeup by pressing buttons on > connected keyboard when in suspend state or connecting/disconnecting > keyboard in suspended state on different ports and only see dp/dm IRQ's > getting fired although we register for hs_phy_irq as well: > > / # cat /proc/interrupts |grep phy_ > 171: 1 0 0 0 0 0 0 0 PDC 127 Edge dp_hs_phy_1 > 172: 2 0 0 0 0 0 0 0 PDC 126 Edge dm_hs_phy_1 > 173: 3 0 0 0 0 0 0 0 PDC 129 Edge dp_hs_phy_2 > 174: 4 0 0 0 0 0 0 0 PDC 128 Edge dm_hs_phy_2 > 175: 0 0 0 0 0 0 0 0 PDC 131 Edge dp_hs_phy_3 > 176: 2 0 0 0 0 0 0 0 PDC 130 Edge dm_hs_phy_3 > 177: 2 0 0 0 0 0 0 0 PDC 133 Edge dp_hs_phy_4 > 178: 5 0 0 0 0 0 0 0 PDC 132 Edge dm_hs_phy_4 > 179: 0 0 0 0 0 0 0 0 PDC 16 Level ss_phy_1 > 180: 0 0 0 0 0 0 0 0 PDC 17 Level ss_phy_2 > 181: 0 0 0 0 0 0 0 0 GICv3 163 Level hs_phy_1 > 182: 0 0 0 0 0 0 0 0 GICv3 168 Level hs_phy_2 > 183: 0 0 0 0 0 0 0 0 GICv3 892 Level hs_phy_3 > 184: 0 0 0 0 0 0 0 0 GICv3 891 Level hs_phy_4 Yes, but that doesn't really say much since you never enable the hs_phy interrupt in the PHY on suspend. > Since the hs_phy_irq is applicable only for qusb2 targets, do we still > need to add it to DT. Are you sure there's no support for hs_phy_irq also in the "femto" PHYs and that it's just that there is currently no driver support for using them? And why is it defined if there is truly no use for it? Also, if hs_phy_irq and dp/dm_phy_irq were mutually exclusive, why does the following Qualcomm SoCs define all three? - qcom,ipq4019-dwc3 - qcom,ipq6018-dwc3 - qcom,ipq8064-dwc3 - qcom,ipq8074-dwc3 - qcom,msm8994-dwc3 - qcom,qcs404-dwc3 - qcom,sc7180-dwc3 - qcom,sc7280-dwc3 - qcom,sdm670-dwc3 - qcom,sdm845-dwc3 - qcom,sdx55-dwc3 - qcom,sdx65-dwc3 - qcom,sm4250-dwc3 - qcom,sm6125-dwc3 - qcom,sm6350-dwc3 - qcom,sm8150-dwc3 - qcom,sm8250-dwc3 - qcom,sm8350-dwc3 - qcom,sm8450-dwc3 - qcom,sm8550-dwc3 Some of those use QUSB2 PHYs and some use "femto" PHYs. And this comes from Qualcomm through commits like: 0b766e7fe5a2 ("arm64: dts: qcom: sc7180: Add USB related nodes") bb9efa59c665 ("arm64: dts: qcom: sc7280: Add USB related nodes") > 2. BAM Irq usage (u_usb31_scnd_mvs_pipe_wrapper_usb31_bam_irq[0]): > > BAM IRQ is not needed in host-only controller. It was just added in > process of porting/deriving code from DRD controllers and is > non-functional (confirmed by HW team here). We can skip this from DT of > multiport. Ok, good. > 3. ctrl_irq[1] usage: > > This is a feature of SNPS controller, not qcom glue wrapper, and is > present on all targets (non-QC as well probably). As mentioned before on > [3], this is used for HW acceleration. > > In host mode, XHCI spec does allow for multiple interrupters when > multiple event rings are used. A possible usage is multiple execution > environments something like what we are doing on mobile with ADSP audio > offload [4]. Another possibility could be some of virtualization where > host/hyp would manage the first interrupter and could allow a guest to > operate only with the second (though current design does not go far > enough to offer true isolation for real VM type workloads). The > additional interrupts (ones other than ctrl_irq[0]) are either for > virtualization use cases, or for our various “hw offload” features. In > device mode, these are used for offloading tethering functionality to > IPA FW. Ok, thanks for clarifying what you meant by "HW acceleration". > Since the DeviceTree passed to the OS, should describe the hardware to > the OS, and should represent the hardware from the point-of-view of the > OS, adding one interrupt (ctrl_irq[0]) might be sufficient as Linux > would not use the other interrupts. I've only skimmed the virtualisation bits in xHCI spec, but it seems Linux as VMM would still be involved in assigning these interrupts to VMs. This may possibly be something that we can ignore for now, but perhaps someone more familiar with the hardware, like Thinh, can chime in. > Furthermore AFAIK even UEFI/Windows > also use only ctrl_irq[0] for host mode in their execution environment > today. Do we still need to add this to bindings and DT ? But the second interrupt is described in the ACPI tables, which means that a simple driver update could (in theory) allow for it to be used. You need to get into the same mindset when it comes to devicetree. Even if Linux currently does not use an interrupt, like the pwr_event_irq, you should still add it so that when/if someone implements support for it, an older platform using the original dt may also take advantage of it. Sure, there are complications and we sometimes break DT backwards compatibility, but this is the goal that you should strive for. Johan
On 11/9/2023 8:48 PM, Johan Hovold wrote: > On Fri, Nov 03, 2023 at 03:34:52PM +0530, Krishna Kurapati PSSNV wrote: >> On 10/24/2023 12:26 PM, Johan Hovold wrote: >>> On Mon, Oct 23, 2023 at 10:42:31PM +0530, Krishna Kurapati PSSNV wrote: >>>> On 10/23/2023 7:37 PM, Johan Hovold wrote: >>> >>>>> Right. And I assume there are hs_phy_irqs also for the first two USB >>>>> controllers on sc8280xp? >>> >>>> There are, I can dig through and find out. Atleast in downstream I don't >>>> see any use of them. >>> >>> Yes, please do post how these are wired as well for completeness. > > Did you find these two interrupts as well? > > >> Regarding the points of discussion we had last week on [1], here are >> some clarifications: >> >> 1. We do have hs_phy_irq 1/2/3/4 for tertiary port of Sc8280 as >> mentioned. Why do we need them and would we use it in multiport targets ? >> >> DPSE and DMSE are single ended line state of DP and DM lines. The DP >> line and DM line stay in steady High or Low during suspend and they flip >> when there is a RESUME or REMOTE WAKE. This is what we do/check in >> dwc3_qcom_enable_interrupts call for dp/dm irq's based on usb2_speed. > > Right, this bit is clear. > >> Initially in QUSB2 targets, the interrupts were enabled and configured >> in phy and the wakeup was interrupt was read on hs_phy_irq vector - [2]. >> In that case, we modify DP/DM interrupts in phy registers, specifically >> QUSB2PHY_INTR_CTRL and when wakeup signal comes in, hs_phy_irq is >> triggered. But in femto targets, this is done via DP/DM interrupts and >> there is no use of hs_phy_irq. Even hw folks confirmed they dont use >> hs_ph_irq in femto phy targets. > > Ok, thanks for pointing to QUSB2. The same mechanism is apparently used > in phy-qcom-usb-hs-28nm.c as well (even if the dtsi currently does not > define the wakeup interrupts). > > Furthermore, that implementation is broken and has never worked due to > another half-arsed, incomplete Qualcomm implementation. Specifically, no > one is changing the PHY mode based on the current speed before suspend > as commits like > > 3b3cd24ae61b ("phy: Add USB speed related PHY modes") > > and > > 891a96f65ac3 ("phy: qcom-qusb2: Add support for runtime PM") > > depend on. Guess I should go revert that mess too... > >> As an experiment, I tried to test wakeup by pressing buttons on >> connected keyboard when in suspend state or connecting/disconnecting >> keyboard in suspended state on different ports and only see dp/dm IRQ's >> getting fired although we register for hs_phy_irq as well: >> >> / # cat /proc/interrupts |grep phy_ >> 171: 1 0 0 0 0 0 0 0 PDC 127 Edge dp_hs_phy_1 >> 172: 2 0 0 0 0 0 0 0 PDC 126 Edge dm_hs_phy_1 >> 173: 3 0 0 0 0 0 0 0 PDC 129 Edge dp_hs_phy_2 >> 174: 4 0 0 0 0 0 0 0 PDC 128 Edge dm_hs_phy_2 >> 175: 0 0 0 0 0 0 0 0 PDC 131 Edge dp_hs_phy_3 >> 176: 2 0 0 0 0 0 0 0 PDC 130 Edge dm_hs_phy_3 >> 177: 2 0 0 0 0 0 0 0 PDC 133 Edge dp_hs_phy_4 >> 178: 5 0 0 0 0 0 0 0 PDC 132 Edge dm_hs_phy_4 >> 179: 0 0 0 0 0 0 0 0 PDC 16 Level ss_phy_1 >> 180: 0 0 0 0 0 0 0 0 PDC 17 Level ss_phy_2 >> 181: 0 0 0 0 0 0 0 0 GICv3 163 Level hs_phy_1 >> 182: 0 0 0 0 0 0 0 0 GICv3 168 Level hs_phy_2 >> 183: 0 0 0 0 0 0 0 0 GICv3 892 Level hs_phy_3 >> 184: 0 0 0 0 0 0 0 0 GICv3 891 Level hs_phy_4 > > Yes, but that doesn't really say much since you never enable the hs_phy > interrupt in the PHY on suspend. I did register to and enabled the hs_phy_irq interrupt when I tested and posted the above table. > >> Since the hs_phy_irq is applicable only for qusb2 targets, do we still >> need to add it to DT. > > Are you sure there's no support for hs_phy_irq also in the "femto" PHYs > and that it's just that there is currently no driver support for using > them? > > And why is it defined if there is truly no use for it? Femto phy's have nothing to be configured for interrupts like we do for qusb2 phy's. I confirmed from hw validation team that they never used hs_phy_irq for validating wakeup. They only used dp/dm IRQ's for wakeup. > Also, if hs_phy_irq and dp/dm_phy_irq were mutually exclusive, why does > the following Qualcomm SoCs define all three? > Similar to BAM IRQ's these might have been just ported over targets I believe. I say so because HW Validation team confirmed they don't use this interrupt at all on femto phy targets. > - qcom,ipq4019-dwc3 > - qcom,ipq6018-dwc3 > - qcom,ipq8064-dwc3 > - qcom,ipq8074-dwc3 > - qcom,msm8994-dwc3 > - qcom,qcs404-dwc3 > - qcom,sc7180-dwc3 > - qcom,sc7280-dwc3 > - qcom,sdm670-dwc3 > - qcom,sdm845-dwc3 > - qcom,sdx55-dwc3 > - qcom,sdx65-dwc3 > - qcom,sm4250-dwc3 > - qcom,sm6125-dwc3 > - qcom,sm6350-dwc3 > - qcom,sm8150-dwc3 > - qcom,sm8250-dwc3 > - qcom,sm8350-dwc3 > - qcom,sm8450-dwc3 > - qcom,sm8550-dwc3 > > Some of those use QUSB2 PHYs and some use "femto" PHYs. > > And this comes from Qualcomm through commits like: > > 0b766e7fe5a2 ("arm64: dts: qcom: sc7180: Add USB related nodes") > bb9efa59c665 ("arm64: dts: qcom: sc7280: Add USB related nodes") > > >> 3. ctrl_irq[1] usage: >> >> This is a feature of SNPS controller, not qcom glue wrapper, and is >> present on all targets (non-QC as well probably). As mentioned before on >> [3], this is used for HW acceleration. >> >> In host mode, XHCI spec does allow for multiple interrupters when >> multiple event rings are used. A possible usage is multiple execution >> environments something like what we are doing on mobile with ADSP audio >> offload [4]. Another possibility could be some of virtualization where >> host/hyp would manage the first interrupter and could allow a guest to >> operate only with the second (though current design does not go far >> enough to offer true isolation for real VM type workloads). The >> additional interrupts (ones other than ctrl_irq[0]) are either for >> virtualization use cases, or for our various “hw offload” features. In >> device mode, these are used for offloading tethering functionality to >> IPA FW. > > Ok, thanks for clarifying what you meant by "HW acceleration". > >> Since the DeviceTree passed to the OS, should describe the hardware to >> the OS, and should represent the hardware from the point-of-view of the >> OS, adding one interrupt (ctrl_irq[0]) might be sufficient as Linux >> would not use the other interrupts. > > I've only skimmed the virtualisation bits in xHCI spec, but it seems > Linux as VMM would still be involved in assigning these interrupts to > VMs. I didn't understand this sentence. Are you referring to cases where Linux needs to act as the entity using the ctrl_irq[1] ? On QCOM SoC's, in reality (atleast in device mode) I can say that we create the event rings for IPA FW (which registers for ctrl_irq[1]) to use and read depevt's. We don't register or get this IRQ from DT and then provide to IPA (not even in downstream). > > This may possibly be something that we can ignore for now, but perhaps > someone more familiar with the hardware, like Thinh, can chime in. > >> Furthermore AFAIK even UEFI/Windows >> also use only ctrl_irq[0] for host mode in their execution environment >> today. Do we still need to add this to bindings and DT ? > > But the second interrupt is described in the ACPI tables, which means > that a simple driver update could (in theory) allow for it to be used. > > You need to get into the same mindset when it comes to devicetree. Even > if Linux currently does not use an interrupt, like the pwr_event_irq, > you should still add it so that when/if someone implements support for > it, an older platform using the original dt may also take advantage of > it. > > Sure, there are complications and we sometimes break DT backwards > compatibility, but this is the goal that you should strive for. > I agree with you. That we need to make the devicetree as flexible and backward compatible as possible. I will let Thinh comment on whether this IRQ needs to be added to DT or not. If someone really wants to add driver support, bindings needs to be clear for the driver design in this case (Linux as VMM like you mentioned) and I have not idea on the same. Regards, Krishna,
On 11/9/2023 8:38 AM, Krishna Kurapati PSSNV wrote: > > > On 11/9/2023 8:48 PM, Johan Hovold wrote: >> On Fri, Nov 03, 2023 at 03:34:52PM +0530, Krishna Kurapati PSSNV wrote: >>> On 10/24/2023 12:26 PM, Johan Hovold wrote: >>>> On Mon, Oct 23, 2023 at 10:42:31PM +0530, Krishna Kurapati PSSNV wrote: >>>>> On 10/23/2023 7:37 PM, Johan Hovold wrote: >>>> >>>>>> Right. And I assume there are hs_phy_irqs also for the first two USB >>>>>> controllers on sc8280xp? >>>> >>>>> There are, I can dig through and find out. Atleast in downstream I >>>>> don't >>>>> see any use of them. >>>> >>>> Yes, please do post how these are wired as well for completeness. >> >> Did you find these two interrupts as well? >> >> >>> Regarding the points of discussion we had last week on [1], here are >>> some clarifications: >>> >>> 1. We do have hs_phy_irq 1/2/3/4 for tertiary port of Sc8280 as >>> mentioned. Why do we need them and would we use it in multiport >>> targets ? >>> >>> DPSE and DMSE are single ended line state of DP and DM lines. The DP >>> line and DM line stay in steady High or Low during suspend and they flip >>> when there is a RESUME or REMOTE WAKE. This is what we do/check in >>> dwc3_qcom_enable_interrupts call for dp/dm irq's based on usb2_speed. >> >> Right, this bit is clear. >> >>> Initially in QUSB2 targets, the interrupts were enabled and configured >>> in phy and the wakeup was interrupt was read on hs_phy_irq vector - [2]. >>> In that case, we modify DP/DM interrupts in phy registers, specifically >>> QUSB2PHY_INTR_CTRL and when wakeup signal comes in, hs_phy_irq is >>> triggered. But in femto targets, this is done via DP/DM interrupts and >>> there is no use of hs_phy_irq. Even hw folks confirmed they dont use >>> hs_ph_irq in femto phy targets. >> >> Ok, thanks for pointing to QUSB2. The same mechanism is apparently used >> in phy-qcom-usb-hs-28nm.c as well (even if the dtsi currently does not >> define the wakeup interrupts). >> >> Furthermore, that implementation is broken and has never worked due to >> another half-arsed, incomplete Qualcomm implementation. Specifically, no >> one is changing the PHY mode based on the current speed before suspend >> as commits like >> >> 3b3cd24ae61b ("phy: Add USB speed related PHY modes") >> >> and >> >> 891a96f65ac3 ("phy: qcom-qusb2: Add support for runtime PM") >> >> depend on. Guess I should go revert that mess too... >> >>> As an experiment, I tried to test wakeup by pressing buttons on >>> connected keyboard when in suspend state or connecting/disconnecting >>> keyboard in suspended state on different ports and only see dp/dm IRQ's >>> getting fired although we register for hs_phy_irq as well: >>> >>> / # cat /proc/interrupts |grep phy_ >>> 171: 1 0 0 0 0 0 0 0 PDC 127 Edge dp_hs_phy_1 >>> 172: 2 0 0 0 0 0 0 0 PDC 126 Edge dm_hs_phy_1 >>> 173: 3 0 0 0 0 0 0 0 PDC 129 Edge dp_hs_phy_2 >>> 174: 4 0 0 0 0 0 0 0 PDC 128 Edge dm_hs_phy_2 >>> 175: 0 0 0 0 0 0 0 0 PDC 131 Edge dp_hs_phy_3 >>> 176: 2 0 0 0 0 0 0 0 PDC 130 Edge dm_hs_phy_3 >>> 177: 2 0 0 0 0 0 0 0 PDC 133 Edge dp_hs_phy_4 >>> 178: 5 0 0 0 0 0 0 0 PDC 132 Edge dm_hs_phy_4 >>> 179: 0 0 0 0 0 0 0 0 PDC 16 Level ss_phy_1 >>> 180: 0 0 0 0 0 0 0 0 PDC 17 Level ss_phy_2 >>> 181: 0 0 0 0 0 0 0 0 GICv3 163 Level hs_phy_1 >>> 182: 0 0 0 0 0 0 0 0 GICv3 168 Level hs_phy_2 >>> 183: 0 0 0 0 0 0 0 0 GICv3 892 Level hs_phy_3 >>> 184: 0 0 0 0 0 0 0 0 GICv3 891 Level hs_phy_4 >> >> Yes, but that doesn't really say much since you never enable the hs_phy >> interrupt in the PHY on suspend. > > I did register to and enabled the hs_phy_irq interrupt when I tested and > posted the above table. > >>> Since the hs_phy_irq is applicable only for qusb2 targets, do we still >>> need to add it to DT. >> >> Are you sure there's no support for hs_phy_irq also in the "femto" PHYs >> and that it's just that there is currently no driver support for using >> them? >> >> And why is it defined if there is truly no use for it? > > Femto phy's have nothing to be configured for interrupts like we do for > qusb2 phy's. I confirmed from hw validation team that they never used > hs_phy_irq for validating wakeup. They only used dp/dm IRQ's for wakeup. > >> Also, if hs_phy_irq and dp/dm_phy_irq were mutually exclusive, why does >> the following Qualcomm SoCs define all three? >> > > Similar to BAM IRQ's these might have been just ported over targets I > believe. I say so because HW Validation team confirmed they don't use > this interrupt at all on femto phy targets. > >> - qcom,ipq4019-dwc3 >> - qcom,ipq6018-dwc3 >> - qcom,ipq8064-dwc3 >> - qcom,ipq8074-dwc3 >> - qcom,msm8994-dwc3 >> - qcom,qcs404-dwc3 >> - qcom,sc7180-dwc3 >> - qcom,sc7280-dwc3 >> - qcom,sdm670-dwc3 >> - qcom,sdm845-dwc3 >> - qcom,sdx55-dwc3 >> - qcom,sdx65-dwc3 >> - qcom,sm4250-dwc3 >> - qcom,sm6125-dwc3 >> - qcom,sm6350-dwc3 >> - qcom,sm8150-dwc3 >> - qcom,sm8250-dwc3 >> - qcom,sm8350-dwc3 >> - qcom,sm8450-dwc3 >> - qcom,sm8550-dwc3 >> >> Some of those use QUSB2 PHYs and some use "femto" PHYs. >> > And this comes from Qualcomm through commits like: >> >> 0b766e7fe5a2 ("arm64: dts: qcom: sc7180: Add USB related nodes") >> bb9efa59c665 ("arm64: dts: qcom: sc7280: Add USB related nodes") >> >> >>> 3. ctrl_irq[1] usage: >>> >>> This is a feature of SNPS controller, not qcom glue wrapper, and is >>> present on all targets (non-QC as well probably). As mentioned before on >>> [3], this is used for HW acceleration. >>> >>> In host mode, XHCI spec does allow for multiple interrupters when >>> multiple event rings are used. A possible usage is multiple execution >>> environments something like what we are doing on mobile with ADSP audio >>> offload [4]. Another possibility could be some of virtualization where >>> host/hyp would manage the first interrupter and could allow a guest to >>> operate only with the second (though current design does not go far >>> enough to offer true isolation for real VM type workloads). The >>> additional interrupts (ones other than ctrl_irq[0]) are either for >>> virtualization use cases, or for our various “hw offload” features. In >>> device mode, these are used for offloading tethering functionality to >>> IPA FW. >> >> Ok, thanks for clarifying what you meant by "HW acceleration". >> >>> Since the DeviceTree passed to the OS, should describe the hardware to >>> the OS, and should represent the hardware from the point-of-view of the >>> OS, adding one interrupt (ctrl_irq[0]) might be sufficient as Linux >>> would not use the other interrupts. >> >> I've only skimmed the virtualisation bits in xHCI spec, but it seems >> Linux as VMM would still be involved in assigning these interrupts to >> VMs. Hi Krishna/Johan, IMO it might be a bit premature to add definitions for how to utilize secondary interrupters since design wise, there's nothing really too well defined yet. At least for the XHCI path, we will have a slew of potential use cases for secondary interrupters, such as USB audio offloading, or for VMMs, etc... I've only heard mentions about some of them after pushing the usb audio offloading series, but I don't have much details on it. For example, for the USB audio offload path, the idea is to not have to interrupt the apps proc, and allow for an external DSP to be managing the event ring. > > I didn't understand this sentence. Are you referring to cases where > Linux needs to act as the entity using the ctrl_irq[1] ? > > On QCOM SoC's, in reality (atleast in device mode) I can say that we > create the event rings for IPA FW (which registers for ctrl_irq[1]) to > use and read depevt's. We don't register or get this IRQ from DT and > then provide to IPA (not even in downstream). > >> >> This may possibly be something that we can ignore for now, but perhaps >> someone more familiar with the hardware, like Thinh, can chime in. >> >>> Furthermore AFAIK even UEFI/Windows >>> also use only ctrl_irq[0] for host mode in their execution environment >>> today. Do we still need to add this to bindings and DT ? >> >> But the second interrupt is described in the ACPI tables, which means >> that a simple driver update could (in theory) allow for it to be used. >> >> You need to get into the same mindset when it comes to devicetree. Even >> if Linux currently does not use an interrupt, like the pwr_event_irq, >> you should still add it so that when/if someone implements support for >> it, an older platform using the original dt may also take advantage of >> it. Yeah, I totally agree with this point, but I'm not sure if adding it into the "interrupts" array is the way to go. It would probably have to change as support is added. Sorry for jumping in, but just giving my two cents since I'm the one trying to do the initial push for the support for secondary interrupters :). Thanks Wesley Cheng
On Thu, Nov 09, 2023 at 10:08:12PM +0530, Krishna Kurapati PSSNV wrote: > On 11/9/2023 8:48 PM, Johan Hovold wrote: > > On Fri, Nov 03, 2023 at 03:34:52PM +0530, Krishna Kurapati PSSNV wrote: > > > On 10/24/2023 12:26 PM, Johan Hovold wrote: > > > > On Mon, Oct 23, 2023 at 10:42:31PM +0530, Krishna Kurapati PSSNV wrote: > > > > > On 10/23/2023 7:37 PM, Johan Hovold wrote: > > > > > > > > > > Right. And I assume there are hs_phy_irqs also for the first two USB > > > > > > controllers on sc8280xp? > > > > > > > > > There are, I can dig through and find out. Atleast in downstream I don't > > > > > see any use of them. > > > > > > > > Yes, please do post how these are wired as well for completeness. > > > > Did you find these two interrupts as well? Please answer. > > > As an experiment, I tried to test wakeup by pressing buttons on > > > connected keyboard when in suspend state or connecting/disconnecting > > > keyboard in suspended state on different ports and only see dp/dm IRQ's > > > getting fired although we register for hs_phy_irq as well: > > > > > > / # cat /proc/interrupts |grep phy_ > > > 171: 1 0 0 0 0 0 0 0 PDC 127 Edge dp_hs_phy_1 > > > 172: 2 0 0 0 0 0 0 0 PDC 126 Edge dm_hs_phy_1 > > > 173: 3 0 0 0 0 0 0 0 PDC 129 Edge dp_hs_phy_2 > > > 174: 4 0 0 0 0 0 0 0 PDC 128 Edge dm_hs_phy_2 > > > 175: 0 0 0 0 0 0 0 0 PDC 131 Edge dp_hs_phy_3 > > > 176: 2 0 0 0 0 0 0 0 PDC 130 Edge dm_hs_phy_3 > > > 177: 2 0 0 0 0 0 0 0 PDC 133 Edge dp_hs_phy_4 > > > 178: 5 0 0 0 0 0 0 0 PDC 132 Edge dm_hs_phy_4 > > > 179: 0 0 0 0 0 0 0 0 PDC 16 Level ss_phy_1 > > > 180: 0 0 0 0 0 0 0 0 PDC 17 Level ss_phy_2 > > > 181: 0 0 0 0 0 0 0 0 GICv3 163 Level hs_phy_1 > > > 182: 0 0 0 0 0 0 0 0 GICv3 168 Level hs_phy_2 > > > 183: 0 0 0 0 0 0 0 0 GICv3 892 Level hs_phy_3 > > > 184: 0 0 0 0 0 0 0 0 GICv3 891 Level hs_phy_4 > > > > Yes, but that doesn't really say much since you never enable the hs_phy > > interrupt in the PHY on suspend. > > I did register to and enabled the hs_phy_irq interrupt when I tested and > posted the above table. Yes, but, again, you never enabled them in the PHY (cf. QUSB2) so it's hardly surprising that they do not fire. Still good to know that requesting them doesn't trigger spurious interrupts either since these are apparently enabled on most Qualcomm SoCs even though they are not used. We should fix that too. > > > Since the hs_phy_irq is applicable only for qusb2 targets, do we still > > > need to add it to DT. > > > > Are you sure there's no support for hs_phy_irq also in the "femto" PHYs > > and that it's just that there is currently no driver support for using > > them? > > > > And why is it defined if there is truly no use for it? > > Femto phy's have nothing to be configured for interrupts like we do for > qusb2 phy's. I confirmed from hw validation team that they never used > hs_phy_irq for validating wakeup. They only used dp/dm IRQ's for wakeup. Ok. Is there some other (non-wakeup) functionality which may potentially use this interrupt? > > Also, if hs_phy_irq and dp/dm_phy_irq were mutually exclusive, why does > > the following Qualcomm SoCs define all three? > > > > Similar to BAM IRQ's these might have been just ported over targets I > believe. I say so because HW Validation team confirmed they don't use this > interrupt at all on femto phy targets. So then including the hs_phy_irq for most of these SoCs was a mistake and we should drop it from the bindings? What about the QUSB2 SoCs that also define DP/DM, are both useable there? And if so, is there any reason to prefer one mechanism over the other? > > - qcom,ipq4019-dwc3 > > - qcom,ipq6018-dwc3 > > - qcom,ipq8064-dwc3 > > - qcom,ipq8074-dwc3 > > - qcom,msm8994-dwc3 > > - qcom,qcs404-dwc3 > > - qcom,sc7180-dwc3 > > - qcom,sc7280-dwc3 > > - qcom,sdm670-dwc3 > > - qcom,sdm845-dwc3 > > - qcom,sdx55-dwc3 > > - qcom,sdx65-dwc3 > > - qcom,sm4250-dwc3 > > - qcom,sm6125-dwc3 > > - qcom,sm6350-dwc3 > > - qcom,sm8150-dwc3 > > - qcom,sm8250-dwc3 > > - qcom,sm8350-dwc3 > > - qcom,sm8450-dwc3 > > - qcom,sm8550-dwc3 > > > > Some of those use QUSB2 PHYs and some use "femto" PHYs. > > > Since the DeviceTree passed to the OS, should describe the hardware to > > > the OS, and should represent the hardware from the point-of-view of the > > > OS, adding one interrupt (ctrl_irq[0]) might be sufficient as Linux > > > would not use the other interrupts. > > > > I've only skimmed the virtualisation bits in xHCI spec, but it seems > > Linux as VMM would still be involved in assigning these interrupts to > > VMs. > > I didn't understand this sentence. Are you referring to cases where Linux > needs to act as the entity using the ctrl_irq[1] ? It seems Linux acting as VMM would need to be involved in configuring such interrupts and passing them to the VM that eventually use them. > On QCOM SoC's, in reality (atleast in device mode) I can say that we create > the event rings for IPA FW (which registers for ctrl_irq[1]) to use and read > depevt's. We don't register or get this IRQ from DT and then provide to IPA > (not even in downstream). Yeah, I don't know how such things would best be handled. Johan
On Thu, Nov 09, 2023 at 12:25:59PM -0800, Wesley Cheng wrote: > > > > Since the DeviceTree passed to the OS, should describe the hardware to > > > > the OS, and should represent the hardware from the point-of-view of the > > > > OS, adding one interrupt (ctrl_irq[0]) might be sufficient as Linux > > > > would not use the other interrupts. > > > > > > I've only skimmed the virtualisation bits in xHCI spec, but it seems > > > Linux as VMM would still be involved in assigning these interrupts to > > > VMs. > IMO it might be a bit premature to add definitions for how to utilize > secondary interrupters since design wise, there's nothing really too well > defined yet. At least for the XHCI path, we will have a slew of potential > use cases for secondary interrupters, such as USB audio offloading, or for > VMMs, etc... I've only heard mentions about some of them after pushing the > usb audio offloading series, but I don't have much details on it. I tend to agree. > > > This may possibly be something that we can ignore for now, but perhaps > > > someone more familiar with the hardware, like Thinh, can chime in. > > > You need to get into the same mindset when it comes to devicetree. Even > > > if Linux currently does not use an interrupt, like the pwr_event_irq, > > > you should still add it so that when/if someone implements support for > > > it, an older platform using the original dt may also take advantage of > > > it. > > Yeah, I totally agree with this point, but I'm not sure if adding it into > the "interrupts" array is the way to go. It would probably have to change > as support is added. Yes, that in itself would probably not be sufficient and possibly not even correct. > Sorry for jumping in, but just giving my two cents since I'm the one trying > to do the initial push for the support for secondary interrupters :). Appreciate your input. Johan
>>>>>> There are, I can dig through and find out. Atleast in downstream I don't >>>>>> see any use of them. >>>>> >>>>> Yes, please do post how these are wired as well for completeness. >>> >>> Did you find these two interrupts as well? > > Please answer. > Yes. Controller-1: u_usb31_prim_mvs_wrapper_usb31_hs_phy_irq SYS_apcsQgicSPI[806] Controller-2: u_usb31_prim_mvs_wrapper_usb31_hs_phy_irq SYS_apcsQgicSPI[791] >>>> As an experiment, I tried to test wakeup by pressing buttons on >>>> connected keyboard when in suspend state or connecting/disconnecting >>>> keyboard in suspended state on different ports and only see dp/dm IRQ's >>>> getting fired although we register for hs_phy_irq as well: >>>> >>>> / # cat /proc/interrupts |grep phy_ >>>> 171: 1 0 0 0 0 0 0 0 PDC 127 Edge dp_hs_phy_1 >>>> 172: 2 0 0 0 0 0 0 0 PDC 126 Edge dm_hs_phy_1 >>>> 173: 3 0 0 0 0 0 0 0 PDC 129 Edge dp_hs_phy_2 >>>> 174: 4 0 0 0 0 0 0 0 PDC 128 Edge dm_hs_phy_2 >>>> 175: 0 0 0 0 0 0 0 0 PDC 131 Edge dp_hs_phy_3 >>>> 176: 2 0 0 0 0 0 0 0 PDC 130 Edge dm_hs_phy_3 >>>> 177: 2 0 0 0 0 0 0 0 PDC 133 Edge dp_hs_phy_4 >>>> 178: 5 0 0 0 0 0 0 0 PDC 132 Edge dm_hs_phy_4 >>>> 179: 0 0 0 0 0 0 0 0 PDC 16 Level ss_phy_1 >>>> 180: 0 0 0 0 0 0 0 0 PDC 17 Level ss_phy_2 >>>> 181: 0 0 0 0 0 0 0 0 GICv3 163 Level hs_phy_1 >>>> 182: 0 0 0 0 0 0 0 0 GICv3 168 Level hs_phy_2 >>>> 183: 0 0 0 0 0 0 0 0 GICv3 892 Level hs_phy_3 >>>> 184: 0 0 0 0 0 0 0 0 GICv3 891 Level hs_phy_4 >>> >>> Yes, but that doesn't really say much since you never enable the hs_phy >>> interrupt in the PHY on suspend. >> >> I did register to and enabled the hs_phy_irq interrupt when I tested and >> posted the above table. > > Yes, but, again, you never enabled them in the PHY (cf. QUSB2) so it's > hardly surprising that they do not fire. > There is no register in femto phy address space of sc8280 (which I am currently testing) where we can configure these registers like qusb2 phy's. > Still good to know that requesting them doesn't trigger spurious > interrupts either since these are apparently enabled on most Qualcomm > SoCs even though they are not used. We should fix that too. > >>>> Since the hs_phy_irq is applicable only for qusb2 targets, do we still >>>> need to add it to DT. >>> >>> Are you sure there's no support for hs_phy_irq also in the "femto" PHYs >>> and that it's just that there is currently no driver support for using >>> them? >>> >>> And why is it defined if there is truly no use for it? >> >> Femto phy's have nothing to be configured for interrupts like we do for >> qusb2 phy's. I confirmed from hw validation team that they never used >> hs_phy_irq for validating wakeup. They only used dp/dm IRQ's for wakeup. > > Ok. > > Is there some other (non-wakeup) functionality which may potentially use > this interrupt? > The only info I (and hw validation team) got from design team is: 1. Common IRQ for power and special events 2. Assert in case of remote wakeup, or resume when in Host or device respectively 3. Also upon disconnect while in suspend state. Same as what we understand as remote wakeup functionality. >>> Also, if hs_phy_irq and dp/dm_phy_irq were mutually exclusive, why does >>> the following Qualcomm SoCs define all three? >>> >> >> Similar to BAM IRQ's these might have been just ported over targets I >> believe. I say so because HW Validation team confirmed they don't use this >> interrupt at all on femto phy targets. > > So then including the hs_phy_irq for most of these SoCs was a mistake > and we should drop it from the bindings? > > What about the QUSB2 SoCs that also define DP/DM, are both useable > there? > > And if so, is there any reason to prefer one mechanism over the other? > No. I didn't ask this question to hw team whether dp/dm are used in qusb2 phy targets. Let me ask them. While I do so, since there are no qusb2 targets present on femto phy's, do you suggest we still add them to port structure in dwc3-qcom ? I am inclined to add it because it would make implementation look cleaner w.r.t code and also spurious interrupts are not getting triggered (which was my primary concern as it was never tested). I know that if hs_phy_irq is for qusb2 and dp/dm are for femto, the cleanup would be big. Regards, Krishna,
On Fri, Nov 10, 2023 at 03:31:15PM +0530, Krishna Kurapati PSSNV wrote: > Controller-1: > u_usb31_prim_mvs_wrapper_usb31_hs_phy_irq SYS_apcsQgicSPI[806] > Controller-2: > u_usb31_prim_mvs_wrapper_usb31_hs_phy_irq SYS_apcsQgicSPI[791] Thanks. > > Yes, but, again, you never enabled them in the PHY (cf. QUSB2) so it's > > hardly surprising that they do not fire. > > > There is no register in femto phy address space of sc8280 (which I am > currently testing) where we can configure these registers like qusb2 phy's. Right, so they are not enabled (and possibly cannot be enabled). > > So then including the hs_phy_irq for most of these SoCs was a mistake > > and we should drop it from the bindings? > > > > What about the QUSB2 SoCs that also define DP/DM, are both useable > > there? > > > > And if so, is there any reason to prefer one mechanism over the other? > > No. I didn't ask this question to hw team whether dp/dm are used in qusb2 > phy targets. Let me ask them. > > While I do so, since there are no qusb2 targets present on femto phy's, do > you suggest we still add them to port structure in dwc3-qcom ? I am inclined > to add it because it would make implementation look cleaner w.r.t code and > also spurious interrupts are not getting triggered (which was my primary > concern as it was never tested). Yes, that's what I've been suggesting all along. It's a per-port interrupt so that's where it belongs. We should still try to determine when each interrupt should be enabled and how best to implement that (hence all my questions). For example, if there is no use for hs interrupts on SoCs using femto PHYs we should fix the bindings. If we can use dp/dm on SoCs using QUSB2 PHYs, we should probably just ignore the hs interrupt when all three are defined (especially since that functionality has never worked anyway). Johan
>> >> While I do so, since there are no qusb2 targets present on femto phy's, do >> you suggest we still add them to port structure in dwc3-qcom ? I am inclined >> to add it because it would make implementation look cleaner w.r.t code and >> also spurious interrupts are not getting triggered (which was my primary >> concern as it was never tested). > > Yes, that's what I've been suggesting all along. It's a per-port > interrupt so that's where it belongs. > > We should still try to determine when each interrupt should be enabled > and how best to implement that (hence all my questions). > > For example, if there is no use for hs interrupts on SoCs using femto > PHYs we should fix the bindings. If we can use dp/dm on SoCs using QUSB2 > PHYs, we should probably just ignore the hs interrupt when all three are > defined (especially since that functionality has never worked anyway). > Sure. Will finalise this once I get the complete info (why do we have dp/dm on qusb targets) And apologies, I mentioned "qusb2 targets on femto phy's". It was supposed to be "hs_phy_irq's on femto phy targets", but I think you got the gist of my question. Thanks for the response. Regards, Krishna,
Hi Johan, > Are you sure there's no support for hs_phy_irq also in the "femto" PHYs > and that it's just that there is currently no driver support for using > them? > > And why is it defined if there is truly no use for it? > We had an internal sync up with HW folks and here is some baseline suggestions we received: If DP/DM interrupts are defined, then that is the preferred path to used, irrespective if HS Phy irq is defined or not / or whether it is Femto / QUSB2 target. There is no target that has femto phy but misses DP/DM today. For cases like sdm660/msm8998/msm8953/msm8956, these targets use hs_phy_irq only and don't rely on DP/DM. So we cannot remove the binding in entirety. > Also, if hs_phy_irq and dp/dm_phy_irq were mutually exclusive, why does > the following Qualcomm SoCs define all three? > HS Phy Irq is redundant or functionality is mutually exclusive in this case. If there are targets that define all three, then we need to update those to only utilize DP/DM interrupts. Regards, Krishna,
On Wed, Nov 15, 2023 at 11:12:16PM +0530, Krishna Kurapati PSSNV wrote: > > Are you sure there's no support for hs_phy_irq also in the "femto" PHYs > > and that it's just that there is currently no driver support for using > > them? > > > > And why is it defined if there is truly no use for it? > We had an internal sync up with HW folks and here is some baseline > suggestions we received: > > If DP/DM interrupts are defined, then that is the preferred path to > used, irrespective if HS Phy irq is defined or not / or whether it is > Femto / QUSB2 target. There is no target that has femto phy but misses > DP/DM today. Ok, but just knowing that it is "preferred" does not in itself mean that it should be removed from the binding. We need to know that it's effectively useless (i.e. that the interrupts are defined but cannot be triggered) for that. We can still use the DP/DM interrupts in favour of HS in the driver however. > For cases like sdm660/msm8998/msm8953/msm8956, these targets use > hs_phy_irq only and don't rely on DP/DM. So we cannot remove the binding > in entirety. I fixed the binding for those specific platforms last year: dd566faebe9f ("dt-bindings: usb: qcom,dwc3: refine interrupt requirements") But as I mentioned in that commit message the following platforms do not have any wakeup interrupts specified in mainline currently: - qcom,ipq4019-dwc3 - qcom,ipq6018-dwc3 - qcom,ipq8064-dwc3 - qcom,ipq8074-dwc3 - qcom,msm8994-dwc3 - qcom,qcs404-dwc3 It would be good to get that cleaned up too (i.e. add the missing interrupt definitions and update the binding to match). > > Also, if hs_phy_irq and dp/dm_phy_irq were mutually exclusive, why does > > the following Qualcomm SoCs define all three? > HS Phy Irq is redundant or functionality is mutually exclusive in this > case. If there are targets that define all three, then we need to update > those to only utilize DP/DM interrupts. No, as I wrote above that depends on if the HS interrupt is truly useless. Otherwise it still belongs in the binding, even if the driver uses DP/DM in place of it. Again, the binding should describe the hardware, not what a particular OS chooses to use. Johan
> >> I didn't add missing interrupts on sc8280xp because I see that current >> interrupts present are working fine (I see ADB working and wakeup >> working as well), but the interrupt vector numbers are off by "1" >> between hs specifics and DT (both upstream and downstream). Will sort it >> out and clean that target up later. > > Which interrupt numbers are off by one here? > My bad, this might be the confusion. The HW specifics say: Controller-2, power_event irq: SYS_apcsQgicSPI[812] Vector-number: 843 Usually vector number = 32 + GIC number AFAIK. By that logic, If vector number is 843, GIC_SPI number is 811 which is same as DT. Probably the GIC_SPI number is printed wrong. The DT matches (vector number - 32). Sorry for mentioning that it is wrong. The DT entries are right and it is working on upstream. The missing hs_phy_irq's have been put on the mail thread on this list before. Regards, Krishna, >> [1]: https://patchwork.kernel.org/project/linux-arm-msm/list/?series=803412 > > I took a quick look at the series, and it looks like this will > eventually clean things up a lot. We should probably define a generic > order for the interrupts with the sometimes optional SS interrupts last. > > Side note: It looks like the threading in that series is broken. > Consider using git-send-email for sending series as it takes care of > things like that. > Usually I do git send-email for the whole out folder where the patches are present, but linux-usb list is common to all the patches in that case, even the DT ones. So to avoid that and to send patches to only relavant mailing lists, I did git send email individually on each patch which might have caused this issue. Will make sure this won't happen again. Regards, Krishna,
On 24/11/2023 10:00, Krishna Kurapati PSSNV wrote: >> >>> I didn't add missing interrupts on sc8280xp because I see that current >>> interrupts present are working fine (I see ADB working and wakeup >>> working as well), but the interrupt vector numbers are off by "1" >>> between hs specifics and DT (both upstream and downstream). Will sort it >>> out and clean that target up later. >> >> Which interrupt numbers are off by one here? >> > > My bad, this might be the confusion. The HW specifics say: > > Controller-2, power_event irq: > > SYS_apcsQgicSPI[812] Vector-number: 843 > > > Usually vector number = 32 + GIC number AFAIK. > By that logic, If vector number is 843, GIC_SPI number is 811 which is > same as DT. Probably the GIC_SPI number is printed wrong. The DT matches > (vector number - 32). > > Sorry for mentioning that it is wrong. The DT entries are right and it > is working on upstream. > > The missing hs_phy_irq's have been put on the mail thread on this list > before. > > Regards, > Krishna, > >>> [1]: https://patchwork.kernel.org/project/linux-arm-msm/list/?series=803412 >> >> I took a quick look at the series, and it looks like this will >> eventually clean things up a lot. We should probably define a generic >> order for the interrupts with the sometimes optional SS interrupts last. >> >> Side note: It looks like the threading in that series is broken. >> Consider using git-send-email for sending series as it takes care of >> things like that. >> > > Usually I do git send-email for the whole out folder where the patches > are present, but linux-usb list is common to all the patches in that > case, even the DT ones. So to avoid that and to send patches to only > relavant mailing lists, I did git send email individually on each patch > which might have caused this issue. I don't understand why. This is some weird workflow. If you do not use b4, then it is simple: git format-patch -10 -v13 get_maintainers v13* git send-email v13* And that's it. Last two steps can be even one command, like I am doing (shared the macro multiple times). Best regards, Krzysztof
On Fri, Nov 24, 2023 at 02:30:56PM +0530, Krishna Kurapati PSSNV wrote: > > > >> I didn't add missing interrupts on sc8280xp because I see that current > >> interrupts present are working fine (I see ADB working and wakeup > >> working as well), but the interrupt vector numbers are off by "1" > >> between hs specifics and DT (both upstream and downstream). Will sort it > >> out and clean that target up later. > > > > Which interrupt numbers are off by one here? > Sorry for mentioning that it is wrong. The DT entries are right and it > is working on upstream. Thanks for clarifying. > >> [1]: https://patchwork.kernel.org/project/linux-arm-msm/list/?series=803412 > > > > I took a quick look at the series, and it looks like this will > > eventually clean things up a lot. We should probably define a generic > > order for the interrupts with the sometimes optional SS interrupts last. > > > > Side note: It looks like the threading in that series is broken. > > Consider using git-send-email for sending series as it takes care of > > things like that. > > Usually I do git send-email for the whole out folder where the patches > are present, but linux-usb list is common to all the patches in that > case, even the DT ones. So to avoid that and to send patches to only > relavant mailing lists, I did git send email individually on each patch > which might have caused this issue. I'd suggest that you just send two separate series, one with binding and driver updates, which will eventually be merged by Greg, and one with the devicetree changes, which goes through Bjorn's tree. It's good if you could add a link to the binding series in the cover letter of the devicetree changes as they are of course going to be quite closely related and need to be reviewed in parallel. Johan
On 11/24/2023 3:43 PM, Johan Hovold wrote: > On Fri, Nov 24, 2023 at 02:30:56PM +0530, Krishna Kurapati PSSNV wrote: >>> >>>> I didn't add missing interrupts on sc8280xp because I see that current >>>> interrupts present are working fine (I see ADB working and wakeup >>>> working as well), but the interrupt vector numbers are off by "1" >>>> between hs specifics and DT (both upstream and downstream). Will sort it >>>> out and clean that target up later. >>> >>> Which interrupt numbers are off by one here? > >> Sorry for mentioning that it is wrong. The DT entries are right and it >> is working on upstream. > > Thanks for clarifying. > >>>> [1]: https://patchwork.kernel.org/project/linux-arm-msm/list/?series=803412 >>> >>> I took a quick look at the series, and it looks like this will >>> eventually clean things up a lot. We should probably define a generic >>> order for the interrupts with the sometimes optional SS interrupts last. >>> >>> Side note: It looks like the threading in that series is broken. >>> Consider using git-send-email for sending series as it takes care of >>> things like that. >> >> Usually I do git send-email for the whole out folder where the patches >> are present, but linux-usb list is common to all the patches in that >> case, even the DT ones. So to avoid that and to send patches to only >> relavant mailing lists, I did git send email individually on each patch >> which might have caused this issue. > > I'd suggest that you just send two separate series, one with binding and > driver updates, which will eventually be merged by Greg, and one with > the devicetree changes, which goes through Bjorn's tree. > > It's good if you could add a link to the binding series in the cover > letter of the devicetree changes as they are of course going to be quite > closely related and need to be reviewed in parallel. > Thanks for this pointer. So for Multiport, can I do it this way: 1. Core bindings and Core driver changes in one series. Now that we finalized we don't be adding the ctrl_irq[1] as discussed on: https://lore.kernel.org/all/ZU33uWpStIobzyd6@hovoldconsulting.com/. 2. QC bindings and QC driver changes for Multiport to be pushed after we clean up the current driver and DT's (an effort which is going on currently). Regards, Krishna,
On Fri, Nov 24, 2023 at 04:08:42PM +0530, Krishna Kurapati PSSNV wrote: > On 11/24/2023 3:43 PM, Johan Hovold wrote: > > I'd suggest that you just send two separate series, one with binding and > > driver updates, which will eventually be merged by Greg, and one with > > the devicetree changes, which goes through Bjorn's tree. > > > > It's good if you could add a link to the binding series in the cover > > letter of the devicetree changes as they are of course going to be quite > > closely related and need to be reviewed in parallel. > > Thanks for this pointer. So for Multiport, can I do it this way: > > 1. Core bindings and Core driver changes in one series. Now that we > finalized we don't be adding the ctrl_irq[1] as discussed on: > https://lore.kernel.org/all/ZU33uWpStIobzyd6@hovoldconsulting.com/. > > 2. QC bindings and QC driver changes for Multiport to be pushed after we > clean up the current driver and DT's (an effort which is going on > currently). No, I was just referring to how to handle binding/driver vs devicetree patches for USB where we send them separately (unlike for most other subsystems). The dwc3 core and Qualcomm glue parts should still go in the same series for multiport support. Whether to do the irq cleanup before or after adding multiport support is a different question, but, yeah, it is probably best to do it before. The question of whether we can drop ACPI support should also be considered as that should also simplify your multiport series. Johan
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index ef2006db7601..863892284146 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -53,14 +53,25 @@ #define APPS_USB_AVG_BW 0 #define APPS_USB_PEAK_BW MBps_to_icc(40) +#define NUM_PHY_IRQ 4 + +enum dwc3_qcom_ph_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; enum usb_device_speed usb2_speed; struct extcon_dev *edev; @@ -91,6 +104,7 @@ struct dwc3_qcom { bool pm_suspended; struct icc_path *icc_path_ddr; struct icc_path *icc_path_apps; + int num_ports; }; static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) @@ -375,16 +389,16 @@ static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom) dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq); if (qcom->usb2_speed == USB_SPEED_LOW) { - dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq); + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]); } else if ((qcom->usb2_speed == USB_SPEED_HIGH) || (qcom->usb2_speed == USB_SPEED_FULL)) { - dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq); + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]); } else { - dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq); - dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq); + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]); + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]); } - dwc3_qcom_disable_wakeup_irq(qcom->ss_phy_irq); + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][0]); } static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) @@ -401,20 +415,20 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) */ if (qcom->usb2_speed == USB_SPEED_LOW) { - dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq, + dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0], IRQ_TYPE_EDGE_FALLING); } else if ((qcom->usb2_speed == USB_SPEED_HIGH) || (qcom->usb2_speed == USB_SPEED_FULL)) { - dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq, + dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0], IRQ_TYPE_EDGE_FALLING); } else { - dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq, + dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0], IRQ_TYPE_EDGE_RISING); - dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq, + dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0], IRQ_TYPE_EDGE_RISING); } - dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq, 0); + dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][0], 0); } static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) @@ -535,8 +549,8 @@ static int dwc3_qcom_get_irq(struct platform_device *pdev, return ret; } -static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, char *irq_name, - char *disp_name, int irq) +static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, const char *irq_name, + const char *disp_name, int irq) { int ret; @@ -553,47 +567,135 @@ static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, char *irq_name, return ret; } +static int dwc3_qcom_get_irq_index(const char *irq_name) +{ + /* + * If we are reading IRQ not supported by the driver + * like pwr_event_irq, then return -1 indicating the next + * helper function to skip processing IRQ name further. + */ + int irq_index = -1; + + if (strncmp(irq_name, "dp_hs_phy", strlen("dp_hs_phy")) == 0) + irq_index = DP_HS_PHY_IRQ_INDEX; + else if (strncmp(irq_name, "dm_hs_phy", strlen("dm_hs_phy")) == 0) + irq_index = DM_HS_PHY_IRQ_INDEX; + else if (strncmp(irq_name, "ss_phy", strlen("ss_phy")) == 0) + irq_index = SS_PHY_IRQ_INDEX; + else if (strncmp(irq_name, "hs_phy", strlen("hs_phy")) == 0) + irq_index = HS_PHY_IRQ_INDEX; + return irq_index; +} + +static int dwc3_qcom_get_port_index(const char *irq_name, int irq_index) +{ + int port_index = -1; + + switch (irq_index) { + case DP_HS_PHY_IRQ_INDEX: + if (strcmp(irq_name, "dp_hs_phy_irq") == 0) + port_index = 1; + else + sscanf(irq_name, "dp_hs_phy_%d", &port_index); + break; + + case DM_HS_PHY_IRQ_INDEX: + if (strcmp(irq_name, "dm_hs_phy_irq") == 0) + port_index = 1; + else + sscanf(irq_name, "dm_hs_phy_%d", &port_index); + break; + + case SS_PHY_IRQ_INDEX: + if (strcmp(irq_name, "ss_phy_irq") == 0) + port_index = 1; + else + sscanf(irq_name, "ss_phy_%d", &port_index); + break; + + case HS_PHY_IRQ_INDEX: + port_index = 1; + break; + } + + if (port_index > DWC3_MAX_PORTS) + port_index = -1; + + return port_index; +} + +static int dwc3_qcom_get_acpi_index(struct dwc3_qcom *qcom, int irq_index, + int port_index) +{ + const struct dwc3_acpi_pdata *pdata = qcom->acpi_pdata; + int acpi_index = -1; + + /* + * Currently multiport supported targets don't have an ACPI variant. + * So return -1 if we are not dealing with first port of the controller. + */ + if ((pdata == NULL) || (port_index != 1)) + goto done; + + if (irq_index == HS_PHY_IRQ_INDEX) + acpi_index = pdata->hs_phy_irq_index; + else + acpi_index = pdata->phy_irq_index[irq_index]; + +done: + return acpi_index; +} + static int dwc3_qcom_setup_irq(struct platform_device *pdev) { struct dwc3_qcom *qcom = platform_get_drvdata(pdev); - const struct dwc3_acpi_pdata *pdata = qcom->acpi_pdata; + struct device_node *np = pdev->dev.of_node; + const char **irq_names; + int port_index; + int acpi_index; + int irq_count; + int irq_index; int irq; int ret; + int i; - irq = dwc3_qcom_get_irq(pdev, "hs_phy_irq", - pdata ? pdata->hs_phy_irq_index : -1); - if (irq > 0) { - ret = dwc3_qcom_prep_irq(qcom, "hs_phy_irq", "qcom_dwc3 HS", irq); - if (ret) - return ret; - qcom->hs_phy_irq = irq; - } + irq_count = of_property_count_strings(np, "interrupt-names"); + irq_names = devm_kzalloc(&pdev->dev, sizeof(*irq_names) * irq_count, GFP_KERNEL); + if (!irq_names) + return -ENOMEM; - irq = dwc3_qcom_get_irq(pdev, "dp_hs_phy_irq", - pdata ? pdata->dp_hs_phy_irq_index : -1); - if (irq > 0) { - ret = dwc3_qcom_prep_irq(qcom, "dp_hs_phy_irq", "qcom_dwc3 DP_HS", irq); - if (ret) - return ret; - qcom->dp_hs_phy_irq = irq; - } + ret = of_property_read_string_array(np, "interrupt-names", + irq_names, irq_count); + for (i = 0; i < irq_count; i++) { + irq_index = dwc3_qcom_get_irq_index(irq_names[i]); + if (irq_index == -1) { + dev_dbg(&pdev->dev, "Invalid IRQ not handled"); + continue; + } - irq = dwc3_qcom_get_irq(pdev, "dm_hs_phy_irq", - pdata ? pdata->dm_hs_phy_irq_index : -1); - if (irq > 0) { - ret = dwc3_qcom_prep_irq(qcom, "dm_hs_phy_irq", "qcom_dwc3 DM_HS", irq); - if (ret) - return ret; - qcom->dm_hs_phy_irq = irq; - } + port_index = dwc3_qcom_get_port_index(irq_names[i], irq_index); + if (port_index == -1) { + dev_dbg(&pdev->dev, "Port index invalid. IRQ not handled"); + continue; + } - irq = dwc3_qcom_get_irq(pdev, "ss_phy_irq", - pdata ? pdata->ss_phy_irq_index : -1); - if (irq > 0) { - ret = dwc3_qcom_prep_irq(qcom, "ss_phy_irq", "qcom_dwc3 SS", irq); - if (ret) - return ret; - qcom->ss_phy_irq = irq; + acpi_index = dwc3_qcom_get_acpi_index(qcom, irq_index, port_index); + + irq = dwc3_qcom_get_irq(pdev, irq_names[i], acpi_index); + if (irq > 0) { + ret = dwc3_qcom_prep_irq(qcom, irq_names[i], + irq_names[i], irq); + if (ret) + return ret; + + if (irq_index == HS_PHY_IRQ_INDEX) + qcom->hs_phy_irq = irq; + else + qcom->phy_irq[irq_index][port_index-1] = irq; + + if (qcom->num_ports < port_index) + qcom->num_ports = port_index; + } } return 0; @@ -1026,20 +1128,16 @@ static const struct dwc3_acpi_pdata sdm845_acpi_pdata = { .qscratch_base_offset = SDM845_QSCRATCH_BASE_OFFSET, .qscratch_base_size = SDM845_QSCRATCH_SIZE, .dwc3_core_base_size = SDM845_DWC3_CORE_SIZE, + .phy_irq_index = {4, 3, 2}, .hs_phy_irq_index = 1, - .dp_hs_phy_irq_index = 4, - .dm_hs_phy_irq_index = 3, - .ss_phy_irq_index = 2 }; static const struct dwc3_acpi_pdata sdm845_acpi_urs_pdata = { .qscratch_base_offset = SDM845_QSCRATCH_BASE_OFFSET, .qscratch_base_size = SDM845_QSCRATCH_SIZE, .dwc3_core_base_size = SDM845_DWC3_CORE_SIZE, + .phy_irq_index = {4, 3, 2}, .hs_phy_irq_index = 1, - .dp_hs_phy_irq_index = 4, - .dm_hs_phy_irq_index = 3, - .ss_phy_irq_index = 2, .is_urs = true, };
Refactor setup_irq call to facilitate reading multiport IRQ's along with non mulitport ones. Read through the interrupt-names property to figure out, the type of interrupt (DP/DM/HS/SS) and to which port it belongs. Also keep track of port index to calculate port count based on interrupts provided as input in DT. Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> --- drivers/usb/dwc3/dwc3-qcom.c | 210 +++++++++++++++++++++++++---------- 1 file changed, 154 insertions(+), 56 deletions(-)