soc: litex-vexriscv: Rewrite litex_read/write

Changes signature so it takes uint32_t instead of pointer to a
register.
Later `sys_read*` and `sys_write*` functions are used, which cast
given address to volatile pointer anyway.

This required changing types of some fields in LiteX GPIO driver and
removal of two casts in clock control driver.

There was a weird assert from LiteX GPIO driver, which checked whether
size of first register in dts was a multiple of 4.
It didn't make much sense, so I removed it.

Previous dts was describing size of a register in terms of subregisters
used. New one uses size of register, so right now it is almost always
4 bytes.

Most drivers don't read register size from dts anyway, so only changes
had to be made in GPIO and clock control drivers.

Both use `litex_read` and `litex_write` to operate on `n`bytes.
Now GPIO driver calculates this `n` value in compile time from given
number of pins and stores it in `reg_size` field of config struct like
before.

Registe sizes in clock control driver are hardcoded, because they are
tied to LiteX wrapper anyway.

This makes it possible to have code, independent of CSR data width.

Signed-off-by: Michal Sieron <[email protected]>

Author: michalsieron

drivers/clock_control/clock_control_litex.c

@@ -25,14 +25,14 @@ static struct litex_clk_clkout *clkouts;/* clkout array for whole driver */
 
 /* All DRP regs addresses and sizes */
 static struct litex_drp_reg drp[] = {
-	{DRP_ADDR_RESET,  DRP_SIZE_RESET},
-	{DRP_ADDR_LOCKED, DRP_SIZE_LOCKED},
-	{DRP_ADDR_READ,   DRP_SIZE_READ},
-	{DRP_ADDR_WRITE,  DRP_SIZE_WRITE},
-	{DRP_ADDR_DRDY,   DRP_SIZE_DRDY},
-	{DRP_ADDR_ADR,    DRP_SIZE_ADR},
-	{DRP_ADDR_DAT_W,  DRP_SIZE_DAT_W},
-	{DRP_ADDR_DAT_R,  DRP_SIZE_DAT_R},
+	{DRP_ADDR_RESET,  1},
+	{DRP_ADDR_LOCKED, 1},
+	{DRP_ADDR_READ,   1},
+	{DRP_ADDR_WRITE,  1},
+	{DRP_ADDR_DRDY,   1},
+	{DRP_ADDR_ADR,    1},
+	{DRP_ADDR_DAT_W,  2},
+	{DRP_ADDR_DAT_R,  2},
 };
 
 struct litex_clk_regs_addr litex_clk_regs_addr_init(void)
@@ -219,12 +219,12 @@ static inline uint64_t litex_clk_lookup_lock(uint32_t glob_mul)
 
 static inline void litex_clk_set_reg(uint32_t reg, uint32_t val)
 {
-	litex_write((uint32_t *)drp[reg].addr, drp[reg].size, val);
+	litex_write(drp[reg].addr, drp[reg].size, val);
 }
 
 static inline uint32_t litex_clk_get_reg(uint32_t reg)
 {
-	return litex_read((uint32_t *)drp[reg].addr, drp[reg].size);
+	return litex_read(drp[reg].addr, drp[reg].size);
 }
 
 static inline void litex_clk_assert_reg(uint32_t reg)

drivers/clock_control/clock_control_litex.h

@@ -41,15 +41,6 @@
 #define DRP_ADDR_ADR		DT_REG_ADDR_BY_IDX(MMCM, 5/*DRP_ADR*/)
 #define DRP_ADDR_DAT_W		DT_REG_ADDR_BY_IDX(MMCM, 6/*DRP_DAT_W*/)
 #define DRP_ADDR_DAT_R		DT_REG_ADDR_BY_IDX(MMCM, 7/*DRP_DAT_R*/)
