Amlogic Meson GXBaby and ODROID-C2 support

All members of the DMA descriptor must be 32-bit, even on 64-bit
architectures: change the type to u32 to ensure this. Also, fix other warnings.

Signed-off-by Beniamino Galvani <[email protected]>

Acked-by Joe Hershberger <[email protected]>

Reviewed-by Tom Rini <[email protected]>

---
 drivers/net/designware.c | 54 ++++++++++++++++++++++++------------------------
 drivers/net/designware.h |  4 ++--
 2 files changed, 29 insertions(+), 29 deletions(-)

drivers/net/designware.c +0 -0

index ca58f34..78d6901 100644

drivers/net/designware.c +0 -1

 #if defined(CONFIG_DW_ALTDESCRIPTOR)
 		desc_p->txrx_status &= ~(DESC_TXSTS_TXINT | DESC_TXSTS_TXLAST |

@@ -117,11 +117,11 @@ static void tx_descs_init(struct dw_eth_dev *priv)
 	}
 
 	/* Correcting the last pointer of the chain */
-	desc_p->dmamac_next = &desc_table_p[0];

This looks more like silencing the warning by a forced cast. The pointer should most likely be sanity-checked to make sure it's in 4GiB address space at least. I am worried such forced casts will bite us in the long run.

This adds platform code for the Amlogic Meson GXBaby (S905) SoC and a board definition for ODROID-C2. This initial submission only supports UART and Ethernet (through the existing Designware driver). DTS files are the ones submitted to Linux arm-soc for 4.7 [1].

[1] https://patchwork.ozlabs.org/patch/603583/

Signed-off-by Beniamino Galvani <[email protected]>

arch/arm/include/asm/arch-meson/gxbb.h new file +5 -0

new file mode 100644
index 0000000..cec06d7
@@ -0,0 +1,77 @@
+/*
+ * (C) Copyright 2016 - Beniamino Galvani <[email protected]>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */

arch/arm/include/asm/arch-meson/gxbb.h new file +10 -0

+#define GXBB_GPIO_1_EN		GXBB_PERIPHS_ADDR(0x0f)
+#define GXBB_GPIO_1_OUT		GXBB_PERIPHS_ADDR(0x10)
+#define GXBB_GPIO_1_IN		GXBB_PERIPHS_ADDR(0x11)
+#define GXBB_GPIO_2_EN		GXBB_PERIPHS_ADDR(0x12)
+#define GXBB_GPIO_2_OUT		GXBB_PERIPHS_ADDR(0x13)
+#define GXBB_GPIO_2_IN		GXBB_PERIPHS_ADDR(0x14)
+#define GXBB_GPIO_3_EN		GXBB_PERIPHS_ADDR(0x15)
+#define GXBB_GPIO_3_OUT		GXBB_PERIPHS_ADDR(0x16)
+#define GXBB_GPIO_3_IN		GXBB_PERIPHS_ADDR(0x17)
+#define GXBB_GPIO_4_EN		GXBB_PERIPHS_ADDR(0x18)

arch/arm/include/asm/arch-meson/gxbb.h new file +13 -0

+#define GXBB_PINMUX_1		GXBB_PERIPHS_ADDR(0x2d)
+#define GXBB_PINMUX_2		GXBB_PERIPHS_ADDR(0x2e)
+#define GXBB_PINMUX_3		GXBB_PERIPHS_ADDR(0x2f)
+#define GXBB_PINMUX_4		GXBB_PERIPHS_ADDR(0x30)
+#define GXBB_PINMUX_5		GXBB_PERIPHS_ADDR(0x31)
+#define GXBB_PINMUX_6		GXBB_PERIPHS_ADDR(0x32)
+#define GXBB_PINMUX_7		GXBB_PERIPHS_ADDR(0x33)
+#define GXBB_PINMUX_8		GXBB_PERIPHS_ADDR(0x34)
+#define GXBB_PINMUX_9		GXBB_PERIPHS_ADDR(0x35)
+#define GXBB_PINMUX_10		GXBB_PERIPHS_ADDR(0x36)
+#define GXBB_PINMUX_11		GXBB_PERIPHS_ADDR(0x37)
+#define GXBB_PINMUX_12		GXBB_PERIPHS_ADDR(0x38)
+

arch/arm/include/asm/arch-meson/gxbb.h new file +19 -0

