[PATCH] PCI: qcom: Add interconnect bandwidth for PCIe Gen4

[PATCH] PCI: qcom: Add interconnect bandwidth for PCIe Gen4

[Manivannan Sadhasivam]

PCIe Gen4 supports the interconnect bandwidth of 1969 MBps. So let's add
the bandwidth support in the driver. Otherwise, the default bandwidth of
985 MBps will be used.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 297442c969b6..6853123f92c1 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1384,11 +1384,14 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
 	case 2:
 		bw = MBps_to_icc(500);
 		break;
+	case 3:
+		bw = MBps_to_icc(985);
+		break;
 	default:
 		WARN_ON_ONCE(1);
 		fallthrough;
-	case 3:
-		bw = MBps_to_icc(985);
+	case 4:
+		bw = MBps_to_icc(1969);
 		break;
 	}
 
-- 
2.25.1

Re: [PATCH] PCI: qcom: Add interconnect bandwidth for PCIe Gen4

[Konrad Dybcio]

On 24.09.2023 18:07, Manivannan Sadhasivam wrote:
> PCIe Gen4 supports the interconnect bandwidth of 1969 MBps. So let's add
> the bandwidth support in the driver. Otherwise, the default bandwidth of
> 985 MBps will be used.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 297442c969b6..6853123f92c1 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1384,11 +1384,14 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
>  	case 2:
>  		bw = MBps_to_icc(500);
>  		break;
> +	case 3:
> +		bw = MBps_to_icc(985);
> +		break;
>  	default:
>  		WARN_ON_ONCE(1);
>  		fallthrough;
> -	case 3:
> -		bw = MBps_to_icc(985);
> +	case 4:
> +		bw = MBps_to_icc(1969);
>  		break;
Are you adding case 4 under `default`? That looks.. bizzare..

Konrad

Re: [PATCH] PCI: qcom: Add interconnect bandwidth for PCIe Gen4

[Abel Vesa]

On 23-09-25 10:57:47, Konrad Dybcio wrote:
> On 24.09.2023 18:07, Manivannan Sadhasivam wrote:
> > PCIe Gen4 supports the interconnect bandwidth of 1969 MBps. So let's add
> > the bandwidth support in the driver. Otherwise, the default bandwidth of
> > 985 MBps will be used.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 297442c969b6..6853123f92c1 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -1384,11 +1384,14 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
> >  	case 2:
> >  		bw = MBps_to_icc(500);
> >  		break;
> > +	case 3:
> > +		bw = MBps_to_icc(985);
> > +		break;
> >  	default:
> >  		WARN_ON_ONCE(1);
> >  		fallthrough;
> > -	case 3:
> > -		bw = MBps_to_icc(985);
> > +	case 4:
> > +		bw = MBps_to_icc(1969);
> >  		break;
> Are you adding case 4 under `default`? That looks.. bizzare..

That's intentional. You want it to use 1969MBps if there is a different
gen value. AFAIU.

> 
> Konrad

Re: [PATCH] PCI: qcom: Add interconnect bandwidth for PCIe Gen4

[Konrad Dybcio]

On 25.09.2023 12:33, Abel Vesa wrote:
> On 23-09-25 10:57:47, Konrad Dybcio wrote:
>> On 24.09.2023 18:07, Manivannan Sadhasivam wrote:
>>> PCIe Gen4 supports the interconnect bandwidth of 1969 MBps. So let's add
>>> the bandwidth support in the driver. Otherwise, the default bandwidth of
>>> 985 MBps will be used.
>>>
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>> ---
>>>  drivers/pci/controller/dwc/pcie-qcom.c | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>>> index 297442c969b6..6853123f92c1 100644
>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>> @@ -1384,11 +1384,14 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
>>>  	case 2:
>>>  		bw = MBps_to_icc(500);
>>>  		break;
>>> +	case 3:
>>> +		bw = MBps_to_icc(985);
>>> +		break;
>>>  	default:
>>>  		WARN_ON_ONCE(1);
>>>  		fallthrough;
>>> -	case 3:
>>> -		bw = MBps_to_icc(985);
>>> +	case 4:
>>> +		bw = MBps_to_icc(1969);
>>>  		break;
>> Are you adding case 4 under `default`? That looks.. bizzare..
> 
> That's intentional. You want it to use 1969MBps if there is a different
> gen value. AFAIU.
Gah right, then the commit message is wrong.

