netdev - Re: [PATCH 03/13] dt-bindings: mmc: vt8500-sdmmc: Convert to YAML

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CABjd4YwpKYr3q06E7H3upFxyUT6uGg-Jzt2_FiyfS8R=j0ye8w@mail.gmail.com>
Date: Tue, 22 Apr 2025 13:01:44 +0400
From: Alexey Charkov <alchark@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Rob Herring <robh@...nel.org>, Andi Shyti <andi.shyti@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Thomas Gleixner <tglx@...utronix.de>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Ulf Hansson <ulf.hansson@...aro.org>, 
	Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Uwe Kleine-König <ukleinek@...nel.org>, 
	Daniel Lezcano <daniel.lezcano@...aro.org>, linux-arm-kernel@...ts.infradead.org, 
	linux-i2c@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-mmc@...r.kernel.org, 
	netdev@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH 03/13] dt-bindings: mmc: vt8500-sdmmc: Convert to YAML

On Tue, Apr 22, 2025 at 12:08 PM Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> On 18/04/2025 14:38, Alexey Charkov wrote:
> >>
> >>>> +
> >>>> +  reg:
> >>>> +    maxItems: 1
> >>>> +
> >>>> +  clocks:
> >>>> +    maxItems: 1
> >>>> +
> >>>> +  interrupts:
> >>>> +    items:
> >>>> +      - description: SDMMC controller interrupt
> >>>> +      - description: SDMMC controller DMA interrupt
> >>>> +
> >>>> +  sdon-inverted:
> >>>> +    type: boolean
> >>>> +    description: SD_ON bit is inverted on the controller
> >>>
> >>> This implies I know what the non-inverted state is. If you know, please
> >>> state that here.
> >>
> >> This is a tricky one. The only answer I have is "it's inverted in
> >> later versions vs. the first version I saw in the wild, and I'm not
> >> sure if it's board related or IP version related - nor if the original
> >> was active low or high". No docs, no schematics, no vendor left around
> >> to chase for answers.
> >>
> >> Will dig around some more and update the description if I succeed in
> >> uncovering any further clues :)
> >
> > I've found some extra clues and would like to consult on the best way forward.
> >
> > It turns out (if my understanding of the decompiled binary-only WM8505
> > vendor driver is correct) that all chips before (not including) WM8505
> > rev. A2 treated their "clock stop" bit (register offset 0x08 a.k.a.
> > SDMMC_BUSMODE, bit 0x10 a.k.a. BM_CST in vendor sources, BM_SD_OFF in
> > mainline) as "set 1 to disable SD clock", while all the later versions
> > treated it as "set 0 to disable SD clock". Which means that there are
> > WM8505 based systems that rely on either of those behaviours, while
> > any later chips need "set 0 to disable". This is not a board related
> > quirk but an on-chip SDMMC controller revision related quirk.
> >
> > I'd love to switch to a compatible-based logic and drop the
> > "sdon-inverted" flag altogether from the binding I'm writing, but here
> > are my doubts where I'd love to consult.
> >
> > * Looks like WM8505 rev. A2 needs a separate compatible string vs.
> > prior WM8505. Can we have something like "wm,wm8505a2-sdhc" and
> > "wm,wm8505-sdhc" respectively? WM8505a2 not being an actual chip name,
> > but something discoverable by reading its hardware ID from a system
> > configuration register at runtime
>
> Then maybe it can be fully detected runtime? E.g. via soc_id driver (see
> drivers/soc/)?

Thanks for pointing this out! Yes, it should work. A separate
mini-driver to identify the SoC based on the system configuration
register readings and a match table on soc_device_attribute inside the
wmt-sdmmc driver to differentiate between different controller
versions which are all identified as "wm,wm8505-sdhc" in current
device trees.

> > * If I introduce new compatible strings for "wm,wm8650-sdhc",
> > "wm,wm8750-sdhc", "wm,wm8850-sdhc" and "wm,wm8880-sdhc" in bindings,
> > DTS and driver code, then the new driver and new DTB should work fine,
> > and the DTS should pass schema checks. New driver code won't work with
> > older DTB unless I keep the logic to parse "sdon-inverted" which
> > wouldn't be part of the binding. Old driver code would not work with
> > newer DTB except for pre-A2 versions of WM8505. Is that acceptable?
> > * Existing DTS doesn't differentiate between pre-A2 vs. post-A2
> > revisions of WM8505 and is bound to fail on the latter
>
> That's an old platform, so we should not break the ABI, thus drivers
> must fully support old DTBs.

Noted, thanks.

> > I realize that breaking backward/forward compatibility is undesirable,
> > but frankly these systems seem to have few mainline users, and those
> > people who do run mainline on them ought to be compiling the kernel
> > and its DTB at the same time, because the firmware doesn't know
>
> There might be other users of DTS and anyway what would be exactly the
> benefit? This hardware aspect is already documented via sdon-inverted
> property.

The benefit is rather cosmetic (to properly describe different
versions of this controller using SoC-specific compatible strings, and
to avoid defining bindings for random vendor-specific properties such
as sdon-inverted).

Best regards,
Alexey