+#define GXBB_HIU_ADDR(off)	(GXBB_HIU_BASE + ((off) << 2))
+
+#define GXBB_MEM_PD_REG_0	GXBB_HIU_ADDR(0x40)
+
+/* Ethernet memory power domain */
+#define GXBB_MEM_PD_REG_0_ETH_MASK	(BIT(2) | BIT(3))
+
+/* Clock gates */
+#define GXBB_GCLK_MPEG_0	GXBB_HIU_ADDR(0x50)
+#define GXBB_GCLK_MPEG_1	GXBB_HIU_ADDR(0x51)
+#define GXBB_GCLK_MPEG_2	GXBB_HIU_ADDR(0x52)
+#define GXBB_GCLK_MPEG_OTHER	GXBB_HIU_ADDR(0x53)
+#define GXBB_GCLK_MPEG_AO	GXBB_HIU_ADDR(0x54)
+
+#define GXBB_GCLK_MPEG_1_ETH	BIT(3)
+
+#define GXBB_ETH_BASE		0xc9410000
+
+#endif /* __GXBB_H__ */

arch/arm/mach-meson/board.c new file +19 -0

new file mode 100644
index 0000000..1d4d32e
@@ -0,0 +1,65 @@
+/*
+ * (C) Copyright 2016 Beniamino Galvani <[email protected]>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <libfdt.h>
+#include <linux/err.h>
+#include <asm/arch/gxbb.h>
+#include <asm/armv8/mmu.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+int dram_init(void)
+{
+	const fdt32_t *val;
+	int offset;
+	int len;

arch/arm/mach-meson/board.c new file +11 -0

+}
+
+static struct mm_region gxbb_mem_map[] = {
+	{
+		.base = 0x0UL,
+		.size = 0x80000000UL,
+		.attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) |
+			 PTE_BLOCK_INNER_SHARE
+	}, {
+		.base = 0x80000000UL,
+		.size = 0x80000000UL,

board/hardkernel/odroid-c2/odroid-c2.c new file +38 -0

new file mode 100644
index 0000000..405ccf5
@@ -0,0 +1,53 @@
+/*
+ * (C) Copyright 2016 Beniamino Galvani <[email protected]>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <asm/io.h>
+#include <asm/arch/gxbb.h>
+#include <dm/platdata.h>
+#include <phy.h>
+
+int board_init(void)
+{
+	return 0;
+}
+
+static const struct eth_pdata gxbb_eth_pdata = {
+	.iobase = GXBB_ETH_BASE,
+	.phy_interface = PHY_INTERFACE_MODE_RGMII,
+};
+
+U_BOOT_DEVICE(meson_eth) = {
+	.name = "eth_designware",
+	.platdata = &gxbb_eth_pdata,
+};
+
+#ifdef CONFIG_MISC_INIT_R
+int misc_init_r(void)
+{
+	/* Select Ethernet function */
+	setbits_le32(GXBB_PINMUX_6, 0x3fff);
+
+	/* Set RGMII mode */
+	setbits_le32(GXBB_ETH_REG_0, GXBB_ETH_REG_0_PHY_INTF |
+				     GXBB_ETH_REG_0_TX_PHASE(1) |
+				     GXBB_ETH_REG_0_TX_RATIO(4) |
+				     GXBB_ETH_REG_0_PHY_CLK_EN |

board/hardkernel/odroid-c2/odroid-c2.c new file +5 -0

+	setbits_le32(GXBB_GPIO_3_OUT, BIT(14));
+
+	return 0;
+}
+#endif /* CONFIG_MISC_INIT_R */

new file mode 100644
index 0000000..140b2d4

drivers/serial/serial_meson.c new file +11 -0

@@ -0,0 +1,162 @@
+/*
+ * (C) Copyright 2016 Beniamino Galvani <[email protected]>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <errno.h>
+#include <fdtdec.h>
+#include <linux/compiler.h>

drivers/serial/serial_meson.c new file +24 -0

+};
+
+struct meson_serial_platdata {
+	struct meson_uart *reg;
+};
+
+/* AML_UART_STATUS bits */
+#define AML_UART_PARITY_ERR		BIT(16)
+#define AML_UART_FRAME_ERR		BIT(17)
+#define AML_UART_TX_FIFO_WERR		BIT(18)
+#define AML_UART_RX_EMPTY		BIT(20)
+#define AML_UART_TX_FULL		BIT(21)
+#define AML_UART_TX_EMPTY		BIT(22)
+#define AML_UART_XMIT_BUSY		BIT(25)
+#define AML_UART_ERR			(AML_UART_PARITY_ERR | \
+					 AML_UART_FRAME_ERR  | \
+					 AML_UART_TX_FIFO_WERR)
+
+/* AML_UART_CONTROL bits */
+#define AML_UART_TX_EN			BIT(12)
+#define AML_UART_RX_EN			BIT(13)
+#define AML_UART_TX_RST			BIT(22)
+#define AML_UART_RX_RST			BIT(23)
+#define AML_UART_CLR_ERR		BIT(24)

