Message-ID: <aFlT7fLePVmvoxBQ@linaro.org>
Date: Mon, 23 Jun 2025 15:17:33 +0200
From: Stephan Gerhold <stephan.gerhold@...aro.org>
To: Bjorn Andersson <andersson@...nel.org>, Stephen Boyd <sboyd@...nel.org>
Cc: Krzysztof Kozlowski <krzk@...nel.org>,
	Saravana Kannan <saravanak@...gle.com>,
	Rob Herring <robh@...nel.org>,
	Jassi Brar <jassisinghbrar@...il.com>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Michael Turquette <mturquette@...libre.com>,
	linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
	Georgi Djakov <djakov@...nel.org>
Subject: Re: [PATCH 1/4] dt-bindings: mailbox: qcom,apcs: Add separate node
 for clock-controller

wOn Sat, Jun 21, 2025 at 01:51:16PM -0700, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2025-06-10 20:31:57)
> > I'm still sceptical here.
> > 
> > In the first snippet above, we describe a single IP block which provides
> > mailboxes and clocks.
> > 
> > In the second snippet we're saying that the IP block is a mailbox, and
> > then it somehow have a subcomponent which is a clock provider.
> > 
> > It seems to me that we're choosing the second option because it better
> > fits the Linux implementation, rather than that it would be a better
> > representation of the hardware. To the point that we can't even describe
> > the register range of the subcomponent...
> > 
> 
> Agreed. Don't workaround problems in the kernel by changing the binding
> to have sub-nodes.

I can describe the register range for the subcomponent if you prefer
(it's reg = <0x50 0xc>; within the parent component). That would be easy
to add.

Your more fundamental concern (working around problems in the kernel by
changing the binding) is a more tricky and subtle one. I had exactly the
same thought when I started making this patch series. However, if you
start looking more closely you will see that this is much easier said
than done. I tried to explain the problem already a few times (in the
cover letter, the commit messages and responses to this series), but let
me try again. Perhaps in different words it will become more
understandable.

Just for clarity, let's take the current device tree description again:

	apcs1_mbox: mailbox@...1000 {
		compatible = "qcom,msm8939-apcs-kpss-global", "syscon";
		reg = <0x0b011000 0x1000>;
		#mbox-cells = <1>;
		clocks = <&a53pll_c1>, <&gcc GPLL0_VOTE>, <&rpmcc RPM_SMD_XO_CLK_SRC>;
		clock-names = "pll", "aux", "ref";
		#clock-cells = <0>;
	};

Clearly this is a mailbox (#mbox-cells) and a clock controller
(#clock-cells). In the hardware these are stuffed into one register
region, but they don't have anything to do with each other. In
particular, the specified clocks are only used by the clock controller.
They are not used or related in any way to the mailbox component.

We need to have the mailbox available early to proceed with booting. The
clock controller can probe anytime later. The &rpmcc clock required by
the clock controller depends on having the mailbox available.

In Linux, I cannot get the mailbox driver to probe as long as the &rpmcc
clock is specified inside this device tree node (or by using
post-init-providers, but see [1]). This is not something I can fix in
the driver. The "problem in the kernel" you are referring to is
essentially "fw_devlink". Independent of the device-specific bindings we
define, it is built with the assumption that resources specified in a
device tree node are required to get a device functioning.

We usually want this behavior, but it doesn't work in this case. I argue
this is because we describe *two* devices as part of a *single* device
tree node. By splitting the *two* devices into *two* device tree nodes,
it is clear which resources belong to which device, and fw_devlink can
function correctly.

You argue this is a problem to be solved in the kernel. In practice,
this would mean one of the following:

 - Remove fw_devlink from Linux.
 - Start adding device-specific quirks into the generic fw_devlink code.
   Hardcode device links that cannot be deferred from the device tree
   because our hardware description is too broad.

Both of these are not really desirable, right?

I don't think there is a good way around making the hardware description
more precise by giving the two devices separate device tree nodes. There
are many different options for modelling these, and I would be fine with
all of them if you think one of them fits better:

Top-level siblings:

	apcs1_mbox: mailbox@...1008 {
		compatible = "qcom,msm8939-apcs-mbox";
		reg = <0x0b011008 0x4>;
		#mbox-cells = <1>;
	};

	apcs1_clk: clock-controller@...1050 {
		compatible = "qcom,msm8939-apcs-clk";
		reg = <0x0b011050 0xc>;
		clocks = <&a53pll_c1>, <&gcc GPLL0_VOTE>, <&rpmcc RPM_SMD_XO_CLK_SRC>;
		clock-names = "pll", "aux", "ref";
		#clock-cells = <0>;		
	};

Top-level syscon wrapper with two children:

	syscon@...1000 {
		compatible = "qcom,msm8939-apcs-kpss-global", "syscon";
		reg = <0x0b011000 0x1000>;
		#adress-cells = <1>;
		#size-cells = <1>;
		ranges = <0 0x0b011000 0x1000>;

		apcs1_mbox: mailbox@8 {
			compatible = "qcom,msm8939-apcs-mbox";
			reg = <0x8 0x4>;
			#mbox-cells = <1>;
		};

		apcs1_clk: clock-controller@50 {
			compatible = "qcom,msm8939-apcs-clk";
			reg = <0x0b011050 0xc>;
			clocks = <&a53pll_c1>, <&gcc GPLL0_VOTE>, <&rpmcc RPM_SMD_XO_CLK_SRC>;
			clock-names = "pll", "aux", "ref";
			#clock-cells = <0>;
		};
	};

Mailbox as parent (what I did in this series):

	apcs1_mbox: mailbox@...1000 {
		compatible = "qcom,msm8939-apcs-kpss-global", "syscon";
		reg = <0x0b011000 0x1000>;
		#mbox-cells = <1>;

		apcs1_clk: clock-controller {
			clocks = <&a53pll_c1>, <&gcc GPLL0_VOTE>, <&rpmcc RPM_SMD_XO_CLK_SRC>;
			clock-names = "pll", "aux", "ref";
			#clock-cells = <0>;
		};
	};

Maybe it makes more sense with this explanation and the other options.
Let me know what you think!

Thanks,
Stephan

[1]: https://lore.kernel.org/linux-arm-msm/aC-AqDa8cjq2AYeM@linaro.org/

Powered by blists - more mailing lists