[PATCH v2] memory: renesas-rpc-if: Fix PHYCNT.STRTIM setting

[PATCH v2] memory: renesas-rpc-if: Fix PHYCNT.STRTIM setting

[Wolfram Sang]

From: Cong Dang <cong.dang.xn@renesas.com>

According to the datasheets, the Strobe Timing Adjustment bit (STRTIM)
setting is different on R-Car SoCs, i.e.

R-Car H3 ES1.*  : STRTIM[2:0] is set to 0x0
R-Car M3 ES1.*  : STRTIM[2:0] is set to 0x6
other R-Car Gen3: STRTIM[2:0] is set to 0x7
other R-Car Gen4: STRTIM[3:0] is set to 0xf

To fix this issue, a DT match data was added to specify the setting
for special use cases.

Signed-off-by: Cong Dang <cong.dang.xn@renesas.com>
Signed-off-by: Hai Pham  <hai.pham.ud@renesas.com>
[wsa: rebased, restructured a little, added Gen4 support]
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Change since V1:

* use proper mask when updating STRTIM bits (thanks, Geert!)

look for 'RPCIF_PHYCNT_STRTIM', there is the change. Rest is the same.

 drivers/memory/renesas-rpc-if.c | 63 ++++++++++++++++++++++++++-------
 include/memory/renesas-rpc-if.h |  6 ++++
 2 files changed, 56 insertions(+), 13 deletions(-)

diff --git a/drivers/memory/renesas-rpc-if.c b/drivers/memory/renesas-rpc-if.c
index c36b407851ff..845b535a5350 100644
--- a/drivers/memory/renesas-rpc-if.c
+++ b/drivers/memory/renesas-rpc-if.c
@@ -7,6 +7,7 @@
  * Copyright (C) 2019-2020 Cogent Embedded, Inc.
  */
 
+#include <linux/bitops.h>
 #include <linux/clk.h>
 #include <linux/io.h>
 #include <linux/module.h>
@@ -15,6 +16,7 @@
 #include <linux/of_device.h>
 #include <linux/regmap.h>
 #include <linux/reset.h>
+#include <linux/sys_soc.h>
 
 #include <memory/renesas-rpc-if.h>
 
@@ -163,6 +165,36 @@ static const struct regmap_access_table rpcif_volatile_table = {
 	.n_yes_ranges	= ARRAY_SIZE(rpcif_volatile_ranges),
 };
 
+static const struct rpcif_info rpcif_info_r8a7795_es1 = {
+	.type = RPCIF_RCAR_GEN3,
+	.strtim = 0,
+};
+
+static const struct rpcif_info rpcif_info_r8a7796_es1 = {
+	.type = RPCIF_RCAR_GEN3,
+	.strtim = 6,
+};
+
+static const struct rpcif_info rpcif_info_gen3 = {
+	.type = RPCIF_RCAR_GEN3,
+	.strtim = 7,
+};
+
+static const struct rpcif_info rpcif_info_rz_g2l = {
+	.type = RPCIF_RZ_G2L,
+	.strtim = 7,
+};
+
+static const struct rpcif_info rpcif_info_gen4 = {
+	.type = RPCIF_RCAR_GEN4,
+	.strtim = 15,
+};
+
+static const struct soc_device_attribute rpcif_info_match[]  = {
+	{ .soc_id = "r8a7795", .revision = "ES1.*", .data = &rpcif_info_r8a7795_es1 },
+	{ .soc_id = "r8a7796", .revision = "ES1.*", .data = &rpcif_info_r8a7796_es1 },
+	{ /* Sentinel. */ }
+};
 
 /*
  * Custom accessor functions to ensure SM[RW]DR[01] are always accessed with
@@ -256,6 +288,8 @@ static const struct regmap_config rpcif_regmap_config = {
 int rpcif_sw_init(struct rpcif *rpc, struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
+	const struct soc_device_attribute *attr;
+	const struct rpcif_info *info;
 	struct resource *res;
 
 	rpc->dev = dev;
@@ -276,9 +310,14 @@ int rpcif_sw_init(struct rpcif *rpc, struct device *dev)
 	rpc->dirmap = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(rpc->dirmap))
 		return PTR_ERR(rpc->dirmap);
-	rpc->size = resource_size(res);
 
-	rpc->type = (uintptr_t)of_device_get_match_data(dev);
+	info = of_device_get_match_data(dev);
+	attr = soc_device_match(rpcif_info_match);
+	if (attr)
+		info = attr->data;
+
+	rpc->info = info;
+	rpc->size = resource_size(res);
 	rpc->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
 
 	return PTR_ERR_OR_ZERO(rpc->rstc);
@@ -305,7 +344,7 @@ int rpcif_hw_init(struct rpcif *rpc, bool hyperflash)
 
 	pm_runtime_get_sync(rpc->dev);
 
-	if (rpc->type == RPCIF_RZ_G2L) {
+	if (rpc->info->type == RPCIF_RZ_G2L) {
 		int ret;
 
 		ret = reset_control_reset(rpc->rstc);
@@ -321,12 +360,10 @@ int rpcif_hw_init(struct rpcif *rpc, bool hyperflash)
 	/* DMA Transfer is not supported */
 	regmap_update_bits(rpc->regmap, RPCIF_PHYCNT, RPCIF_PHYCNT_HS, 0);
 
