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

[Kever Yang]

Hi Jonas,

On 2024/1/23 01:55, Jonas Karlman wrote:
> Hi,
>
> On 2024-01-22 08:46, Kever Yang wrote:
>> From: YouMin Chen <cym at rock-chips.com>
>>
>> This patch add support for additional bank info used by LPDDR5.
>>
>> Signed-off-by: YouMin Chen <cym at rock-chips.com>
>> Signed-off-by: Kever Yang <kever.yang at 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);

This is better to understand, LPDDR5 has different bank definition with 
other type dram.

Thanks,

- Kever

>   		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);