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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.