[PATCH V7 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller.

[Vikas MANOCHA]

Hi,



> On Aug 20, 2015, at 10:20 PM, Marek Vasut <marex at denx.de> wrote:
> 
>> On Tuesday, August 18, 2015 at 04:34:53 AM, vikas wrote:
>> Hi Marek,
> 
> Hi,
> 
> [...]
> 
>>> +#define CQSPI_POLL_IDLE_RETRY                  3
>>> +
>>> +#define CQSPI_REG_SRAM_RESV_WORDS              2
>>> +#define CQSPI_REG_SRAM_PARTITION_WR            1
>> 
>> remove unused macros.
>> 
>>> +#define CQSPI_REG_SRAM_THRESHOLD_BYTES         50
>> 
>> the macro is used only for write sram watermark, something like
>> ...WR_THRESH_BYTES would be better. Infact instead of magic number like
>> 50, it should be based on sram_depth similar to read watermark
>> configuration.
> 
> I suppose if the fifo falls below 1/8, that might be a sensible time to
> trigger an underflow interrupt.
> 
>>> +
>>> +/* Instruction type */
>>> +#define CQSPI_INST_TYPE_SINGLE                 0
>>> +#define CQSPI_INST_TYPE_DUAL                   1
>>> +#define CQSPI_INST_TYPE_QUAD                   2
>>> +
>>> +#define CQSPI_DUMMY_CLKS_MAX                   31
>>> +
>>> +#define CQSPI_STIG_DATA_LEN_MAX                        8
>>> +
>>> +/* Register map */
>>> +#define CQSPI_REG_CONFIG                       0x00
>>> +#define CQSPI_REG_CONFIG_ENABLE_MASK           BIT(0)
>>> +#define CQSPI_REG_CONFIG_DECODE_MASK           BIT(9)
>>> +#define CQSPI_REG_CONFIG_CHIPSELECT_LSB                10
>>> +#define CQSPI_REG_CONFIG_DMA_MASK              BIT(15)
>>> +#define CQSPI_REG_CONFIG_BAUD_LSB              19
>>> +#define CQSPI_REG_CONFIG_IDLE_LSB              31
>> 
>> To be consistent, it would be good to use BIT(nr) for all bit positions.
> 
> You don't use BIT() macro for bitfields and the above are bitfields.
> Thus, this is not applicable.
> 
>>> +#define CQSPI_REG_CONFIG_CHIPSELECT_MASK       0xF
>>> +#define CQSPI_REG_CONFIG_BAUD_MASK             0xF
> 
> [...]
> 
>>> +#define CQSPI_REG_CMDADDRESS                   0x94
>>> +#define CQSPI_REG_CMDREADDATALOWER             0xA0
>>> +#define CQSPI_REG_CMDREADDATAUPPER             0xA4
>>> +#define CQSPI_REG_CMDWRITEDATALOWER            0xA8
>>> +#define CQSPI_REG_CMDWRITEDATAUPPER            0xAC
>> 
>> I am not sure if there is any recommended way but instread of register
>> macros with offset, isn't it better to use something like struct cdns_qspi
>> {
>>                    u32 config,
>>                    u32 rd_instn,
>>                    ....
>>                };
>> & then configuring the pointer to this structure to the peripheral's (qspi
>> controller in this case) base address.
> 
> U-Boot is slowly abolishing this practice as it turned out to be a horrible
> mistake, which is annoying to deal with . No, I will not do this.
> 
>> It helps in debugging also as you can have all registers under one
>> structure, easy/clean access of register like cdsn_qspi_ptr->config
>> instead of adding offset for every access & also clean picture of all
>> peripheral registers.
> 
> Once you have three similar controllers with only one register mapped
> elsewhere, you'd need three such structures. This approach does look
> nice at first, but certainly does not scale.
> 
>>> +
>>> +/* Interrupt status bits */
>>> +#define CQSPI_REG_IRQ_MODE_ERR                 BIT(0)
>>> +#define CQSPI_REG_IRQ_UNDERFLOW BIT(1)
>>> +#define CQSPI_REG_IRQ_IND_COMP                 BIT(2)
>>> +#define CQSPI_REG_IRQ_IND_RD_REJECT            BIT(3)
>>> +#define CQSPI_REG_IRQ_WR_PROTECTED_ERR         BIT(4)
>>> +#define CQSPI_REG_IRQ_ILLEGAL_AHB_ERR          BIT(5)
>>> +#define CQSPI_REG_IRQ_WATERMARK                        BIT(6)
>>> +#define CQSPI_REG_IRQ_IND_RD_OVERFLOW          BIT(12)
> 
> [...]
> 
>>> +
>>> +static unsigned int cqspi_check_timeout(const unsigned long timeout)
>>> +{
>>> +       return time_before(jiffies, timeout);
>>> +}
>> 
>> try to replace using blocking jiffies check using kernel timeout function.
> 
> What function do you refer to ?
> 
> [...]
> 
>>> +static int cqspi_indirect_read_execute(struct spi_nor *nor,
>>> +                                      u8 *rxbuf, const unsigned n_rx)
>>> +{
>>> +       struct cqspi_flash_pdata *f_pdata = nor->priv;
>>> +       struct cqspi_st *cqspi = f_pdata->cqspi;
>>> +       void __iomem *reg_base = cqspi->iobase;
>>> +       void __iomem *ahb_base = cqspi->ahb_base;
>>> +       unsigned int remaining = n_rx;
>>> +       unsigned int reg = 0;
>>> +       unsigned int bytes_to_read = 0;
>>> +       unsigned int timeout;
>>> +       int ret = 0;
>>> +
>>> +       writel(remaining, reg_base + CQSPI_REG_INDIRECTRDBYTES);
>>> +
>>> +       /* Clear all interrupts. */
>>> +       writel(CQSPI_IRQ_STATUS_MASK, reg_base + CQSPI_REG_IRQSTATUS);
>>> +
>>> +       cqspi->irq_mask = CQSPI_IRQ_MASK_RD;
>>> +       writel(cqspi->irq_mask, reg_base + CQSPI_REG_IRQMASK);
>> 
>> here interrupt mask is being configured for every read, better would be to
>> move it in init.
> 
> It certainly would not, it is configured differently for READ and WRITE.

And what is blocking to configure read and write mask together at one time in the register ?

> 
>>> +
>>> +       reinit_completion(&cqspi->transfer_complete);
>>> +       writel(CQSPI_REG_INDIRECTRD_START_MASK,
>>> +              reg_base + CQSPI_REG_INDIRECTRD);
>>> +
>>> +       while (remaining > 0) {
>>> +               ret =
> 
> [...]
> 
>>> +static void cqspi_controller_init(struct cqspi_st *cqspi)
>>> +{
>>> +       cqspi_controller_disable(cqspi);
>>> +
>>> +       /* Configure the remap address register, no remap */
>>> +       writel(0, cqspi->iobase + CQSPI_REG_REMAP);
>>> +
>>> +       /* Disable all interrupts. */
>>> +       writel(0, cqspi->iobase + CQSPI_REG_IRQMASK);
>>> +
>>> +       /* Configure the SRAM split to 1:1 . */
>>> +       writel(cqspi->fifo_depth / 2, cqspi->iobase +
>>> CQSPI_REG_SRAMPARTITION); +
>>> +       /* Load indirect trigger address. */
>> 
>> almost all comments of this function seems unnecessary.
> 
> I disagree, having sensible comments in code makes my life easier after
> I have to revisit the code a few years later.

Yes for sensible comments , you are loading value in one register and commenting it.

Cheers,
Vikas 

> 
>>> +       writel(cqspi->trigger_address,
>>> +              cqspi->iobase + CQSPI_REG_INDIRECTTRIGGER);
>>> +
>>> +       /* Program read and write watermark. */
>>> +       writel(cqspi->fifo_depth * cqspi->fifo_width / 2,
>>> +              cqspi->iobase + CQSPI_REG_INDIRECTRDWATERMARK);
>>> +       writel(CQSPI_REG_SRAM_THRESHOLD_BYTES,
>>> +              cqspi->iobase + CQSPI_REG_INDIRECTWRWATERMARK);
> 
> [...]
> 
> Best regards,