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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>>>>> ---
>>>>> 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
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> >>> ---
> >>> 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
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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
Tested-by: Abel Vesa <[email protected]>
> ---
> 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
>
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 <[email protected]>
> ---
> 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;
> }
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 <[email protected]>
> >>>>> ---
> >>>>> 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
--
மணிவண்ணன் சதாசிவம்
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 <[email protected]>
> > ---
> > 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;
> > }
--
மணிவண்ணன் சதாசிவம்