Re: [PATCH v9 03/23] dt-bindings: ufs: mediatek,ufs: Add mt8196 variant

[Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>]

Hi Rob,

On Tue, 2026-03-10 at 13:10 -0500, Rob Herring wrote:
> On Fri, Mar 6, 2026 at 12:37 PM Nicolas Frattaroli
> <nicolas.frattaroli@collabora.com> wrote:
> > 
> > On Friday, 6 March 2026 17:33:05 Central European Standard Time Rob
> > Herring wrote:
> > > On Fri, Mar 06, 2026 at 02:24:44PM +0100, Nicolas Frattaroli
> > > wrote:
> > > > The MediaTek MT8196 SoC's UFS controller uses three additional
> > > > clocks
> > > > compared to the MT8195, and a different set of supplies. It is
> > > > therefore
> > > > not compatible with the MT8195.
> > > > 
> > > > While it does have a AVDD09_UFS_1 pin in addition to the
> > > > AVDD09_UFS pin,
> > > > it appears that these two pins are commoned together, as the
> > > > board
> > > > schematic I have access to uses the same supply for both, and
> > > > the
> > > > downstream driver does not distinguish between the two supplies
> > > > either.
> > > > 
> > > > Add a compatible for it, and modify the binding
> > > > correspondingly.
> > > > 
> > > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > > > Acked-by: Vinod Koul <vkoul@kernel.org>
> > > > Acked-by: Conor Dooley <conor.dooley@microchip.com>
> > > > Reviewed-by: AngeloGioacchino Del Regno
> > > > <angelogioacchino.delregno@collabora.com>
> > > > Signed-off-by: Nicolas Frattaroli
> > > > <nicolas.frattaroli@collabora.com>
> > > > ---
> > > >  .../devicetree/bindings/ufs/mediatek,ufs.yaml      | 58
> > > > +++++++++++++++++++++-
> > > >  1 file changed, 57 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
> > > > b/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
> > > > index e0aef3e5f56b..a82119ecbfe8 100644
> > > > --- a/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
> > > > +++ b/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
> > > > @@ -16,10 +16,11 @@ properties:
> > > >        - mediatek,mt8183-ufshci
> > > >        - mediatek,mt8192-ufshci
> > > >        - mediatek,mt8195-ufshci
> > > > +      - mediatek,mt8196-ufshci
> > > > 
> > > >    clocks:
> > > >      minItems: 1
> > > > -    maxItems: 13
> > > > +    maxItems: 16
> > > > 
> > > >    clock-names:
> > > >      minItems: 1
> > > > @@ -37,6 +38,9 @@ properties:
> > > >        - const: crypt_perf
> > > >        - const: ufs_rx_symbol0
> > > >        - const: ufs_rx_symbol1
> > > > +      - const: ufs_sel
> > > 
> > > "ufs" is redundant as all the clocks are for UFS. Same comment on
> > > prior
> > > patch.
> > 
> > Is this naming a big enough concern to block this series with two
> > explicit acks on this patch that fixes a wholly broken and useless
> > binding?
> 
> Shrug... Is changing it really that hard?
> 
Since I'm currently working on rebasing this series and fixing its
remaining open issues (compilation, dt-bindings warnings,...) to send a
new revision, I've looked at the questions you raised.

First, renaming those clocks and all the other starting with "ufs_"
prefix (including the one that is simply named ufs) is indeed easy and
needs just a little rework. 
Mostly, a driver patch, as it is currently explicitly using the name of
the 3 ufs_sel clocks and devicetree patches to adapt to this change.

The next series revision will include the devicetree patches for MT8195
SoC and the two boards that integrate this SoC and an UFS storage
(Genio 1200 EVK UFS and Radxa NIO-12L), to fix the warnings that the
dt-binding changes, done by patch 1 (additional clocks, freq-table-hz
deprecation, additional power supplies) in the v9 revision, generate.

