[4.19.y-cip,08/39] soc: renesas: r8a7795-sysc: Fix power request conflicts
diff mbox series

Message ID 1576668829-59767-9-git-send-email-biju.das@bp.renesas.com
State Accepted
Delegated to: Chris Paterson
Headers show
Series
  • Add RZ/G2N SYSC/RST/Clock/PFC support
Related show

Commit Message

Biju Das Dec. 18, 2019, 11:33 a.m. UTC
From: Geert Uytterhoeven <geert+renesas@glider.be>

commit 0e0c4db2fa0982af0956c0eed0a94e0b412ef2f4 upstream.

Describe the location and contents of the SYSCEXTMASK register on R-Car
H3, to prevent conflicts between internal and external power requests.

This register does not exist on R-Car H3 ES1.x and ES2.x.

Based on a patch in the BSP by Dien Pham <dien.pham.ry@renesas.com>.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
Link: https://lore.kernel.org/r/20190828113618.6672-3-geert+renesas@glider.be
Signed-off-by: Biju Das <biju.das@bp.renesas.com>
---
 drivers/soc/renesas/r8a7795-sysc.c | 32 +++++++++++++++++++++++++++-----
 drivers/soc/renesas/rcar-sysc.h    |  2 +-
 2 files changed, 28 insertions(+), 6 deletions(-)

Comments

Pavel Machek Dec. 18, 2019, 7:32 p.m. UTC | #1
Hi!

> This register does not exist on R-Car H3 ES1.x and ES2.x.
> 
> Based on a patch in the BSP by Dien Pham <dien.pham.ry@renesas.com>.

So you are storing bitfield in the pointer, ok.

