| 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
| ||
|
Message-ID: <6837084c.050a0220.1e474f.3f20@mx.google.com>
Date: Wed, 28 May 2025 14:57:46 +0200
From: Christian Marangi <ansuelsmth@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Philipp Zabel <p.zabel@...gutronix.de>,
Felix Fietkau <nbd@....name>, linux-clk@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/5] dt-bindings: clock: airoha: Document support for
AN7583 clock
On Wed, May 28, 2025 at 01:56:54PM +0200, Krzysztof Kozlowski wrote:
> On 28/05/2025 10:54, Christian Marangi wrote:
> > On Wed, May 28, 2025 at 09:30:37AM +0200, Krzysztof Kozlowski wrote:
> >> On 28/05/2025 02:49, Christian Marangi wrote:
> >>> - if:
> >>> properties:
> >>> compatible:
> >>> @@ -75,6 +78,17 @@ allOf:
> >>> reg:
> >>> maxItems: 1
> >>>
> >>> + required:
> >>> + - reg
> >>> +
> >>> + - if:
> >>> + properties:
> >>> + compatible:
> >>> + const: airoha,an7583-clock
> >>> + then:
> >>> + properties:
> >>> + reg: false
> >>
> >>
> >> No resources here, so this should be part of parent node.
> >>
> >
> > Ok hope you can help here. This is another case of "MFD" thing.
> >
> > I was with the idea that it was O.K. to use this with very different
> > devices. (current scenario Clock controller and MDIO controller)
> >
> > The node structure I had in mind was
> >
> > system-controller@...20000 {
> > compatible = "airoha,an7583-scu", "syscon", "simple-mfd";
> > reg = <0x0 0x1fb00000 0x0 0x970>;
> >
> > scuclk: scuclk {
> > compatible = "airoha,an7583-clock";
> > #clock-cells = <1>;
> > #reset-cells = <1>;
> > };
> >
> > mdio {
> > compatible = "airoha,an7583-mdio";
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > mdio_0: bus@0 {
> > reg = <0>;
> > resets = <&scuclk AN7583_MDIO0>;
> > };
> >
> > mdio_1: bus@1 {
> > reg = <1>;
> > resets = <&scuclk AN7583_MDIO1>;
> > };
> > };
> > };
> >
> > But you want
> >
> > system-controller@...20000 {
> > compatible = "airoha,an7583-scu", "syscon";
> > reg = <0x0 0x1fb00000 0x0 0x970>;
> >
> > #clock-cells = <1>;
> > #reset-cells = <1>;
> >
>
> mdio could be here just to group the bus (it's pretty common I think),
> although not sure if compatible is useful then.
>
> > mdio_0: bus@0 {
> > reg = <0>;
> > resets = <&scuclk AN7583_MDIO0>;
> > };
> >
> > mdio_1: bus@1 {
> > reg = <1>;
> > resets = <&scuclk AN7583_MDIO1>;
> > };
> > };
> >
> > Again sorry if this question keeps coming around and I can totally
> > understand if you are getting annoyed by this. The reason I always ask
> > this is because it's a total PAIN to implement this with the driver
> > structure due to the old "simple-mfd" model.
>
> ... and Rob was saying multiple times: be careful when adding
> simple-mfd. If it bites back, then I am sorry, but everyone were warned,
> weren't they?
>
> What is exactly the pain anyway? You cannot instantiate children from
> SCU driver?
>
Answering below since they are related.
> >
> > (as again putting everything in a single node conflicts with the OF
> > principle of autoprobing stuff with compatible property)
>
> I am not sure if I follow. What principle? Where is this principle
> expressed?
>
> And you do not have in your second example additional compatibles, so
> even if such principle exists it is not broken: everything autoprobes, I
> think.
>
> >
>
>
The principle I'm talking about is one driver for one compatible.
(to be more precise excluding syscon compatible that is actually
ignored, if a driver for the compatible is found, any other compatible
is ignored.)
This means that declaring multiple compatible as:
compatible = "airoha,clock", "airoha,mdio"
doesn't result in the clock driver and the mdio driver probed but only
one of the 2 (probably only clock since it does have priority)
The "simple-mfd" compatible is just a simple compatible that indicate to
the OF system that every child (with a compatible) should be also probed.
And then automagically the driver gets probed.
Now the ""PAIN"" explaination. Not using the "simple-mfd" way with the
child with compatible and putting everything in the node means having to
create a dedicated MFD driver that just instruct to manually probe the
clock and mdio driver. (cause the compatible system can't be used)
So it's 3 driver instead of 2 with the extra effort of MFD driver
maintainer saying "Why simple-mfd is not used?"
There is a solution for this but I always feel it's more of a workaround
since it doesn't really describe the HW with the DT node.
The workaround is:
system-controller@...20000 {
/* The parent SCU node implement the clock driver */
compatible = "airoha,an7583-scu", "syscon";
reg = <0x0 0x1fb00000 0x0 0x970>;
#clock-cells = <1>;
#reset-cells = <1>;
/* Clock driver is instructed to probe child */
mdio {
compatible = "airoha,an7583-mdio";
#address-cells = <1>;
#size-cells = <0>;
mdio_0: bus@0 {
reg = <0>;
resets = <&scuclk AN7583_MDIO0>;
};
mdio_1: bus@1 {
reg = <1>;
resets = <&scuclk AN7583_MDIO1>;
};
};
};
But this really moves the probe from the simple-mfd to the clock driver.
So it's either 3 solution
1. 2 driver + "simple-mfd"
2. 3 driver + PAIN (due to MFD required driver)
3. 2 driver + not very correct DT node structure
Maybe option 3. is more acceptable?
The SCU node is mainly clock + reset controller and the MDIO bus is an
expansion of it?
Hope the long explaination makes sense to you (especially about the
OF principle thing)
--
Ansuel
Powered by blists - more mailing lists