| Message ID | 20240223-opp_support-v7-6-10b4363d7e71@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Manivannan Sadhasivam |
| Headers | show |
| Series | PCI: qcom: Add support for OPP | expand |
Mention the new interface name in the subject and in the commit log. s/pcie/PCIe/ The subject says "to MBps", but the commit log says "to frequency". On Fri, Feb 23, 2024 at 08:18:03PM +0530, Krishna chaitanya chundru wrote: > Bring the switch case in pcie_link_speed_mbps to new function to > the header file so that it can be used in other places like > in controller driver. s/pcie_link_speed_mbps/pcie_link_speed_mbps()/ to identify it as a function. > Create a new macro to convert from MBps to frequency. Include the new macro name here. I think pcie_link_speed_mbps() returns Mb/s (mega*bits* per second), not MB/s (mega*bytes* per second). > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > --- > drivers/pci/pci.c | 19 +------------------ > drivers/pci/pci.h | 24 ++++++++++++++++++++++++ > 2 files changed, 25 insertions(+), 18 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index d8f11a078924..b441ab862a8d 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -6309,24 +6309,7 @@ int pcie_link_speed_mbps(struct pci_dev *pdev) > if (err) > return err; > > - switch (to_pcie_link_speed(lnksta)) { > - case PCIE_SPEED_2_5GT: > - return 2500; > - case PCIE_SPEED_5_0GT: > - return 5000; > - case PCIE_SPEED_8_0GT: > - return 8000; > - case PCIE_SPEED_16_0GT: > - return 16000; > - case PCIE_SPEED_32_0GT: > - return 32000; > - case PCIE_SPEED_64_0GT: > - return 64000; > - default: > - break; > - } > - > - return -EINVAL; > + return pcie_link_speed_to_mbps(to_pcie_link_speed(lnksta)); > } > EXPORT_SYMBOL(pcie_link_speed_mbps); > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index 2336a8d1edab..82e715ebe383 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -282,6 +282,30 @@ void pci_bus_put(struct pci_bus *bus); > (speed) == PCIE_SPEED_2_5GT ? 2500*8/10 : \ > 0) > > +static inline int pcie_link_speed_to_mbps(enum pci_bus_speed speed) > +{ > + switch (speed) { > + case PCIE_SPEED_2_5GT: > + return 2500; > + case PCIE_SPEED_5_0GT: > + return 5000; > + case PCIE_SPEED_8_0GT: > + return 8000; > + case PCIE_SPEED_16_0GT: > + return 16000; > + case PCIE_SPEED_32_0GT: > + return 32000; > + case PCIE_SPEED_64_0GT: > + return 64000; > + default: > + break; > + } > + > + return -EINVAL; > +} > + > +#define PCIE_MBS2FREQ(speed) (pcie_link_speed_to_mbps(speed) * 1000) I feel like I might have asked some of this before; if so, my apologies and maybe a comment would be useful here to save answering again. The MBS2FREQ name suggests that "speed" is Mb/s, but it's not; it's an enum pci_bus_speed just like PCIE_SPEED2MBS_ENC() takes. When PCI SIG defines a new data rate, PCIE_MBS2FREQ() will do something completely wrong when pcie_link_speed_to_mbps() returns -EINVAL. I think it would be better to do this in a way that we can warn about the unknown speed and fall back to some reasonable default instead of whatever (-EINVAL * 1000) works out to. PCIE_MBS2FREQ() looks an awful lot like PCIE_SPEED2MBS_ENC(), except that it doesn't adjust for the encoding overhead and it multiplies by 1000. I don't know what that result means. The name suggests a frequency? pcie_link_speed_to_mbps(PCIE_SPEED_2_5GT) == 2500 Mbit/s (raw data rate) PCIE_SPEED2MBS_ENC(PCIE_SPEED_2_5GT) == 2000 Mbit/s or 2 Gbit/s (effective data rate) PCIE_MBS2FREQ(PCIE_SPEED_2_5GT) == 2500000 (? 2.5M of something) I don't really know how OPP works, but it looks like maybe PCIE_MBS2FREQ() is a shim that depends on how the OPP tables in DT are encoded? I'm surprised that the DT OPP tables aren't encoded with either the raw data rate or the effective data rate directly instead of what looks like the raw data rate / 1000. Is this a standard OPP encoding that will apply to other drivers? If so, it would be helpful to point to where that encoding is defined. If not, PCIE_MBS2FREQ() should probably be defined in pcie-qcom.c. Bjorn
On 2/28/2024 5:55 AM, Bjorn Helgaas wrote: > Mention the new interface name in the subject and in the commit log. > > s/pcie/PCIe/ > > The subject says "to MBps", but the commit log says "to frequency". > > On Fri, Feb 23, 2024 at 08:18:03PM +0530, Krishna chaitanya chundru wrote: >> Bring the switch case in pcie_link_speed_mbps to new function to >> the header file so that it can be used in other places like >> in controller driver. > > s/pcie_link_speed_mbps/pcie_link_speed_mbps()/ to identify it as a > function. > >> Create a new macro to convert from MBps to frequency. > > Include the new macro name here. > > I think pcie_link_speed_mbps() returns Mb/s (mega*bits* per second), > not MB/s (mega*bytes* per second). > >> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >> --- >> drivers/pci/pci.c | 19 +------------------ >> drivers/pci/pci.h | 24 ++++++++++++++++++++++++ >> 2 files changed, 25 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index d8f11a078924..b441ab862a8d 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -6309,24 +6309,7 @@ int pcie_link_speed_mbps(struct pci_dev *pdev) >> if (err) >> return err; >> >> - switch (to_pcie_link_speed(lnksta)) { >> - case PCIE_SPEED_2_5GT: >> - return 2500; >> - case PCIE_SPEED_5_0GT: >> - return 5000; >> - case PCIE_SPEED_8_0GT: >> - return 8000; >> - case PCIE_SPEED_16_0GT: >> - return 16000; >> - case PCIE_SPEED_32_0GT: >> - return 32000; >> - case PCIE_SPEED_64_0GT: >> - return 64000; >> - default: >> - break; >> - } >> - >> - return -EINVAL; >> + return pcie_link_speed_to_mbps(to_pcie_link_speed(lnksta)); >> } >> EXPORT_SYMBOL(pcie_link_speed_mbps); >> >> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h >> index 2336a8d1edab..82e715ebe383 100644 >> --- a/drivers/pci/pci.h >> +++ b/drivers/pci/pci.h >> @@ -282,6 +282,30 @@ void pci_bus_put(struct pci_bus *bus); >> (speed) == PCIE_SPEED_2_5GT ? 2500*8/10 : \ >> 0) >> >> +static inline int pcie_link_speed_to_mbps(enum pci_bus_speed speed) >> +{ >> + switch (speed) { >> + case PCIE_SPEED_2_5GT: >> + return 2500; >> + case PCIE_SPEED_5_0GT: >> + return 5000; >> + case PCIE_SPEED_8_0GT: >> + return 8000; >> + case PCIE_SPEED_16_0GT: >> + return 16000; >> + case PCIE_SPEED_32_0GT: >> + return 32000; >> + case PCIE_SPEED_64_0GT: >> + return 64000; >> + default: >> + break; >> + } >> + >> + return -EINVAL; >> +} >> + >> +#define PCIE_MBS2FREQ(speed) (pcie_link_speed_to_mbps(speed) * 1000) > > I feel like I might have asked some of this before; if so, my > apologies and maybe a comment would be useful here to save answering > again. > > The MBS2FREQ name suggests that "speed" is Mb/s, but it's not; it's an > enum pci_bus_speed just like PCIE_SPEED2MBS_ENC() takes. > > When PCI SIG defines a new data rate, PCIE_MBS2FREQ() will do > something completely wrong when pcie_link_speed_to_mbps() returns > -EINVAL. I think it would be better to do this in a way that we can > warn about the unknown speed and fall back to some reasonable default > instead of whatever (-EINVAL * 1000) works out to. > As commented below I will move PCIE_MBS2FREQ to qcom driver and I will take care about -EINVAL in the qcom driver itself. > PCIE_MBS2FREQ() looks an awful lot like PCIE_SPEED2MBS_ENC(), except > that it doesn't adjust for the encoding overhead and it multiplies by > 1000. I don't know what that result means. The name suggests a > frequency? > > pcie_link_speed_to_mbps(PCIE_SPEED_2_5GT) == 2500 Mbit/s (raw data rate) > PCIE_SPEED2MBS_ENC(PCIE_SPEED_2_5GT) == 2000 Mbit/s or 2 Gbit/s (effective data rate) > PCIE_MBS2FREQ(PCIE_SPEED_2_5GT) == 2500000 (? 2.5M of something) > > I don't really know how OPP works, but it looks like maybe > PCIE_MBS2FREQ() is a shim that depends on how the OPP tables in DT are > encoded? I'm surprised that the DT OPP tables aren't encoded with > either the raw data rate or the effective data rate directly instead > of what looks like the raw data rate / 1000. > > Is this a standard OPP encoding that will apply to other drivers? If > so, it would be helpful to point to where that encoding is defined. > If not, PCIE_MBS2FREQ() should probably be defined in pcie-qcom.c. > It depends on how driver use OPP, I think as you suggested PCIE_MBS2FREQ should belong to pcie-qcom.c as no other driver is using it for now. I will move to pcie_qcom.c in my next series. - Krishna Chaitanya. > Bjorn >
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index d8f11a078924..b441ab862a8d 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -6309,24 +6309,7 @@ int pcie_link_speed_mbps(struct pci_dev *pdev) if (err) return err; - switch (to_pcie_link_speed(lnksta)) { - case PCIE_SPEED_2_5GT: - return 2500; - case PCIE_SPEED_5_0GT: - return 5000; - case PCIE_SPEED_8_0GT: - return 8000; - case PCIE_SPEED_16_0GT: - return 16000; - case PCIE_SPEED_32_0GT: - return 32000; - case PCIE_SPEED_64_0GT: - return 64000; - default: - break; - } - - return -EINVAL; + return pcie_link_speed_to_mbps(to_pcie_link_speed(lnksta)); } EXPORT_SYMBOL(pcie_link_speed_mbps); diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 2336a8d1edab..82e715ebe383 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -282,6 +282,30 @@ void pci_bus_put(struct pci_bus *bus); (speed) == PCIE_SPEED_2_5GT ? 2500*8/10 : \ 0) +static inline int pcie_link_speed_to_mbps(enum pci_bus_speed speed) +{ + switch (speed) { + case PCIE_SPEED_2_5GT: + return 2500; + case PCIE_SPEED_5_0GT: + return 5000; + case PCIE_SPEED_8_0GT: + return 8000; + case PCIE_SPEED_16_0GT: + return 16000; + case PCIE_SPEED_32_0GT: + return 32000; + case PCIE_SPEED_64_0GT: + return 64000; + default: + break; + } + + return -EINVAL; +} + +#define PCIE_MBS2FREQ(speed) (pcie_link_speed_to_mbps(speed) * 1000) + const char *pci_speed_string(enum pci_bus_speed speed); enum pci_bus_speed pcie_get_speed_cap(struct pci_dev *dev); enum pcie_link_width pcie_get_width_cap(struct pci_dev *dev);
Bring the switch case in pcie_link_speed_mbps to new function to the header file so that it can be used in other places like in controller driver. Create a new macro to convert from MBps to frequency. Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> --- drivers/pci/pci.c | 19 +------------------ drivers/pci/pci.h | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+), 18 deletions(-)