> -	{ .soc_id = "r8a7795", .revision = "ES1.*" },
> +#define HAS_A2VC0	BIT(0)		/* Power domain A2VC0 is present */
> +#define NO_EXTMASK	BIT(1)		/* Missing SYSCEXTMASK register */
> +
> +static const struct soc_device_attribute r8a7795_quirks_match[] __initconst = {
> +	{
> +		.soc_id = "r8a7795", .revision = "ES1.*",
> +		.data = (void *)(HAS_A2VC0 | NO_EXTMASK),
> +	}, {
> +		.soc_id = "r8a7795", .revision = "ES2.*",
> +		.data = (void *)(NO_EXTMASK),
> +	},
>  	{ /* sentinel */ }
>  };
>  
>  static int __init r8a7795_sysc_init(void)
>  {
> -	if (!soc_device_match(r8a7795es1))
> +	const struct soc_device_attribute *attr;
> +	u32 quirks = 0;
> +
> +	attr = soc_device_match(r8a7795_quirks_match);
> +	if (attr)
> +		quirks = (uintptr_t)attr->data;

But now you do strange dance with types. I'd understand quirks being
unsigned long (because that's same size as void *). I also could
understand quirks being uintptr_t in the first place (but we normally
use unsigned long for that in the kernel).

But having it as u32, then casting it over uintptr_t is strange.

Best regards,
								Pavel
Biju Das Dec. 19, 2019, 7:43 a.m. UTC | #2
Hi Pavel,

I am adding Dien and Geert to provide feedback on you r questions

Regards,
Bij

> -----Original Message-----
> From: Pavel Machek <pavel@denx.de>
> Sent: Wednesday, December 18, 2019 7:33 PM
> To: Biju Das <biju.das@bp.renesas.com>
> Cc: cip-dev@lists.cip-project.org; Nobuhiro Iwamatsu
> <nobuhiro1.iwamatsu@toshiba.co.jp>; Pavel Machek <pavel@denx.de>;
> Chris Paterson <Chris.Paterson2@renesas.com>; Marian-Cristian Rotariu
> <marian-cristian.rotariu.rb@bp.renesas.com>; Fabrizio Castro
> <fabrizio.castro@bp.renesas.com>
> Subject: Re: [PATCH 4.19.y-cip 08/39] soc: renesas: r8a7795-sysc: Fix power
> request conflicts
> 
> Hi!
> 
> > This register does not exist on R-Car H3 ES1.x and ES2.x.
> >
> > Based on a patch in the BSP by Dien Pham <dien.pham.ry@renesas.com>.
> 
> So you are storing bitfield in the pointer, ok.
> 
> > -	{ .soc_id = "r8a7795", .revision = "ES1.*" },
> > +#define HAS_A2VC0	BIT(0)		/* Power domain A2VC0 is
> present */
> > +#define NO_EXTMASK	BIT(1)		/* Missing SYSCEXTMASK
> register */
> > +
> > +static const struct soc_device_attribute r8a7795_quirks_match[]
> __initconst = {
> > +	{
> > +		.soc_id = "r8a7795", .revision = "ES1.*",
> > +		.data = (void *)(HAS_A2VC0 | NO_EXTMASK),
> > +	}, {
> > +		.soc_id = "r8a7795", .revision = "ES2.*",
> > +		.data = (void *)(NO_EXTMASK),
> > +	},
> >  	{ /* sentinel */ }
> >  };
> >
> >  static int __init r8a7795_sysc_init(void)  {
> > -	if (!soc_device_match(r8a7795es1))
> > +	const struct soc_device_attribute *attr;
> > +	u32 quirks = 0;
> > +
> > +	attr = soc_device_match(r8a7795_quirks_match);
> > +	if (attr)
> > +		quirks = (uintptr_t)attr->data;
> 
> But now you do strange dance with types. I'd understand quirks being
> unsigned long (because that's same size as void *). I also could understand
> quirks being uintptr_t in the first place (but we normally use unsigned long
> for that in the kernel).
> 
> But having it as u32, then casting it over uintptr_t is strange.
> 


Dien, Geert,

Could you please share your thoughts on Pavel's comments?

Regards,
Biju
Geert Uytterhoeven Dec. 19, 2019, 9:44 a.m. UTC | #3
Hi Biju, Pavel,

On Thu, Dec 19, 2019 at 8:43 AM Biju Das <biju.das@bp.renesas.com> wrote:
> > -----Original Message-----
> > From: Pavel Machek <pavel@denx.de>
> > Sent: Wednesday, December 18, 2019 7:33 PM
> > To: Biju Das <biju.das@bp.renesas.com>
> > Cc: cip-dev@lists.cip-project.org; Nobuhiro Iwamatsu
> > <nobuhiro1.iwamatsu@toshiba.co.jp>; Pavel Machek <pavel@denx.de>;
> > Chris Paterson <Chris.Paterson2@renesas.com>; Marian-Cristian Rotariu
> > <marian-cristian.rotariu.rb@bp.renesas.com>; Fabrizio Castro
> > <fabrizio.castro@bp.renesas.com>
> > Subject: Re: [PATCH 4.19.y-cip 08/39] soc: renesas: r8a7795-sysc: Fix power
> > request conflicts
> >
> > > This register does not exist on R-Car H3 ES1.x and ES2.x.
> > >
> > > Based on a patch in the BSP by Dien Pham <dien.pham.ry@renesas.com>.
> >
> > So you are storing bitfield in the pointer, ok.
> >
> > > -   { .soc_id = "r8a7795", .revision = "ES1.*" },
> > > +#define HAS_A2VC0  BIT(0)          /* Power domain A2VC0 is
> > present */
> > > +#define NO_EXTMASK BIT(1)          /* Missing SYSCEXTMASK
> > register */
> > > +
> > > +static const struct soc_device_attribute r8a7795_quirks_match[]
> > __initconst = {
> > > +   {
> > > +           .soc_id = "r8a7795", .revision = "ES1.*",
> > > +           .data = (void *)(HAS_A2VC0 | NO_EXTMASK),
> > > +   }, {
> > > +           .soc_id = "r8a7795", .revision = "ES2.*",
> > > +           .data = (void *)(NO_EXTMASK),
> > > +   },
> > >     { /* sentinel */ }
> > >  };
> > >
> > >  static int __init r8a7795_sysc_init(void)  {
> > > -   if (!soc_device_match(r8a7795es1))
> > > +   const struct soc_device_attribute *attr;
> > > +   u32 quirks = 0;
> > > +
> > > +   attr = soc_device_match(r8a7795_quirks_match);
> > > +   if (attr)
> > > +           quirks = (uintptr_t)attr->data;
> >
> > But now you do strange dance with types. I'd understand quirks being
> > unsigned long (because that's same size as void *). I also could understand
> > quirks being uintptr_t in the first place (but we normally use unsigned long
> > for that in the kernel).
> >
> > But having it as u32, then casting it over uintptr_t is strange.
>
> Dien, Geert,
>
> Could you please share your thoughts on Pavel's comments?

struct soc_device_attribute.data contains an opaque value, just like
e.g. of_device_id.data and platform_device_id.driver_data.
In some structs, the opaque value is a "void *", in other structs it is an
"unsigned long".  But the actual meaning and contents depend on the
driver.  It may be used to store a pointer to a struct with features,
an integer value, or a feature mask.
Conversion between pointer and integer is done by casting to "void *",
or to "uintptr_t" (historically, "unsigned long" was used for this, but
modern code uses "uintptr_t").

In this case, it contains a feature mask, which fits in "u32", which is
"unsigned int", and "(unsigned) int" is still the standard type for any random
variable that doesn't have special requirements.
  - To store the feature mask, a cast to "void *" is needed.
  - To retrieve the feature mask, a cast to "uintptr_t" is needed.
    A cast to "u32" would cause a warning on 64-bit platforms, due to
    the truncation of a 64-bit pointer to a 32-bit integer.

I agree "unsigned long" could have been a suitable type for "quirks"
as well, but I see no reason to make that change when backporting
a patch to stable.

$ git grep "unsigned long quirks" | wc -l
58
$ git grep "unsigned int quirks" | wc -l
26
$ git grep "u32 quirks" | wc -l
46

Hope this helps.

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

Patch
diff mbox series

diff --git a/drivers/soc/renesas/r8a7795-sysc.c b/drivers/soc/renesas/r8a7795-sysc.c
index 7412666..37b05e5 100644
--- a/drivers/soc/renesas/r8a7795-sysc.c
+++ b/drivers/soc/renesas/r8a7795-sysc.c
@@ -8,6 +8,7 @@ 
  * the Free Software Foundation; version 2 of the License.
  */
 
+#include <linux/bits.h>
 #include <linux/bug.h>
 #include <linux/kernel.h>
 #include <linux/sys_soc.h>
@@ -54,25 +55,46 @@  static struct rcar_sysc_area r8a7795_areas[] __initdata = {
 
 
 	/*
-	 * Fixups for R-Car H3 revisions after ES1.x
+	 * Fixups for R-Car H3 revisions
 	 */
 
-static const struct soc_device_attribute r8a7795es1[] __initconst = {
-	{ .soc_id = "r8a7795", .revision = "ES1.*" },
+#define HAS_A2VC0	BIT(0)		/* Power domain A2VC0 is present */
+#define NO_EXTMASK	BIT(1)		/* Missing SYSCEXTMASK register */
+
+static const struct soc_device_attribute r8a7795_quirks_match[] __initconst = {
+	{
+		.soc_id = "r8a7795", .revision = "ES1.*",
+		.data = (void *)(HAS_A2VC0 | NO_EXTMASK),
+	}, {
+		.soc_id = "r8a7795", .revision = "ES2.*",
+		.data = (void *)(NO_EXTMASK),
+	},
 	{ /* sentinel */ }
 };
 
 static int __init r8a7795_sysc_init(void)
 {
-	if (!soc_device_match(r8a7795es1))
+	const struct soc_device_attribute *attr;
+	u32 quirks = 0;
+
+	attr = soc_device_match(r8a7795_quirks_match);
+	if (attr)
+		quirks = (uintptr_t)attr->data;
+
+	if (!(quirks & HAS_A2VC0))
 		rcar_sysc_nullify(r8a7795_areas, ARRAY_SIZE(r8a7795_areas),
 				  R8A7795_PD_A2VC0);
 
+	if (quirks & NO_EXTMASK)
+		r8a7795_sysc_info.extmask_val = 0;
+
 	return 0;
 }
 
-const struct rcar_sysc_info r8a7795_sysc_info __initconst = {
+struct rcar_sysc_info r8a7795_sysc_info __initdata = {
 	.init = r8a7795_sysc_init,
 	.areas = r8a7795_areas,
 	.num_areas = ARRAY_SIZE(r8a7795_areas),
+	.extmask_offs = 0x2f8,
+	.extmask_val = BIT(0),
 };
diff --git a/drivers/soc/renesas/rcar-sysc.h b/drivers/soc/renesas/rcar-sysc.h
index 9b2a869..733b8ec 100644
--- a/drivers/soc/renesas/rcar-sysc.h
+++ b/drivers/soc/renesas/rcar-sysc.h
@@ -62,7 +62,7 @@  extern const struct rcar_sysc_info r8a7790_sysc_info;
 extern const struct rcar_sysc_info r8a7791_sysc_info;
 extern const struct rcar_sysc_info r8a7792_sysc_info;
 extern const struct rcar_sysc_info r8a7794_sysc_info;
-extern const struct rcar_sysc_info r8a7795_sysc_info;
+extern struct rcar_sysc_info r8a7795_sysc_info;
 extern const struct rcar_sysc_info r8a7796_sysc_info;
 extern const struct rcar_sysc_info r8a77965_sysc_info;
 extern const struct rcar_sysc_info r8a77970_sysc_info;