Konrad

Re: [PATCH] PCI: qcom: Add interconnect bandwidth for PCIe Gen4

[Abel Vesa]

On 23-09-25 12:34:53, Konrad Dybcio wrote:
> On 25.09.2023 12:33, Abel Vesa wrote:
> > On 23-09-25 10:57:47, Konrad Dybcio wrote:
> >> On 24.09.2023 18:07, Manivannan Sadhasivam wrote:
> >>> PCIe Gen4 supports the interconnect bandwidth of 1969 MBps. So let's add
> >>> the bandwidth support in the driver. Otherwise, the default bandwidth of
> >>> 985 MBps will be used.
> >>>
> >>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> >>> ---
> >>>  drivers/pci/controller/dwc/pcie-qcom.c | 7 +++++--
> >>>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> >>> index 297442c969b6..6853123f92c1 100644
> >>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> >>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> >>> @@ -1384,11 +1384,14 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
> >>>  	case 2:
> >>>  		bw = MBps_to_icc(500);
> >>>  		break;
> >>> +	case 3:
> >>> +		bw = MBps_to_icc(985);
> >>> +		break;
> >>>  	default:
> >>>  		WARN_ON_ONCE(1);
> >>>  		fallthrough;
> >>> -	case 3:
> >>> -		bw = MBps_to_icc(985);
> >>> +	case 4:
> >>> +		bw = MBps_to_icc(1969);
> >>>  		break;
> >> Are you adding case 4 under `default`? That looks.. bizzare..
> > 
> > That's intentional. You want it to use 1969MBps if there is a different
> > gen value. AFAIU.
> Gah right, then the commit message is wrong.

Yep, should be: "Otherwise, the default bandwidth of 1969 MBps will be
used."

But maybe we should not default to that. Maybe we should still default
to 985 MBps.

> 
> Konrad

Re: [PATCH] PCI: qcom: Add interconnect bandwidth for PCIe Gen4

[Konrad Dybcio]

On 25.09.2023 12:37, Abel Vesa wrote:
> On 23-09-25 12:34:53, Konrad Dybcio wrote:
>> On 25.09.2023 12:33, Abel Vesa wrote:
>>> On 23-09-25 10:57:47, Konrad Dybcio wrote:
>>>> On 24.09.2023 18:07, Manivannan Sadhasivam wrote:
>>>>> PCIe Gen4 supports the interconnect bandwidth of 1969 MBps. So let's add
>>>>> the bandwidth support in the driver. Otherwise, the default bandwidth of
>>>>> 985 MBps will be used.
>>>>>
>>>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>>>> ---
>>>>>  drivers/pci/controller/dwc/pcie-qcom.c | 7 +++++--
>>>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>>>>> index 297442c969b6..6853123f92c1 100644
>>>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>>>> @@ -1384,11 +1384,14 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
>>>>>  	case 2:
>>>>>  		bw = MBps_to_icc(500);
>>>>>  		break;
>>>>> +	case 3:
>>>>> +		bw = MBps_to_icc(985);
>>>>> +		break;
>>>>>  	default:
>>>>>  		WARN_ON_ONCE(1);
>>>>>  		fallthrough;
>>>>> -	case 3:
>>>>> -		bw = MBps_to_icc(985);
>>>>> +	case 4:
>>>>> +		bw = MBps_to_icc(1969);
>>>>>  		break;
>>>> Are you adding case 4 under `default`? That looks.. bizzare..
>>>
>>> That's intentional. You want it to use 1969MBps if there is a different
>>> gen value. AFAIU.
>> Gah right, then the commit message is wrong.
> 
> Yep, should be: "Otherwise, the default bandwidth of 1969 MBps will be
> used."
> 
> But maybe we should not default to that. Maybe we should still default
> to 985 MBps.
Perhaps we shouldn't have a default at all..

E.g. if the gen5 bus may get clogged if we exceed gen4
limits

Konrad

Re: [PATCH] PCI: qcom: Add interconnect bandwidth for PCIe Gen4

