| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8fea4eb8-6160-448c-b0b2-5c093a1ed769@tuxon.dev>
Date: Mon, 26 Jan 2026 10:51:53 +0200
From: Claudiu Beznea <claudiu.beznea@...on.dev>
To: ryan.wanner@...rochip.com, mturquette@...libre.com, sboyd@...nel.org,
nicolas.ferre@...rochip.com, alexandre.belloni@...tlin.com,
bmasney@...hat.com, alexander.sverdlin@...il.com,
varshini.rajendran@...rochip.com
Cc: cristian.birsan@...rochip.com, balamanikandan.gunasundar@...rochip.com,
linux-clk@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 03/31] clk: at91: sam9x75: switch to parent_hw and
parent_data
Hi, Ryan,
On 1/16/26 22:06, ryan.wanner@...rochip.com wrote:
> From: Ryan Wanner <Ryan.Wanner@...rochip.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@...rochip.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,
Powered by blists - more mailing lists