-	if (rpc->type == RPCIF_RCAR_GEN3)
-		regmap_update_bits(rpc->regmap, RPCIF_PHYCNT,
-				   RPCIF_PHYCNT_STRTIM(7), RPCIF_PHYCNT_STRTIM(7));
-	else if (rpc->type == RPCIF_RCAR_GEN4)
-		regmap_update_bits(rpc->regmap, RPCIF_PHYCNT,
-				   RPCIF_PHYCNT_STRTIM(15), RPCIF_PHYCNT_STRTIM(15));
+	regmap_update_bits(rpc->regmap, RPCIF_PHYCNT,
+			   /* create mask with all affected bits set */
+			   RPCIF_PHYCNT_STRTIM(BIT(fls(rpc->info->strtim)) - 1),
+			   RPCIF_PHYCNT_STRTIM(rpc->info->strtim));
 
 	regmap_update_bits(rpc->regmap, RPCIF_PHYOFFSET1, RPCIF_PHYOFFSET1_DDRTMG(3),
 			   RPCIF_PHYOFFSET1_DDRTMG(3));
@@ -337,7 +374,7 @@ int rpcif_hw_init(struct rpcif *rpc, bool hyperflash)
 		regmap_update_bits(rpc->regmap, RPCIF_PHYINT,
 				   RPCIF_PHYINT_WPVAL, 0);
 
