Skip to contentSkip to conversation

PCI: mvebu: Slot support

PATCH Superseded Pali Rohár 4 participants 26 messages Feb 22, 2022 – Mar 1, 2022

Subsystems (4)

v1 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Pali Rohár

author maintainer
#0 lore raw

cover PCI: mvebu: Slot support

This patch series add slot support to pci-mvebu.c driver.

It is based on branch pci/mvebu of git repository:
https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git

Some patches were reviewed in other patch series.

Pali Rohár (6):
PCI: Add PCI_EXP_SLTCTL_ASPL_DISABLE macro
PCI: Add PCI_EXP_SLTCAP_*_SHIFT macros
dt-bindings: Add 'slot-power-limit-milliwatt' PCIe port property
PCI: Add function for parsing 'slot-power-limit-milliwatt' DT property
PCI: mvebu: Add support for sending Set_Slot_Power_Limit message
ARM: dts: turris-omnia: Set PCIe slot-power-limit-milliwatt properties

 Documentation/devicetree/bindings/pci/pci.txt |  6 ++
 arch/arm/boot/dts/armada-385-turris-omnia.dts |  3 +
 drivers/pci/controller/pci-mvebu.c            | 85 +++++++++++++++++--
 drivers/pci/of.c                              | 64 ++++++++++++++
 drivers/pci/pci.h                             | 15 ++++
 include/uapi/linux/pci_regs.h                 |  4 +
 6 files changed, 172 insertions(+), 5 deletions(-)
Signature · 1 line
2.20.1
author maintainer
#1 lore raw

PATCH 1/6 PCI: Add PCI_EXP_SLTCTL_ASPL_DISABLE macro

Add macro defining Auto Slot Power Limit Disable bit in Slot Control Register.

---
 include/uapi/linux/pci_regs.h | 1 +
 1 file changed, 1 insertion(+)
include/uapi/linux/pci_regs.h +1 -0
index bee1a9ed6e66..108f8523fa04 100644
@@ -616,6 +616,7 @@
 #define  PCI_EXP_SLTCTL_PWR_OFF        0x0400 /* Power Off */
 #define  PCI_EXP_SLTCTL_EIC	0x0800	/* Electromechanical Interlock Control */
 #define  PCI_EXP_SLTCTL_DLLSCE	0x1000	/* Data Link Layer State Changed Enable */
+#define  PCI_EXP_SLTCTL_ASPL_DISABLE	0x2000 /* Auto Slot Power Limit Disable */
 #define  PCI_EXP_SLTCTL_IBPD_DISABLE	0x4000 /* In-band PD disable */
 #define PCI_EXP_SLTSTA		0x1a	/* Slot Status */
 #define  PCI_EXP_SLTSTA_ABP	0x0001	/* Attention Button Pressed */
Signature · 1 line
2.20.1
Acked-by on v1 1/6
#2 lore raw

Re: [PATCH 1/6] PCI: Add PCI_EXP_SLTCTL_ASPL_DISABLE macro

Quoted 5 lines · Pali Rohár

Add macro defining Auto Slot Power Limit Disable bit in Slot Control Register.

Quoted 19 lines
---
 include/uapi/linux/pci_regs.h | 1 +
 1 file changed, 1 insertion(+)
include/uapi/linux/pci_regs.h +1 -0
index bee1a9ed6e66..108f8523fa04 100644
@@ -616,6 +616,7 @@
 #define  PCI_EXP_SLTCTL_PWR_OFF        0x0400 /* Power Off */
 #define  PCI_EXP_SLTCTL_EIC	0x0800	/* Electromechanical Interlock Control */
 #define  PCI_EXP_SLTCTL_DLLSCE	0x1000	/* Data Link Layer State Changed Enable */
+#define  PCI_EXP_SLTCTL_ASPL_DISABLE	0x2000 /* Auto Slot Power Limit Disable */
 #define  PCI_EXP_SLTCTL_IBPD_DISABLE	0x4000 /* In-band PD disable */
 #define PCI_EXP_SLTSTA		0x1a	/* Slot Status */
 #define  PCI_EXP_SLTSTA_ABP	0x0001	/* Attention Button Pressed */
Signature · 1 line
2.20.1
author maintainer
#3 lore raw

PATCH 2/6 PCI: Add PCI_EXP_SLTCAP_*_SHIFT macros

In reply to Pali Rohár

These macros allows to easily compose and extract Slot Power Limit and Physical Slot Number values from Slot Capability Register.

---
 include/uapi/linux/pci_regs.h | 3 +++
 1 file changed, 3 insertions(+)
include/uapi/linux/pci_regs.h +3 -0
index 108f8523fa04..3fc9a4cac630 100644
@@ -591,10 +591,13 @@
 #define  PCI_EXP_SLTCAP_HPS	0x00000020 /* Hot-Plug Surprise */
 #define  PCI_EXP_SLTCAP_HPC	0x00000040 /* Hot-Plug Capable */
 #define  PCI_EXP_SLTCAP_SPLV	0x00007f80 /* Slot Power Limit Value */
+#define  PCI_EXP_SLTCAP_SPLV_SHIFT	7  /* Slot Power Limit Value shift */
 #define  PCI_EXP_SLTCAP_SPLS	0x00018000 /* Slot Power Limit Scale */
+#define  PCI_EXP_SLTCAP_SPLS_SHIFT	15 /* Slot Power Limit Scale shift */
 #define  PCI_EXP_SLTCAP_EIP	0x00020000 /* Electromechanical Interlock Present */
 #define  PCI_EXP_SLTCAP_NCCS	0x00040000 /* No Command Completed Support */
 #define  PCI_EXP_SLTCAP_PSN	0xfff80000 /* Physical Slot Number */
+#define  PCI_EXP_SLTCAP_PSN_SHIFT	19 /* Physical Slot Number shift */
 #define PCI_EXP_SLTCTL		0x18	/* Slot Control */
 #define  PCI_EXP_SLTCTL_ABPE	0x0001	/* Attention Button Pressed Enable */
 #define  PCI_EXP_SLTCTL_PFDE	0x0002	/* Power Fault Detected Enable */
Signature · 1 line
2.20.1
#4 lore raw

Re: [PATCH 2/6] PCI: Add PCI_EXP_SLTCAP_*_SHIFT macros

Quoted 14 lines · Pali Rohár

These macros allows to easily compose and extract Slot Power Limit and Physical Slot Number values from Slot Capability Register.

---
 include/uapi/linux/pci_regs.h | 3 +++
 1 file changed, 3 insertions(+)
include/uapi/linux/pci_regs.h +0 -0
index 108f8523fa04..3fc9a4cac630 100644

include/uapi/linux/pci_regs.h line 594

Quoted 4 lines
include/uapi/linux/pci_regs.h +1 -0
@@ -591,10 +591,13 @@
 #define  PCI_EXP_SLTCAP_HPS	0x00000020 /* Hot-Plug Surprise */
 #define  PCI_EXP_SLTCAP_HPC	0x00000040 /* Hot-Plug Capable */
 #define  PCI_EXP_SLTCAP_SPLV	0x00007f80 /* Slot Power Limit Value */
+#define  PCI_EXP_SLTCAP_SPLV_SHIFT	7  /* Slot Power Limit Value shift */

Is there a way to use FIELD_PREP() and FIELD_GET() instead? It seems like that's what the cool kids are doing now.

At first I didn't really like them, but they do remove the need for adding _SHIFT macros that have to be manually related to the other.

Quoted 12 lines
include/uapi/linux/pci_regs.h +2 -0
 #define  PCI_EXP_SLTCAP_SPLS	0x00018000 /* Slot Power Limit Scale */
+#define  PCI_EXP_SLTCAP_SPLS_SHIFT	15 /* Slot Power Limit Scale shift */
 #define  PCI_EXP_SLTCAP_EIP	0x00020000 /* Electromechanical Interlock Present */
 #define  PCI_EXP_SLTCAP_NCCS	0x00040000 /* No Command Completed Support */
 #define  PCI_EXP_SLTCAP_PSN	0xfff80000 /* Physical Slot Number */
+#define  PCI_EXP_SLTCAP_PSN_SHIFT	19 /* Physical Slot Number shift */
 #define PCI_EXP_SLTCTL		0x18	/* Slot Control */
 #define  PCI_EXP_SLTCTL_ABPE	0x0001	/* Attention Button Pressed Enable */
 #define  PCI_EXP_SLTCTL_PFDE	0x0002	/* Power Fault Detected Enable */
Signature · 1 line
2.20.1
author maintainer
#5 lore raw

Re: [PATCH 2/6] PCI: Add PCI_EXP_SLTCAP_*_SHIFT macros

Quoted 10 lines · On Thursday 24 February 2022 14:28:43 Bjorn Helgaas wrote:
Quoted 9 lines · Pali Rohár

These macros allows to easily compose and extract Slot Power Limit and Physical Slot Number values from Slot Capability Register.

---
 include/uapi/linux/pci_regs.h | 3 +++
 1 file changed, 3 insertions(+)
Quoted 12 lines

include/uapi/linux/pci_regs.h line 594

Quoted 9 lines
include/uapi/linux/pci_regs.h +1 -0
index 108f8523fa04..3fc9a4cac630 100644
@@ -591,10 +591,13 @@
 #define  PCI_EXP_SLTCAP_HPS	0x00000020 /* Hot-Plug Surprise */
 #define  PCI_EXP_SLTCAP_HPC	0x00000040 /* Hot-Plug Capable */
 #define  PCI_EXP_SLTCAP_SPLV	0x00007f80 /* Slot Power Limit Value */
+#define  PCI_EXP_SLTCAP_SPLV_SHIFT	7  /* Slot Power Limit Value shift */

Is there a way to use FIELD_PREP() and FIELD_GET() instead? It seems like that's what the cool kids are doing now.

This is possible too.

I have proposed a patch with _SHIFT macros as this is the way how are other macros in this file defined and used.

Quoted 15 lines

At first I didn't really like them, but they do remove the need for adding _SHIFT macros that have to be manually related to the other.

Quoted 12 lines
include/uapi/linux/pci_regs.h +2 -0
 #define  PCI_EXP_SLTCAP_SPLS	0x00018000 /* Slot Power Limit Scale */
+#define  PCI_EXP_SLTCAP_SPLS_SHIFT	15 /* Slot Power Limit Scale shift */
 #define  PCI_EXP_SLTCAP_EIP	0x00020000 /* Electromechanical Interlock Present */
 #define  PCI_EXP_SLTCAP_NCCS	0x00040000 /* No Command Completed Support */
 #define  PCI_EXP_SLTCAP_PSN	0xfff80000 /* Physical Slot Number */
+#define  PCI_EXP_SLTCAP_PSN_SHIFT	19 /* Physical Slot Number shift */
 #define PCI_EXP_SLTCTL		0x18	/* Slot Control */
 #define  PCI_EXP_SLTCTL_ABPE	0x0001	/* Attention Button Pressed Enable */
 #define  PCI_EXP_SLTCTL_PFDE	0x0002	/* Power Fault Detected Enable */
Signature · 1 line
2.20.1
#6 lore raw

Re: [PATCH 2/6] PCI: Add PCI_EXP_SLTCAP_*_SHIFT macros

Quoted 24 lines · Pali Rohár
Quoted 10 lines · On Thursday 24 February 2022 14:28:43 Bjorn Helgaas wrote:
Quoted 9 lines · Pali Rohár

These macros allows to easily compose and extract Slot Power Limit and Physical Slot Number values from Slot Capability Register.

---
 include/uapi/linux/pci_regs.h | 3 +++
 1 file changed, 3 insertions(+)
Quoted 12 lines

include/uapi/linux/pci_regs.h line 594

Quoted 9 lines
include/uapi/linux/pci_regs.h +1 -0
index 108f8523fa04..3fc9a4cac630 100644
@@ -591,10 +591,13 @@
 #define  PCI_EXP_SLTCAP_HPS	0x00000020 /* Hot-Plug Surprise */
 #define  PCI_EXP_SLTCAP_HPC	0x00000040 /* Hot-Plug Capable */
 #define  PCI_EXP_SLTCAP_SPLV	0x00007f80 /* Slot Power Limit Value */
+#define  PCI_EXP_SLTCAP_SPLV_SHIFT	7  /* Slot Power Limit Value shift */

Is there a way to use FIELD_PREP() and FIELD_GET() instead? It seems like that's what the cool kids are doing now.

Quoted 4 lines

This is possible too.

I have proposed a patch with _SHIFT macros as this is the way how are other macros in this file defined and used.

Yes, it's a mix. For some recent additions, I've resisted adding the _SHIFT macros on the theory that they clutter the file, they never change, and the main point of the #defines is readability and so grep/tags/etc can find things.

There are a *few* users of FIELD_PREP() and FIELD_GET():

git grep -E "FIELD_(GET|PREP)\(PCI_EXP"

and I'm inclined to go that direction in the future. What do you think?

Bjorn

#7 lore raw

Re: [PATCH 2/6] PCI: Add PCI_EXP_SLTCAP_*_SHIFT macros

