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@xxxxxxxxxxx>
> 
> 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@xxxxxxxxxxx>
> Signed-off-by: Hai Pham  <hai.pham.ud@xxxxxxxxxxx>
> [wsa: rebased, restructured a little, added Gen4 support]
> Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> ---
> 
> 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