Re: [PATCH v5 04/31] clk: at91: clk-sam9x60-pll: use clk_parent_data

[Claudiu Beznea]

On 1/16/26 22:06, ryan.wanner@xxxxxxxxxxxxx wrote:
> From: Claudiu Beznea <claudiu.beznea@xxxxxxxxx>
>
> Use struct clk_parent_data instead of struct parent_hw as this leads
> to less usage of __clk_get_hw() in SoC specific clock drivers and simpler
> conversion of existing SoC specific clock drivers from parent_names to
> modern clk_parent_data structures. As clk-sam9x60-pll need to know
> parent's rate at initialization we pass it now from SoC specific drivers.
> This will lead in the end at removing __clk_get_hw() in SoC specific
> drivers (that will be solved by subsequent commits).
>
> Get the main_xtal name via of_clk_get_parent_name() to consistently get
> the correct name for the main_xtal.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea@xxxxxxxxx>
> [ryan.wanner@xxxxxxxxxxxxx: Add SAMA7D65 and SAM9X75 SoCs to the change set.]
> Signed-off-by: Ryan Wanner <Ryan.Wanner@xxxxxxxxxxxxx>
> ---

[ ... ]

> @@ -1137,10 +1141,8 @@ static void __init sama7d65_pmc_setup(struct device_node *np)
>   
>   	bypass = of_property_read_bool(np, "atmel,osc-bypass");
>   
> -	parent_data.name = main_xtal_name;
> -	parent_data.fw_name = main_xtal_name;
>   	main_osc_hw = at91_clk_register_main_osc(regmap, "main_osc", NULL,
> -						 &parent_data, bypass);
> +						 &AT91_CLK_PD_NAME(main_xtal_name), bypass);
>   	if (IS_ERR(main_osc_hw))
>   		goto err_free;
>   
> @@ -1154,7 +1156,7 @@ static void __init sama7d65_pmc_setup(struct device_node *np)
>   
>   	for (i = 0; i < PLL_ID_MAX; i++) {
>   		for (j = 0; j < PLL_COMPID_MAX; j++) {
> -			struct clk_hw *parent_hw;
> +			unsigned long parent_rate;
>   
>   			if (!sama7d65_plls[i][j].n)
>   				continue;
> @@ -1163,20 +1165,24 @@ static void __init sama7d65_pmc_setup(struct device_node *np)
>   			case PLL_TYPE_FRAC:
>   				switch (sama7d65_plls[i][j].p) {
>   				case SAMA7D65_PLL_PARENT_MAINCK:
> -					parent_hw = sama7d65_pmc->chws[PMC_MAIN];
> +					parent_data = AT91_CLK_PD_NAME("mainck");
> +					hw = sama7d65_pmc->chws[PMC_MAIN];
>   					break;
>   				case SAMA7D65_PLL_PARENT_MAIN_XTAL:
> -					parent_hw = main_xtal_hw;
> +					parent_data = AT91_CLK_PD_NAME(main_xtal_name);
> +					hw = main_xtal_hw;

main_xtal_hw is not initialized. You should be able to do something similar to 
what was done in patch 7/31 for sama7g5.

>   					break;
>   				default:
>   					/* Should not happen. */
> -					parent_hw = NULL;
>   					break;
>   				}
> +				parent_rate = clk_hw_get_rate(hw);
> +				if (!parent_rate)
> +					return;
>   
>   				hw = sam9x60_clk_register_frac_pll(regmap,
>   					&pmc_pll_lock, sama7d65_plls[i][j].n,
> -					NULL, parent_hw, i,
> +					&parent_data, parent_rate, i,
>   					sama7d65_plls[i][j].c,
>   					sama7d65_plls[i][j].l,
>   					sama7d65_plls[i][j].f);
> diff --git a/drivers/clk/at91/sama7g5.c b/drivers/clk/at91/sama7g5.c
> index 713f5dfe7be2..c6550044cba1 100644
> --- a/drivers/clk/at91/sama7g5.c
> +++ b/drivers/clk/at91/sama7g5.c
> @@ -971,7 +971,7 @@ static const struct clk_pcr_layout sama7g5_pcr_layout = {
>   
>   static void __init sama7g5_pmc_setup(struct device_node *np)
>   {
> -	const char *main_xtal_name = "main_xtal";
> +	const char *main_xtal_name;
>   	struct pmc_data *sama7g5_pmc;
>   	void **alloc_mem = NULL;
>   	int alloc_mem_size = 0;
> @@ -983,11 +983,15 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
>   	bool bypass;
>   	int i, j;
>   
> +	i = of_property_match_string(np, "clock-names", "main_xtal");
> +	if (i < 0)
> +		return;
> +	main_xtal_name = of_clk_get_parent_name(np, i);
> +
>   	td_slck_hw = __clk_get_hw(of_clk_get_by_name(np, "td_slck"));
>   	md_slck_hw = __clk_get_hw(of_clk_get_by_name(np, "md_slck"));
> -	main_xtal_hw = __clk_get_hw(of_clk_get_by_name(np, main_xtal_name));
>   
> -	if (!td_slck_hw || !md_slck_hw || !main_xtal_hw)
> +	if (!td_slck_hw || !md_slck_hw)
>   		return;
>   
>   	regmap = device_node_to_regmap(np);
> @@ -1014,10 +1018,8 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
>   
>   	bypass = of_property_read_bool(np, "atmel,osc-bypass");
>   
> -	parent_data.name = main_xtal_name;
> -	parent_data.fw_name = main_xtal_name;
>   	main_osc_hw = at91_clk_register_main_osc(regmap, "main_osc", NULL,
> -						 &parent_data, bypass);
> +						 &AT91_CLK_PD_NAME(main_xtal_name), bypass);
>   	if (IS_ERR(main_osc_hw))
>   		goto err_free;
>   
> @@ -1031,7 +1033,7 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
>   
>   	for (i = 0; i < PLL_ID_MAX; i++) {
>   		for (j = 0; j < PLL_COMPID_MAX; j++) {
> -			struct clk_hw *parent_hw;
> +			unsigned long parent_rate;
>   
>   			if (!sama7g5_plls[i][j].n)
>   				continue;
> @@ -1040,20 +1042,25 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
>   			case PLL_TYPE_FRAC:
>   				switch (sama7g5_plls[i][j].p) {
>   				case SAMA7G5_PLL_PARENT_MAINCK:
> -					parent_hw = sama7g5_pmc->chws[PMC_MAIN];
> +					parent_data = AT91_CLK_PD_NAME("mainck");
> +					hw = sama7g5_pmc->chws[PMC_MAIN];
>   					break;
>   				case SAMA7G5_PLL_PARENT_MAIN_XTAL:
> -					parent_hw = main_xtal_hw;
> +					parent_data = AT91_CLK_PD_NAME(main_xtal_name);
> +					hw = main_xtal_hw;

main_xtal_hw is not initialized. You should be able to do something similar to 
what was done in patch 7/31 for sama7g5.

The rest LGTM.

Thank you,
Claudiu