On Fri, Feb 25, 2022 at 01:30:51PM +0100, Pali Rohár wrote:
> On Thursday 24 February 2022 14:47:15 Bjorn Helgaas wrote:
> > On Tue, Feb 22, 2022 at 05:31:56PM +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@xxxxxxxxxx>
> > > Signed-off-by: Marek Behún <kabel@xxxxxxxxxx>
> > > Reviewed-by: Rob Herring <robh@xxxxxxxxxx>
> > > Acked-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > > ---
> > > 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..2b0c0a3641a8 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;
> >
> > Including "mw" or similar reference to the units would give a hint of
> > how to relate the code to the spec.
> >
> > > + 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.
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.
> > > + 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;
> >
> > 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.
>
> > 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?
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
