[U-Boot] [PATCH 1/3] driver/ddr: Add support for setting timing in hws_topology_map

[Stefan Roese]

Hi Marek,


looks good, as small comment below...

On 12.05.2017 16:10, Marek Behún wrote:
> The DDR3 training code for Marvell A38X currently computes 1t timing
> when given board topology map of the Turris Omnia, but Omnia needs 2t.
> 
> This patch adds support for enforcing the 2t timing in struct
> hws_topology_map, through a new enum hws_timing, which can assume
> following values:
>   HWS_TIM_DEFAULT - default behaviour, compute whether to enable 2t
>                     from the number of CSs
>   HWS_TIM_1T      - enforce 1t
>   HWS_TIM_2T      - enforce 2t
> 
> This patch also sets all the board topology maps (db-88f6820-amc,
> db-88f6820-gp, controlcenterdc and clearfog) to have timing set to
> HWS_TIM_DEFAULT.
> 
> Signed-off-by: Marek Behun <marek.behun at nic.cz>
> ---
>  board/Marvell/db-88f6820-amc/db-88f6820-amc.c | 3 ++-
>  board/Marvell/db-88f6820-gp/db-88f6820-gp.c   | 3 ++-
>  board/gdsys/a38x/controlcenterdc.c            | 3 ++-
>  board/solidrun/clearfog/clearfog.c            | 3 ++-
>  drivers/ddr/marvell/a38x/ddr3_training.c      | 5 +++++
>  drivers/ddr/marvell/a38x/ddr_topology_def.h   | 9 +++++++++
>  6 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/board/Marvell/db-88f6820-amc/db-88f6820-amc.c b/board/Marvell/db-88f6820-amc/db-88f6820-amc.c
> index cade99c..40fa599 100644
> --- a/board/Marvell/db-88f6820-amc/db-88f6820-amc.c
> +++ b/board/Marvell/db-88f6820-amc/db-88f6820-amc.c
> @@ -69,7 +69,8 @@ static struct hws_topology_map board_topology_map = {
>  	    MEM_4G,			/* mem_size */
>  	    DDR_FREQ_800,		/* frequency */
>  	    0, 0,			/* cas_l cas_wl */
> -	    HWS_TEMP_LOW} },		/* temperature */
> +	    HWS_TEMP_LOW,		/* temperature */
> +	    HWS_TIM_DEFAULT} },		/* timing */
>  	5,				/* Num Of Bus Per Interface*/
>  	BUS_MASK_32BIT			/* Busses mask */
>  };
> diff --git a/board/Marvell/db-88f6820-gp/db-88f6820-gp.c b/board/Marvell/db-88f6820-gp/db-88f6820-gp.c
> index e700781..a1974cb 100644
> --- a/board/Marvell/db-88f6820-gp/db-88f6820-gp.c
> +++ b/board/Marvell/db-88f6820-gp/db-88f6820-gp.c
> @@ -90,7 +90,8 @@ static struct hws_topology_map board_topology_map = {
>  	    MEM_4G,			/* mem_size */
>  	    DDR_FREQ_800,		/* frequency */
>  	    0, 0,			/* cas_l cas_wl */
> -	    HWS_TEMP_LOW} },		/* temperature */
> +	    HWS_TEMP_LOW,		/* temperature */
> +	    HWS_TIM_DEFAULT} },		/* timing */
>  	5,				/* Num Of Bus Per Interface*/
>  	BUS_MASK_32BIT			/* Busses mask */
>  };
> diff --git a/board/gdsys/a38x/controlcenterdc.c b/board/gdsys/a38x/controlcenterdc.c
> index f0efb53..32168d3 100644
> --- a/board/gdsys/a38x/controlcenterdc.c
> +++ b/board/gdsys/a38x/controlcenterdc.c
> @@ -53,7 +53,8 @@ static struct hws_topology_map ddr_topology_map = {
>  	    MEM_4G,			/* mem_size */
>  	    DDR_FREQ_533,		/* frequency */
>  	    0, 0,			/* cas_l cas_wl */
> -	    HWS_TEMP_LOW} },		/* temperature */
> +	    HWS_TEMP_LOW,		/* temperature */
> +	    HWS_TIM_DEFAULT} },		/* timing */
>  	5,				/* Num Of Bus Per Interface*/
>  	BUS_MASK_32BIT			/* Busses mask */
>  };
> diff --git a/board/solidrun/clearfog/clearfog.c b/board/solidrun/clearfog/clearfog.c
> index 2773f59..036e183 100644
> --- a/board/solidrun/clearfog/clearfog.c
> +++ b/board/solidrun/clearfog/clearfog.c
> @@ -83,7 +83,8 @@ static struct hws_topology_map board_topology_map = {
>  	    MEM_4G,			/* mem_size */
>  	    DDR_FREQ_800,		/* frequency */
>  	    0, 0,			/* cas_l cas_wl */
> -	    HWS_TEMP_LOW} },		/* temperature */
> +	    HWS_TEMP_LOW,		/* temperature */
> +	    HWS_TIM_DEFAULT} },		/* timing */
>  	5,				/* Num Of Bus Per Interface*/
>  	BUS_MASK_32BIT			/* Busses mask */
>  };
> diff --git a/drivers/ddr/marvell/a38x/ddr3_training.c b/drivers/ddr/marvell/a38x/ddr3_training.c
> index 7e0749f..c79db6a 100644
> --- a/drivers/ddr/marvell/a38x/ddr3_training.c
> +++ b/drivers/ddr/marvell/a38x/ddr3_training.c
> @@ -571,6 +571,11 @@ int hws_ddr3_tip_init_controller(u32 dev_num, struct init_cntr_param *init_cntr_
>  
>  			if (mode2_t != 0xff) {
>  				t2t = mode2_t;
> +			} else if (tm->interface_params[if_id].
> +				   timing != HWS_TIM_DEFAULT) {
> +				/* Board topology map is forcing timing */
> +				t2t = (tm->interface_params[if_id].
> +				       timing == HWS_TIM_2T) ? 1 : 0;

Perhaps its better to create a local variable for this 
"tm->interface_params[if_id].timing" value. It makes the lines somewhat
shorter and easier to read and you don't need to address it twice this
way. Something like this instead:

+			timing = tm->interface_params[if_id].timing;
			...

+			} else if (timing != HWS_TIM_DEFAULT) {
+				/* Board topology map is forcing timing */
+				t2t = timing == HWS_TIM_2T ? 1 : 0;

Other than this, you can add my:

Reviewed-by: Stefan Roese <sr at denx.de>

to the next patch version.

Thanks,
Stefan