Re: [PATCH] rockchip: sdram: fix LPDDR5 bank info for sys_reg version 3

[Jonas Karlman]

Hi,

On 2024-01-22 08:46, Kever Yang wrote:
> From: YouMin Chen <c...@rock-chips.com>
> 
> This patch add support for additional bank info used by LPDDR5.
> 
> Signed-off-by: YouMin Chen <c...@rock-chips.com>
> Signed-off-by: Kever Yang <kever.y...@rock-chips.com>
> ---
> 
>  arch/arm/mach-rockchip/sdram.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm/mach-rockchip/sdram.c b/arch/arm/mach-rockchip/sdram.c
> index 99ecbdc3412..d65c48bf515 100644
> --- a/arch/arm/mach-rockchip/sdram.c
> +++ b/arch/arm/mach-rockchip/sdram.c
> @@ -110,6 +110,13 @@ size_t rockchip_sdram_size(phys_addr_t reg)
>                         SYS_REG_COL_MASK);
>               cs1_col = cs0_col;
>               bk = 3 - ((sys_reg2 >> SYS_REG_BK_SHIFT(ch)) & SYS_REG_BK_MASK);
> +             /*
> +              * SYS_REG_BK(Version 3):
> +              * 1) Except LPDDR5 0:8bank(bk=3), 1:4bank(bk=2)
> +              * 2) LPDDR5 0:8bank(bk=3), 1:16bank(bk=4)
> +              */
> +             if (version == 3 && dram_type == LPDDR5 && bk == 2)
> +                     bk = 4;

The version == 3 test is not really needed because dram_type cannot be
assigned to LPDDR5 (9) since the dram_type high bits is only read for
version >= 3.

Also not sure I fully like the if bk==2 then bk=4 override code style
here because the code comment do not fully match the code flow. I had to
stop and think twice on why the test was for bk==2 when it is bk bit = 1
that should result in bk=4.

Maybe we can make the code closer match the comment with something like
the following:


@@ -109,7 +109,14 @@ size_t rockchip_sdram_size(phys_addr_t reg)
                cs0_col = 9 + (sys_reg2 >> SYS_REG_COL_SHIFT(ch) &
                          SYS_REG_COL_MASK);
                cs1_col = cs0_col;
-               bk = 3 - ((sys_reg2 >> SYS_REG_BK_SHIFT(ch)) & SYS_REG_BK_MASK);
+               if (dram_type == LPDDR5)
+                       /* LPDDR5: 0:8bank(bk=3), 1:16bank(bk=4) */
+                       bk = 3 + ((sys_reg2 >> SYS_REG_BK_SHIFT(ch)) &
+                                 SYS_REG_BK_MASK);
+               else
+                       /* Other: 0:8bank(bk=3), 1:4bank(bk=2) */
+                       bk = 3 - ((sys_reg2 >> SYS_REG_BK_SHIFT(ch)) &
+                                 SYS_REG_BK_MASK);
                if (version >= 2) {
                        cs1_col = 9 + (sys_reg3 >> SYS_REG_CS1_COL_SHIFT(ch) &
                                  SYS_REG_CS1_COL_MASK);


or possible use of a conditional operator:


@@ -109,7 +109,13 @@ size_t rockchip_sdram_size(phys_addr_t reg)
                cs0_col = 9 + (sys_reg2 >> SYS_REG_COL_SHIFT(ch) &
                          SYS_REG_COL_MASK);
                cs1_col = cs0_col;
-               bk = 3 - ((sys_reg2 >> SYS_REG_BK_SHIFT(ch)) & SYS_REG_BK_MASK);
+               /*
+                * SYS_REG_BK:
+                * LPDDR5: 0:8bank(bk=3), 1:16bank(bk=4)
+                * Other: 0:8bank(bk=3), 1:4bank(bk=2)
+                */
+               bk = (sys_reg2 >> SYS_REG_BK_SHIFT(ch)) & SYS_REG_BK_MASK;
+               bk = (dram_type == LPDDR5) ? 3 + bk : 3 - bk;
                if (version >= 2) {
                        cs1_col = 9 + (sys_reg3 >> SYS_REG_CS1_COL_SHIFT(ch) &
                                  SYS_REG_CS1_COL_MASK);


Not sure any of the above suggestions makes the code any easier to
understand.

Regards,
Jonas

>               if (version >= 2) {
>                       cs1_col = 9 + (sys_reg3 >> SYS_REG_CS1_COL_SHIFT(ch) &
>                                 SYS_REG_CS1_COL_MASK);