Re: [PATCH v5 1/2] Add helper functions for Power Budgeting Extended Capability

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Tue, 10 Dec 2024 13:08:20 +0900
"Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com> wrote:

> Add functions to return a text description of the supplied
> power_budget scale/base power/rail.
> Export these functions so they can be used by modules.
> 
> Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>

Hi.  A few comments inline,

Thanks,

Jonathan

> ---
>  drivers/pci/pci.h             |  3 ++
>  drivers/pci/probe.c           | 66 +++++++++++++++++++++++++++++++++++
>  include/uapi/linux/pci_regs.h |  3 +-
>  3 files changed, 71 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 14d00ce45bfa..967b53996694 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -374,6 +374,9 @@ static inline int pcie_dev_speed_mbps(enum pci_bus_speed speed)
>  }
>  
>  const char *pci_speed_string(enum pci_bus_speed speed);
> +const char *pci_power_budget_scale_string(u8 num);
> +const char *pci_power_budget_alt_encode_string(u8 num);
> +const char *pci_power_budget_rail_string(u8 num);
>  enum pci_bus_speed pcie_get_speed_cap(struct pci_dev *dev);
>  enum pcie_link_width pcie_get_width_cap(struct pci_dev *dev);
>  void __pcie_print_link_status(struct pci_dev *dev, bool verbose);
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index f1615805f5b0..18a920527f69 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -748,6 +748,72 @@ void pcie_update_link_speed(struct pci_bus *bus, u16 linksta)
>  }
>  EXPORT_SYMBOL_GPL(pcie_update_link_speed);
>  
> +const char *pci_power_budget_rail_string(u8 num)
> +{
> +	/* Indexed by the rail number */
> +	static const char *rail_strings[] = {
> +	    "Power(12V)",		/* 0x00 */

For these you could do
	[0x00] = "Power(12V"), rather than the comment?
Makes the code self documenting and keeps it easy to compare
with the specification.


> +	    "Power(3.3V)",		/* 0x01 */
> +	    "Power(1.5Vor1.8V)",		/* 0x02 */
> +	    "Power(48V)",		/* 0x03 */
> +	    "Power(5V)",		/* 0x04 */
> +	    "Thermal",			/* 0x05 */
> +	};
> +
> +	if (num < ARRAY_SIZE(rail_strings))
> +		return rail_strings[num];
> +	return "Unknown";
> +}
> +EXPORT_SYMBOL_GPL(pci_power_budget_rail_string);
> +
> +const char *pci_power_budget_scale_string(u8 num)
> +{
> +	/* Indexed by the scale number */
> +	static const char *scale_strings[] = {
As above.

> +	    "x1.0",		/* 0x00 */
> +	    "x0.1",		/* 0x01 */
> +	    "x0.01",		/* 0x02 */
> +	    "x0.001",		/* 0x03 */
> +	    "x10",		/* 0x04 */
> +	    "x100",			/* 0x05 */
> +	};
> +
> +	if (num < ARRAY_SIZE(scale_strings))
> +		return scale_strings[num];
> +	return "Unknown";
> +}
> +EXPORT_SYMBOL_GPL(pci_power_budget_scale_string);
> +
> +const char *pci_power_budget_alt_encode_string(u8 num)
> +{
> +	u8 n;
> +	n = num & 0x0f;

	u8 n = num & 0x0f;

> +	/* Indexed by the Base Power number */
> +	static const char *Power_strings[] = {
> +	    "> 239 W and ≤ 250 W Slot Power Limit",		/* 0xF0 */
> +	    "> 250 W and ≤ 275 W Slot Power Limit",		/* 0xF1 */
> +	    "> 275 W and ≤ 300 W Slot Power Limit",		/* 0xF2 */
> +	    "> 300 W and ≤ 325 W Slot Power Limit",		/* 0xF3 */
> +	    "> 325 W and ≤ 350 W Slot Power Limit",		/* 0xF4 */
> +	    "> 350 W and ≤ 375 W Slot Power Limit",		/* 0xF5 */
> +	    "> 375 W and ≤ 400 W Slot Power Limit",		/* 0xF6 */
> +	    "> 400 W and ≤ 425 W Slot Power Limit",		/* 0xF7 */
> +	    "> 425 W and ≤ 450 W Slot Power Limit",		/* 0xF8 */
> +	    "> 450 W and ≤ 475 W Slot Power Limit",		/* 0xF9 */
> +	    "> 475 W and ≤ 500 W Slot Power Limit",		/* 0xFA */
> +	    "> 500 W and ≤ 525 W Slot Power Limit",		/* 0xFB */
> +	    "> 525 W and ≤ 550 W Slot Power Limit",		/* 0xFC */
> +	    "> 550 W and ≤ 575 W Slot Power Limit",		/* 0xFD */
> +	    "> 575 W and ≤ 600 W Slot Power Limit",		/* 0xFE */
> +	    "Greater than 600 W",		/* 0xFF */

Technically the spec says it's "Reserved for values greater than 600 W", but
which I assume they mean a new interface will be needed if you see 0xFF.
I guess your text works for that though.

> +	};
> +
> +	if (n < ARRAY_SIZE(Power_strings))
> +		return Power_strings[n];
> +	return "Unknown";
> +}
> +EXPORT_SYMBOL_GPL(pci_power_budget_alt_encode_string);

Why do you need the export? Aren't all the users in the core PCI code?

> +
>  static unsigned char agp_speeds[] = {
>  	AGP_UNKNOWN,
>  	AGP_1X,
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 12323b3334a9..3a5e238b98d8 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -842,11 +842,12 @@
>  #define PCI_PWR_DSR		0x04	/* Data Select Register */
>  #define PCI_PWR_DATA		0x08	/* Data Register */
>  #define  PCI_PWR_DATA_BASE(x)	((x) & 0xff)	    /* Base Power */
> -#define  PCI_PWR_DATA_SCALE(x)	(((x) >> 8) & 3)    /* Data Scale */
> +#define  PCI_PWR_DATA_SCALE(x)	(((x) >> 8) & 3)    /* Data Scale[1:0] */
>  #define  PCI_PWR_DATA_PM_SUB(x)	(((x) >> 10) & 7)   /* PM Sub State */
>  #define  PCI_PWR_DATA_PM_STATE(x) (((x) >> 13) & 3) /* PM State */
>  #define  PCI_PWR_DATA_TYPE(x)	(((x) >> 15) & 7)   /* Type */
>  #define  PCI_PWR_DATA_RAIL(x)	(((x) >> 18) & 7)   /* Power Rail */
> +#define  PCI_PWR_DATA_SCALE_UP(x)	(((x) >> 21) & 1)    /* Data Scale[2] */
>  #define PCI_PWR_CAP		0x0c	/* Capability */
>  #define  PCI_PWR_CAP_BUDGET(x)	((x) & 1)	/* Included in system budget */
>  #define PCI_EXT_CAP_PWR_SIZEOF	0x10