Skip to contentSkip to conversation

sunxi: H6: DRAM: avoid memcpy() on MMIO registers

PATCH New Andre Przywara 2 participants 3 messages Dec 6, 2018 – Dec 7, 2018

Subsystems (3)
Arm (32-bit) Arm sub-architectures Arm/Allwinner sunXi SoC support
lore
Share & tools
lore

Andre Przywara

author
#0 lore raw

PATCH 1/1 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 <[email protected]>
---
 arch/arm/mach-sunxi/dram_sun50i_h6.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
arch/arm/mach-sunxi/dram_sun50i_h6.c +4 -2
index 5da90a2835..e2f141eb9b 100644
@@ -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]);
 
 	/* set DRAM timing */
 	writel((twtp << 24) | (tfaw << 16) | (trasmax << 8) | tras,
Signature · 1 line
2.14.5
#1 lore raw

PATCH 1/1 sunxi: H6: DRAM: avoid memcpy() on MMIO registers

Quoted 29 lines · Andre Przywara

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 <[email protected]>
---
 arch/arm/mach-sunxi/dram_sun50i_h6.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
arch/arm/mach-sunxi/dram_sun50i_h6.c +1 -0
index 5da90a2835..e2f141eb9b 100644
@@ -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);

arch/arm/mach-sunxi/dram_sun50i_h6.c static void mctl_set_timing_lpddr3(struct dram_para *para) · line 243

Quoted 8 lines
arch/arm/mach-sunxi/dram_sun50i_h6.c +3 -2
@@ -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

Signature · 10 lines
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181206/93f2f7c7/attachment.sig>
author
#2 lore raw

PATCH 1/1 sunxi: H6: DRAM: avoid memcpy() on MMIO registers

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

Hi,

Quoted 34 lines · Andre Przywara
Quoted 33 lines

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 <[email protected]>
---
 arch/arm/mach-sunxi/dram_sun50i_h6.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
(quoted diff) +0 -0

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

arch/arm/mach-sunxi/dram_sun50i_h6.c +1 -0
@@ -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;
arch/arm/mach-sunxi/dram_sun50i_h6.c +0 -0
 	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;
Quoted 7 lines

arch/arm/mach-sunxi/dram_sun50i_h6.c static void mctl_set_timing_lpddr3(struct dram_para *para) · line 243

Quoted 5 lines
arch/arm/mach-sunxi/dram_sun50i_h6.c +3 -2
-	/* 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.