On Tuesday, January 12, 2016 at 05:59:11 AM, Vignesh R wrote:
> Hi,
Hi!
> On 01/11/2016 10:20 AM, Marek Vasut wrote:
> > On Monday, January 11, 2016 at 05:14:17 AM, R, Vignesh wrote:
> >> Hi Marek,
> >
> > Hi!
> >
> >> On 08/21/2015 02:50 PM, Marek Vasut wrote:
> >>> From: Graham Moore <grmoore@xxxxxxxxxxxxxxxxxxxxx>
> >>>
> >>> Add support for the Cadence QSPI controller. This controller is
> >>> present in the Altera SoCFPGA SoCs and this driver has been tested
> >>> on the Cyclone V SoC.
> >>
> >> Thanks for the patch.
> >> I have a TI EVM with Cadence QSPI controller connected to Spansion
> >> flash(s25fl512s), and I gave this series a try. It worked with couple of
> >> changes, please see comments inline.
> >
> > I just posted the latest greatest version I have in tree, could you give
> > it a spin please ?
>
> Thanks, I will test it soon.
Cool
> >>> Signed-off-by: Graham Moore <grmoore@xxxxxxxxxxxxxxxxxxxxx>
> >>> Signed-off-by: Marek Vasut <marex@xxxxxxx>
> >>> Cc: Alan Tull <atull@xxxxxxxxxxxxxxxxxxxxx>
> >>> Cc: Brian Norris <computersforpeace@xxxxxxxxx>
> >>> Cc: David Woodhouse <dwmw2@xxxxxxxxxxxxx>
> >>> Cc: Dinh Nguyen <dinguyen@xxxxxxxxxxxxxxxxxxxxx>
> >>> Cc: Graham Moore <grmoore@xxxxxxxxxxxxxxxxxxxxx>
> >>> Cc: Vikas MANOCHA <vikas.manocha@xxxxxx>
> >>> Cc: Yves Vandervennet <yvanderv@xxxxxxxxxxxxxxxxxxxxx>
> >>> Cc: devicetree@xxxxxxxxxxxxxxx
> >
> > [...]
> >
> >>> +static irqreturn_t cqspi_irq_handler(int this_irq, void *dev)
> >>> +{
> >>> + struct cqspi_st *cqspi = dev;
> >>> + unsigned int irq_status;
> >>> +
> >>> + /* Read interrupt status */
> >>> + irq_status = readl(cqspi->iobase + CQSPI_REG_IRQSTATUS);
> >>> +
> >>> + /* Clear interrupt */
> >>> + writel(irq_status, cqspi->iobase + CQSPI_REG_IRQSTATUS);
> >>> +
> >>> + irq_status &= CQSPI_IRQ_MASK_RD | CQSPI_IRQ_MASK_WR;
> >>> +
> >>> + if (irq_status)
> >>> + complete(&cqspi->transfer_complete);
> >>> +
> >>
> >> You seem to signal completion even if the interrupt is
> >> CQSPI_REG_IRQ_IND_(WR|RD)_OVERFLOW. Doesn't this lead to data loss on
> >> overflow?
> >
> > That's a good question. The write routine,
> > cqspi_indirect_write_execute(), always writes a page and then waits for
> > completion, so the overflow should never happen.
> >
> > What would you suggest to do about write overflow interrupt anyway ? :(
>
> My concern was signalling completion on CQSPI_REG_IRQ_IND_WR_OVERFLOW
> interrupt which meant data loss.
> But, I see you have disabled CQSPI_REG_IRQ_IND_WR_OVERFLOW in v10. This
> addresses my concern.
All right :)
> >>> + return IRQ_HANDLED;
> >>> +}
> >
> > [...]
> >
> >>> +static int cqspi_prep(struct spi_nor *nor, enum spi_nor_ops ops)
> >>> +{
> >>> + struct cqspi_flash_pdata *f_pdata = nor->priv;
> >>> + struct cqspi_st *cqspi = f_pdata->cqspi;
> >>> + const unsigned int sclk = f_pdata->clk_rate;
> >>> +
> >>> + /* Switch chip select. */
> >>> + if (cqspi->current_cs != f_pdata->cs) {
> >>> + cqspi->current_cs = f_pdata->cs;
> >>> + cqspi_switch_cs(nor);
> >>> + }
> >>> +
> >>> + /* Setup baudrate divisor and delays */
> >>> + if (cqspi->sclk != sclk) {
> >>> + cqspi->sclk = sclk;
> >>> + cqspi_controller_disable(cqspi);
> >>> + cqspi_config_baudrate_div(cqspi, sclk);
> >>> + cqspi_delay(nor, sclk);
> >>> + cqspi_readdata_capture(cqspi, 1, f_pdata->read_delay);
> >>
> >> I see bypass field value is being hard coded to 1. Atleast on TI SoC,
> >> bypass=0 when SPI bus frequency is >= 50MHz. Therefore, is it possible
> >> to provide a way to configure bypass bit from DT?
> >> Or if bypass=0 for >=50MHz on your SoC too then, its better to configure
> >> bypass bit based on SPI bus frequency.
> >
> > You can have multiple SPI peripherals attached to the QSPI block, each
> > running at different bus speed, so I think the bypass should be
> > configured based on the frequency of the active peripheral, not from OF.
>
> I agree, if the dependency is uniform across SoCs, then bypass should be
> configured based on the frequency of the active peripheral.
>
> > I don't know exactly what impact configuring this bit has, the socfpga
> > datasheet is not very clear about the meaning of this bit, so it'd be
> > better if someone from Altera answered this question.
>
> Have you tested the driver at SPI bus frequency >50MHz?
Yeah, I run it at 100MHz .
> >>> + cqspi_controller_enable(cqspi);
> >>> + }
> >>> + return 0;
> >>> +}
> >
> > [...]
>
> [...]
>
> >>> + ret = devm_request_irq(dev, irq, cqspi_irq_handler, 0,
> >>> + pdev->name, cqspi);
> >>> + if (ret) {
> >>> + dev_err(dev, "Cannot request IRQ.\n");
> >>> + return ret;
> >>> + }
> >>> +
> >>
> >> pm_runtime_enable(), pm_runtime_get_sync() might be needed here as QSPI
> >> clocks may not be enabled by default on all platforms.
> >
> > Can you whip up a bit of code and test it on your platform ? I dont think
> > socfpga has runtime PM yet. I'd like to add it into the driver or you can
> > send a small patch later, whichever way you prefer.
>
> Ok, I can send a patch once the driver is accepted.
Thanks!
> >>> + cqspi_wait_idle(cqspi);
> >>> + cqspi_controller_init(cqspi);
> >>> + cqspi->current_cs = -1;
> >>> + cqspi->sclk = 0;
> >>> +
> >>> + ret = cqspi_setup_flash(cqspi, np);
> >>> + if (ret) {
> >>> + dev_err(dev, "Cadence QSPI NOR probe failed %d\n", ret);
> >>> + cqspi_controller_disable(cqspi);
> >>> + }
> >>> +
> >>> + return ret;
> >>> +}
> >
> > Thanks!
--
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
