RE: [LINUX PATCH v2 3/3] spi: spi-mem: Add support for Zynq QSPI controller

[Naga Sureshkumar Relli]

Hi Vignesh,

Thanks for the review.

> -----Original Message-----
> From: Vignesh Raghavendra <vigneshr@xxxxxx>
> Sent: Friday, April 5, 2019 10:14 AM
> To: Naga Sureshkumar Relli <nagasure@xxxxxxxxxx>; broonie@xxxxxxxxxx;
> bbrezillon@xxxxxxxxxx
> Cc: linux-spi@xxxxxxxxxxxxxxx; dwmw2@xxxxxxxxxxxxx; marek.vasut@xxxxxxxxx;
> richard@xxxxxx; linux-mtd@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Michal Simek
> <michals@xxxxxxxxxx>; nagasuresh12@xxxxxxxxx
> Subject: Re: [LINUX PATCH v2 3/3] spi: spi-mem: Add support for Zynq QSPI controller
> 
> 
> 
> On 01/04/19 1:29 PM, Naga Sureshkumar Relli wrote:
> > +/**
> > + * zynq_qspi_config_op - Configure QSPI controller for specified transfer
> > + * @xqspi:	Pointer to the zynq_qspi structure
> > + * @qspi:	Pointer to the spi_device structure
> > + *
> > + * Sets the operational mode of QSPI controller for the next QSPI
> > +transfer and
> > + * sets the requested clock frequency.
> > + *
> > + * Return:	0 on success and -EINVAL on invalid input parameter
> > + *
> > + * Note: If the requested frequency is not an exact match with what
> > +can be
> > + * obtained using the prescalar 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 zynq_qspi_config_op(struct zynq_qspi *xqspi, struct
> > +spi_device *spi) {
> > +	u32 config_reg, baud_rate_val = 0;
> > +
> > +	/*
> > +	 * Set the clock frequency
> > +	 * The baud rate divisor is not a direct mapping to the value written
> > +	 * into the configuration register (config_reg[5:3])
> > +	 * i.e. 000 - divide by 2
> > +	 *      001 - divide by 4
> > +	 *      ----------------
> > +	 *      111 - divide by 256
> > +	 */
> > +	while ((baud_rate_val < ZYNQ_QSPI_BAUD_DIV_MAX)  &&
> > +	       (clk_get_rate(xqspi->refclk) / (2 << baud_rate_val)) >
> > +		spi->max_speed_hz)
> > +		baud_rate_val++;
> > +
> 
> Instead use DIV_ROUND_UP, something like below should work(untested):
> 
> 	unsigned long refclk_rate = clk_get_rate(xqspi->refclk);
> 	u32 baud_rate_val = DIV_ROUND_UP(refclk_rate, spi->max_speed_hz) - 1;
> 
This is not just direct calculation.
i.e. for example
refclk_rate = 200MHz and max_speed_hx = 100MHz.
then DIV_ROUND_UP gives a value of 2.
But writing a value of 2 to config registers means, divide by 8. But we should write divide by 2 (value of zero).
Thatâs why we implemented the above calculation.

000: divide by 2.
001: divide by 4
010: divide by 8
011: divide by 16
100: divide by 32
101: divide by 64
110: divide by 128
111: divide by 256

Thanks,
Naga Sureshkumar Relli
> 	if (baud_rate_val > ZYNQ_QSPI_BAUD_DIV_MAX)
> 		baud_rate_val = ZYNQ_QSPI_BAUD_DIV_MAX;
> 
> > +	config_reg = zynq_qspi_read(xqspi, ZYNQ_QSPI_CONFIG_OFFSET);
> > +
> > +	/* Set the QSPI clock phase and clock polarity */
> > +	config_reg &= (~ZYNQ_QSPI_CONFIG_CPHA_MASK) &
> > +		      (~ZYNQ_QSPI_CONFIG_CPOL_MASK);
> > +	if (spi->mode & SPI_CPHA)
> > +		config_reg |= ZYNQ_QSPI_CONFIG_CPHA_MASK;
> > +	if (spi->mode & SPI_CPOL)
> > +		config_reg |= ZYNQ_QSPI_CONFIG_CPOL_MASK;
> > +
> > +	config_reg &= ~ZYNQ_QSPI_CONFIG_BDRATE_MASK;
> > +	config_reg |= (baud_rate_val << ZYNQ_QSPI_BAUD_DIV_SHIFT);
> > +	zynq_qspi_write(xqspi, ZYNQ_QSPI_CONFIG_OFFSET, config_reg);
> > +
> > +	return 0;
> > +}
> > +
> 
> --
> Regards
> Vignesh