Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Wed, 27 Sep 2023 15:04:55 +0200
From:   Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     lpieralisi@kernel.org, kw@linux.com, andersson@kernel.org,
        konrad.dybcio@linaro.org, bhelgaas@google.com,
        linux-arm-msm@vger.kernel.org, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org, abel.vesa@linaro.org
Subject: Re: [PATCH] PCI: qcom: Add interconnect bandwidth for PCIe Gen4
Message-ID: <20230927130455.GB19623@thinkpad>
References: <20230924160713.217086-1-manivannan.sadhasivam@linaro.org>
 <20230926210823.GA427669@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230926210823.GA427669@bhelgaas>
Precedence: bulk

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

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