RE: [LINUX RFC V2 2/2] spi: Add support for Zynq Ultrascale+ MPSoC GQSPI controller

2015-06-08 Thread Ranjit Abhimanyu Waghmode
Hi,

> >
> > + */
> > +static void zynqmp_qspi_chipselect(struct spi_device *qspi, bool
> > +is_high) {
> > +   struct zynqmp_qspi *xqspi = spi_master_get_devdata(qspi->master);
> > +   u32 genfifoentry = 0x0, statusreg, timeout;
> > +
> > +   genfifoentry |= GQSPI_GENFIFO_MODE_SPI;
> > +   genfifoentry |= xqspi->genfifobus;
> > +
> > +   if (!is_high) {
> > +   genfifoentry |= xqspi->genfifocs;
> > +   genfifoentry |= GQSPI_GENFIFO_CS_SETUP;
> > +   } else {
> > +   genfifoentry |= GQSPI_GENFIFO_CS_HOLD;
> > +   }
> > +
> > +   zynqmp_gqspi_write(xqspi, GQSPI_GEN_FIFO_OFST, genfifoentry);
> > +
> > +   /* Dummy generic FIFO entry */
> > +   zynqmp_gqspi_write(xqspi, GQSPI_GEN_FIFO_OFST, 0x0);
> > +
> > +   /* Manually start the generic FIFO command */
> > +   zynqmp_gqspi_write(xqspi, GQSPI_CONFIG_OFST,
> > +   zynqmp_gqspi_read(xqspi, GQSPI_CONFIG_OFST) |
> > +   GQSPI_CFG_START_GEN_FIFO_MASK);
> > +   timeout = 1;
> > +   /* Wait until the generic FIFO command is empty */
> > +   do {
> > +   statusreg = zynqmp_gqspi_read(xqspi, GQSPI_ISR_OFST);
> > +   timeout--;
>
> Can this be not busy.
Ok, will try to see the other way by which it can be implemented.
>
> > +   } while (!(statusreg &
> > +  GQSPI_ISR_GENFIFOEMPTY_MASK) &&
> > +(statusreg & GQSPI_ISR_TXEMPTY_MASK) && timeout);
> > +   if (!timeout)
> > +   dev_err(xqspi->dev, "Chip select timed out\n"); }
> > +
> > +/**
> > + * zynqmp_qspi_setup_transfer: Configure QSPI controller for specified
> > + * transfer
> > + * @qspi:  Pointer to the spi_device structure
> > + * @transfer:  Pointer to the spi_transfer structure which provides
> > + * information about next transfer setup parameters
> > + *
> > + * Sets the operational mode of QSPI controller for the next QSPI
> > +transfer and
> > + * sets the requested clock frequency.
> > + *
> > + * Return: Always 0
> > + *
> > + * Note:
> > + * If the requested frequency is not an exact match with what can be
> > + * obtained using the pre-scalar value, the driver sets the clock
> > + * frequency which is lower than the requested frequency (maximum 
> > lower)
> > + * for the transfer.
> > + *
> > + * If the requested frequency is higher or lower than that is supported
> > + * by the QSPI controller the driver will set the highest or lowest
> > + * frequency supported by controller.
> > + */
> > +static int zynqmp_qspi_setup_transfer(struct spi_device *qspi,
> > + struct spi_transfer *transfer) {
> > +   struct zynqmp_qspi *xqspi = spi_master_get_devdata(qspi->master);
> > +   ulong clk_rate;
> > +   u32 config_reg, req_hz, baud_rate_val = 0;
> > +
> > +   if (transfer)
> > +   req_hz = transfer->speed_hz;
> > +   else
> > +   req_hz = qspi->max_speed_hz;
> > +
> > +   /* Set the clock frequency */
> > +   /* If req_hz == 0, default to lowest speed */
> > +   clk_rate = clk_get_rate(xqspi->refclk);
> > +
> > +   while ((baud_rate_val < GQSPI_BAUD_DIV_MAX) &&
> > +  (clk_rate /
> > +   (GQSPI_BAUD_DIV_SHIFT << baud_rate_val)) > req_hz)
> > +   baud_rate_val++;
> > +
> > +   config_reg = zynqmp_gqspi_read(xqspi, GQSPI_CONFIG_OFST);
> > +
> > +   /* Set the QSPI clock phase and clock polarity */
> > +   config_reg &= (~GQSPI_CFG_CLK_PHA_MASK) &
> > + (~GQSPI_CFG_CLK_POL_MASK);
> > +
> > +   if (qspi->mode & SPI_CPHA)
> > +   config_reg |= GQSPI_CFG_CLK_PHA_MASK;
> > +   if (qspi->mode & SPI_CPOL)
> > +   config_reg |= GQSPI_CFG_CLK_POL_MASK;
> > +
> > +   config_reg &= ~GQSPI_CFG_BAUD_RATE_DIV_MASK;
> > +   config_reg |= (baud_rate_val << GQSPI_CFG_BAUD_RATE_DIV_SHIFT);
> > +   zynqmp_gqspi_write(xqspi, GQSPI_CONFIG_OFST, config_reg);
> > +   return 0;
> > +}
> > +
> > +/**
> > + * zynqmp_qspi_setup:  Configure the QSPI controller
> > + * @qspi:  Pointer to the spi_device structure
> > + *
> > + * Sets the operational mode of QSPI controller for the next QSPI
> > +transfer,
> > + * baud rate and divisor value to setup the requested qspi clock.
> > + *
> > + * Return: 0 Always
>
> doesnt seem to be true
Will update this.
>
> > + */
> > +static int zynqmp_qspi_setup(struct spi_device *qspi) {
> > +   if (qspi->master->busy)
> > +   return -EBUSY;
> > +   return 0;
> > +}
> > +
> > +/**
> > + * zynqmp_qspi_filltxfifo: Fills the TX FIFO as long as there is room 
> > in
> > + * the FIFO or the bytes required to be
> > + * transmitted.
> > + * @xqspi: Pointer to the zynqmp_qspi structure
> > + * @size:  Number of bytes to be copied from TX buffer

Re: [LINUX RFC V2 2/2] spi: Add support for Zynq Ultrascale+ MPSoC GQSPI controller

2015-06-06 Thread Shubhrajyoti Datta
hi,

Some minor comments.


On Fri, Jun 5, 2015 at 6:37 PM, Ranjit Waghmode
 wrote:
> This patch adds support for GQSPI controller driver used by
> Zynq Ultrascale+ MPSoC
>
> Signed-off-by: Ranjit Waghmode 
> ---
> Here is the v2 series.

> + */
> +static void zynqmp_qspi_chipselect(struct spi_device *qspi, bool is_high)
> +{
> +   struct zynqmp_qspi *xqspi = spi_master_get_devdata(qspi->master);
> +   u32 genfifoentry = 0x0, statusreg, timeout;
> +
> +   genfifoentry |= GQSPI_GENFIFO_MODE_SPI;
> +   genfifoentry |= xqspi->genfifobus;
> +
> +   if (!is_high) {
> +   genfifoentry |= xqspi->genfifocs;
> +   genfifoentry |= GQSPI_GENFIFO_CS_SETUP;
> +   } else {
> +   genfifoentry |= GQSPI_GENFIFO_CS_HOLD;
> +   }
> +
> +   zynqmp_gqspi_write(xqspi, GQSPI_GEN_FIFO_OFST, genfifoentry);
> +
> +   /* Dummy generic FIFO entry */
> +   zynqmp_gqspi_write(xqspi, GQSPI_GEN_FIFO_OFST, 0x0);
> +
> +   /* Manually start the generic FIFO command */
> +   zynqmp_gqspi_write(xqspi, GQSPI_CONFIG_OFST,
> +   zynqmp_gqspi_read(xqspi, GQSPI_CONFIG_OFST) |
> +   GQSPI_CFG_START_GEN_FIFO_MASK);
> +   timeout = 1;
> +   /* Wait until the generic FIFO command is empty */
> +   do {
> +   statusreg = zynqmp_gqspi_read(xqspi, GQSPI_ISR_OFST);
> +   timeout--;

Can this be not busy.

> +   } while (!(statusreg &
> +  GQSPI_ISR_GENFIFOEMPTY_MASK) &&
> +(statusreg & GQSPI_ISR_TXEMPTY_MASK) && timeout);
> +   if (!timeout)
> +   dev_err(xqspi->dev, "Chip select timed out\n");
> +}
> +
> +/**
> + * zynqmp_qspi_setup_transfer: Configure QSPI controller for specified
> + * transfer
> + * @qspi:  Pointer to the spi_device structure
> + * @transfer:  Pointer to the spi_transfer structure which provides
> + * information about next transfer setup parameters
> + *
> + * Sets the operational mode of QSPI controller for the next QSPI transfer 
> and
> + * sets the requested clock frequency.
> + *
> + * Return: Always 0
> + *
> + * Note:
> + * If the requested frequency is not an exact match with what can be
> + * obtained using the pre-scalar value, the driver sets the clock
> + * frequency which is lower than the requested frequency (maximum lower)
> + * for the transfer.
> + *
> + * If the requested frequency is higher or lower than that is supported
> + * by the QSPI controller the driver will set the highest or lowest
> + * frequency supported by controller.
> + */
> +static int zynqmp_qspi_setup_transfer(struct spi_device *qspi,
> + struct spi_transfer *transfer)
> +{
> +   struct zynqmp_qspi *xqspi = spi_master_get_devdata(qspi->master);
> +   ulong clk_rate;
> +   u32 config_reg, req_hz, baud_rate_val = 0;
> +
> +   if (transfer)
> +   req_hz = transfer->speed_hz;
> +   else
> +   req_hz = qspi->max_speed_hz;
> +
> +   /* Set the clock frequency */
> +   /* If req_hz == 0, default to lowest speed */
> +   clk_rate = clk_get_rate(xqspi->refclk);
> +
> +   while ((baud_rate_val < GQSPI_BAUD_DIV_MAX) &&
> +  (clk_rate /
> +   (GQSPI_BAUD_DIV_SHIFT << baud_rate_val)) > req_hz)
> +   baud_rate_val++;
> +
> +   config_reg = zynqmp_gqspi_read(xqspi, GQSPI_CONFIG_OFST);
> +
> +   /* Set the QSPI clock phase and clock polarity */
> +   config_reg &= (~GQSPI_CFG_CLK_PHA_MASK) & (~GQSPI_CFG_CLK_POL_MASK);
> +
> +   if (qspi->mode & SPI_CPHA)
> +   config_reg |= GQSPI_CFG_CLK_PHA_MASK;
> +   if (qspi->mode & SPI_CPOL)
> +   config_reg |= GQSPI_CFG_CLK_POL_MASK;
> +
> +   config_reg &= ~GQSPI_CFG_BAUD_RATE_DIV_MASK;
> +   config_reg |= (baud_rate_val << GQSPI_CFG_BAUD_RATE_DIV_SHIFT);
> +   zynqmp_gqspi_write(xqspi, GQSPI_CONFIG_OFST, config_reg);
> +   return 0;
> +}
> +
> +/**
> + * zynqmp_qspi_setup:  Configure the QSPI controller
> + * @qspi:  Pointer to the spi_device structure
> + *
> + * Sets the operational mode of QSPI controller for the next QSPI transfer,
> + * baud rate and divisor value to setup the requested qspi clock.
> + *
> + * Return: 0 Always

doesnt seem to be true

> + */
> +static int zynqmp_qspi_setup(struct spi_device *qspi)
> +{
> +   if (qspi->master->busy)
> +   return -EBUSY;
> +   return 0;
> +}
> +
> +/**
> + * zynqmp_qspi_filltxfifo: Fills the TX FIFO as long as there is room in
> + * the FIFO or the bytes required to be
> + * transmitted.
> + * @xqspi: Pointer to the zynqmp_qspi structure
> + * @size:  Number of bytes to be copied from TX buffer to TX FIFO
> + */
> +static void zynqmp_qspi_filltxfifo(struct zynqmp_qspi *xqspi, int size)
> +{
> +