| 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: <6839ce13.050a0220.1038eb.83e1@mx.google.com>
Date: Fri, 30 May 2025 17:26:07 +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 Thu, May 29, 2025 at 11:00:39AM +0200, Krzysztof Kozlowski wrote:
> On 28/05/2025 14:57, Christian Marangi wrote:
> >>> 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.
>
> There is no such principle. One compatible can map to many drivers and
> many compatibles can map to one driver.
>
I might be wrong but there is currently a limitation on the OF system
where if a driver gets probed for a node then it's ignored for any other
driver. (sorry for the bad explaination, hope it's understandable)
> > (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)
>
> I don't understand this example. It makes no sense - clock is not
> compatible with mdio.
>
This was an example to put every properties in the oparent node.
> >
> > 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)
>
> You already have that driver - SCU. No need for new MFD driver...
>
The SCU driver is actually the clock driver (currently). This was done
for simplicity and because in SCU there were only some bits.
But now with AN7583 they put 2 MDIO controller in it.
>
> >
> > So it's 3 driver instead of 2 with the extra effort of MFD driver
> > maintainer saying "Why simple-mfd is not used?"
>
> Sorry, that's a wrong argument. You can use simple-mfd, iff it follows
> standard practices. If it does not fit standard practices, you cannot
> use an argument "now I need more complicated solution".
>
Then I think we are getting confused because I am following the usual
pattern.
This is what would be the ideal and easy solution. (ti what was done on
EN7581 with pinctrl and pwm)
scu: system-controller@...20000 {
compatible = "syscon", "simple-mfd";
reg = <0x0 0x1fb00000 0x0 0x970>;
scuclk: scuclk {
compatible = "airoha,an7583-scu";
#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>;
};
};
};
> >
> >
> > 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.
>
> Really? All arguments you used here are driver arguments - that
> something is a pain in drivers. Now you mention that hardware would not
> match description.
>
> Then let's change entire talk towards hardware description and send
> patches matching hardware, not matching your MFD driver structure.
>
Ok to describe the HW for this register block
SCU register:
- clock
- mdio controller 1
- mdio controller 2
So this is why I think a good matching node block is:
parent node (SCU register):
- child 1 (clock)
- child 2 (mdio controller)
- child 1 (mdio bus 1)
- child 2 (mdio bus 2)
Is it wrong to model the DT node this way?
> >
> > 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";
>
> Again, drop compatible.
>
To drop the compatible a dedicated MFD driver is needed (or adding code
in the clock driver to register the MDIO controller and that is net
clean code wise)
> > #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
>
> Option 4:
> Describe it correctly. You have one device which is the SCU which is
> clock provider and has subnode for MDIO bus. I don't care how many
> drivers you have there (but I am sure one can do it in a simple way).
>
Ok extra driver is not a problem. Problem here is understand what is the
correct node structure cause there are various options.
If the register block is still not clear to you just tell me and will
try to describe it even more.
> >
> > 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)
> >
>
>
> Best regards,
> Krzysztof
--
Ansuel
Powered by blists - more mailing lists