arch/arm/include/asm/arch-meson/gxbb.h line 15

Quoted 4 lines

arch/arm/include/asm/arch-meson/gxbb.h new file +3 -0

+#define GXBB_GPIO_0_EN		GXBB_PERIPHS_ADDR(0x0c)
+#define GXBB_GPIO_0_OUT		GXBB_PERIPHS_ADDR(0x0d)
+#define GXBB_GPIO_0_IN		GXBB_PERIPHS_ADDR(0x0e)

You can also define this as
GXBB_GPIO_EN(n) (0xc + 3 * (n) + 0)
GXBB_GPIO_OUT(n) (0xc + 3 * (n) + 1)
GXBB_GPIO_IN(n) (0xc + 3 * (n) + 2)

Quoted 13 lines

arch/arm/include/asm/arch-meson/gxbb.h new file +13 -0

+#define GXBB_GPIO_1_EN		GXBB_PERIPHS_ADDR(0x0f)
+#define GXBB_GPIO_1_OUT		GXBB_PERIPHS_ADDR(0x10)
+#define GXBB_GPIO_1_IN		GXBB_PERIPHS_ADDR(0x11)
+#define GXBB_GPIO_2_EN		GXBB_PERIPHS_ADDR(0x12)
+#define GXBB_GPIO_2_OUT		GXBB_PERIPHS_ADDR(0x13)
+#define GXBB_GPIO_2_IN		GXBB_PERIPHS_ADDR(0x14)
+#define GXBB_GPIO_3_EN		GXBB_PERIPHS_ADDR(0x15)
+#define GXBB_GPIO_3_OUT		GXBB_PERIPHS_ADDR(0x16)
+#define GXBB_GPIO_3_IN		GXBB_PERIPHS_ADDR(0x17)
+#define GXBB_GPIO_4_EN		GXBB_PERIPHS_ADDR(0x18)
+#define GXBB_GPIO_4_OUT		GXBB_PERIPHS_ADDR(0x19)
+#define GXBB_GPIO_4_IN		GXBB_PERIPHS_ADDR(0x1a)
+#define GXBB_GPIO_5_EN		GXBB_PERIPHS_ADDR(0x1b)

arch/arm/include/asm/arch-meson/gxbb.h line 33

Quoted 5 lines

arch/arm/include/asm/arch-meson/gxbb.h new file +5 -0

+#define GXBB_GPIO_5_OUT		GXBB_PERIPHS_ADDR(0x1c)
+#define GXBB_GPIO_5_IN		GXBB_PERIPHS_ADDR(0x1d)
+#define GXBB_GPIO_6_EN		GXBB_PERIPHS_ADDR(0x08)
+#define GXBB_GPIO_6_OUT		GXBB_PERIPHS_ADDR(0x09)
+#define GXBB_GPIO_6_IN		GXBB_PERIPHS_ADDR(0x0a)

It'd be nice to have base addresses somewhere at the beginning instead of having them mixed with the bit macros, but that's a matter of taste.

arch/arm/mach-meson/board.c line 29

Quoted 5 lines

arch/arm/mach-meson/board.c new file +5 -0

+	val = fdt_getprop(gd->fdt_blob, offset, "reg", &len);
+	if (len < sizeof(*val) * 4)
+		return -EINVAL;
+
+	/* Don't use fdt64_t to avoid unaligned access */

This looks iffy, can you elaborate on this issue ?

arch/arm/mach-meson/board.c line 38

Quoted 1 line

arch/arm/mach-meson/board.c new file +1 -0

+	/* Reserve first 16 MiB of RAM */

Why ?

Review

Quoted 2 lines

(quoted diff) +2 -0

