diff mbox series

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

Message ID 20210611134642.24029-3-biju.das.jz@bp.renesas.com (mailing list archive)
State Changes Requested, archived
Headers show
Series None | expand

Commit Message

Biju Das June 11, 2021, 1:46 p.m. UTC
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>
---
 drivers/clk/renesas/r9a07g044-cpg.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Geert Uytterhoeven June 14, 2021, 12:26 p.m. UTC | #1
Hi Biju,

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?

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven June 15, 2021, 8:58 a.m. UTC | #2
Hi Biju,

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.

Perhaps merging the clock binding definitions and initial driver for
v5.14 was a bit premature...
Anyway, we'll have 6 rcs after v5.14-rc1 to get it right ;-)

What do you think?

Gr{oetje,eeting}s,

                        Geert
Laurent Pinchart June 15, 2021, 9:48 a.m. UTC | #3
Hi Geert,

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 ?

> Perhaps merging the clock binding definitions and initial driver for
> v5.14 was a bit premature...
> Anyway, we'll have 6 rcs after v5.14-rc1 to get it right ;-)
> 
> What do you think?
Geert Uytterhoeven June 15, 2021, 9:53 a.m. UTC | #4
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
Laurent Pinchart June 15, 2021, 9:57 a.m. UTC | #5
On Tue, Jun 15, 2021 at 11:53:37AM +0200, Geert Uytterhoeven wrote:
> 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?

If the device has two clock inputs, I'd model the two clocks separately
in the DT bindings. If those two clocks are gated by the same bit in an
SoC, we have two options to model the integration:

- Create a driver that registers different clocks with the same gating
  bit. We'll have two clocks to reference in DT.

- Model both clocks as a single clock in the clock driver, and reference
  that clock twice in DT. This is simpler, but only works if the
  consumer doesn't need to query the clock rate.
Geert Uytterhoeven June 15, 2021, 10:24 a.m. UTC | #6
Hi Laurent,

On Tue, Jun 15, 2021 at 11:57 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tue, Jun 15, 2021 at 11:53:37AM +0200, Geert Uytterhoeven wrote:
> > 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?
>
> If the device has two clock inputs, I'd model the two clocks separately
> in the DT bindings. If those two clocks are gated by the same bit in an
> SoC, we have two options to model the integration:
>
> - Create a driver that registers different clocks with the same gating
>   bit. We'll have two clocks to reference in DT.

OK, that's what I suggested.

> - Model both clocks as a single clock in the clock driver, and reference
>   that clock twice in DT. This is simpler, but only works if the
>   consumer doesn't need to query the clock rate.

Modelling them as a single clock is how the current RZ/G2L clock
driver would implement it. But why bother referencing it twice in DT?
renesas,ether*.yaml (assuming the Ethernet block is compatible)
documents a single clock only (ignoring optional refclk), and the driver
doesn't care about the clock rate.

Gr{oetje,eeting}s,

                        Geert
Biju Das June 16, 2021, 10:12 a.m. UTC | #7
Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH 2/6] drivers: clk: renesas: r9a07g044-cpg: Add USB
> clocks
> 
> Hi Biju,
> 
> 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.
> 
> Perhaps merging the clock binding definitions and initial driver for
> v5.14 was a bit premature...
> Anyway, we'll have 6 rcs after v5.14-rc1 to get it right ;-)
> 
> What do you think?

I am ok with this approach for USB, SDHI and Ethernet.

Regards,
Biju
Biju Das June 16, 2021, 10:16 a.m. UTC | #8
Hi Geert and Laurent,

Thanks for the feedback.

I have gone through the below discussion and agreeing for two clock entries in dt bindings
and implement a gate for controlling this clocks.

Regards,
Biju

> Subject: Re: [PATCH 2/6] drivers: clk: renesas: r9a07g044-cpg: Add USB
> clocks
> 
> Hi Laurent,
> 
> On Tue, Jun 15, 2021 at 11:57 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > On Tue, Jun 15, 2021 at 11:53:37AM +0200, Geert Uytterhoeven wrote:
> > > 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?
> >
> > If the device has two clock inputs, I'd model the two clocks
> > separately in the DT bindings. If those two clocks are gated by the
> > same bit in an SoC, we have two options to model the integration:
> >
> > - Create a driver that registers different clocks with the same gating
> >   bit. We'll have two clocks to reference in DT.
> 
> OK, that's what I suggested.
> 
> > - Model both clocks as a single clock in the clock driver, and reference
> >   that clock twice in DT. This is simpler, but only works if the
> >   consumer doesn't need to query the clock rate.
> 
> Modelling them as a single clock is how the current RZ/G2L clock driver
> would implement it. But why bother referencing it twice in DT?
> renesas,ether*.yaml (assuming the Ethernet block is compatible) documents
> a single clock only (ignoring optional refclk), and the driver doesn't
> care about the clock rate.
> 
> 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
Laurent Pinchart June 16, 2021, 11:33 a.m. UTC | #9
Hi Geert,

On Tue, Jun 15, 2021 at 12:24:52PM +0200, Geert Uytterhoeven wrote:
> On Tue, Jun 15, 2021 at 11:57 AM Laurent Pinchart wrote:
> > On Tue, Jun 15, 2021 at 11:53:37AM +0200, Geert Uytterhoeven wrote:
> > > On Tue, Jun 15, 2021 at 11:49 AM Laurent Pinchart 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 wrote:
> > > > > > On Fri, Jun 11, 2021 at 3:46 PM Biju Das 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?
> >
> > If the device has two clock inputs, I'd model the two clocks separately
> > in the DT bindings. If those two clocks are gated by the same bit in an
> > SoC, we have two options to model the integration:
> >
> > - Create a driver that registers different clocks with the same gating
> >   bit. We'll have two clocks to reference in DT.
> 
> OK, that's what I suggested.
> 
> > - Model both clocks as a single clock in the clock driver, and reference
> >   that clock twice in DT. This is simpler, but only works if the
> >   consumer doesn't need to query the clock rate.
> 
> Modelling them as a single clock is how the current RZ/G2L clock
> driver would implement it. But why bother referencing it twice in DT?
> renesas,ether*.yaml (assuming the Ethernet block is compatible)
> documents a single clock only (ignoring optional refclk), and the driver
> doesn't care about the clock rate.

The idea here is that if the device has two clock inputs, its driver
could handle clocks without knowing that in some SoCs they are handled
by a single gate (or rather two gates with the same control signal).
diff mbox series

Patch

diff --git a/drivers/clk/renesas/r9a07g044-cpg.c b/drivers/clk/renesas/r9a07g044-cpg.c
index 04123908511c..2d2bc78b84a2 100644
--- 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)),