Quoted 35 lines · On Fri, 25 Feb 2022 09:37:56 -0600 Bjorn Helgaas <[email protected]> wrote:
Quoted 24 lines · Pali Rohár
Quoted 10 lines · On Thursday 24 February 2022 14:28:43 Bjorn Helgaas wrote:
Quoted 9 lines · Pali Rohár

These macros allows to easily compose and extract Slot Power Limit and Physical Slot Number values from Slot Capability Register.

---
 include/uapi/linux/pci_regs.h | 3 +++
 1 file changed, 3 insertions(+)
Quoted 12 lines

include/uapi/linux/pci_regs.h line 594

Quoted 9 lines
include/uapi/linux/pci_regs.h +1 -0
index 108f8523fa04..3fc9a4cac630 100644
@@ -591,10 +591,13 @@
 #define  PCI_EXP_SLTCAP_HPS	0x00000020 /* Hot-Plug Surprise */
 #define  PCI_EXP_SLTCAP_HPC	0x00000040 /* Hot-Plug Capable */
 #define  PCI_EXP_SLTCAP_SPLV	0x00007f80 /* Slot Power Limit Value */
+#define  PCI_EXP_SLTCAP_SPLV_SHIFT	7  /* Slot Power Limit Value shift */  

Is there a way to use FIELD_PREP() and FIELD_GET() instead? It seems like that's what the cool kids are doing now.

Quoted 4 lines

This is possible too.

I have proposed a patch with _SHIFT macros as this is the way how are other macros in this file defined and used.

Yes, it's a mix. For some recent additions, I've resisted adding the _SHIFT macros on the theory that they clutter the file, they never change, and the main point of the #defines is readability and so grep/tags/etc can find things.

Quoted 6 lines

There are a *few* users of FIELD_PREP() and FIELD_GET():

git grep -E "FIELD_(GET|PREP)\(PCI_EXP"

and I'm inclined to go that direction in the future. What do you think?

I am also pro that direction.

Would you also like to convert current usages in the .c driver files?

We can't remove the existing macros since they are in UAPI, but we can convert drivers so that they don't use _SHIFT macros.

Marek

#8 lore raw

Re: [PATCH 2/6] PCI: Add PCI_EXP_SLTCAP_*_SHIFT macros

Quoted 15 lines · Marek Behún
Quoted 11 lines · On Fri, 25 Feb 2022 09:37:56 -0600 Bjorn Helgaas <[email protected]> wrote:

Yes, it's a mix. For some recent additions, I've resisted adding the _SHIFT macros on the theory that they clutter the file, they never change, and the main point of the #defines is readability and so grep/tags/etc can find things.

There are a *few* users of FIELD_PREP() and FIELD_GET():

git grep -E "FIELD_(GET|PREP)\(PCI_EXP"

and I'm inclined to go that direction in the future. What do you think?

Quoted 6 lines

I am also pro that direction.

Would you also like to convert current usages in the .c driver files?

We can't remove the existing macros since they are in UAPI, but we can convert drivers so that they don't use _SHIFT macros.

Certainly not a high priority, but it actually looks like there aren't *that* many uses in drivers/pci/, so I'd be OK with converting them.

Bjorn

author maintainer
#9 lore raw

PATCH 3/6 dt-bindings: Add 'slot-power-limit-milliwatt' PCIe port property

In reply to Pali Rohár

This property specifies slot power limit in mW unit. It is a form-factor and board specific value and must be initialized by hardware.

Some PCIe controllers delegate this work to software to allow hardware flexibility and therefore this property basically specifies what should host bridge program into PCIe Slot Capabilities registers.

The property needs to be specified in mW unit instead of the special format defined by Slot Capabilities (which encodes scaling factor or different unit). Host drivers should convert the value from mW to needed format.

---
This change was already accepted into dt-schema repo by Rob Herring:
https://github.com/devicetree-org/dt-schema/pull/66

---
 Documentation/devicetree/bindings/pci/pci.txt | 6 ++++++
 1 file changed, 6 insertions(+)
Documentation/devicetree/bindings/pci/pci.txt +6 -0
index 6a8f2874a24d..b0cc133ed00d 100644
@@ -32,6 +32,12 @@ driver implementation may support the following properties:
    root port to downstream device and host bridge drivers can do programming
    which depends on CLKREQ signal existence. For example, programming root port
    not to advertise ASPM L1 Sub-States support if there is no CLKREQ signal.
+- slot-power-limit-milliwatt:
+   If present, this property specifies slot power limit in milliwatts. Host
+   drivers can parse this property and use it for programming Root Port or host
+   bridge, or for composing and sending PCIe Set_Slot_Power_Limit messages
+   through the Root Port or host bridge when transitioning PCIe link from a
+   non-DL_Up Status to a DL_Up Status.
 
 PCI-PCI Bridge properties
 -------------------------
Signature · 1 line
2.20.1
#10 lore raw

Re: [PATCH 3/6] dt-bindings: Add 'slot-power-limit-milliwatt' PCIe port property

Quoted 7 lines · On Tue, 22 Feb 2022 17:31:55 +0100 Pali Rohár <[email protected]> wrote:

This property specifies slot power limit in mW unit. It is a form-factor and board specific value and must be initialized by hardware.

Some PCIe controllers delegate this work to software to allow hardware flexibility and therefore this property basically specifies what should host bridge program into PCIe Slot Capabilities registers.

Quoted 6 lines

The property needs to be specified in mW unit instead of the special format defined by Slot Capabilities (which encodes scaling factor or different unit). Host drivers should convert the value from mW to needed format.

This patch is not needed, the property already is described in dtschema.

Marek

author maintainer
#11 lore raw

Re: [PATCH 3/6] dt-bindings: Add 'slot-power-limit-milliwatt' PCIe port property

Quoted 14 lines · On Tuesday 22 February 2022 18:24:15 Marek Behún wrote:
Quoted 11 lines · On Tue, 22 Feb 2022 17:31:55 +0100 Pali Rohár <[email protected]> wrote:

This property specifies slot power limit in mW unit. It is a form-factor and board specific value and must be initialized by hardware.

Some PCIe controllers delegate this work to software to allow hardware flexibility and therefore this property basically specifies what should host bridge program into PCIe Slot Capabilities registers.

The property needs to be specified in mW unit instead of the special format defined by Slot Capabilities (which encodes scaling factor or different unit). Host drivers should convert the value from mW to needed format.

Quoted 5 lines
Quoted 2 lines

This patch is not needed, the property already is described in dtschema.

But dtschema with this property is not included in kernel and this file is the only in-kernel documentation. So updating it makes sense.

author maintainer
#12 lore raw

PATCH 4/6 PCI: Add function for parsing 'slot-power-limit-milliwatt' DT property

In reply to Pali Rohár

Add function of_pci_get_slot_power_limit(), which parses the 'slot-power-limit-milliwatt' DT property, returning the value in milliwatts and in format ready for the PCIe Slot Capabilities Register.

---
 drivers/pci/of.c  | 64 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h | 15 +++++++++++
 2 files changed, 79 insertions(+)
drivers/pci/of.c +64 -0
index cb2e8351c2cc..2b0c0a3641a8 100644
@@ -633,3 +633,67 @@ int of_pci_get_max_link_speed(struct device_node *node)
 	return max_link_speed;
 }
 EXPORT_SYMBOL_GPL(of_pci_get_max_link_speed);
+
+/**
+ * of_pci_get_slot_power_limit - Parses the "slot-power-limit-milliwatt"
+ *				 property.
+ *
+ * @node: device tree node with the slot power limit information
+ * @slot_power_limit_value: pointer where the value should be stored in PCIe
+ *			    Slot Capabilities Register format
+ * @slot_power_limit_scale: pointer where the scale should be stored in PCIe
+ *			    Slot Capabilities Register format
+ *
+ * Returns the slot power limit in milliwatts and if @slot_power_limit_value
+ * and @slot_power_limit_scale pointers are non-NULL, fills in the value and
+ * scale in format used by PCIe Slot Capabilities Register.
+ *
+ * If the property is not found or is invalid, returns 0.
+ */
+u32 of_pci_get_slot_power_limit(struct device_node *node,
+				u8 *slot_power_limit_value,
+				u8 *slot_power_limit_scale)
+{
+	u32 slot_power_limit;
+	u8 value, scale;
+
+	if (of_property_read_u32(node, "slot-power-limit-milliwatt",
+				 &slot_power_limit))
+		slot_power_limit = 0;
+
+	/* Calculate Slot Power Limit Value and Slot Power Limit Scale */
+	if (slot_power_limit == 0) {
+		value = 0x00;
+		scale = 0;
+	} else if (slot_power_limit <= 255) {
+		value = slot_power_limit;
+		scale = 3;
+	} else if (slot_power_limit <= 255*10) {
+		value = slot_power_limit / 10;
+		scale = 2;
+	} else if (slot_power_limit <= 255*100) {
+		value = slot_power_limit / 100;
+		scale = 1;
+	} else if (slot_power_limit <= 239*1000) {
+		value = slot_power_limit / 1000;
+		scale = 0;
+	} else if (slot_power_limit <= 250*1000) {
+		value = 0xF0;
+		scale = 0;
+	} else if (slot_power_limit <= 275*1000) {
+		value = 0xF1;
+		scale = 0;
+	} else {
+		value = 0xF2;
+		scale = 0;
+	}
+
+	if (slot_power_limit_value)
+		*slot_power_limit_value = value;
+
+	if (slot_power_limit_scale)
+		*slot_power_limit_scale = scale;
+
+	return slot_power_limit;
+}
+EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
drivers/pci/pci.h +15 -0
index 3d60cabde1a1..e10cdec6c56e 100644
@@ -627,6 +627,9 @@ struct device_node;
 int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
 int of_get_pci_domain_nr(struct device_node *node);
 int of_pci_get_max_link_speed(struct device_node *node);
+u32 of_pci_get_slot_power_limit(struct device_node *node,
+				u8 *slot_power_limit_value,
+				u8 *slot_power_limit_scale);
 void pci_set_of_node(struct pci_dev *dev);
 void pci_release_of_node(struct pci_dev *dev);
 void pci_set_bus_of_node(struct pci_bus *bus);
@@ -653,6 +656,18 @@ of_pci_get_max_link_speed(struct device_node *node)
 	return -EINVAL;
 }
 
+static inline u32
+of_pci_get_slot_power_limit(struct device_node *node,
+			    u8 *slot_power_limit_value,
+			    u8 *slot_power_limit_scale)
+{
+	if (slot_power_limit_value)
+		*slot_power_limit_value = 0;
+	if (slot_power_limit_scale)
+		*slot_power_limit_scale = 0;
+	return 0;
+}
+
 static inline void pci_set_of_node(struct pci_dev *dev) { }
 static inline void pci_release_of_node(struct pci_dev *dev) { }
 static inline void pci_set_bus_of_node(struct pci_bus *bus) { }
Signature · 1 line
2.20.1
#13 lore raw

Re: [PATCH 4/6] PCI: Add function for parsing 'slot-power-limit-milliwatt' DT property

Quoted 33 lines · Pali Rohár

Add function of_pci_get_slot_power_limit(), which parses the 'slot-power-limit-milliwatt' DT property, returning the value in milliwatts and in format ready for the PCIe Slot Capabilities Register.

---
 drivers/pci/of.c  | 64 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h | 15 +++++++++++
 2 files changed, 79 insertions(+)
drivers/pci/of.c +12 -0
index cb2e8351c2cc..2b0c0a3641a8 100644
@@ -633,3 +633,67 @@ int of_pci_get_max_link_speed(struct device_node *node)
 	return max_link_speed;
 }
 EXPORT_SYMBOL_GPL(of_pci_get_max_link_speed);
