Re: [PATCH 1/5] soc: renesas: Add symbols for R-Car Gen3 register offsets

2018-02-16 Thread Geert Uytterhoeven 
Hi Simon,

On Thu, Feb 15, 2018 at 4:02 PM, Simon Horman  wrote:
> On Wed, Feb 14, 2018 at 11:25:19AM +0100, Geert Uytterhoeven wrote:
>> On Mon, Feb 12, 2018 at 4:44 PM, Simon Horman
>>  wrote:
>> > Add symbols for Gen3 register offsets.
>> > These may be used to improve readability of users of these offsets.
>> >
>> > This does not introduce any functional change.
>> >
>> > Signed-off-by: Simon Horman 
>>
>> Thanks for your patch!
>>
>> > --- a/drivers/soc/renesas/rcar-sysc.h
>> > +++ b/drivers/soc/renesas/rcar-sysc.h
>> > @@ -24,6 +24,25 @@
>> >  #define PD_CPU_NOCRPD_CPU | PD_NO_CR /* CPU area lacks CR (R-Car 
>> > Gen2/3) */
>> >  #define PD_ALWAYS_ON   PD_NO_CR  /* Always-on area */
>> >
>> > +/*
>> > + * R-Car Gen3 register offsets
>> > + */
>> > +
>> > +#define RCAR_GEN3_SYSCSR  0
>>
>> Please drop this. The "always-on" domains use 0 to indicate "no register
>> block", not as a reference to the SYSCSR register.
>
> Ok, understood.
>
> Could I tempt you with RCAR_SYSC_ALWAYS_ON ?

IMHO that obfuscates what's going on, while 0 is a clear indicator.

>> > +#define RCAR_GEN3_PWRSR0  0x80
>> > +#define RCAR_GEN3_PWRSR2  0x100
>> > +#define RCAR_GEN3_PWRSR3  0x140
>> > +#define RCAR_GEN3_PWRSR4  0x180
>> > +#define RCAR_GEN3_PWRSR5  0x1c0
>> > +#define RCAR_GEN3_PWRSR6  0x200
>> > +#define RCAR_GEN3_PWRSR7  0x240
>> > +#define RCAR_GEN3_PWRSR8  0x340
>> > +#define RCAR_GEN3_PWRSR9  0x380
>> > +#define RCAR_GEN3_PWRSR10 0x3c0
>> > +#define RCAR_GEN3_PWRSR11 0x400
>> > +#define RCAR_GEN3_PWRSR12 0x2c0
>> > +#define RCAR_GEN3_PWRSR13 0x300
>> > +#define RCAR_GEN3_PWRSR14 0x280
>>
>> What about
>>
>> #define RCAR_GEN3_PWRSR(n) (0x80 + (n) * 0x40)
>>
>> instead?
>> Bummer, they have a hole between 0x240 and 0x340, which they filled later :-(
>> So I don't know how stable the PWRSRx indices are...
>
> Ok, would you rather use the approach I provided or wait?

Your approach is fine, due to the non-contiguous numbering.
Although I still have mixed feelings, as I usually look at the actual
base offsets,
not register abbreviations, when adding/reviewing support for new SoCs.
I may be biased, though.

Jacopo/Sergei: You were the last persons adding a new SYSC driver.
Do you have any opinion?

Thanks!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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

Re: [PATCH 1/5] soc: renesas: Add symbols for R-Car Gen3 register offsets

2018-02-15 Thread Simon Horman 
On Wed, Feb 14, 2018 at 11:25:19AM +0100, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Mon, Feb 12, 2018 at 4:44 PM, Simon Horman
>  wrote:
> > Add symbols for Gen3 register offsets.
> > These may be used to improve readability of users of these offsets.
> >
> > This does not introduce any functional change.
> >
> > Signed-off-by: Simon Horman 
> 
> Thanks for your patch!
> 
> > --- a/drivers/soc/renesas/rcar-sysc.h
> > +++ b/drivers/soc/renesas/rcar-sysc.h
> > @@ -24,6 +24,25 @@
> >  #define PD_CPU_NOCRPD_CPU | PD_NO_CR /* CPU area lacks CR (R-Car 
> > Gen2/3) */
> >  #define PD_ALWAYS_ON   PD_NO_CR  /* Always-on area */
> >
> > +/*
> > + * R-Car Gen3 register offsets
> > + */
> > +
> > +#define RCAR_GEN3_SYSCSR  0
> 
> Please drop this. The "always-on" domains use 0 to indicate "no register
> block", not as a reference to the SYSCSR register.

Ok, understood.

Could I tempt you with RCAR_SYSC_ALWAYS_ON ?

