Hi Ralph,
ralph.siemsen@xxxxxxxxxx wrote on Mon, 27 Feb 2023 13:39:36 -0500:
> Each entry in the clock table specifies a number of individual bits in
> registers, for contolling clock reset, gaiting, etc. These reg/bit were
> packed into a u16 to save space. The combined value is difficult to
> understand when reviewing the clock table entries.
I totally agree.
> Introduce a "struct regbit" which still occupies only 16 bits, but
> allows the register and bit values to be specified explicitly. Convert
> all previous uses of u16 for reg/bit into "struct regbit".
I was sure the structure would be bigger than 2B but yeah, gcc seems to
keep it small. However if at some point we add another member, we
might consider packing it.
> The bulk of this patch converts the clock table to use struct regbit,
> making use of the RB() helper macro. The conversion was automated by
> script, and as a further verification, the compiled binary of the table
> was compared before/after the change (with objdump -D).
I will trust your tool on the conversion.
> The clk_rdesc_set() function now checks for zero reg/bit internally.
> This allows callers of that function to remove those checks.
>
> Signed-off-by: Ralph Siemsen <ralph.siemsen@xxxxxxxxxx>
> ---
>
> drivers/clk/renesas/r9a06g032-clocks.c | 564 ++++++++++++++++++-------
> 1 file changed, 410 insertions(+), 154 deletions(-)
>
> diff --git a/drivers/clk/renesas/r9a06g032-clocks.c b/drivers/clk/renesas/r9a06g032-clocks.c
> index 1b7801f14c8c..f5d12d8f1b22 100644
> --- a/drivers/clk/renesas/r9a06g032-clocks.c
> +++ b/drivers/clk/renesas/r9a06g032-clocks.c
> @@ -27,6 +27,34 @@
>
> #define R9A06G032_SYSCTRL_DMAMUX 0xA0
>
> +/**
> + * struct regbit - describe one bit in a register
> + * @reg: offset of register relative to base address,
> + * expressed in units of 32-bit words (not bytes),
> + * @bit: which bit (0 to 31) in the register
> + *
> + * This structure is used to compactly encode the location
> + * of a single bit in a register. Five bits are needed to
> + * encode the bit number. With uint16_t data type, this
> + * leaves 11 bits to encode a register offset up to 2047.
> + *
> + * Since registers are aligned on 32-bit boundaries, the
> + * offset will be specified in 32-bit words rather than bytes.
> + * This allows encoding an offset up to 0x1FFC (8188) bytes.
> + *
> + * Helper macro RB() takes care of converting the register
> + * offset from bytes to 32-bit words.
> + */
> +struct regbit {
> + u16 reg:11;
> + u16 bit:5;
> +};
> +
> +#define RB(_reg, _bit) ((struct regbit) { \
> + .reg = (_reg) >> 2, \
Here and below, I would really prefer a "* 4" and a "/ 4". IMHO
shifts should stay reserved to bit operations. Here, what we want
is to convert a 1-byte offset into a 4-byte offset, thus the operation
is a multiplication. Even in -O0, all compilers would convert this into
a shift operation in the end. I believe we shall not sacrifice
readability in situations like that, were the optimization is trivial
for the tools.
> + .bit = (_bit) \
> +})
[...]
>
> struct r9a06g032_priv {
> @@ -419,26 +677,30 @@ int r9a06g032_sysctrl_set_dmamux(u32 mask, u32 val)
> }
> EXPORT_SYMBOL_GPL(r9a06g032_sysctrl_set_dmamux);
>
> -/* register/bit pairs are encoded as an uint16_t */
> -static void
> -clk_rdesc_set(struct r9a06g032_priv *clocks,
> - u16 one, unsigned int on)
> +static void clk_rdesc_set(struct r9a06g032_priv *clocks,
> + struct regbit rb, unsigned int on)
> {
> - u32 __iomem *reg = clocks->reg + (4 * (one >> 5));
> - u32 val = readl(reg);
> + u32 offset = rb.reg << 2;
Same here.
> + u32 bit = rb.bit;
> + u32 __iomem *reg;
> + u32 val;
>
> - val = (val & ~(1U << (one & 0x1f))) | ((!!on) << (one & 0x1f));
> + if (!offset && !bit)
'bit' being an offset, is it correct to refuse writing BIT(0) ?
> + return;
> +
> + reg = clocks->reg + offset;
> + val = readl(reg);
Could you unify the how reg is accessed here and below? I think I have
a slight preference for:
u32 __iomem *reg = clocks->reg + (rb.reg * 4);
Thanks,
Miquèl
