Re: [PATCH v5 03/31] clk: at91: sam9x75: switch to parent_hw and parent_data

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

Hi, Ryan,

On 1/16/26 22:06, ryan.wanner@microchip.com wrote:
> From: Ryan Wanner <Ryan.Wanner@microchip.com>
> 
> Switch SAM9X75 clocks to use parent_hw and parent_data. Having
> parent_hw instead of parent names improves to clock registration
> speed and re-parenting.
> 
> The USBCLK will be updated in subsequent patches that update the clock
> registration functions to use parent_hw and parent_data.
> 
> __clk_get_hw() will be removed in subsequent patches in this series.
> 
> Signed-off-by: Ryan Wanner <Ryan.Wanner@microchip.com>
> ---
>   drivers/clk/at91/sam9x7.c | 305 ++++++++++++++++++++++----------------
>   1 file changed, 174 insertions(+), 131 deletions(-)
> 
> diff --git a/drivers/clk/at91/sam9x7.c b/drivers/clk/at91/sam9x7.c
> index 89868a0aeaba..c48c91da914e 100644
> --- a/drivers/clk/at91/sam9x7.c
> +++ b/drivers/clk/at91/sam9x7.c


[ ...]

>   
>   /*
> @@ -426,7 +450,8 @@ static const struct {
>   /*
>    * Generic clock description
>    * @n:			clock name
> - * @pp:			PLL parents
> + * @pp:			PLL parents (entry formed by PLL components identifiers
> + *			(see enum pll_component_id))
>    * @pp_mux_table:	PLL parents mux table
>    * @r:			clock output range
>    * @pp_chg_id:		id in parent array of changeable PLL parent
> @@ -435,7 +460,10 @@ static const struct {
>    */
>   static const struct {
>   	const char *n;
> -	const char *pp[8];
> +	struct {
> +			int pll_id;
> +			int pll_compid;

Still having more than 2 tabs here.

[ ... ]

>   static void __init sam9x7_pmc_setup(struct device_node *np)
>   {
>   	struct clk_range range = CLK_RANGE(0, 0);
> -	const char *td_slck_name, *md_slck_name, *mainxtal_name;
> +	const char *main_xtal_name;
>   	struct pmc_data *sam9x7_pmc;
>   	const char *parent_names[9];
>   	void **clk_mux_buffer = NULL;
>   	int clk_mux_buffer_size = 0;
> -	struct clk_hw *main_osc_hw;
>   	struct regmap *regmap;
> -	struct clk_hw *hw;
> +	struct clk_hw *hw, *main_rc_hw, *main_osc_hw, *main_xtal_hw;
> +	struct clk_hw *td_slck_hw, *md_slck_hw, *usbck_hw;
> +	struct clk_hw *parent_hws[9];
>   	int i, j;
>   
> -	i = of_property_match_string(np, "clock-names", "td_slck");
> -	if (i < 0)
> -		return;
> -
> -	td_slck_name = of_clk_get_parent_name(np, i);
> +	i = of_property_match_string(np, "clock-names", "main_xtal");
>   
> -	i = of_property_match_string(np, "clock-names", "md_slck");
>   	if (i < 0)
>   		return;
> +	main_xtal_name = of_clk_get_parent_name(np, i);
>   
> -	md_slck_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"));
>   
> -	i = of_property_match_string(np, "clock-names", "main_xtal");
> -	if (i < 0)
> +	if (!td_slck_hw || !md_slck_hw)
>   		return;
> -	mainxtal_name = of_clk_get_parent_name(np, i);

I would have been prefered that the code here to look more organized, something 
like:

	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"));
	if (!td_slck_hw || !md_slck_hw)
		return;

but it is OK for now as the final form looks alike.

>   
>   	regmap = device_node_to_regmap(np);
>   	if (IS_ERR(regmap))
> @@ -760,26 +784,26 @@ static void __init sam9x7_pmc_setup(struct device_node *np)
>   	if (!clk_mux_buffer)
>   		goto err_free;
>   
> -	hw = at91_clk_register_main_rc_osc(regmap, "main_rc_osc", 12000000,
> -					   50000000);
> -	if (IS_ERR(hw))
> +	main_rc_hw = at91_clk_register_main_rc_osc(regmap, "main_rc_osc", 12000000,
> +						   50000000);
> +	if (IS_ERR(main_rc_hw))
>   		goto err_free;
>   
> -	hw = at91_clk_register_main_osc(regmap, "main_osc", mainxtal_name, NULL, 0);
> -	if (IS_ERR(hw))
> +	main_osc_hw = at91_clk_register_main_osc(regmap, "main_osc", NULL,
> +						 &AT91_CLK_PD_NAME(main_xtal_name), 0);
> +	if (IS_ERR(main_osc_hw))
>   		goto err_free;
> -	main_osc_hw = hw;
>   
> -	parent_names[0] = "main_rc_osc";
> -	parent_names[1] = "main_osc";
> -	hw = at91_clk_register_sam9x5_main(regmap, "mainck", parent_names, NULL, 2);
> +	parent_hws[0] = main_rc_hw;
> +	parent_hws[1] = main_osc_hw;
> +	hw = at91_clk_register_sam9x5_main(regmap, "mainck", NULL, parent_hws, 2);
>   	if (IS_ERR(hw))
>   		goto err_free;
>   
>   	sam9x7_pmc->chws[PMC_MAIN] = hw;
>   
>   	for (i = 0; i < PLL_ID_MAX; i++) {
> -		for (j = 0; j < 3; j++) {
> +		for (j = 0; j < PLL_COMPID_MAX; j++) {
>   			struct clk_hw *parent_hw;
>   
>   			if (!sam9x7_plls[i][j].n)
> @@ -787,19 +811,23 @@ static void __init sam9x7_pmc_setup(struct device_node *np)
>   
>   			switch (sam9x7_plls[i][j].t) {
>   			case PLL_TYPE_FRAC:
> -				if (!strcmp(sam9x7_plls[i][j].p, "mainck"))
> +				switch (sam9x7_plls[i][j].p) {
> +				case SAM9X7_PLL_PARENT_MAINCK:
>   					parent_hw = sam9x7_pmc->chws[PMC_MAIN];
> -				else if (!strcmp(sam9x7_plls[i][j].p, "main_osc"))
> -					parent_hw = main_osc_hw;
> -				else
> -					parent_hw = __clk_get_hw(of_clk_get_by_name
> -								 (np, sam9x7_plls[i][j].p));
> +					break;
> +				case SAM9X7_PLL_PARENT_MAIN_XTAL:
> +					parent_hw = main_xtal_hw;

main_xtal_hw is not initialized. You should be able to do here as you did on 
patch 7/31.

> +					break;
> +				default:
> +					/* Should not happen. */
> +					parent_hw = NULL;
> +					break;
> +				}
>   
>   				hw = sam9x60_clk_register_frac_pll(regmap,
>   								   &pmc_pll_lock,
>   								   sam9x7_plls[i][j].n,
> -								   sam9x7_plls[i][j].p,
> -								   parent_hw, i,
> +								   NULL, parent_hw, i,
>   								   sam9x7_plls[i][j].c,
>   								   sam9x7_plls[i][j].l,
>   								   sam9x7_plls[i][j].f);
> @@ -809,7 +837,7 @@ static void __init sam9x7_pmc_setup(struct device_node *np)
>   				hw = sam9x60_clk_register_div_pll(regmap,
>   								  &pmc_pll_lock,
>   								  sam9x7_plls[i][j].n,
> -								  sam9x7_plls[i][j].p, NULL, i,
> +								  NULL, sam9x7_plls[i][0].hw, i,
>   								  sam9x7_plls[i][j].c,
>   								  sam9x7_plls[i][j].l,
>   								  sam9x7_plls[i][j].f, 0);
> @@ -822,23 +850,24 @@ static void __init sam9x7_pmc_setup(struct device_node *np)
>   			if (IS_ERR(hw))
>   				goto err_free;
>   
> +			sam9x7_plls[i][j].hw = hw;
>   			if (sam9x7_plls[i][j].eid)
>   				sam9x7_pmc->chws[sam9x7_plls[i][j].eid] = hw;
>   		}
>   	}
>   
> -	parent_names[0] = md_slck_name;
> -	parent_names[1] = "mainck";
> -	parent_names[2] = "plla_divpmcck";
> -	parent_names[3] = "upll_divpmcck";
> +	parent_hws[0] = md_slck_hw;
> +	parent_hws[1] = sam9x7_pmc->chws[PMC_MAIN];
> +	parent_hws[2] = sam9x7_plls[PLL_ID_PLLA][PLL_COMPID_DIV0].hw;
> +	parent_hws[3] = sam9x7_plls[PLL_ID_UPLL][PLL_COMPID_DIV0].hw;
>   	hw = at91_clk_register_master_pres(regmap, "masterck_pres", 4,
> -					   parent_names, NULL, &sam9x7_master_layout,
> +					   NULL, parent_hws, &sam9x7_master_layout,
>   					   &mck_characteristics, &mck_lock);
>   	if (IS_ERR(hw))
>   		goto err_free;
>   
>   	hw = at91_clk_register_master_div(regmap, "masterck_div",
> -					  "masterck_pres", NULL, &sam9x7_master_layout,
> +					  NULL, hw, &sam9x7_master_layout,
>   					  &mck_characteristics, &mck_lock,
>   					  CLK_SET_RATE_GATE, 0);
>   	if (IS_ERR(hw))
> @@ -849,24 +878,24 @@ static void __init sam9x7_pmc_setup(struct device_node *np)
>   	parent_names[0] = "plla_divpmcck";
>   	parent_names[1] = "upll_divpmcck";
>   	parent_names[2] = "main_osc";
> -	hw = sam9x60_clk_register_usb(regmap, "usbck", parent_names, 3);
> -	if (IS_ERR(hw))
> +	usbck_hw = sam9x60_clk_register_usb(regmap, "usbck", parent_names, 3);
> +	if (IS_ERR(usbck_hw))
>   		goto err_free;
>   
> -	parent_names[0] = md_slck_name;
> -	parent_names[1] = td_slck_name;
> -	parent_names[2] = "mainck";
> -	parent_names[3] = "masterck_div";
> -	parent_names[4] = "plla_divpmcck";
> -	parent_names[5] = "upll_divpmcck";
> -	parent_names[6] = "audiopll_divpmcck";
> +	parent_hws[0] = md_slck_hw;
> +	parent_hws[1] = td_slck_hw;
> +	parent_hws[2] = sam9x7_pmc->chws[PMC_MAIN];
> +	parent_hws[3] = sam9x7_pmc->chws[PMC_MCK];
> +	parent_hws[4] = sam9x7_plls[PLL_ID_PLLA][PLL_COMPID_DIV0].hw;
> +	parent_hws[5] = sam9x7_plls[PLL_ID_UPLL][PLL_COMPID_DIV0].hw;
> +	parent_hws[6] = sam9x7_plls[PLL_ID_AUDIO][PLL_COMPID_DIV0].hw;
>   	for (i = 0; i < 2; i++) {
>   		char name[6];
>   
>   		snprintf(name, sizeof(name), "prog%d", i);
>   
>   		hw = at91_clk_register_programmable(regmap, name,
> -						    parent_names, NULL, 7, i,
> +						    NULL, parent_hws, 7, i,
>   						    &sam9x7_programmable_layout,
>   						    NULL);
>   		if (IS_ERR(hw))
> @@ -875,9 +904,14 @@ static void __init sam9x7_pmc_setup(struct device_node *np)
>   		sam9x7_pmc->pchws[i] = hw;
>   	}
>   
> +	/* Set systemck parent hws. */
> +	sam9x7_systemck[0].parent_hw = sam9x7_pmc->chws[PMC_MCK];
> +	sam9x7_systemck[1].parent_hw = usbck_hw;
> +	sam9x7_systemck[2].parent_hw = sam9x7_pmc->pchws[0];
> +	sam9x7_systemck[3].parent_hw = sam9x7_pmc->pchws[1];
>   	for (i = 0; i < ARRAY_SIZE(sam9x7_systemck); i++) {
>   		hw = at91_clk_register_system(regmap, sam9x7_systemck[i].n,
> -					      sam9x7_systemck[i].p, NULL,
> +					      NULL, sam9x7_systemck[i].parent_hw,
>   					      sam9x7_systemck[i].id,
>   					      sam9x7_systemck[i].flags);
>   		if (IS_ERR(hw))
> @@ -890,7 +924,7 @@ static void __init sam9x7_pmc_setup(struct device_node *np)
>   		hw = at91_clk_register_sam9x5_peripheral(regmap, &pmc_pcr_lock,
>   							 &sam9x7_pcr_layout,
>   							 sam9x7_periphck[i].n,
> -							 "masterck_div", NULL,
> +							 NULL, sam9x7_pmc->chws[PMC_MCK],
>   							 sam9x7_periphck[i].id,
>   							 &range, INT_MIN,
>   							 sam9x7_periphck[i].f);
> @@ -900,12 +934,13 @@ static void __init sam9x7_pmc_setup(struct device_node *np)
>   		sam9x7_pmc->phws[sam9x7_periphck[i].id] = hw;
>   	}
>   
> -	parent_names[0] = md_slck_name;
> -	parent_names[1] = td_slck_name;
> -	parent_names[2] = "mainck";
> -	parent_names[3] = "masterck_div";
> +	parent_hws[0] = md_slck_hw;
> +	parent_hws[1] = td_slck_hw;
> +	parent_hws[2] = sam9x7_pmc->chws[PMC_MAIN];
> +	parent_hws[3] = sam9x7_pmc->chws[PMC_MCK];
>   	for (i = 0; i < ARRAY_SIZE(sam9x7_gck); i++) {
>   		u8 num_parents = 4 + sam9x7_gck[i].pp_count;
> +		struct clk_hw *tmp_parent_hws[6];

I see maximum pp_count is 2, thus the size of this array could be reduced.

>   		u32 *mux_table;
>   
>   		mux_table = kmalloc_array(num_parents, sizeof(*mux_table),
> @@ -916,13 +951,21 @@ static void __init sam9x7_pmc_setup(struct device_node *np)
>   		PMC_INIT_TABLE(mux_table, 4);
>   		PMC_FILL_TABLE(&mux_table[4], sam9x7_gck[i].pp_mux_table,
>   			       sam9x7_gck[i].pp_count);
> -		PMC_FILL_TABLE(&parent_names[4], sam9x7_gck[i].pp,
> +
> +		for (j = 0; j < sam9x7_gck[i].pp_count; j++) {
> +			u8 pll_id = sam9x7_gck[i].pp[j].pll_id;
> +			u8 pll_compid = sam9x7_gck[i].pp[j].pll_compid;
> +
> +			tmp_parent_hws[j] = sam9x7_plls[pll_id][pll_compid].hw;

You can get rid of tmp_parent_hws[] and the above PMC_FILL_TABLE() and just use 
here something like:

			parent_hws[4 + j] = sam9x7_plls[pll_id][pll_compid].hw;

> +		}
> +
> +		PMC_FILL_TABLE(&parent_hws[4], tmp_parent_hws,
>   			       sam9x7_gck[i].pp_count);
>   
>   		hw = at91_clk_register_generated(regmap, &pmc_pcr_lock,
>   						 &sam9x7_pcr_layout,
>   						 sam9x7_gck[i].n,
> -						 parent_names, NULL, mux_table,
> +						 NULL, parent_hws, mux_table,
>   						 num_parents,
>   						 sam9x7_gck[i].id,
>   						 &sam9x7_gck[i].r,