+void reset_cpu(ulong addr)
+{

How does the system reboot then ?

Quoted 1 line

+struct mm_region *mem_map = gxbb_mem_map;

This looks super-iffy, I wouldn't be surprised if this started being optimized away at some point.

board/hardkernel/odroid-c2/odroid-c2.c line 48

Quoted 4 lines

board/hardkernel/odroid-c2/odroid-c2.c new file +4 -0

+	/* Reset PHY on GPIOZ_14 */
+	clrbits_le32(GXBB_GPIO_3_EN, BIT(14));
+	clrbits_le32(GXBB_GPIO_3_OUT, BIT(14));
+	udelay(100000);

mdelay(100); , though that is quite some wait.

drivers/serial/serial_meson.c

Quoted 1 line

drivers/serial/serial_meson.c +0 -0

This should go in a separate patch.

Quoted 9 lines · Marek Vasut

arch/arm/include/asm/arch-meson/gxbb.h line 15

Quoted 4 lines

arch/arm/include/asm/arch-meson/gxbb.h new file +3 -0

+#define GXBB_GPIO_0_EN		GXBB_PERIPHS_ADDR(0x0c)
+#define GXBB_GPIO_0_OUT		GXBB_PERIPHS_ADDR(0x0d)
+#define GXBB_GPIO_0_IN		GXBB_PERIPHS_ADDR(0x0e)

You can also define this as
GXBB_GPIO_EN(n) (0xc + 3 * (n) + 0)
GXBB_GPIO_OUT(n) (0xc + 3 * (n) + 1)
GXBB_GPIO_IN(n) (0xc + 3 * (n) + 2)

This would work well if GPIO_6 didn't exist :)

Quoted 13 lines

Quoted 13 lines

arch/arm/include/asm/arch-meson/gxbb.h new file +13 -0

+#define GXBB_GPIO_1_EN		GXBB_PERIPHS_ADDR(0x0f)
+#define GXBB_GPIO_1_OUT		GXBB_PERIPHS_ADDR(0x10)
+#define GXBB_GPIO_1_IN		GXBB_PERIPHS_ADDR(0x11)
+#define GXBB_GPIO_2_EN		GXBB_PERIPHS_ADDR(0x12)
+#define GXBB_GPIO_2_OUT		GXBB_PERIPHS_ADDR(0x13)
+#define GXBB_GPIO_2_IN		GXBB_PERIPHS_ADDR(0x14)
+#define GXBB_GPIO_3_EN		GXBB_PERIPHS_ADDR(0x15)
+#define GXBB_GPIO_3_OUT		GXBB_PERIPHS_ADDR(0x16)
+#define GXBB_GPIO_3_IN		GXBB_PERIPHS_ADDR(0x17)
+#define GXBB_GPIO_4_EN		GXBB_PERIPHS_ADDR(0x18)
+#define GXBB_GPIO_4_OUT		GXBB_PERIPHS_ADDR(0x19)
+#define GXBB_GPIO_4_IN		GXBB_PERIPHS_ADDR(0x1a)
+#define GXBB_GPIO_5_EN		GXBB_PERIPHS_ADDR(0x1b)

Quoted 8 lines

arch/arm/include/asm/arch-meson/gxbb.h line 33

Quoted 5 lines

arch/arm/include/asm/arch-meson/gxbb.h new file +5 -0

+#define GXBB_GPIO_5_OUT		GXBB_PERIPHS_ADDR(0x1c)
+#define GXBB_GPIO_5_IN		GXBB_PERIPHS_ADDR(0x1d)
+#define GXBB_GPIO_6_EN		GXBB_PERIPHS_ADDR(0x08)
+#define GXBB_GPIO_6_OUT		GXBB_PERIPHS_ADDR(0x09)
+#define GXBB_GPIO_6_IN		GXBB_PERIPHS_ADDR(0x0a)

It'd be nice to have base addresses somewhere at the beginning instead of having them mixed with the bit macros, but that's a matter of taste.

I agree, I'll change this.

Quoted 7 lines

arch/arm/mach-meson/board.c line 29

Quoted 5 lines

arch/arm/mach-meson/board.c new file +5 -0

+	val = fdt_getprop(gd->fdt_blob, offset, "reg", &len);
+	if (len < sizeof(*val) * 4)
+		return -EINVAL;
+
+	/* Don't use fdt64_t to avoid unaligned access */

This looks iffy, can you elaborate on this issue ?

I was getting a "Synchronous Abort handler, esr 0x96000021" which seemed to indicate a alignment fault, but thinking again about it I'm not sure anymore of the real cause. fdt64_t and fdt64_to_cpu() don't work here, I will try to investigate better why. Suggestions are welcome :)

Quoted 3 lines

arch/arm/mach-meson/board.c line 38

Quoted 1 line

arch/arm/mach-meson/board.c new file +1 -0

+	/* Reserve first 16 MiB of RAM */

Why ?

