[PATCH] ddr: fsl: Make bank_addr_bits reflect actual bits

[PATCH] ddr: fsl: Make bank_addr_bits reflect actual bits

[Sean Anderson]

In both the Freescale DDR controller and the SPD spec, bank address bits
are stored as the number of bank address bits minus 2. For example, if a
chip had 8 banks (3 total bank address bits), the value of
bank_addr_bits would be 1. This is rather surprising for users
configuring their memory manually, since they can't set bank_addr_bits
to the actual number of bank address bits. Rectify this.

There is at least one example of this kind of mistake already, in
board/freescale/t102xrdb/ddr.c. The documented MT40A512M8HX has two bank
address bits, but bank_addr_bits was set to 2, implying 4 bank address
bits. Such a value is reserved in BA_BITS_CS, but I suspect the
controller simply ignores the top bit, making this kind of mistake
harmless, if misleading.

Fixes: e8a7f1c32b5 ("powerpc/t1023rdb: Add T1023 RDB board support")
Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 board/freescale/ls1043ardb/ddr.c   | 2 +-
 drivers/ddr/fsl/ctrl_regs.c        | 2 +-
 drivers/ddr/fsl/ddr4_dimm_params.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/board/freescale/ls1043ardb/ddr.c b/board/freescale/ls1043ardb/ddr.c
