[v8,6/9] usb: dwc3: qcom: Add multiport controller support for qcom wrapper

  QCOM SoC SA8295P's tertiary quad port controller supports 2 HS+SS
ports and 2 HS only ports. Add support for configuring PWR_EVENT_IRQ's
for all the ports during suspend/resume.

Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 drivers/usb/dwc3/dwc3-qcom.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)
  

On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote:
> QCOM SoC SA8295P's tertiary quad port controller supports 2 HS+SS
> ports and 2 HS only ports. Add support for configuring PWR_EVENT_IRQ's
> for all the ports during suspend/resume.
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 959fc925ca7c..7a9bce66295d 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -37,7 +37,10 @@
>  #define PIPE3_PHYSTATUS_SW			BIT(3)
>  #define PIPE_UTMI_CLK_DIS			BIT(8)
>  
> -#define PWR_EVNT_IRQ_STAT_REG			0x58
> +#define PWR_EVNT_IRQ1_STAT_REG			0x58
> +#define PWR_EVNT_IRQ2_STAT_REG			0x1dc
> +#define PWR_EVNT_IRQ3_STAT_REG			0x228
> +#define PWR_EVNT_IRQ4_STAT_REG			0x238
>  #define PWR_EVNT_LPM_IN_L2_MASK			BIT(4)
>  #define PWR_EVNT_LPM_OUT_L2_MASK		BIT(5)
>  
> @@ -93,6 +96,13 @@ struct dwc3_qcom {
>  	struct icc_path		*icc_path_apps;
>  };
>  
> +static u32 pwr_evnt_irq_stat_reg_offset[4] = {
> +			PWR_EVNT_IRQ1_STAT_REG,
> +			PWR_EVNT_IRQ2_STAT_REG,
> +			PWR_EVNT_IRQ3_STAT_REG,
> +			PWR_EVNT_IRQ4_STAT_REG,

Seems to be excessive indentation of these...

Can you also please confirm that these should be counted starting at 1 -
given that you otherwise talk about port0..N-1?

> +};
> +
>  static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
>  {
>  	u32 reg;
> @@ -413,13 +423,16 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
>  {
>  	u32 val;
>  	int i, ret;
> +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
>  
>  	if (qcom->is_suspended)
>  		return 0;
>  
> -	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
> -	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
> -		dev_err(qcom->dev, "HS-PHY not in L2\n");
> +	for (i = 0; i < dwc->num_usb2_ports; i++) {

In the event that the dwc3 core fails to acquire or enable e.g. clocks
its drvdata will be NULL. If you then hit a runtime pm transition in the
dwc3-qcom glue you will dereference NULL here. (You can force this issue
by e.g. returning -EINVAL from dwc3_clk_enable()).

So if you're peaking into qcom->dwc3 you need to handle the fact that
dwc might be NULL, here and in resume below.

Regards,
Bjorn
  

On 5/16/2023 3:57 AM, Bjorn Andersson wrote:
> On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote:
>> QCOM SoC SA8295P's tertiary quad port controller supports 2 HS+SS
>> ports and 2 HS only ports. Add support for configuring PWR_EVENT_IRQ's
>> for all the ports during suspend/resume.
>>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> ---
>>   drivers/usb/dwc3/dwc3-qcom.c | 28 ++++++++++++++++++++++------
>>   1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
>> index 959fc925ca7c..7a9bce66295d 100644
>> --- a/drivers/usb/dwc3/dwc3-qcom.c
>> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>> @@ -37,7 +37,10 @@
>>   #define PIPE3_PHYSTATUS_SW			BIT(3)
>>   #define PIPE_UTMI_CLK_DIS			BIT(8)
>>   
>> -#define PWR_EVNT_IRQ_STAT_REG			0x58
>> +#define PWR_EVNT_IRQ1_STAT_REG			0x58
>> +#define PWR_EVNT_IRQ2_STAT_REG			0x1dc
>> +#define PWR_EVNT_IRQ3_STAT_REG			0x228
>> +#define PWR_EVNT_IRQ4_STAT_REG			0x238
>>   #define PWR_EVNT_LPM_IN_L2_MASK			BIT(4)
>>   #define PWR_EVNT_LPM_OUT_L2_MASK		BIT(5)
>>   
>> @@ -93,6 +96,13 @@ struct dwc3_qcom {
>>   	struct icc_path		*icc_path_apps;
>>   };
>>   
>> +static u32 pwr_evnt_irq_stat_reg_offset[4] = {
>> +			PWR_EVNT_IRQ1_STAT_REG,
>> +			PWR_EVNT_IRQ2_STAT_REG,
>> +			PWR_EVNT_IRQ3_STAT_REG,
>> +			PWR_EVNT_IRQ4_STAT_REG,
> 
> Seems to be excessive indentation of these...
> 
> Can you also please confirm that these should be counted starting at 1 -
> given that you otherwise talk about port0..N-1?
> 
Hi Bjorn,

   I am fine with either way. Since this just denoted 4 different ports, 
I named them starting with 1. Either ways, we will run through array 
from (0-3), so we must be fine.

>> +};
>> +
>>   static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
>>   {
>>   	u32 reg;
>> @@ -413,13 +423,16 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
>>   {
>>   	u32 val;
>>   	int i, ret;
>> +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
>>   
>>   	if (qcom->is_suspended)
>>   		return 0;
>>   
>> -	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
>> -	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
>> -		dev_err(qcom->dev, "HS-PHY not in L2\n");
>> +	for (i = 0; i < dwc->num_usb2_ports; i++) {
> 
> In the event that the dwc3 core fails to acquire or enable e.g. clocks
> its drvdata will be NULL. If you then hit a runtime pm transition in the
> dwc3-qcom glue you will dereference NULL here. (You can force this issue
> by e.g. returning -EINVAL from dwc3_clk_enable()).
> 
> So if you're peaking into qcom->dwc3 you need to handle the fact that
> dwc might be NULL, here and in resume below.
> 
Thanks for catching this. You are right, there were instances where the 
we saw probe for dwc3 being deferred while the probe for dwc3-qcom was 
still successful [1]. In this case, if the dwc3 probe never happened and 
system tries to enter suspend, we might hit a NULL pointer dereference.

I will fix this in v9.

[1]: 
https://patchwork.kernel.org/project/linux-usb/patch/1657809067-25815-1-git-send-email-quic_kriskura@quicinc.com/

Regards,
Krishna,
  

On Tue, May 16, 2023 at 07:49:14AM +0530, Krishna Kurapati PSSNV wrote:
> 
> 
> On 5/16/2023 3:57 AM, Bjorn Andersson wrote:
> > On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote:

> >> -#define PWR_EVNT_IRQ_STAT_REG			0x58
> >> +#define PWR_EVNT_IRQ1_STAT_REG			0x58
> >> +#define PWR_EVNT_IRQ2_STAT_REG			0x1dc
> >> +#define PWR_EVNT_IRQ3_STAT_REG			0x228
> >> +#define PWR_EVNT_IRQ4_STAT_REG			0x238
> >>   #define PWR_EVNT_LPM_IN_L2_MASK			BIT(4)
> >>   #define PWR_EVNT_LPM_OUT_L2_MASK		BIT(5)
> >>   
> >> @@ -93,6 +96,13 @@ struct dwc3_qcom {
> >>   	struct icc_path		*icc_path_apps;
> >>   };
> >>   
> >> +static u32 pwr_evnt_irq_stat_reg_offset[4] = {
> >> +			PWR_EVNT_IRQ1_STAT_REG,
> >> +			PWR_EVNT_IRQ2_STAT_REG,
> >> +			PWR_EVNT_IRQ3_STAT_REG,
> >> +			PWR_EVNT_IRQ4_STAT_REG,
> > 
> > Seems to be excessive indentation of these...
> > 
> > Can you also please confirm that these should be counted starting at 1 -
> > given that you otherwise talk about port0..N-1?

>    I am fine with either way. Since this just denoted 4 different ports, 
> I named them starting with 1. Either ways, we will run through array 
> from (0-3), so we must be fine.

Actually, the USB ports are indexed from 1, so the above naming may or
may not be correct depending on how they are defined.

> >> +};
> >> +
> >>   static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
> >>   {
> >>   	u32 reg;
> >> @@ -413,13 +423,16 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
> >>   {
> >>   	u32 val;
> >>   	int i, ret;
> >> +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> >>   
> >>   	if (qcom->is_suspended)
> >>   		return 0;
> >>   
> >> -	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
> >> -	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
> >> -		dev_err(qcom->dev, "HS-PHY not in L2\n");
> >> +	for (i = 0; i < dwc->num_usb2_ports; i++) {
> > 
> > In the event that the dwc3 core fails to acquire or enable e.g. clocks
> > its drvdata will be NULL. If you then hit a runtime pm transition in the
> > dwc3-qcom glue you will dereference NULL here. (You can force this issue
> > by e.g. returning -EINVAL from dwc3_clk_enable()).
> > 
> > So if you're peaking into qcom->dwc3 you need to handle the fact that
> > dwc might be NULL, here and in resume below.
> > 
> Thanks for catching this. You are right, there were instances where the 
> we saw probe for dwc3 being deferred while the probe for dwc3-qcom was 
> still successful [1]. In this case, if the dwc3 probe never happened and 
> system tries to enter suspend, we might hit a NULL pointer dereference.

I don't think we should be adding more of these layering violations. A
parent device driver has no business messing with the driver data for a
child device which may or may not even have probed yet.

I added a FIXME elsewhere in the driver about fixing up the current
instances that have already snuck in (which in some sense is even worse
by accessing driver data of a grandchild device).

We really need to try sort this mess out and how to properly handle the
interactions between these layers (e.g. glue, dwc3 core and xhci). This
will likely involve adding callbacks from the child to the parent, for
example, when the child is suspending.

Johan
  

On 5/17/2023 10:07 PM, Johan Hovold wrote:
> On Tue, May 16, 2023 at 07:49:14AM +0530, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 5/16/2023 3:57 AM, Bjorn Andersson wrote:
>>> On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote:
> 
>>>> -#define PWR_EVNT_IRQ_STAT_REG			0x58
>>>> +#define PWR_EVNT_IRQ1_STAT_REG			0x58
>>>> +#define PWR_EVNT_IRQ2_STAT_REG			0x1dc
>>>> +#define PWR_EVNT_IRQ3_STAT_REG			0x228
>>>> +#define PWR_EVNT_IRQ4_STAT_REG			0x238
>>>>    #define PWR_EVNT_LPM_IN_L2_MASK			BIT(4)
>>>>    #define PWR_EVNT_LPM_OUT_L2_MASK		BIT(5)
>>>>    
>>>> @@ -93,6 +96,13 @@ struct dwc3_qcom {
>>>>    	struct icc_path		*icc_path_apps;
>>>>    };
>>>>    
>>>> +static u32 pwr_evnt_irq_stat_reg_offset[4] = {
>>>> +			PWR_EVNT_IRQ1_STAT_REG,
>>>> +			PWR_EVNT_IRQ2_STAT_REG,
>>>> +			PWR_EVNT_IRQ3_STAT_REG,
>>>> +			PWR_EVNT_IRQ4_STAT_REG,
>>>
>>> Seems to be excessive indentation of these...
>>>
>>> Can you also please confirm that these should be counted starting at 1 -
>>> given that you otherwise talk about port0..N-1?
> 
>>     I am fine with either way. Since this just denoted 4 different ports,
>> I named them starting with 1. Either ways, we will run through array
>> from (0-3), so we must be fine.
> 
> Actually, the USB ports are indexed from 1, so the above naming may or
> may not be correct depending on how they are defined.
> 
Ok, will rename them as PWR_EVNT_IRQx_STAT_REG  (x = 0,1,2,3)

>>>> +};
>>>> +
>>>>    static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
>>>>    {
>>>>    	u32 reg;
>>>> @@ -413,13 +423,16 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
>>>>    {
>>>>    	u32 val;
>>>>    	int i, ret;
>>>> +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
>>>>    
>>>>    	if (qcom->is_suspended)
>>>>    		return 0;
>>>>    
>>>> -	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
>>>> -	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
>>>> -		dev_err(qcom->dev, "HS-PHY not in L2\n");
>>>> +	for (i = 0; i < dwc->num_usb2_ports; i++) {
>>>
>>> In the event that the dwc3 core fails to acquire or enable e.g. clocks
>>> its drvdata will be NULL. If you then hit a runtime pm transition in the
>>> dwc3-qcom glue you will dereference NULL here. (You can force this issue
>>> by e.g. returning -EINVAL from dwc3_clk_enable()).
>>>
>>> So if you're peaking into qcom->dwc3 you need to handle the fact that
>>> dwc might be NULL, here and in resume below.
>>>
>> Thanks for catching this. You are right, there were instances where the
>> we saw probe for dwc3 being deferred while the probe for dwc3-qcom was
>> still successful [1]. In this case, if the dwc3 probe never happened and
>> system tries to enter suspend, we might hit a NULL pointer dereference.
> 
> I don't think we should be adding more of these layering violations. A
> parent device driver has no business messing with the driver data for a
> child device which may or may not even have probed yet.
> 
> I added a FIXME elsewhere in the driver about fixing up the current
> instances that have already snuck in (which in some sense is even worse
> by accessing driver data of a grandchild device).
> 
> We really need to try sort this mess out and how to properly handle the
> interactions between these layers (e.g. glue, dwc3 core and xhci). This
> will likely involve adding callbacks from the child to the parent, for
> example, when the child is suspending.
> 

Hi Johan,

  I agree with you, but in this case I believe there is no other way we 
can find the number of ports present. Unless its a dt property which 
parent driver can access the child's of node and get the details. Like 
done in v4 [1]. But it would be adding redundant data into DT as pointed 
out by Rob and Krzysztof and so we removed these properties.

Also, since this is a read only operation being done and no 
modifications are being done to driver data of child, is it still not 
acceptable as both dwc3-qcom and core are tightly coupled entities.

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

Regards,
Krishna,
  

On Mon, May 15, 2023 at 03:27:30PM -0700, Bjorn Andersson wrote:
> On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote:
> > QCOM SoC SA8295P's tertiary quad port controller supports 2 HS+SS
> > ports and 2 HS only ports. Add support for configuring PWR_EVENT_IRQ's
> > for all the ports during suspend/resume.
> > 
> > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > ---
> >  drivers/usb/dwc3/dwc3-qcom.c | 28 ++++++++++++++++++++++------
> >  1 file changed, 22 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > index 959fc925ca7c..7a9bce66295d 100644
> > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > @@ -37,7 +37,10 @@
> >  #define PIPE3_PHYSTATUS_SW			BIT(3)
> >  #define PIPE_UTMI_CLK_DIS			BIT(8)
> >  
> > -#define PWR_EVNT_IRQ_STAT_REG			0x58
> > +#define PWR_EVNT_IRQ1_STAT_REG			0x58
> > +#define PWR_EVNT_IRQ2_STAT_REG			0x1dc
> > +#define PWR_EVNT_IRQ3_STAT_REG			0x228
> > +#define PWR_EVNT_IRQ4_STAT_REG			0x238
> >  #define PWR_EVNT_LPM_IN_L2_MASK			BIT(4)
> >  #define PWR_EVNT_LPM_OUT_L2_MASK		BIT(5)
> >  
> > @@ -93,6 +96,13 @@ struct dwc3_qcom {
> >  	struct icc_path		*icc_path_apps;
> >  };
> >  
> > +static u32 pwr_evnt_irq_stat_reg_offset[4] = {
> > +			PWR_EVNT_IRQ1_STAT_REG,
> > +			PWR_EVNT_IRQ2_STAT_REG,
> > +			PWR_EVNT_IRQ3_STAT_REG,
> > +			PWR_EVNT_IRQ4_STAT_REG,
> 
> Seems to be excessive indentation of these...
> 
> Can you also please confirm that these should be counted starting at 1 -
> given that you otherwise talk about port0..N-1?
> 
> > +};
> > +
> >  static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
> >  {
> >  	u32 reg;
> > @@ -413,13 +423,16 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
> >  {
> >  	u32 val;
> >  	int i, ret;
> > +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> >  
> >  	if (qcom->is_suspended)
> >  		return 0;
> >  
> > -	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
> > -	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
> > -		dev_err(qcom->dev, "HS-PHY not in L2\n");
> > +	for (i = 0; i < dwc->num_usb2_ports; i++) {
> 
> In the event that the dwc3 core fails to acquire or enable e.g. clocks
> its drvdata will be NULL. If you then hit a runtime pm transition in the
> dwc3-qcom glue you will dereference NULL here. (You can force this issue
> by e.g. returning -EINVAL from dwc3_clk_enable()).
> 

I looked at this once more, and realized that I missed the fact that
dwc3_qcom_is_host() will happily dereference the drvdata() just a few
lines further down...

So this is already broken.

> So if you're peaking into qcom->dwc3 you need to handle the fact that
> dwc might be NULL, here and in resume below.
> 

We need to fix the dwc3 glue design, so that the glue and the core can
cooperate - and we have a few other use cases where this is needed (e.g.
usb_role_switch propagation to the glue code).

Regards,
Bjorn

> Regards,
> Bjorn
  

On 5/26/2023 8:25 AM, Bjorn Andersson wrote:
>>> +
>>>   static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
>>>   {
>>>   	u32 reg;
>>> @@ -413,13 +423,16 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
>>>   {
>>>   	u32 val;
>>>   	int i, ret;
>>> +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
>>>   
>>>   	if (qcom->is_suspended)
>>>   		return 0;
>>>   
>>> -	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
>>> -	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
>>> -		dev_err(qcom->dev, "HS-PHY not in L2\n");
>>> +	for (i = 0; i < dwc->num_usb2_ports; i++) {
>>
>> In the event that the dwc3 core fails to acquire or enable e.g. clocks
>> its drvdata will be NULL. If you then hit a runtime pm transition in the
>> dwc3-qcom glue you will dereference NULL here. (You can force this issue
>> by e.g. returning -EINVAL from dwc3_clk_enable()).
>>
> 
> I looked at this once more, and realized that I missed the fact that
> dwc3_qcom_is_host() will happily dereference the drvdata() just a few
> lines further down...
> 
> So this is already broken.
> 
>> So if you're peaking into qcom->dwc3 you need to handle the fact that
>> dwc might be NULL, here and in resume below.
>>
> 
> We need to fix the dwc3 glue design, so that the glue and the core can
> cooperate - and we have a few other use cases where this is needed (e.g.
> usb_role_switch propagation to the glue code).
> 
Hi Bjorn, Johan,

   Thanks for the comments on this patch. I had some suggestions come in 
from the team internally:

1. To use the notifier call available in drivers/usb/core/notify.c and 
make sure that host mode is enabled. That way we can access dwc or xhci 
without any issue.

2. For this particular case where we are trying to get info on number of 
ports present (dwc->num_usb2_ports), we can add compatible data for 
sc8280-mp and provide input to driver telling num ports is 4.

For the first idea, I tried it out as follows:

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 959fc925ca7c..ce2f867d7c9a 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -91,6 +91,9 @@ struct dwc3_qcom {
         bool                    pm_suspended;
         struct icc_path         *icc_path_ddr;
         struct icc_path         *icc_path_apps;
+
+       bool                    in_host_mode;
+       struct notifier_block   xhci_nb;
  };

  static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, 
u32 val)
@@ -305,14 +308,6 @@ static void dwc3_qcom_interconnect_exit(struct 
dwc3_qcom *qcom)
         icc_put(qcom->icc_path_apps);
  }

-/* Only usable in contexts where the role can not change. */
-static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
-{
-       struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
-
-       return dwc->xhci;
-}
-
  static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct 
dwc3_qcom *qcom)
  {
         struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
@@ -432,7 +427,7 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, 
bool wakeup)
          * The role is stable during suspend as role switching is done 
from a
          * freezable workqueue.
          */
-       if (dwc3_qcom_is_host(qcom) && wakeup) {
+       if (qcom->in_host_mode && wakeup) {
                 qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom);
                 dwc3_qcom_enable_interrupts(qcom);
         }
@@ -450,7 +445,7 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, 
bool wakeup)
         if (!qcom->is_suspended)
                 return 0;

-       if (dwc3_qcom_is_host(qcom) && wakeup)
+       if (qcom->in_host_mode && wakeup)
                 dwc3_qcom_disable_interrupts(qcom);

         for (i = 0; i < qcom->num_clocks; i++) {
@@ -488,7 +483,7 @@ static irqreturn_t qcom_dwc3_resume_irq(int irq, 
void *data)
          * This is safe as role switching is done from a freezable 
workqueue
          * and the wakeup interrupts are disabled as part of resume.
          */
-       if (dwc3_qcom_is_host(qcom))
+       if (qcom->in_host_mode)
                 pm_runtime_resume(&dwc->xhci->dev);

         return IRQ_HANDLED;
@@ -785,6 +780,41 @@ dwc3_qcom_create_urs_usb_platdev(struct device *dev)
         return acpi_create_platform_device(adev, NULL);
  }

+static int dwc3_xhci_event_notifier(struct notifier_block *nb,
+                               unsigned long event, void *ptr)
+{
+       struct dwc3_qcom *qcom = container_of(nb, struct dwc3_qcom, 
xhci_nb);
+       struct usb_bus *ubus = ptr;
+       struct usb_hcd *hcd;
+
+       if (event != USB_BUS_ADD && event != USB_BUS_REMOVE)
+               return NOTIFY_DONE;
+
+       //TODO: Check whether event generated is for this qcom 
controller or not
+
+       hcd = bus_to_hcd(ubus);
+       if (event == USB_BUS_ADD) {
+               /*
+                * Assuming both usb2 and usb3 roothubs wil
+                * be registered, wait for shared hcd to be
+                * registered to ensure that we are in host mode
+                * completely.
+                */
+               if (!usb_hcd_is_primary_hcd(hcd))
+                       qcom->in_host_mode = true;
+       } else if (event == USB_BUS_REMOVE) {
+               /*
+                * While exiting host mode, primary hcd is
+                * removed at the end. Use it as indication
+                * that host stack has been removed successfully.
+                */
+               if (usb_hcd_is_primary_hcd(hcd))
+                       qcom->in_host_mode = false;
+       }
+
+       return 0;
+}
+
  static int dwc3_qcom_probe(struct platform_device *pdev)
  {
         struct device_node      *np = pdev->dev.of_node;
@@ -884,6 +914,9 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
         if (ignore_pipe_clk)
                 dwc3_qcom_select_utmi_clk(qcom);

+       qcom->xhci_nb.notifier_call = dwc3_xhci_event_notifier;
+       usb_register_notify(&qcom->xhci_nb);
+
         if (np)
                 ret = dwc3_qcom_of_register_core(pdev);
         else
@@ -923,6 +956,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
  interconnect_exit:
         dwc3_qcom_interconnect_exit(qcom);
  depopulate:
+       usb_unregister_notify(&qcom->xhci_nb);
         if (np)
                 of_platform_depopulate(&pdev->dev);


I tested the patch on sc7280 and see that wakeup from system suspend 
works fine:

[    3.807449] K: Before notify register
[    3.810774] K: After notify register
[    3.814006] K: calling dwc3_qcom_of_register_core
[    3.818467] dwc3 a600000.usb: Adding to iommu group 8
[    3.840031] K: before plat adde
[    3.849151] K: before add hcd
[    3.852219] xhci-hcd xhci-hcd.12.auto: xHCI Host Controller
[    3.857956] K: usb_notify_add_bus: Before notify add bus event: 3
[    3.866481] K: dwc3_xhci_event_notifier recevied event: 3
[    3.874007] K: dwc3_xhci_event_notifier: its a bus add/remove event
[    3.880451] K: dwc3_xhci_event_notifier hcd added
[    3.885301] K: in host mode: 0
[    3.888441] K: usb_notify_add_bus: After notify add bus
[    3.893815] xhci-hcd xhci-hcd.12.auto: new USB bus registered, 
assigned bus number 1
[    3.903799] xhci-hcd xhci-hcd.12.auto: hcc params 0x0230fe65 hci 
version 0x110 quirks 0x0000000000010010
[    3.913564] xhci-hcd xhci-hcd.12.auto: irq 214, io mem 0x0a600000
[    3.919938] K: after add hcd
[    3.922913] K: before add shared hcd
[    3.926589] xhci-hcd xhci-hcd.12.auto: xHCI Host Controller
[    3.932327] K: usb_notify_add_bus: Before notify add bus event: 3
[    3.940973] K: dwc3_xhci_event_notifier recevied event: 3
[    3.948492] K: dwc3_xhci_event_notifier: its a bus add/remove event
[    3.954936] K: dwc3_xhci_event_notifier hcd added
[    3.959770] K: dwc3_xhci_event_notifier: Its shared hcd
[    3.965143] K: in host mode: 1
[    3.968283] K: usb_notify_add_bus: After notify add bus
[    3.973675] xhci-hcd xhci-hcd.12.auto: new USB bus registered, 
assigned bus number 2
[    3.981645] xhci-hcd xhci-hcd.12.auto: Host supports USB 3.0 SuperSpeed
[    3.988537] usb usb1: New USB device found, idVendor=1d6b, 
idProduct=0002, bcdDevice= 5.15
[    3.997024] usb usb1: New USB device strings: Mfr=3, Product=2, 
SerialNumber=1
[    4.004438] usb usb1: Product: xHCI Host Controller
[    4.009459] usb usb1: Manufacturer: Linux 
5.15.90-25532-g92932f8e612f-dirty xhci-hcd
[    4.017420] usb usb1: SerialNumber: xhci-hcd.12.auto

.....


[   90.809188] Resume caused by IRQ 211, qcom_dwc3 DP_HS

Attached the patch file as well as in mail.
Let me know if this is a good enough way to fix the layering violations.

Regards,
Krishna,
From 3a318ab38c4959b21df3cce6b933b6d0abe5eb4b Mon Sep 17 00:00:00 2001
From: Krishna Kurapati <quic_kriskura@quicinc.com>
Date: Fri, 26 May 2023 15:03:07 +0530
Subject: [PATCH] usb: dwc3: qcom: Cleanup layering violations in dwc3 qcom

Implement host notifier in qcom to remove layering violations

Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 drivers/usb/dwc3/dwc3-qcom.c | 56 +++++++++++++++++++++++++++++-------
 1 file changed, 45 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 959fc925ca7c..ce2f867d7c9a 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -91,6 +91,9 @@ struct dwc3_qcom {
 	bool			pm_suspended;
 	struct icc_path		*icc_path_ddr;
 	struct icc_path		*icc_path_apps;
+
+	bool			in_host_mode;
+	struct notifier_block	xhci_nb;
 };
 
 static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
@@ -305,14 +308,6 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
 	icc_put(qcom->icc_path_apps);
 }
 
-/* Only usable in contexts where the role can not change. */
-static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
-{
-	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
-
-	return dwc->xhci;
-}
-
 static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
 {
 	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
@@ -432,7 +427,7 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
 	 * The role is stable during suspend as role switching is done from a
 	 * freezable workqueue.
 	 */
-	if (dwc3_qcom_is_host(qcom) && wakeup) {
+	if (qcom->in_host_mode && wakeup) {
 		qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom);
 		dwc3_qcom_enable_interrupts(qcom);
 	}
@@ -450,7 +445,7 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
 	if (!qcom->is_suspended)
 		return 0;
 
-	if (dwc3_qcom_is_host(qcom) && wakeup)
+	if (qcom->in_host_mode && wakeup)
 		dwc3_qcom_disable_interrupts(qcom);
 
 	for (i = 0; i < qcom->num_clocks; i++) {
@@ -488,7 +483,7 @@ static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
 	 * This is safe as role switching is done from a freezable workqueue
 	 * and the wakeup interrupts are disabled as part of resume.
 	 */
-	if (dwc3_qcom_is_host(qcom))
+	if (qcom->in_host_mode)
 		pm_runtime_resume(&dwc->xhci->dev);
 
 	return IRQ_HANDLED;
@@ -785,6 +780,41 @@ dwc3_qcom_create_urs_usb_platdev(struct device *dev)
 	return acpi_create_platform_device(adev, NULL);
 }
 
+static int dwc3_xhci_event_notifier(struct notifier_block *nb,
+				unsigned long event, void *ptr)
+{
+	struct dwc3_qcom *qcom = container_of(nb, struct dwc3_qcom, xhci_nb);
+	struct usb_bus *ubus = ptr;
+	struct usb_hcd *hcd;
+
+	if (event != USB_BUS_ADD && event != USB_BUS_REMOVE)
+		return NOTIFY_DONE;
+
+	//TODO: Check whether event generated is for this qcom controller or not
+
+	hcd = bus_to_hcd(ubus);
+	if (event == USB_BUS_ADD) {
+		/*
+		 * Assuming both usb2 and usb3 roothubs wil
+		 * be registered, wait for shared hcd to be
+		 * registered to ensure that we are in host mode
+		 * completely.
+		 */
+		if (!usb_hcd_is_primary_hcd(hcd))
+			qcom->in_host_mode = true;
+	} else if (event == USB_BUS_REMOVE) {
+		/*
+		 * While exiting host mode, primary hcd is
+		 * removed at the end. Use it as indication
+		 * that host stack has been removed successfully.
+		 */
+		if (usb_hcd_is_primary_hcd(hcd))
+			qcom->in_host_mode = false;
+	}
+
+	return 0;
+}
+
 static int dwc3_qcom_probe(struct platform_device *pdev)
 {
 	struct device_node	*np = pdev->dev.of_node;
@@ -884,6 +914,9 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	if (ignore_pipe_clk)
 		dwc3_qcom_select_utmi_clk(qcom);
 
+	qcom->xhci_nb.notifier_call = dwc3_xhci_event_notifier;
+	usb_register_notify(&qcom->xhci_nb);
+
 	if (np)
 		ret = dwc3_qcom_of_register_core(pdev);
 	else
@@ -923,6 +956,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 interconnect_exit:
 	dwc3_qcom_interconnect_exit(qcom);
 depopulate:
+	usb_unregister_notify(&qcom->xhci_nb);
 	if (np)
 		of_platform_depopulate(&pdev->dev);
 	else
  

Hi Krishna,

and sorry about the delay in following up on this. As usual, there are
just way too many threads and this one in particular requires a bit of
thought.

On Sat, May 20, 2023 at 11:18:52PM +0530, Krishna Kurapati PSSNV wrote:
> On 5/17/2023 10:07 PM, Johan Hovold wrote:

> > I don't think we should be adding more of these layering violations. A
> > parent device driver has no business messing with the driver data for a
> > child device which may or may not even have probed yet.
> > 
> > I added a FIXME elsewhere in the driver about fixing up the current
> > instances that have already snuck in (which in some sense is even worse
> > by accessing driver data of a grandchild device).
> > 
> > We really need to try sort this mess out and how to properly handle the
> > interactions between these layers (e.g. glue, dwc3 core and xhci). This
> > will likely involve adding callbacks from the child to the parent, for
> > example, when the child is suspending.

>   I agree with you, but in this case I believe there is no other way we 
> can find the number of ports present. Unless its a dt property which 
> parent driver can access the child's of node and get the details. Like 
> done in v4 [1]. But it would be adding redundant data into DT as pointed 
> out by Rob and Krzysztof and so we removed these properties.

So there at least two issues with this series:

	1. accessing xhci registers from the dwc3 core
	2. accessing driver data of a child device

1. The first part about accessing xhci registers goes against the clear
separation between glue, core and xhci that Felipe tried to maintain.

I'm not entirely against doing this from the core driver before
registering the xhci platform device as the registers are unmapped
afterwards. But if this is to be allowed, then the implementation should
be shared with xhci rather than copied verbatim.

The alternative that avoids this issue entirely could indeed be to
simply count the number of PHYs described in DT as Rob initially
suggested. Why would that not work?

2. The driver is already accessing driver data of the child device so
arguably your series is not really making things much worse than they
already are.

I've just sent a couple of fixes to address some of the symptoms of
this layering violation here:

	https://lore.kernel.org/lkml/20230607100540.31045-1-johan+linaro@kernel.org/

Getting this fixed properly is going to take a bit of work, and
depending on how your multiport wake up implementation is going to look
like, adding support for multiport controllers may not make this much
harder to address.

So perhaps we can indeed merge support for multiport and then get back
to cleaning this up.

Johan
  

On Fri, May 26, 2023 at 08:55:22PM +0530, Krishna Kurapati PSSNV wrote:
> On 5/26/2023 8:25 AM, Bjorn Andersson wrote:

> > We need to fix the dwc3 glue design, so that the glue and the core can
> > cooperate - and we have a few other use cases where this is needed (e.g.
> > usb_role_switch propagation to the glue code).

>    Thanks for the comments on this patch. I had some suggestions come in 
> from the team internally:
> 
> 1. To use the notifier call available in drivers/usb/core/notify.c and 
> make sure that host mode is enabled. That way we can access dwc or xhci 
> without any issue.

I don't think this is a good idea and instead the callbacks should be
dedicated for the xhci and dwc3 drivers. A struct with callbacks can be
passed down to the child devices, which call back into the drivers of
their parents for notifications and when they need services from them
(e.g. during suspend or on role changes).

> 2. For this particular case where we are trying to get info on number of 
> ports present (dwc->num_usb2_ports), we can add compatible data for 
> sc8280-mp and provide input to driver telling num ports is 4.

That may also work as a way to avoid parsing the xhci registers, but I'm
still not sure why simply counting the PHYs in DT would not work.

Johan
  

On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote:
> QCOM SoC SA8295P's tertiary quad port controller supports 2 HS+SS
> ports and 2 HS only ports. Add support for configuring PWR_EVENT_IRQ's
> for all the ports during suspend/resume.
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 959fc925ca7c..7a9bce66295d 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -37,7 +37,10 @@
>  #define PIPE3_PHYSTATUS_SW			BIT(3)
>  #define PIPE_UTMI_CLK_DIS			BIT(8)
>  
> -#define PWR_EVNT_IRQ_STAT_REG			0x58
> +#define PWR_EVNT_IRQ1_STAT_REG			0x58
> +#define PWR_EVNT_IRQ2_STAT_REG			0x1dc
> +#define PWR_EVNT_IRQ3_STAT_REG			0x228
> +#define PWR_EVNT_IRQ4_STAT_REG			0x238
>  #define PWR_EVNT_LPM_IN_L2_MASK			BIT(4)
>  #define PWR_EVNT_LPM_OUT_L2_MASK		BIT(5)
>  
> @@ -93,6 +96,13 @@ struct dwc3_qcom {
>  	struct icc_path		*icc_path_apps;
>  };
>  
> +static u32 pwr_evnt_irq_stat_reg_offset[4] = {
> +			PWR_EVNT_IRQ1_STAT_REG,
> +			PWR_EVNT_IRQ2_STAT_REG,
> +			PWR_EVNT_IRQ3_STAT_REG,
> +			PWR_EVNT_IRQ4_STAT_REG,
> +};

Indentation is off, as I believe Bjorn pointed out.

>  static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
>  {
>  	u32 reg;
> @@ -413,13 +423,16 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
>  {
>  	u32 val;
>  	int i, ret;
> +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
>  
>  	if (qcom->is_suspended)
>  		return 0;
>  
> -	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
> -	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
> -		dev_err(qcom->dev, "HS-PHY not in L2\n");
> +	for (i = 0; i < dwc->num_usb2_ports; i++) {
> +		val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg_offset[i]);
> +		if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
> +			dev_err(qcom->dev, "HS-PHY%d not in L2\n", i);
> +	}

You need check for NULL dwc as we just discussed and skip the above
check if core has not probed yet.

When testing this on the X13s I get:

	dwc3-qcom a4f8800.usb: HS-PHY2 not in L2

for the third port, whose status registers always seems to return zero
(e.g. as if we're checking the wrong register?):

dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 0, pwr_event_stat = 38103c
dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 1, pwr_event_stat = 38103c
dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 2, pwr_event_stat = 00
dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 3, pwr_event_stat = 140030

I verified that everything appears to work as expected on sa8295p-adp.

Do you have any idea of what may be causing this?

Johan
  

On 6/7/2023 5:07 PM, Johan Hovold wrote:
> Hi Krishna,
> 
> and sorry about the delay in following up on this. As usual, there are
> just way too many threads and this one in particular requires a bit of
> thought.
> 
Hi Johan,

   Thanks for taking the time out and reviewing the patches again.

> On Sat, May 20, 2023 at 11:18:52PM +0530, Krishna Kurapati PSSNV wrote:
>> On 5/17/2023 10:07 PM, Johan Hovold wrote:
> 
>>> I don't think we should be adding more of these layering violations. A
>>> parent device driver has no business messing with the driver data for a
>>> child device which may or may not even have probed yet.
>>>
>>> I added a FIXME elsewhere in the driver about fixing up the current
>>> instances that have already snuck in (which in some sense is even worse
>>> by accessing driver data of a grandchild device).
>>>
>>> We really need to try sort this mess out and how to properly handle the
>>> interactions between these layers (e.g. glue, dwc3 core and xhci). This
>>> will likely involve adding callbacks from the child to the parent, for
>>> example, when the child is suspending.
> 
>>    I agree with you, but in this case I believe there is no other way we
>> can find the number of ports present. Unless its a dt property which
>> parent driver can access the child's of node and get the details. Like
>> done in v4 [1]. But it would be adding redundant data into DT as pointed
>> out by Rob and Krzysztof and so we removed these properties.
> 
> So there at least two issues with this series:
> 
> 	1. accessing xhci registers from the dwc3 core
> 	2. accessing driver data of a child device
> 
> 1. The first part about accessing xhci registers goes against the clear
> separation between glue, core and xhci that Felipe tried to maintain.
> 
> I'm not entirely against doing this from the core driver before
> registering the xhci platform device as the registers are unmapped
> afterwards. But if this is to be allowed, then the implementation should
> be shared with xhci rather than copied verbatim.
> 
> The alternative that avoids this issue entirely could indeed be to
> simply count the number of PHYs described in DT as Rob initially
> suggested. Why would that not work?
> 
The reason why I didn't want to read the Phy's from DT is explained in 
[1]. I felt it makes the code unreadable and its very tricky to read the 
phy's properly, so we decided we would initialize phy's for all ports 
and if a phy is missing in DT, the corresponding member in 
dwc->usbX_generic_phy[] would be NULL and any phy op on it would be a NOP.

Also as per Krzysztof suggestion on [2], we can add a compatible to read 
number of phy's / ports present. This avoids accessing xhci members 
atleast in driver core. But the layering violations would still be present.

> 2. The driver is already accessing driver data of the child device so
> arguably your series is not really making things much worse than they
> already are.
> 
> I've just sent a couple of fixes to address some of the symptoms of
> this layering violation here:
> 
> 	https://lore.kernel.org/lkml/20230607100540.31045-1-johan+linaro@kernel.org/
>

  As you pointed out offline to me that using xhci event notifiers I 
mentioned on [3], if it is not acceptable to use them as notifications 
to check whether we are in host mode, I believe the only way would be to 
use the patches you pushed in.

> Getting this fixed properly is going to take a bit of work, and
> depending on how your multiport wake up implementation is going to look
> like, adding support for multiport controllers may not make this much
> harder to address.
> 
> So perhaps we can indeed merge support for multiport and then get back
> to cleaning this up.
So, you are referring to use the patches you pushed today as a partial 
way to cleanup layering violations and merge multiport on top of it ? If 
so, I am fine with it. I can rebase my changes on top of them.

[1]: 
https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/
[2]: 
https://lore.kernel.org/all/ca729f62-672e-d3de-4069-e2205c97e7d8@linaro.org/
[3]: 
https://lore.kernel.org/all/37fd026e-ecb1-3584-19f3-f8c1e5a9d20a@quicinc.com/

Regards,
Krishna,
  

On 6/7/2023 5:14 PM, Johan Hovold wrote:
> On Fri, May 26, 2023 at 08:55:22PM +0530, Krishna Kurapati PSSNV wrote:
>> On 5/26/2023 8:25 AM, Bjorn Andersson wrote:
> 
>>> We need to fix the dwc3 glue design, so that the glue and the core can
>>> cooperate - and we have a few other use cases where this is needed (e.g.
>>> usb_role_switch propagation to the glue code).
> 
>>     Thanks for the comments on this patch. I had some suggestions come in
>> from the team internally:
>>
>> 1. To use the notifier call available in drivers/usb/core/notify.c and
>> make sure that host mode is enabled. That way we can access dwc or xhci
>> without any issue.
> 
> I don't think this is a good idea and instead the callbacks should be
> dedicated for the xhci and dwc3 drivers. A struct with callbacks can be
> passed down to the child devices, which call back into the drivers of
> their parents for notifications and when they need services from them
> (e.g. during suspend or on role changes).
> 
Hi Johan,

   While I agree with you that these notifications are to be used during 
role switch or suspend/resume, there is no restriction on using them for 
checking whether we are in host mode or not. IMO, it would be cleaner as 
we won't be dereferencing dwc driver data at all to check if we are in 
host mode or not.

Regards,
Krishna,

>> 2. For this particular case where we are trying to get info on number of
>> ports present (dwc->num_usb2_ports), we can add compatible data for
>> sc8280-mp and provide input to driver telling num ports is 4.
> 
> That may also work as a way to avoid parsing the xhci registers, but I'm
> still not sure why simply counting the PHYs in DT would not work.
> 
> Johan
  

On Thu, Jun 08, 2023 at 01:21:02AM +0530, Krishna Kurapati PSSNV wrote:
> On 6/7/2023 5:07 PM, Johan Hovold wrote:

> > So there at least two issues with this series:
> > 
> > 	1. accessing xhci registers from the dwc3 core
> > 	2. accessing driver data of a child device
> > 
> > 1. The first part about accessing xhci registers goes against the clear
> > separation between glue, core and xhci that Felipe tried to maintain.
> > 
> > I'm not entirely against doing this from the core driver before
> > registering the xhci platform device as the registers are unmapped
> > afterwards. But if this is to be allowed, then the implementation should
> > be shared with xhci rather than copied verbatim.
> > 
> > The alternative that avoids this issue entirely could indeed be to
> > simply count the number of PHYs described in DT as Rob initially
> > suggested. Why would that not work?
> > 
> The reason why I didn't want to read the Phy's from DT is explained in 
> [1]. I felt it makes the code unreadable and its very tricky to read the 
> phy's properly, so we decided we would initialize phy's for all ports 
> and if a phy is missing in DT, the corresponding member in 
> dwc->usbX_generic_phy[] would be NULL and any phy op on it would be a NOP.

That doesn't sound too convincing. Can't you just iterate over the PHYs
described in DT and determine the maximum port number used for HS and
SS?
 
> Also as per Krzysztof suggestion on [2], we can add a compatible to read 
> number of phy's / ports present. This avoids accessing xhci members 
> atleast in driver core. But the layering violations would still be present.

Yes, but if the information is already available in DT it's better to use
it rather than re-encode it in the driver.
 
> > 2. The driver is already accessing driver data of the child device so
> > arguably your series is not really making things much worse than they
> > already are.
> > 
> > I've just sent a couple of fixes to address some of the symptoms of
> > this layering violation here:
> > 
> > 	https://lore.kernel.org/lkml/20230607100540.31045-1-johan+linaro@kernel.org/
> >
> 
>   As you pointed out offline to me that using xhci event notifiers I 
> mentioned on [3], if it is not acceptable to use them as notifications 
> to check whether we are in host mode, I believe the only way would be to 
> use the patches you pushed in.

I still think we'll end up using callbacks from the xhci/core into the
glue driver, but dedicated ones rather than using usb_register_notify().

The fixes I posted can work as a stop-gap solution until then.

> > Getting this fixed properly is going to take a bit of work, and
> > depending on how your multiport wake up implementation is going to look
> > like, adding support for multiport controllers may not make this much
> > harder to address.
> > 
> > So perhaps we can indeed merge support for multiport and then get back
> > to cleaning this up.
> So, you are referring to use the patches you pushed today as a partial 
> way to cleanup layering violations and merge multiport on top of it ? If 
> so, I am fine with it. I can rebase my changes on top of them.

Right. A bit depending on how your wakeup implementation looks like, it
seems we can merge multiport support and then address the layering
issues which are already present in the driver.

> [1]: 
> https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/
> [2]: 
> https://lore.kernel.org/all/ca729f62-672e-d3de-4069-e2205c97e7d8@linaro.org/
> [3]: 
> https://lore.kernel.org/all/37fd026e-ecb1-3584-19f3-f8c1e5a9d20a@quicinc.com/

Johan
  

On Thu, Jun 08, 2023 at 01:25:25AM +0530, Krishna Kurapati PSSNV wrote:
> On 6/7/2023 5:14 PM, Johan Hovold wrote:
> > On Fri, May 26, 2023 at 08:55:22PM +0530, Krishna Kurapati PSSNV wrote:
> >> On 5/26/2023 8:25 AM, Bjorn Andersson wrote:
> > 
> >>> We need to fix the dwc3 glue design, so that the glue and the core can
> >>> cooperate - and we have a few other use cases where this is needed (e.g.
> >>> usb_role_switch propagation to the glue code).
> > 
> >>     Thanks for the comments on this patch. I had some suggestions come in
> >> from the team internally:
> >>
> >> 1. To use the notifier call available in drivers/usb/core/notify.c and
> >> make sure that host mode is enabled. That way we can access dwc or xhci
> >> without any issue.
> > 
> > I don't think this is a good idea and instead the callbacks should be
> > dedicated for the xhci and dwc3 drivers. A struct with callbacks can be
> > passed down to the child devices, which call back into the drivers of
> > their parents for notifications and when they need services from them
> > (e.g. during suspend or on role changes).

>    While I agree with you that these notifications are to be used during 
> role switch or suspend/resume, there is no restriction on using them for 
> checking whether we are in host mode or not. IMO, it would be cleaner as 
> we won't be dereferencing dwc driver data at all to check if we are in 
> host mode or not.

I'm not sure I understand what you're saying here. Could you try to
rephrase it?

Johan
  

On 6/8/2023 3:12 PM, Johan Hovold wrote:
> On Thu, Jun 08, 2023 at 01:21:02AM +0530, Krishna Kurapati PSSNV wrote:
>> On 6/7/2023 5:07 PM, Johan Hovold wrote:
> 
>>> So there at least two issues with this series:
>>>
>>> 	1. accessing xhci registers from the dwc3 core
>>> 	2. accessing driver data of a child device
>>>
>>> 1. The first part about accessing xhci registers goes against the clear
>>> separation between glue, core and xhci that Felipe tried to maintain.
>>>
>>> I'm not entirely against doing this from the core driver before
>>> registering the xhci platform device as the registers are unmapped
>>> afterwards. But if this is to be allowed, then the implementation should
>>> be shared with xhci rather than copied verbatim.
>>>
>>> The alternative that avoids this issue entirely could indeed be to
>>> simply count the number of PHYs described in DT as Rob initially
>>> suggested. Why would that not work?
>>>
>> The reason why I didn't want to read the Phy's from DT is explained in
>> [1]. I felt it makes the code unreadable and its very tricky to read the
>> phy's properly, so we decided we would initialize phy's for all ports
>> and if a phy is missing in DT, the corresponding member in
>> dwc->usbX_generic_phy[] would be NULL and any phy op on it would be a NOP.
> 
> That doesn't sound too convincing. Can't you just iterate over the PHYs
> described in DT and determine the maximum port number used for HS and
> SS?
>   
>> Also as per Krzysztof suggestion on [2], we can add a compatible to read
>> number of phy's / ports present. This avoids accessing xhci members
>> atleast in driver core. But the layering violations would still be present.
> 
> Yes, but if the information is already available in DT it's better to use
> it rather than re-encode it in the driver.
>   

Hi Johan,

   Are you suggesting that we just do something like
num_ports = max( highest usb2 portnum, highest usb3 port num)

If so, incase the usb2 phy of quad port controller is missing in DT, we 
would still read num_usb2_ports as 4 but the usb2_generic_phy[1] would 
be NULL and any phy ops would still be NOP. But we would be getting rid 
of reading the xhci registers compeltely in core driver.

Thinh, Bjorn, can you also let us know your views on this.

1. Read:
   num_usb3_ports = highest usb3 port index in DT
   num_usb2_ports = max( highest usb2 port index, num_usb3_ports)

2. Read the same by parsing xhci registers as done in recent versions of 
this series.

Regards,
Krishna,

>>> 2. The driver is already accessing driver data of the child device so
>>> arguably your series is not really making things much worse than they
>>> already are.
>>> >>> I've just sent a couple of fixes to address some of the symptoms of
>>> this layering violation here:
>>>
>>> 	https://lore.kernel.org/lkml/20230607100540.31045-1-johan+linaro@kernel.org/
>>>
>>
>>    As you pointed out offline to me that using xhci event notifiers I
>> mentioned on [3], if it is not acceptable to use them as notifications
>> to check whether we are in host mode, I believe the only way would be to
>> use the patches you pushed in.
> 
> I still think we'll end up using callbacks from the xhci/core into the
> glue driver, but dedicated ones rather than using usb_register_notify().
> 
> The fixes I posted can work as a stop-gap solution until then.
> 
>>> Getting this fixed properly is going to take a bit of work, and
>>> depending on how your multiport wake up implementation is going to look
>>> like, adding support for multiport controllers may not make this much
>>> harder to address.
>>>
>>> So perhaps we can indeed merge support for multiport and then get back
>>> to cleaning this up.
>> So, you are referring to use the patches you pushed today as a partial
>> way to cleanup layering violations and merge multiport on top of it ? If
>> so, I am fine with it. I can rebase my changes on top of them.
> 
> Right. A bit depending on how your wakeup implementation looks like, it
> seems we can merge multiport support and then address the layering
> issues which are already present in the driver.
> 
>> [1]:
>> https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/
>> [2]:
>> https://lore.kernel.org/all/ca729f62-672e-d3de-4069-e2205c97e7d8@linaro.org/
>> [3]:
>> https://lore.kernel.org/all/37fd026e-ecb1-3584-19f3-f8c1e5a9d20a@quicinc.com/
> 
> Johan
  

On Thu, Jun 08, 2023, Krishna Kurapati PSSNV wrote:
> 
> 
> On 6/8/2023 3:12 PM, Johan Hovold wrote:
> > On Thu, Jun 08, 2023 at 01:21:02AM +0530, Krishna Kurapati PSSNV wrote:
> > > On 6/7/2023 5:07 PM, Johan Hovold wrote:
> > 
> > > > So there at least two issues with this series:
> > > > 
> > > > 	1. accessing xhci registers from the dwc3 core
> > > > 	2. accessing driver data of a child device
> > > > 
> > > > 1. The first part about accessing xhci registers goes against the clear
> > > > separation between glue, core and xhci that Felipe tried to maintain.
> > > > 
> > > > I'm not entirely against doing this from the core driver before
> > > > registering the xhci platform device as the registers are unmapped
> > > > afterwards. But if this is to be allowed, then the implementation should
> > > > be shared with xhci rather than copied verbatim.

The core will just be looking at the HW capability registers and
accessing the ports capability. Our programming guide also listed the
host capability registers in its documentation. We're not driving the
xhci controller here. We're initializing some of the core configs base
on its capability.

We're duplicating the logic here and not exactly doing it verbatim.
Let's try not to share the whole xhci header where we should not have
visibility over. Perhaps it makes sense in some other driver, but let's
not do it here.

> > > > 
> > > > The alternative that avoids this issue entirely could indeed be to
> > > > simply count the number of PHYs described in DT as Rob initially
> > > > suggested. Why would that not work?

See below.

> > > > 
> > > The reason why I didn't want to read the Phy's from DT is explained in
> > > [1]. I felt it makes the code unreadable and its very tricky to read the
> > > phy's properly, so we decided we would initialize phy's for all ports
> > > and if a phy is missing in DT, the corresponding member in
> > > dwc->usbX_generic_phy[] would be NULL and any phy op on it would be a NOP.
> > 
> > That doesn't sound too convincing. Can't you just iterate over the PHYs
> > described in DT and determine the maximum port number used for HS and
> > SS?
> > > Also as per Krzysztof suggestion on [2], we can add a compatible to read
> > > number of phy's / ports present. This avoids accessing xhci members
> > > atleast in driver core. But the layering violations would still be present.
> > 
> > Yes, but if the information is already available in DT it's better to use
> > it rather than re-encode it in the driver.
> 
> Hi Johan,
> 
>   Are you suggesting that we just do something like
> num_ports = max( highest usb2 portnum, highest usb3 port num)

Why do we want to do this? This makes num_ports ambiguous. Let's not
sacrifice clarity for some lines of code.

> 
> If so, incase the usb2 phy of quad port controller is missing in DT, we
> would still read num_usb2_ports as 4 but the usb2_generic_phy[1] would be
> NULL and any phy ops would still be NOP. But we would be getting rid of
> reading the xhci registers compeltely in core driver.
> 
> Thinh, Bjorn, can you also let us know your views on this.
> 
> 1. Read:
>   num_usb3_ports = highest usb3 port index in DT
>   num_usb2_ports = max( highest usb2 port index, num_usb3_ports)
> 
> 2. Read the same by parsing xhci registers as done in recent versions of
> this series.
> 

DT is not reliable to get this info. As noted, the DT may skip some
ports and still be fine. However, the driver doesn't know which port
reflects which port config index without the exact port count.

More importantly, the host controller that lives on the PCI bus will not
use DT. This can be useful for some re-configurations if the controller
is a PCI device and that goes through the dwc3 code path.

Thanks,
Thinh
  

On Thu, Jun 08, 2023 at 05:57:23PM +0000, Thinh Nguyen wrote:
> On Thu, Jun 08, 2023, Krishna Kurapati PSSNV wrote:
> > On 6/8/2023 3:12 PM, Johan Hovold wrote:
> > > On Thu, Jun 08, 2023 at 01:21:02AM +0530, Krishna Kurapati PSSNV wrote:
> > > > On 6/7/2023 5:07 PM, Johan Hovold wrote:
> > > 
> > > > > So there at least two issues with this series:
> > > > > 
> > > > > 	1. accessing xhci registers from the dwc3 core
> > > > > 	2. accessing driver data of a child device
> > > > > 
> > > > > 1. The first part about accessing xhci registers goes against the clear
> > > > > separation between glue, core and xhci that Felipe tried to maintain.
> > > > > 
> > > > > I'm not entirely against doing this from the core driver before
> > > > > registering the xhci platform device as the registers are unmapped
> > > > > afterwards. But if this is to be allowed, then the implementation should
> > > > > be shared with xhci rather than copied verbatim.
> 
> The core will just be looking at the HW capability registers and
> accessing the ports capability. Our programming guide also listed the
> host capability registers in its documentation. We're not driving the
> xhci controller here. We're initializing some of the core configs base
> on its capability.
> 
> We're duplicating the logic here and not exactly doing it verbatim.
> Let's try not to share the whole xhci header where we should not have
> visibility over. Perhaps it makes sense in some other driver, but let's
> not do it here.

The patch series even copied the kernel doc verbatim. This is just not
the way things are supposed to be done upstream. We share defines and
implementations all the time, but we should not be making copies of
them.

> > > > > 
> > > > > The alternative that avoids this issue entirely could indeed be to
> > > > > simply count the number of PHYs described in DT as Rob initially
> > > > > suggested. Why would that not work?
> 
> See below.
> 
> > > > > 
> > > > The reason why I didn't want to read the Phy's from DT is explained in
> > > > [1]. I felt it makes the code unreadable and its very tricky to read the
> > > > phy's properly, so we decided we would initialize phy's for all ports
> > > > and if a phy is missing in DT, the corresponding member in
> > > > dwc->usbX_generic_phy[] would be NULL and any phy op on it would be a NOP.
> > > 
> > > That doesn't sound too convincing. Can't you just iterate over the PHYs
> > > described in DT and determine the maximum port number used for HS and
> > > SS?
> > > > Also as per Krzysztof suggestion on [2], we can add a compatible to read
> > > > number of phy's / ports present. This avoids accessing xhci members
> > > > atleast in driver core. But the layering violations would still be present.
> > > 
> > > Yes, but if the information is already available in DT it's better to use
> > > it rather than re-encode it in the driver.

> >   Are you suggesting that we just do something like
> > num_ports = max( highest usb2 portnum, highest usb3 port num)
> 
> Why do we want to do this? This makes num_ports ambiguous. Let's not
> sacrifice clarity for some lines of code.

This is not about lines of code, but avoiding the bad practice of
copying code around and, to some degree, maintaining the separation
between the glue, core, and xhci which Felipe (perhaps mistakingly) has
fought for.

If you just need to know how many PHYs you have in DT so that you can
iterate over that internal array, you can just look at the max index in
DT where the indexes are specified in the first place.

Don't get hung up on the current variable names, those can be renamed to
match the implementation. Call it max_ports or whatever.

> > If so, incase the usb2 phy of quad port controller is missing in DT, we
> > would still read num_usb2_ports as 4 but the usb2_generic_phy[1] would be
> > NULL and any phy ops would still be NOP. But we would be getting rid of
> > reading the xhci registers compeltely in core driver.
> > 
> > Thinh, Bjorn, can you also let us know your views on this.
> > 
> > 1. Read:
> >   num_usb3_ports = highest usb3 port index in DT
> >   num_usb2_ports = max( highest usb2 port index, num_usb3_ports)
> > 
> > 2. Read the same by parsing xhci registers as done in recent versions of
> > this series.
> 
> DT is not reliable to get this info. As noted, the DT may skip some
> ports and still be fine. However, the driver doesn't know which port
> reflects which port config index without the exact port count.

That's not correct. DT provides the port indexes already, for example:

	phy-names = "usb2-port0", "usb3-port0",
		    "usb2-port1", "usb3-port1",
		    "usb2-port2",
		    "usb2-port3";

So if you just need this to iterate over the PHYs all the information
needed is here.

If you need to access ports which do not have a PHY described in DT,
then this is not going to suffice, but I have not seen anyone claim that
that is needed yet.
 
> More importantly, the host controller that lives on the PCI bus will not
> use DT. This can be useful for some re-configurations if the controller
> is a PCI device and that goes through the dwc3 code path.

Ok, this is a bit hand wavy, but if this ever turns out to be needed it
can also be implemented then.

Or just generalise the xhci implementation for parsing these registers
and reuse that from the start. (As a bonus you'd shrink the kernel text
size by getting rid of that iffy inline implementation.)

Johan
  

On Fri, Jun 09, 2023, Johan Hovold wrote:
> On Thu, Jun 08, 2023 at 05:57:23PM +0000, Thinh Nguyen wrote:
> > On Thu, Jun 08, 2023, Krishna Kurapati PSSNV wrote:
> > > On 6/8/2023 3:12 PM, Johan Hovold wrote:
> > > > On Thu, Jun 08, 2023 at 01:21:02AM +0530, Krishna Kurapati PSSNV wrote:
> > > > > On 6/7/2023 5:07 PM, Johan Hovold wrote:
> > > > 
> > > > > > So there at least two issues with this series:
> > > > > > 
> > > > > > 	1. accessing xhci registers from the dwc3 core
> > > > > > 	2. accessing driver data of a child device
> > > > > > 
> > > > > > 1. The first part about accessing xhci registers goes against the clear
> > > > > > separation between glue, core and xhci that Felipe tried to maintain.
> > > > > > 
> > > > > > I'm not entirely against doing this from the core driver before
> > > > > > registering the xhci platform device as the registers are unmapped
> > > > > > afterwards. But if this is to be allowed, then the implementation should
> > > > > > be shared with xhci rather than copied verbatim.
> > 
> > The core will just be looking at the HW capability registers and
> > accessing the ports capability. Our programming guide also listed the
> > host capability registers in its documentation. We're not driving the
> > xhci controller here. We're initializing some of the core configs base
> > on its capability.
> > 
> > We're duplicating the logic here and not exactly doing it verbatim.
> > Let's try not to share the whole xhci header where we should not have
> > visibility over. Perhaps it makes sense in some other driver, but let's
> > not do it here.
> 
> The patch series even copied the kernel doc verbatim. This is just not
> the way things are supposed to be done upstream. We share defines and
> implementations all the time, but we should not be making copies of
> them.

 We had some fixes to the kernel doc as it's incorrect description.
 Perhaps we can fully rewrite the kernel-doc if that what makes it
 better. We can share define implementations if they are meant to be
 shared. However, with the current way xhci header is implemented, it's
 not meant to be shared with dwc3. You agreed that we are violating this
 in some driver, but you're also insistent that we should not duplicate
 the logic to avoid this violation. Perhaps I'm not a maintainer here
 long enough to know some violation is better kept. If sharing the xhci
 header is what it takes to get this through, then fine.

> 
> > > > > > 
> > > > > > The alternative that avoids this issue entirely could indeed be to
> > > > > > simply count the number of PHYs described in DT as Rob initially
> > > > > > suggested. Why would that not work?
> > 
> > See below.
> > 
> > > > > > 
> > > > > The reason why I didn't want to read the Phy's from DT is explained in
> > > > > [1]. I felt it makes the code unreadable and its very tricky to read the
> > > > > phy's properly, so we decided we would initialize phy's for all ports
> > > > > and if a phy is missing in DT, the corresponding member in
> > > > > dwc->usbX_generic_phy[] would be NULL and any phy op on it would be a NOP.
> > > > 
> > > > That doesn't sound too convincing. Can't you just iterate over the PHYs
> > > > described in DT and determine the maximum port number used for HS and
> > > > SS?
> > > > > Also as per Krzysztof suggestion on [2], we can add a compatible to read
> > > > > number of phy's / ports present. This avoids accessing xhci members
> > > > > atleast in driver core. But the layering violations would still be present.
> > > > 
> > > > Yes, but if the information is already available in DT it's better to use
> > > > it rather than re-encode it in the driver.
> 
> > >   Are you suggesting that we just do something like
> > > num_ports = max( highest usb2 portnum, highest usb3 port num)
> > 
> > Why do we want to do this? This makes num_ports ambiguous. Let's not
> > sacrifice clarity for some lines of code.
> 
> This is not about lines of code, but avoiding the bad practice of
> copying code around and, to some degree, maintaining the separation
> between the glue, core, and xhci which Felipe (perhaps mistakingly) has
> fought for.

We're talking about combining num_usb3_ports and num_usb2_ports here,
what does that have to do with layer separation?

> 
> If you just need to know how many PHYs you have in DT so that you can
> iterate over that internal array, you can just look at the max index in
> DT where the indexes are specified in the first place.
> 
> Don't get hung up on the current variable names, those can be renamed to
> match the implementation. Call it max_ports or whatever.

It doesn't matter what variable name is given, it doesn't change the
fact that this "num_ports" or "max_ports" obfuscated usb2 vs usb3 ports
just for this specific implementation. So, don't do that.

> 
> > > If so, incase the usb2 phy of quad port controller is missing in DT, we
> > > would still read num_usb2_ports as 4 but the usb2_generic_phy[1] would be
> > > NULL and any phy ops would still be NOP. But we would be getting rid of
> > > reading the xhci registers compeltely in core driver.
> > > 
> > > Thinh, Bjorn, can you also let us know your views on this.
> > > 
> > > 1. Read:
> > >   num_usb3_ports = highest usb3 port index in DT
> > >   num_usb2_ports = max( highest usb2 port index, num_usb3_ports)
> > > 
> > > 2. Read the same by parsing xhci registers as done in recent versions of
> > > this series.
> > 
> > DT is not reliable to get this info. As noted, the DT may skip some
> > ports and still be fine. However, the driver doesn't know which port
> > reflects which port config index without the exact port count.
> 
> That's not correct. DT provides the port indexes already, for example:
> 
> 	phy-names = "usb2-port0", "usb3-port0",
> 		    "usb2-port1", "usb3-port1",
> 		    "usb2-port2",
> 		    "usb2-port3";
> 
> So if you just need this to iterate over the PHYs all the information
> needed is here.
> 
> If you need to access ports which do not have a PHY described in DT,
> then this is not going to suffice, but I have not seen anyone claim that
> that is needed yet.

Perhaps I misunderstand the conversation. However, there isn't a method
that everyone's agree on yet regarding DT [*]. Perhaps this indicates it
may not be the best approach. You can resume the conversation if you
want to:

[*] https://lore.kernel.org/linux-usb/9671cade-1820-22e1-9db9-5c9836414908@quicinc.com/#t

>  
> > More importantly, the host controller that lives on the PCI bus will not
> > use DT. This can be useful for some re-configurations if the controller
> > is a PCI device and that goes through the dwc3 code path.
> 
> Ok, this is a bit hand wavy, but if this ever turns out to be needed it
> can also be implemented then.

What does hand wavy mean? We have case where it's useful outside of
this, and it would be useful for PCI device too:

https://lore.kernel.org/linux-usb/20230517233218.rjfmvptrexgkpam3@synopsys.com/

> 
> Or just generalise the xhci implementation for parsing these registers
> and reuse that from the start. (As a bonus you'd shrink the kernel text
> size by getting rid of that iffy inline implementation.)
> 

I don't like the iffy inline function either. We changed that here. To
rework the xhci header and define its global header seems a bit
excessive just for dwc3 to get the port capability. Regardless, as I've
said, if we _must_, perhaps we can just import xhci-ext-caps.h instead
of the whole xhci.h.

BR,
Thinh
  

On 6/9/2023 11:46 PM, Thinh Nguyen wrote:
> On Fri, Jun 09, 2023, Johan Hovold wrote:
>> On Thu, Jun 08, 2023 at 05:57:23PM +0000, Thinh Nguyen wrote:
>>> On Thu, Jun 08, 2023, Krishna Kurapati PSSNV wrote:
>>>> On 6/8/2023 3:12 PM, Johan Hovold wrote:
>>>>> On Thu, Jun 08, 2023 at 01:21:02AM +0530, Krishna Kurapati PSSNV wrote:
>>>>>> On 6/7/2023 5:07 PM, Johan Hovold wrote:
>>>>>
>>>>>>> So there at least two issues with this series:
>>>>>>>
>>>>>>> 	1. accessing xhci registers from the dwc3 core
>>>>>>> 	2. accessing driver data of a child device
>>>>>>>
>>>>>>> 1. The first part about accessing xhci registers goes against the clear
>>>>>>> separation between glue, core and xhci that Felipe tried to maintain.
>>>>>>>
>>>>>>> I'm not entirely against doing this from the core driver before
>>>>>>> registering the xhci platform device as the registers are unmapped
>>>>>>> afterwards. But if this is to be allowed, then the implementation should
>>>>>>> be shared with xhci rather than copied verbatim.
>>>
>>> The core will just be looking at the HW capability registers and
>>> accessing the ports capability. Our programming guide also listed the
>>> host capability registers in its documentation. We're not driving the
>>> xhci controller here. We're initializing some of the core configs base
>>> on its capability.
>>>
>>> We're duplicating the logic here and not exactly doing it verbatim.
>>> Let's try not to share the whole xhci header where we should not have
>>> visibility over. Perhaps it makes sense in some other driver, but let's
>>> not do it here.
>>
>> The patch series even copied the kernel doc verbatim. This is just not
>> the way things are supposed to be done upstream. We share defines and
>> implementations all the time, but we should not be making copies of
>> them.
> 
>   We had some fixes to the kernel doc as it's incorrect description.
>   Perhaps we can fully rewrite the kernel-doc if that what makes it
>   better. We can share define implementations if they are meant to be
>   shared. However, with the current way xhci header is implemented, it's
>   not meant to be shared with dwc3. You agreed that we are violating this
>   in some driver, but you're also insistent that we should not duplicate
>   the logic to avoid this violation. Perhaps I'm not a maintainer here
>   long enough to know some violation is better kept. If sharing the xhci
>   header is what it takes to get this through, then fine.
> 
>>
>>>>>>>
>>>>>>> The alternative that avoids this issue entirely could indeed be to
>>>>>>> simply count the number of PHYs described in DT as Rob initially
>>>>>>> suggested. Why would that not work?
>>>
>>> See below.
>>>
>>>>>>>
>>>>>> The reason why I didn't want to read the Phy's from DT is explained in
>>>>>> [1]. I felt it makes the code unreadable and its very tricky to read the
>>>>>> phy's properly, so we decided we would initialize phy's for all ports
>>>>>> and if a phy is missing in DT, the corresponding member in
>>>>>> dwc->usbX_generic_phy[] would be NULL and any phy op on it would be a NOP.
>>>>>
>>>>> That doesn't sound too convincing. Can't you just iterate over the PHYs
>>>>> described in DT and determine the maximum port number used for HS and
>>>>> SS?
>>>>>> Also as per Krzysztof suggestion on [2], we can add a compatible to read
>>>>>> number of phy's / ports present. This avoids accessing xhci members
>>>>>> atleast in driver core. But the layering violations would still be present.
>>>>>
>>>>> Yes, but if the information is already available in DT it's better to use
>>>>> it rather than re-encode it in the driver.
>>
>>>>    Are you suggesting that we just do something like
>>>> num_ports = max( highest usb2 portnum, highest usb3 port num)
>>>
>>> Why do we want to do this? This makes num_ports ambiguous. Let's not
>>> sacrifice clarity for some lines of code.
>>
>> This is not about lines of code, but avoiding the bad practice of
>> copying code around and, to some degree, maintaining the separation
>> between the glue, core, and xhci which Felipe (perhaps mistakingly) has
>> fought for.
> 
> We're talking about combining num_usb3_ports and num_usb2_ports here,
> what does that have to do with layer separation?
> 
>>
>> If you just need to know how many PHYs you have in DT so that you can
>> iterate over that internal array, you can just look at the max index in
>> DT where the indexes are specified in the first place.
>>
>> Don't get hung up on the current variable names, those can be renamed to
>> match the implementation. Call it max_ports or whatever.
> 
> It doesn't matter what variable name is given, it doesn't change the
> fact that this "num_ports" or "max_ports" obfuscated usb2 vs usb3 ports
> just for this specific implementation. So, don't do that.
> 
>>
>>>> If so, incase the usb2 phy of quad port controller is missing in DT, we
>>>> would still read num_usb2_ports as 4 but the usb2_generic_phy[1] would be
>>>> NULL and any phy ops would still be NOP. But we would be getting rid of
>>>> reading the xhci registers compeltely in core driver.
>>>>
>>>> Thinh, Bjorn, can you also let us know your views on this.
>>>>
>>>> 1. Read:
>>>>    num_usb3_ports = highest usb3 port index in DT
>>>>    num_usb2_ports = max( highest usb2 port index, num_usb3_ports)
>>>>
>>>> 2. Read the same by parsing xhci registers as done in recent versions of
>>>> this series.
>>>
>>> DT is not reliable to get this info. As noted, the DT may skip some
>>> ports and still be fine. However, the driver doesn't know which port
>>> reflects which port config index without the exact port count.
>>
>> That's not correct. DT provides the port indexes already, for example:
>>
>> 	phy-names = "usb2-port0", "usb3-port0",
>> 		    "usb2-port1", "usb3-port1",
>> 		    "usb2-port2",
>> 		    "usb2-port3";
>>
>> So if you just need this to iterate over the PHYs all the information
>> needed is here.
>>
>> If you need to access ports which do not have a PHY described in DT,
>> then this is not going to suffice, but I have not seen anyone claim that
>> that is needed yet.
> 
> Perhaps I misunderstand the conversation. However, there isn't a method
> that everyone's agree on yet regarding DT [*]. Perhaps this indicates it
> may not be the best approach. You can resume the conversation if you
> want to:
> 
> [*] https://lore.kernel.org/linux-usb/9671cade-1820-22e1-9db9-5c9836414908@quicinc.com/#t
> 
>>   
>>> More importantly, the host controller that lives on the PCI bus will not
>>> use DT. This can be useful for some re-configurations if the controller
>>> is a PCI device and that goes through the dwc3 code path.
>>
>> Ok, this is a bit hand wavy, but if this ever turns out to be needed it
>> can also be implemented then.
> 
> What does hand wavy mean? We have case where it's useful outside of
> this, and it would be useful for PCI device too:
> 
> https://lore.kernel.org/linux-usb/20230517233218.rjfmvptrexgkpam3@synopsys.com/
> 
>>
>> Or just generalise the xhci implementation for parsing these registers
>> and reuse that from the start. (As a bonus you'd shrink the kernel text
>> size by getting rid of that iffy inline implementation.)
>>
> 
> I don't like the iffy inline function either. We changed that here. To
> rework the xhci header and define its global header seems a bit
> excessive just for dwc3 to get the port capability. Regardless, as I've
> said, if we _must_, perhaps we can just import xhci-ext-caps.h instead
> of the whole xhci.h.

Hi Thinh, Johan,

  How about we add compatible data indicating the number of usb2/usb3 
ports. That way we needn't parse the DT or read xhci registers atleast 
as a temporary solution to unblock other patches. Once this series is 
merged, we can get back to fixing the port count calculation. Does it 
seem fine ?

Regards,
Krishna,
  

On Thu, Jun 15, 2023, Krishna Kurapati PSSNV wrote:
> 
> 
> On 6/9/2023 11:46 PM, Thinh Nguyen wrote:
> > On Fri, Jun 09, 2023, Johan Hovold wrote:
> > > On Thu, Jun 08, 2023 at 05:57:23PM +0000, Thinh Nguyen wrote:
> > > > On Thu, Jun 08, 2023, Krishna Kurapati PSSNV wrote:
> > > > > On 6/8/2023 3:12 PM, Johan Hovold wrote:
> > > > > > On Thu, Jun 08, 2023 at 01:21:02AM +0530, Krishna Kurapati PSSNV wrote:
> > > > > > > On 6/7/2023 5:07 PM, Johan Hovold wrote:
> > > > > > 
> > > > > > > > So there at least two issues with this series:
> > > > > > > > 
> > > > > > > > 	1. accessing xhci registers from the dwc3 core
> > > > > > > > 	2. accessing driver data of a child device
> > > > > > > > 
> > > > > > > > 1. The first part about accessing xhci registers goes against the clear
> > > > > > > > separation between glue, core and xhci that Felipe tried to maintain.
> > > > > > > > 
> > > > > > > > I'm not entirely against doing this from the core driver before
> > > > > > > > registering the xhci platform device as the registers are unmapped
> > > > > > > > afterwards. But if this is to be allowed, then the implementation should
> > > > > > > > be shared with xhci rather than copied verbatim.
> > > > 
> > > > The core will just be looking at the HW capability registers and
> > > > accessing the ports capability. Our programming guide also listed the
> > > > host capability registers in its documentation. We're not driving the
> > > > xhci controller here. We're initializing some of the core configs base
> > > > on its capability.
> > > > 
> > > > We're duplicating the logic here and not exactly doing it verbatim.
> > > > Let's try not to share the whole xhci header where we should not have
> > > > visibility over. Perhaps it makes sense in some other driver, but let's
> > > > not do it here.
> > > 
> > > The patch series even copied the kernel doc verbatim. This is just not
> > > the way things are supposed to be done upstream. We share defines and
> > > implementations all the time, but we should not be making copies of
> > > them.
> > 
> >   We had some fixes to the kernel doc as it's incorrect description.
> >   Perhaps we can fully rewrite the kernel-doc if that what makes it
> >   better. We can share define implementations if they are meant to be
> >   shared. However, with the current way xhci header is implemented, it's
> >   not meant to be shared with dwc3. You agreed that we are violating this
> >   in some driver, but you're also insistent that we should not duplicate
> >   the logic to avoid this violation. Perhaps I'm not a maintainer here
> >   long enough to know some violation is better kept. If sharing the xhci
> >   header is what it takes to get this through, then fine.
> > 
> > > 
> > > > > > > > 
> > > > > > > > The alternative that avoids this issue entirely could indeed be to
> > > > > > > > simply count the number of PHYs described in DT as Rob initially
> > > > > > > > suggested. Why would that not work?
> > > > 
> > > > See below.
> > > > 
> > > > > > > > 
> > > > > > > The reason why I didn't want to read the Phy's from DT is explained in
> > > > > > > [1]. I felt it makes the code unreadable and its very tricky to read the
> > > > > > > phy's properly, so we decided we would initialize phy's for all ports
> > > > > > > and if a phy is missing in DT, the corresponding member in
> > > > > > > dwc->usbX_generic_phy[] would be NULL and any phy op on it would be a NOP.
> > > > > > 
> > > > > > That doesn't sound too convincing. Can't you just iterate over the PHYs
> > > > > > described in DT and determine the maximum port number used for HS and
> > > > > > SS?
> > > > > > > Also as per Krzysztof suggestion on [2], we can add a compatible to read
> > > > > > > number of phy's / ports present. This avoids accessing xhci members
> > > > > > > atleast in driver core. But the layering violations would still be present.
> > > > > > 
> > > > > > Yes, but if the information is already available in DT it's better to use
> > > > > > it rather than re-encode it in the driver.
> > > 
> > > > >    Are you suggesting that we just do something like
> > > > > num_ports = max( highest usb2 portnum, highest usb3 port num)
> > > > 
> > > > Why do we want to do this? This makes num_ports ambiguous. Let's not
> > > > sacrifice clarity for some lines of code.
> > > 
> > > This is not about lines of code, but avoiding the bad practice of
> > > copying code around and, to some degree, maintaining the separation
> > > between the glue, core, and xhci which Felipe (perhaps mistakingly) has
> > > fought for.
> > 
> > We're talking about combining num_usb3_ports and num_usb2_ports here,
> > what does that have to do with layer separation?
> > 
> > > 
> > > If you just need to know how many PHYs you have in DT so that you can
> > > iterate over that internal array, you can just look at the max index in
> > > DT where the indexes are specified in the first place.
> > > 
> > > Don't get hung up on the current variable names, those can be renamed to
> > > match the implementation. Call it max_ports or whatever.
> > 
> > It doesn't matter what variable name is given, it doesn't change the
> > fact that this "num_ports" or "max_ports" obfuscated usb2 vs usb3 ports
> > just for this specific implementation. So, don't do that.
> > 
> > > 
> > > > > If so, incase the usb2 phy of quad port controller is missing in DT, we
> > > > > would still read num_usb2_ports as 4 but the usb2_generic_phy[1] would be
> > > > > NULL and any phy ops would still be NOP. But we would be getting rid of
> > > > > reading the xhci registers compeltely in core driver.
> > > > > 
> > > > > Thinh, Bjorn, can you also let us know your views on this.
> > > > > 
> > > > > 1. Read:
> > > > >    num_usb3_ports = highest usb3 port index in DT
> > > > >    num_usb2_ports = max( highest usb2 port index, num_usb3_ports)
> > > > > 
> > > > > 2. Read the same by parsing xhci registers as done in recent versions of
> > > > > this series.
> > > > 
> > > > DT is not reliable to get this info. As noted, the DT may skip some
> > > > ports and still be fine. However, the driver doesn't know which port
> > > > reflects which port config index without the exact port count.
> > > 
> > > That's not correct. DT provides the port indexes already, for example:
> > > 
> > > 	phy-names = "usb2-port0", "usb3-port0",
> > > 		    "usb2-port1", "usb3-port1",
> > > 		    "usb2-port2",
> > > 		    "usb2-port3";
> > > 
> > > So if you just need this to iterate over the PHYs all the information
> > > needed is here.
> > > 
> > > If you need to access ports which do not have a PHY described in DT,
> > > then this is not going to suffice, but I have not seen anyone claim that
> > > that is needed yet.
> > 
> > Perhaps I misunderstand the conversation. However, there isn't a method
> > that everyone's agree on yet regarding DT [*]. Perhaps this indicates it
> > may not be the best approach. You can resume the conversation if you
> > want to:
> > 
> > [*] https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/9671cade-1820-22e1-9db9-5c9836414908@quicinc.com/*t__;Iw!!A4F2R9G_pg!YNb76pwkiNunnVGWfpM33LmCTJQNL7zPRooIIygA5rsUzkPGglyrsj5SLCy2raqkqwtjizd5js2wJ_OAP1Pp0N6mN4myMg$
> > 
> > > > More importantly, the host controller that lives on the PCI bus will not
> > > > use DT. This can be useful for some re-configurations if the controller
> > > > is a PCI device and that goes through the dwc3 code path.
> > > 
> > > Ok, this is a bit hand wavy, but if this ever turns out to be needed it
> > > can also be implemented then.
> > 
> > What does hand wavy mean? We have case where it's useful outside of
> > this, and it would be useful for PCI device too:
> > 
> > https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20230517233218.rjfmvptrexgkpam3@synopsys.com/__;!!A4F2R9G_pg!YNb76pwkiNunnVGWfpM33LmCTJQNL7zPRooIIygA5rsUzkPGglyrsj5SLCy2raqkqwtjizd5js2wJ_OAP1Pp0N4CJPF7cQ$
> > 
> > > 
> > > Or just generalise the xhci implementation for parsing these registers
> > > and reuse that from the start. (As a bonus you'd shrink the kernel text
> > > size by getting rid of that iffy inline implementation.)
> > > 
> > 
> > I don't like the iffy inline function either. We changed that here. To
> > rework the xhci header and define its global header seems a bit
> > excessive just for dwc3 to get the port capability. Regardless, as I've
> > said, if we _must_, perhaps we can just import xhci-ext-caps.h instead
> > of the whole xhci.h.
> 
> Hi Thinh, Johan,
> 
>  How about we add compatible data indicating the number of usb2/usb3 ports.
> That way we needn't parse the DT or read xhci registers atleast as a
> temporary solution to unblock other patches. Once this series is merged, we
> can get back to fixing the port count calculation. Does it seem fine ?
> 

Temporary solution should not involve DT as it's not easily reverted or
changed. Just include xhci-ext-caps.h and use the inline function. I
think Johan is fine with that. If not, he can provide more feedback.

Thanks,
Thinh
  

Hi Thinh,

and sorry about the late reply. Was on holiday last week.

On Fri, Jun 09, 2023 at 06:16:13PM +0000, Thinh Nguyen wrote:
> On Fri, Jun 09, 2023, Johan Hovold wrote:
> > On Thu, Jun 08, 2023 at 05:57:23PM +0000, Thinh Nguyen wrote:
> > > On Thu, Jun 08, 2023, Krishna Kurapati PSSNV wrote:
> > > > On 6/8/2023 3:12 PM, Johan Hovold wrote:
> > > > > On Thu, Jun 08, 2023 at 01:21:02AM +0530, Krishna Kurapati PSSNV wrote:
> > > > > > On 6/7/2023 5:07 PM, Johan Hovold wrote:
> > > > > 
> > > > > > > So there at least two issues with this series:
> > > > > > > 
> > > > > > > 	1. accessing xhci registers from the dwc3 core
> > > > > > > 	2. accessing driver data of a child device
> > > > > > > 
> > > > > > > 1. The first part about accessing xhci registers goes against the clear
> > > > > > > separation between glue, core and xhci that Felipe tried to maintain.
> > > > > > > 
> > > > > > > I'm not entirely against doing this from the core driver before
> > > > > > > registering the xhci platform device as the registers are unmapped
> > > > > > > afterwards. But if this is to be allowed, then the implementation should
> > > > > > > be shared with xhci rather than copied verbatim.
> > > 
> > > The core will just be looking at the HW capability registers and
> > > accessing the ports capability. Our programming guide also listed the
> > > host capability registers in its documentation. We're not driving the
> > > xhci controller here. We're initializing some of the core configs base
> > > on its capability.
> > > 
> > > We're duplicating the logic here and not exactly doing it verbatim.
> > > Let's try not to share the whole xhci header where we should not have
> > > visibility over. Perhaps it makes sense in some other driver, but let's
> > > not do it here.
> > 
> > The patch series even copied the kernel doc verbatim. This is just not
> > the way things are supposed to be done upstream. We share defines and
> > implementations all the time, but we should not be making copies of
> > them.
> 
>  We had some fixes to the kernel doc as it's incorrect description.
>  Perhaps we can fully rewrite the kernel-doc if that what makes it
>  better.

No, this is an example of why you should not copy code (and docs)
around, for example, so you don't have to fix the same bug (or typo) in
multiple places.

> We can share define implementations if they are meant to be
>  shared. However, with the current way xhci header is implemented, it's
>  not meant to be shared with dwc3. You agreed that we are violating this
>  in some driver, but you're also insistent that we should not duplicate
>  the logic to avoid this violation. Perhaps I'm not a maintainer here
>  long enough to know some violation is better kept. If sharing the xhci
>  header is what it takes to get this through, then fine.

Just because no-one has used these outside of xhci today, doesn't mean
that you should copy the defines once they are needed outside that
driver.

And in fact, they are used outside of the xhci driver proper already
today, only that people took the lazy approach and shared the inline
helper for that.

> > > > > > > 
> > > > > > > The alternative that avoids this issue entirely could indeed be to
> > > > > > > simply count the number of PHYs described in DT as Rob initially
> > > > > > > suggested. Why would that not work?
> > > 
> > > See below.
> > > 
> > > > > > > 
> > > > > > The reason why I didn't want to read the Phy's from DT is explained in
> > > > > > [1]. I felt it makes the code unreadable and its very tricky to read the
> > > > > > phy's properly, so we decided we would initialize phy's for all ports
> > > > > > and if a phy is missing in DT, the corresponding member in
> > > > > > dwc->usbX_generic_phy[] would be NULL and any phy op on it would be a NOP.
> > > > > 
> > > > > That doesn't sound too convincing. Can't you just iterate over the PHYs
> > > > > described in DT and determine the maximum port number used for HS and
> > > > > SS?
> > > > > > Also as per Krzysztof suggestion on [2], we can add a compatible to read
> > > > > > number of phy's / ports present. This avoids accessing xhci members
> > > > > > atleast in driver core. But the layering violations would still be present.
> > > > > 
> > > > > Yes, but if the information is already available in DT it's better to use
> > > > > it rather than re-encode it in the driver.
> > 
> > > >   Are you suggesting that we just do something like
> > > > num_ports = max( highest usb2 portnum, highest usb3 port num)
> > > 
> > > Why do we want to do this? This makes num_ports ambiguous. Let's not
> > > sacrifice clarity for some lines of code.
> > 
> > This is not about lines of code, but avoiding the bad practice of
> > copying code around and, to some degree, maintaining the separation
> > between the glue, core, and xhci which Felipe (perhaps mistakingly) has
> > fought for.
> 
> We're talking about combining num_usb3_ports and num_usb2_ports here,
> what does that have to do with layer separation?

I mentioned this is as an alternative if you're are hell-bent on not
reusing the xhci register defines and parsing code.

> > If you just need to know how many PHYs you have in DT so that you can
> > iterate over that internal array, you can just look at the max index in
> > DT where the indexes are specified in the first place.
> > 
> > Don't get hung up on the current variable names, those can be renamed to
> > match the implementation. Call it max_ports or whatever.
> 
> It doesn't matter what variable name is given, it doesn't change the
> fact that this "num_ports" or "max_ports" obfuscated usb2 vs usb3 ports
> just for this specific implementation. So, don't do that.

Ok, then make sure you reuse the xhci implementation for that then.

> > > > If so, incase the usb2 phy of quad port controller is missing in DT, we
> > > > would still read num_usb2_ports as 4 but the usb2_generic_phy[1] would be
> > > > NULL and any phy ops would still be NOP. But we would be getting rid of
> > > > reading the xhci registers compeltely in core driver.
> > > > 
> > > > Thinh, Bjorn, can you also let us know your views on this.
> > > > 
> > > > 1. Read:
> > > >   num_usb3_ports = highest usb3 port index in DT
> > > >   num_usb2_ports = max( highest usb2 port index, num_usb3_ports)
> > > > 
> > > > 2. Read the same by parsing xhci registers as done in recent versions of
> > > > this series.
> > > 
> > > DT is not reliable to get this info. As noted, the DT may skip some
> > > ports and still be fine. However, the driver doesn't know which port
> > > reflects which port config index without the exact port count.
> > 
> > That's not correct. DT provides the port indexes already, for example:
> > 
> > 	phy-names = "usb2-port0", "usb3-port0",
> > 		    "usb2-port1", "usb3-port1",
> > 		    "usb2-port2",
> > 		    "usb2-port3";
> > 
> > So if you just need this to iterate over the PHYs all the information
> > needed is here.
> > 
> > If you need to access ports which do not have a PHY described in DT,
> > then this is not going to suffice, but I have not seen anyone claim that
> > that is needed yet.
> 
> Perhaps I misunderstand the conversation. However, there isn't a method
> that everyone's agree on yet regarding DT [*]. Perhaps this indicates it
> may not be the best approach. You can resume the conversation if you
> want to:
> 
> [*] https://lore.kernel.org/linux-usb/9671cade-1820-22e1-9db9-5c9836414908@quicinc.com/#t

This was the approach suggested by Rob, the DT maintainer, in that very
thread IIRC.

> > > More importantly, the host controller that lives on the PCI bus will not
> > > use DT. This can be useful for some re-configurations if the controller
> > > is a PCI device and that goes through the dwc3 code path.
> > 
> > Ok, this is a bit hand wavy, but if this ever turns out to be needed it
> > can also be implemented then.
> 
> What does hand wavy mean? We have case where it's useful outside of
> this, and it would be useful for PCI device too:
> 
> https://lore.kernel.org/linux-usb/20230517233218.rjfmvptrexgkpam3@synopsys.com/

Hand-wavy as in lacking detail which makes it hard to evaluate the
argument.

> > Or just generalise the xhci implementation for parsing these registers
> > and reuse that from the start. (As a bonus you'd shrink the kernel text
> > size by getting rid of that iffy inline implementation.)
> > 
> 
> I don't like the iffy inline function either. We changed that here. To
> rework the xhci header and define its global header seems a bit
> excessive just for dwc3 to get the port capability. Regardless, as I've
> said, if we _must_, perhaps we can just import xhci-ext-caps.h instead
> of the whole xhci.h.

No one said anything about importing the whole of xhci.h.

Johan
  

On Thu, Jun 15, 2023 at 09:08:01PM +0000, Thinh Nguyen wrote:
> On Thu, Jun 15, 2023, Krishna Kurapati PSSNV wrote:

> >  How about we add compatible data indicating the number of usb2/usb3 ports.
> > That way we needn't parse the DT or read xhci registers atleast as a
> > temporary solution to unblock other patches. Once this series is merged, we
> > can get back to fixing the port count calculation. Does it seem fine ?
> > 
> 
> Temporary solution should not involve DT as it's not easily reverted or
> changed. Just include xhci-ext-caps.h and use the inline function. I
> think Johan is fine with that. If not, he can provide more feedback.

Yes, I already suggested that as a quick way forward since it is already
used this way by the xhci debug driver.

Johan
  

On 6/21/2023 1:08 PM, Johan Hovold wrote:
> On Thu, Jun 15, 2023 at 09:08:01PM +0000, Thinh Nguyen wrote:
>> On Thu, Jun 15, 2023, Krishna Kurapati PSSNV wrote:
> 
>>>   How about we add compatible data indicating the number of usb2/usb3 ports.
>>> That way we needn't parse the DT or read xhci registers atleast as a
>>> temporary solution to unblock other patches. Once this series is merged, we
>>> can get back to fixing the port count calculation. Does it seem fine ?
>>>
>>
>> Temporary solution should not involve DT as it's not easily reverted or
>> changed. Just include xhci-ext-caps.h and use the inline function. I
>> think Johan is fine with that. If not, he can provide more feedback.
> 
> Yes, I already suggested that as a quick way forward since it is already
> used this way by the xhci debug driver.
> 
> Johan

Hi Johan, Thinh,

  Pushed a v9 following the above suggestion.

Thanks,
Krishna,
  

On Wed, Jun 21, 2023, Johan Hovold wrote:
> Hi Thinh,
> 
> and sorry about the late reply. Was on holiday last week.
> 
> On Fri, Jun 09, 2023 at 06:16:13PM +0000, Thinh Nguyen wrote:
> > On Fri, Jun 09, 2023, Johan Hovold wrote:
> > > On Thu, Jun 08, 2023 at 05:57:23PM +0000, Thinh Nguyen wrote:
> > > > On Thu, Jun 08, 2023, Krishna Kurapati PSSNV wrote:
> > > > > On 6/8/2023 3:12 PM, Johan Hovold wrote:
> > > > > > On Thu, Jun 08, 2023 at 01:21:02AM +0530, Krishna Kurapati PSSNV wrote:
> > > > > > > On 6/7/2023 5:07 PM, Johan Hovold wrote:
> > > > > > 
> > > > > > > > So there at least two issues with this series:
> > > > > > > > 
> > > > > > > > 	1. accessing xhci registers from the dwc3 core
> > > > > > > > 	2. accessing driver data of a child device
> > > > > > > > 
> > > > > > > > 1. The first part about accessing xhci registers goes against the clear
> > > > > > > > separation between glue, core and xhci that Felipe tried to maintain.
> > > > > > > > 
> > > > > > > > I'm not entirely against doing this from the core driver before
> > > > > > > > registering the xhci platform device as the registers are unmapped
> > > > > > > > afterwards. But if this is to be allowed, then the implementation should
> > > > > > > > be shared with xhci rather than copied verbatim.
> > > > 
> > > > The core will just be looking at the HW capability registers and
> > > > accessing the ports capability. Our programming guide also listed the
> > > > host capability registers in its documentation. We're not driving the
> > > > xhci controller here. We're initializing some of the core configs base
> > > > on its capability.
> > > > 
> > > > We're duplicating the logic here and not exactly doing it verbatim.
> > > > Let's try not to share the whole xhci header where we should not have
> > > > visibility over. Perhaps it makes sense in some other driver, but let's
> > > > not do it here.
> > > 
> > > The patch series even copied the kernel doc verbatim. This is just not
> > > the way things are supposed to be done upstream. We share defines and
> > > implementations all the time, but we should not be making copies of
> > > them.
> > 
> >  We had some fixes to the kernel doc as it's incorrect description.
> >  Perhaps we can fully rewrite the kernel-doc if that what makes it
> >  better.
> 
> No, this is an example of why you should not copy code (and docs)
> around, for example, so you don't have to fix the same bug (or typo) in
> multiple places.
> 
> > We can share define implementations if they are meant to be
> >  shared. However, with the current way xhci header is implemented, it's
> >  not meant to be shared with dwc3. You agreed that we are violating this
> >  in some driver, but you're also insistent that we should not duplicate
> >  the logic to avoid this violation. Perhaps I'm not a maintainer here
> >  long enough to know some violation is better kept. If sharing the xhci
> >  header is what it takes to get this through, then fine.
> 
> Just because no-one has used these outside of xhci today, doesn't mean
> that you should copy the defines once they are needed outside that
> driver.
> 
> And in fact, they are used outside of the xhci driver proper already
> today, only that people took the lazy approach and shared the inline
> helper for that.
> 
> > > > > > > > 
> > > > > > > > The alternative that avoids this issue entirely could indeed be to
> > > > > > > > simply count the number of PHYs described in DT as Rob initially
> > > > > > > > suggested. Why would that not work?
> > > > 
> > > > See below.
> > > > 
> > > > > > > > 
> > > > > > > The reason why I didn't want to read the Phy's from DT is explained in
> > > > > > > [1]. I felt it makes the code unreadable and its very tricky to read the
> > > > > > > phy's properly, so we decided we would initialize phy's for all ports
> > > > > > > and if a phy is missing in DT, the corresponding member in
> > > > > > > dwc->usbX_generic_phy[] would be NULL and any phy op on it would be a NOP.
> > > > > > 
> > > > > > That doesn't sound too convincing. Can't you just iterate over the PHYs
> > > > > > described in DT and determine the maximum port number used for HS and
> > > > > > SS?
> > > > > > > Also as per Krzysztof suggestion on [2], we can add a compatible to read
> > > > > > > number of phy's / ports present. This avoids accessing xhci members
> > > > > > > atleast in driver core. But the layering violations would still be present.
> > > > > > 
> > > > > > Yes, but if the information is already available in DT it's better to use
> > > > > > it rather than re-encode it in the driver.
> > > 
> > > > >   Are you suggesting that we just do something like
> > > > > num_ports = max( highest usb2 portnum, highest usb3 port num)
> > > > 
> > > > Why do we want to do this? This makes num_ports ambiguous. Let's not
> > > > sacrifice clarity for some lines of code.
> > > 
> > > This is not about lines of code, but avoiding the bad practice of
> > > copying code around and, to some degree, maintaining the separation
> > > between the glue, core, and xhci which Felipe (perhaps mistakingly) has
> > > fought for.
> > 
> > We're talking about combining num_usb3_ports and num_usb2_ports here,
> > what does that have to do with layer separation?
> 
> I mentioned this is as an alternative if you're are hell-bent on not
> reusing the xhci register defines and parsing code.
> 
> > > If you just need to know how many PHYs you have in DT so that you can
> > > iterate over that internal array, you can just look at the max index in
> > > DT where the indexes are specified in the first place.
> > > 
> > > Don't get hung up on the current variable names, those can be renamed to
> > > match the implementation. Call it max_ports or whatever.
> > 
> > It doesn't matter what variable name is given, it doesn't change the
> > fact that this "num_ports" or "max_ports" obfuscated usb2 vs usb3 ports
> > just for this specific implementation. So, don't do that.
> 
> Ok, then make sure you reuse the xhci implementation for that then.
> 
> > > > > If so, incase the usb2 phy of quad port controller is missing in DT, we
> > > > > would still read num_usb2_ports as 4 but the usb2_generic_phy[1] would be
> > > > > NULL and any phy ops would still be NOP. But we would be getting rid of
> > > > > reading the xhci registers compeltely in core driver.
> > > > > 
> > > > > Thinh, Bjorn, can you also let us know your views on this.
> > > > > 
> > > > > 1. Read:
> > > > >   num_usb3_ports = highest usb3 port index in DT
> > > > >   num_usb2_ports = max( highest usb2 port index, num_usb3_ports)
> > > > > 
> > > > > 2. Read the same by parsing xhci registers as done in recent versions of
> > > > > this series.
> > > > 
> > > > DT is not reliable to get this info. As noted, the DT may skip some
> > > > ports and still be fine. However, the driver doesn't know which port
> > > > reflects which port config index without the exact port count.
> > > 
> > > That's not correct. DT provides the port indexes already, for example:
> > > 
> > > 	phy-names = "usb2-port0", "usb3-port0",
> > > 		    "usb2-port1", "usb3-port1",
> > > 		    "usb2-port2",
> > > 		    "usb2-port3";
> > > 
> > > So if you just need this to iterate over the PHYs all the information
> > > needed is here.
> > > 
> > > If you need to access ports which do not have a PHY described in DT,
> > > then this is not going to suffice, but I have not seen anyone claim that
> > > that is needed yet.
> > 
> > Perhaps I misunderstand the conversation. However, there isn't a method
> > that everyone's agree on yet regarding DT [*]. Perhaps this indicates it
> > may not be the best approach. You can resume the conversation if you
> > want to:
> > 
> > [*] https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/9671cade-1820-22e1-9db9-5c9836414908@quicinc.com/*t__;Iw!!A4F2R9G_pg!btc_fkrnAm1eFG5c5V0bWMnPnq4yvFyza1nDRJE8mPvSSrYIyxkUxjAARU51cRsoo2n0eLwTGt_SYvnQOaQ$ 
> 
> This was the approach suggested by Rob, the DT maintainer, in that very
> thread IIRC.
> 
> > > > More importantly, the host controller that lives on the PCI bus will not
> > > > use DT. This can be useful for some re-configurations if the controller
> > > > is a PCI device and that goes through the dwc3 code path.
> > > 
> > > Ok, this is a bit hand wavy, but if this ever turns out to be needed it
> > > can also be implemented then.
> > 
> > What does hand wavy mean? We have case where it's useful outside of
> > this, and it would be useful for PCI device too:
> > 
> > https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20230517233218.rjfmvptrexgkpam3@synopsys.com/__;!!A4F2R9G_pg!btc_fkrnAm1eFG5c5V0bWMnPnq4yvFyza1nDRJE8mPvSSrYIyxkUxjAARU51cRsoo2n0eLwTGt_SqPJ_Tn0$ 
> 
> Hand-wavy as in lacking detail which makes it hard to evaluate the
> argument.
> 
> > > Or just generalise the xhci implementation for parsing these registers
> > > and reuse that from the start. (As a bonus you'd shrink the kernel text
> > > size by getting rid of that iffy inline implementation.)
> > > 
> > 
> > I don't like the iffy inline function either. We changed that here. To
> > rework the xhci header and define its global header seems a bit
> > excessive just for dwc3 to get the port capability. Regardless, as I've
> > said, if we _must_, perhaps we can just import xhci-ext-caps.h instead
> > of the whole xhci.h.
> 
> No one said anything about importing the whole of xhci.h.
> 

This was how it was originally done in the earlier version of the
series. Perhaps that's why I was against it. It looks wrong for dwc3 to
know about xhci_hcd structure.

Thanks,
Thinh
  

Hi Krishna,

On Wed, Jun 07, 2023 at 02:16:37PM +0200, Johan Hovold wrote:
> On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote:

> >  static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
> >  {
> >  	u32 reg;
> > @@ -413,13 +423,16 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
> >  {
> >  	u32 val;
> >  	int i, ret;
> > +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> >  
> >  	if (qcom->is_suspended)
> >  		return 0;
> >  
> > -	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
> > -	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
> > -		dev_err(qcom->dev, "HS-PHY not in L2\n");
> > +	for (i = 0; i < dwc->num_usb2_ports; i++) {
> > +		val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg_offset[i]);
> > +		if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
> > +			dev_err(qcom->dev, "HS-PHY%d not in L2\n", i);
> > +	}

> When testing this on the X13s I get:
> 
> 	dwc3-qcom a4f8800.usb: HS-PHY2 not in L2
> 
> for the third port, whose status registers always seems to return zero
> (e.g. as if we're checking the wrong register?):
> 
> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 0, pwr_event_stat = 38103c
> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 1, pwr_event_stat = 38103c
> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 2, pwr_event_stat = 00
> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 3, pwr_event_stat = 140030
> 
> I verified that everything appears to work as expected on sa8295p-adp.
> 
> Do you have any idea of what may be causing this?

You never replied to this; do you have any idea why the status register
for the second port seemingly always read back as 0 on the X13s?

Johan
  

On 6/27/2023 9:13 PM, Johan Hovold wrote:
> Hi Krishna,
> 
> On Wed, Jun 07, 2023 at 02:16:37PM +0200, Johan Hovold wrote:
>> On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote:
> 
>>>   static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
>>>   {
>>>   	u32 reg;
>>> @@ -413,13 +423,16 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
>>>   {
>>>   	u32 val;
>>>   	int i, ret;
>>> +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
>>>   
>>>   	if (qcom->is_suspended)
>>>   		return 0;
>>>   
>>> -	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
>>> -	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
>>> -		dev_err(qcom->dev, "HS-PHY not in L2\n");
>>> +	for (i = 0; i < dwc->num_usb2_ports; i++) {
>>> +		val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg_offset[i]);
>>> +		if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
>>> +			dev_err(qcom->dev, "HS-PHY%d not in L2\n", i);
>>> +	}
> 
>> When testing this on the X13s I get:
>>
>> 	dwc3-qcom a4f8800.usb: HS-PHY2 not in L2
>>
>> for the third port, whose status registers always seems to return zero
>> (e.g. as if we're checking the wrong register?):
>>
>> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 0, pwr_event_stat = 38103c
>> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 1, pwr_event_stat = 38103c
>> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 2, pwr_event_stat = 00
>> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 3, pwr_event_stat = 140030
>>
>> I verified that everything appears to work as expected on sa8295p-adp.
>>
>> Do you have any idea of what may be causing this?
> 
> You never replied to this; do you have any idea why the status register
> for the second port seemingly always read back as 0 on the X13s?
> 
> Johan

Hi Johan,

  Missed this mail. This never popped up on my system. So no idea what 
is different in Lenovo X13s. Might need to check with team internally.

Regards,
Krishna,
  

On Mon, Jul 03, 2023 at 12:35:48AM +0530, Krishna Kurapati PSSNV wrote:
> On 6/27/2023 9:13 PM, Johan Hovold wrote:
> > On Wed, Jun 07, 2023 at 02:16:37PM +0200, Johan Hovold wrote:
> >> On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote:

> >>> -	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
> >>> -	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
> >>> -		dev_err(qcom->dev, "HS-PHY not in L2\n");
> >>> +	for (i = 0; i < dwc->num_usb2_ports; i++) {
> >>> +		val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg_offset[i]);
> >>> +		if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
> >>> +			dev_err(qcom->dev, "HS-PHY%d not in L2\n", i);
> >>> +	}
> > 
> >> When testing this on the X13s I get:
> >>
> >> 	dwc3-qcom a4f8800.usb: HS-PHY2 not in L2
> >>
> >> for the third port, whose status registers always seems to return zero
> >> (e.g. as if we're checking the wrong register?):
> >>
> >> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 0, pwr_event_stat = 38103c
> >> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 1, pwr_event_stat = 38103c
> >> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 2, pwr_event_stat = 00
> >> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 3, pwr_event_stat = 140030
> >>
> >> I verified that everything appears to work as expected on sa8295p-adp.
> >>
> >> Do you have any idea of what may be causing this?
> > 
> > You never replied to this; do you have any idea why the status register
> > for the second port seemingly always read back as 0 on the X13s?

>   Missed this mail. This never popped up on my system. So no idea what 
> is different in Lenovo X13s. Might need to check with team internally.

Did you hear anything back regarding the above?

Could it even be that the register offset it not correct for sc8280xp?

Johan
  

On 7/14/2023 2:30 PM, Johan Hovold wrote:
> On Mon, Jul 03, 2023 at 12:35:48AM +0530, Krishna Kurapati PSSNV wrote:
>> On 6/27/2023 9:13 PM, Johan Hovold wrote:
>>> On Wed, Jun 07, 2023 at 02:16:37PM +0200, Johan Hovold wrote:
>>>> On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote:
> 
>>>>> -	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
>>>>> -	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
>>>>> -		dev_err(qcom->dev, "HS-PHY not in L2\n");
>>>>> +	for (i = 0; i < dwc->num_usb2_ports; i++) {
>>>>> +		val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg_offset[i]);
>>>>> +		if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
>>>>> +			dev_err(qcom->dev, "HS-PHY%d not in L2\n", i);
>>>>> +	}
>>>
>>>> When testing this on the X13s I get:
>>>>
>>>> 	dwc3-qcom a4f8800.usb: HS-PHY2 not in L2
>>>>
>>>> for the third port, whose status registers always seems to return zero
>>>> (e.g. as if we're checking the wrong register?):
>>>>
>>>> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 0, pwr_event_stat = 38103c
>>>> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 1, pwr_event_stat = 38103c
>>>> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 2, pwr_event_stat = 00
>>>> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 3, pwr_event_stat = 140030
>>>>
>>>> I verified that everything appears to work as expected on sa8295p-adp.
>>>>
>>>> Do you have any idea of what may be causing this?
>>>
>>> You never replied to this; do you have any idea why the status register
>>> for the second port seemingly always read back as 0 on the X13s?
> 
>>    Missed this mail. This never popped up on my system. So no idea what
>> is different in Lenovo X13s. Might need to check with team internally.
> 
> Did you hear anything back regarding the above?
> 
> Could it even be that the register offset it not correct for sc8280xp?
>

Hi Johan,

No. I rechecked the register offsets and they are proper. (same as what 
we are using in downstream).

Adding Jack and Wesley to help with any suggestions here.

Regards,
Krishna,
  

On Fri, Jul 14, 2023 at 04:08:45PM +0530, Krishna Kurapati PSSNV wrote:
> On 7/14/2023 2:30 PM, Johan Hovold wrote:
> > On Mon, Jul 03, 2023 at 12:35:48AM +0530, Krishna Kurapati PSSNV wrote:
> >> On 6/27/2023 9:13 PM, Johan Hovold wrote:
> >>> On Wed, Jun 07, 2023 at 02:16:37PM +0200, Johan Hovold wrote:
> >>>> On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote:
> > 
> >>>>> -	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
> >>>>> -	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
> >>>>> -		dev_err(qcom->dev, "HS-PHY not in L2\n");
> >>>>> +	for (i = 0; i < dwc->num_usb2_ports; i++) {
> >>>>> +		val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg_offset[i]);
> >>>>> +		if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
> >>>>> +			dev_err(qcom->dev, "HS-PHY%d not in L2\n", i);
> >>>>> +	}
> >>>
> >>>> When testing this on the X13s I get:
> >>>>
> >>>> 	dwc3-qcom a4f8800.usb: HS-PHY2 not in L2
> >>>>
> >>>> for the third port, whose status registers always seems to return zero
> >>>> (e.g. as if we're checking the wrong register?):
> >>>>
> >>>> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 0, pwr_event_stat = 38103c
> >>>> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 1, pwr_event_stat = 38103c
> >>>> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 2, pwr_event_stat = 00
> >>>> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 3, pwr_event_stat = 140030
> >>>>
> >>>> I verified that everything appears to work as expected on sa8295p-adp.
> >>>>
> >>>> Do you have any idea of what may be causing this?
> >>>
> >>> You never replied to this; do you have any idea why the status register
> >>> for the second port seemingly always read back as 0 on the X13s?
> > 
> >>    Missed this mail. This never popped up on my system. So no idea what
> >> is different in Lenovo X13s. Might need to check with team internally.
> > 
> > Did you hear anything back regarding the above?
> > 
> > Could it even be that the register offset it not correct for sc8280xp?

> No. I rechecked the register offsets and they are proper. (same as what 
> we are using in downstream).
> 
> Adding Jack and Wesley to help with any suggestions here.

Still no idea as to why this appears to be broken on sc8280xp and
triggers an error on every suspend?

Johan
  

On 21.07.2023 13:16, Johan Hovold wrote:
> On Fri, Jul 14, 2023 at 04:08:45PM +0530, Krishna Kurapati PSSNV wrote:
>> On 7/14/2023 2:30 PM, Johan Hovold wrote:
>>> On Mon, Jul 03, 2023 at 12:35:48AM +0530, Krishna Kurapati PSSNV wrote:
>>>> On 6/27/2023 9:13 PM, Johan Hovold wrote:
>>>>> On Wed, Jun 07, 2023 at 02:16:37PM +0200, Johan Hovold wrote:
>>>>>> On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote:
>>>
>>>>>>> -	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
>>>>>>> -	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
>>>>>>> -		dev_err(qcom->dev, "HS-PHY not in L2\n");
>>>>>>> +	for (i = 0; i < dwc->num_usb2_ports; i++) {
>>>>>>> +		val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg_offset[i]);
>>>>>>> +		if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
>>>>>>> +			dev_err(qcom->dev, "HS-PHY%d not in L2\n", i);
>>>>>>> +	}
>>>>>
>>>>>> When testing this on the X13s I get:
>>>>>>
>>>>>> 	dwc3-qcom a4f8800.usb: HS-PHY2 not in L2
Sidenote, I get this on any Qcom device on any platform I try
to enter suspend on, without these MP patches.

Konrad
  

On Fri, Jul 21, 2023 at 02:10:07PM +0200, Konrad Dybcio wrote:
> On 21.07.2023 13:16, Johan Hovold wrote:
> > On Fri, Jul 14, 2023 at 04:08:45PM +0530, Krishna Kurapati PSSNV wrote:
> >> On 7/14/2023 2:30 PM, Johan Hovold wrote:
> >>> On Mon, Jul 03, 2023 at 12:35:48AM +0530, Krishna Kurapati PSSNV wrote:
> >>>> On 6/27/2023 9:13 PM, Johan Hovold wrote:
> >>>>> On Wed, Jun 07, 2023 at 02:16:37PM +0200, Johan Hovold wrote:
> >>>>>> On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote:
> >>>
> >>>>>>> -	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
> >>>>>>> -	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
> >>>>>>> -		dev_err(qcom->dev, "HS-PHY not in L2\n");
> >>>>>>> +	for (i = 0; i < dwc->num_usb2_ports; i++) {
> >>>>>>> +		val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg_offset[i]);
> >>>>>>> +		if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
> >>>>>>> +			dev_err(qcom->dev, "HS-PHY%d not in L2\n", i);
> >>>>>>> +	}
> >>>>>
> >>>>>> When testing this on the X13s I get:
> >>>>>>
> >>>>>> 	dwc3-qcom a4f8800.usb: HS-PHY2 not in L2

> Sidenote, I get this on any Qcom device on any platform I try
> to enter suspend on, without these MP patches.

Ok, that might provide some hint. But on sc8280xp (X13s) we only get it
on one of the four MP ports (i.e. on one out of six ports in total).

While on sa8295p-adp there are no such errors on any port.

Johan
  

On 21.07.2023 14:54, Johan Hovold wrote:
> On Fri, Jul 21, 2023 at 02:10:07PM +0200, Konrad Dybcio wrote:
>> On 21.07.2023 13:16, Johan Hovold wrote:
>>> On Fri, Jul 14, 2023 at 04:08:45PM +0530, Krishna Kurapati PSSNV wrote:
>>>> On 7/14/2023 2:30 PM, Johan Hovold wrote:
>>>>> On Mon, Jul 03, 2023 at 12:35:48AM +0530, Krishna Kurapati PSSNV wrote:
>>>>>> On 6/27/2023 9:13 PM, Johan Hovold wrote:
>>>>>>> On Wed, Jun 07, 2023 at 02:16:37PM +0200, Johan Hovold wrote:
>>>>>>>> On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote:
>>>>>
>>>>>>>>> -	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
>>>>>>>>> -	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
>>>>>>>>> -		dev_err(qcom->dev, "HS-PHY not in L2\n");
>>>>>>>>> +	for (i = 0; i < dwc->num_usb2_ports; i++) {
>>>>>>>>> +		val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg_offset[i]);
>>>>>>>>> +		if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
>>>>>>>>> +			dev_err(qcom->dev, "HS-PHY%d not in L2\n", i);
>>>>>>>>> +	}
>>>>>>>
>>>>>>>> When testing this on the X13s I get:
>>>>>>>>
>>>>>>>> 	dwc3-qcom a4f8800.usb: HS-PHY2 not in L2
> 
>> Sidenote, I get this on any Qcom device on any platform I try
>> to enter suspend on, without these MP patches.
> 
> Ok, that might provide some hint. But on sc8280xp (X13s) we only get it
> on one of the four MP ports (i.e. on one out of six ports in total).
> 
> While on sa8295p-adp there are no such errors on any port.
I've been playing with 8450 and it looks like snps,dis_u2_susphy_quirk
causes this error.

The downstream tree contains this property and I'm inclined to believe
it means that this platforms should define it (as the devicetrees are
machine-generated to a degree, AFAIK), especially since this quirk does
the exact same thing on a known-working downstream, namely unsetting
DWC3_GUSB2PHYCFG_SUSPHY.

Digging a bit deeper, dwc3-msm-core [1], the downstream version of dwc3-qcom
performs a bit of a dance in a couple of places.. Look for that register name.

Unfortunately I have little idea what the "USB2 suspend phy" is.. is it a PHY
used in suspend? Is it the suspension of the USB2 PHY? No clue.

[1] https://git.codelinaro.org/clo/la/kernel/msm-5.10/-/blob/KERNEL.PLATFORM.1.0.r2-08800-WAIPIOLE.0/drivers/usb/dwc3/dwc3-msm-core.c

Konrad
  

On 8/11/2023 10:18 PM, Konrad Dybcio wrote:
> On 21.07.2023 14:54, Johan Hovold wrote:
>> On Fri, Jul 21, 2023 at 02:10:07PM +0200, Konrad Dybcio wrote:
>>> On 21.07.2023 13:16, Johan Hovold wrote:
>>>> On Fri, Jul 14, 2023 at 04:08:45PM +0530, Krishna Kurapati PSSNV wrote:
>>>>> On 7/14/2023 2:30 PM, Johan Hovold wrote:
>>>>>> On Mon, Jul 03, 2023 at 12:35:48AM +0530, Krishna Kurapati PSSNV wrote:
>>>>>>> On 6/27/2023 9:13 PM, Johan Hovold wrote:
>>>>>>>> On Wed, Jun 07, 2023 at 02:16:37PM +0200, Johan Hovold wrote:
>>>>>>>>> On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote:
>>>>>>
>>>>>>>>>> -	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
>>>>>>>>>> -	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
>>>>>>>>>> -		dev_err(qcom->dev, "HS-PHY not in L2\n");
>>>>>>>>>> +	for (i = 0; i < dwc->num_usb2_ports; i++) {
>>>>>>>>>> +		val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg_offset[i]);
>>>>>>>>>> +		if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
>>>>>>>>>> +			dev_err(qcom->dev, "HS-PHY%d not in L2\n", i);
>>>>>>>>>> +	}
>>>>>>>>
>>>>>>>>> When testing this on the X13s I get:
>>>>>>>>>
>>>>>>>>> 	dwc3-qcom a4f8800.usb: HS-PHY2 not in L2
>>
>>> Sidenote, I get this on any Qcom device on any platform I try
>>> to enter suspend on, without these MP patches.
>>
>> Ok, that might provide some hint. But on sc8280xp (X13s) we only get it
>> on one of the four MP ports (i.e. on one out of six ports in total).
>>
>> While on sa8295p-adp there are no such errors on any port.
> I've been playing with 8450 and it looks like snps,dis_u2_susphy_quirk
> causes this error.
> 
> The downstream tree contains this property and I'm inclined to believe
> it means that this platforms should define it (as the devicetrees are
> machine-generated to a degree, AFAIK), especially since this quirk does
> the exact same thing on a known-working downstream, namely unsetting
> DWC3_GUSB2PHYCFG_SUSPHY.
> 
> Digging a bit deeper, dwc3-msm-core [1], the downstream version of dwc3-qcom
> performs a bit of a dance in a couple of places.. Look for that register name.
> 
> Unfortunately I have little idea what the "USB2 suspend phy" is.. is it a PHY
> used in suspend? Is it the suspension of the USB2 PHY? No clue.
> 
> [1] https://git.codelinaro.org/clo/la/kernel/msm-5.10/-/blob/KERNEL.PLATFORM.1.0.r2-08800-WAIPIOLE.0/drivers/usb/dwc3/dwc3-msm-core.c
> 

The description for that bit (BIT(6)) as per the databook is as follows:

---

6 SUSPENDUSB20 R_W Suspend USB2.0 HS/FS/LS PHY (SusPHY)
When set, USB2.0 PHY enters Suspend mode if Suspend
conditions are valid.

For DRD/OTG configurations, it is recommended that this bit is set
to 0 during coreConsultant configuration. If it is set to 1, then the
application must clear this bit after power-on reset. Application
needs to set it to 1 after the core initialization completes.
For all other configurations, this bit can be set to 1 during core
configuration.

Note:
■ In host mode, on reset, this bit is set to 1. Software can override
this bit after reset.
■ In device mode, before issuing any device endpoint command

when operating in 2.0 speeds, disable this bit and enable it after
the command completes. If you issue a command without
disabling this bit when the device is in L2 state and if mac2_clk
(utmi_clk/ulpi_clk) is gated off, the command will not get
completed

---

"L2" is the term we say when PHY is suspended, i.e., the main PLL is 
shut off. Internally, I was able to find out that there are several 
conditions where phy can fail to enter L2. The entry into L2 is 
controlled by the USB controller itself, but can be limited by toggling 
GUSB2PHY SUSPENDABLE bit.  if that bit is 0 then controller won't place 
HSPHY into L2. For the failure to enter L2, there can be several 
situations, like there may be some pending line state change that 
happened on the bus.

But Johan's error seems to be different as the register itself reads 
zero which I don't understand.

Regards,
Krishna,