Message-ID: <f906a056-3c6b-4115-9fd0-e50da8fc049b@broadcom.com>
Date: Thu, 19 Dec 2024 15:43:17 -0800
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: linux-arm-kernel@...ts.infradead.org, Rob Herring <robh@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Broadcom internal kernel review list
 <bcm-kernel-feedback-list@...adcom.com>,
 "open list:MEMORY CONTROLLER DRIVERS" <linux-kernel@...r.kernel.org>,
 "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
 <devicetree@...r.kernel.org>
Subject: Re: [PATCH 1/2] dt-bindings: memory-controller: Document rev c.1.5
 compatible

On 12/19/24 00:58, Krzysztof Kozlowski wrote:
> On Wed, Dec 18, 2024 at 09:15:08AM -0800, Florian Fainelli wrote:
>> On 12/18/24 03:37, Krzysztof Kozlowski wrote:
>>> On Tue, Dec 17, 2024 at 11:44:38AM -0800, Florian Fainelli wrote:
>>>> Document the revision c.1.5 compatible string that is present on newer
>>>> Broadcom STB memory controllers (74165 and onwards).
>>>>
>>>> Signed-off-by: Florian Fainelli <florian.fainelli@...adcom.com>
>>>> ---
>>>>    .../bindings/memory-controllers/brcm,brcmstb-memc-ddr.yaml       | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/brcm,brcmstb-memc-ddr.yaml b/Documentation/devicetree/bindings/memory-controllers/brcm,brcmstb-memc-ddr.yaml
>>>> index 4b072c879b02..99d79ccd1036 100644
>>>> --- a/Documentation/devicetree/bindings/memory-controllers/brcm,brcmstb-memc-ddr.yaml
>>>> +++ b/Documentation/devicetree/bindings/memory-controllers/brcm,brcmstb-memc-ddr.yaml
>>>> @@ -29,6 +29,7 @@ properties:
>>>>              - brcm,brcmstb-memc-ddr-rev-c.1.2
>>>>              - brcm,brcmstb-memc-ddr-rev-c.1.3
>>>>              - brcm,brcmstb-memc-ddr-rev-c.1.4
>>>> +          - brcm,brcmstb-memc-ddr-rev-c.1.5
>>>
>>> You should use v2.1 fallback and drop driver patch. Or explain in
>>> commit briefly why different approach is suitable.
>>
>> Are you suggesting that we should have fallback compatible strings, such
>> that we have something like this:
>>
>> compatible = "brcm,brcmstb-memc-ddr-rev-c.1.5",
>> "brcm,brcmstb-memc-ddr-rev-c", "brcm,brcmstb-memc-ddr"
>>
>> and the driver only needs to match on "brcm,brcmstb-memc-ddr-rev-c" and
>> apply the adequate register offset table?
> 
> Almost, fallback should be brcm,brcmstb-memc-ddr-rev-b.2.1 or whatever
> was in the driver first or whatever is the oldest known common
> interface.
> 
> brcm,brcmstb-memc-ddr-rev-c is not a specific compatible.
> 
>> If so, that is not how the current binding, and therefore DTBs are being
>> deployed, so that will introduce a breakage until we update all DTBs in the
>> field...
> 
> No. First, I thought about new comaptible so the one you add here. No
> breakage, it's new compatible. This saves you these pointless updates of
> driver everytime you add new compatible.

Yes, but that is not how the binding has been defined until now, so all 
of the DTBs out there have:

compatible = "brcm,brcmstb-memc-ddr-rev-b.2.x", "brcm,brcmstb-memc-ddr"

(where X is in range [1..5])

and there is no fallback defined to "brcm,brcmstb-memc-ddr-rev-b.2.1", 
so it is not like we can retrofit that easily by adding one now.

 > > Second, you can introduce fallbacks to older compatibles as well - 
there
> will be no breakage, because you add one more compatible. The old
> compatibles (covered by fallback) of course stays in the driver, so
> there is no breakage at all. We did it multiple times for several
> different bindings in Qualcomm. People were doing exactly the same:
> adding compatible for new device to binding and driver, without
> considering the compatibility at all.
> 
> Except being logically correct choice - using fallbacks - this really
> has huge benefits when later upstreaming complete, big SoCs, like we do
> for latest Qualcomm SoCs: several changes will be only bindings updates.

Yes, there are advantages to using fallbacks and we (ab)use that 
whenever practical.

The driver only uses a very limited subset of registers (for now), the 
registers change between minor revisions as well in a way that using a 
fallback like "brcm,brcmstb-memc-ddr-rev-b.2.1" is not accurate enough 
not practical. In particular for some of the changes that I am thinking 
of adding later on, we would need the precise minor version because the 
behavior and/or register interface is subtly different that this matters.

Thanks
-- 
Florian

Powered by blists - more mailing lists