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.
>> + if (cmd->enable.bits.data && cmd->tx_buf)
>> + print_hex_dump(KERN_DEBUG, "qspi tx : ", DUMP_PREFIX_NONE,
>> + 32, 1, cmd->tx_buf, cmd->buf_len, false);
>> +#endif
>> +}
>> +#else
>> +#define atmel_qspi_debug_command(aq, cmd)
>> +#endif
>
> [...]
>
Best Regards,
Cyrille
--
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
