by Pali Rohár

[permalink] [raw]

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

On Friday 25 February 2022 10:57:31 Bjorn Helgaas wrote:
> On Fri, Feb 25, 2022 at 01:54:07PM +0100, Pali Rohár wrote:
> > On Thursday 24 February 2022 15:28:11 Bjorn Helgaas wrote:
> > > On Tue, Feb 22, 2022 at 05:31:57PM +0100, Pali Rohár wrote:
> > > > This PCIe message is sent automatically by mvebu HW when link changes
> > > > status from down to up.
>
> > > > + * 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".
>
> 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.

Ok!

> > > 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.
>
> 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.

Ok!

> > > > + 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.
> >
> > Yes.
> >
> > > 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?

Yes!