Hi Geert-san,
Thank you for your review!
> From: Geert Uytterhoeven, Sent: Tuesday, September 8, 2020 6:24 PM
>
> Hi Shimoda-san,
>
> Thanks for your patch!
>
> On Mon, Sep 7, 2020 at 11:20 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote:
> > Add all Clock Pulse Generator Core Clock Outputs for the Renesas R-Car
> > V3U (R8A779A0) SoC.
>
> So obviously s/61/a0/ in the patch subject ;-)
Oops. You're correct :)
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
>
> > --- /dev/null
> > +++ b/include/dt-bindings/clock/r8a779a0-cpg-mssr.h
> > @@ -0,0 +1,63 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
>
> GPL-2.0-only?
I'll fix it.
> > +/*
> > + * Copyright (C) 2020 Renesas Electronics Corp.
> > + */
> > +#ifndef __DT_BINDINGS_CLOCK_R8A779A0_CPG_MSSR_H__
> > +#define __DT_BINDINGS_CLOCK_R8A779A0_CPG_MSSR_H__
> > +
> > +#include <dt-bindings/clock/renesas-cpg-mssr.h>
> > +
> > +/* r8a779A0 CPG Core Clocks */
> > +#define R8A779A0_CLK_Z0 0
> > +#define R8A779A0_CLK_ZX 1
> > +#define R8A779A0_CLK_Z1 2
> > +#define R8A779A0_CLK_ZR 3
> > +#define R8A779A0_CLK_ZS 4
> > +#define R8A779A0_CLK_ZT 5
> > +#define R8A779A0_CLK_ZTR 6
> > +#define R8A779A0_CLK_S1 7
> > +#define R8A779A0_CLK_S3 8
>
> On other SoCs, we didn't include S1 and S3, as they are internal clocks.
I got it. I'll drop S1 and S3.
> > +#define R8A779A0_CLK_S1D1 9
> > +#define R8A779A0_CLK_S1D2 10
> > +#define R8A779A0_CLK_S1D4 11
> > +#define R8A779A0_CLK_S1D8 12
> > +#define R8A779A0_CLK_S1D12 13
>
> No S1D8 nor S1D12 in the table in Section 8.1.4 ("List of Clock Out")?
As you mentioned other email thread, it is listed as a child of S1.
And, I checked an internal material and then S1D8 and S1D12 exist.
> > +#define R8A779A0_CLK_S2D1 14
> > +#define R8A779A0_CLK_S2D2 15
> > +#define R8A779A0_CLK_S2D4 16
>
> Given the table has no S2 parent clock, and there are no other
> references to any of the S2D* clocks, I wonder if they exist at all?
I also checked the material, and then, V3U doesn't have any S2*.
So, I'll drop these S2*.
> As this file will become stable DT ABI, it would be good to have a
> definitive answer.
I got it.
> > +#define R8A779A0_CLK_S3D1 17
> > +#define R8A779A0_CLK_S3D2 18
> > +#define R8A779A0_CLK_S3D4 19
> > +#define R8A779A0_CLK_LB 20
> > +#define R8A779A0_CLK_CP 21
> > +#define R8A779A0_CLK_CL 22
> > +#define R8A779A0_CLK_CL16MCK 23
> > +#define R8A779A0_CLK_ZB30 24
> > +#define R8A779A0_CLK_ZB30D2 25
> > +#define R8A779A0_CLK_ZB30D4 26
> > +#define R8A779A0_CLK_ZB31 27
> > +#define R8A779A0_CLK_ZB31D2 28
> > +#define R8A779A0_CLK_ZB31D4 29
> > +#define R8A779A0_CLK_SD0H 30
> > +#define R8A779A0_CLK_SD0 31
> > +#define R8A779A0_CLK_RPC 32
> > +#define R8A779A0_CLK_RPCD2 33
> > +#define R8A779A0_CLK_MSO 34
> > +#define R8A779A0_CLK_CANFD 35
> > +#define R8A779A0_CLK_CSI0 36
> > +#define R8A779A0_CLK_FRAY 37
> > +#define R8A779A0_CLK_POST 38
> > +#define R8A779A0_CLK_POST2 39
> > +#define R8A779A0_CLK_POST3 40
>
> Do we need the POST clocks (BTW, POST4 is missing)?
> On other SoCs, we didn't include them.
I see. So, I'll drop POST*.
> > +#define R8A779A0_CLK_DSI 41
> > +#define R8A779A0_CLK_VIP 42
> > +#define R8A779A0_CLK_ADGH 43
> > +#define R8A779A0_CLK_CNNDSP 44
> > +#define R8A779A0_CLK_ICU 45
> > +#define R8A779A0_CLK_ICUD2 46
> > +#define R8A779A0_CLK_VCBUS 47
> > +#define R8A779A0_CLK_CBFUSA 48
> > +#define R8A779A0_CLK_RCLK 49
> > +#define R8A779A0_CLK_OSCCLK 50
>
> On other SoCs, we called them <SOC>_CLK_R and <SOC>_CLK_OSC.
I'll fix these definitions.
Best regards,
Yoshihiro Shimoda
