Re: [PATCH v1 1/3] dt-bindings: PCI: brcmstb: Add two optional props

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Jim Quinlan <jim2101024@gmail.com>,
	Stefan Wahren <stefan.wahren@i2se.com>
Cc: linux-pci@vger.kernel.org,
	"Nicolas Saenz Julienne" <nsaenz@kernel.org>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Cyril Brulebois" <kibi@debian.org>,
	"Phil Elwell" <phil@raspberrypi.com>,
	bcm-kernel-feedback-list@broadcom.com,
	james.quinlan@broadcom.com,
	"Florian Fainelli" <f.fainelli@gmail.com>,
	"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Rob Herring" <robh@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE"
	<linux-rpi-kernel@lists.infradead.org>,
	"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE"
	<linux-arm-kernel@lists.infradead.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
	<devicetree@vger.kernel.org>,
	"open list" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1 1/3] dt-bindings: PCI: brcmstb: Add two optional props
Date: Thu, 6 Apr 2023 20:35:35 +0200	[thread overview]
Message-ID: <ab1b0161-20c2-83eb-e371-e8363547e758@linaro.org> (raw)
In-Reply-To: <CANCKTBsLxkPb1ajACkyhJk6J1aB2iwX0oKifHkADG0fFPUqMhQ@mail.gmail.com>

On 06/04/2023 18:58, Jim Quinlan wrote:
> On Thu, Apr 6, 2023 at 11:39 AM Stefan Wahren <stefan.wahren@i2se.com> wrote:
>>
>> Hi Jim,
>>
>> Am 06.04.23 um 14:46 schrieb Jim Quinlan:
>>> Regarding "brcm,enable-l1ss":
>>>
>>>    The Broadcom STB/CM PCIe HW -- which is also used by RPi SOCs -- requires
>>>    the driver probe to configure one of three clkreq# modes:
>>>
>>>    (a) clkreq# driven by the RC
>>>    (b) clkreq# driven by the EP for ASPM L0s, L1
>>>    (c) bidirectional clkreq#, as used for L1 Substates (L1SS).
>>>
>>>    The HW can tell the difference between (a) and (b), but does not know
>>>    when to configure (c).  Further, the HW will cause a CPU abort on boot if
>>>    guesses wrong regarding the need for (c).  So we introduce the boolean
>>>    "brcm,enable-l1ss" property to indicate that (c) is desired.  This
>>>    property is already present in the Raspian version of Linux, but the
>>>    driver implementaion that will follow adds more details and discerns
>>>    between (a) and (b).
>>>
>>> Regarding "brcm,completion-timeout-msecs"
>>>
>>>    Our HW will cause a CPU abort if the L1SS exit time is longer than the
>>>    completion abort timeout.  We've been asked to make this configurable, so
>>>    we are introducing "brcm,completion-abort-msecs".
>>>
>>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
>>> ---
>>>   .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 ++++++++++++
>>>   1 file changed, 12 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> index 7e15aae7d69e..ef4ccc05b258 100644
>>> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> @@ -64,6 +64,18 @@ properties:
>>>
>>>     aspm-no-l0s: true
>>>
>>> +  brcm,enable-l1ss:
>>> +    description: Indicates that the downstream device is L1SS
>>> +      capable and L1SS is desired, e.g. by setting
>>> +      CONFIG_PCIEASPM_POWER_SUPERSAVE=y.  Note that CLKREQ#
>>
>> not sure about this, but maybe we should avoid references to Linux
>> kernel config parameter in a DT binding. Since the driver already gaves
>> warning in case the DT parameter is present, but kernel config doesn't
>> fit, this should be enough.
> 
> Hello Stefan,
> I will remove this reference.
>>
>>> +      assertion to clock active must be within 400ns.
>>> +    type: boolean
>>> +
>>> +  brcm,completion-timeout-msecs:
>>> +    description: Number of msecs before completion timeout
>>> +      abort occurs.
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>
>> According to the driver at least 0 is not allowed, maybe we should
>> define minimum and maximum here and let dtbs_check take care of invalid
>> values?
> I'm not sure I follow what you mean about a zero value;  the property
> may have any value but the driver will clamp it
> to a minimum of ~30msec.  Regardless, I can add a  "minimum: 30" line
> to the YAML.

If "completion" means Linux completion, then it is not suitable for DT
and entire property should be removed. If it is something else, then
explain here and commit msg. So far both refer to some completion...

Best regards,
Krzysztof