-/* Register size */
-#define DRP_SIZE_RESET		DT_REG_SIZE_BY_IDX(MMCM, 0/*DRP_RESET*/)
-#define DRP_SIZE_LOCKED		DT_REG_SIZE_BY_IDX(MMCM, 1/*DRP_LOCKED*/)
-#define DRP_SIZE_READ		DT_REG_SIZE_BY_IDX(MMCM, 2/*DRP_READ*/)
-#define DRP_SIZE_WRITE		DT_REG_SIZE_BY_IDX(MMCM, 3/*DRP_WRITE*/)
-#define DRP_SIZE_DRDY		DT_REG_SIZE_BY_IDX(MMCM, 4/*DRP_DRDY*/)
-#define DRP_SIZE_ADR		DT_REG_SIZE_BY_IDX(MMCM, 5/*DRP_ADR*/)
-#define DRP_SIZE_DAT_W		DT_REG_SIZE_BY_IDX(MMCM, 6/*DRP_DAT_W*/)
-#define DRP_SIZE_DAT_R		DT_REG_SIZE_BY_IDX(MMCM, 7/*DRP_DAT_R*/)
 
 /* Devicetree global defines */
 #define LOCK_TIMEOUT		DT_PROP(MMCM, litex_lock_timeout)

drivers/gpio/gpio_litex.c

@@ -33,12 +33,12 @@ static const char *LITEX_LOG_CANNOT_CHANGE_DIR =
 	"Cannot change port direction selected in device tree\n";
 
 struct gpio_litex_cfg {
-	volatile uint32_t *reg_addr;
+	uint32_t reg_addr;
 	int reg_size;
-	volatile uint32_t *ev_pending_addr;
-	volatile uint32_t *ev_enable_addr;
-	volatile uint32_t *ev_mode_addr;
-	volatile uint32_t *ev_edge_addr;
+	uint32_t ev_pending_addr;
+	uint32_t ev_enable_addr;
+	uint32_t ev_mode_addr;
+	uint32_t ev_edge_addr;
 	int nr_gpios;
 	bool port_is_output;
 };
@@ -288,24 +288,16 @@ static const struct gpio_driver_api gpio_litex_driver_api = {
 
 #define GPIO_LITEX_INIT(n) \
 	static int gpio_litex_port_init_##n(const struct device *dev); \
-	BUILD_ASSERT(DT_INST_REG_SIZE(n) != 0 \
-		     && DT_INST_REG_SIZE(n) % 4 == 0, \
-		     "Register size must be a multiple of 4"); \
 \
 	static const struct gpio_litex_cfg gpio_litex_cfg_##n = { \
-		.reg_addr = \
-		(volatile uint32_t *) DT_INST_REG_ADDR(n), \
-		.reg_size = DT_INST_REG_SIZE(n) / 4, \
+		.reg_addr = DT_INST_REG_ADDR(n), \
+		.reg_size = DT_INST_REG_SIZE(n), \
 		.nr_gpios = DT_INST_PROP(n, ngpios), \
 		IF_ENABLED(DT_INST_IRQ_HAS_IDX(n, 0), ( \
-			.ev_mode_addr = \
-			(volatile uint32_t *) DT_INST_REG_ADDR_BY_NAME(n, irq_mode), \
-			.ev_edge_addr = \
-			(volatile uint32_t *) DT_INST_REG_ADDR_BY_NAME(n, irq_edge), \
-			.ev_pending_addr = \
-			(volatile uint32_t *) DT_INST_REG_ADDR_BY_NAME(n, irq_pend), \
-			.ev_enable_addr = \
-			(volatile uint32_t *) DT_INST_REG_ADDR_BY_NAME(n, irq_en), \
+			.ev_mode_addr    = DT_INST_REG_ADDR_BY_NAME(n, irq_mode), \
+			.ev_edge_addr    = DT_INST_REG_ADDR_BY_NAME(n, irq_edge), \
+			.ev_pending_addr = DT_INST_REG_ADDR_BY_NAME(n, irq_pend), \
+			.ev_enable_addr  = DT_INST_REG_ADDR_BY_NAME(n, irq_en), \
 		)) \
 		.port_is_output = DT_INST_PROP(n, port_is_output), \
 	}; \
