On Monday, July 27, 2015 at 10:41:37 AM, Cyrille Pitchen wrote: > Hi Marek, > > Le 22/07/2015 15:50, Marek Vasut a écrit : > > On Wednesday, July 22, 2015 at 03:17:10 PM, Cyrille Pitchen wrote: > >> This driver add support to the new Atmel QSPI controller embedded into > >> sama5d2x SoCs. It expects a NOR memory to be connected to the QSPI > >> controller. > >> > >> Signed-off-by: Cyrille Pitchen <[email protected]> > >> --- > > > > [...] > > > >> +/* QSPI register offsets */ > >> +#define QSPI_CR 0x0000 /* Control Register */ > >> +#define QSPI_MR 0x0004 /* Mode Register */ > >> +#define QSPI_RD 0x0008 /* Receive Data Register */ > >> +#define QSPI_TD 0x000c /* Transmit Data Register */ > >> +#define QSPI_SR 0x0010 /* Status Register */ > >> +#define QSPI_IER 0x0014 /* Interrupt Enable Register */ > >> +#define QSPI_IDR 0x0018 /* Interrupt Disable Register */ > >> +#define QSPI_IMR 0x001c /* Interrupt Mask Register */ > >> +#define QSPI_SCR 0x0020 /* Serial Clock Register */ > >> + > >> +#define QSPI_IAR 0x0030 /* Instruction Address Register */ > >> +#define QSPI_ICR 0x0034 /* Instruction Code Register */ > >> +#define QSPI_IFR 0x0038 /* Instruction Frame Register */ > >> + > >> +#define QSPI_SMR 0x0040 /* Scrambling Mode Register */ > >> +#define QSPI_SKR 0x0044 /* Scrambling Key Register */ > >> + > >> +#define QSPI_WPMR 0x00E4 /* Write Protection Mode Register */ > >> +#define QSPI_WPSR 0x00E8 /* Write Protection Status Register */ > >> + > >> +#define QSPI_VERSION 0x00FC /* Version Register */ > >> + > >> +/* Bitfields in QSPI_CR (Control Register) */ > >> +#define QSPI_CR_QSPIEN_OFFSET 0 > >> +#define QSPI_CR_QSPIEN_SIZE 1 > >> +#define QSPI_CR_QSPIDIS_OFFSET 1 > >> +#define QSPI_CR_QSPIDIS_SIZE 1 > >> +#define QSPI_CR_SWRST_OFFSET 7 > >> +#define QSPI_CR_SWRST_SIZE 1 > >> +#define QSPI_CR_LASTXFER_OFFSET 24 > >> +#define QSPI_CR_LASTXFER_SIZE 1 > >> + > >> +/* Bitfields in QSPI_MR (Mode Register) */ > >> +#define QSPI_MR_SSM_OFFSET 0 > >> +#define QSPI_MR_SSM_SIZE 1 > >> +#define QSPI_MR_LLB_OFFSET 1 > >> +#define QSPI_MR_LLB_SIZE 1 > >> +#define QSPI_MR_WDRBT_OFFSET 2 > >> +#define QSPI_MR_WDRBT_SIZE 1 > >> +#define QPSI_MR_SMRM_OFFSET 3 > >> +#define QSPI_MR_SMRM_SIZE 1 > >> +#define QSPI_MR_CSMODE_OFFSET 4 > >> +#define QSPI_MR_CSMODE_SIZE 2 > >> +#define QSPI_MR_NBBITS_OFFSET 8 > >> +#define QSPI_MR_NBBITS_SIZE 4 > >> +#define QSPI_MR_NBBITS_8_BIT 0 > >> +#define QSPI_MR_NBBITS_9_BIT 1 > >> +#define QSPI_MR_NBBITS_10_BIT 2 > >> +#define QSPI_MR_NBBITS_11_BIT 3 > >> +#define QSPI_MR_NBBITS_12_BIT 4 > >> +#define QSPI_MR_NBBITS_13_BIT 5 > >> +#define QSPI_MR_NBBITS_14_BIT 6 > >> +#define QSPI_MR_NBBITS_15_BIT 7 > >> +#define QSPI_MR_NBBITS_16_BIT 8 > > > > You might want to turn this into something like: > > > > #define QSPI_NR_NBBITS(n) ((n) - 8) > > done in the next series > > >> +#define QSPI_MR_DLYBCT_OFFSET 16 > >> +#define QSPI_MR_DLYBCT_SIZE 8 > >> +#define QSPI_MR_DLYCS_OFFSET 24 > >> +#define QSPI_MR_DLYCS_SIZE 8 > > > > [...] > > > >> +/* Bitfields in QSPI_IFR (Instruction Frame Register) */ > >> +#define QSPI_IFR_WIDTH_OFFSET 0 > >> +#define QSPI_IFR_WIDTH_SIZE 3 > >> +#define QSPI_IFR_WIDTH_SINGLE_BIT_SPI 0 > >> +#define QSPI_IFR_WIDTH_DUAL_OUTPUT 1 > >> +#define QSPI_IFR_WIDTH_QUAD_OUTPUT 2 > >> +#define QSPI_IFR_WIDTH_DUAL_IO 3 > >> +#define QSPI_IFR_WIDTH_QUAD_IO 4 > >> +#define QSPI_IFR_WIDTH_DUAL_CMD 5 > >> +#define QSPI_IFR_WIDTH_QUAD_CMD 6 > > > > Please use #define[SPACE] instead of inconsistent #define[TAB] > > done in the next series. I also use BIT and GENMASK macros as much as > possible to define the register bit fields. > > > [...] > > > >> +/* Bit manipulation macros */ > >> +#define QSPI_BIT(name) \ > >> + (1 << QSPI_##name##_OFFSET) > >> +#define QSPI_BF(name, value) \ > >> + (((value) & ((1 << QSPI_##name##_SIZE) - 1)) << QSPI_##name##_OFFSET) > >> +#define QSPI_BFEXT(name, value) \ > >> + (((value) >> QSPI_##name##_OFFSET) & ((1 << QSPI_##name##_SIZE) - 1)) > >> +#define QSPI_BFINS(name, value, old) \ > >> + (((old) & ~(((1 << QSPI_##name##_SIZE) - 1) << QSPI_##name##_OFFSET)) > >> \ + | QSPI_BF(name, value)) > >> + > >> +/* Register access macros */ > >> +#define qspi_readl(port, reg) \ > >> + readl_relaxed((port)->regs + QSPI_##reg) > >> +#define qspi_writel(port, reg, value) \ > >> + writel_relaxed((value), (port)->regs + QSPI_##reg) > >> + > >> +#define qspi_readw(port, reg) \ > >> + readw_relaxed((port)->regs + QSPI_##reg) > >> +#define qspi_writew(port, reg, value) \ > >> + writew_relaxed((value), (port)->regs + QSPI_##reg) > >> + > >> +#define qspi_readb(port, reg) \ > >> + readb_relaxed((port)->regs + QSPI_##reg) > >> +#define qspi_writeb(port, reg, value) \ > >> + writeb_relaxed((value), (port)->regs + QSPI_##reg) > > > > I cannot say I'd be very fond of those preprocessor concatenations. Can't > > you do something about them? It's really hard to look for symbols which > > are in fact result of this horrible macro voodoo . > > I agree with you. I did it this way at first to make it consistent with > other Atmel drivers which use such macros but we tend to remove them > progressively as they prevent from using ctags for instance. > > >> +struct atmel_qspi { > >> + void __iomem *regs; > >> + void __iomem *mem; > >> + dma_addr_t phys_addr; > >> + struct dma_chan *chan; > >> + struct clk *clk; > >> + struct platform_device *pdev; > >> + u32 ifr_width; > >> + u32 pending; > >> + > >> + struct mtd_info mtd; > >> + struct spi_nor nor; > >> + u32 clk_rate; > >> + struct completion completion; > >> + > >> +#ifdef DEBUG > >> + u8 last_instruction; > >> +#endif > >> +}; > > > > [...] > > > >> +#ifdef DEBUG > >> +static void atmel_qspi_debug_command(struct atmel_qspi *aq, > >> + const struct atmel_qspi_command *cmd) > >> +{ > >> + u8 cmd_buf[SPI_NOR_MAX_CMD_SIZE]; > >> + size_t len = 0; > >> + int i; > >> + > >> + if (cmd->enable.bits.instruction) { > >> + if (aq->last_instruction == cmd->instruction) > >> + return; > >> + aq->last_instruction = cmd->instruction; > >> + } > >> + > >> + if (cmd->enable.bits.instruction) > >> + cmd_buf[len++] = cmd->instruction; > >> + > >> + for (i = cmd->enable.bits.address-1; i >= 0; --i) > >> + cmd_buf[len++] = (cmd->address >> (i << 3)) & 0xff; > >> + > >> + if (cmd->enable.bits.mode) > >> + cmd_buf[len++] = cmd->mode; > >> + > >> + if (cmd->enable.bits.dummy) { > >> + int num = cmd->num_dummy_cycles; > >> + > >> + switch (aq->ifr_width) { > >> + case QSPI_IFR_WIDTH_SINGLE_BIT_SPI: > >> + case QSPI_IFR_WIDTH_DUAL_OUTPUT: > >> + case QSPI_IFR_WIDTH_QUAD_OUTPUT: > >> + num >>= 3; > >> + break; > >> + case QSPI_IFR_WIDTH_DUAL_IO: > >> + case QSPI_IFR_WIDTH_DUAL_CMD: > >> + num >>= 2; > >> + break; > >> + case QSPI_IFR_WIDTH_QUAD_IO: > >> + case QSPI_IFR_WIDTH_QUAD_CMD: > >> + num >>= 1; > >> + break; > >> + default: > >> + return; > >> + } > >> + > >> + for (i = 0; i < num; ++i) > >> + cmd_buf[len++] = 0; > >> + } > >> + > >> + print_hex_dump(KERN_DEBUG, "qspi cmd: ", DUMP_PREFIX_NONE, > >> + 32, 1, cmd_buf, len, false); > >> + > >> +#ifdef VERBOSE_DEBUG > > > > The print_hex_dump() is already KERN_DEBUG, so I don't think there's any > > need to introduce yet another preprocessor check here. > > Indeed, there is only one debug level for printk() and print_hex_dump(): > KERN_DEBUG. However I've used the DEBUG and VERBOSE_DEBUG macros to have > two levels of debug output. When the DEBUG macro is defined the driver > prints the QSPI command. When both the DEBUG and VERBOSE_DEBUG macro are > defined, the driver also prints the RX or TX data following the commands. > Depending on the command to debug, data can be usefull, as for the READ ID > command, or really annoying as for big Fast Read commands. > > If it's fine with you, I'd rather keep these two debug levels.
I'm not the one to decide, but it does indeed make sense. Thanks a lot for doing the cleanups ! -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
