[PATCH v3 1/5] mtd: spi-nor: add memory controllers for the Aspeed AST2500 SoC

[Marek Vasut]

On 12/09/2016 05:49 PM, Cédric Le Goater wrote:

[...]


> +static int aspeed_smc_read_from_ahb(void *buf, const void __iomem *src,
> +				    size_t len)
> +{
> +	if (IS_ALIGNED((u32)src, sizeof(u32)) &&
> +	    IS_ALIGNED((u32)buf, sizeof(u32)) &&
> +	    IS_ALIGNED(len, sizeof(u32))) {

Did you try compiling this on any 64bit system ?

> +		while (len > 3) {
> +			*(u32 *)buf = readl(src);

ioread32_rep() to avoid open-coding the loop .

> +			buf += 4;
> +			src += 4;
> +			len -= 4;
> +		}
> +	}
> +
> +	while (len--) {
> +		*(u8 *)buf = readb(src);
> +		buf += 1;
> +		src += 1;
> +	}
> +	return 0;
> +}
> +
> +static int aspeed_smc_write_to_ahb(void __iomem *dst, const void *buf,
> +				   size_t len)
> +{
> +	if (IS_ALIGNED((u32)dst, sizeof(u32)) &&
> +	    IS_ALIGNED((u32)buf, sizeof(u32)) &&
> +	    IS_ALIGNED(len, sizeof(u32))) {
> +		while (len > 3) {
> +			u32 val = *(u32 *)buf;
> +
> +			writel(val, dst);

iowrite32_rep()

> +			buf += 4;
> +			dst += 4;
> +			len -= 4;
> +		}
> +	}
> +
> +	while (len--) {
> +		u8 val = *(u8 *)buf;
> +
> +		writeb(val, dst);
> +		buf += 1;
> +		dst += 1;
> +	}
> +	return 0;
> +}
> +
> +/*
> + * The driver only support SPI flash
> + */
> +enum aspeed_smc_flash_type {
> +	smc_type_nor  = 0,
> +	smc_type_nand = 1,
> +	smc_type_spi  = 2,
> +};
> +
> +struct aspeed_smc_chip;
> +
> +struct aspeed_smc_info {
> +	u32 maxsize;		/* maximum size of chip window */
> +	u8 nce;			/* number of chip enables */
> +	bool hastype;		/* flash type field exists in config reg */
> +	u8 we0;			/* shift for write enable bit for CE0 */
> +	u8 ctl0;		/* offset in regs of ctl for CE0 */
> +
> +	void (*set_4b)(struct aspeed_smc_chip *chip);
> +};

Move all the structs to the beginning of the driver please.

> +static void aspeed_smc_chip_set_4b(struct aspeed_smc_chip *chip);
> +
> +static const struct aspeed_smc_info fmc_2500_info = {
> +	.maxsize = 256 * 1024 * 1024,
> +	.nce = 3,
> +	.hastype = true,
> +	.we0 = 16,
> +	.ctl0 = 0x10,
> +	.set_4b = aspeed_smc_chip_set_4b,
> +};
> +
> +static const struct aspeed_smc_info spi_2500_info = {
> +	.maxsize = 128 * 1024 * 1024,
> +	.nce = 2,
> +	.hastype = false,
> +	.we0 = 16,
> +	.ctl0 = 0x10,
> +	.set_4b = aspeed_smc_chip_set_4b,
> +};
> +
> +enum aspeed_smc_ctl_reg_value {
> +	smc_base,		/* base value without mode for other commands */
> +	smc_read,		/* command reg for (maybe fast) reads */
> +	smc_write,		/* command reg for writes */
> +	smc_max,
> +};

[...]

> +#define CONTROL_CE_STOP_ACTIVE_CONTROL	BIT(2)
> +#define CONTROL_COMMAND_MODE_MASK	GENMASK(1, 0)
> +#define CONTROL_COMMAND_MODE_NORMAL	(0)
> +#define CONTROL_COMMAND_MODE_FREAD	(1)
> +#define CONTROL_COMMAND_MODE_WRITE	(2)
> +#define CONTROL_COMMAND_MODE_USER	(3)

Drop the parenthesis around constants :)

> +#define CONTROL_KEEP_MASK						\
> +	(CONTROL_AAF_MODE | CONTROL_CE_INACTIVE_MASK | CONTROL_CLK_DIV4 | \
> +	 CONTROL_IO_DUMMY_MASK | CONTROL_CLOCK_FREQ_SEL_MASK |		\
> +	 CONTROL_LSB_FIRST | CONTROL_CLOCK_MODE_3)
> +
> +/*
> + * Segment Address Registers. Start and end addresses are encoded
> + * using 8MB units
> + */
> +#define SEGMENT_ADDR_REG0		0x30
> +#define SEGMENT_ADDR_START(_r)		((((_r) >> 16) & 0xFF) << 23)

is that ((r) & 0xff0000) << 7 ?

> +#define SEGMENT_ADDR_END(_r)		((((_r) >> 24) & 0xFF) << 23)

((r) & 0xff000000) >> 1 ?

> +static inline u32 aspeed_smc_chip_write_bit(struct aspeed_smc_chip *chip)
> +{
> +	return BIT(chip->controller->info->we0 + chip->cs);
> +}

[...]

> +static int aspeed_smc_chip_setup_init(struct aspeed_smc_chip *chip,
> +				      struct resource *res)
> +{
> +	struct aspeed_smc_controller *controller = chip->controller;
> +	const struct aspeed_smc_info *info = controller->info;
> +	u32 reg, base_reg;
> +
> +	/*
> +	 * Always turn on the write enable bit to allow opcodes to be
> +	 * sent in user mode.
> +	 */
> +	aspeed_smc_chip_enable_write(chip);
> +
> +	/* The driver only supports SPI type flash */
> +	if (info->hastype)
> +		aspeed_smc_chip_set_type(chip, smc_type_spi);
> +
> +	/*
> +	 * Configure chip base address in memory
> +	 */
> +	chip->ahb_base = aspeed_smc_chip_base(chip, res);
> +	if (!chip->ahb_base) {
> +		dev_warn(chip->nor.dev, "CE segment window closed.\n");
> +		return -1;

return -EINVAL ? Check whether you always use errno.h macros in returns.

> +	}
> +
> +	/*
> +	 * Get value of the inherited control register. U-Boot usually
> +	 * does some timing calibration on the FMC chip, so it's good
> +	 * to keep them. In the future, we should handle calibration
> +	 * from Linux.
> +	 */
> +	reg = readl(chip->ctl);
> +	dev_dbg(controller->dev, "control register: %08x\n", reg);
> +
> +	base_reg = reg & CONTROL_KEEP_MASK;
> +	if (base_reg != reg) {
> +		dev_info(controller->dev,
> +			 "control register changed to: %08x\n",
> +			 base_reg);
> +	}
> +	chip->ctl_val[smc_base] = base_reg;

[...]

Mostly minor nits, looks nice otherwise, thanks :)

-- 
Best regards,
Marek Vasut