I'll add a comment, first 16 MiB seems to be reserved for firmware.

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+void reset_cpu(ulong addr)
+{

How does the system reboot then ?

The system reboots through a call to secure monitor, which is not implemented in this submission.

Quoted 4 lines

Quoted 1 line

+struct mm_region *mem_map = gxbb_mem_map;

This looks super-iffy, I wouldn't be surprised if this started being optimized away at some point.

I don't understand, why that should happen?

Quoted 6 lines

board/hardkernel/odroid-c2/odroid-c2.c line 48

Quoted 4 lines

board/hardkernel/odroid-c2/odroid-c2.c new file +4 -0

+	/* Reset PHY on GPIOZ_14 */
+	clrbits_le32(GXBB_GPIO_3_EN, BIT(14));
+	clrbits_le32(GXBB_GPIO_3_OUT, BIT(14));
+	udelay(100000);

mdelay(100); , though that is quite some wait.

Will change and decrease the timeout.

Quoted 3 lines

drivers/serial/serial_meson.c

Quoted 1 line

drivers/serial/serial_meson.c +0 -0

This should go in a separate patch.

I originally submitted the driver as separate patch, then Tom suggested that the initial platform patch should contain everything needed to perform a boot, so UART, DDR, board file and DTS.

Thanks for the review,

Beniamino

Quoted 8 lines

Quoted 7 lines

arch/arm/mach-meson/board.c line 29

Quoted 5 lines

arch/arm/mach-meson/board.c new file +5 -0

+	val = fdt_getprop(gd->fdt_blob, offset, "reg", &len);
+	if (len < sizeof(*val) * 4)
+		return -EINVAL;
+
+	/* Don't use fdt64_t to avoid unaligned access */

This looks iffy, can you elaborate on this issue ?

Quoted 5 lines

I was getting a "Synchronous Abort handler, esr 0x96000021" which seemed to indicate a alignment fault, but thinking again about it I'm not sure anymore of the real cause. fdt64_t and fdt64_to_cpu() don't work here, I will try to investigate better why. Suggestions are welcome :)

Toolchain issues ? Stack alignment issue ?

Quoted 14 lines · Marek Vasut

Quoted 13 lines

Quoted 7 lines

arch/arm/mach-meson/board.c line 29

Quoted 5 lines

arch/arm/mach-meson/board.c new file +5 -0

+	val = fdt_getprop(gd->fdt_blob, offset, "reg", &len);
+	if (len < sizeof(*val) * 4)
+		return -EINVAL;
+
+	/* Don't use fdt64_t to avoid unaligned access */

This looks iffy, can you elaborate on this issue ?

I was getting a "Synchronous Abort handler, esr 0x96000021" which seemed to indicate a alignment fault, but thinking again about it I'm not sure anymore of the real cause. fdt64_t and fdt64_to_cpu() don't work here, I will try to investigate better why. Suggestions are welcome :)

Quoted 1 line

Toolchain issues ? Stack alignment issue ?

So, after some investigation, the reason is that the code runs when caches are still disabled and thus all the memory is treated as Device-nGnRnE, requiring aligned accesses.

The return value of
fdt_getprop() is guaranteed to be aligned to a 4 byte boundary (but not 8)

and therefore a 32-bit type must be used to avoid alignment faults. Probably the comment should be updated to explain this better.

Beniamino

Quoted 3 lines

So, after some investigation, the reason is that the code runs when caches are still disabled and thus all the memory is treated as Device-nGnRnE, requiring aligned accesses.

You mean 8-byte aligned accesses, correct ?

Quoted 3 lines

The return value of
fdt_getprop() is guaranteed to be aligned to a 4 byte boundary (but not 8)

The return value of fdt_getprop() is a pointer, thus 8byte long on aarch64 and thus aligned to 8 bytes on the stack unless there is some real problem.

Quoted 2 lines

and therefore a 32-bit type must be used to avoid alignment faults. Probably the comment should be updated to explain this better.

Take a look at what uniphier does : arch/arm/mach-uniphier/dram_init.c Does that approach with fdt64_t work for you?

Quoted 5 lines · Marek Vasut

Quoted 3 lines

So, after some investigation, the reason is that the code runs when caches are still disabled and thus all the memory is treated as Device-nGnRnE, requiring aligned accesses.

You mean 8-byte aligned accesses, correct ?

Yes.

Quoted 7 lines

Quoted 3 lines

The return value of
fdt_getprop() is guaranteed to be aligned to a 4 byte boundary (but not 8)

The return value of fdt_getprop() is a pointer, thus 8byte long on aarch64 and thus aligned to 8 bytes on the stack unless there is some real problem.

Right, however I'm not talking about the alignment of the pointer on the stack, but about the value of the pointer, which depends on the offset inside the device tree blob of the property. If I use this:

    val = fdt_getprop(gd->fdt_blob, offset, "reg", &len)
    gd->ram_size = fdt64_to_cpu(*(fdt64_t *)val)

when the CPU tries to dereference val (which is something like 0x00000000010429e4) an alignment fault is generated for the reason stated above.

Quoted 5 lines

Quoted 2 lines

and therefore a 32-bit type must be used to avoid alignment faults. Probably the comment should be updated to explain this better.

Take a look at what uniphier does : arch/arm/mach-uniphier/dram_init.c Does that approach with fdt64_t work for you?

