On 05/13/2016 02:00 AM, Trent Piepho wrote: > On Mon, 2016-01-11 at 05:34 +0100, Marek Vasut wrote: >> From: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@xxxxxxxxxxxxxxxx> >> >> 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. > > I'm trying to use this driver the Alaric Devkit for Altera's Arria10 > SoC. It's not working so far. In the course of trying to debug it, > I've found a few things with the driver in the socfpga-4.1-ltsi branch. So are you trying to debug this driver or some other out-of-tree driver? btw. I pushed the latest version here: https://git.kernel.org/cgit/linux/kernel/git/marex/linux-2.6.git/log/?h=next/cadence-qspi > However most of them are in code that's not present in this driver, > which is the newest post of the code I could find to reply to. I will check the rest later, thanks! >> + CQSPI_REG_IRQ_WATERMARK | \ >> + CQSPI_REG_IRQ_UNDERFLOW) >> + >> +#define CQSPI_IRQ_STATUS_MASK 0x1FFFF >> + > > Perhaps a comment. > /* waits for all bits set in mask to be zero (clear==false) or one > (clear==true) */ > >> +static int cqspi_wait_for_bit(void __iomem *reg, const u32 mask, bool clear) >> +{ >> + unsigned long end = jiffies + msecs_to_jiffies(CQSPI_TIMEOUT_MS); >> + u32 val; >> + >> + while (1) { >> + val = readl(reg); >> + if (clear) >> + val = ~val; >> + val &= mask; >> + >> + if (val == mask) >> + return 0; > > Somewhat simpler. > > if ((readl(reg) & mask) == (clear ? 0 : mask)) > return 0; > >> + >> + if (time_after(jiffies, end)) >> + return -ETIMEDOUT; > > Note that there is a hypervisor/vm/long hardirq etc. bug that can happen > without timeouts like this. What happens is after the check of the bits > fails, there is a very long delay before this task runs again. This can > be easy if one is running under virtualization. The the time_after call > reports the timeout expired. But the last time it the bit was checked, > before the long delay, was well before the timeout expired. The way to > avoid this is to always be sure to check the condition once after the > timeout expired. This is sure to give the full timeout. > > >> + } >> +} >> + > >> + >> +static unsigned int calculate_ticks_for_ns(const unsigned int ref_clk_hz, >> + const unsigned int ns_val) >> +{ >> + unsigned int ticks; >> + >> + ticks = ref_clk_hz / 1000; /* kHz */ > This division doesn't round up. > >> + ticks = DIV_ROUND_UP(ticks * ns_val, 1000000); > But this one does. > >> + >> + return ticks; >> +} >> + >> +static void cqspi_delay(struct spi_nor *nor, const unsigned int sclk_hz) >> +{ >> + struct cqspi_flash_pdata *f_pdata = nor->priv; >> + struct cqspi_st *cqspi = f_pdata->cqspi; > > Isn't the sclk_hz parameter of this function already available here as > cqspi->sclk? > >> + void __iomem *iobase = cqspi->iobase; >> + const unsigned int ref_clk_hz = cqspi->master_ref_clk_hz; >> + unsigned int tshsl, tchsh, tslch, tsd2d; >> + unsigned int reg; >> + unsigned int tsclk; >> + >> + /* calculate the number of ref ticks for one sclk tick */ >> + tsclk = (ref_clk_hz + sclk_hz - 1) / sclk_hz; > > DIV_ROUND_UP(ref_clk_hz, sclk_hz); > >> + >> + tshsl = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tshsl_ns); >> + /* this particular value must be at least one sclk */ >> + if (tshsl < tsclk) >> + tshsl = tsclk; >> + >> + tchsh = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tchsh_ns); >> + tslch = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tslch_ns); >> + tsd2d = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tsd2d_ns); >> + >> + reg = (tshsl & CQSPI_REG_DELAY_TSHSL_MASK) >> + << CQSPI_REG_DELAY_TSHSL_LSB; >> + reg |= (tchsh & CQSPI_REG_DELAY_TCHSH_MASK) >> + << CQSPI_REG_DELAY_TCHSH_LSB; >> + reg |= (tslch & CQSPI_REG_DELAY_TSLCH_MASK) >> + << CQSPI_REG_DELAY_TSLCH_LSB; >> + reg |= (tsd2d & CQSPI_REG_DELAY_TSD2D_MASK) >> + << CQSPI_REG_DELAY_TSD2D_LSB; >> + writel(reg, iobase + CQSPI_REG_DELAY); >> +} >> + >> +static void cqspi_config_baudrate_div(struct cqspi_st *cqspi, >> + const unsigned int sclk_hz) >> +{ > > sclk_hz is available as cqspi->sclk. > >> + const unsigned int ref_clk_hz = cqspi->master_ref_clk_hz; >> + void __iomem *reg_base = cqspi->iobase; >> + unsigned int reg; >> + unsigned int div; >> + >> + reg = readl(reg_base + CQSPI_REG_CONFIG); >> + reg &= ~(CQSPI_REG_CONFIG_BAUD_MASK << CQSPI_REG_CONFIG_BAUD_LSB); >> + >> + div = ref_clk_hz / sclk_hz; > > Should this round up too? > >> + >> + /* Recalculate the baudrate divisor based on QSPI specification. */ >> + if (div > 32) >> + div = 32; >> + >> + /* Check if even number. */ >> + if (div & 1) >> + div = (div / 2); >> + else >> + div = (div / 2) - 1; > > Wouldn't this be the same as div = DIV_ROUND_UP(div, 2) - 1; > > The entire div calculation could be done with: > > /* Register programmed with divider minus 1 */ > div = DIV_ROUND_UP(ref_clk_hz, s_clk_hz * 2) - 1; > if (div > 15) > div = 15; > > >> + >> + div = (div & CQSPI_REG_CONFIG_BAUD_MASK) << CQSPI_REG_CONFIG_BAUD_LSB; >> + reg |= div; >> + writel(reg, reg_base + CQSPI_REG_CONFIG); >> +} >> + > -- Best regards, Marek Vasut -- 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
