From: "Pali Rohár" <pali@kernel.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
"Bjorn Helgaas" <bhelgaas@google.com>,
"Rob Herring" <robh+dt@kernel.org>,
"Andrew Lunn" <andrew@lunn.ch>,
"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
"Krzysztof Wilczyński" <kw@linux.com>,
"Marek Behún" <kabel@kernel.org>,
"Russell King" <rmk+kernel@armlinux.org.uk>,
"Gregory Clement" <gregory.clement@bootlin.com>,
linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 5/6] PCI: mvebu: Add support for sending Set_Slot_Power_Limit message
Date: Tue, 1 Mar 2022 10:47:07 +0100 [thread overview]
Message-ID: <20220301094707.64jbqpoxhmana7ua@pali> (raw)
In-Reply-To: <20220225165731.GA359939@bhelgaas>
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!
next prev parent reply other threads:[~2022-03-01 9:47 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-22 16:31 [PATCH 0/6] PCI: mvebu: Slot support Pali Rohár
2022-02-22 16:31 ` [PATCH 1/6] PCI: Add PCI_EXP_SLTCTL_ASPL_DISABLE macro Pali Rohár
2022-02-24 20:13 ` Bjorn Helgaas
2022-02-22 16:31 ` [PATCH 2/6] PCI: Add PCI_EXP_SLTCAP_*_SHIFT macros Pali Rohár
2022-02-24 20:28 ` Bjorn Helgaas
2022-02-25 12:24 ` Pali Rohár
2022-02-25 15:37 ` Bjorn Helgaas
2022-02-25 17:22 ` Marek Behún
2022-02-25 17:51 ` Bjorn Helgaas
2022-02-22 16:31 ` [PATCH 3/6] dt-bindings: Add 'slot-power-limit-milliwatt' PCIe port property Pali Rohár
2022-02-22 17:24 ` Marek Behún
2022-02-22 17:53 ` Pali Rohár
2022-02-22 16:31 ` [PATCH 4/6] PCI: Add function for parsing 'slot-power-limit-milliwatt' DT property Pali Rohár
2022-02-24 20:47 ` Bjorn Helgaas
2022-02-25 12:30 ` Pali Rohár
2022-02-25 15:51 ` Bjorn Helgaas
2022-02-25 17:58 ` Pali Rohár
2022-02-22 16:31 ` [PATCH 5/6] PCI: mvebu: Add support for sending Set_Slot_Power_Limit message Pali Rohár
2022-02-24 21:28 ` Bjorn Helgaas
2022-02-25 12:54 ` Pali Rohár
2022-02-25 16:57 ` Bjorn Helgaas
2022-03-01 9:47 ` Pali Rohár [this message]
2022-02-25 17:02 ` Bjorn Helgaas
2022-03-01 9:50 ` Pali Rohár
2022-02-22 16:31 ` [PATCH 6/6] ARM: dts: turris-omnia: Set PCIe slot-power-limit-milliwatt properties Pali Rohár
2022-02-28 16:13 ` Gregory CLEMENT
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220301094707.64jbqpoxhmana7ua@pali \
--to=pali@kernel.org \
--cc=andrew@lunn.ch \
--cc=bhelgaas@google.com \
--cc=gregory.clement@bootlin.com \
--cc=helgaas@kernel.org \
--cc=kabel@kernel.org \
--cc=kw@linux.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=rmk+kernel@armlinux.org.uk \
--cc=robh+dt@kernel.org \
--cc=thomas.petazzoni@bootlin.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).