[Manivannan Sadhasivam]

On Mon, Sep 25, 2023 at 12:40:34PM +0200, Konrad Dybcio wrote:
> On 25.09.2023 12:37, Abel Vesa wrote:
> > On 23-09-25 12:34:53, Konrad Dybcio wrote:
> >> On 25.09.2023 12:33, Abel Vesa wrote:
> >>> On 23-09-25 10:57:47, Konrad Dybcio wrote:
> >>>> On 24.09.2023 18:07, Manivannan Sadhasivam wrote:
> >>>>> PCIe Gen4 supports the interconnect bandwidth of 1969 MBps. So let's add
> >>>>> the bandwidth support in the driver. Otherwise, the default bandwidth of
> >>>>> 985 MBps will be used.
> >>>>>
> >>>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> >>>>> ---
> >>>>>  drivers/pci/controller/dwc/pcie-qcom.c | 7 +++++--
> >>>>>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> >>>>> index 297442c969b6..6853123f92c1 100644
> >>>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> >>>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> >>>>> @@ -1384,11 +1384,14 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
> >>>>>  	case 2:
> >>>>>  		bw = MBps_to_icc(500);
> >>>>>  		break;
> >>>>> +	case 3:
> >>>>> +		bw = MBps_to_icc(985);
> >>>>> +		break;
> >>>>>  	default:
> >>>>>  		WARN_ON_ONCE(1);
> >>>>>  		fallthrough;
> >>>>> -	case 3:
> >>>>> -		bw = MBps_to_icc(985);
> >>>>> +	case 4:
> >>>>> +		bw = MBps_to_icc(1969);
> >>>>>  		break;
> >>>> Are you adding case 4 under `default`? That looks.. bizzare..
> >>>
> >>> That's intentional. You want it to use 1969MBps if there is a different
> >>> gen value. AFAIU.
> >> Gah right, then the commit message is wrong.
> > 
> > Yep, should be: "Otherwise, the default bandwidth of 1969 MBps will be
> > used."
> > 
> > But maybe we should not default to that. Maybe we should still default
> > to 985 MBps.
> Perhaps we shouldn't have a default at all..
> 
> E.g. if the gen5 bus may get clogged if we exceed gen4
> limits
> 

So the idea here is that if we happen to run this driver on a new Gen supported
SoC, we have to let the user know that the interconnects are running at a lower
gen speed and it needs attention.

But I think we can simplify it by fixing a default bandwidth, say Gen3 and get
rid of the fallthrough. And yeah, the same needs to be done for the pcie-qcom-ep
driver as well.

- Mani

> Konrad

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: qcom: Add interconnect bandwidth for PCIe Gen4

[Abel Vesa]

On 23-09-24 18:07:13, Manivannan Sadhasivam wrote:
> PCIe Gen4 supports the interconnect bandwidth of 1969 MBps. So let's add
> the bandwidth support in the driver. Otherwise, the default bandwidth of
> 985 MBps will be used.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Tested-by: Abel Vesa <abel.vesa@linaro.org>

> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 297442c969b6..6853123f92c1 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1384,11 +1384,14 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
>  	case 2:
>  		bw = MBps_to_icc(500);
>  		break;
> +	case 3:
> +		bw = MBps_to_icc(985);
> +		break;
>  	default:
>  		WARN_ON_ONCE(1);
>  		fallthrough;
> -	case 3:
> -		bw = MBps_to_icc(985);
> +	case 4:
> +		bw = MBps_to_icc(1969);
>  		break;
>  	}
>  
> -- 
> 2.25.1
> 

Re: [PATCH] PCI: qcom: Add interconnect bandwidth for PCIe Gen4

[Bjorn Helgaas]

On Sun, Sep 24, 2023 at 06:07:13PM +0200, Manivannan Sadhasivam wrote:
> PCIe Gen4 supports the interconnect bandwidth of 1969 MBps. So let's add
> the bandwidth support in the driver. Otherwise, the default bandwidth of
> 985 MBps will be used.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 297442c969b6..6853123f92c1 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1384,11 +1384,14 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
>  	case 2:
>  		bw = MBps_to_icc(500);
>  		break;
> +	case 3:
> +		bw = MBps_to_icc(985);
> +		break;
>  	default:
>  		WARN_ON_ONCE(1);
>  		fallthrough;
> -	case 3:
> -		bw = MBps_to_icc(985);
> +	case 4:
> +		bw = MBps_to_icc(1969);