-	if (rpc->type == RPCIF_RZ_G2L)
+	if (rpc->info->type == RPCIF_RZ_G2L)
 		regmap_update_bits(rpc->regmap, RPCIF_CMNCR,
 				   RPCIF_CMNCR_MOIIO(3) | RPCIF_CMNCR_IOFV(3) |
 				   RPCIF_CMNCR_BSZ(3),
@@ -720,9 +757,9 @@ static int rpcif_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id rpcif_of_match[] = {
-	{ .compatible = "renesas,rcar-gen3-rpc-if", .data = (void *)RPCIF_RCAR_GEN3 },
-	{ .compatible = "renesas,rcar-gen4-rpc-if", .data = (void *)RPCIF_RCAR_GEN4 },
-	{ .compatible = "renesas,rzg2l-rpc-if", .data = (void *)RPCIF_RZ_G2L },
+	{ .compatible = "renesas,rcar-gen3-rpc-if", .data = &rpcif_info_gen3 },
+	{ .compatible = "renesas,rcar-gen4-rpc-if", .data = &rpcif_info_gen4 },
+	{ .compatible = "renesas,rzg2l-rpc-if", .data = &rpcif_info_rz_g2l },
 	{},
 };
 MODULE_DEVICE_TABLE(of, rpcif_of_match);
diff --git a/include/memory/renesas-rpc-if.h b/include/memory/renesas-rpc-if.h
index 862eff613dc7..75da785a18ff 100644
--- a/include/memory/renesas-rpc-if.h
+++ b/include/memory/renesas-rpc-if.h
@@ -63,6 +63,11 @@ enum rpcif_type {
 	RPCIF_RZ_G2L,
 };
 
+struct rpcif_info {
+	enum rpcif_type type;
+	u8 strtim;
+};
+
 struct rpcif {
 	struct device *dev;
 	void __iomem *base;
@@ -71,6 +76,7 @@ struct rpcif {
 	struct reset_control *rstc;
 	size_t size;
 	enum rpcif_type type;
+	const struct rpcif_info *info;
 	enum rpcif_data_dir dir;
 	u8 bus_size;
 	u8 xfer_size;
-- 
2.30.2

Re: [PATCH v2] memory: renesas-rpc-if: Fix PHYCNT.STRTIM setting

[Krzysztof Kozlowski]

On 18/01/2023 11:46, Wolfram Sang wrote:
> From: Cong Dang <cong.dang.xn@renesas.com>
> 
> According to the datasheets, the Strobe Timing Adjustment bit (STRTIM)
> setting is different on R-Car SoCs, i.e.
> 
> R-Car H3 ES1.*  : STRTIM[2:0] is set to 0x0
> R-Car M3 ES1.*  : STRTIM[2:0] is set to 0x6
> other R-Car Gen3: STRTIM[2:0] is set to 0x7
> other R-Car Gen4: STRTIM[3:0] is set to 0xf
> 
> To fix this issue, a DT match data was added to specify the setting
> for special use cases.
> 
> Signed-off-by: Cong Dang <cong.dang.xn@renesas.com>
> Signed-off-by: Hai Pham  <hai.pham.ud@renesas.com>
> [wsa: rebased, restructured a little, added Gen4 support]
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Change since V1:
> 
> * use proper mask when updating STRTIM bits (thanks, Geert!)
> 
> look for 'RPCIF_PHYCNT_STRTIM', there is the change. Rest is the same.
> 
>  drivers/memory/renesas-rpc-if.c | 63 ++++++++++++++++++++++++++-------
>  include/memory/renesas-rpc-if.h |  6 ++++
>  2 files changed, 56 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/memory/renesas-rpc-if.c b/drivers/memory/renesas-rpc-if.c
> index c36b407851ff..845b535a5350 100644
> --- a/drivers/memory/renesas-rpc-if.c
> +++ b/drivers/memory/renesas-rpc-if.c
> @@ -7,6 +7,7 @@
>   * Copyright (C) 2019-2020 Cogent Embedded, Inc.
>   */
>  
> +#include <linux/bitops.h>
>  #include <linux/clk.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> @@ -15,6 +16,7 @@
>  #include <linux/of_device.h>
>  #include <linux/regmap.h>
>  #include <linux/reset.h>
> +#include <linux/sys_soc.h>
>  
>  #include <memory/renesas-rpc-if.h>
>  
> @@ -163,6 +165,36 @@ static const struct regmap_access_table rpcif_volatile_table = {
>  	.n_yes_ranges	= ARRAY_SIZE(rpcif_volatile_ranges),
>  };
>  
> +static const struct rpcif_info rpcif_info_r8a7795_es1 = {
> +	.type = RPCIF_RCAR_GEN3,
> +	.strtim = 0,
> +};
> +
> +static const struct rpcif_info rpcif_info_r8a7796_es1 = {
> +	.type = RPCIF_RCAR_GEN3,
> +	.strtim = 6,
> +};
> +
> +static const struct rpcif_info rpcif_info_gen3 = {
> +	.type = RPCIF_RCAR_GEN3,
> +	.strtim = 7,
> +};
> +
> +static const struct rpcif_info rpcif_info_rz_g2l = {
> +	.type = RPCIF_RZ_G2L,
> +	.strtim = 7,
> +};
> +
> +static const struct rpcif_info rpcif_info_gen4 = {
> +	.type = RPCIF_RCAR_GEN4,
> +	.strtim = 15,
> +};
> +
> +static const struct soc_device_attribute rpcif_info_match[]  = {
> +	{ .soc_id = "r8a7795", .revision = "ES1.*", .data = &rpcif_info_r8a7795_es1 },
> +	{ .soc_id = "r8a7796", .revision = "ES1.*", .data = &rpcif_info_r8a7796_es1 },

Why do you need soc match? Can't this be inferred from device
compatible? Maybe the device compatible is not specific enough? Devices
should not be interested in which SoC they are running - it does not
matter for them, because the device difference is in the device itself,
not in the SoC (different SoCs come with different devices).

Best regards,
Krzysztof

Re: [PATCH v2] memory: renesas-rpc-if: Fix PHYCNT.STRTIM setting

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 927 bytes --]

Hi Krzysztof,

> > +static const struct soc_device_attribute rpcif_info_match[]  = {
> > +	{ .soc_id = "r8a7795", .revision = "ES1.*", .data = &rpcif_info_r8a7795_es1 },
> > +	{ .soc_id = "r8a7796", .revision = "ES1.*", .data = &rpcif_info_r8a7796_es1 },
> 
> Why do you need soc match? Can't this be inferred from device
> compatible? Maybe the device compatible is not specific enough? Devices
> should not be interested in which SoC they are running - it does not
> matter for them, because the device difference is in the device itself,
> not in the SoC (different SoCs come with different devices).

I need it because of ".revision". This only applies to "ES1.*",
there are "ES2.*" and "ES3.*" around which have the same SoC number.
Also, there is usually no version numbering for the IP core. We need to
use this scheme in a number of other places already, sadly.

Thanks for reviewing,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] memory: renesas-rpc-if: Fix PHYCNT.STRTIM setting

[Krzysztof Kozlowski]

On 23/01/2023 07:58, Wolfram Sang wrote:
> Hi Krzysztof,
> 
>>> +static const struct soc_device_attribute rpcif_info_match[]  = {
>>> +	{ .soc_id = "r8a7795", .revision = "ES1.*", .data = &rpcif_info_r8a7795_es1 },
>>> +	{ .soc_id = "r8a7796", .revision = "ES1.*", .data = &rpcif_info_r8a7796_es1 },
>>
>> Why do you need soc match? Can't this be inferred from device
>> compatible? Maybe the device compatible is not specific enough? Devices
>> should not be interested in which SoC they are running - it does not
>> matter for them, because the device difference is in the device itself,
>> not in the SoC (different SoCs come with different devices).
> 
> I need it because of ".revision". This only applies to "ES1.*",
> there are "ES2.*" and "ES3.*" around which have the same SoC number.
> Also, there is usually no version numbering for the IP core. We need to
> use this scheme in a number of other places already, sadly.

I did not get whether this is runtime characteristics or it can be
customized with compatible (just you did not do it)?

Best regards,
Krzysztof

Re: [PATCH v2] memory: renesas-rpc-if: Fix PHYCNT.STRTIM setting

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1413 bytes --]

Hi Krzysztof,

> > I need it because of ".revision". This only applies to "ES1.*",
> > there are "ES2.*" and "ES3.*" around which have the same SoC number.
> > Also, there is usually no version numbering for the IP core. We need to
> > use this scheme in a number of other places already, sadly.
> 
> I did not get whether this is runtime characteristics or it can be
> customized with compatible (just you did not do it)?

We have compatibles per SoC, i.e. "r8a7795". We don't have compatibles
for ES versions, i.e. no "r8a7795-es10" or "r8a7795-es20".

The latter would not be practical. We can't know in advance how many ES
revisions there will be, so we can't prepare DTs accordingly. Updating
later would be also difficult because we are usually not notified if
there is a new ES version. Only if there are problems with it. And which
board is available with which ES version is chaotic^2.

Also, if we update DTs later, old DTBs would not work with newer kernels
(requiring a later added compaible for a new ES version). This all still
ignores that it would be a churn to update for every ES version of every
SoC. We have quite many to support. That's why we use soc_device_match()
for ES versions in many places alreday. It was never a problem so far.

That's my reasoning, probably Geert has something to add. He maintains
the Renesas DT files.

All the best,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] memory: renesas-rpc-if: Fix PHYCNT.STRTIM setting

[Geert Uytterhoeven]

Hi Wolfram,

Thanks for reminding me I still had to chime in on this ;-)

On Thu, Jan 26, 2023 at 10:03 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > > I need it because of ".revision". This only applies to "ES1.*",
> > > there are "ES2.*" and "ES3.*" around which have the same SoC number.
> > > Also, there is usually no version numbering for the IP core. We need to
> > > use this scheme in a number of other places already, sadly.
> >
> > I did not get whether this is runtime characteristics or it can be
> > customized with compatible (just you did not do it)?
>
> We have compatibles per SoC, i.e. "r8a7795". We don't have compatibles
> for ES versions, i.e. no "r8a7795-es10" or "r8a7795-es20".
>
> The latter would not be practical. We can't know in advance how many ES
> revisions there will be, so we can't prepare DTs accordingly. Updating
> later would be also difficult because we are usually not notified if
> there is a new ES version. Only if there are problems with it. And which
> board is available with which ES version is chaotic^2.
>
> Also, if we update DTs later, old DTBs would not work with newer kernels
> (requiring a later added compaible for a new ES version). This all still
> ignores that it would be a churn to update for every ES version of every
> SoC. We have quite many to support. That's why we use soc_device_match()
> for ES versions in many places alreday. It was never a problem so far.
>
> That's my reasoning, probably Geert has something to add. He maintains
> the Renesas DT files.

Exactly. We only use soc_device_match() to distinguish where we do not
have a compatible value to do so.  As we have SoC-specific compatible
values for about everything, this means we usually use soc_device_match()
only to handle quirks on specific revisions of SoCs.

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

Re: [PATCH v2] memory: renesas-rpc-if: Fix PHYCNT.STRTIM setting

[Krzysztof Kozlowski]

On 26/01/2023 10:27, Geert Uytterhoeven wrote:
>>> I did not get whether this is runtime characteristics or it can be
>>> customized with compatible (just you did not do it)?
>>
>> We have compatibles per SoC, i.e. "r8a7795". We don't have compatibles
>> for ES versions, i.e. no "r8a7795-es10" or "r8a7795-es20".
>>
>> The latter would not be practical. We can't know in advance how many ES
>> revisions there will be, so we can't prepare DTs accordingly. Updating
>> later would be also difficult because we are usually not notified if
>> there is a new ES version. Only if there are problems with it. And which
>> board is available with which ES version is chaotic^2.
>>
>> Also, if we update DTs later, old DTBs would not work with newer kernels
>> (requiring a later added compaible for a new ES version). This all still
>> ignores that it would be a churn to update for every ES version of every
>> SoC. We have quite many to support. That's why we use soc_device_match()
>> for ES versions in many places alreday. It was never a problem so far.
>>
>> That's my reasoning, probably Geert has something to add. He maintains
>> the Renesas DT files.
> 
> Exactly. We only use soc_device_match() to distinguish where we do not
> have a compatible value to do so.  As we have SoC-specific compatible
> values for about everything, this means we usually use soc_device_match()
> only to handle quirks on specific revisions of SoCs.
> 

OK, thank you both for clarifying.

Best regards,
Krzysztof

Re: [PATCH v2] memory: renesas-rpc-if: Fix PHYCNT.STRTIM setting

[Geert Uytterhoeven]

"Hi Wolfram,

On Wed, Jan 18, 2023 at 12:38 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> From: Cong Dang <cong.dang.xn@renesas.com>
>
> According to the datasheets, the Strobe Timing Adjustment bit (STRTIM)
> setting is different on R-Car SoCs, i.e.
>
> R-Car H3 ES1.*  : STRTIM[2:0] is set to 0x0
> R-Car M3 ES1.*  : STRTIM[2:0] is set to 0x6
> other R-Car Gen3: STRTIM[2:0] is set to 0x7
> other R-Car Gen4: STRTIM[3:0] is set to 0xf
>
> To fix this issue, a DT match data was added to specify the setting
> for special use cases.
>
> Signed-off-by: Cong Dang <cong.dang.xn@renesas.com>
> Signed-off-by: Hai Pham  <hai.pham.ud@renesas.com>
> [wsa: rebased, restructured a little, added Gen4 support]
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> Change since V1:
>
> * use proper mask when updating STRTIM bits (thanks, Geert!)

Thanks for the update!

> --- a/drivers/memory/renesas-rpc-if.c
> +++ b/drivers/memory/renesas-rpc-if.c

> @@ -163,6 +165,36 @@ static const struct regmap_access_table rpcif_volatile_table = {
>         .n_yes_ranges   = ARRAY_SIZE(rpcif_volatile_ranges),
>  };
>
> +static const struct rpcif_info rpcif_info_r8a7795_es1 = {
> +       .type = RPCIF_RCAR_GEN3,
> +       .strtim = 0,
> +};
> +
> +static const struct rpcif_info rpcif_info_r8a7796_es1 = {
> +       .type = RPCIF_RCAR_GEN3,
> +       .strtim = 6,
> +};
> +
> +static const struct rpcif_info rpcif_info_gen3 = {
> +       .type = RPCIF_RCAR_GEN3,
> +       .strtim = 7,
> +};
> +
> +static const struct rpcif_info rpcif_info_rz_g2l = {
> +       .type = RPCIF_RZ_G2L,
> +       .strtim = 7,
> +};
> +
> +static const struct rpcif_info rpcif_info_gen4 = {
> +       .type = RPCIF_RCAR_GEN4,
> +       .strtim = 15,
> +};
> +
> +static const struct soc_device_attribute rpcif_info_match[]  = {
> +       { .soc_id = "r8a7795", .revision = "ES1.*", .data = &rpcif_info_r8a7795_es1 },
> +       { .soc_id = "r8a7796", .revision = "ES1.*", .data = &rpcif_info_r8a7796_es1 },

As we do have a separate compatible value for R-Car M3-W+ aka R-Car M3-W ES3.0
("renesas,r8a77961-rpc-if"), and there is no R-Car M3-W ES2.x (see the PRR
screwup handling in renesas_soc_init()), you can just match against
"renesas,r8a7796-rpc-if instead.

> +       { /* Sentinel. */ }
> +};
>
>  /*
>   * Custom accessor functions to ensure SM[RW]DR[01] are always accessed with

> @@ -321,12 +360,10 @@ int rpcif_hw_init(struct rpcif *rpc, bool hyperflash)
>         /* DMA Transfer is not supported */
>         regmap_update_bits(rpc->regmap, RPCIF_PHYCNT, RPCIF_PHYCNT_HS, 0);
>
> -       if (rpc->type == RPCIF_RCAR_GEN3)
> -               regmap_update_bits(rpc->regmap, RPCIF_PHYCNT,
> -                                  RPCIF_PHYCNT_STRTIM(7), RPCIF_PHYCNT_STRTIM(7));:q

> -       else if (rpc->type == RPCIF_RCAR_GEN4)
> -               regmap_update_bits(rpc->regmap, RPCIF_PHYCNT,
> -                                  RPCIF_PHYCNT_STRTIM(15), RPCIF_PHYCNT_STRTIM(15));
> +       regmap_update_bits(rpc->regmap, RPCIF_PHYCNT,
> +                          /* create mask with all affected bits set */
> +                          RPCIF_PHYCNT_STRTIM(BIT(fls(rpc->info->strtim)) - 1),

fls(0) = 0, and BIT(-1) is undefined, so this won't work for R-Car
H3 ES1.x.  So I'm afraid you cannot handle this without storing the
actual mask ;-)

However, that issue will be moot as soon as we drop upstream support
for R-Car H3 ES1.x.  I cannot test RPC on R-Car H3 ES1.x anyway, as
the RPC is locked by TF/A, and the firmware cannot be upgraded easily
due to lack of upstream TF/A support for R-Car H3 ES1.x

> +                          RPCIF_PHYCNT_STRTIM(rpc->info->strtim));
>
>         regmap_update_bits(rpc->regmap, RPCIF_PHYOFFSET1, RPCIF_PHYOFFSET1_DDRTMG(3),
>                            RPCIF_PHYOFFSET1_DDRTMG(3));

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

