[U-Boot] sunxi: H6: DRAM: avoid memcpy() on MMIO registers

Using memcpy() for MMIO operations is, however tempting, not a good idea:
It depends on the specific implementation of memcpy, also lacks barriers.
In this particular case the first registers were written using 64-bit
writes, and the last register using four separate single-byte writes.
Neither is what we actually want.
We get away with it in this case because of the particular details of
the bus implementation, the DRAM controller IP and the values that we
actually write, but we should not leave a bad example around.

Replace the memcpy with a proper loop using the writel() accessor.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm/mach-sunxi/dram_sun50i_h6.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

On Thu, Dec 06, 2018 at 12:02:20AM +0000, Andre Przywara wrote:
> Using memcpy() for MMIO operations is, however tempting, not a good idea:
> It depends on the specific implementation of memcpy, also lacks barriers.
> In this particular case the first registers were written using 64-bit
> writes, and the last register using four separate single-byte writes.
> Neither is what we actually want.
> We get away with it in this case because of the particular details of
> the bus implementation, the DRAM controller IP and the values that we
> actually write, but we should not leave a bad example around.
> 
> Replace the memcpy with a proper loop using the writel() accessor.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arch/arm/mach-sunxi/dram_sun50i_h6.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-sunxi/dram_sun50i_h6.c b/arch/arm/mach-sunxi/dram_sun50i_h6.c
> index 5da90a2835..e2f141eb9b 100644
> --- a/arch/arm/mach-sunxi/dram_sun50i_h6.c
> +++ b/arch/arm/mach-sunxi/dram_sun50i_h6.c
> @@ -182,6 +182,7 @@ static void mctl_set_timing_lpddr3(struct dram_para *para)
>  			(struct sunxi_mctl_ctl_reg *)SUNXI_DRAM_CTL0_BASE;
>  	struct sunxi_mctl_phy_reg * const mctl_phy =
>  			(struct sunxi_mctl_phy_reg *)SUNXI_DRAM_PHY0_BASE;
> +	int i;
>  
>  	u8 tccd		= 2;
>  	u8 tfaw		= max(ns_to_t(50), 4);
> @@ -237,8 +238,9 @@ static void mctl_set_timing_lpddr3(struct dram_para *para)
>  	u8 twr2rd	= tcwl + 4 + 1 + twtr;
>  	u8 trd2wr	= tcl + 4 + (tcksrea >> 1) - tcwl + 1;
>  
> -	/* set mode register */
> -	memcpy(mctl_phy->mr, mr_lpddr3, sizeof(mr_lpddr3));
> +	/* set mode registers */
> +	for (i = 0; i < ARRAY_SIZE(mr_lpddr3); i++)
> +		writel(mr_lpddr3[i], &mctl_phy->mr[i]);

memcpy_toio is meant to do just that.

Maxime

On 06/12/2018 07:42, Maxime Ripard wrote:

Hi,

> On Thu, Dec 06, 2018 at 12:02:20AM +0000, Andre Przywara wrote:
>> Using memcpy() for MMIO operations is, however tempting, not a good
>> idea: It depends on the specific implementation of memcpy, also
>> lacks barriers. In this particular case the first registers were
>> written using 64-bit writes, and the last register using four
>> separate single-byte writes. Neither is what we actually want.
>> We get away with it in this case because of the particular details of
>> the bus implementation, the DRAM controller IP and the values that we
>> actually write, but we should not leave a bad example around.
>>
>> Replace the memcpy with a proper loop using the writel() accessor.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  arch/arm/mach-sunxi/dram_sun50i_h6.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mach-sunxi/dram_sun50i_h6.c
>> b/arch/arm/mach-sunxi/dram_sun50i_h6.c index 5da90a2835..e2f141eb9b
>> 100644 --- a/arch/arm/mach-sunxi/dram_sun50i_h6.c
>> +++ b/arch/arm/mach-sunxi/dram_sun50i_h6.c
>> @@ -182,6 +182,7 @@ static void mctl_set_timing_lpddr3(struct
>> dram_para *para) (struct sunxi_mctl_ctl_reg *)SUNXI_DRAM_CTL0_BASE;
>>  	struct sunxi_mctl_phy_reg * const mctl_phy =
>>  			(struct sunxi_mctl_phy_reg
>> *)SUNXI_DRAM_PHY0_BASE;
>> +	int i;
>>  
>>  	u8 tccd		= 2;
>>  	u8 tfaw		= max(ns_to_t(50), 4);
>> @@ -237,8 +238,9 @@ static void mctl_set_timing_lpddr3(struct
>> dram_para *para) u8 twr2rd	= tcwl + 4 + 1 + twtr;
>>  	u8 trd2wr	= tcl + 4 + (tcksrea >> 1) - tcwl + 1;
>>  
>> -	/* set mode register */
>> -	memcpy(mctl_phy->mr, mr_lpddr3, sizeof(mr_lpddr3));
>> +	/* set mode registers */
>> +	for (i = 0; i < ARRAY_SIZE(mr_lpddr3); i++)
>> +		writel(mr_lpddr3[i], &mctl_phy->mr[i]);
> 
> memcpy_toio is meant to do just that.

"meant to" is the right wording ;-), since it doesn't seem to do it: If
I understand asm/io.h correctly (which apparently was just copied 20
years ago or so from Russell's Linux), it simply defines to
memcpy - if we don't have __mempci defined - whatever that means in
2018's U-Boot ;-) Funny enough even the Linux implementation is somewhat
dodgy, as it uses the raw_read<x> accessors (which don't do much in
U-Boot, I believe), but still uses 8-bit and 64-bit wide accesses, in
the hope that this is handled by the device.

So I'd rather stick with the explicit loop, as I saw funny effects with
byte writes to DRAM registers.

Cheers,
Andre.