The bare numbers here are sort of weird.  I assume they correspond to
the Supported Link Speeds Vector in Link Cap 2, and I expected them to
correspond somehow to PCIE_SPEED2MBS_ENC(), which computes the usable
PCIe bandwidth per lane.  I see the ratios between 250, 500, 986, 1969
*do* match up with the ratios of PCIE_SPEED2MBS_ENC() values, but I
don't know the PCIE_SPEED2MBS_ENC() values aren't used:

            SLS Vector                         PCIE_SPEED2MBS_ENC()
  CLS 1:  bit 0  2.5 GT/s   MBps_to_icc(250)      2000 Mbps
  CLS 2:  bit 1  5.0 GT/s   MBps_to_icc(500)      4000 Mbps
  CLS 3:  bit 2  8.0 GT/s   MBps_to_icc(985)      7879 Mbps
  CLS 4:  bit 3 16.0 GT/s   MBps_to_icc(1969)    15753 Mbps

This is just my curiosity, probably no change is needed, or at most a
short comment.

I do notice that pcie-qcom-ep.c uses #defines like PCIE_GEN1_BW_MBPS,
and it seems like both could use the same style.

Also agree with Konrad that the ordering ends up looking unusual;
maybe would be more readable if the default case repeated the speed
you want instead of using the fallthrough.

>  		break;
>  	}

Re: [PATCH] PCI: qcom: Add interconnect bandwidth for PCIe Gen4

[Manivannan Sadhasivam]

On Tue, Sep 26, 2023 at 04:08:23PM -0500, Bjorn Helgaas wrote:
> On Sun, Sep 24, 2023 at 06:07:13PM +0200, Manivannan Sadhasivam wrote:
> > PCIe Gen4 supports the interconnect bandwidth of 1969 MBps. So let's add
> > the bandwidth support in the driver. Otherwise, the default bandwidth of
> > 985 MBps will be used.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 297442c969b6..6853123f92c1 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -1384,11 +1384,14 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
> >  	case 2:
> >  		bw = MBps_to_icc(500);
> >  		break;
> > +	case 3:
> > +		bw = MBps_to_icc(985);
> > +		break;
> >  	default:
> >  		WARN_ON_ONCE(1);
> >  		fallthrough;
> > -	case 3:
> > -		bw = MBps_to_icc(985);
> > +	case 4:
> > +		bw = MBps_to_icc(1969);
> 
> The bare numbers here are sort of weird.  I assume they correspond to
> the Supported Link Speeds Vector in Link Cap 2, and I expected them to
> correspond somehow to PCIE_SPEED2MBS_ENC(), which computes the usable
> PCIe bandwidth per lane.  I see the ratios between 250, 500, 986, 1969
> *do* match up with the ratios of PCIE_SPEED2MBS_ENC() values, but I
> don't know the PCIE_SPEED2MBS_ENC() values aren't used:
> 
>             SLS Vector                         PCIE_SPEED2MBS_ENC()
>   CLS 1:  bit 0  2.5 GT/s   MBps_to_icc(250)      2000 Mbps
>   CLS 2:  bit 1  5.0 GT/s   MBps_to_icc(500)      4000 Mbps
>   CLS 3:  bit 2  8.0 GT/s   MBps_to_icc(985)      7879 Mbps
>   CLS 4:  bit 3 16.0 GT/s   MBps_to_icc(1969)    15753 Mbps
> 
> This is just my curiosity, probably no change is needed, or at most a
> short comment.
> 

You are right. I'm not aware of this macro before and yes, I can make use of it.

> I do notice that pcie-qcom-ep.c uses #defines like PCIE_GEN1_BW_MBPS,
> and it seems like both could use the same style.
> 
> Also agree with Konrad that the ordering ends up looking unusual;
> maybe would be more readable if the default case repeated the speed
> you want instead of using the fallthrough.
> 

Yes, that would be more readable.

- Mani

> >  		break;
> >  	}

-- 
மணிவண்ணன் சதாசிவம்