Re: [PATCH 2/6] drivers: clk: renesas: r9a07g044-cpg: Add USB clocks

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Laurent,

On Tue, Jun 15, 2021 at 11:49 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tue, Jun 15, 2021 at 10:58:57AM +0200, Geert Uytterhoeven wrote:
> > On Mon, Jun 14, 2021 at 2:26 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Fri, Jun 11, 2021 at 3:46 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > Add clock entries for USB{0,1}.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > Thanks for your patch!
> > >
> > > > --- a/drivers/clk/renesas/r9a07g044-cpg.c
> > > > +++ b/drivers/clk/renesas/r9a07g044-cpg.c
> > > > @@ -88,6 +88,12 @@ static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
> > > >         DEF_MOD("dmac",         R9A07G044_CLK_DMAC,
> > > >                                 R9A07G044_CLK_P1,
> > > >                                 0x52c, (BIT(0) | BIT(1)), (BIT(0) | BIT(1))),
> > > > +       DEF_MOD("usb0",         R9A07G044_CLK_USB0,
> > > > +                               R9A07G044_CLK_P1,
> > > > +                               0x578, (BIT(0) | BIT(2) | BIT(3)), (BIT(0) | BIT(2) | BIT(3))),
> > > > +       DEF_MOD("usb1",         R9A07G044_CLK_USB1,
> > > > +                               R9A07G044_CLK_P1,
> > > > +                               0x578, (BIT(1) | BIT(3)), (BIT(1) | BIT(3))),
> > > >         DEF_MOD("scif0",        R9A07G044_CLK_SCIF0,
> > > >                                 R9A07G044_CLK_P0,
> > > >                                 0x584, BIT(0), BIT(0)),
> > >
> > > While the above matches the datasheet, I see a problem with the
> > > implementation. As BIT(3) of the CPG_{CLKON,CLKMON,RST}_USB is shared by
> > > the two USB2.0 channels, disabling USB_PCLK or asserting USB_PRESETN
> > > will affect both channels.  So it looks like you need special handling
> > > to make sure that doesn't happen while the other channel is in use.
> > >
> > > Or am I missing something?
> >
> > I'm getting the impression we do have to model the individual bits
> > as separate clocks (and resets).  That would solve the problem with
> > the shared USB_PCLK, as the clock framework will take care of keeping
> > it enabled when at least one channel is in use.
> >
> > Besides USB, SDHI has 4 clock bits, which we definitely don't want
> > to control together, as the card detect clock must not be stopped
> > while suspended.
> > However, the exception to the rule is Ethernet: each channel has
> > 2 clocks, but only a single bit to control, so this needs a custom
> > single-gate-for-dual-clock driver.
>
> Does it ? Can't the same clock be referenced twice in DT ?

Yes, you can reference the same clock twice. But what's the point?
If they're two different clocks, DT should reference two different
clocks.  But the single bit should correspond to the ORed value of
the two clock enable states.

Or do you mean something different?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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