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
Changes in v3:
* Set 600 W when DT slot-power-limit-milliwatt > 600 W
Changes in v2:
* Dropped patch with PCI_EXP_SLTCAP_*_SHIFT macros as it is not needed anymore
* Dropped patch "ARM: dts: turris-omnia: Set PCIe slot-power-limit-milliwatt properties" which was applied
* Added support for PCIe 6.0 slot power limit encodings
* Round down slot power limit value
* Fix handling of slot power limit with scale x1.0 (0x00 value)
* Use FIELD_PREP instead of _SHIFT macros
* Changed commit message to Bjorn's suggestion
* Changed comments in the code to match PCIe spec
* Preserve user settings of PCI_EXP_SLTCTL_ASPL_DISABLE bit
Pali Rohár (4):
PCI: Add PCI_EXP_SLTCTL_ASPL_DISABLE macro
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
Documentation/devicetree/bindings/pci/pci.txt | 6 ++
drivers/pci/controller/pci-mvebu.c | 96 ++++++++++++++++++-
drivers/pci/of.c | 64 +++++++++++++
drivers/pci/pci.h | 15 +++
include/uapi/linux/pci_regs.h | 1 +
5 files changed, 177 insertions(+), 5 deletions(-)
--
2.20.1
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.
Signed-off-by: Pali Rohár <[email protected]>
Signed-off-by: Marek Behún <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
Changes in v3:
* Set 600 W when DT slot-power-limit-milliwatt > 600 W
Changes in v2:
* Added support for PCIe 6.0 slot power limit encodings
* Round down slot power limit value
---
drivers/pci/of.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++
drivers/pci/pci.h | 15 +++++++++++
2 files changed, 79 insertions(+)
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index cb2e8351c2cc..5ebff26edd41 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -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_mw;
+ u8 value, scale;
+
+ if (of_property_read_u32(node, "slot-power-limit-milliwatt",
+ &slot_power_limit_mw))
+ slot_power_limit_mw = 0;
+
+ /* Calculate Slot Power Limit Value and Slot Power Limit Scale */
+ if (slot_power_limit_mw == 0) {
+ value = 0x00;
+ scale = 0;
+ } else if (slot_power_limit_mw <= 255) {
+ value = slot_power_limit_mw;
+ scale = 3;
+ } else if (slot_power_limit_mw <= 255*10) {
+ value = slot_power_limit_mw / 10;
+ scale = 2;
+ } else if (slot_power_limit_mw <= 255*100) {
+ value = slot_power_limit_mw / 100;
+ scale = 1;
+ } else if (slot_power_limit_mw <= 239*1000) {
+ value = slot_power_limit_mw / 1000;
+ scale = 0;
+ } else if (slot_power_limit_mw <= 250*1000) {
+ value = 0xF0;
+ scale = 0;
+ } else if (slot_power_limit_mw <= 600*1000) {
+ value = 0xF0 + (slot_power_limit_mw / 1000 - 250) / 25;
+ scale = 0;
+ } else {
+ value = 0xFE;
+ 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_mw;
+}
+EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 3d60cabde1a1..e10cdec6c56e 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -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) { }
--
2.20.1
Add macro defining Auto Slot Power Limit Disable bit in Slot Control
Register.
Signed-off-by: Pali Rohár <[email protected]>
Signed-off-by: Marek Behún <[email protected]>
Acked-by: Bjorn Helgaas <[email protected]>
---
include/uapi/linux/pci_regs.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index bee1a9ed6e66..108f8523fa04 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -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 */
--
2.20.1
On Fri, Mar 25, 2022 at 10:38:26AM +0100, Pali Roh?r wrote:
> 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.
>
> Signed-off-by: Pali Roh?r <[email protected]>
> Signed-off-by: Marek Beh?n <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
> ---
> Changes in v3:
> * Set 600 W when DT slot-power-limit-milliwatt > 600 W
> Changes in v2:
> * Added support for PCIe 6.0 slot power limit encodings
> * Round down slot power limit value
> ---
> drivers/pci/of.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++
> drivers/pci/pci.h | 15 +++++++++++
> 2 files changed, 79 insertions(+)
>
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index cb2e8351c2cc..5ebff26edd41 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -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_mw;
> + u8 value, scale;
> +
> + if (of_property_read_u32(node, "slot-power-limit-milliwatt",
> + &slot_power_limit_mw))
> + slot_power_limit_mw = 0;
> +
> + /* Calculate Slot Power Limit Value and Slot Power Limit Scale */
> + if (slot_power_limit_mw == 0) {
> + value = 0x00;
> + scale = 0;
> + } else if (slot_power_limit_mw <= 255) {
> + value = slot_power_limit_mw;
> + scale = 3;
> + } else if (slot_power_limit_mw <= 255*10) {
> + value = slot_power_limit_mw / 10;
> + scale = 2;
> + } else if (slot_power_limit_mw <= 255*100) {
> + value = slot_power_limit_mw / 100;
> + scale = 1;
> + } else if (slot_power_limit_mw <= 239*1000) {
> + value = slot_power_limit_mw / 1000;
> + scale = 0;
> + } else if (slot_power_limit_mw <= 250*1000) {
> + value = 0xF0;
> + scale = 0;
I think the spec is poorly worded here. PCIe r6.0, sec 7.5.3.9, says:
F0h > 239 W and <= 250 W Slot Power Limit
I don't think it's meaningful for the spec to include a range here.
The amount of power the slot can supply has a single maximum. I
suspect the *intent* of F0h/00b is that a device in the slot may
consume up to 250W.
Your code above would mean that slot_power_limit_mw == 245,000 would
cause the slot to advertise F0h/00b (250W), which seems wrong.
I think we should do something like this instead:
scale = 0;
if (slot_power_limit_mw >= 600*1000) {
value = 0xFE;
slot_power_limit_mw = 600*1000;
} else if (slot_power_limit_mw >= 575*1000) {
value = 0xFD;
slot_power_limit_mw = 575*1000;
} ...
I raised an issue with the PCI SIG about this.
> + } else if (slot_power_limit_mw <= 600*1000) {
> + value = 0xF0 + (slot_power_limit_mw / 1000 - 250) / 25;
> + scale = 0;
> + } else {
> + value = 0xFE;
> + 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_mw;
If the DT tells us 800W is available, we'll store (FEh/00b), which
means the slot can advertise to a downstream device that 600W is
available. I think that's correct, since the current spec doesn't
provide a way to encode any value larger than 600W.
But the function still returns 800,000 mW, which means the next patch will
print:
%s: Slot power limit 800.0W
even though it programs Slot Capabilities to advertise 600W.
That's why I suggested setting slot_power_limit_mw = 600*1000 above.
> +}
> +EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 3d60cabde1a1..e10cdec6c56e 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -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) { }
> --
> 2.20.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Friday 08 April 2022 10:27:50 Bjorn Helgaas wrote:
> On Fri, Mar 25, 2022 at 10:38:26AM +0100, Pali Rohár wrote:
> > 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.
> >
> > Signed-off-by: Pali Rohár <[email protected]>
> > Signed-off-by: Marek Behún <[email protected]>
> > Reviewed-by: Rob Herring <[email protected]>
> > ---
> > Changes in v3:
> > * Set 600 W when DT slot-power-limit-milliwatt > 600 W
> > Changes in v2:
> > * Added support for PCIe 6.0 slot power limit encodings
> > * Round down slot power limit value
> > ---
> > drivers/pci/of.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++
> > drivers/pci/pci.h | 15 +++++++++++
> > 2 files changed, 79 insertions(+)
> >
> > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > index cb2e8351c2cc..5ebff26edd41 100644
> > --- a/drivers/pci/of.c
> > +++ b/drivers/pci/of.c
> > @@ -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_mw;
> > + u8 value, scale;
> > +
> > + if (of_property_read_u32(node, "slot-power-limit-milliwatt",
> > + &slot_power_limit_mw))
> > + slot_power_limit_mw = 0;
> > +
> > + /* Calculate Slot Power Limit Value and Slot Power Limit Scale */
> > + if (slot_power_limit_mw == 0) {
> > + value = 0x00;
> > + scale = 0;
> > + } else if (slot_power_limit_mw <= 255) {
> > + value = slot_power_limit_mw;
> > + scale = 3;
> > + } else if (slot_power_limit_mw <= 255*10) {
> > + value = slot_power_limit_mw / 10;
> > + scale = 2;
> > + } else if (slot_power_limit_mw <= 255*100) {
> > + value = slot_power_limit_mw / 100;
> > + scale = 1;
> > + } else if (slot_power_limit_mw <= 239*1000) {
> > + value = slot_power_limit_mw / 1000;
> > + scale = 0;
> > + } else if (slot_power_limit_mw <= 250*1000) {
> > + value = 0xF0;
> > + scale = 0;
>
> I think the spec is poorly worded here. PCIe r6.0, sec 7.5.3.9, says:
>
> F0h > 239 W and <= 250 W Slot Power Limit
>
> I don't think it's meaningful for the spec to include a range here.
> The amount of power the slot can supply has a single maximum. I
> suspect the *intent* of F0h/00b is that a device in the slot may
> consume up to 250W.
>
> Your code above would mean that slot_power_limit_mw == 245,000 would
> cause the slot to advertise F0h/00b (250W), which seems wrong.
So for slot_power_limit_mw == 245 W we should set following values?
slot_power_limit_mw = 239 W
value = 0xF0
scale = 0
> I think we should do something like this instead:
>
> scale = 0;
> if (slot_power_limit_mw >= 600*1000) {
> value = 0xFE;
> slot_power_limit_mw = 600*1000;
> } else if (slot_power_limit_mw >= 575*1000) {
> value = 0xFD;
> slot_power_limit_mw = 575*1000;
> } ...
This is already implemented in branch:
} else if (slot_power_limit_mw <= 600*1000) {
value = 0xF0 + (slot_power_limit_mw / 1000 - 250) / 25;
scale = 0;
I will just add reducing of final slot_power_limit_mw value.
> I raised an issue with the PCI SIG about this.
>
> > + } else if (slot_power_limit_mw <= 600*1000) {
> > + value = 0xF0 + (slot_power_limit_mw / 1000 - 250) / 25;
> > + scale = 0;
> > + } else {
> > + value = 0xFE;
> > + 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_mw;
>
> If the DT tells us 800W is available, we'll store (FEh/00b), which
> means the slot can advertise to a downstream device that 600W is
> available. I think that's correct, since the current spec doesn't
> provide a way to encode any value larger than 600W.
>
> But the function still returns 800,000 mW, which means the next patch will
> print:
>
> %s: Slot power limit 800.0W
>
> even though it programs Slot Capabilities to advertise 600W.
> That's why I suggested setting slot_power_limit_mw = 600*1000 above.
Ok, I will update slot_power_limit_mw value in next patch version.
> > +}
> > +EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 3d60cabde1a1..e10cdec6c56e 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -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) { }
> > --
> > 2.20.1
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Monday 11 April 2022 13:14:07 Pali Rohár wrote:
> On Friday 08 April 2022 10:27:50 Bjorn Helgaas wrote:
> > On Fri, Mar 25, 2022 at 10:38:26AM +0100, Pali Rohár wrote:
> > > 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.
> > >
> > > Signed-off-by: Pali Rohár <[email protected]>
> > > Signed-off-by: Marek Behún <[email protected]>
> > > Reviewed-by: Rob Herring <[email protected]>
> > > ---
> > > Changes in v3:
> > > * Set 600 W when DT slot-power-limit-milliwatt > 600 W
> > > Changes in v2:
> > > * Added support for PCIe 6.0 slot power limit encodings
> > > * Round down slot power limit value
> > > ---
> > > drivers/pci/of.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++
> > > drivers/pci/pci.h | 15 +++++++++++
> > > 2 files changed, 79 insertions(+)
> > >
> > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > > index cb2e8351c2cc..5ebff26edd41 100644
> > > --- a/drivers/pci/of.c
> > > +++ b/drivers/pci/of.c
> > > @@ -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_mw;
> > > + u8 value, scale;
> > > +
> > > + if (of_property_read_u32(node, "slot-power-limit-milliwatt",
> > > + &slot_power_limit_mw))
> > > + slot_power_limit_mw = 0;
> > > +
> > > + /* Calculate Slot Power Limit Value and Slot Power Limit Scale */
> > > + if (slot_power_limit_mw == 0) {
> > > + value = 0x00;
> > > + scale = 0;
> > > + } else if (slot_power_limit_mw <= 255) {
> > > + value = slot_power_limit_mw;
> > > + scale = 3;
> > > + } else if (slot_power_limit_mw <= 255*10) {
> > > + value = slot_power_limit_mw / 10;
> > > + scale = 2;
> > > + } else if (slot_power_limit_mw <= 255*100) {
> > > + value = slot_power_limit_mw / 100;
> > > + scale = 1;
> > > + } else if (slot_power_limit_mw <= 239*1000) {
> > > + value = slot_power_limit_mw / 1000;
> > > + scale = 0;
> > > + } else if (slot_power_limit_mw <= 250*1000) {
> > > + value = 0xF0;
> > > + scale = 0;
> >
> > I think the spec is poorly worded here. PCIe r6.0, sec 7.5.3.9, says:
> >
> > F0h > 239 W and <= 250 W Slot Power Limit
> >
> > I don't think it's meaningful for the spec to include a range here.
> > The amount of power the slot can supply has a single maximum. I
> > suspect the *intent* of F0h/00b is that a device in the slot may
> > consume up to 250W.
> >
> > Your code above would mean that slot_power_limit_mw == 245,000 would
> > cause the slot to advertise F0h/00b (250W), which seems wrong.
>
> So for slot_power_limit_mw == 245 W we should set following values?
>
> slot_power_limit_mw = 239 W
> value = 0xF0
> scale = 0
I changed it in v4
> > I think we should do something like this instead:
> >
> > scale = 0;
> > if (slot_power_limit_mw >= 600*1000) {
> > value = 0xFE;
> > slot_power_limit_mw = 600*1000;
> > } else if (slot_power_limit_mw >= 575*1000) {
> > value = 0xFD;
> > slot_power_limit_mw = 575*1000;
> > } ...
>
> This is already implemented in branch:
>
> } else if (slot_power_limit_mw <= 600*1000) {
> value = 0xF0 + (slot_power_limit_mw / 1000 - 250) / 25;
> scale = 0;
>
> I will just add reducing of final slot_power_limit_mw value.
>
> > I raised an issue with the PCI SIG about this.
> >
> > > + } else if (slot_power_limit_mw <= 600*1000) {
> > > + value = 0xF0 + (slot_power_limit_mw / 1000 - 250) / 25;
> > > + scale = 0;
> > > + } else {
> > > + value = 0xFE;
> > > + 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_mw;
> >
> > If the DT tells us 800W is available, we'll store (FEh/00b), which
> > means the slot can advertise to a downstream device that 600W is
> > available. I think that's correct, since the current spec doesn't
> > provide a way to encode any value larger than 600W.
> >
> > But the function still returns 800,000 mW, which means the next patch will
> > print:
> >
> > %s: Slot power limit 800.0W
> >
> > even though it programs Slot Capabilities to advertise 600W.
> > That's why I suggested setting slot_power_limit_mw = 600*1000 above.
>
> Ok, I will update slot_power_limit_mw value in next patch version.
And also fixed this in v4.
Please review v4 if is is OK now.
> > > +}
> > > +EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
> > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > index 3d60cabde1a1..e10cdec6c56e 100644
> > > --- a/drivers/pci/pci.h
> > > +++ b/drivers/pci/pci.h
> > > @@ -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) { }
> > > --
> > > 2.20.1
> > >
> > >
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > [email protected]
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Apr 11, 2022 at 01:14:07PM +0200, Pali Roh?r wrote:
> On Friday 08 April 2022 10:27:50 Bjorn Helgaas wrote:
> > On Fri, Mar 25, 2022 at 10:38:26AM +0100, Pali Roh?r wrote:
> > > 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.
> > >
> > > Signed-off-by: Pali Roh?r <[email protected]>
> > > Signed-off-by: Marek Beh?n <[email protected]>
> > > Reviewed-by: Rob Herring <[email protected]>
> > > ---
> > > Changes in v3:
> > > * Set 600 W when DT slot-power-limit-milliwatt > 600 W
> > > Changes in v2:
> > > * Added support for PCIe 6.0 slot power limit encodings
> > > * Round down slot power limit value
> > > ---
> > > drivers/pci/of.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++
> > > drivers/pci/pci.h | 15 +++++++++++
> > > 2 files changed, 79 insertions(+)
> > >
> > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > > index cb2e8351c2cc..5ebff26edd41 100644
> > > --- a/drivers/pci/of.c
> > > +++ b/drivers/pci/of.c
> > > @@ -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_mw;
> > > + u8 value, scale;
> > > +
> > > + if (of_property_read_u32(node, "slot-power-limit-milliwatt",
> > > + &slot_power_limit_mw))
> > > + slot_power_limit_mw = 0;
> > > +
> > > + /* Calculate Slot Power Limit Value and Slot Power Limit Scale */
> > > + if (slot_power_limit_mw == 0) {
> > > + value = 0x00;
> > > + scale = 0;
> > > + } else if (slot_power_limit_mw <= 255) {
> > > + value = slot_power_limit_mw;
> > > + scale = 3;
> > > + } else if (slot_power_limit_mw <= 255*10) {
> > > + value = slot_power_limit_mw / 10;
> > > + scale = 2;
> > > + } else if (slot_power_limit_mw <= 255*100) {
> > > + value = slot_power_limit_mw / 100;
> > > + scale = 1;
> > > + } else if (slot_power_limit_mw <= 239*1000) {
> > > + value = slot_power_limit_mw / 1000;
> > > + scale = 0;
> > > + } else if (slot_power_limit_mw <= 250*1000) {
> > > + value = 0xF0;
> > > + scale = 0;
> >
> > I think the spec is poorly worded here. PCIe r6.0, sec 7.5.3.9, says:
> >
> > F0h > 239 W and <= 250 W Slot Power Limit
> >
> > I don't think it's meaningful for the spec to include a range here.
> > The amount of power the slot can supply has a single maximum. I
> > suspect the *intent* of F0h/00b is that a device in the slot may
> > consume up to 250W.
> >
> > Your code above would mean that slot_power_limit_mw == 245,000 would
> > cause the slot to advertise F0h/00b (250W), which seems wrong.
>
> So for slot_power_limit_mw == 245 W we should set following values?
>
> slot_power_limit_mw = 239 W
> value = 0xF0
> scale = 0
I think Slot Cap should never advertise more power than the slot can
supply. So if the DT tells us the slot can supply 245 W, I don't
think Slot Cap should advertise that it can supply 250 W. I think we
should drop down to the next lower possible value, which is 239 W
(value 0xEF, scale 0). I think this is what your v4 does.
> > I think we should do something like this instead:
> >
> > scale = 0;
> > if (slot_power_limit_mw >= 600*1000) {
> > value = 0xFE;
> > slot_power_limit_mw = 600*1000;
> > } else if (slot_power_limit_mw >= 575*1000) {
> > value = 0xFD;
> > slot_power_limit_mw = 575*1000;
> > } ...
>
> This is already implemented in branch:
>
> } else if (slot_power_limit_mw <= 600*1000) {
> value = 0xF0 + (slot_power_limit_mw / 1000 - 250) / 25;
> scale = 0;
OK, I was thinking there was a hole here, but I guess not. I think do
think it's easier to read and verify if it's structured as "the slot
can supply at least X, so advertise X", as opposed to "the slot can
supply X or less, so advertise Y".
Bjorn
On Fri, Apr 08, 2022 at 10:27:50AM -0500, Bjorn Helgaas wrote:
> On Fri, Mar 25, 2022 at 10:38:26AM +0100, Pali Roh?r wrote:
> > 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.
> ...
> I think the spec is poorly worded here. PCIe r6.0, sec 7.5.3.9, says:
>
> F0h > 239 W and <= 250 W Slot Power Limit
>
> I don't think it's meaningful for the spec to include a range here.
> The amount of power the slot can supply has a single maximum. I
> suspect the *intent* of F0h/00b is that a device in the slot may
> consume up to 250W.
>
> Your code above would mean that slot_power_limit_mw == 245,000 would
> cause the slot to advertise F0h/00b (250W), which seems wrong.
>
> I think we should do something like this instead:
>
> scale = 0;
> if (slot_power_limit_mw >= 600*1000) {
> value = 0xFE;
> slot_power_limit_mw = 600*1000;
> } else if (slot_power_limit_mw >= 575*1000) {
> value = 0xFD;
> slot_power_limit_mw = 575*1000;
> } ...
>
> I raised an issue with the PCI SIG about this.
Just to close the loop here, a PCI SIG moderator agrees that F0h
should mean up to 250 W may be consumed. My question as posed:
7.5.3.9 defines alternate encodings for Slot Power Limit Values
F0h-FEh when Slot Power Limit Scale is 00b. For example:
F0h > 239 W and <= 250 W Slot Power Limit
How should an Upstream Port interpret a Set_Slot_Power_Limit message
with a payload of Scale 00b and Value F0h? Obviously the device may
consume up to 239 W. Is it allowed to consume 240 W? 245 W? 250 W?
If it is allowed to consume 250 W, I suggest that there is no reason
to mention 239 W at all, and I suggest that the table be reworked so
each encoding specifies a single limit, e.g.,
F0h 250 W Slot Power Limit
F1h 275 W Slot Power Limit
This question arises because a Linux Device Tree may specify the
amount of power a slot can supply. If the Device Tree says a slot
can supply 245 W, how should Slot Power Limit Value/Scale be
programmed? F0h/00b because 245 is between 239 and 250? Or EFh/00b
(239 W) because F0h/00b actually means the slot must be able to
supply 250 W and this slot can't do that?
PCI-SIG MODERATOR RESPONSE:
A setting of F0h should be interpreted as allowing up to 250 W to be
consumed. I agree that it makes sense to list the limits as:
F0h 250 W Slot Power Limit
F1h 275 W Slot Power Limit
...
I will propose your suggested simplification to the Protocol Work Group.
For PCI-SIG members, this discussion is at
https://forum.pcisig.com/viewtopic.php?f=616&p=3914#p3914
Bjorn