Re: [PATCH v2] memory: renesas-rpc-if: Fix PHYCNT.STRTIM setting

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1366 bytes --]

Hi Geert,

> > +static const struct soc_device_attribute rpcif_info_match[]  = {
> > +       { .soc_id = "r8a7795", .revision = "ES1.*", .data = &rpcif_info_r8a7795_es1 },
> > +       { .soc_id = "r8a7796", .revision = "ES1.*", .data = &rpcif_info_r8a7796_es1 },
> 
> As we do have a separate compatible value for R-Car M3-W+ aka R-Car M3-W ES3.0
> ("renesas,r8a77961-rpc-if"), and there is no R-Car M3-W ES2.x (see the PRR
> screwup handling in renesas_soc_init()), you can just match against
> "renesas,r8a7796-rpc-if instead.

Right, I missed that! This is awesome news because with us dropping H3
ES1 support, this means we can drop all the soc_device_match() handling
now and use compatibles only. There, everybody happy :D

> > +       regmap_update_bits(rpc->regmap, RPCIF_PHYCNT,
> > +                          /* create mask with all affected bits set */
> > +                          RPCIF_PHYCNT_STRTIM(BIT(fls(rpc->info->strtim)) - 1),
> 
> fls(0) = 0, and BIT(-1) is undefined, so this won't work for R-Car
> H3 ES1.x.  So I'm afraid you cannot handle this without storing the
> actual mask ;-)

You misread the parens, it is: BIT(0) - 1 = 0

I actually wrote a program to print out the calculations to make sure I
got it right. But yeah, ES1 is obsolete now.

I'll send V3 later today.

Thanks everyone,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] memory: renesas-rpc-if: Fix PHYCNT.STRTIM setting