index 08b43ff5e4c..4d2fce38412 100644
--- a/board/freescale/ls1043ardb/ddr.c
+++ b/board/freescale/ls1043ardb/ddr.c
@@ -114,7 +114,7 @@ dimm_params_t ddr_raw_timing = {
 	.mirrored_dimm = 0,
 	.n_row_addr = 15,
 	.n_col_addr = 10,
-	.bank_addr_bits = 0,
+	.bank_addr_bits = 2,
 	.bank_group_bits = 2,
 	.edc_config = 0,
 	.burst_lengths_bitmask = 0x0c,
diff --git a/drivers/ddr/fsl/ctrl_regs.c b/drivers/ddr/fsl/ctrl_regs.c
index b5122d1a1c3..0b0b4e5cb7e 100644
--- a/drivers/ddr/fsl/ctrl_regs.c
+++ b/drivers/ddr/fsl/ctrl_regs.c
@@ -214,7 +214,7 @@ static void set_csn_config(int dimm_number, int i, fsl_ddr_cfg_regs_t *ddr,
 		odt_rd_cfg = popts->cs_local_opts[i].odt_rd_cfg;
 		odt_wr_cfg = popts->cs_local_opts[i].odt_wr_cfg;
 #ifdef CONFIG_SYS_FSL_DDR4
-		ba_bits_cs_n = dimm_params[dimm_number].bank_addr_bits;
+		ba_bits_cs_n = dimm_params[dimm_number].bank_addr_bits - 2;
 		bg_bits_cs_n = dimm_params[dimm_number].bank_group_bits;
 #else
 		n_banks_per_sdram_device
diff --git a/drivers/ddr/fsl/ddr4_dimm_params.c b/drivers/ddr/fsl/ddr4_dimm_params.c
index e2bdc12ef2c..ea791622628 100644
--- a/drivers/ddr/fsl/ddr4_dimm_params.c
+++ b/drivers/ddr/fsl/ddr4_dimm_params.c
@@ -246,7 +246,7 @@ unsigned int ddr_compute_dimm_parameters(const unsigned int ctrl_num,
 	/* SDRAM device parameters */
 	pdimm->n_row_addr = ((spd->addressing >> 3) & 0x7) + 12;
 	pdimm->n_col_addr = (spd->addressing & 0x7) + 9;
-	pdimm->bank_addr_bits = (spd->density_banks >> 4) & 0x3;
+	pdimm->bank_addr_bits = ((spd->density_banks >> 4) & 0x3) + 2;
 	pdimm->bank_group_bits = (spd->density_banks >> 6) & 0x3;
 
 	/*
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH] ddr: fsl: Make bank_addr_bits reflect actual bits

[Peng Fan]

On 8/31/2022 5:01 AM, Sean Anderson wrote:
> In both the Freescale DDR controller and the SPD spec, bank address bits
> are stored as the number of bank address bits minus 2. For example, if a
> chip had 8 banks (3 total bank address bits), the value of
> bank_addr_bits would be 1. This is rather surprising for users
> configuring their memory manually, since they can't set bank_addr_bits
> to the actual number of bank address bits. Rectify this.
> 
> There is at least one example of this kind of mistake already, in
> board/freescale/t102xrdb/ddr.c. The documented MT40A512M8HX has two bank
> address bits, but bank_addr_bits was set to 2, implying 4 bank address
> bits. Such a value is reserved in BA_BITS_CS, but I suspect the
> controller simply ignores the top bit, making this kind of mistake
> harmless, if misleading.
> 
> Fixes: e8a7f1c32b5 ("powerpc/t1023rdb: Add T1023 RDB board support")
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>

In tag:
https://source.denx.de/u-boot/custodians/u-boot-fsl-qoriq/-/tags/fsl-qoriq-2022-9-6

Waiting CI results.

Regards,
Peng.

> ---
> 
>   board/freescale/ls1043ardb/ddr.c   | 2 +-
>   drivers/ddr/fsl/ctrl_regs.c        | 2 +-
>   drivers/ddr/fsl/ddr4_dimm_params.c | 2 +-
>   3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/board/freescale/ls1043ardb/ddr.c b/board/freescale/ls1043ardb/ddr.c
> index 08b43ff5e4c..4d2fce38412 100644
> --- a/board/freescale/ls1043ardb/ddr.c
> +++ b/board/freescale/ls1043ardb/ddr.c
> @@ -114,7 +114,7 @@ dimm_params_t ddr_raw_timing = {
>   	.mirrored_dimm = 0,
>   	.n_row_addr = 15,
>   	.n_col_addr = 10,
> -	.bank_addr_bits = 0,
> +	.bank_addr_bits = 2,
>   	.bank_group_bits = 2,
>   	.edc_config = 0,
>   	.burst_lengths_bitmask = 0x0c,
> diff --git a/drivers/ddr/fsl/ctrl_regs.c b/drivers/ddr/fsl/ctrl_regs.c
> index b5122d1a1c3..0b0b4e5cb7e 100644
> --- a/drivers/ddr/fsl/ctrl_regs.c
> +++ b/drivers/ddr/fsl/ctrl_regs.c
> @@ -214,7 +214,7 @@ static void set_csn_config(int dimm_number, int i, fsl_ddr_cfg_regs_t *ddr,
>   		odt_rd_cfg = popts->cs_local_opts[i].odt_rd_cfg;
>   		odt_wr_cfg = popts->cs_local_opts[i].odt_wr_cfg;
>   #ifdef CONFIG_SYS_FSL_DDR4
> -		ba_bits_cs_n = dimm_params[dimm_number].bank_addr_bits;
> +		ba_bits_cs_n = dimm_params[dimm_number].bank_addr_bits - 2;
>   		bg_bits_cs_n = dimm_params[dimm_number].bank_group_bits;
>   #else
>   		n_banks_per_sdram_device
> diff --git a/drivers/ddr/fsl/ddr4_dimm_params.c b/drivers/ddr/fsl/ddr4_dimm_params.c
> index e2bdc12ef2c..ea791622628 100644
> --- a/drivers/ddr/fsl/ddr4_dimm_params.c
> +++ b/drivers/ddr/fsl/ddr4_dimm_params.c
> @@ -246,7 +246,7 @@ unsigned int ddr_compute_dimm_parameters(const unsigned int ctrl_num,
>   	/* SDRAM device parameters */
>   	pdimm->n_row_addr = ((spd->addressing >> 3) & 0x7) + 12;
>   	pdimm->n_col_addr = (spd->addressing & 0x7) + 9;
> -	pdimm->bank_addr_bits = (spd->density_banks >> 4) & 0x3;
> +	pdimm->bank_addr_bits = ((spd->density_banks >> 4) & 0x3) + 2;
>   	pdimm->bank_group_bits = (spd->density_banks >> 6) & 0x3;
>   
>   	/*