| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CA+V-a8s0RbgEB2kHDtv35jOtSNw2ThMB_GEgX1SLdOdiRtiPfQ@mail.gmail.com>
Date: Mon, 14 Apr 2025 11:21:39 +0100
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
Cc: Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Linus Walleij <linus.walleij@...aro.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jirislaby@...nel.org>, Magnus Damm <magnus.damm@...il.com>,
linux-renesas-soc@...r.kernel.org, linux-clk@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-gpio@...r.kernel.org, linux-serial@...r.kernel.org,
Biju Das <biju.das.jz@...renesas.com>,
Fabrizio Castro <fabrizio.castro.jz@...esas.com>,
Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH v2 04/12] soc: renesas: sysc: Add SoC identification for
RZ/V2N SoC
Hi Geert,
Thank you for the review.
On Thu, Apr 10, 2025 at 10:28 AM Geert Uytterhoeven
<geert@...ux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Mon, 7 Apr 2025 at 21:16, Prabhakar <prabhakar.csengg@...il.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> >
> > Add SoC identification for the RZ/V2N SoC using the System Controller
> > (SYS) block.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
>
> 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;
>
Agreed.
> > + 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.
>
Ok, I will switch like below:
part_number = 41 + feature_flags;
dev_info(dev, "Detected Renesas %s %sn%d Rev %s%s%s%s%s\n",
soc_dev_attr->family,
soc_dev_attr->soc_id, part_number,
soc_dev_attr->revision, feature_flags ?
" with" : "", feature_flags & SYS_RZV2N_FEATURE_G31 ? "
GE3D (Mali-G31)" : "",
feature_flags & SYS_RZV2N_FEATURE_SEC ? " Cryptographic
engine" : "",
feature_flags & SYS_RZV2N_FEATURE_C55 ? " ISP (Mali-C55)" : "");
> > + 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.
>
Agreed.
Cheers,
Prabhakar
Powered by blists - more mailing lists