diff mbox series

[v14,3/4] PCI: Bring the PCIe speed to MBps logic to new pcie_link_speed_to_mbps()

Message ID 20240609-opp_support-v14-3-801cff862b5a@quicinc.com (mailing list archive)
State Superseded
Headers show
Series PCI: qcom: Add support for OPP | expand

Commit Message

Krishna Chaitanya Chundru June 9, 2024, 2:38 a.m. UTC 
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.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c | 19 +------------------
 drivers/pci/pci.h | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+), 18 deletions(-)

Comments

Ilpo Järvinen June 14, 2024, 12:32 p.m. UTC | #1 
On Sun, 9 Jun 2024, 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.
> 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pci.c | 19 +------------------
>  drivers/pci/pci.h | 22 ++++++++++++++++++++++
>  2 files changed, 23 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index d2c388761ba9..6e50fa89b913 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6027,24 +6027,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));

pcie_link_speed_mbps() calls pcie_link_speed_to_mbps(), seems quite 
confusing to me. Perhaps renaming one to pcie_dev_speed_mbps() would help
against the almost identical naming.

In general, I don't like moving that code into a header file, did you 
check how large footprint the new function is (when it's not inlined)?

Unrelated to this patch, it would be nice if LNKSTA register read would 
not be needed at all here but since cur_bus_speed is what it is currently, 
it's just wishful thinking.

>  }
>  EXPORT_SYMBOL(pcie_link_speed_mbps);
>  
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 1b021579f26a..391a5cd388bd 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -333,6 +333,28 @@ 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;
> +}
Krishna Chaitanya Chundru June 19, 2024, 2:30 p.m. UTC | #2 
On 6/14/2024 6:02 PM, Ilpo Järvinen wrote:
> On Sun, 9 Jun 2024, 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.
>>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>> ---
>>   drivers/pci/pci.c | 19 +------------------
>>   drivers/pci/pci.h | 22 ++++++++++++++++++++++
>>   2 files changed, 23 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index d2c388761ba9..6e50fa89b913 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -6027,24 +6027,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));
> 
> pcie_link_speed_mbps() calls pcie_link_speed_to_mbps(), seems quite
> confusing to me. Perhaps renaming one to pcie_dev_speed_mbps() would help
> against the almost identical naming.
> 
Ack I will modify in the next patch.
> In general, I don't like moving that code into a header file, did you
> check how large footprint the new function is (when it's not inlined)?
> 
I checked with and without inline and I don't see any difference in both
cases
aarch64-linux-gnu-size ../drivers/pci/pci.o
    text    data     bss     dec     hex filename
   41440    1334      64   42838    a756 ../kobj/drivers/pci/pci.o

   text    data     bss     dec     hex filename
   41440    1334      64   42838    a756 ../kobj/drivers/pci/pci.o

- Krishna chaitanya.
> Unrelated to this patch, it would be nice if LNKSTA register read would
> not be needed at all here but since cur_bus_speed is what it is currently,
> it's just wishful thinki
> 
>>   }
>>   EXPORT_SYMBOL(pcie_link_speed_mbps);
>>   
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 1b021579f26a..391a5cd388bd 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -333,6 +333,28 @@ 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;
>> +}
> 
> 
>
Krishna Chaitanya Chundru June 19, 2024, 2:54 p.m. UTC | #3 
On 6/14/2024 6:02 PM, Ilpo Järvinen wrote:
> On Sun, 9 Jun 2024, 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.
>>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>> ---
>>   drivers/pci/pci.c | 19 +------------------
>>   drivers/pci/pci.h | 22 ++++++++++++++++++++++
>>   2 files changed, 23 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index d2c388761ba9..6e50fa89b913 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -6027,24 +6027,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));
> 
> pcie_link_speed_mbps() calls pcie_link_speed_to_mbps(), seems quite
> confusing to me. Perhaps renaming one to pcie_dev_speed_mbps() would help
> against the almost identical naming.
> 
> In general, I don't like moving that code into a header file, did you
> check how large footprint the new function is (when it's not inlined)?
> 
if we remove this patch we see difference of 8, I think it should be fine.
with change
aarch64-linux-gnu-size ../drivers/pci/pci.o
    text    data     bss     dec     hex filename
   41440    1334      64   42838    a756 ../kobj/drivers/pci/pci.o
without the change
text    data     bss     dec     hex filename
   41432    1334      64   42830    a74e ../kobj/drivers/pci/pci.o

- Krishna Chaitanya.
> Unrelated to this patch, it would be nice if LNKSTA register read would
> not be needed at all here but since cur_bus_speed is what it is currently,
> it's just wishful thinking.
> 
>>   }
>>   EXPORT_SYMBOL(pcie_link_speed_mbps);
>>   
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 1b021579f26a..391a5cd388bd 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -333,6 +333,28 @@ 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;
>> +}
> 
> 
>
Ilpo Järvinen June 23, 2024, 3:26 p.m. UTC | #4 
On Wed, 19 Jun 2024, Krishna Chaitanya Chundru wrote:

> 
> 
> On 6/14/2024 6:02 PM, Ilpo Järvinen wrote:
> > On Sun, 9 Jun 2024, 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.
> > > 
> > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > > ---
> > >   drivers/pci/pci.c | 19 +------------------
> > >   drivers/pci/pci.h | 22 ++++++++++++++++++++++
> > >   2 files changed, 23 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index d2c388761ba9..6e50fa89b913 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -6027,24 +6027,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));
> > 
> > pcie_link_speed_mbps() calls pcie_link_speed_to_mbps(), seems quite
> > confusing to me. Perhaps renaming one to pcie_dev_speed_mbps() would help
> > against the almost identical naming.
> > 
> > In general, I don't like moving that code into a header file, did you
> > check how large footprint the new function is (when it's not inlined)?
> > 
> if we remove this patch we see difference of 8, I think it should be fine.
> with change
> aarch64-linux-gnu-size ../drivers/pci/pci.o
>    text    data     bss     dec     hex filename
>   41440    1334      64   42838    a756 ../kobj/drivers/pci/pci.o
> without the change
> text    data     bss     dec     hex filename
>   41432    1334      64   42830    a74e ../kobj/drivers/pci/pci.o

Thanks for checking it out, seems the inline here was a non-problem.
diff mbox series

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d2c388761ba9..6e50fa89b913 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6027,24 +6027,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 1b021579f26a..391a5cd388bd 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -333,6 +333,28 @@  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;
+}
+
 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);