diff mbox series

PCI: qcom: Add interconnect bandwidth for PCIe Gen4

Message ID 20230924160713.217086-1-manivannan.sadhasivam@linaro.org
State New
Headers show
Series PCI: qcom: Add interconnect bandwidth for PCIe Gen4 | expand

Commit Message

Manivannan Sadhasivam Sept. 24, 2023, 4:07 p.m. UTC 
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(-)

Comments

Konrad Dybcio Sept. 25, 2023, 8:57 a.m. UTC | #1 
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
Abel Vesa Sept. 25, 2023, 10:33 a.m. UTC | #2 
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
Konrad Dybcio Sept. 25, 2023, 10:34 a.m. UTC | #3 
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
Abel Vesa Sept. 25, 2023, 10:35 a.m. UTC | #4 
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
>
Abel Vesa Sept. 25, 2023, 10:37 a.m. UTC | #5 
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
Konrad Dybcio Sept. 25, 2023, 10:40 a.m. UTC | #6 
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
Bjorn Helgaas Sept. 26, 2023, 9:08 p.m. UTC | #7 
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;
>  	}
Manivannan Sadhasivam Sept. 27, 2023, 1:04 p.m. UTC | #8 
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;
> >  	}
diff mbox series

Patch

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;
 	}