Hi Prabhakar,
On Sat, Jul 27, 2024 at 12:51 PM Lad, Prabhakar
<prabhakar.csengg@xxxxxxxxx> wrote:
> On Fri, Jul 26, 2024 at 3:53 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > 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>
> > > --- /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?
Sorry for being unclear. I did not mean packing the fields in the struct
into a single word, but packing the entries in the r9a09g057_resets[]
array. Using the rzv2h_reset structure is fine.
With:
#define DEF_RST_BASE(_id, _resindex, _resbit, _monindex, _monbit) \
[_id] = { \
.reset_index = (_resindex), \
.reset_bit = (_resbit), \
.mon_index = (_monindex), \
.mon_bit = (_monbit), \
}
#define DEF_RST(_resindex, _resbit, _monindex, _monbit) \
DEF_RST_BASE(RST_ID((_resindex), (_resbit)), _resindex,
_resbit, _monindex, _monbit)
static const struct rzv2h_reset r9a09g057_resets[] __initconst = {
DEF_RST(9, 5, 4, 6), /* SCIF_0_RST_SYSTEM_N */
};
is expanded into an array of 150 entries (9 * 16 + 5 = 149 empty entries
followed by the SCIF_0_RST_SYSTEM_N entry), which is wasteful.
Over time the array will be filled more, but I expect there will still
be lots of unused entries.
Hence I suggest to drop the "[id]":
- define DEF_RST_BASE(_id, _resindex, _resbit, _monindex, _monbit) \
- [_id] = { \
+#define DEF_RST(_resindex, _resbit, _monindex, _monbit) \
+ { \
.reset_index = (_resindex), \
.reset_bit = (_resbit), \
.mon_index = (_monindex), \
.mon_bit = (_monbit), \
}
-
-#define DEF_RST(_resindex, _resbit, _monindex, _monbit) \
- DEF_RST_BASE(RST_ID((_resindex), (_resbit)), _resindex,
_resbit, _monindex, _monbit)
Then r9a09g057_resets[] will contain only non-empty entries, at the
expense of no longer being able to index it directly by reset ID.
To solve the indexing, there are two options.
Option A: Translate from reset ID to real index during lookup, like
you do in the rzv2h_cpg_reset_xlate() above:
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++) {
if (rst_index == priv->resets[i].reset_index &&
rst_bit == ->resets[i].reset_bit)
return i;
}
return -EINVAL;
}
Option B: "Unpack" rzv2h_cpg_info.resets[] during copying in
rzv2h_cpg_probe():
priv->resets = devm_kcalloc(dev, max_num_reset_ids,
sizeof(*priv->resets), GFP_KERNEL);
for (i = 0; i < ARRAY_SIZE(info->resets); i++) {
id = RST_ID(info->resets[i].reset_index, info->resets[i].reset_bit);
priv->resets[id] = info->resets[i];
}
BTW, for option B (and for the current code in v4),
rzv2h_cpg_reset_xlate() should validate that the entry is non-empty.
I hope this is more clear?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