+
+/**
+ * of_pci_get_slot_power_limit - Parses the "slot-power-limit-milliwatt"
+ *				 property.
+ *
+ * @node: device tree node with the slot power limit information
+ * @slot_power_limit_value: pointer where the value should be stored in PCIe
+ *			    Slot Capabilities Register format
+ * @slot_power_limit_scale: pointer where the scale should be stored in PCIe
+ *			    Slot Capabilities Register format
+ *
+ * Returns the slot power limit in milliwatts and if @slot_power_limit_value

drivers/pci/of.c int of_pci_get_max_link_speed(struct device_node *node) · line 657

Quoted 10 lines
drivers/pci/of.c +10 -0
+ * and @slot_power_limit_scale pointers are non-NULL, fills in the value and
+ * scale in format used by PCIe Slot Capabilities Register.
+ *
+ * If the property is not found or is invalid, returns 0.
+ */
+u32 of_pci_get_slot_power_limit(struct device_node *node,
+				u8 *slot_power_limit_value,
+				u8 *slot_power_limit_scale)
+{
+	u32 slot_power_limit;

Including "mw" or similar reference to the units would give a hint of how to relate the code to the spec.

drivers/pci/of.c int of_pci_get_max_link_speed(struct device_node *node) · line 664

Quoted 7 lines
drivers/pci/of.c +7 -0
+	u8 value, scale;
+
+	if (of_property_read_u32(node, "slot-power-limit-milliwatt",
+				 &slot_power_limit))
+		slot_power_limit = 0;
+
+	/* Calculate Slot Power Limit Value and Slot Power Limit Scale */

Add a spec reference to PCIe r6.0, sec 7.5.3.9. IIUC, this supports up to 300W, which was what r5.0 defined, but r6.0 added values up to 0xfe (600W).

Quoted 23 lines
drivers/pci/of.c +23 -0
+	if (slot_power_limit == 0) {
+		value = 0x00;
+		scale = 0;
+	} else if (slot_power_limit <= 255) {
+		value = slot_power_limit;
+		scale = 3;
+	} else if (slot_power_limit <= 255*10) {
+		value = slot_power_limit / 10;
+		scale = 2;
+	} else if (slot_power_limit <= 255*100) {
+		value = slot_power_limit / 100;
+		scale = 1;
+	} else if (slot_power_limit <= 239*1000) {
+		value = slot_power_limit / 1000;
+		scale = 0;
+	} else if (slot_power_limit <= 250*1000) {
+		value = 0xF0;
+		scale = 0;
+	} else if (slot_power_limit <= 275*1000) {
+		value = 0xF1;
+		scale = 0;
+	} else {
+		value = 0xF2;

drivers/pci/of.c int of_pci_get_max_link_speed(struct device_node *node) · line 697

Quoted 10 lines
drivers/pci/of.c +10 -0
+		scale = 0;
+	}
+
+	if (slot_power_limit_value)
+		*slot_power_limit_value = value;
+
+	if (slot_power_limit_scale)
+		*slot_power_limit_scale = scale;
+
+	return slot_power_limit;

If "slot-power-limit-milliwatt" contains a value larger than can be represented in "value" and "scale", the return value will not agree with value/scale, will it?

Currently you only use the return value for a log message, so no real harm yet, other than the fact that we might print "Slot power limit 1000.0W" when the hardware will only advertise 600W available.

Also, if "slot-power-limit-milliwatt" contains something like 260000 mW (260 W), we'll return 0xF1/0, so the hardware will advertise 275 W available.

Quoted 38 lines
drivers/pci/of.c +2 -0
+}
+EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
drivers/pci/pci.h +15 -0
index 3d60cabde1a1..e10cdec6c56e 100644
@@ -627,6 +627,9 @@ struct device_node;
 int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
 int of_get_pci_domain_nr(struct device_node *node);
 int of_pci_get_max_link_speed(struct device_node *node);
+u32 of_pci_get_slot_power_limit(struct device_node *node,
+				u8 *slot_power_limit_value,
+				u8 *slot_power_limit_scale);
 void pci_set_of_node(struct pci_dev *dev);
 void pci_release_of_node(struct pci_dev *dev);
 void pci_set_bus_of_node(struct pci_bus *bus);
@@ -653,6 +656,18 @@ of_pci_get_max_link_speed(struct device_node *node)
 	return -EINVAL;
 }
 
+static inline u32
+of_pci_get_slot_power_limit(struct device_node *node,
+			    u8 *slot_power_limit_value,
+			    u8 *slot_power_limit_scale)
+{
+	if (slot_power_limit_value)
+		*slot_power_limit_value = 0;
+	if (slot_power_limit_scale)
+		*slot_power_limit_scale = 0;
+	return 0;
+}
+
 static inline void pci_set_of_node(struct pci_dev *dev) { }
 static inline void pci_release_of_node(struct pci_dev *dev) { }
 static inline void pci_set_bus_of_node(struct pci_bus *bus) { }
Signature · 1 line
2.20.1
author maintainer
#14 lore raw

Re: [PATCH 4/6] PCI: Add function for parsing 'slot-power-limit-milliwatt' DT property

Quoted 48 lines · On Thursday 24 February 2022 14:47:15 Bjorn Helgaas wrote:
Quoted 33 lines · Pali Rohár

Add function of_pci_get_slot_power_limit(), which parses the 'slot-power-limit-milliwatt' DT property, returning the value in milliwatts and in format ready for the PCIe Slot Capabilities Register.

---
 drivers/pci/of.c  | 64 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h | 15 +++++++++++
 2 files changed, 79 insertions(+)
drivers/pci/of.c +12 -0
index cb2e8351c2cc..2b0c0a3641a8 100644
@@ -633,3 +633,67 @@ int of_pci_get_max_link_speed(struct device_node *node)
 	return max_link_speed;
 }
 EXPORT_SYMBOL_GPL(of_pci_get_max_link_speed);
+
+/**
+ * of_pci_get_slot_power_limit - Parses the "slot-power-limit-milliwatt"
+ *				 property.
+ *
+ * @node: device tree node with the slot power limit information
+ * @slot_power_limit_value: pointer where the value should be stored in PCIe
+ *			    Slot Capabilities Register format
+ * @slot_power_limit_scale: pointer where the scale should be stored in PCIe
+ *			    Slot Capabilities Register format
+ *
+ * Returns the slot power limit in milliwatts and if @slot_power_limit_value

drivers/pci/of.c int of_pci_get_max_link_speed(struct device_node *node) · line 657

Quoted 10 lines
drivers/pci/of.c +10 -0
+ * and @slot_power_limit_scale pointers are non-NULL, fills in the value and
+ * scale in format used by PCIe Slot Capabilities Register.
+ *
+ * If the property is not found or is invalid, returns 0.
+ */
+u32 of_pci_get_slot_power_limit(struct device_node *node,
+				u8 *slot_power_limit_value,
+				u8 *slot_power_limit_scale)
+{
+	u32 slot_power_limit;

Including "mw" or similar reference to the units would give a hint of how to relate the code to the spec.

Quoted 11 lines

drivers/pci/of.c int of_pci_get_max_link_speed(struct device_node *node) · line 664

Quoted 7 lines
drivers/pci/of.c +7 -0
+	u8 value, scale;
+
+	if (of_property_read_u32(node, "slot-power-limit-milliwatt",
+				 &slot_power_limit))
+		slot_power_limit = 0;
+
+	/* Calculate Slot Power Limit Value and Slot Power Limit Scale */

Add a spec reference to PCIe r6.0, sec 7.5.3.9. IIUC, this supports up to 300W, which was what r5.0 defined, but r6.0 added values up to 0xfe (600W).

I did not know about it and I have not seen/read r6.0.

It would be nice if somebody with access to r6.0 send a patch to lspci utility, so we could write support for 600W based on lspci parser.

Quoted 29 lines
Quoted 29 lines
drivers/pci/of.c +29 -0
+	if (slot_power_limit == 0) {
+		value = 0x00;
+		scale = 0;
+	} else if (slot_power_limit <= 255) {
+		value = slot_power_limit;
+		scale = 3;
+	} else if (slot_power_limit <= 255*10) {
+		value = slot_power_limit / 10;
+		scale = 2;
+	} else if (slot_power_limit <= 255*100) {
+		value = slot_power_limit / 100;
+		scale = 1;
+	} else if (slot_power_limit <= 239*1000) {
+		value = slot_power_limit / 1000;
+		scale = 0;
+	} else if (slot_power_limit <= 250*1000) {
+		value = 0xF0;
+		scale = 0;
+	} else if (slot_power_limit <= 275*1000) {
+		value = 0xF1;
+		scale = 0;
+	} else {
+		value = 0xF2;
+		scale = 0;
+	}
+
+	if (slot_power_limit_value)
+		*slot_power_limit_value = value;
+
Quoted 8 lines

drivers/pci/of.c int of_pci_get_max_link_speed(struct device_node *node) · line 697

Quoted 4 lines
drivers/pci/of.c +4 -0
+	if (slot_power_limit_scale)
+		*slot_power_limit_scale = scale;
+
+	return slot_power_limit;

If "slot-power-limit-milliwatt" contains a value larger than can be represented in "value" and "scale", the return value will not agree with value/scale, will it?

In previous version 0xF2 was reserved for values above 275 W. So for me it looked like a correct solution.

Quoted 7 lines

Currently you only use the return value for a log message, so no real harm yet, other than the fact that we might print "Slot power limit 1000.0W" when the hardware will only advertise 600W available.

Also, if "slot-power-limit-milliwatt" contains something like 260000 mW (260 W), we'll return 0xF1/0, so the hardware will advertise 275 W available.

There is no way how to encode 260 W. It is possible only 250 W or 275 W, and nothing between. I chose to round value to upper limit. What do you prefer in these cases? Upper or lower limit?

Quoted 38 lines
Quoted 38 lines
drivers/pci/of.c +2 -0
+}
+EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
drivers/pci/pci.h +15 -0
index 3d60cabde1a1..e10cdec6c56e 100644
@@ -627,6 +627,9 @@ struct device_node;
 int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
 int of_get_pci_domain_nr(struct device_node *node);
 int of_pci_get_max_link_speed(struct device_node *node);
+u32 of_pci_get_slot_power_limit(struct device_node *node,
+				u8 *slot_power_limit_value,
+				u8 *slot_power_limit_scale);
 void pci_set_of_node(struct pci_dev *dev);
 void pci_release_of_node(struct pci_dev *dev);
 void pci_set_bus_of_node(struct pci_bus *bus);
@@ -653,6 +656,18 @@ of_pci_get_max_link_speed(struct device_node *node)
 	return -EINVAL;
 }
 
+static inline u32
+of_pci_get_slot_power_limit(struct device_node *node,
+			    u8 *slot_power_limit_value,
+			    u8 *slot_power_limit_scale)
+{
+	if (slot_power_limit_value)
+		*slot_power_limit_value = 0;
+	if (slot_power_limit_scale)
+		*slot_power_limit_scale = 0;
+	return 0;
+}
+
 static inline void pci_set_of_node(struct pci_dev *dev) { }
 static inline void pci_release_of_node(struct pci_dev *dev) { }
 static inline void pci_set_bus_of_node(struct pci_bus *bus) { }
Signature · 1 line
2.20.1
#15 lore raw

Re: [PATCH 4/6] PCI: Add function for parsing 'slot-power-limit-milliwatt' DT property

Quoted 61 lines · Pali Rohár
Quoted 48 lines · On Thursday 24 February 2022 14:47:15 Bjorn Helgaas wrote:
Quoted 33 lines · Pali Rohár

Add function of_pci_get_slot_power_limit(), which parses the 'slot-power-limit-milliwatt' DT property, returning the value in milliwatts and in format ready for the PCIe Slot Capabilities Register.

---
 drivers/pci/of.c  | 64 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h | 15 +++++++++++
 2 files changed, 79 insertions(+)
drivers/pci/of.c +12 -0
index cb2e8351c2cc..2b0c0a3641a8 100644
@@ -633,3 +633,67 @@ int of_pci_get_max_link_speed(struct device_node *node)
 	return max_link_speed;
 }
 EXPORT_SYMBOL_GPL(of_pci_get_max_link_speed);
