From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Jerome Brunet <jbrunet@baylibre.com>
Cc: Jacky Huang <ychuang3@nuvoton.com>,
Shan-Chun Hung <schung@nuvoton.com>,
Michael Turquette <mturquette@baylibre.com>,
Stephen Boyd <sboyd@kernel.org>,
Richard Cochran <richardcochran@gmail.com>,
Arnd Bergmann <arnd@arndb.de>,
Brian Masney <bmasney+clk@redhat.com>,
Jerome Brunet <jbrunet+clk@baylibre.com>,
Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzk+dt@kernel.org>,
Conor Dooley <conor+dt@kernel.org>,
Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
Steam Lin <STLin2@winbond.com>,
linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org,
linux-kernel@vger.kernel.org,
Krzysztof Kozlowski <krzk@kernel.org>,
devicetree@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH v4 5/6] clk: nuvoton: ma35d1: Use clk_hw pointers as mux parents
Date: Mon, 28 Sep 2026 18:02:12 +0200 [thread overview]
Message-ID: <87h5j974aj.fsf@bootlin.com> (raw)
In-Reply-To: <1jeceeaaug.fsf@starbuckisacylon.baylibre.com> (Jerome Brunet's message of "Sun, 27 Sep 2026 19:00:07 +0200")
On 27/09/2026 at 19:00:07 +02, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On ven. 25 sept. 2026 at 17:10, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
>> The MA35D1 clock provider registers its muxes with parent data
>> structures filling .fw_name. This is not the ideal approach since that
>> would require a massive amount of internal clock names declaration in
>> the DT. Since the DT does not play the game of exposing all these names,
>> none of the parent lookups performed when instantiating the muxes
>> succeed. As a result, these muxes get registered as root clocks, leading
>> to a sadly flat clock tree and no frequency assigned to most of the
>> peripheral clocks:
>>
>> enable prepare protect
>> clock count count count rate
>>
>> usbphy1 0 0 0 480000000
>> usbphy0 0 0 0 480000000
>> husbh1_gate 0 0 0 480000000
>> husbh0_gate 0 0 0 480000000
>> usbh_gate 0 0 0 480000000
>> usbd_gate 0 0 0 480000000
>> syspll 0 0 0 180000000
>> lirc 0 0 0 32000
>> lirc_gate 0 0 0 32000
>> hirc 0 0 0 12000000
>> gtmr_gate 0 0 0 12000000
>> hirc_gate 0 0 0 12000000
>> lxt 0 0 0 32768
>> rtc_gate 0 0 0 32768
>> lxt_gate 0 0 0 32768
>> hxt 0 0 0 24000000
>> vpll 0 0 0 1224000000
>> dcup_div 0 0 0 612000000
>> epll 0 0 0 6000000000
>> epll_div8 0 0 0 750000000
>>
>> epll_div4 0 0 0 1500000000
>> epll_div2 0 0 0 3000000000
>> emac1_gate 0 0 0 3000000000
>>
>> emac0_gate 0 0 0 3000000000
>>
>> apll 0 0 0 6048000000
>> ddrpll 0 0 0 266460000
>> ddr_gate 0 0 0 266460000
>> ddr6_gate 0 0 0 266460000
>> ddr0_gate 0 0 0 266460000
>> capll 0 0 0 2400000000
>> hxt_gate 0 0 0 24000000
>> clk_hxt 0 0 0 24000000
>> spi3_mux 0 0 0 0
>> spi3_gate 0 0 0 0
>> spi2_mux 0 0 0 0
>> spi2_gate 0 0 0 0
>> spi1_mux 0 0 0 0
>> spi1_gate 0 0 0 0
>> spi0_mux 0 0 0 0
>> spi0_gate 0 0 0 0
>> i2s1_mux 0 0 0 0
>> i2s1_gate 0 0 0 0
>> i2s0_mux 0 0 0 0
>> i2s0_gate 0 0 0 0
>> ...
>>
>> Apart from the wrong clock tree representation, it means that none of
>> the device drivers (spi & i2c in the excerpt above) can actually query
>> their clock rate, or they would get 0Hz.
>>
>> Instead of declaring the parents in the clk_parent_data structure, use
>> the actual HW clocks to lookup the parents directly: parents are
>> described by an array of indices into the controller's main clock table
>> (like in other clock controller drivers), which the "new" mux helper now
>> resolves.
>>
>> The WDT and WWDT muxes list the /4096 children of PCLK3 and PCLK4 among
>> their possible parents. Those two clocks are now registered by the
>> previous commit, so their entries in the parent tables are restored
>> instead of being turned into invalid slots.
>>
>> enable prepare protect
>> clock count count count rate
>>
>> usbphy1 0 0 0 480000000
>> usbphy0 0 0 0 480000000
>> husbh1_gate 0 0 0 480000000
>> husbh0_gate 0 0 0 480000000
>> usbh_gate 0 0 0 480000000
>> usbd_gate 0 0 0 480000000
>> syspll 1 1 0 180000000
>> dbg_mux 0 0 0 180000000
>> sdh1_mux 0 0 0 180000000
>> sdh1_gate 0 0 0 180000000
>> sdh0_mux 0 0 0 180000000
>> sdh0_gate 0 0 0 180000000
>> sysclk1_mux 2 2 0 180000000
>> pclk4 0 0 0 90000000
>> pclk3 0 0 0 90000000
>> sspcc_gate 0 0 0 90000000
>> ssmcc_gate 0 0 0 90000000
>> hclk3 0 0 0 90000000
>> pclk2 0 0 0 180000000
>> eadc_div 0 0 0 90000000
>> eadc_gate 0 0 0 90000000
>> qei1_gate 0 0 0 180000000
>> ecap1_gate 0 0 0 180000000
>> spi3_mux 0 0 0 180000000
>> spi3_gate 0 0 0 180000000
>> spi1_mux 0 0 0 180000000
>> spi1_gate 0 0 0 180000000
>> epwm1_gate 0 0 0 180000000
>> i2c5_gate 0 0 0 180000000
>> i2c2_gate 0 0 0 180000000
>> pclk1 0 0 0 180000000
>> qei2_gate 0 0 0 180000000
>> qei0_gate 0 0 0 180000000
>> ecap2_gate 0 0 0 180000000
>> ecap0_gate 0 0 0 180000000
>> spi2_mux 0 0 0 180000000
>> spi2_gate 0 0 0 180000000
>> spi0_mux 0 0 0 180000000
>> spi0_gate 0 0 0 180000000
>> epwm2_gate 0 0 0 180000000
>> epwm0_gate 0 0 0 180000000
>> i2c4_gate 0 0 0 180000000
>> i2c1_gate 0 0 0 180000000
>> pclk0 1 1 0 180000000
>> adc_div 0 0 0 90000000
>> adc_gate 0 0 0 90000000
>> qspi1_mux 0 0 0 180000000
>> qspi1_gate 0 0 0 180000000
>> qspi0_mux 1 1 0 180000000
>> qspi0_gate 1 1 0 180000000
>>
>> i2c3_gate 0 0 0 180000000
>> i2c0_gate 0 0 0 180000000
>>
>> Fixes: f50a000b4219 ("clk: nuvoton: Use clk_parent_data instead of string for parent clock")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>> ---
>> drivers/clk/nuvoton/clk-ma35d1.c | 626 +++++++++++----------------------------
>> 1 file changed, 177 insertions(+), 449 deletions(-)
>>
>> diff --git a/drivers/clk/nuvoton/clk-ma35d1.c b/drivers/clk/nuvoton/clk-ma35d1.c
>> index c914079cee2d..1a857f28310f 100644
>> --- a/drivers/clk/nuvoton/clk-ma35d1.c
>> +++ b/drivers/clk/nuvoton/clk-ma35d1.c
>> @@ -63,300 +63,49 @@ static DEFINE_SPINLOCK(ma35d1_lock);
>> #define PLL_MODE_FRAC 1
>> #define PLL_MODE_SS 2
>>
>> -static const struct clk_parent_data ca35clk_sel_clks[] = {
>> - { .fw_name = "hxt", },
>> - { .fw_name = "capll", },
>> - { .fw_name = "ddrpll", },
>> -};
>> +#define MA35D1_MUX_MAX_PARENTS 10
>
> I'm bit puzzled how this was supposed to work before. Most of those
> inputs are not present in binding doc. The controller was supposed get
> clocks from itself through DT ???
No idea why it has been written like that. Just to keep things clear, my
re-write is a fix, not a cleanup. Without it, the SPI controller does
not probe and I care about the SPI controller being fixed in this cycle.
> There is one input documented though. It seems to be hxt, so you should
> probably continue to use fw_name for this one at least.
"documented" is maybe a bit strong. There is one reference to a phandle
named clk_hxt, that's it? I don't think it qualifies as a validated
binding :)
> That being said, the bindings doc seems wrong. Looking at the driver you
> should have 4 inputs (hxt, lxt, hirc, lirc), unless those are actually
> generated on SoC ? Since there is already a DT using these, I suppose it
> is too late for the last 3 and you'll be stuck pretending they are
> generated in this controller :/
The TRM identifies:
- HXT and LXT as external crystal oscillators
- HIRC and LIRC as internal RC oscillators
So we want an accurate description, HXT/LXT are worth declaring, but not
HIRC/LIRC.
There is no way to fix this without a breaking change.
My approach for these clocks:
- Describe lxt like hxt in the DT. In the driver, I will take it from
DT, or fallback to a known base rate otherwise (so no breaking
change). This is possible since the TRM itself forces the rate of the
two oscillators. I will also ask for clock names in the binding.
- Fix the name of the output clocks to match the driver (use "hxt"
instead of "clk_hxt" in the DT). I could fix the driver instead, but
all other clocks would be named differently, which would be
strange. So since we anyway *need* a DT update, let's go for a clean
naming.
The other patches can still go like they are.
Thanks,
Miquèl
next prev parent reply other threads:[~2026-09-28 16:02 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-25 15:10 [PATCH v4 0/6] clk: nuvoton: ma35d1: Fix mux parenting and peripheral clock rates Miquel Raynal
2026-09-25 15:10 ` [PATCH v4 1/6] clk: nuvoton: ma35d1: Keep the clock count in the driver Miquel Raynal
2026-09-25 15:10 ` [PATCH v4 2/6] dt-bindings: clock: ma35d1: Drop CLK_MAX_IDX define Miquel Raynal
2026-09-25 16:41 ` Conor Dooley
2026-09-25 15:10 ` [PATCH v4 3/6] dt-bindings: clock: ma35d1: Add missing WDT/WWDT parent clocks Miquel Raynal
2026-09-25 16:42 ` Conor Dooley
2026-09-25 15:10 ` [PATCH v4 4/6] clk: nuvoton: " Miquel Raynal
2026-09-27 17:04 ` Jerome Brunet
2026-09-25 15:10 ` [PATCH v4 5/6] clk: nuvoton: ma35d1: Use clk_hw pointers as mux parents Miquel Raynal
2026-09-27 17:00 ` Jerome Brunet
2026-09-28 16:02 ` Miquel Raynal [this message]
2026-09-25 15:10 ` [PATCH v4 6/6] clk: nuvoton: ma35d1: Avoid possible error pointer dereferencing Miquel Raynal
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87h5j974aj.fsf@bootlin.com \
--to=miquel.raynal@bootlin.com \
--cc=STLin2@winbond.com \
--cc=arnd@arndb.de \
--cc=bmasney+clk@redhat.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=jbrunet+clk@baylibre.com \
--cc=jbrunet@baylibre.com \
--cc=krzk+dt@kernel.org \
--cc=krzk@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mturquette@baylibre.com \
--cc=richardcochran@gmail.com \
--cc=robh@kernel.org \
--cc=sboyd@kernel.org \
--cc=schung@nuvoton.com \
--cc=stable@vger.kernel.org \
--cc=thomas.petazzoni@bootlin.com \
--cc=ychuang3@nuvoton.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®