Hi Geert,
Thank you for the review.
On Mon, Aug 26, 2024 at 2:34 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> On Thu, Aug 22, 2024 at 1:16 PM Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > Add support for dynamic switching divider clocks.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > v1->v2
> > - Dropped DDIV_DIVCTL_WIDTH
> > - width is now extracted from conf
> > - Updated DDIV_GET_* macros
> > - Now doing rmw as some of the DIVCTLx require it
>
> Thanks for the update!
>
> > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
> > @@ -45,14 +45,23 @@
> > #define PDIV(val) FIELD_GET(GENMASK(5, 0), (val))
> > #define SDIV(val) FIELD_GET(GENMASK(2, 0), (val))
> >
> > +#define DDIV_DIVCTL_WEN(shift) (1 << ((shift) + 16))
>
> BIT((shift) + 16)
>
Agreed.
> > +#define DDIV_GET_WIDTH(val) FIELD_GET(GENMASK(3, 0), (val))
> > +#define DDIV_GET_SHIFT(val) FIELD_GET(GENMASK(7, 4), (val))
> > +#define DDIV_GET_REG_OFFSET(val) FIELD_GET(GENMASK(18, 8), (val))
> > +#define DDIV_GET_MON(val) FIELD_GET(GENMASK(23, 19), (val))
>
> These are not register fields, so you might as well just use C bitfields
> accesses instead:
>
> struct ddiv {
> unsigned int width:4;
> unsigned int shift:4;
> unsigned int offset:11;
> unsigned int monbit:5;
> };
>
> if ((shift + core->ddiv.width > 16)
> return ERR_PTR(-EINVAL);
>
> (you can put cpg_core_clk.conf and cpg_core_clk.ddiv in a union to save space)
>
OK, I'll update it to below,
/**
* struct ddiv - Structure for dynamic switching divider
*
* @offset: register offset
* @shift: position of the divider bit
* @width: width of the divider
* @monbit: monitor bit in CPG_CLKSTATUS0 register
*/
struct ddiv {
unsigned int offset:11;
unsigned int shift:4;
unsigned int width:4;
unsigned int monbit:5;
};
#define DDIV_PACK(_offset, _shift, _width, _monbit) \
((struct ddiv){ \
.offset = _offset, \
.shift = _shift, \
.width = _width, \
.monbit = _monbit \
})
#define CPG_CDDIV0 (0x400)
#define CDDIV0_DIVCTL2 DDIV_PACK(CPG_CDDIV0, 8, 3, 2)
/**
* Definitions of CPG Core Clocks
*
* These include:
* - Clock outputs exported to DT
* - External input clocks
* - Internal CPG clocks
*/
struct cpg_core_clk {
const char *name;
unsigned int id;
unsigned int parent;
unsigned int div;
unsigned int mult;
unsigned int type;
union {
unsigned int conf;
struct ddiv ddiv;
} cfg;
const struct clk_div_table *dtable;
u32 flag;
};
Cheers,
Prabhakar