+
+/**
+ * of_pci_get_slot_power_limit - Parses the "slot-power-limit-milliwatt"
+ *				 property.
+ *
+ * @node: device tree node with the slot power limit information
+ * @slot_power_limit_value: pointer where the value should be stored in PCIe
+ *			    Slot Capabilities Register format
+ * @slot_power_limit_scale: pointer where the scale should be stored in PCIe
+ *			    Slot Capabilities Register format
+ *
+ * Returns the slot power limit in milliwatts and if @slot_power_limit_value

drivers/pci/of.c int of_pci_get_max_link_speed(struct device_node *node) · line 657

Quoted 10 lines
drivers/pci/of.c +10 -0
+ * and @slot_power_limit_scale pointers are non-NULL, fills in the value and
+ * scale in format used by PCIe Slot Capabilities Register.
+ *
+ * If the property is not found or is invalid, returns 0.
+ */
+u32 of_pci_get_slot_power_limit(struct device_node *node,
+				u8 *slot_power_limit_value,
+				u8 *slot_power_limit_scale)
+{
+	u32 slot_power_limit;

Including "mw" or similar reference to the units would give a hint of how to relate the code to the spec.

Quoted 11 lines

drivers/pci/of.c int of_pci_get_max_link_speed(struct device_node *node) · line 664

Quoted 7 lines
drivers/pci/of.c +7 -0
+	u8 value, scale;
+
+	if (of_property_read_u32(node, "slot-power-limit-milliwatt",
+				 &slot_power_limit))
+		slot_power_limit = 0;
+
+	/* Calculate Slot Power Limit Value and Slot Power Limit Scale */

Add a spec reference to PCIe r6.0, sec 7.5.3.9. IIUC, this supports up to 300W, which was what r5.0 defined, but r6.0 added values up to 0xfe (600W).

Quoted 4 lines

I did not know about it and I have not seen/read r6.0.

It would be nice if somebody with access to r6.0 send a patch to lspci utility, so we could write support for 600W based on lspci parser.

Of course, sorry! Obviously you would have implemented them all if you had the spec!

Here's the info from r6.0, sec 7.5.3.9:

Slot Power Limit Value - In combination with the Slot Power Limit Scale value, specifies the upper limit on power supplied by the slot (see § Section 6.9) or by other means to the adapter.

Power limit (in Watts) is calculated by multiplying the value in this field by the value in the Slot Power Limit Scale field except when the Slot Power Limit Scale field equals 00b (1.0x) and Slot Power Limit Value exceeds EFh, the following alternative encodings are used:

    F0h   > 239 W and ≤ 250 W Slot Power Limit
    F1h   > 250 W and ≤ 275 W Slot Power Limit
    F2h   > 275 W and ≤ 300 W Slot Power Limit
    F3h   > 300 W and ≤ 325 W Slot Power Limit
    F4h   > 325 W and ≤ 350 W Slot Power Limit
    F5h   > 350 W and ≤ 375 W Slot Power Limit
    F6h   > 375 W and ≤ 400 W Slot Power Limit
    F7h   > 400 W and ≤ 425 W Slot Power Limit
    F8h   > 425 W and ≤ 450 W Slot Power Limit
    F9h   > 450 W and ≤ 475 W Slot Power Limit
    FAh   > 475 W and ≤ 500 W Slot Power Limit
    FBh   > 500 W and ≤ 525 W Slot Power Limit
    FCh   > 525 W and ≤ 550 W Slot Power Limit
    FDh   > 550 W and ≤ 575 W Slot Power Limit
    FEh   > 575 W and ≤ 600 W Slot Power Limit
    FFh   Reserved for Slot Power Limit Values above 600 W

This register must be implemented if the Slot Implemented bit is Set.

Writes to this register also cause the Port to send the Set_Slot_Power_Limit Message.

Quoted 45 lines
Quoted 29 lines
Quoted 29 lines
drivers/pci/of.c +29 -0
+	if (slot_power_limit == 0) {
+		value = 0x00;
+		scale = 0;
+	} else if (slot_power_limit <= 255) {
+		value = slot_power_limit;
+		scale = 3;
+	} else if (slot_power_limit <= 255*10) {
+		value = slot_power_limit / 10;
+		scale = 2;
+	} else if (slot_power_limit <= 255*100) {
+		value = slot_power_limit / 100;
+		scale = 1;
+	} else if (slot_power_limit <= 239*1000) {
+		value = slot_power_limit / 1000;
+		scale = 0;
+	} else if (slot_power_limit <= 250*1000) {
+		value = 0xF0;
+		scale = 0;
+	} else if (slot_power_limit <= 275*1000) {
+		value = 0xF1;
+		scale = 0;
+	} else {
+		value = 0xF2;
+		scale = 0;
+	}
+
+	if (slot_power_limit_value)
+		*slot_power_limit_value = value;
+
Quoted 8 lines

drivers/pci/of.c int of_pci_get_max_link_speed(struct device_node *node) · line 697

Quoted 4 lines
drivers/pci/of.c +4 -0
+	if (slot_power_limit_scale)
+		*slot_power_limit_scale = scale;
+
+	return slot_power_limit;

If "slot-power-limit-milliwatt" contains a value larger than can be represented in "value" and "scale", the return value will not agree with value/scale, will it?

In previous version 0xF2 was reserved for values above 275 W. So for me it looked like a correct solution.

Quoted 4 lines

Currently you only use the return value for a log message, so no real harm yet, other than the fact that we might print "Slot power limit 1000.0W" when the hardware will only advertise 600W available.

Quoted 7 lines
Quoted 3 lines

Also, if "slot-power-limit-milliwatt" contains something like 260000 mW (260 W), we'll return 0xF1/0, so the hardware will advertise 275 W available.

There is no way how to encode 260 W. It is possible only 250 W or 275 W, and nothing between. I chose to round value to upper limit. What do you prefer in these cases? Upper or lower limit?

I think rounding down is better. If we round up, the slot will advertise more power than it can deliver, and if the device tries to consume the amount of power advertised, it may not work reliably.

So I think we should return encoded values that are no higher than what the slot can actually deliver, and the return value should match what Slot Capabilities advertises.

Bjorn

author maintainer
#16 lore raw

Re: [PATCH 4/6] PCI: Add function for parsing 'slot-power-limit-milliwatt' DT property

Quoted 99 lines · On Friday 25 February 2022 09:51:56 Bjorn Helgaas wrote:
Quoted 61 lines · Pali Rohár
Quoted 48 lines · On Thursday 24 February 2022 14:47:15 Bjorn Helgaas wrote:
Quoted 33 lines · Pali Rohár

Add function of_pci_get_slot_power_limit(), which parses the 'slot-power-limit-milliwatt' DT property, returning the value in milliwatts and in format ready for the PCIe Slot Capabilities Register.

---
 drivers/pci/of.c  | 64 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h | 15 +++++++++++
 2 files changed, 79 insertions(+)
drivers/pci/of.c +12 -0
index cb2e8351c2cc..2b0c0a3641a8 100644
@@ -633,3 +633,67 @@ int of_pci_get_max_link_speed(struct device_node *node)
 	return max_link_speed;
 }
 EXPORT_SYMBOL_GPL(of_pci_get_max_link_speed);