@@ -325,8 +317,11 @@ static const struct gpio_driver_api gpio_litex_driver_api = {
 	{ \
 		const struct gpio_litex_cfg *gpio_config = DEV_GPIO_CFG(dev); \
 \
-		/* each 4-byte register is able to handle 8 GPIO pins */ \
-		if (gpio_config->nr_gpios > (gpio_config->reg_size * 8)) { \
+		/* Check if gpios fit in declared register space */ \
+		/* Number of subregisters times size in bits */ \
+		const int max_gpios_can_fit = DT_INST_REG_SIZE(n) / 4 \
+					* CONFIG_LITEX_CSR_DATA_WIDTH; \
+		if (gpio_config->nr_gpios > max_gpios_can_fit) { \
 			LOG_ERR("%s", LITEX_LOG_REG_SIZE_NGPIOS_MISMATCH); \
 			return -EINVAL; \
 		} \

dts/riscv/riscv32-litex-vexriscv.dtsi

@@ -115,10 +115,10 @@
 		gpio_in: gpio@e0006000 {
 			compatible = "litex,gpio";
 			reg = <0xe0006000 0x4
-				0xe0006004 0x1
-				0xe0006008 0x1
-				0xe0006010 0x1
-				0xe0006014 0x1>;
+				0xe0006004 0x4
+				0xe0006008 0x4
+				0xe0006010 0x4
+				0xe0006014 0x4>;
 			interrupt-parent = <&intc0>;
 			interrupts = <4 2>;
 			reg-names = "base",
@@ -204,14 +204,14 @@
 		clock0: clock@82005000 {
 			compatible = "litex,clk";
 			label = "clock0";
-			reg = <0x82005000 0x1
-			       0x82005004 0x1
-			       0x82005008 0x1
-			       0x8200500c 0x1
-			       0x82005010 0x1
-			       0x82005014 0x1
-			       0x82005018 0x2
-			       0x82005020 0x2>;
+			reg = <0x82005000 0x4
+			       0x82005004 0x4
+			       0x82005008 0x4
+			       0x8200500c 0x4
+			       0x82005010 0x4
+			       0x82005014 0x4
+			       0x82005018 0x8
+			       0x82005020 0x8>;
 			reg-names = "drp_reset",
 				    "drp_locked",
 				    "drp_read",

soc/riscv/litex-vexriscv/soc.h

@@ -10,9 +10,6 @@
 #include "../riscv-privilege/common/soc_common.h"
 #include <devicetree.h>
 
-#define LITEX_SUBREG_SIZE          0x1
-#define LITEX_SUBREG_SIZE_BIT      (LITEX_SUBREG_SIZE * 8)
-
 /* lib-c hooks required RAM defined variables */
 #define RISCV_RAM_BASE              DT_REG_ADDR(DT_INST(0, mmio_sram))
 #define RISCV_RAM_SIZE              DT_REG_SIZE(DT_INST(0, mmio_sram))
@@ -110,30 +107,45 @@ static inline void litex_write32(unsigned int value, unsigned long addr)
 #endif
 }
 
-static inline void litex_write(volatile uint32_t *reg, uint32_t reg_size, uint32_t val)
+/*
+ * Operates on uint32_t values only
+ * Size is in bytes and meaningful are 1, 2 or 4
+ * Address must be aligned to 4 bytes
+ */
+static inline void litex_write(uint32_t addr, uint32_t size, uint32_t value)
 {
-	uint32_t shifted_data, i;
-	volatile uint32_t *reg_addr;
-
-	for (i = 0; i < reg_size; ++i) {
-		shifted_data = val >> ((reg_size - i - 1) *
-					LITEX_SUBREG_SIZE_BIT);
-		reg_addr = ((volatile uint32_t *) reg) + i;
-		*(reg_addr) = shifted_data;
+	switch (size) {
+	case 1:
+		litex_write8(value, addr);
+		break;
+	case 2:
+		litex_write16(value, addr);
+		break;
+	case 4:
+		litex_write32(value, addr);
+		break;
+	default:
+		break;
 	}
 }
 
-static inline uint32_t litex_read(volatile uint32_t *reg, uint32_t reg_size)
+/*
+ * Operates on uint32_t values only
+ * Size is in bytes and meaningful are 1, 2 or 4
+ * Address must be aligned to 4 bytes
+ */
+static inline uint32_t litex_read(uint32_t addr, uint32_t size)
 {
-	uint32_t shifted_data, i, result = 0;
-
-	for (i = 0; i < reg_size; ++i) {
-		shifted_data = *(reg + i) << ((reg_size - i - 1) *
-						LITEX_SUBREG_SIZE_BIT);
-		result |= shifted_data;
+	switch (size) {
+	case 1:
+		return litex_read8(addr);
+	case 2:
+		return litex_read16(addr);
+	case 4:
+		return litex_read32(addr);
+	default:
+		return 0;
 	}
-
-	return result;
 }
 
 #endif /* _ASMLANGUAGE */