> > +#define RCAR_GEN3_PWRSR0  0x80
> > +#define RCAR_GEN3_PWRSR2  0x100
> > +#define RCAR_GEN3_PWRSR3  0x140
> > +#define RCAR_GEN3_PWRSR4  0x180
> > +#define RCAR_GEN3_PWRSR5  0x1c0
> > +#define RCAR_GEN3_PWRSR6  0x200
> > +#define RCAR_GEN3_PWRSR7  0x240
> > +#define RCAR_GEN3_PWRSR8  0x340
> > +#define RCAR_GEN3_PWRSR9  0x380
> > +#define RCAR_GEN3_PWRSR10 0x3c0
> > +#define RCAR_GEN3_PWRSR11 0x400
> > +#define RCAR_GEN3_PWRSR12 0x2c0
> > +#define RCAR_GEN3_PWRSR13 0x300
> > +#define RCAR_GEN3_PWRSR14 0x280
> 
> What about
> 
> #define RCAR_GEN3_PWRSR(n) (0x80 + (n) * 0x40)
> 
> instead?
> Bummer, they have a hole between 0x240 and 0x340, which they filled later :-(
> So I don't know how stable the PWRSRx indices are...

Ok, would you rather use the approach I provided or wait?

> 
> For R-Car H1 and Gen2 you can use the following, though:
> 
> #define RCAR_H1_PWRSR(n) (0x40 + (n) * 0x40)
> #define RCAR_GEN2_PWRSR(n) (0x40 + (n) * 0x40)

Thanks, got it.

Re: [PATCH 1/5] soc: renesas: Add symbols for R-Car Gen3 register offsets

2018-02-14 Thread Geert Uytterhoeven 
Hi Simon,

On Mon, Feb 12, 2018 at 4:44 PM, Simon Horman
 wrote:
> Add symbols for Gen3 register offsets.
> These may be used to improve readability of users of these offsets.
>
> This does not introduce any functional change.
>
> Signed-off-by: Simon Horman 

Thanks for your patch!

> --- a/drivers/soc/renesas/rcar-sysc.h
> +++ b/drivers/soc/renesas/rcar-sysc.h
> @@ -24,6 +24,25 @@
>  #define PD_CPU_NOCRPD_CPU | PD_NO_CR /* CPU area lacks CR (R-Car Gen2/3) 
> */
>  #define PD_ALWAYS_ON   PD_NO_CR  /* Always-on area */
>
> +/*
> + * R-Car Gen3 register offsets
> + */
> +
> +#define RCAR_GEN3_SYSCSR  0

Please drop this. The "always-on" domains use 0 to indicate "no register
block", not as a reference to the SYSCSR register.

> +#define RCAR_GEN3_PWRSR0  0x80
> +#define RCAR_GEN3_PWRSR2  0x100
> +#define RCAR_GEN3_PWRSR3  0x140
> +#define RCAR_GEN3_PWRSR4  0x180
> +#define RCAR_GEN3_PWRSR5  0x1c0
> +#define RCAR_GEN3_PWRSR6  0x200
> +#define RCAR_GEN3_PWRSR7  0x240
> +#define RCAR_GEN3_PWRSR8  0x340
> +#define RCAR_GEN3_PWRSR9  0x380
> +#define RCAR_GEN3_PWRSR10 0x3c0
> +#define RCAR_GEN3_PWRSR11 0x400
> +#define RCAR_GEN3_PWRSR12 0x2c0
> +#define RCAR_GEN3_PWRSR13 0x300
> +#define RCAR_GEN3_PWRSR14 0x280

What about

#define RCAR_GEN3_PWRSR(n) (0x80 + (n) * 0x40)

instead?
Bummer, they have a hole between 0x240 and 0x340, which they filled later :-(
So I don't know how stable the PWRSRx indices are...

For R-Car H1 and Gen2 you can use the following, though:

#define RCAR_H1_PWRSR(n) (0x40 + (n) * 0x40)
#define RCAR_GEN2_PWRSR(n) (0x40 + (n) * 0x40)

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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

[PATCH 1/5] soc: renesas: Add symbols for R-Car Gen3 register offsets

2018-02-12 Thread Simon Horman 
Add symbols for Gen3 register offsets.
These may be used to improve readability of users of these offsets.

This does not introduce any functional change.

Signed-off-by: Simon Horman 
---
 drivers/soc/renesas/rcar-sysc.h | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/drivers/soc/renesas/rcar-sysc.h b/drivers/soc/renesas/rcar-sysc.h
index 9d9daf9eb91b..c6041e29fbcb 100644
--- a/drivers/soc/renesas/rcar-sysc.h
+++ b/drivers/soc/renesas/rcar-sysc.h
@@ -24,6 +24,25 @@
 #define PD_CPU_NOCRPD_CPU | PD_NO_CR /* CPU area lacks CR (R-Car Gen2/3) */
 #define PD_ALWAYS_ON   PD_NO_CR  /* Always-on area */
 
+/*
+ * R-Car Gen3 register offsets
+ */
+
+#define RCAR_GEN3_SYSCSR  0
+#define RCAR_GEN3_PWRSR0  0x80
+#define RCAR_GEN3_PWRSR2  0x100
+#define RCAR_GEN3_PWRSR3  0x140
+#define RCAR_GEN3_PWRSR4  0x180
+#define RCAR_GEN3_PWRSR5  0x1c0
+#define RCAR_GEN3_PWRSR6  0x200
+#define RCAR_GEN3_PWRSR7  0x240
+#define RCAR_GEN3_PWRSR8  0x340
+#define RCAR_GEN3_PWRSR9  0x380
+#define RCAR_GEN3_PWRSR10 0x3c0
+#define RCAR_GEN3_PWRSR11 0x400
+#define RCAR_GEN3_PWRSR12 0x2c0
+#define RCAR_GEN3_PWRSR13 0x300
+#define RCAR_GEN3_PWRSR14 0x280
 
 /*
  * Description of a Power Area
-- 
2.11.0