[v3,2/3] PCI: qcom-ep: Make use of PCIE_SPEED2MBS_ENC() macro for encoding link speed
Commit Message
Instead of hardcoding the link speed in MBps, let's make use of the
existing PCIE_SPEED2MBS_ENC() macro that does the encoding of the
link speed for us. Also, let's Wrap it with QCOM_PCIE_LINK_SPEED_TO_BW()
macro to do the conversion to ICC speed.
This eliminates the need for a switch case in qcom_pcie_icc_update() and
also works for future Gen speeds without any code modifications.
Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
Changes in v3:
- New patch
drivers/pci/controller/dwc/pcie-qcom-ep.c | 31 +++++------------------
1 file changed, 6 insertions(+), 25 deletions(-)
Comments
On Wed, Sep 27, 2023 at 12:55:42PM -0500, Bjorn Helgaas wrote:
> On Wed, Sep 27, 2023 at 05:46:02PM +0200, Manivannan Sadhasivam wrote:
> > Instead of hardcoding the link speed in MBps, let's make use of the
> > existing PCIE_SPEED2MBS_ENC() macro that does the encoding of the
> > link speed for us. Also, let's Wrap it with QCOM_PCIE_LINK_SPEED_TO_BW()
> > macro to do the conversion to ICC speed.
>
> In subject, /Make use of/Use/ would make better use of the subject
> line. Lots of "uses" there :)
>
> Above, s/let's//, and also s/make use of/use/.
>
Ok, I will reword accordingly.
> > +#define QCOM_PCIE_LINK_SPEED_TO_BW(speed) \
> > + Mbps_to_icc(PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]))
>
> It's a shame to have to duplicate this macro in pcie-qcom.c and in
> pcie-qcom-ep.c, especially since there's nothing qcom-specific in it.
>
> Would a macro like this fit in interconnect.h?
>
Unfortunately, no. This macro is neither interconnect specific nor PCI, but a
kind of both used by the driver. So it makes sense to keep it in the driver
itself for now.
If we happen to create a common header for host and ep drivers in the future, it
can be moved to that.
> > - ret = icc_set_bw(pcie_ep->icc_mem, 0, MBps_to_icc(PCIE_GEN1_BW_MBPS));
> > + ret = icc_set_bw(pcie_ep->icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
>
> "1" is not very informative here. Maybe PCIE_SPEED_2_5GT? (I didn't
> completely verify that this is equivalent.)
>
No. PCIE_SPEED_2_5GT is defined as 0x14 in pci.h. And I do not want to add a
macro for just "1" here.
- Mani
> Bjorn
On Thu, Sep 28, 2023 at 08:48:08PM +0200, Manivannan Sadhasivam wrote:
> On Wed, Sep 27, 2023 at 12:55:42PM -0500, Bjorn Helgaas wrote:
> > On Wed, Sep 27, 2023 at 05:46:02PM +0200, Manivannan Sadhasivam wrote:
> > > Instead of hardcoding the link speed in MBps, let's make use of the
> > > existing PCIE_SPEED2MBS_ENC() macro that does the encoding of the
> > > link speed for us. Also, let's Wrap it with QCOM_PCIE_LINK_SPEED_TO_BW()
> > > macro to do the conversion to ICC speed.
> > > - ret = icc_set_bw(pcie_ep->icc_mem, 0, MBps_to_icc(PCIE_GEN1_BW_MBPS));
> > > + ret = icc_set_bw(pcie_ep->icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
> >
> > "1" is not very informative here. Maybe PCIE_SPEED_2_5GT? (I didn't
> > completely verify that this is equivalent.)
>
> No. PCIE_SPEED_2_5GT is defined as 0x14 in pci.h. And I do not want to add a
> macro for just "1" here.
Is there no other existing macro that contains the 2.5GT/s hint?
On Thu, Sep 28, 2023 at 04:27:57PM -0500, Bjorn Helgaas wrote:
> On Thu, Sep 28, 2023 at 08:48:08PM +0200, Manivannan Sadhasivam wrote:
> > On Wed, Sep 27, 2023 at 12:55:42PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Sep 27, 2023 at 05:46:02PM +0200, Manivannan Sadhasivam wrote:
> > > > Instead of hardcoding the link speed in MBps, let's make use of the
> > > > existing PCIE_SPEED2MBS_ENC() macro that does the encoding of the
> > > > link speed for us. Also, let's Wrap it with QCOM_PCIE_LINK_SPEED_TO_BW()
> > > > macro to do the conversion to ICC speed.
>
> > > > - ret = icc_set_bw(pcie_ep->icc_mem, 0, MBps_to_icc(PCIE_GEN1_BW_MBPS));
> > > > + ret = icc_set_bw(pcie_ep->icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
> > >
> > > "1" is not very informative here. Maybe PCIE_SPEED_2_5GT? (I didn't
> > > completely verify that this is equivalent.)
> >
> > No. PCIE_SPEED_2_5GT is defined as 0x14 in pci.h. And I do not want to add a
> > macro for just "1" here.
>
> Is there no other existing macro that contains the 2.5GT/s hint?
I couldn't find any :/
Adding a new macro would collide with the existing one IMO.
- Mani
@@ -23,6 +23,7 @@
#include <linux/reset.h>
#include <linux/module.h>
+#include "../../pci.h"
#include "pcie-designware.h"
/* PARF registers */
@@ -135,10 +136,8 @@
#define CORE_RESET_TIME_US_MAX 1005
#define WAKE_DELAY_US 2000 /* 2 ms */
-#define PCIE_GEN1_BW_MBPS 250
-#define PCIE_GEN2_BW_MBPS 500
-#define PCIE_GEN3_BW_MBPS 985
-#define PCIE_GEN4_BW_MBPS 1969
+#define QCOM_PCIE_LINK_SPEED_TO_BW(speed) \
+ Mbps_to_icc(PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]))
#define to_pcie_ep(x) dev_get_drvdata((x)->dev)
@@ -266,7 +265,7 @@ static void qcom_pcie_dw_stop_link(struct dw_pcie *pci)
static void qcom_pcie_ep_icc_update(struct qcom_pcie_ep *pcie_ep)
{
struct dw_pcie *pci = &pcie_ep->pci;
- u32 offset, status, bw;
+ u32 offset, status;
int speed, width;
int ret;
@@ -279,25 +278,7 @@ static void qcom_pcie_ep_icc_update(struct qcom_pcie_ep *pcie_ep)
speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
- switch (speed) {
- case 1:
- bw = MBps_to_icc(PCIE_GEN1_BW_MBPS);
- break;
- case 2:
- bw = MBps_to_icc(PCIE_GEN2_BW_MBPS);
- break;
- case 3:
- bw = MBps_to_icc(PCIE_GEN3_BW_MBPS);
- break;
- default:
- dev_warn(pci->dev, "using default GEN4 bandwidth\n");
- fallthrough;
- case 4:
- bw = MBps_to_icc(PCIE_GEN4_BW_MBPS);
- break;
- }
-
- ret = icc_set_bw(pcie_ep->icc_mem, 0, width * bw);
+ ret = icc_set_bw(pcie_ep->icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
if (ret)
dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
ret);
@@ -335,7 +316,7 @@ static int qcom_pcie_enable_resources(struct qcom_pcie_ep *pcie_ep)
* Set an initial peak bandwidth corresponding to single-lane Gen 1
* for the pcie-mem path.
*/
- ret = icc_set_bw(pcie_ep->icc_mem, 0, MBps_to_icc(PCIE_GEN1_BW_MBPS));
+ ret = icc_set_bw(pcie_ep->icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
if (ret) {
dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
ret);