Hi Geert, Thank you for the review. On Fri, Jul 26, 2024 at 3:53 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote: > > Hi Prabhakar, > > On Mon, Jul 15, 2024 at 2:56 PM Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > > > Add family-specific clock driver for RZ/V2H(P) SoCs. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > --- > > v3->v4 > > - Dropped masking of parent clks with 0xffff > > - Dropped storing mod clk id and now calculating it > > based on index and bit. > > - Made parent to u16 in struct rzv2h_mod_clk > > - Made a copy of resets array in struct rzv2h_cpg_priv > > Thanks for the update! > > > --- /dev/null > > +++ b/drivers/clk/renesas/rzv2h-cpg.c > > > +/** > > + * struct rzv2h_cpg_priv - Clock Pulse Generator Private Data > > + * > > + * @info: Pointer to platform data > > There is no longer an info member. > Agreed, I will drop it. > Hint: W=1 would have told you. > Thanks for the pointer. > > + * @dev: CPG device > > + * @base: CPG register block base address > > + * @clks: Array containing all Core and Module Clocks > > + * @num_core_clks: Number of Core Clocks in clks[] > > + * @num_mod_clks: Number of Module Clocks in clks[] > > + * @resets: Array of resets > > + * @num_resets: Number of Module Resets in info->resets[] > > + * @last_dt_core_clk: ID of the last Core Clock exported to DT > > + * @rcdev: Reset controller entity > > + */ > > +struct rzv2h_cpg_priv { > > + struct device *dev; > > + void __iomem *base; > > + > > + struct clk **clks; > > + unsigned int num_core_clks; > > + unsigned int num_mod_clks; > > + struct rzv2h_reset *resets; > > + unsigned int num_resets; > > + unsigned int last_dt_core_clk; > > + > > + struct reset_controller_dev rcdev; > > +}; > > > index 000000000000..33631c101541 > > --- /dev/null > > +++ b/drivers/clk/renesas/rzv2h-cpg.h > > > +#define DEF_RST_BASE(_id, _resindex, _resbit, _monindex, _monbit) \ > > + [_id] = { \ > > Indexing by _id means the reset array will be very sparse. E.g. the > innocent-looking r9a09g057_resets[] with only a single entry takes > 600 bytes: > > $ nm -S drivers/clk/renesas/r9a09g057-cpg.o | grep r9a09g057_resets > 0000000000000038 0000000000000258 r r9a09g057_resets > Agreed. > So please pack the array here, and either unpack it while making the > priv->resets copy, or implement translation ("look-up") from ID to > packed index in rzv2h_cpg_reset_xlate(). > OK, I will implement the below: #define PACK_RESET(_resindex, _resbit, _monindex, _monbit) \ (((_resindex) << 24) | ((_resbit) << 16) | ((_monindex) << 8) | (_monbit)) #define DEF_RST(_resindex, _resbit, _monindex, _monbit) \ PACK_RESET(_resindex, _resbit, _monindex, _monbit) #define GET_RESET_INDEX(x) (((x) >> 24) & 0xFF) #define GET_RESET_BIT(x) (((x) >> 16) & 0xFF) #define GET_MON_INDEX(x) (((x) >> 8) & 0xFF) #define GET_MON_BIT(x) ((x) & 0xFF) static int rzv2h_cpg_reset_xlate(struct reset_controller_dev *rcdev, const struct of_phandle_args *reset_spec) { struct rzv2h_cpg_priv *priv = rcdev_to_priv(rcdev); unsigned int id = reset_spec->args[0]; u8 rst_index = id / 16; u8 rst_bit = id % 16; unsigned int i; for (i = 0; i < rcdev->nr_resets; i++) { u8 cur_index = GET_RESET_INDEX(priv->resets[i]); u8 cur_bit = GET_RESET_BIT(priv->resets[i]); if (rst_index == cur_index && rst_bit == cur_bit) return i; } return -EINVAL; } Let me know if this is OK, or to avoid looping in xlate maybe we can have a packed entry in the resets property of DT by this way we can avoid having the resets array all together? Cheers, Prabhakar