Message-ID: <20220225165731.GA359939@bhelgaas>
Date:   Fri, 25 Feb 2022 10:57:31 -0600
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Pali Rohár <pali@...nel.org>
Cc:     Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Rob Herring <robh+dt@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Marek Behún <kabel@...nel.org>,
        Russell King <rmk+kernel@...linux.org.uk>,
        Gregory Clement <gregory.clement@...tlin.com>,
        linux-pci@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/6] PCI: mvebu: Add support for sending
 Set_Slot_Power_Limit message

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.

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

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

Bjorn

Powered by blists - more mailing lists