> > > > +      - const: ufs_sel_min_src
> > > > +      - const: ufs_sel_max_src
> > > 
> > > "src" sounds like a parent clock? If so, probably shouldn't be in
> > > the
> > > clocks list. 'assigned-clocks' is for dealing with parent clocks.
> > > 
> > 
> > I don't know what it is, and I have no way to consult any
> > documentation
> > that would tell me what it is. I am trying to put out this dumpster
> > fire
> > of a downstream turd that made its way into mainline as the review
> > process
> > has been completely subverted, and is only getting worse with each
> > passing
> > month that MediaTek is allowed to block this series from
> > progressing while
> > sneaking further changes through.
> 
> It's good Mediatek is active, then they can tell us what the clocks
> are for. I would think the driver would give some clue.
> 
Second, when searching in the driver code and the git history, it shows
that ufs_sel is indeed a parent clock.
The ufs_sel/ufs_sel_min_src/ufs_sel_max_src clock use in the driver
code was introduced by the commit b7dbc686f60b ("scsi: ufs: ufs-
mediatek: Support clk-scaling to optimize power consumption") to
implement a dynamic clock scaling feature.

ufs_sel is supposed to be the parent clock of the main clock ("ufs" in
dt-bindings) and both ufs_sel_min_src/ufs_sel_max_src the parent of
ufs_sel.
The code switches conditionally the ufs_sel parent to modify the ufs
clock rate (ufs_sel_max_src for maximum performance, ufs_sel_min_src
otherwise).
I've also looked at different downstream kernel trees, the assigned
clocks values for those 3 clocks in the ufshci node in devicetree are
consistent with the clock hierarchy in the MT8196 clock controllers
drivers.

This feature also seems to be linked to another one that added the
clock scaling for the FDE clock (ufs_aes in dt-bindings). 
The commit that introduced it is 5e5976f5242d ("scsi: ufs: host:
mediatek: Support FDE (AES) clock scaling") and it also added the 3
undocumented clocks: ufs_fde, ufs_fde_min_src, ufs_fde_max_src.
It is similar to the previous described one.

The current driver code seems to require having the "ufs_fde" clock
(supposed to be the ufs_aes parent clock) in the devicetree, so that
the clock scaling feature for the "ufs_sel" clock is performed and I
did not find why. 
So, with this current series dt-bindings patches, ufs clock scaling
feature seems not completely described yet.

There is also the crypto boost feature that make use of the
crypt_mux/crypt_lp/crypt_perf clocks in a similar way.
They were missing from dt-bindings, before this series patches made by
Nicolas to documented them. Angelo also sent a patch two years ago in
that regard ([1]) but did not get picked.
The feature has been introduced by commit 590b0d2372fe ("scsi: ufs-
mediatek: Support performance mode for inline encryption engine")
The crypt_mux clock is also supposed to be the ufs_aes parent clock and
its own parent is switched on need between crypt_lp (low power) and
crypt_perf (performance).
This feature is also depending two undocumented property:
- dvfsrc-vcore-supply: Angelo sent [2] to add it and this current
series forgot to add it too (to be done for v10 ?)
- mediatek,ufs-boost-crypt: vendor specific property to enable this
feature. Angelo sent [3] to remove its need but got reject back then

Note that crypto boost and FDE clock scaling features seem not to be
supposed to be enabled at the same time.

Again, this crypto boost feature seems not completely described yet.

Sorry for the wall of text but I felt it was better to add extra
details regarding all those features and how they relate to each other
to have a more complete answer regarding ufs_sel.

[1]
https://lore.kernel.org/linux-mediatek/20240612074309.50278-8-angelogioacchino.delregno@collabora.com/
[2]
https://lore.kernel.org/linux-mediatek/20240612074309.50278-9-angelogioacchino.delregno@collabora.com/
[3]
https://lore.kernel.org/linux-mediatek/20240612074309.50278-4-angelogioacchino.delregno@collabora.com/

> I don't see how accepting sub-par bindings or not fixes the issues
> here.

So, since all these features rely heavily on optional parent clocks,
that were not documented before being used in driver code, what should
be done to make progress and fix these dt-bindings?

What would you recommend to do in order to resolve those topics?
I'm open for ideas, please.

Best regards,
Louis-Alexis

> 
> Rob

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy