On Tue, 10 Dec 2024 13:08:20 +0900
"Kobayashi,Daisuke" <kobayashi.da-06@xxxxxxxxxxx> 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@xxxxxxxxxxx>
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