+
+/**
+ * of_pci_get_slot_power_limit - Parses the "slot-power-limit-milliwatt"
+ *				 property.
+ *
+ * @node: device tree node with the slot power limit information
+ * @slot_power_limit_value: pointer where the value should be stored in PCIe
+ *			    Slot Capabilities Register format
+ * @slot_power_limit_scale: pointer where the scale should be stored in PCIe
+ *			    Slot Capabilities Register format
+ *
+ * Returns the slot power limit in milliwatts and if @slot_power_limit_value

drivers/pci/of.c int of_pci_get_max_link_speed(struct device_node *node) · line 657

Quoted 10 lines
drivers/pci/of.c +10 -0
+ * and @slot_power_limit_scale pointers are non-NULL, fills in the value and
+ * scale in format used by PCIe Slot Capabilities Register.
+ *
+ * If the property is not found or is invalid, returns 0.
+ */
+u32 of_pci_get_slot_power_limit(struct device_node *node,
+				u8 *slot_power_limit_value,
+				u8 *slot_power_limit_scale)
+{
+	u32 slot_power_limit;

Including "mw" or similar reference to the units would give a hint of how to relate the code to the spec.

Quoted 11 lines

drivers/pci/of.c int of_pci_get_max_link_speed(struct device_node *node) · line 664

Quoted 7 lines
drivers/pci/of.c +7 -0
+	u8 value, scale;
+
+	if (of_property_read_u32(node, "slot-power-limit-milliwatt",
+				 &slot_power_limit))
+		slot_power_limit = 0;
+
+	/* Calculate Slot Power Limit Value and Slot Power Limit Scale */

Add a spec reference to PCIe r6.0, sec 7.5.3.9. IIUC, this supports up to 300W, which was what r5.0 defined, but r6.0 added values up to 0xfe (600W).

Quoted 4 lines

I did not know about it and I have not seen/read r6.0.

It would be nice if somebody with access to r6.0 send a patch to lspci utility, so we could write support for 600W based on lspci parser.

Of course, sorry! Obviously you would have implemented them all if you had the spec!

Here's the info from r6.0, sec 7.5.3.9:

Slot Power Limit Value - In combination with the Slot Power Limit Scale value, specifies the upper limit on power supplied by the slot (see § Section 6.9) or by other means to the adapter.

Power limit (in Watts) is calculated by multiplying the value in this field by the value in the Slot Power Limit Scale field except when the Slot Power Limit Scale field equals 00b (1.0x) and Slot Power Limit Value exceeds EFh, the following alternative encodings are used:

    F0h   > 239 W and ≤ 250 W Slot Power Limit
    F1h   > 250 W and ≤ 275 W Slot Power Limit
    F2h   > 275 W and ≤ 300 W Slot Power Limit
    F3h   > 300 W and ≤ 325 W Slot Power Limit
    F4h   > 325 W and ≤ 350 W Slot Power Limit
    F5h   > 350 W and ≤ 375 W Slot Power Limit
    F6h   > 375 W and ≤ 400 W Slot Power Limit
    F7h   > 400 W and ≤ 425 W Slot Power Limit
    F8h   > 425 W and ≤ 450 W Slot Power Limit
    F9h   > 450 W and ≤ 475 W Slot Power Limit
    FAh   > 475 W and ≤ 500 W Slot Power Limit
    FBh   > 500 W and ≤ 525 W Slot Power Limit
    FCh   > 525 W and ≤ 550 W Slot Power Limit
    FDh   > 550 W and ≤ 575 W Slot Power Limit
    FEh   > 575 W and ≤ 600 W Slot Power Limit
    FFh   Reserved for Slot Power Limit Values above 600 W
Quoted 4 lines

This register must be implemented if the Slot Implemented bit is Set.

Writes to this register also cause the Port to send the Set_Slot_Power_Limit Message.

Ok, thank you!

I will send also update for lspci.

Quoted 57 lines
Quoted 45 lines
Quoted 29 lines
Quoted 29 lines
drivers/pci/of.c +29 -0
+	if (slot_power_limit == 0) {
+		value = 0x00;
+		scale = 0;
+	} else if (slot_power_limit <= 255) {
+		value = slot_power_limit;
+		scale = 3;
+	} else if (slot_power_limit <= 255*10) {
+		value = slot_power_limit / 10;
+		scale = 2;
+	} else if (slot_power_limit <= 255*100) {
+		value = slot_power_limit / 100;
+		scale = 1;
+	} else if (slot_power_limit <= 239*1000) {
+		value = slot_power_limit / 1000;
+		scale = 0;
+	} else if (slot_power_limit <= 250*1000) {
+		value = 0xF0;
+		scale = 0;
+	} else if (slot_power_limit <= 275*1000) {
+		value = 0xF1;
+		scale = 0;
+	} else {
+		value = 0xF2;
+		scale = 0;
+	}
+
+	if (slot_power_limit_value)
+		*slot_power_limit_value = value;
+
Quoted 8 lines

drivers/pci/of.c int of_pci_get_max_link_speed(struct device_node *node) · line 697

Quoted 4 lines
drivers/pci/of.c +4 -0
+	if (slot_power_limit_scale)
+		*slot_power_limit_scale = scale;
+
+	return slot_power_limit;

If "slot-power-limit-milliwatt" contains a value larger than can be represented in "value" and "scale", the return value will not agree with value/scale, will it?

In previous version 0xF2 was reserved for values above 275 W. So for me it looked like a correct solution.

Quoted 4 lines

Currently you only use the return value for a log message, so no real harm yet, other than the fact that we might print "Slot power limit 1000.0W" when the hardware will only advertise 600W available.

Quoted 7 lines
Quoted 3 lines

Also, if "slot-power-limit-milliwatt" contains something like 260000 mW (260 W), we'll return 0xF1/0, so the hardware will advertise 275 W available.

There is no way how to encode 260 W. It is possible only 250 W or 275 W, and nothing between. I chose to round value to upper limit. What do you prefer in these cases? Upper or lower limit?

I think rounding down is better. If we round up, the slot will advertise more power than it can deliver, and if the device tries to consume the amount of power advertised, it may not work reliably.

Quoted 5 lines

So I think we should return encoded values that are no higher than what the slot can actually deliver, and the return value should match what Slot Capabilities advertises.

Bjorn

It makes sense.

author maintainer
#17 lore raw

PATCH 5/6 PCI: mvebu: Add support for sending Set_Slot_Power_Limit message

In reply to Pali Rohár

This PCIe message is sent automatically by mvebu HW when link changes status from down to up.

Slot power limit is read from DT property 'slot-power-limit-milliwatt' and set to mvebu HW. When this DT property is not specified then driver treat it as "Slot Capabilities register has not yet been initialized".

---
 drivers/pci/controller/pci-mvebu.c | 85 ++++++++++++++++++++++++++++--
 1 file changed, 80 insertions(+), 5 deletions(-)
drivers/pci/controller/pci-mvebu.c +80 -5
index a75d2b9196f9..c786feec4d17 100644
@@ -66,6 +66,12 @@
 #define  PCIE_STAT_BUS                  0xff00
 #define  PCIE_STAT_DEV                  0x1f0000
 #define  PCIE_STAT_LINK_DOWN		BIT(0)
+#define PCIE_SSPL_OFF		0x1a0c
+#define  PCIE_SSPL_VALUE_SHIFT		0
+#define  PCIE_SSPL_VALUE_MASK		GENMASK(7, 0)
+#define  PCIE_SSPL_SCALE_SHIFT		8
+#define  PCIE_SSPL_SCALE_MASK		GENMASK(9, 8)
+#define  PCIE_SSPL_ENABLE		BIT(16)
 #define PCIE_RC_RTSTA		0x1a14
 #define PCIE_DEBUG_CTRL         0x1a60
 #define  PCIE_DEBUG_SOFT_RESET		BIT(20)
@@ -111,6 +117,8 @@ struct mvebu_pcie_port {
 	struct mvebu_pcie_window iowin;
 	u32 saved_pcie_stat;
 	struct resource regs;
+	u8 slot_power_limit_value;
+	u8 slot_power_limit_scale;
 	struct irq_domain *intx_irq_domain;
 	raw_spinlock_t irq_lock;
 	int intx_irq;
@@ -239,7 +247,7 @@ static void mvebu_pcie_setup_wins(struct mvebu_pcie_port *port)
 
 static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 {
-	u32 ctrl, lnkcap, cmd, dev_rev, unmask;
+	u32 ctrl, lnkcap, cmd, dev_rev, unmask, sspl;
 
 	/* Setup PCIe controller to Root Complex mode. */
 	ctrl = mvebu_readl(port, PCIE_CTRL_OFF);
@@ -292,6 +300,20 @@ static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 	/* Point PCIe unit MBUS decode windows to DRAM space. */
 	mvebu_pcie_setup_wins(port);
 
+	/*
+	 * Program Root Complex to automatically sends Set Slot Power Limit
+	 * PCIe Message when changing status from Dl-Down to Dl-Up and valid
+	 * slot power limit was specified.
+	 */
+	sspl = mvebu_readl(port, PCIE_SSPL_OFF);
+	sspl &= ~(PCIE_SSPL_VALUE_MASK | PCIE_SSPL_SCALE_MASK | PCIE_SSPL_ENABLE);
+	if (port->slot_power_limit_value && port->slot_power_limit_scale) {
+		sspl |= port->slot_power_limit_value << PCIE_SSPL_VALUE_SHIFT;
+		sspl |= port->slot_power_limit_scale << PCIE_SSPL_SCALE_SHIFT;
+		sspl |= PCIE_SSPL_ENABLE;
+	}
+	mvebu_writel(port, sspl, PCIE_SSPL_OFF);
+
 	/* Mask all interrupt sources. */
 	mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_UNMASK_OFF);
 
@@ -628,9 +650,16 @@ mvebu_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge,
 			  (PCI_EXP_LNKSTA_DLLLA << 16) : 0);
 		break;
 
-	case PCI_EXP_SLTCTL:
-		*value = PCI_EXP_SLTSTA_PDS << 16;
+	case PCI_EXP_SLTCTL: {
+		u16 slotsta = le16_to_cpu(bridge->pcie_conf.slotsta);
+		u32 val = 0;
+		if (!(mvebu_readl(port, PCIE_SSPL_OFF) & PCIE_SSPL_ENABLE))
+			val |= PCI_EXP_SLTCTL_ASPL_DISABLE;
+		/* PCI_EXP_SLTCTL is 32-bit and contains also slot status bits */
+		val |= slotsta << 16;
+		*value = val;
 		break;
+	}
 
 	case PCI_EXP_RTSTA:
 		*value = mvebu_readl(port, PCIE_RC_RTSTA);
@@ -774,6 +803,19 @@ mvebu_pci_bridge_emul_pcie_conf_write(struct pci_bridge_emul *bridge,
 		mvebu_writel(port, new, PCIE_CAP_PCIEXP + PCI_EXP_LNKCTL);
 		break;
 
+	case PCI_EXP_SLTCTL:
+		if ((mask & PCI_EXP_SLTCTL_ASPL_DISABLE) &&
+		    port->slot_power_limit_value &&
+		    port->slot_power_limit_scale) {
+			u32 sspl = mvebu_readl(port, PCIE_SSPL_OFF);
+			if (new & PCI_EXP_SLTCTL_ASPL_DISABLE)
+				sspl &= ~PCIE_SSPL_ENABLE;
+			else
+				sspl |= PCIE_SSPL_ENABLE;
+			mvebu_writel(port, sspl, PCIE_SSPL_OFF);
+		}
+		break;
+
 	case PCI_EXP_RTSTA:
 		/*
 		 * PME Status bit in Root Status Register (PCIE_RC_RTSTA)
@@ -868,8 +910,26 @@ static int mvebu_pci_bridge_emul_init(struct mvebu_pcie_port *port)
 	/*
 	 * Older mvebu hardware provides PCIe Capability structure only in
 	 * version 1. New hardware provides it in version 2.
+	 * Enable slot support which is emulated.
 	 */
-	bridge->pcie_conf.cap = cpu_to_le16(pcie_cap_ver);
+	bridge->pcie_conf.cap = cpu_to_le16(pcie_cap_ver | PCI_EXP_FLAGS_SLOT);
+
+	/*
+	 * Set Presence Detect State bit permanently as there is no support for
+	 * unplugging PCIe card from the slot. Assume that PCIe card is always
+	 * connected in slot.
+	 *
+	 * Set physical slot number to port+1 as mvebu ports are indexed from
+	 * zero and zero value is reserved for ports within the same silicon
+	 * as Root Port which is not mvebu case.
+	 *
+	 * Also set correct slot power limit.
+	 */
+	bridge->pcie_conf.slotcap = cpu_to_le32(
+		(port->slot_power_limit_value << PCI_EXP_SLTCAP_SPLV_SHIFT) |
+		(port->slot_power_limit_scale << PCI_EXP_SLTCAP_SPLS_SHIFT) |
+		((port->port+1) << PCI_EXP_SLTCAP_PSN_SHIFT));
+	bridge->pcie_conf.slotsta = cpu_to_le16(PCI_EXP_SLTSTA_PDS);
 
 	bridge->subsystem_vendor_id = ssdev_id & 0xffff;
 	bridge->subsystem_id = ssdev_id >> 16;
@@ -1191,6 +1251,7 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
 {
 	struct device *dev = &pcie->pdev->dev;
 	enum of_gpio_flags flags;
+	u32 slot_power_limit;
 	int reset_gpio, ret;
 	u32 num_lanes;
 
@@ -1291,6 +1352,15 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
 		port->reset_gpio = gpio_to_desc(reset_gpio);
 	}
 
+	slot_power_limit = of_pci_get_slot_power_limit(child,
+				&port->slot_power_limit_value,
+				&port->slot_power_limit_scale);
+	if (slot_power_limit)
+		dev_info(dev, "%s: Slot power limit %u.%uW\n",
+			 port->name,
+			 slot_power_limit / 1000,
+			 (slot_power_limit / 100) % 10);
+
 	port->clk = of_clk_get_by_name(child, NULL);
 	if (IS_ERR(port->clk)) {
 		dev_err(dev, "%s: cannot get clock\n", port->name);
@@ -1587,7 +1657,7 @@ static int mvebu_pcie_remove(struct platform_device *pdev)
 {
 	struct mvebu_pcie *pcie = platform_get_drvdata(pdev);
 	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
-	u32 cmd;
+	u32 cmd, sspl;
 	int i;
 
 	/* Remove PCI bus with all devices. */
@@ -1624,6 +1694,11 @@ static int mvebu_pcie_remove(struct platform_device *pdev)
 		/* Free config space for emulated root bridge. */
 		pci_bridge_emul_cleanup(&port->bridge);
 
+		/* Disable sending Set Slot Power Limit PCIe Message. */
+		sspl = mvebu_readl(port, PCIE_SSPL_OFF);
+		sspl &= ~(PCIE_SSPL_VALUE_MASK | PCIE_SSPL_SCALE_MASK | PCIE_SSPL_ENABLE);
+		mvebu_writel(port, sspl, PCIE_SSPL_OFF);
+
 		/* Disable and clear BARs and windows. */
 		mvebu_pcie_disable_wins(port);
 
Signature · 1 line
2.20.1
#18 lore raw

Re: [PATCH 5/6] PCI: mvebu: Add support for sending Set_Slot_Power_Limit message

Quoted 2 lines · Pali Rohár

This PCIe message is sent automatically by mvebu HW when link changes status from down to up.

I *think* the intent of the above is:

If DT supplies the 'slot-power-limit-milliwatt' property, program the value in the Slot Power Limit in the Slot Capabilities register and program the Root Port to send a Set_Slot_Power_Limit Message when the Link transitions to DL_Up.

PCIe r6.0, sec 2.2.8.5 and 7.5.3.9, also say Set_Slot_Power_Limit must be sent on a config write to Slot Capabilities. I don't really understand that, since AFAICS, everything in that register is read-only. But there must be some use case for forcing a message.

Quoted 3 lines

Slot power limit is read from DT property 'slot-power-limit-milliwatt' and set to mvebu HW. When this DT property is not specified then driver treat it as "Slot Capabilities register has not yet been initialized".

Does this last sentence mean "the Slot Power Limit is set to 0W (Value = 00h and Scale = 00b = 1.0x) and Auto Slot Power Limit Disable is set, so Set_Slot_Power_Limit Messages will never be sent"?

Quoted 41 lines
---
 drivers/pci/controller/pci-mvebu.c | 85 ++++++++++++++++++++++++++++--
 1 file changed, 80 insertions(+), 5 deletions(-)
drivers/pci/controller/pci-mvebu.c +9 -1
index a75d2b9196f9..c786feec4d17 100644
@@ -66,6 +66,12 @@
 #define  PCIE_STAT_BUS                  0xff00
 #define  PCIE_STAT_DEV                  0x1f0000
 #define  PCIE_STAT_LINK_DOWN		BIT(0)
+#define PCIE_SSPL_OFF		0x1a0c
+#define  PCIE_SSPL_VALUE_SHIFT		0
+#define  PCIE_SSPL_VALUE_MASK		GENMASK(7, 0)
+#define  PCIE_SSPL_SCALE_SHIFT		8
+#define  PCIE_SSPL_SCALE_MASK		GENMASK(9, 8)
+#define  PCIE_SSPL_ENABLE		BIT(16)
 #define PCIE_RC_RTSTA		0x1a14
 #define PCIE_DEBUG_CTRL         0x1a60
 #define  PCIE_DEBUG_SOFT_RESET		BIT(20)
@@ -111,6 +117,8 @@ struct mvebu_pcie_port {
 	struct mvebu_pcie_window iowin;
 	u32 saved_pcie_stat;
 	struct resource regs;
+	u8 slot_power_limit_value;
+	u8 slot_power_limit_scale;
 	struct irq_domain *intx_irq_domain;
 	raw_spinlock_t irq_lock;
 	int intx_irq;
@@ -239,7 +247,7 @@ static void mvebu_pcie_setup_wins(struct mvebu_pcie_port *port)
 
 static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 {
-	u32 ctrl, lnkcap, cmd, dev_rev, unmask;
+	u32 ctrl, lnkcap, cmd, dev_rev, unmask, sspl;
 
 	/* Setup PCIe controller to Root Complex mode. */
 	ctrl = mvebu_readl(port, PCIE_CTRL_OFF);

drivers/pci/controller/pci-mvebu.c static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port) · line 306

Quoted 7 lines
drivers/pci/controller/pci-mvebu.c +4 -0
@@ -292,6 +300,20 @@ static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 	/* Point PCIe unit MBUS decode windows to DRAM space. */
 	mvebu_pcie_setup_wins(port);
 
+	/*
+	 * Program Root Complex to automatically sends Set Slot Power Limit
+	 * PCIe Message when changing status from Dl-Down to Dl-Up and valid
+	 * slot power limit was specified.

s/Root Complex/Root Port/, right? AFAIK the message would be sent by a Downstream Port, i.e., a Root Port in this case.

s/sends/send/
s/Set Slot Power Limit/Set_Slot_Power_Limit/ to match spec usage (also below)
s/Dl-Down/DL_Down/ to match spec usage
s/Dl-Up/DL_Up/ ditto

Quoted 15 lines
drivers/pci/controller/pci-mvebu.c +10 -0
+	 */
+	sspl = mvebu_readl(port, PCIE_SSPL_OFF);
+	sspl &= ~(PCIE_SSPL_VALUE_MASK | PCIE_SSPL_SCALE_MASK | PCIE_SSPL_ENABLE);
+	if (port->slot_power_limit_value && port->slot_power_limit_scale) {
+		sspl |= port->slot_power_limit_value << PCIE_SSPL_VALUE_SHIFT;
+		sspl |= port->slot_power_limit_scale << PCIE_SSPL_SCALE_SHIFT;
+		sspl |= PCIE_SSPL_ENABLE;
+	}
+	mvebu_writel(port, sspl, PCIE_SSPL_OFF);
+
 	/* Mask all interrupt sources. */
 	mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_UNMASK_OFF);
 
@@ -628,9 +650,16 @@ mvebu_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge,
 			  (PCI_EXP_LNKSTA_DLLLA << 16) : 0);

drivers/pci/controller/pci-mvebu.c mvebu_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge, · line 658

Quoted 10 lines
drivers/pci/controller/pci-mvebu.c +6 -2
 		break;
 
-	case PCI_EXP_SLTCTL:
-		*value = PCI_EXP_SLTSTA_PDS << 16;
+	case PCI_EXP_SLTCTL: {
+		u16 slotsta = le16_to_cpu(bridge->pcie_conf.slotsta);
+		u32 val = 0;
+		if (!(mvebu_readl(port, PCIE_SSPL_OFF) & PCIE_SSPL_ENABLE))
+			val |= PCI_EXP_SLTCTL_ASPL_DISABLE;
+		/* PCI_EXP_SLTCTL is 32-bit and contains also slot status bits */

This comment is a little bit confusing because PCI_EXP_SLTCTL is not actually 32 bits; it's 16 bits. It's just that we "read" 32 bits, which includes both PCI_EXP_SLTCTL and PCI_EXP_SLTSTA. If you use "PCI_EXP_SLTCTL", I think it would be helpful to also say "PCI_EXP_SLTSTA" instead of "slot status bits".

Quoted 11 lines
drivers/pci/controller/pci-mvebu.c +3 -0
+		val |= slotsta << 16;
+		*value = val;
 		break;
+	}
 
 	case PCI_EXP_RTSTA:
 		*value = mvebu_readl(port, PCIE_RC_RTSTA);
@@ -774,6 +803,19 @@ mvebu_pci_bridge_emul_pcie_conf_write(struct pci_bridge_emul *bridge,
 		mvebu_writel(port, new, PCIE_CAP_PCIEXP + PCI_EXP_LNKCTL);
 		break;
 

drivers/pci/controller/pci-mvebu.c mvebu_pci_bridge_emul_pcie_conf_write(struct pci_bridge_emul *bridge, · line 815

Quoted 10 lines
drivers/pci/controller/pci-mvebu.c +10 -0
+	case PCI_EXP_SLTCTL:
+		if ((mask & PCI_EXP_SLTCTL_ASPL_DISABLE) &&
+		    port->slot_power_limit_value &&
+		    port->slot_power_limit_scale) {
+			u32 sspl = mvebu_readl(port, PCIE_SSPL_OFF);
+			if (new & PCI_EXP_SLTCTL_ASPL_DISABLE)
+				sspl &= ~PCIE_SSPL_ENABLE;
+			else
+				sspl |= PCIE_SSPL_ENABLE;
+			mvebu_writel(port, sspl, PCIE_SSPL_OFF);

IIUC, the behavior of PCI_EXP_SLTCTL_ASPL_DISABLE as observed by software that sets it and reads it back will depend on whether the DT contains "slot-power-limit-milliwatt".

If there is no DT property, port->slot_power_limit_value will be zero and PCIE_SSPL_ENABLE will never be set. So if I clear PCI_EXP_SLTCTL_ASPL_DISABLE, then read it back, it looks like it will read as being set. That's not what I would expect from the spec (PCIe r6.0, sec 7.5.3.10).

Quoted 82 lines
drivers/pci/controller/pci-mvebu.c +38 -2
+		}
+		break;
+
 	case PCI_EXP_RTSTA:
 		/*
 		 * PME Status bit in Root Status Register (PCIE_RC_RTSTA)
@@ -868,8 +910,26 @@ static int mvebu_pci_bridge_emul_init(struct mvebu_pcie_port *port)
 	/*
 	 * Older mvebu hardware provides PCIe Capability structure only in
 	 * version 1. New hardware provides it in version 2.
+	 * Enable slot support which is emulated.
 	 */
-	bridge->pcie_conf.cap = cpu_to_le16(pcie_cap_ver);
+	bridge->pcie_conf.cap = cpu_to_le16(pcie_cap_ver | PCI_EXP_FLAGS_SLOT);
+
+	/*
+	 * Set Presence Detect State bit permanently as there is no support for
+	 * unplugging PCIe card from the slot. Assume that PCIe card is always
+	 * connected in slot.
+	 *
+	 * Set physical slot number to port+1 as mvebu ports are indexed from
+	 * zero and zero value is reserved for ports within the same silicon
+	 * as Root Port which is not mvebu case.
+	 *
+	 * Also set correct slot power limit.
+	 */
+	bridge->pcie_conf.slotcap = cpu_to_le32(
+		(port->slot_power_limit_value << PCI_EXP_SLTCAP_SPLV_SHIFT) |
+		(port->slot_power_limit_scale << PCI_EXP_SLTCAP_SPLS_SHIFT) |
+		((port->port+1) << PCI_EXP_SLTCAP_PSN_SHIFT));
+	bridge->pcie_conf.slotsta = cpu_to_le16(PCI_EXP_SLTSTA_PDS);
 
 	bridge->subsystem_vendor_id = ssdev_id & 0xffff;
 	bridge->subsystem_id = ssdev_id >> 16;
@@ -1191,6 +1251,7 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
 {
 	struct device *dev = &pcie->pdev->dev;
 	enum of_gpio_flags flags;
+	u32 slot_power_limit;
 	int reset_gpio, ret;
 	u32 num_lanes;
 
@@ -1291,6 +1352,15 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
 		port->reset_gpio = gpio_to_desc(reset_gpio);
 	}
 
+	slot_power_limit = of_pci_get_slot_power_limit(child,
+				&port->slot_power_limit_value,
+				&port->slot_power_limit_scale);
+	if (slot_power_limit)
+		dev_info(dev, "%s: Slot power limit %u.%uW\n",
+			 port->name,
+			 slot_power_limit / 1000,
+			 (slot_power_limit / 100) % 10);
+
 	port->clk = of_clk_get_by_name(child, NULL);
 	if (IS_ERR(port->clk)) {
 		dev_err(dev, "%s: cannot get clock\n", port->name);
@@ -1587,7 +1657,7 @@ static int mvebu_pcie_remove(struct platform_device *pdev)
 {
 	struct mvebu_pcie *pcie = platform_get_drvdata(pdev);
 	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
-	u32 cmd;
+	u32 cmd, sspl;
 	int i;
 
 	/* Remove PCI bus with all devices. */
@@ -1624,6 +1694,11 @@ static int mvebu_pcie_remove(struct platform_device *pdev)
 		/* Free config space for emulated root bridge. */
 		pci_bridge_emul_cleanup(&port->bridge);
 
+		/* Disable sending Set Slot Power Limit PCIe Message. */
+		sspl = mvebu_readl(port, PCIE_SSPL_OFF);
+		sspl &= ~(PCIE_SSPL_VALUE_MASK | PCIE_SSPL_SCALE_MASK | PCIE_SSPL_ENABLE);
+		mvebu_writel(port, sspl, PCIE_SSPL_OFF);
+
 		/* Disable and clear BARs and windows. */
 		mvebu_pcie_disable_wins(port);
 
Signature · 1 line
2.20.1
author maintainer
#19 lore raw

Re: [PATCH 5/6] PCI: mvebu: Add support for sending Set_Slot_Power_Limit message

Quoted 10 lines · On Thursday 24 February 2022 15:28:11 Bjorn Helgaas wrote:
Quoted 2 lines · Pali Rohár

This PCIe message is sent automatically by mvebu HW when link changes status from down to up.

I *think* the intent of the above is:

If DT supplies the 'slot-power-limit-milliwatt' property, program the value in the Slot Power Limit in the Slot Capabilities register and program the Root Port to send a Set_Slot_Power_Limit Message when the Link transitions to DL_Up.

Exactly!

Quoted 4 lines

PCIe r6.0, sec 2.2.8.5 and 7.5.3.9, also say Set_Slot_Power_Limit must be sent on a config write to Slot Capabilities. I don't really understand that, since AFAICS, everything in that register is read-only. But there must be some use case for forcing a message.

I understood it in this way: Capabilities register is read-only hw-init and so firmware / driver can write initialization values into this register. And when firmware / driver is doing this write then Root port should send that Set_Slot_Power_Limit message.

But this is just my interpretation which I thought that makes sense.

Quoted 7 lines
Quoted 3 lines

Slot power limit is read from DT property 'slot-power-limit-milliwatt' and set to mvebu HW. When this DT property is not specified then driver treat it as "Slot Capabilities register has not yet been initialized".

Does this last sentence mean "the Slot Power Limit is set to 0W (Value = 00h and Scale = 00b = 1.0x) and Auto Slot Power Limit Disable is set, so Set_Slot_Power_Limit Messages will never be sent"?

Yes!

Quoted 44 lines
Quoted 44 lines
---
 drivers/pci/controller/pci-mvebu.c | 85 ++++++++++++++++++++++++++++--
 1 file changed, 80 insertions(+), 5 deletions(-)
drivers/pci/controller/pci-mvebu.c +9 -1
index a75d2b9196f9..c786feec4d17 100644
@@ -66,6 +66,12 @@
 #define  PCIE_STAT_BUS                  0xff00
 #define  PCIE_STAT_DEV                  0x1f0000
 #define  PCIE_STAT_LINK_DOWN		BIT(0)
+#define PCIE_SSPL_OFF		0x1a0c
+#define  PCIE_SSPL_VALUE_SHIFT		0
+#define  PCIE_SSPL_VALUE_MASK		GENMASK(7, 0)
+#define  PCIE_SSPL_SCALE_SHIFT		8
+#define  PCIE_SSPL_SCALE_MASK		GENMASK(9, 8)
+#define  PCIE_SSPL_ENABLE		BIT(16)
 #define PCIE_RC_RTSTA		0x1a14
 #define PCIE_DEBUG_CTRL         0x1a60
 #define  PCIE_DEBUG_SOFT_RESET		BIT(20)
@@ -111,6 +117,8 @@ struct mvebu_pcie_port {
 	struct mvebu_pcie_window iowin;
 	u32 saved_pcie_stat;
 	struct resource regs;
+	u8 slot_power_limit_value;
+	u8 slot_power_limit_scale;
 	struct irq_domain *intx_irq_domain;
 	raw_spinlock_t irq_lock;
 	int intx_irq;
@@ -239,7 +247,7 @@ static void mvebu_pcie_setup_wins(struct mvebu_pcie_port *port)
 
 static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 {
-	u32 ctrl, lnkcap, cmd, dev_rev, unmask;
+	u32 ctrl, lnkcap, cmd, dev_rev, unmask, sspl;
 
 	/* Setup PCIe controller to Root Complex mode. */
 	ctrl = mvebu_readl(port, PCIE_CTRL_OFF);
@@ -292,6 +300,20 @@ static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 	/* Point PCIe unit MBUS decode windows to DRAM space. */
 	mvebu_pcie_setup_wins(port);
 
Quoted 7 lines

drivers/pci/controller/pci-mvebu.c static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port) · line 306

Quoted 4 lines
drivers/pci/controller/pci-mvebu.c +4 -0
+	/*
+	 * Program Root Complex to automatically sends Set Slot Power Limit
+	 * PCIe Message when changing status from Dl-Down to Dl-Up and valid
+	 * slot power limit was specified.

s/Root Complex/Root Port/, right? AFAIK the message would be sent by a Downstream Port, i.e., a Root Port in this case.

Yes!

I see that on more places that names "Root Port", "Root Bridge" and "Root Complex" used as the one thing.

It is probably because HW has only one Root Port and is integrated into same silicon as Root Complex and shares HW registers. And Root Port has PCI class code "PCI Bridge", hence Root Bridge.

But I agree that correct name is "Root Port".

Moreover in Armada 38x Functional Specification is this register named "Root Complex Set Slot Power Limit" and not Root "Port".

Quoted 5 lines

s/sends/send/
s/Set Slot Power Limit/Set_Slot_Power_Limit/ to match spec usage (also below)
s/Dl-Down/DL_Down/ to match spec usage
s/Dl-Up/DL_Up/ ditto

In Armada 38x Functional Specification spec it is called like I wrote and some people told me to use "naming" as written in SoC/HW specification to not confuse other people who are writing / developing drivers according to official SoC/HW specification.

I see that both has pro and cons. Usage of terminology from PCIe spec is what PCIe people expect and terminology from vendor SoC HW spec is what people who develop that SoC expect.

I can update and change comments without issue to any variant which you prefer. No problem with it. Just I wanted to write why I chose those names.

Quoted 17 lines
Quoted 17 lines
drivers/pci/controller/pci-mvebu.c +10 -0
+	 */
+	sspl = mvebu_readl(port, PCIE_SSPL_OFF);
+	sspl &= ~(PCIE_SSPL_VALUE_MASK | PCIE_SSPL_SCALE_MASK | PCIE_SSPL_ENABLE);
+	if (port->slot_power_limit_value && port->slot_power_limit_scale) {
+		sspl |= port->slot_power_limit_value << PCIE_SSPL_VALUE_SHIFT;
+		sspl |= port->slot_power_limit_scale << PCIE_SSPL_SCALE_SHIFT;
+		sspl |= PCIE_SSPL_ENABLE;
+	}
+	mvebu_writel(port, sspl, PCIE_SSPL_OFF);
+
 	/* Mask all interrupt sources. */
 	mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_UNMASK_OFF);
 
@@ -628,9 +650,16 @@ mvebu_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge,
 			  (PCI_EXP_LNKSTA_DLLLA << 16) : 0);
 		break;
 
Quoted 11 lines

drivers/pci/controller/pci-mvebu.c mvebu_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge, · line 658

Quoted 8 lines
drivers/pci/controller/pci-mvebu.c +6 -2
-	case PCI_EXP_SLTCTL:
-		*value = PCI_EXP_SLTSTA_PDS << 16;
+	case PCI_EXP_SLTCTL: {
+		u16 slotsta = le16_to_cpu(bridge->pcie_conf.slotsta);
+		u32 val = 0;
+		if (!(mvebu_readl(port, PCIE_SSPL_OFF) & PCIE_SSPL_ENABLE))
+			val |= PCI_EXP_SLTCTL_ASPL_DISABLE;
+		/* PCI_EXP_SLTCTL is 32-bit and contains also slot status bits */

This comment is a little bit confusing because PCI_EXP_SLTCTL is not actually 32 bits; it's 16 bits.

My comment refers to pci-bridge-emul.c PCI_EXP_SLTCTL register, which is 32-bit. That pci-bridge-emul.c driver has 32-bit registers and in places where PCIe register is only 16-bit, it is concatenated to previous 16-bit.

Quoted 4 lines

It's just that we "read" 32 bits,
which includes both PCI_EXP_SLTCTL and PCI_EXP_SLTSTA. If you use "PCI_EXP_SLTCTL", I think it would be helpful to also say "PCI_EXP_SLTSTA" instead of "slot status bits".

I agree, this is misleading and confusing. And because I'm working with pci-bridge-emul.c pci-aardvark.c and pci-mvebu.c for more than year I probably totally forgot about confusion here as I already catch these things...

I will try to change comments to be less confusing.

Quoted 26 lines
Quoted 11 lines
drivers/pci/controller/pci-mvebu.c +3 -0
+		val |= slotsta << 16;
+		*value = val;
 		break;
+	}
 
 	case PCI_EXP_RTSTA:
 		*value = mvebu_readl(port, PCIE_RC_RTSTA);
@@ -774,6 +803,19 @@ mvebu_pci_bridge_emul_pcie_conf_write(struct pci_bridge_emul *bridge,
 		mvebu_writel(port, new, PCIE_CAP_PCIEXP + PCI_EXP_LNKCTL);
 		break;
 

drivers/pci/controller/pci-mvebu.c mvebu_pci_bridge_emul_pcie_conf_write(struct pci_bridge_emul *bridge, · line 815

Quoted 10 lines
drivers/pci/controller/pci-mvebu.c +10 -0
+	case PCI_EXP_SLTCTL:
+		if ((mask & PCI_EXP_SLTCTL_ASPL_DISABLE) &&
+		    port->slot_power_limit_value &&
+		    port->slot_power_limit_scale) {
+			u32 sspl = mvebu_readl(port, PCIE_SSPL_OFF);
+			if (new & PCI_EXP_SLTCTL_ASPL_DISABLE)
+				sspl &= ~PCIE_SSPL_ENABLE;
+			else
+				sspl |= PCIE_SSPL_ENABLE;
+			mvebu_writel(port, sspl, PCIE_SSPL_OFF);

IIUC, the behavior of PCI_EXP_SLTCTL_ASPL_DISABLE as observed by software that sets it and reads it back will depend on whether the DT contains "slot-power-limit-milliwatt".

Quoted 4 lines

If there is no DT property, port->slot_power_limit_value will be zero and PCIE_SSPL_ENABLE will never be set. So if I clear PCI_EXP_SLTCTL_ASPL_DISABLE, then read it back, it looks like it will read as being set.

Yes.

Quoted 1 line

That's not what I would expect from the spec (PCIe r6.0, sec 7.5.3.10).

Ok. What you would expect here? That PCI_EXP_SLTCTL_ASPL_DISABLE is not set even when Set_Slot_Power_Limit was never sent and would be never sent (as it was not programmed by firmware = in DT)?

Quoted 82 lines
Quoted 82 lines
drivers/pci/controller/pci-mvebu.c +38 -2
+		}
+		break;
+
 	case PCI_EXP_RTSTA:
 		/*
 		 * PME Status bit in Root Status Register (PCIE_RC_RTSTA)
@@ -868,8 +910,26 @@ static int mvebu_pci_bridge_emul_init(struct mvebu_pcie_port *port)
 	/*
 	 * Older mvebu hardware provides PCIe Capability structure only in
 	 * version 1. New hardware provides it in version 2.
+	 * Enable slot support which is emulated.
 	 */
-	bridge->pcie_conf.cap = cpu_to_le16(pcie_cap_ver);
+	bridge->pcie_conf.cap = cpu_to_le16(pcie_cap_ver | PCI_EXP_FLAGS_SLOT);
+
+	/*
+	 * Set Presence Detect State bit permanently as there is no support for
+	 * unplugging PCIe card from the slot. Assume that PCIe card is always
+	 * connected in slot.
+	 *
+	 * Set physical slot number to port+1 as mvebu ports are indexed from
+	 * zero and zero value is reserved for ports within the same silicon
+	 * as Root Port which is not mvebu case.
+	 *
+	 * Also set correct slot power limit.
+	 */
+	bridge->pcie_conf.slotcap = cpu_to_le32(
+		(port->slot_power_limit_value << PCI_EXP_SLTCAP_SPLV_SHIFT) |
+		(port->slot_power_limit_scale << PCI_EXP_SLTCAP_SPLS_SHIFT) |
+		((port->port+1) << PCI_EXP_SLTCAP_PSN_SHIFT));
+	bridge->pcie_conf.slotsta = cpu_to_le16(PCI_EXP_SLTSTA_PDS);
 
 	bridge->subsystem_vendor_id = ssdev_id & 0xffff;
 	bridge->subsystem_id = ssdev_id >> 16;
@@ -1191,6 +1251,7 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
 {
 	struct device *dev = &pcie->pdev->dev;
 	enum of_gpio_flags flags;
+	u32 slot_power_limit;
 	int reset_gpio, ret;
 	u32 num_lanes;
 
@@ -1291,6 +1352,15 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
 		port->reset_gpio = gpio_to_desc(reset_gpio);
 	}
 
+	slot_power_limit = of_pci_get_slot_power_limit(child,
+				&port->slot_power_limit_value,
+				&port->slot_power_limit_scale);
+	if (slot_power_limit)
+		dev_info(dev, "%s: Slot power limit %u.%uW\n",
+			 port->name,
+			 slot_power_limit / 1000,
+			 (slot_power_limit / 100) % 10);
+
 	port->clk = of_clk_get_by_name(child, NULL);
 	if (IS_ERR(port->clk)) {
 		dev_err(dev, "%s: cannot get clock\n", port->name);
@@ -1587,7 +1657,7 @@ static int mvebu_pcie_remove(struct platform_device *pdev)
 {
 	struct mvebu_pcie *pcie = platform_get_drvdata(pdev);
 	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
-	u32 cmd;
+	u32 cmd, sspl;
 	int i;
 
 	/* Remove PCI bus with all devices. */
@@ -1624,6 +1694,11 @@ static int mvebu_pcie_remove(struct platform_device *pdev)
 		/* Free config space for emulated root bridge. */
 		pci_bridge_emul_cleanup(&port->bridge);
 
+		/* Disable sending Set Slot Power Limit PCIe Message. */
+		sspl = mvebu_readl(port, PCIE_SSPL_OFF);
+		sspl &= ~(PCIE_SSPL_VALUE_MASK | PCIE_SSPL_SCALE_MASK | PCIE_SSPL_ENABLE);
+		mvebu_writel(port, sspl, PCIE_SSPL_OFF);
+
 		/* Disable and clear BARs and windows. */
 		mvebu_pcie_disable_wins(port);
 
Signature · 1 line
2.20.1
#20 lore raw

Re: [PATCH 5/6] PCI: mvebu: Add support for sending Set_Slot_Power_Limit message

Quoted 17 lines · Pali Rohár
Quoted 10 lines · On Thursday 24 February 2022 15:28:11 Bjorn Helgaas wrote:

drivers/pci/controller/pci-mvebu.c static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port) · line 306

Quoted 6 lines · Pali Rohár

This PCIe message is sent automatically by mvebu HW when link changes status from down to up.

drivers/pci/controller/pci-mvebu.c +3 -0
+	 * Program Root Complex to automatically sends Set Slot Power Limit
+	 * PCIe Message when changing status from Dl-Down to Dl-Up and valid
+	 * slot power limit was specified.

s/Root Complex/Root Port/, right? AFAIK the message would be sent by a Downstream Port, i.e., a Root Port in this case.

Yes!

I see that on more places that names "Root Port", "Root Bridge" and "Root Complex" used as the one thing.

Quoted 8 lines

It is probably because HW has only one Root Port and is integrated into same silicon as Root Complex and shares HW registers. And Root Port has PCI class code "PCI Bridge", hence Root Bridge.

But I agree that correct name is "Root Port".

Moreover in Armada 38x Functional Specification is this register named "Root Complex Set Slot Power Limit" and not Root "Port".

Haha, yes, sounds like this stems from the knowledge that "of course this Root Complex only has one Root Port, so there's no real difference between them."

So I think it makes sense for #defines for device-specific registers like PCIE_SSPL_OFF to match the Armada spec, but I think it would be better if the comments and code structure did not have the assumption that there's only one Root Port baked into them. For one thing, this can help make the driver structure more uniform across all the drivers.

Quoted 11 lines
Quoted 5 lines

s/sends/send/
s/Set Slot Power Limit/Set_Slot_Power_Limit/ to match spec usage (also below)
s/Dl-Down/DL_Down/ to match spec usage
s/Dl-Up/DL_Up/ ditto

In Armada 38x Functional Specification spec it is called like I wrote and some people told me to use "naming" as written in SoC/HW specification to not confuse other people who are writing / developing drivers according to official SoC/HW specification.

Quoted 7 lines

I see that both has pro and cons. Usage of terminology from PCIe spec is what PCIe people expect and terminology from vendor SoC HW spec is what people who develop that SoC expect.

I can update and change comments without issue to any variant which you prefer. No problem with it. Just I wanted to write why I chose those names.

All these concepts are purely PCIe. There is no Armada-specific meaning because they have to behave as specified by the PCIe spec so they work across the Link with non-Armada devices on the other end. If the Armada spec spells them differently, I claim that's an editing mistake in that spec.

My preference is to use the PCIe spec naming except for Armada-specific things. The Armada spellings don't appear in the PCIe spec, so it's just an unnecessary irritant when trying to look them up.

Quoted 20 lines
Quoted 15 lines

drivers/pci/controller/pci-mvebu.c mvebu_pci_bridge_emul_pcie_conf_write(struct pci_bridge_emul *bridge, · line 815

Quoted 10 lines
drivers/pci/controller/pci-mvebu.c +10 -0
+	case PCI_EXP_SLTCTL:
+		if ((mask & PCI_EXP_SLTCTL_ASPL_DISABLE) &&
+		    port->slot_power_limit_value &&
+		    port->slot_power_limit_scale) {
+			u32 sspl = mvebu_readl(port, PCIE_SSPL_OFF);
+			if (new & PCI_EXP_SLTCTL_ASPL_DISABLE)
+				sspl &= ~PCIE_SSPL_ENABLE;
+			else
+				sspl |= PCIE_SSPL_ENABLE;
+			mvebu_writel(port, sspl, PCIE_SSPL_OFF);

IIUC, the behavior of PCI_EXP_SLTCTL_ASPL_DISABLE as observed by software that sets it and reads it back will depend on whether the DT contains "slot-power-limit-milliwatt".

Quoted 4 lines

If there is no DT property, port->slot_power_limit_value will be zero and PCIE_SSPL_ENABLE will never be set. So if I clear PCI_EXP_SLTCTL_ASPL_DISABLE, then read it back, it looks like it will read as being set.

Quoted 7 lines

Yes.

Quoted 1 line

That's not what I would expect from the spec (PCIe r6.0, sec 7.5.3.10).

Ok. What you would expect here? That PCI_EXP_SLTCTL_ASPL_DISABLE is not set even when Set_Slot_Power_Limit was never sent and would be never sent (as it was not programmed by firmware = in DT)?

Per spec, I would expect PCI_EXP_SLTCTL_ASPL_DISABLE to be either:

- Hardwired to 0, so writes have no effect and reads always return

    0, or

- Writable, so a read always returns what was previously written.

Here's the relevant text from r6.0, sec 7.5.3.10:

Auto Slot Power Limit Disable - When Set, this disables the automatic sending of a Set_Slot_Power_Limit Message when a Link transitions from a non-DL_Up status to a DL_Up status.

Downstream ports that don’t support DPC are permitted to hardwire this bit to 0.

Default value of this bit is implementation specific.

AFAICT, the Slot Power Control mechanism is required for all devices, but without "slot-power-limit-milliwatt", we don't know what limit to use. Apparently the CEM specs specify minimum values, but they depend on the form factor.

From r6.0, sec 6.9:

For Adapters:

- Until and unless a Set_Slot_Power_Limit Message is received indicating a Slot Power Limit value greater than the lowest value specified in the form factor specification for the adapter's form factor, the adapter must not consume more than the lowest value specified.

- An adapter must never consume more power than what was specified in the most recently received Set_Slot_Power_Limit Message or the minimum value specified in the corresponding form factor specification, whichever is higher.

If PCIE_SSPL_ENABLE is never set, Set_Slot_Power_Limit will never be sent, and the device is not allowed to consume more than the minimum power specified by its form factor spec.

I'd say reading PCI_EXP_SLTCTL should return whatever PCI_EXP_SLTCTL_ASPL_DISABLE value was most recently written, but we should set PCIE_SSPL_ENABLE only when we have a "slot-power-limit-milliwatt" property AND PCI_EXP_SLTCTL_ASPL_DISABLE == 0.

Does that make sense?

Bjorn

author maintainer
#21 lore raw

Re: [PATCH 5/6] PCI: mvebu: Add support for sending Set_Slot_Power_Limit message

Quoted 31 lines · On Friday 25 February 2022 10:57:31 Bjorn Helgaas wrote:
Quoted 17 lines · Pali Rohár
Quoted 10 lines · On Thursday 24 February 2022 15:28:11 Bjorn Helgaas wrote:

drivers/pci/controller/pci-mvebu.c static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port) · line 306

Quoted 6 lines · Pali Rohár

This PCIe message is sent automatically by mvebu HW when link changes status from down to up.

drivers/pci/controller/pci-mvebu.c +3 -0
+	 * Program Root Complex to automatically sends Set Slot Power Limit
+	 * PCIe Message when changing status from Dl-Down to Dl-Up and valid
+	 * slot power limit was specified.

s/Root Complex/Root Port/, right? AFAIK the message would be sent by a Downstream Port, i.e., a Root Port in this case.

Yes!

I see that on more places that names "Root Port", "Root Bridge" and "Root Complex" used as the one thing.

Quoted 8 lines

It is probably because HW has only one Root Port and is integrated into same silicon as Root Complex and shares HW registers. And Root Port has PCI class code "PCI Bridge", hence Root Bridge.

But I agree that correct name is "Root Port".

Moreover in Armada 38x Functional Specification is this register named "Root Complex Set Slot Power Limit" and not Root "Port".

Haha, yes, sounds like this stems from the knowledge that "of course this Root Complex only has one Root Port, so there's no real difference between them."

Quoted 6 lines

So I think it makes sense for #defines for device-specific registers like PCIE_SSPL_OFF to match the Armada spec, but I think it would be better if the comments and code structure did not have the assumption that there's only one Root Port baked into them. For one thing, this can help make the driver structure more uniform across all the drivers.

Ok!

Quoted 25 lines
Quoted 11 lines
Quoted 5 lines

s/sends/send/
s/Set Slot Power Limit/Set_Slot_Power_Limit/ to match spec usage (also below)
s/Dl-Down/DL_Down/ to match spec usage
s/Dl-Up/DL_Up/ ditto

In Armada 38x Functional Specification spec it is called like I wrote and some people told me to use "naming" as written in SoC/HW specification to not confuse other people who are writing / developing drivers according to official SoC/HW specification.

Quoted 7 lines

I see that both has pro and cons. Usage of terminology from PCIe spec is what PCIe people expect and terminology from vendor SoC HW spec is what people who develop that SoC expect.

I can update and change comments without issue to any variant which you prefer. No problem with it. Just I wanted to write why I chose those names.

All these concepts are purely PCIe. There is no Armada-specific meaning because they have to behave as specified by the PCIe spec so they work across the Link with non-Armada devices on the other end. If the Armada spec spells them differently, I claim that's an editing mistake in that spec.

Quoted 4 lines

My preference is to use the PCIe spec naming except for Armada-specific things. The Armada spellings don't appear in the PCIe spec, so it's just an unnecessary irritant when trying to look them up.

Ok!

Quoted 70 lines
Quoted 20 lines
Quoted 15 lines

drivers/pci/controller/pci-mvebu.c mvebu_pci_bridge_emul_pcie_conf_write(struct pci_bridge_emul *bridge, · line 815

Quoted 10 lines
drivers/pci/controller/pci-mvebu.c +10 -0
+	case PCI_EXP_SLTCTL:
+		if ((mask & PCI_EXP_SLTCTL_ASPL_DISABLE) &&
+		    port->slot_power_limit_value &&
+		    port->slot_power_limit_scale) {
+			u32 sspl = mvebu_readl(port, PCIE_SSPL_OFF);
+			if (new & PCI_EXP_SLTCTL_ASPL_DISABLE)
+				sspl &= ~PCIE_SSPL_ENABLE;
+			else
+				sspl |= PCIE_SSPL_ENABLE;
+			mvebu_writel(port, sspl, PCIE_SSPL_OFF);

IIUC, the behavior of PCI_EXP_SLTCTL_ASPL_DISABLE as observed by software that sets it and reads it back will depend on whether the DT contains "slot-power-limit-milliwatt".

Quoted 4 lines

If there is no DT property, port->slot_power_limit_value will be zero and PCIE_SSPL_ENABLE will never be set. So if I clear PCI_EXP_SLTCTL_ASPL_DISABLE, then read it back, it looks like it will read as being set.

Quoted 7 lines

Yes.

Quoted 1 line

That's not what I would expect from the spec (PCIe r6.0, sec 7.5.3.10).

Ok. What you would expect here? That PCI_EXP_SLTCTL_ASPL_DISABLE is not set even when Set_Slot_Power_Limit was never sent and would be never sent (as it was not programmed by firmware = in DT)?

Per spec, I would expect PCI_EXP_SLTCTL_ASPL_DISABLE to be either:

- Hardwired to 0, so writes have no effect and reads always return

    0, or

- Writable, so a read always returns what was previously written.

Here's the relevant text from r6.0, sec 7.5.3.10:

Auto Slot Power Limit Disable - When Set, this disables the automatic sending of a Set_Slot_Power_Limit Message when a Link transitions from a non-DL_Up status to a DL_Up status.

Downstream ports that don’t support DPC are permitted to hardwire this bit to 0.

Default value of this bit is implementation specific.

AFAICT, the Slot Power Control mechanism is required for all devices, but without "slot-power-limit-milliwatt", we don't know what limit to use. Apparently the CEM specs specify minimum values, but they depend on the form factor.

From r6.0, sec 6.9:

For Adapters:

- Until and unless a Set_Slot_Power_Limit Message is received indicating a Slot Power Limit value greater than the lowest value specified in the form factor specification for the adapter's form factor, the adapter must not consume more than the lowest value specified.

- An adapter must never consume more power than what was specified in the most recently received Set_Slot_Power_Limit Message or the minimum value specified in the corresponding form factor specification, whichever is higher.

If PCIE_SSPL_ENABLE is never set, Set_Slot_Power_Limit will never be sent, and the device is not allowed to consume more than the minimum power specified by its form factor spec.

Quoted 7 lines

I'd say reading PCI_EXP_SLTCTL should return whatever PCI_EXP_SLTCTL_ASPL_DISABLE value was most recently written, but we should set PCIE_SSPL_ENABLE only when we have a "slot-power-limit-milliwatt" property AND PCI_EXP_SLTCTL_ASPL_DISABLE == 0.

Does that make sense?

Yes!

#22 lore raw

Re: [PATCH 5/6] PCI: mvebu: Add support for sending Set_Slot_Power_Limit message

In reply to Pali Rohár
Quoted 10 lines · Pali Rohár
Quoted 8 lines · On Thursday 24 February 2022 15:28:11 Bjorn Helgaas wrote:
Quoted 2 lines · Pali Rohár

This PCIe message is sent automatically by mvebu HW when link changes status from down to up.

PCIe r6.0, sec 2.2.8.5 and 7.5.3.9, also say Set_Slot_Power_Limit must be sent on a config write to Slot Capabilities. I don't really understand that, since AFAICS, everything in that register is read-only. But there must be some use case for forcing a message.

Quoted 4 lines

I understood it in this way: Capabilities register is read-only hw-init and so firmware / driver can write initialization values into this register. And when firmware / driver is doing this write then Root port should send that Set_Slot_Power_Limit message.

Sec 7.5.3.9 describes the behavior of Slot Capabilities in config space, where it must be read-only. Firmware (or the mvebu driver) must use a different mechanism to initialize the values.

FWIW, I found this implementation note in PCIe r6.0, sec 6.9 that explains why config writes to this read-only register would be useful:

IMPLEMENTATION NOTE: AUTO SLOT POWER LIMIT DISABLE

In some environments host software may wish to directly manage the transmission of a Set_Slot_Power_Limit message by performing a Configuration Write to the Slot Capabilities register rather than have the transmission automatically occur when the Link transitions from a non-DL_Up to a DL_Up status. This allows host software to limit power supply surge current by staggering the transition of Endpoints to a higher power state following a Link Down or when multiple Endpoints are simultaneously hot-added due to cable or adapter insertion.

Bjorn

author maintainer
#23 lore raw

Re: [PATCH 5/6] PCI: mvebu: Add support for sending Set_Slot_Power_Limit message

Quoted 35 lines · On Friday 25 February 2022 11:02:25 Bjorn Helgaas wrote:
Quoted 10 lines · Pali Rohár
Quoted 8 lines · On Thursday 24 February 2022 15:28:11 Bjorn Helgaas wrote:
Quoted 2 lines · Pali Rohár

This PCIe message is sent automatically by mvebu HW when link changes status from down to up.

PCIe r6.0, sec 2.2.8.5 and 7.5.3.9, also say Set_Slot_Power_Limit must be sent on a config write to Slot Capabilities. I don't really understand that, since AFAICS, everything in that register is read-only. But there must be some use case for forcing a message.

Quoted 4 lines

I understood it in this way: Capabilities register is read-only hw-init and so firmware / driver can write initialization values into this register. And when firmware / driver is doing this write then Root port should send that Set_Slot_Power_Limit message.

Sec 7.5.3.9 describes the behavior of Slot Capabilities in config space, where it must be read-only. Firmware (or the mvebu driver) must use a different mechanism to initialize the values.

FWIW, I found this implementation note in PCIe r6.0, sec 6.9 that explains why config writes to this read-only register would be useful:

IMPLEMENTATION NOTE: AUTO SLOT POWER LIMIT DISABLE

In some environments host software may wish to directly manage the transmission of a Set_Slot_Power_Limit message by performing a Configuration Write to the Slot Capabilities register rather than have the transmission automatically occur when the Link transitions from a non-DL_Up to a DL_Up status. This allows host software to limit power supply surge current by staggering the transition of Endpoints to a higher power state following a Link Down or when multiple Endpoints are simultaneously hot-added due to cable or adapter insertion.

Quoted 1 line

Bjorn

Hm... I did not understand from this description what should happen when write operation is performed to the slot capabilities register. It is allowed to change content of this register?

Or better question, what should write hook in pci-mvebu.c driver for slot capabilities do?

author maintainer
#24 lore raw

PATCH 6/6 ARM: dts: turris-omnia: Set PCIe slot-power-limit-milliwatt properties

In reply to Pali Rohár

All 3 miniPCIe slots in Turris Omnia are designed for 10 W.

---
 arch/arm/boot/dts/armada-385-turris-omnia.dts | 3 +++
 1 file changed, 3 insertions(+)
arch/arm/boot/dts/armada-385-turris-omnia.dts +3 -0
index 5bd6a66d2c2b..f240018148f6 100644
@@ -71,16 +71,19 @@
 			pcie@1,0 {
 				/* Port 0, Lane 0 */
 				status = "okay";
+				slot-power-limit-milliwatt = <10000>;
 			};
 
 			pcie@2,0 {
 				/* Port 1, Lane 0 */
 				status = "okay";
+				slot-power-limit-milliwatt = <10000>;
 			};
 
 			pcie@3,0 {
 				/* Port 2, Lane 0 */
 				status = "okay";
+				slot-power-limit-milliwatt = <10000>;
 			};
 		};
 	};
Signature · 1 line
2.20.1
#25 lore raw

Re: [PATCH 6/6] ARM: dts: turris-omnia: Set PCIe slot-power-limit-milliwatt properties

Quoted 3 lines · Pali Rohár

All 3 miniPCIe slots in Turris Omnia are designed for 10 W.

Applied on mvebu/dt64

Thanks,

Gregory

Quoted 31 lines
---
 arch/arm/boot/dts/armada-385-turris-omnia.dts | 3 +++
 1 file changed, 3 insertions(+)
arch/arm/boot/dts/armada-385-turris-omnia.dts +3 -0
index 5bd6a66d2c2b..f240018148f6 100644
@@ -71,16 +71,19 @@
 			pcie@1,0 {
 				/* Port 0, Lane 0 */
 				status = "okay";
+				slot-power-limit-milliwatt = <10000>;
 			};
 
 			pcie@2,0 {
 				/* Port 1, Lane 0 */
 				status = "okay";
+				slot-power-limit-milliwatt = <10000>;
 			};
 
 			pcie@3,0 {
 				/* Port 2, Lane 0 */
 				status = "okay";
+				slot-power-limit-milliwatt = <10000>;
 			};
 		};
 	};
Signature · 1 line
2.20.1
Signature · 3 lines
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com