Hello,
On 12/10/2016 05:01 AM, Marek Vasut wrote:
> 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 ?
I cross compile the kernel on a 64bit host and then upload on the
target. What kind of problem are you forseeing ?
>
>> + while (len > 3) {
>> + *(u32 *)buf = readl(src);
>
> ioread32_rep() to avoid open-coding the loop .
ok.
>> + 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.
ok.
>
>> +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 :)
yeah
>> +#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 ?
yes.
I rather keep the initial macros though, which I found easier to
understand.
The Segment Register uses a 8MB unit to encode the start address
and the end address of the mapping window of a flash SPI slave :
| byte 1 | byte 2 | byte 3 | byte 4 |
+--------+--------+--------+--------+
| end | start | 0 | 0 |
>> +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.
ah yes. I missed that one.
>
>> + }
>> +
>> + /*
>> + * 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 :)
Thanks for the review,
C.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