Nope, that's the first thing I've tried :(

Beniamino

Quoted 24 lines · Beniamino Galvani

Quoted 5 lines · Marek Vasut

Quoted 3 lines

So, after some investigation, the reason is that the code runs when caches are still disabled and thus all the memory is treated as Device-nGnRnE, requiring aligned accesses.

You mean 8-byte aligned accesses, correct ?

Yes.

Quoted 7 lines

Quoted 3 lines

The return value of
fdt_getprop() is guaranteed to be aligned to a 4 byte boundary (but not 8)

The return value of fdt_getprop() is a pointer, thus 8byte long on aarch64 and thus aligned to 8 bytes on the stack unless there is some real problem.

Right, however I'm not talking about the alignment of the pointer on the stack, but about the value of the pointer, which depends on the offset inside the device tree blob of the property. If I use this:

    val = fdt_getprop(gd->fdt_blob, offset, "reg", &len)
    gd->ram_size = fdt64_to_cpu(*(fdt64_t *)val)

Quoted 3 lines

when the CPU tries to dereference val (which is something like 0x00000000010429e4) an alignment fault is generated for the reason stated above.

Oh, now it's clear what the problem is, thanks. But then, we'd need such fixups all over the place I'm afraid. Isn't there some way to enable support for "unaligned" accesses instead?

Quoted 25 lines · Marek Vasut

Quoted 24 lines · Beniamino Galvani

Quoted 5 lines · Marek Vasut

Quoted 3 lines

So, after some investigation, the reason is that the code runs when caches are still disabled and thus all the memory is treated as Device-nGnRnE, requiring aligned accesses.

You mean 8-byte aligned accesses, correct ?

Yes.

Quoted 7 lines

Quoted 3 lines

The return value of
fdt_getprop() is guaranteed to be aligned to a 4 byte boundary (but not 8)

The return value of fdt_getprop() is a pointer, thus 8byte long on aarch64 and thus aligned to 8 bytes on the stack unless there is some real problem.

Right, however I'm not talking about the alignment of the pointer on the stack, but about the value of the pointer, which depends on the offset inside the device tree blob of the property. If I use this:

    val = fdt_getprop(gd->fdt_blob, offset, "reg", &len)
    gd->ram_size = fdt64_to_cpu(*(fdt64_t *)val)

Quoted 7 lines

Quoted 3 lines

when the CPU tries to dereference val (which is something like 0x00000000010429e4) an alignment fault is generated for the reason stated above.

Oh, now it's clear what the problem is, thanks. But then, we'd need such fixups all over the place I'm afraid. Isn't there some way to enable support for "unaligned" accesses instead?

Yes, and it's called "enable the MMU". You could probably do this in the early dram init stage already, but I'm not sure it's worth it. The NXP people are the only ones doing it really early today FWIW.

Also, if you find it more readable, you could just use get_unaligned_be64(). It gets you down to byte accesses rather than 32bit fetches, but the function name makes it pretty obvious what we're looking at.

Quoted 38 lines · Alexander Graf

Quoted 25 lines · Marek Vasut

Quoted 24 lines · Beniamino Galvani

Quoted 5 lines · Marek Vasut

Quoted 3 lines

So, after some investigation, the reason is that the code runs when caches are still disabled and thus all the memory is treated as Device-nGnRnE, requiring aligned accesses.

You mean 8-byte aligned accesses, correct ?

Yes.

Quoted 7 lines

Quoted 3 lines

The return value of
fdt_getprop() is guaranteed to be aligned to a 4 byte boundary (but not 8)

The return value of fdt_getprop() is a pointer, thus 8byte long on aarch64 and thus aligned to 8 bytes on the stack unless there is some real problem.

Right, however I'm not talking about the alignment of the pointer on the stack, but about the value of the pointer, which depends on the offset inside the device tree blob of the property. If I use this:

    val = fdt_getprop(gd->fdt_blob, offset, "reg", &len)
    gd->ram_size = fdt64_to_cpu(*(fdt64_t *)val)

Quoted 7 lines

Quoted 3 lines

when the CPU tries to dereference val (which is something like 0x00000000010429e4) an alignment fault is generated for the reason stated above.

Oh, now it's clear what the problem is, thanks. But then, we'd need such fixups all over the place I'm afraid. Isn't there some way to enable support for "unaligned" accesses instead?

Yes, and it's called "enable the MMU". You could probably do this in the early dram init stage already, but I'm not sure it's worth it. The NXP people are the only ones doing it really early today FWIW.

Quoted 4 lines

Also, if you find it more readable, you could just use get_unaligned_be64(). It gets you down to byte accesses rather than 32bit fetches, but the function name makes it pretty obvious what we're looking at.

OK, now I'm starting to get nightmares back to our last unaligned access discussion. Is ARMv8 doing something radically different from ARMv7 here, wrt unaligned accesses?

Quoted 44 lines · Tom Rini

Quoted 42 lines · Alexander Graf

Quoted 25 lines · Marek Vasut

Quoted 24 lines · Beniamino Galvani

Quoted 5 lines · Marek Vasut

Quoted 3 lines

So, after some investigation, the reason is that the code runs when caches are still disabled and thus all the memory is treated as Device-nGnRnE, requiring aligned accesses.

You mean 8-byte aligned accesses, correct ?

Yes.

Quoted 7 lines

Quoted 3 lines

The return value of
fdt_getprop() is guaranteed to be aligned to a 4 byte boundary (but not 8)

The return value of fdt_getprop() is a pointer, thus 8byte long on aarch64 and thus aligned to 8 bytes on the stack unless there is some real problem.

Right, however I'm not talking about the alignment of the pointer on the stack, but about the value of the pointer, which depends on the offset inside the device tree blob of the property. If I use this:

    val = fdt_getprop(gd->fdt_blob, offset, "reg", &len)
    gd->ram_size = fdt64_to_cpu(*(fdt64_t *)val)

Quoted 7 lines

Quoted 3 lines

when the CPU tries to dereference val (which is something like 0x00000000010429e4) an alignment fault is generated for the reason stated above.

Oh, now it's clear what the problem is, thanks. But then, we'd need such fixups all over the place I'm afraid. Isn't there some way to enable support for "unaligned" accesses instead?

Yes, and it's called "enable the MMU". You could probably do this in the early dram init stage already, but I'm not sure it's worth it. The NXP people are the only ones doing it really early today FWIW.

Also, if you find it more readable, you could just use get_unaligned_be64(). It gets you down to byte accesses rather than 32bit fetches, but the function name makes it pretty obvious what we're looking at.

Quoted 3 lines

OK, now I'm starting to get nightmares back to our last unaligned access discussion. Is ARMv8 doing something radically different from ARMv7 here, wrt unaligned accesses?

No, it does the same. To handle not naturally memory accesses you need to have dcache enabled and to enable the dcache the MMU needs to be turned on. ARMv7 is the same for all I'm aware of.

Quoted 51 lines · Alexander Graf

Quoted 47 lines · Tom Rini

Quoted 34 lines · Alexander Graf

Quoted 25 lines · Marek Vasut

Quoted 24 lines · Beniamino Galvani

Quoted 5 lines · Marek Vasut

Quoted 3 lines

So, after some investigation, the reason is that the code runs when caches are still disabled and thus all the memory is treated as Device-nGnRnE, requiring aligned accesses.

You mean 8-byte aligned accesses, correct ?

Yes.

Quoted 7 lines

Quoted 3 lines

The return value of
fdt_getprop() is guaranteed to be aligned to a 4 byte boundary (but not 8)

The return value of fdt_getprop() is a pointer, thus 8byte long on aarch64 and thus aligned to 8 bytes on the stack unless there is some real problem.

Right, however I'm not talking about the alignment of the pointer on the stack, but about the value of the pointer, which depends on the offset inside the device tree blob of the property. If I use this:

    val = fdt_getprop(gd->fdt_blob, offset, "reg", &len)
    gd->ram_size = fdt64_to_cpu(*(fdt64_t *)val)

Quoted 7 lines

Quoted 3 lines

when the CPU tries to dereference val (which is something like 0x00000000010429e4) an alignment fault is generated for the reason stated above.

Oh, now it's clear what the problem is, thanks. But then, we'd need such fixups all over the place I'm afraid. Isn't there some way to enable support for "unaligned" accesses instead?

Quoted 8 lines

Yes, and it's called "enable the MMU". You could probably do this in the early dram init stage already, but I'm not sure it's worth it. The NXP people are the only ones doing it really early today FWIW.

Also, if you find it more readable, you could just use get_unaligned_be64(). It gets you down to byte accesses rather than 32bit fetches, but the function name makes it pretty obvious what we're looking at.

OK, now I'm starting to get nightmares back to our last unaligned access discussion. Is ARMv8 doing something radically different from ARMv7 here, wrt unaligned accesses?

Quoted 3 lines

No, it does the same. To handle not naturally memory accesses you need to have dcache enabled and to enable the dcache the MMU needs to be turned on. ARMv7 is the same for all I'm aware of.

Ah, OK, so we just need to get the MMU on for ARMv8, and that's more complex? Or am I just flat out missing something?

Quoted 56 lines · Tom Rini

Quoted 54 lines · Alexander Graf

Quoted 39 lines · Tom Rini

Quoted 38 lines · Alexander Graf

Quoted 25 lines · Marek Vasut

Quoted 24 lines · Beniamino Galvani

Quoted 5 lines · Marek Vasut

Quoted 3 lines

So, after some investigation, the reason is that the code runs when caches are still disabled and thus all the memory is treated as Device-nGnRnE, requiring aligned accesses.

You mean 8-byte aligned accesses, correct ?

Yes.

Quoted 7 lines

Quoted 3 lines

The return value of
fdt_getprop() is guaranteed to be aligned to a 4 byte boundary (but not 8)

The return value of fdt_getprop() is a pointer, thus 8byte long on aarch64 and thus aligned to 8 bytes on the stack unless there is some real problem.

Right, however I'm not talking about the alignment of the pointer on the stack, but about the value of the pointer, which depends on the offset inside the device tree blob of the property. If I use this:

    val = fdt_getprop(gd->fdt_blob, offset, "reg", &len)
    gd->ram_size = fdt64_to_cpu(*(fdt64_t *)val)

Quoted 7 lines

Quoted 3 lines

when the CPU tries to dereference val (which is something like 0x00000000010429e4) an alignment fault is generated for the reason stated above.

Oh, now it's clear what the problem is, thanks. But then, we'd need such fixups all over the place I'm afraid. Isn't there some way to enable support for "unaligned" accesses instead?

Yes, and it's called "enable the MMU". You could probably do this in the early dram init stage already, but I'm not sure it's worth it. The NXP people are the only ones doing it really early today FWIW.

Quoted 8 lines

Quoted 4 lines

Also, if you find it more readable, you could just use get_unaligned_be64(). It gets you down to byte accesses rather than 32bit fetches, but the function name makes it pretty obvious what we're looking at.

OK, now I'm starting to get nightmares back to our last unaligned access discussion. Is ARMv8 doing something radically different from ARMv7 here, wrt unaligned accesses?

No, it does the same. To handle not naturally memory accesses you need to have dcache enabled and to enable the dcache the MMU needs to be turned on. ARMv7 is the same for all I'm aware of.

Quoted 3 lines

Ah, OK, so we just need to get the MMU on for ARMv8, and that's more complex? Or am I just flat out missing something?

It's not terribly complex, but the code in question runs in very early init (it initializes dram). I guess people usually like their caches off that early.

Quoted 63 lines · Alexander Graf

Quoted 59 lines · Tom Rini

Quoted 47 lines · Alexander Graf

Quoted 44 lines · Tom Rini

Quoted 42 lines · Alexander Graf

Quoted 25 lines · Marek Vasut

Quoted 24 lines · Beniamino Galvani

Quoted 5 lines · Marek Vasut

Quoted 3 lines

So, after some investigation, the reason is that the code runs when caches are still disabled and thus all the memory is treated as Device-nGnRnE, requiring aligned accesses.

You mean 8-byte aligned accesses, correct ?

Yes.

Quoted 7 lines

Quoted 3 lines

The return value of
fdt_getprop() is guaranteed to be aligned to a 4 byte boundary (but not 8)

The return value of fdt_getprop() is a pointer, thus 8byte long on aarch64 and thus aligned to 8 bytes on the stack unless there is some real problem.

Right, however I'm not talking about the alignment of the pointer on the stack, but about the value of the pointer, which depends on the offset inside the device tree blob of the property. If I use this:

    val = fdt_getprop(gd->fdt_blob, offset, "reg", &len)
    gd->ram_size = fdt64_to_cpu(*(fdt64_t *)val)

Quoted 7 lines

Quoted 3 lines

when the CPU tries to dereference val (which is something like 0x00000000010429e4) an alignment fault is generated for the reason stated above.

Oh, now it's clear what the problem is, thanks. But then, we'd need such fixups all over the place I'm afraid. Isn't there some way to enable support for "unaligned" accesses instead?

Yes, and it's called "enable the MMU". You could probably do this in the early dram init stage already, but I'm not sure it's worth it. The NXP people are the only ones doing it really early today FWIW.

Also, if you find it more readable, you could just use get_unaligned_be64(). It gets you down to byte accesses rather than 32bit fetches, but the function name makes it pretty obvious what we're looking at.

Quoted 7 lines

Quoted 3 lines

OK, now I'm starting to get nightmares back to our last unaligned access discussion. Is ARMv8 doing something radically different from ARMv7 here, wrt unaligned accesses?

No, it does the same. To handle not naturally memory accesses you need to have dcache enabled and to enable the dcache the MMU needs to be turned on. ARMv7 is the same for all I'm aware of.

Ah, OK, so we just need to get the MMU on for ARMv8, and that's more complex? Or am I just flat out missing something?

Quoted 3 lines

It's not terribly complex, but the code in question runs in very early init (it initializes dram). I guess people usually like their caches off that early.

Caches and MMU are two orthogonal things though.