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@xxxxxxxxxxx/
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
