| 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: <CAMuHMdUu4o5VU9Sd1CW5QyEZewKLj--2yMPYCfcwVMut5FiVyg@mail.gmail.com>
Date: Thu, 17 Apr 2025 16:15:37 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Thierry Bultel <thierry.bultel.yh@...renesas.com>
Cc: thierry.bultel@...atsea.fr, linux-renesas-soc@...r.kernel.org,
paul.barker.ct@...renesas.com, linux-clk@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 05/13] clk: renesas: Pass sub struct of cpg_mssr_priv
to cpg_clk_register
Hi Thierry,
On Thu, 3 Apr 2025 at 23:30, Thierry Bultel
<thierry.bultel.yh@...renesas.com> wrote:
> In a subsequent patch, the registration callback will need more parameters
> from cpg_mssr_priv (like another base address with clock controllers
> with double register block, and also, notifiers and rmw_lock).
> Instead of adding more parameters, move the needed parameters to a public
> sub-struct.
> Instead moving clks to this structure, which would have implied to add
> an allocation (and cleanup) for it, keep the way the allocation is done
> and just have a copy of the pointer in the public structure.
>
> Signed-off-by: Thierry Bultel <thierry.bultel.yh@...renesas.com>
Thanks for your patch!
> --- a/drivers/clk/renesas/r8a77970-cpg-mssr.c
> +++ b/drivers/clk/renesas/r8a77970-cpg-mssr.c
> @@ -218,11 +218,13 @@ static int __init r8a77970_cpg_mssr_init(struct device *dev)
> }
>
> static struct clk * __init r8a77970_cpg_clk_register(struct device *dev,
> - const struct cpg_core_clk *core, const struct cpg_mssr_info *info,
> - struct clk **clks, void __iomem *base,
> - struct raw_notifier_head *notifiers)
> + const struct cpg_core_clk *core,
> + const struct cpg_mssr_info *info,
These two still fit on a single line, like before.
> + struct cpg_mssr_pub *pub)
> {
> const struct clk_div_table *table;
> + void __iomem *base = pub->base0;
> + struct clk **clks = pub->clks;
> const struct clk *parent;
> unsigned int shift;
>
> --- a/drivers/clk/renesas/rcar-gen3-cpg.h
> +++ b/drivers/clk/renesas/rcar-gen3-cpg.h
> @@ -80,9 +80,9 @@ struct rcar_gen3_cpg_pll_config {
> #define CPG_RCKCR 0x240
>
> struct clk *rcar_gen3_cpg_clk_register(struct device *dev,
> - const struct cpg_core_clk *core, const struct cpg_mssr_info *info,
> - struct clk **clks, void __iomem *base,
> - struct raw_notifier_head *notifiers);
> + const struct cpg_core_clk *core,
> + const struct cpg_mssr_info *info,
Likewise.
> + struct cpg_mssr_pub *pub);
> int rcar_gen3_cpg_init(const struct rcar_gen3_cpg_pll_config *config,
> unsigned int clk_extalr, u32 mode);
>
> --- a/drivers/clk/renesas/rcar-gen4-cpg.c
> +++ b/drivers/clk/renesas/rcar-gen4-cpg.c
> @@ -418,9 +418,11 @@ static const struct clk_div_table cpg_rpcsrc_div_table[] = {
>
> struct clk * __init rcar_gen4_cpg_clk_register(struct device *dev,
> const struct cpg_core_clk *core, const struct cpg_mssr_info *info,
> - struct clk **clks, void __iomem *base,
> - struct raw_notifier_head *notifiers)
> + struct cpg_mssr_pub *pub)
> {
> + struct raw_notifier_head *notifiers = &pub->notifiers;
> + void __iomem *base = pub->base0;
> + struct clk **clks = pub->clks;
> const struct clk *parent;
> unsigned int mult = 1;
> unsigned int div = 1;
> @@ -517,7 +519,7 @@ struct clk * __init rcar_gen4_cpg_clk_register(struct device *dev,
>
> case CLK_TYPE_GEN4_RPC:
> return cpg_rpc_clk_register(core->name, base + CPG_RPCCKCR,
> - __clk_get_name(parent), notifiers);
> + __clk_get_name(parent), &pub->notifiers);
Please drop this unneeded change.
>
> case CLK_TYPE_GEN4_RPCD2:
> return cpg_rpcd2_clk_register(core->name, base + CPG_RPCCKCR,
> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> @@ -127,14 +127,12 @@ static const u16 srstclr_for_gen4[] = {
> *
> * @rcdev: Optional reset controller entity
> * @dev: CPG/MSSR device
> - * @base: CPG/MSSR register block base address
> * @reg_layout: CPG/MSSR register layout
> * @rmw_lock: protects RMW register accesses
rmw_lock is removed below.
> * @np: Device node in DT for this CPG/MSSR module
> * @num_core_clks: Number of Core Clocks in clks[]
> * @num_mod_clks: Number of Module Clocks in clks[]
> * @last_dt_core_clk: ID of the last Core Clock exported to DT
> - * @notifiers: Notifier chain to save/restore clock state for system resume
> * @status_regs: Pointer to status registers array
> * @control_regs: Pointer to control registers array
> * @reset_regs: Pointer to reset registers array
> @@ -143,6 +141,7 @@ static const u16 srstclr_for_gen4[] = {
> * [].val: Saved values of SMSTPCR[]
> * @reserved_ids: Temporary used, reserved id list
> * @num_reserved_ids: Temporary used, number of reserved id list
> + * @pub: Data passed to clock registration callback
> * @clks: Array containing all Core and Module Clocks
> */
> struct cpg_mssr_priv {
> @@ -150,16 +149,13 @@ struct cpg_mssr_priv {
> struct reset_controller_dev rcdev;
> #endif
> struct device *dev;
> - void __iomem *base;
> enum clk_reg_layout reg_layout;
> - spinlock_t rmw_lock;
> struct device_node *np;
>
> unsigned int num_core_clks;
> unsigned int num_mod_clks;
> unsigned int last_dt_core_clk;
>
> - struct raw_notifier_head notifiers;
> const u16 *status_regs;
> const u16 *control_regs;
> const u16 *reset_regs;
> @@ -172,6 +168,7 @@ struct cpg_mssr_priv {
> unsigned int *reserved_ids;
> unsigned int num_reserved_ids;
>
> + struct cpg_mssr_pub pub;
Perhaps insert this at the top of the structure, so &priv->pub needs
no calculation?
> struct clk *clks[];
> };
> --- a/drivers/clk/renesas/renesas-cpg-mssr.h
> +++ b/drivers/clk/renesas/renesas-cpg-mssr.h
> @@ -27,6 +29,21 @@ struct cpg_core_clk {
> unsigned int div;
> unsigned int mult;
> unsigned int offset;
> +
> +/**
> + * struct cpg_mssr_pub - Private data shared with
The "private" sounds a bit weird here, so please just drop it.
> + * device-specific clk registration code
> + *
> + * @base0: CPG/MSSR register block base0 address
> + * @rmw_lock: protects RMW register accesses
> + * @notifiers: Notifier chain to save/restore clock state for system resume
These two lines should be exchanged, to match the declaration order
below.
> + * @clks: pointer to clocks
> + */
> +struct cpg_mssr_pub {
> + void __iomem *base0;
> + struct raw_notifier_head notifiers;
> + spinlock_t rmw_lock;
> + struct clk **clks;
> };
>
> enum clk_types {
The rest LGTM.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Powered by blists - more mailing lists