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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGTfZH1DzC9odJVDfYCYw4+Ph5_1CjmrpqR_NUFh1SsVVVLM0g@mail.gmail.com>
Date: Sun, 7 Sep 2025 01:09:53 +0900
From: Chanwoo Choi <chanwoo@...nel.org>
To: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
Cc: Chanwoo Choi <cw00.choi@...sung.com>, MyungJoo Ham <myungjoo.ham@...sung.com>, 
	Kyungmin Park <kyungmin.park@...sung.com>, Heiko Stuebner <heiko@...ech.de>, 
	Sascha Hauer <s.hauer@...gutronix.de>, Jonathan Cameron <Jonathan.Cameron@...wei.com>, 
	Sebastian Reichel <sebastian.reichel@...labora.com>, kernel@...labora.com, 
	linux-pm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] PM / devfreq: rockchip-dfi: add support for LPDDR5

Hi,

I'm sorry for late reply.

Looks good to me. But, this patch contains the change of following header.
If there are ACK about change in include/soc/rockchip, I'll merge this series.

include/soc/rockchip/rk3588_grf.h    |  8 +++-
include/soc/rockchip/rockchip_grf.h  |  1 +

On Fri, May 30, 2025 at 10:39 PM Nicolas Frattaroli
<nicolas.frattaroli@...labora.com> wrote:
>
> The Rockchip RK3588 SoC can also support LPDDR5 memory. This type of
> memory needs some special case handling in the rockchip-dfi driver.
>
> Add support for it in rockchip-dfi, as well as the needed GRF register
> definitions.
>
> This has been tested as returning both the right cycle count and
> bandwidth on a LPDDR5 board where the CKR bit is 1. I couldn't test
> whether the values are correct on a system where CKR is 0, as I'm not
> savvy enough with the Rockchip tooling to know whether this can be set
> in the DDR init blob.
>
> Downstream has some special case handling for a hardware version where
> not just the control bits differ, but also the register. Since I don't
> know whether that hardware version is in any production silicon, it's
> left unimplemented for now, with an error message urging users to report
> if they have such a system.
>
> There is a slight change of behaviour for non-LPDDR5 systems: instead of
> writing 0 as the control flags to the control register and pretending
> everything is alright if the memory type is unknown, we now explicitly
> return an error.
>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
> ---
>  drivers/devfreq/event/rockchip-dfi.c | 84 ++++++++++++++++++++++++++++--------
>  include/soc/rockchip/rk3588_grf.h    |  8 +++-
>  include/soc/rockchip/rockchip_grf.h  |  1 +
>  3 files changed, 73 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 54effb63519653d20b40eed88681330983399a77..5a2c9badcc64c552303c2f55c52e5420dec5ffc1 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -34,15 +34,18 @@
>
>  /* DDRMON_CTRL */
>  #define DDRMON_CTRL    0x04
> +#define DDRMON_CTRL_LPDDR5             BIT(6)
>  #define DDRMON_CTRL_DDR4               BIT(5)
>  #define DDRMON_CTRL_LPDDR4             BIT(4)
>  #define DDRMON_CTRL_HARDWARE_EN                BIT(3)
>  #define DDRMON_CTRL_LPDDR23            BIT(2)
>  #define DDRMON_CTRL_SOFTWARE_EN                BIT(1)
>  #define DDRMON_CTRL_TIMER_CNT_EN       BIT(0)
> -#define DDRMON_CTRL_DDR_TYPE_MASK      (DDRMON_CTRL_DDR4 | \
> +#define DDRMON_CTRL_DDR_TYPE_MASK      (DDRMON_CTRL_LPDDR5 | \
> +                                        DDRMON_CTRL_DDR4 | \
>                                          DDRMON_CTRL_LPDDR4 | \
>                                          DDRMON_CTRL_LPDDR23)
> +#define DDRMON_CTRL_LP5_BANK_MODE_MASK GENMASK(8, 7)
>
>  #define DDRMON_CH0_WR_NUM              0x20
>  #define DDRMON_CH0_RD_NUM              0x24
> @@ -116,13 +119,60 @@ struct rockchip_dfi {
>         int buswidth[DMC_MAX_CHANNELS];
>         int ddrmon_stride;
>         bool ddrmon_ctrl_single;
> +       u32 lp5_bank_mode;
> +       bool lp5_ckr;   /* true if in 4:1 command-to-data clock ratio mode */
>         unsigned int count_multiplier;  /* number of data clocks per count */
>  };
>
> +static int rockchip_dfi_ddrtype_to_ctrl(struct rockchip_dfi *dfi, u32 *ctrl,
> +                                       u32 *mask)
> +{
> +       u32 ddrmon_ver;
> +
> +       *mask = DDRMON_CTRL_DDR_TYPE_MASK;
> +
> +       switch (dfi->ddr_type) {
> +       case ROCKCHIP_DDRTYPE_LPDDR2:
> +       case ROCKCHIP_DDRTYPE_LPDDR3:
> +               *ctrl = DDRMON_CTRL_LPDDR23;
> +               break;
> +       case ROCKCHIP_DDRTYPE_LPDDR4:
> +       case ROCKCHIP_DDRTYPE_LPDDR4X:
> +               *ctrl = DDRMON_CTRL_LPDDR4;
> +               break;
> +       case ROCKCHIP_DDRTYPE_LPDDR5:
> +               ddrmon_ver = readl_relaxed(dfi->regs);
> +               if (ddrmon_ver < 0x40) {
> +                       *ctrl = DDRMON_CTRL_LPDDR5 | dfi->lp5_bank_mode;
> +                       *mask |= DDRMON_CTRL_LP5_BANK_MODE_MASK;
> +                       break;
> +               }
> +
> +               /*
> +                * As it is unknown whether the unpleasant special case
> +                * behaviour used by the vendor kernel is needed for any
> +                * shipping hardware, ask users to report if they have
> +                * some of that hardware.
> +                */
> +               dev_err(&dfi->edev->dev,
> +                       "unsupported DDRMON version 0x%04X, please let linux-rockchip know!\n",
> +                       ddrmon_ver);
> +               return -EOPNOTSUPP;
> +       default:
> +               dev_err(&dfi->edev->dev, "unsupported memory type 0x%X\n",
> +                       dfi->ddr_type);
> +               return -EOPNOTSUPP;
> +       }
> +
> +       return 0;
> +}
> +
>  static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
>  {
>         void __iomem *dfi_regs = dfi->regs;
>         int i, ret = 0;
> +       u32 ctrl;
> +       u32 ctrl_mask;
>
>         mutex_lock(&dfi->mutex);
>
> @@ -136,8 +186,11 @@ static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
>                 goto out;
>         }
>
> +       ret = rockchip_dfi_ddrtype_to_ctrl(dfi, &ctrl, &ctrl_mask);
> +       if (ret)
> +               goto out;
> +
>         for (i = 0; i < dfi->max_channels; i++) {
> -               u32 ctrl = 0;
>
>                 if (!(dfi->channel_mask & BIT(i)))
>                         continue;
> @@ -147,21 +200,7 @@ static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
>                                DDRMON_CTRL_SOFTWARE_EN | DDRMON_CTRL_HARDWARE_EN),
>                                dfi_regs + i * dfi->ddrmon_stride + DDRMON_CTRL);
>
> -               /* set ddr type to dfi */
> -               switch (dfi->ddr_type) {
> -               case ROCKCHIP_DDRTYPE_LPDDR2:
> -               case ROCKCHIP_DDRTYPE_LPDDR3:
> -                       ctrl = DDRMON_CTRL_LPDDR23;
> -                       break;
> -               case ROCKCHIP_DDRTYPE_LPDDR4:
> -               case ROCKCHIP_DDRTYPE_LPDDR4X:
> -                       ctrl = DDRMON_CTRL_LPDDR4;
> -                       break;
> -               default:
> -                       break;
> -               }
> -
> -               writel_relaxed(HIWORD_UPDATE(ctrl, DDRMON_CTRL_DDR_TYPE_MASK),
> +               writel_relaxed(HIWORD_UPDATE(ctrl, ctrl_mask),
>                                dfi_regs + i * dfi->ddrmon_stride + DDRMON_CTRL);
>
>                 /* enable count, use software mode */
> @@ -652,6 +691,7 @@ static int rockchip_ddr_perf_init(struct rockchip_dfi *dfi)
>                 break;
>         case ROCKCHIP_DDRTYPE_LPDDR4:
>         case ROCKCHIP_DDRTYPE_LPDDR4X:
> +       case ROCKCHIP_DDRTYPE_LPDDR5:
>                 dfi->burst_len = 16;
>                 break;
>         }
> @@ -730,7 +770,7 @@ static int rk3568_dfi_init(struct rockchip_dfi *dfi)
>  static int rk3588_dfi_init(struct rockchip_dfi *dfi)
>  {
>         struct regmap *regmap_pmu = dfi->regmap_pmu;
> -       u32 reg2, reg3, reg4;
> +       u32 reg2, reg3, reg4, reg6;
>
>         regmap_read(regmap_pmu, RK3588_PMUGRF_OS_REG2, &reg2);
>         regmap_read(regmap_pmu, RK3588_PMUGRF_OS_REG3, &reg3);
> @@ -757,6 +797,14 @@ static int rk3588_dfi_init(struct rockchip_dfi *dfi)
>         dfi->ddrmon_stride = 0x4000;
>         dfi->count_multiplier = 2;
>
> +       if (dfi->ddr_type == ROCKCHIP_DDRTYPE_LPDDR5) {
> +               regmap_read(regmap_pmu, RK3588_PMUGRF_OS_REG6, &reg6);
> +               dfi->lp5_bank_mode = FIELD_GET(RK3588_PMUGRF_OS_REG6_LP5_BANK_MODE, reg6) << 7;
> +               dfi->lp5_ckr = FIELD_GET(RK3588_PMUGRF_OS_REG6_LP5_CKR, reg6);
> +               if (dfi->lp5_ckr)
> +                       dfi->count_multiplier *= 2;
> +       }
> +
>         return 0;
>  };
>
> diff --git a/include/soc/rockchip/rk3588_grf.h b/include/soc/rockchip/rk3588_grf.h
> index 630b35a550640e57f1b5a50dfbe362653a7cbcc1..02a7b2432d9942e15a77424c44fefec189faaa33 100644
> --- a/include/soc/rockchip/rk3588_grf.h
> +++ b/include/soc/rockchip/rk3588_grf.h
> @@ -12,7 +12,11 @@
>  #define RK3588_PMUGRF_OS_REG3_DRAMTYPE_INFO_V3         GENMASK(13, 12)
>  #define RK3588_PMUGRF_OS_REG3_SYSREG_VERSION           GENMASK(31, 28)
>
> -#define RK3588_PMUGRF_OS_REG4           0x210
> -#define RK3588_PMUGRF_OS_REG5           0x214
> +#define RK3588_PMUGRF_OS_REG4                          0x210
> +#define RK3588_PMUGRF_OS_REG5                          0x214
> +#define RK3588_PMUGRF_OS_REG6                          0x218
> +#define RK3588_PMUGRF_OS_REG6_LP5_BANK_MODE            GENMASK(2, 1)
> +/* Whether the LPDDR5 is in 2:1 (= 0) or 4:1 (= 1) CKR a.k.a. DQS mode */
> +#define RK3588_PMUGRF_OS_REG6_LP5_CKR                  BIT(0)
>
>  #endif /* __SOC_RK3588_GRF_H */
> diff --git a/include/soc/rockchip/rockchip_grf.h b/include/soc/rockchip/rockchip_grf.h
> index e46fd72aea8d1f649768a3269b85176dacceef0e..41c7bb26fd5387df85e5b58186b67bf74706f360 100644
> --- a/include/soc/rockchip/rockchip_grf.h
> +++ b/include/soc/rockchip/rockchip_grf.h
> @@ -13,6 +13,7 @@ enum {
>         ROCKCHIP_DDRTYPE_LPDDR3 = 6,
>         ROCKCHIP_DDRTYPE_LPDDR4 = 7,
>         ROCKCHIP_DDRTYPE_LPDDR4X = 8,
> +       ROCKCHIP_DDRTYPE_LPDDR5 = 9,
>  };
>
>  #endif /* __SOC_ROCKCHIP_GRF_H */
>
> --
> 2.49.0
>
>


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