| Message ID | 20220325093827.4983-4-pali@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI: mvebu: Slot support | expand |
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 <pali@kernel.org> > Signed-off-by: Marek Behún <kabel@kernel.org> > Reviewed-by: Rob Herring <robh@kernel.org> > --- > 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 > linux-arm-kernel@lists.infradead.org > 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 <pali@kernel.org> > > Signed-off-by: Marek Behún <kabel@kernel.org> > > Reviewed-by: Rob Herring <robh@kernel.org> > > --- > > 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 > > linux-arm-kernel@lists.infradead.org > > 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 <pali@kernel.org> > > > Signed-off-by: Marek Behún <kabel@kernel.org> > > > Reviewed-by: Rob Herring <robh@kernel.org> > > > --- > > > 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 > > > linux-arm-kernel@lists.infradead.org > > > 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 <pali@kernel.org> > > > Signed-off-by: Marek Behún <kabel@kernel.org> > > > Reviewed-by: Rob Herring <robh@kernel.org> > > > --- > > > 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
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) { }