Hi Prabhakar,
On Mon, 7 Apr 2025 at 21:16, Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> Add SoC identification for the RZ/V2N SoC using the System Controller
> (SYS) block.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
Thanks for your patch!
> --- /dev/null
> +++ b/drivers/soc/renesas/r9a09g056-sys.c
> @@ -0,0 +1,107 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * RZ/V2N System controller (SYS) driver
> + *
> + * Copyright (C) 2025 Renesas Electronics Corp.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/string.h>
> +
> +#include "rz-sysc.h"
> +
> +/* Register Offsets */
> +#define SYS_LSI_MODE 0x300
> +#define SYS_LSI_MODE_SEC_EN BIT(16)
> +/*
> + * BOOTPLLCA[1:0]
> + * [0,0] => 1.1GHZ
> + * [0,1] => 1.5GHZ
> + * [1,0] => 1.6GHZ
> + * [1,1] => 1.7GHZ
> + */
> +#define SYS_LSI_MODE_STAT_BOOTPLLCA55 GENMASK(12, 11)
> +#define SYS_LSI_MODE_CA55_1_7GHZ 0x3
> +
> +#define SYS_LSI_PRR 0x308
> +#define SYS_LSI_PRR_GPU_DIS BIT(0)
> +#define SYS_LSI_PRR_ISP_DIS BIT(4)
> +
> +#define SYS_RZV2N_FEATURE_G31 BIT(0)
> +#define SYS_RZV2N_FEATURE_C55 BIT(1)
> +#define SYS_RZV2N_FEATURE_SEC BIT(2)
> +
> +static void rzv2n_sys_print_id(struct device *dev,
> + void __iomem *sysc_base,
> + struct soc_device_attribute *soc_dev_attr)
> +{
> + unsigned int part_number;
> + char features[75] = "";
> + u32 prr_val, mode_val;
> + u8 feature_flags;
> +
> + prr_val = readl(sysc_base + SYS_LSI_PRR);
> + mode_val = readl(sysc_base + SYS_LSI_MODE);
> +
> + /* Check GPU, ISP and Cryptographic configuration */
> + feature_flags = !(prr_val & SYS_LSI_PRR_GPU_DIS) ? SYS_RZV2N_FEATURE_G31 : 0;
> + feature_flags |= !(prr_val & SYS_LSI_PRR_ISP_DIS) ? SYS_RZV2N_FEATURE_C55 : 0;
> + feature_flags |= (mode_val & SYS_LSI_MODE_SEC_EN) ? SYS_RZV2N_FEATURE_SEC : 0;
> +
> + part_number = 41;
> + if (feature_flags & SYS_RZV2N_FEATURE_G31)
> + part_number++;
> + if (feature_flags & SYS_RZV2N_FEATURE_C55)
> + part_number += 2;
> + if (feature_flags & SYS_RZV2N_FEATURE_SEC)
> + part_number += 4;
The above construct can be simplified to
part_number = 41 + feature_flags;
> + if (feature_flags) {
> + unsigned int features_len = sizeof(features);
> +
> + strscpy(features, "with ");
> + if (feature_flags & SYS_RZV2N_FEATURE_G31)
> + strlcat(features, "GE3D (Mali-G31)", features_len);
> +
> + if (feature_flags == (SYS_RZV2N_FEATURE_G31 |
> + SYS_RZV2N_FEATURE_C55 |
> + SYS_RZV2N_FEATURE_SEC))
> + strlcat(features, ", ", features_len);
> + else if ((feature_flags & SYS_RZV2N_FEATURE_G31) &&
> + (feature_flags & (SYS_RZV2N_FEATURE_C55 | SYS_RZV2N_FEATURE_SEC)))
> + strlcat(features, " and ", features_len);
> +
> + if (feature_flags & SYS_RZV2N_FEATURE_SEC)
> + strlcat(features, "Cryptographic engine", features_len);
> +
> + if ((feature_flags & SYS_RZV2N_FEATURE_SEC) &&
> + (feature_flags & SYS_RZV2N_FEATURE_C55))
> + strlcat(features, " and ", features_len);
> +
> + if (feature_flags & SYS_RZV2N_FEATURE_C55)
> + strlcat(features, "ISP (Mali-C55)", features_len);
> + }
The above looks overly complicated to me. What about handling it
like on RZ/V2H? I agree having 3x "with" doesn't look nice, but you
could just drop all "with"s.
> + dev_info(dev, "Detected Renesas %s %sn%d Rev %s %s\n", soc_dev_attr->family,
> + soc_dev_attr->soc_id, part_number, soc_dev_attr->revision, features);
This prints a trailing space if features is empty.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
