[PATCH v4 3/5] clk: ast2600: Add full configs for I3C clocks

[Joel Stanley]

On Wed, 1 Mar 2023 at 00:58, Jeremy Kerr <jk at codeconstruct.com.au> wrote:
>
> Hi Joel,
>
> Thanks for the review. Some replies inline:
>
> > > @@ -15,7 +16,7 @@
> > >
> > >  #include "clk-aspeed.h"
> > >
> > > -#define ASPEED_G6_NUM_CLKS             71
> > > +#define ASPEED_G6_NUM_CLKS             72
> >
> > NUM_CLKS seems dangerous. Should we instead use
> > ARRAY_SIZE(aspeed_g6_gates)?
>
> Yep, that would have saved me some time debugging. That would suit as a
> separate change though, would you like it in the same series?

Doesn't matter much. Perhaps include it at the end, for both the
aspeed drivers? But separately is fine too.

>
> > >         /* USB 2.0 port1 phy 40MHz clock */
> > >         hw = clk_hw_register_fixed_rate(NULL, "usb-phy-40m", NULL,
> > > 0, 40000000);
> > >         aspeed_g6_clk_data->hws[ASPEED_CLK_USBPHY_40M] = hw;
> > > +
> > > +       /* i3c clock: source from apll, divide by 8 */
> > > +       regmap_read(map, ASPEED_G6_CLK_SELECTION5, &val);
> > > +       val &= ~(I3C_CLK_SELECTION | APLL_DIV_SELECTION);
> >
> > Is there any value in registering a mux device here? See the emmc
> > extclk device.
>
> We won't be doing any mux configuration here, so I figure the static
> setup is fine.

ack

>
> > > +       val |= FIELD_PREP(I3C_CLK_SELECTION,
> > > I3C_CLK_SELECT_APLL_DIV);
> > > +       val |= FIELD_PREP(APLL_DIV_SELECTION, APLL_DIV_8);
> > > +       regmap_write(map, ASPEED_G6_CLK_SELECTION5, val);
> >
> > This is a departure in style from the existing code. The existing
> > code did things like this:
> >
> >         regmap_update_bits(map, ASPEED_G6_CLK_SELECTION1, GENMASK(10, 8), BIT(10));
> >
> > Which uses the regmap API instead of FIELD_PREP macros.
>
> Yep, that's much nicer, I'll change. The FIELD_PREP parts are just from
> the initial ASPEED implementation.

Cool.