Re: [PATCH v6 08/12] clk: nuvoton: ma35d1: Retrieve HXT/LXT from DT

[Jerome Brunet <jbrunet@baylibre.com>]

On jeu. 01 oct. 2026 at 18:12, Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> On 01/10/2026 at 11:36:33 +02, Jerome Brunet <jbrunet@baylibre.com> wrote:
>
>> On mer. 30 sept. 2026 at 19:24, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>>
>>> HXT and LXT are crystal oscillator inputs of the clock controller, they
>>> are described in the DT, so retrieve them, in order, and store them in
>>> their respective HXT/LXT hw table entries.
>>>
>>> Since old DTs reference the HXT fixed-clock without naming it and do not
>>> describe LXT at all, we assume that HXT must be present, and fallback to
>>> creating a fixed clock for LXT if it is not described (for backward
>>> compatibility purposes).
>>>
>>> The downstream gate clocks can directly use the hw clocks as parents,
>>> instead of relying on string matching.
>>>
>>> Fixes: 691521a367cf ("clk: nuvoton: Add clock driver for ma35d1 clock controller")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>> ---
>>>  drivers/clk/nuvoton/clk-ma35d1.c | 43 ++++++++++++++++++++++++++++++++--------
>>>  1 file changed, 35 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/clk/nuvoton/clk-ma35d1.c b/drivers/clk/nuvoton/clk-ma35d1.c
>>> index ceebcbd8c18b..d955d79abdd2 100644
>>> --- a/drivers/clk/nuvoton/clk-ma35d1.c
>>> +++ b/drivers/clk/nuvoton/clk-ma35d1.c
>>> @@ -4,6 +4,7 @@
>>>   * Author: Chi-Fang Li <cfli0@nuvoton.com>
>>>   */
>>>  
>>> +#include <linux/clk.h>
>>>  #include <linux/clk-provider.h>
>>>  #include <linux/mfd/syscon.h>
>>>  #include <linux/module.h>
>>> @@ -191,6 +192,15 @@ static struct clk_hw *ma35d1_clk_gate(struct device *dev, const char *name, cons
>>>  				    reg, shift, 0, &ma35d1_lock);
>>>  }
>>>  
>>> +static struct clk_hw *ma35d1_clk_gate_parent(struct device *dev, const char *name,
>>> +					     struct clk_hw *parent,
>>> +					     void __iomem *reg, u8 shift)
>>> +{
>>> +	return devm_clk_hw_register_gate_parent_hw(dev, name, parent,
>>> +						   CLK_SET_RATE_PARENT,
>>> +						   reg, shift, 0, &ma35d1_lock);
>>> +}
>>> +
>>>  static int ma35d1_get_pll_setting(struct device_node *clk_node, u32 *pllmode)
>>>  {
>>>  	const char *of_str;
>>> @@ -215,10 +225,12 @@ static int ma35d1_clocks_probe(struct platform_device *pdev)
>>>  {
>>>  	struct device *dev = &pdev->dev;
>>>  	struct device_node *clk_node = pdev->dev.of_node;
>>> +	struct clk_bulk_data *clks;
>>>  	void __iomem *clk_base;
>>>  	static struct clk_hw **hws;
>>>  	static struct clk_hw_onecell_data *ma35d1_hw_data;
>>>  	u32 pllmode[PLL_MAX_NUM];
>>> +	int num_clks;
>>>  	int ret;
>>>  
>>>  	ma35d1_hw_data = devm_kzalloc(dev,
>>> @@ -240,12 +252,27 @@ static int ma35d1_clocks_probe(struct platform_device *pdev)
>>>  		return -EINVAL;
>>>  	}
>>>  
>>> -	hws[HXT] = ma35d1_clk_fixed("hxt", 24000000);
>>> -	hws[HXT_GATE] = ma35d1_clk_gate(dev, "hxt_gate", "hxt",
>>> -					clk_base + REG_CLK_PWRCTL, 0);
>>> -	hws[LXT] = ma35d1_clk_fixed("lxt", 32768);
>>> -	hws[LXT_GATE] = ma35d1_clk_gate(dev, "lxt_gate", "lxt",
>>> -					clk_base + REG_CLK_PWRCTL, 1);
>>> +	num_clks = devm_clk_bulk_get_all(dev, &clks);
>>> +	if (num_clks < 0)
>>> +		return num_clks;
>>> +
>>> +	if (!num_clks) {
>>> +		dev_err(dev, "missing crystal input clocks\n");
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	hws[HXT] = __clk_get_hw(clks[0].clk);
>>
>> Don't open code it. use .fw_name
>
> Ok, if I understand your suggestion, I will go for the use of
>
>     devm_clk_hw_register_fixed_rate_parent_data()
>
> for these fixed clocks.
>
>>
>>> +
>>> +	if (num_clks > 1)
>>> +		hws[LXT] = __clk_get_hw(clks[1].clk);
>>> +	else
>>> +		/* Old DTs do not describe the low-speed crystal */
>>> +		hws[LXT] = ma35d1_clk_fixed("lxt", 32768);
>>
>> I'd give it another name so you can clearly see the difference between the
>> DT one and the manually registered one.
>>
>>> +
>>
>> Don't need to open code this either.
>> provide both .fw_name and .name - CCF will fallback to the name.
>>
>> When you want to conditionally register the fixed is up to you.
>
> Ok, so if my understanding is correct, I should use parent data with:
> * .fw_name being the clock-names entry
> * .name being the name of the clock that will be created ex-nihilo
> Am I correct? And clock-output-names in that case has no importance at
> all (hence this strengthen my wish to get rid of it)?

Yes

>
> In the fallback case, what naming makes sense? I don't know. I would
> have preferred to just name it "hxt" (respectively "lxt") in both cases
> because we truly don't care about the name, except it would be nicer for
> the reader of clk_summary. Do you mind if I keep "hxt"/"lxt" for both?

Picking another name is merely a suggestion. I don't have a strong opinion
over this.

>
> Thanks a lot for the hints!
> Miquèl

-- 
Jerome