Re: [PATCH v2 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

[Marek Vasut <marex@xxxxxxx>]

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 <cyrille.pitchen@xxxxxxxxx>
> >> ---
> > 
> > [...]
> > 
> >> +/* 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html