Hi Miquèl,
On Wed, Mar 1, 2023 at 7:58 AM Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
>
> > 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.
If we should need to expand this, eg. to handle something more
complicated than a single-bit control, then I would probably want to
rename the structure as well.
> > 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.
I'm going to check again using objdump, just to make sure nothing slips through.
> > +#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.
Reasonable, I'll make the change.
> > + 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) ?
This replaces the check that callers previously did on the combined u16 value:
if (g->reset)
clk_rdesc_set(clocks, g->reset, 1);
As you can see, it used zero as a special value to indicate "no reset
bit". I just kept the same approach, but moved the test inside the
function, to streamline the callers.
Of course, this means we cannot control a BIT(0) at register offset=0.
This is okay on RZ/N1 since offset=0 is an unrelated USB mode
configuration register.
> > + 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);
Yes, I will unify both.
Ralph
