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.
> > +
> > + 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.
> > + 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,
--
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