[Geert Uytterhoeven]

Hi Wolfram,

On Thu, Jan 26, 2023 at 1:48 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > > +       regmap_update_bits(rpc->regmap, RPCIF_PHYCNT,
> > > +                          /* create mask with all affected bits set */
> > > +                          RPCIF_PHYCNT_STRTIM(BIT(fls(rpc->info->strtim)) - 1),
> >
> > fls(0) = 0, and BIT(-1) is undefined, so this won't work for R-Car
> > H3 ES1.x.  So I'm afraid you cannot handle this without storing the
> > actual mask ;-)
>
> You misread the parens, it is: BIT(0) - 1 = 0

Doh... You're right.

Still, it won't clear the bits on H3 ES1.x.
And when changing strtim to a value smaller than 4, it may not work
as expected, as it doesn't clear the upper bits.

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

Re: [PATCH v2] memory: renesas-rpc-if: Fix PHYCNT.STRTIM setting

[Lad, Prabhakar]

Hi Wolfram,

Thank you for the patch.

I am yet to test this patch on g2l (I'll test v3 as you plan to send it today).

On Wed, Jan 18, 2023 at 10:46 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> From: Cong Dang <cong.dang.xn@renesas.com>
>
> According to the datasheets, the Strobe Timing Adjustment bit (STRTIM)
> setting is different on R-Car SoCs, i.e.
>
> R-Car H3 ES1.*  : STRTIM[2:0] is set to 0x0
> R-Car M3 ES1.*  : STRTIM[2:0] is set to 0x6
> other R-Car Gen3: STRTIM[2:0] is set to 0x7
> other R-Car Gen4: STRTIM[3:0] is set to 0xf
>
> To fix this issue, a DT match data was added to specify the setting
> for special use cases.
>
> Signed-off-by: Cong Dang <cong.dang.xn@renesas.com>
> Signed-off-by: Hai Pham  <hai.pham.ud@renesas.com>
> [wsa: rebased, restructured a little, added Gen4 support]
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
<snip>
> +struct rpcif_info {
> +       enum rpcif_type type;
> +       u8 strtim;
> +};
> +
>  struct rpcif {
>         struct device *dev;
>         void __iomem *base;
> @@ -71,6 +76,7 @@ struct rpcif {
>         struct reset_control *rstc;
>         size_t size;
>         enum rpcif_type type;
I think now you can get rid of this member?

Cheers,
Prabhakar

Re: [PATCH v2] memory: renesas-rpc-if: Fix PHYCNT.STRTIM setting

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 229 bytes --]

Hi Prabhakar,

> I am yet to test this patch on g2l (I'll test v3 as you plan to send it today).

Awesome, thank you!

> I think now you can get rid of this member?

Good point! I'll remove it in v3.

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]