[RFC,1/2] devicetree: Add devicetree bindings documentation for ZynqMP GQSPI

Add bindings documentation for GQSPI controller driver used by
Zynq Ultrascale+ MPSoC

Signed-off-by: Ranjit Waghmode <ranjit.waghmode@xilinx.com>
---
 .../devicetree/bindings/spi/spi-zynqmp-qspi.txt    | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.txt

--
2.1.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, 2015-05-20 at 12:57PM +0530, Ranjit Waghmode wrote:
> Add bindings documentation for GQSPI controller driver used by
> Zynq Ultrascale+ MPSoC
> 
> Signed-off-by: Ranjit Waghmode <ranjit.waghmode@xilinx.com>
> ---
>  .../devicetree/bindings/spi/spi-zynqmp-qspi.txt    | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.txt
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.txt b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.txt
> new file mode 100644
> index 0000000..cec6330
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.txt
> @@ -0,0 +1,26 @@
> +Xilinx Zynq UltraScale+ MPSoC GQSPI controller Device Tree Bindings
> +-------------------------------------------------------------------
> +
> +Required properties:
> +- compatible		: Should be "xlnx,zynqmp-qspi-1.0".
> +- reg			: Physical base address and size of GQSPI registers map.
> +- interrupts		: Property with a value describing the interrupt
> +			  number.
> +- interrupt-parent	: Must be core interrupt controller.
> +- clock-names		: List of input clock names - "ref_clk", "pclk"
> +			  (See clock bindings for details).
> +- clocks		: Clock phandles (see clock bindings for details).
> +
> +Optional properties:
> +- num-cs		: Number of chip selects used.
> +
> +Example:
> +	qspi: spi@ff0f0000 {
> +		compatible = "xlnx,zynqmp-qspi-1.0";
> +		clock-names = "ref_clk", "pclk";
> +		clocks = <&misc_clk &misc_clk>;
> +		interrupts = <0 15 4>;
> +		interrupt-parent = <&gic>;
> +		num-cs = <1>;
> +		reg = <0x0 0xff0f0000 0x1000 0x0 0xc0000000 0x8000000>;

I find things a lot easier to read when there is something separating
the groups. Could this become:
	reg = <0x0 0xff0f0000 0x1000>, <0x0 0xc0000000 0x8000000>;
?

	Thanks,
	Sören
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Ranjit,

On Wed, 2015-05-20 at 12:57PM +0530, Ranjit Waghmode wrote:
> This patch adds support for GQSPI controller driver used by
> Zynq Ultrascale+ MPSoC
> 
> Signed-off-by: Ranjit Waghmode <ranjit.waghmode@xilinx.com>
> ---
[...]
> +/**
> + * zynqmp_gqspi_read:	For GQSPI controller read operation
> + * @xqspi:	Pointer to the zynqmp_qspi structure
> + * @offset:	Offset from where to read
> + */
> +static u32 zynqmp_gqspi_read(struct zynqmp_qspi *xqspi, u32 offset)
> +{
> +	return readl_relaxed(xqspi->regs + offset);
> +}
> +
> +/**
> + * zynqmp_gqspi_write:	For GQSPI controller write operation
> + * @xqspi:	Pointer to the zynqmp_qspi structure
> + * @offset:	Offset where to write
> + * @val:	Value to be written
> + */
> +static inline void zynqmp_gqspi_write(struct zynqmp_qspi *xqspi, u32 offset,
> +				      u32 val)
> +{
> +	writel_relaxed(val, (xqspi->regs + offset));
> +}

why are you wrapping (readl|writel)_relaxed?

[...]
> +/**
> + * zynqmp_qspi_copy_read_data:	Copy data to RX buffer
> + * @xqspi:	Pointer to the zynqmp_qspi structure
> + * @data:	The 32 bit variable where data is stored
> + * @size:	Number of bytes to be copied from data to RX buffer
> + */
> +static void zynqmp_qspi_copy_read_data(struct zynqmp_qspi *xqspi,
> +				       u32 data, u8 size)
> +{
> +	memcpy(xqspi->rxbuf, ((u8 *) &data), size);

Is this cast really needed? IIRC, memcpy takes (void *). The outer
parentheses for that argument are not needed.

> +	xqspi->rxbuf += size;
> +	xqspi->bytes_to_receive -= size;
> +}
> +
> +/**
> + * zynqmp_prepare_transfer_hardware:	Prepares hardware for transfer.
> + * @master:	Pointer to the spi_master structure which provides
> + *		information about the controller.
> + *
> + * This function enables SPI master controller.
> + *
> + * Return:	Always 0
> + */
> +static int zynqmp_prepare_transfer_hardware(struct spi_master *master)
> +{
> +	struct zynqmp_qspi *xqspi = spi_master_get_devdata(master);
> +
> +	clk_enable(xqspi->refclk);
> +	clk_enable(xqspi->pclk);

Did you consider using runtime_pm? Then this would just bit
pm_runtime_get_sync(...)

> +	zynqmp_gqspi_write(xqspi, GQSPI_EN_OFST, GQSPI_EN_MASK);
> +	return 0;
> +}
> +
> +/**
> + * zynqmp_unprepare_transfer_hardware:	Relaxes hardware after transfer
> + * @master:	Pointer to the spi_master structure which provides
> + *		information about the controller.
> + *
> + * This function disables the SPI master controller.
> + *
> + * Return:	Always 0
> + */
> +static int zynqmp_unprepare_transfer_hardware(struct spi_master *master)
> +{
> +	struct zynqmp_qspi *xqspi = spi_master_get_devdata(master);
> +
> +	zynqmp_gqspi_write(xqspi, GQSPI_EN_OFST, 0x0);
> +	clk_disable(xqspi->refclk);
> +	clk_disable(xqspi->pclk);

and this would become pm_runtime_put()

[...]
> +/**
> + * 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)
> +{
> +	u32 count = 0;
> +
> +	while ((xqspi->bytes_to_transfer > 0) && (count < size)) {
> +		writel(*((u32 *) xqspi->txbuf), xqspi->regs + GQSPI_TXD_OFST);

Here the writel wrapper is not used. Is there a reason, then it would
probably  deserve a comment.

[...]
> +/**
> + * zynqmp_process_dma_irq:	Handler for DMA done interrupt of QSPI
> + *				controller
> + * @xqspi:	zynqmp_qspi instance pointer
> + *
> + * This function handles DMA interrupt only.
> + */
> +static void zynqmp_process_dma_irq(struct zynqmp_qspi *xqspi)
> +{
> +	u32 config_reg, genfifoentry;
> +
> +	dma_unmap_single(xqspi->dev, xqspi->dma_addr,
> +				xqspi->dma_rx_bytes, DMA_FROM_DEVICE);
> +	xqspi->rxbuf += xqspi->dma_rx_bytes;
> +	xqspi->bytes_to_receive -= xqspi->dma_rx_bytes;
> +	xqspi->dma_rx_bytes = 0;
> +
> +	/* Disabling the DMA interrupts */
> +	writel(GQSPI_QSPIDMA_DST_I_EN_DONE_MASK,
> +			xqspi->regs + GQSPI_QSPIDMA_DST_I_DIS_OFST);
> +
> +	if (xqspi->bytes_to_receive > 0) {
> +		/* Switch to IO mode,for remaining bytes to receive */
> +		config_reg = readl(xqspi->regs + GQSPI_CONFIG_OFST);
> +		config_reg &= ~GQSPI_CFG_MODE_EN_MASK;
> +		writel(config_reg, xqspi->regs + GQSPI_CONFIG_OFST);
> +
> +		/* Initiate the transfer of remaining bytes */
> +		genfifoentry = xqspi->genfifoentry;
> +		genfifoentry |= xqspi->bytes_to_receive;
> +		writel(genfifoentry,
> +				xqspi->regs + GQSPI_GEN_FIFO_OFST);
> +
> +		/* Dummy generic FIFO entry */
> +		writel(0x0, xqspi->regs + GQSPI_GEN_FIFO_OFST);

not using wrapper?

> +
> +		/* Manual start */
> +		zynqmp_gqspi_write(xqspi, GQSPI_CONFIG_OFST,
> +			(readl(xqspi->regs + GQSPI_CONFIG_OFST) |

not using wrapper?

Overall:
The usage of those readl/writel wrappers seems inconsistent to me. I
usually prefer not wrapping things like that at all but at least it
should be consistent.

And I believe the handling of clocks would benefit from using of the
runtime_pm framework.

	Thanks,
	Sören
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, May 20, 2015 at 07:55:53AM -0700, Sören Brinkmann wrote:
> On Wed, 2015-05-20 at 12:57PM +0530, Ranjit Waghmode wrote:

> > +static u32 zynqmp_gqspi_read(struct zynqmp_qspi *xqspi, u32 offset)

> > +static inline void zynqmp_gqspi_write(struct zynqmp_qspi *xqspi, u32 offset,
> > +				      u32 val)

> why are you wrapping (readl|writel)_relaxed?

It's very common to wrap the lookup from driver data to the MMIO
address.

Hi Soren,

On Wed, 2015-05-20 at 12:57PM +0530, Ranjit Waghmode wrote:
> Add bindings documentation for GQSPI controller driver used by Zynq 

> Ultrascale+ MPSoC

> 

> Signed-off-by: Ranjit Waghmode <ranjit.waghmode@xilinx.com>

> ---

>  .../devicetree/bindings/spi/spi-zynqmp-qspi.txt    | 26 ++++++++++++++++++++++

>  1 file changed, 26 insertions(+)

>  create mode 100644 

> Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.txt

> 

> diff --git a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.txt 

> b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.txt

> new file mode 100644

> index 0000000..cec6330

> --- /dev/null

> +++ b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.txt

> @@ -0,0 +1,26 @@

> +Xilinx Zynq UltraScale+ MPSoC GQSPI controller Device Tree Bindings

> +-------------------------------------------------------------------

> +

> +Required properties:

> +- compatible		: Should be "xlnx,zynqmp-qspi-1.0".

> +- reg			: Physical base address and size of GQSPI registers map.

> +- interrupts		: Property with a value describing the interrupt

> +			  number.

> +- interrupt-parent	: Must be core interrupt controller.

> +- clock-names		: List of input clock names - "ref_clk", "pclk"

> +			  (See clock bindings for details).

> +- clocks		: Clock phandles (see clock bindings for details).

> +

> +Optional properties:

> +- num-cs		: Number of chip selects used.

> +

> +Example:

> +	qspi: spi@ff0f0000 {

> +		compatible = "xlnx,zynqmp-qspi-1.0";

> +		clock-names = "ref_clk", "pclk";

> +		clocks = <&misc_clk &misc_clk>;

> +		interrupts = <0 15 4>;

> +		interrupt-parent = <&gic>;

> +		num-cs = <1>;

> +		reg = <0x0 0xff0f0000 0x1000 0x0 0xc0000000 0x8000000>;


I find things a lot easier to read when there is something separating the groups. Could this become:
	reg = <0x0 0xff0f0000 0x1000>, <0x0 0xc0000000 0x8000000>; ?

[Ranjit]: I will try your suggestion and send you in the next version.

Regards,
Ranjit

On Wed, May 20, 2015 at 12:57:51PM +0530, Ranjit Waghmode wrote:

This looks pretty good overall but there are a few issues below from a
first read through.

> +static void zynqmp_gqspi_selectflash(struct zynqmp_qspi *instanceptr,
> +				     u8 flashcs, u8 flashbus)

Is this a SPI controller or a flash controller?  It looks like it is
actually a SPI controller but the above is a bit worrying.

> +{
> +	/*
> +	 * Bus and CS lines selected here will be updated in the instance and
> +	 * used for subsequent GENFIFO entries during transfer.
> +	 */
> +	/* Choose slave select line */

Coding style - at least a blank linne between the two comment blocks.

> +	switch (flashcs) {
> +	case GQSPI_SELECT_FLASH_CS_BOTH:
> +		instanceptr->genfifocs = GQSPI_GENFIFO_CS_LOWER |
> +		    GQSPI_GENFIFO_CS_UPPER;
> +		break;
> +	case GQSPI_SELECT_FLASH_CS_UPPER:
> +		instanceptr->genfifocs = GQSPI_GENFIFO_CS_UPPER;
> +		break;
> +	case GQSPI_SELECT_FLASH_CS_LOWER:
> +	default:
> +		instanceptr->genfifocs = GQSPI_GENFIFO_CS_LOWER;
> +	}

Why is there a default case here?  That's going to men we try to handle
any random chip select that gets passed in as pointing to this lower
device which doesn't seem right.  The fact that this is trying to handle
mirroring of the chip select to two devices is also raising alarm bells
here...

> +/**
> + * zynqmp_qspi_copy_read_data:	Copy data to RX buffer
> + * @xqspi:	Pointer to the zynqmp_qspi structure
> + * @data:	The 32 bit variable where data is stored
> + * @size:	Number of bytes to be copied from data to RX buffer
> + */
> +static void zynqmp_qspi_copy_read_data(struct zynqmp_qspi *xqspi,
> +				       u32 data, u8 size)
> +{
> +	memcpy(xqspi->rxbuf, ((u8 *) &data), size);
> +	xqspi->rxbuf += size;
> +	xqspi->bytes_to_receive -= size;
> +}

This looks like there's some type abuse going on and is going to be
broken for 64 bit systems - why are we passing pointers around as 32 bit
unsigned values?

> +static int zynqmp_prepare_transfer_hardware(struct spi_master *master)
> +{
> +	struct zynqmp_qspi *xqspi = spi_master_get_devdata(master);
> +
> +	clk_enable(xqspi->refclk);
> +	clk_enable(xqspi->pclk);

Should be checking return codes here.

> +static void zynqmp_qspi_chipselect(struct spi_device *qspi, bool is_high)
> +{

> +	if (is_high) {
> +		/* 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);

No, this is broken - setting the chip select should set the chip select,
it shouldn't have any impact on transfers.  Transfers should be started
in the transfer operations.

> +	/* If req_hz == 0, default to lowest speed */
> +	while ((baud_rate_val < GQSPI_BAUD_DIV_MAX) &&
> +	       (clk_get_rate(xqspi->refclk) /
> +		(GQSPI_BAUD_DIV_SHIFT << baud_rate_val)) > req_hz)
> +		baud_rate_val++;

It'd be better to factor the clk_get_rate() out of the loop rather than
repeatedly calling into the clock API.

> + * 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
> + */
> +static int zynqmp_qspi_setup(struct spi_device *qspi)
> +{
> +	if (qspi->master->busy)
> +		return -EBUSY;
> +	return zynqmp_qspi_setup_transfer(qspi, NULL);
> +}

The setup() function shouldn't affect the hardwaer but _setup_transfer()
does.  See spi-summary.

> +static void zynqmp_qspi_filltxfifo(struct zynqmp_qspi *xqspi, int size)
> +{
> +	u32 count = 0;
> +
> +	while ((xqspi->bytes_to_transfer > 0) && (count < size)) {
> +		writel(*((u32 *) xqspi->txbuf), xqspi->regs + GQSPI_TXD_OFST);
> +		if (xqspi->bytes_to_transfer >= 4) {
> +			xqspi->txbuf += 4;
> +			xqspi->bytes_to_transfer -= 4;
> +		} else {
> +			xqspi->txbuf += xqspi->bytes_to_transfer;
> +			xqspi->bytes_to_transfer = 0;
> +		}
> +		count++;
> +	}
> +}

This doesn't appear to take any account of endianness which is
especially alarming in the case where we're dealing with the final word
and the casting here looks unsafe.  I'd expect to be using endanness
annotated pointers with no need for casting.

> + */
> +static void zynqmp_process_dma_irq(struct zynqmp_qspi *xqspi)
> +{
> +	u32 config_reg, genfifoentry;
> +
> +	dma_unmap_single(xqspi->dev, xqspi->dma_addr,
> +				xqspi->dma_rx_bytes, DMA_FROM_DEVICE);

Don't manually do DMA mapping, let the core do it.  It looks like we
need to enhance the core DMA mapping to support partial mapping of
transfers here, this case where you're doing the tail of the transfer as
PIO is a reasonable one (I'm a bit surprised we haven't run into it
before to be honest).

> +static inline u32 zynqmp_qspi_selectspimode(u8 spimode)
> +{
> +	u32 mask;
> +
> +	switch (spimode) {
> +	case GQSPI_SELECT_MODE_DUALSPI:
> +		mask = GQSPI_GENFIFO_MODE_DUALSPI;
> +		break;
> +	case GQSPI_SELECT_MODE_QUADSPI:
> +		mask = GQSPI_GENFIFO_MODE_QUADSPI;
> +		break;
> +	case GQSPI_SELECT_MODE_SPI:
> +	default:
> +		mask = GQSPI_GENFIFO_MODE_SPI;
> +	}

Again this default case looks buggy.

> +static int __maybe_unused zynqmp_qspi_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = container_of(dev,
> +						    struct platform_device,
> +						    dev);
> +	struct spi_master *master = platform_get_drvdata(pdev);
> +
> +	spi_master_suspend(master);
> +
> +	zynqmp_unprepare_transfer_hardware(master);

Why are you manually unpreparing the hardware here?  Obviously if the
core has suspended it ought to have done this...

> +	ret = of_property_read_u32(pdev->dev.of_node, "num-cs", &num_cs);
> +	if (ret < 0)
> +		master->num_chipselect = GQSPI_DEFAULT_NUM_CS;
> +	else
> +		master->num_chipselect = num_cs;

Why is this selectable from DT?  I'm not seeing any code here which
really validates chip select numbers or handles arbatrary chip select
numbers...

Hi Mark,

On Fri, May 22, 2015 at 5:28 PM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, May 20, 2015 at 12:57:51PM +0530, Ranjit Waghmode wrote:
>
> This looks pretty good overall but there are a few issues below from a
> first read through.
>
>> +static void zynqmp_gqspi_selectflash(struct zynqmp_qspi *instanceptr,
>> +                                  u8 flashcs, u8 flashbus)
>
> Is this a SPI controller or a flash controller?  It looks like it is
> actually a SPI controller but the above is a bit worrying.

It is a Quad SPI controller. SPI NOR flash devices are the most common
interface. But we hope to keep this driver generic.
The above function name is probably misleading; it can be renamed.

>
>> +{
>> +     /*
>> +      * Bus and CS lines selected here will be updated in the instance and
>> +      * used for subsequent GENFIFO entries during transfer.
>> +      */
>> +     /* Choose slave select line */
>
> Coding style - at least a blank linne between the two comment blocks.
>
>> +     switch (flashcs) {
>> +     case GQSPI_SELECT_FLASH_CS_BOTH:
>> +             instanceptr->genfifocs = GQSPI_GENFIFO_CS_LOWER |
>> +                 GQSPI_GENFIFO_CS_UPPER;
>> +             break;
>> +     case GQSPI_SELECT_FLASH_CS_UPPER:
>> +             instanceptr->genfifocs = GQSPI_GENFIFO_CS_UPPER;
>> +             break;
>> +     case GQSPI_SELECT_FLASH_CS_LOWER:
>> +     default:
>> +             instanceptr->genfifocs = GQSPI_GENFIFO_CS_LOWER;
>> +     }
>
> Why is there a default case here?  That's going to men we try to handle
> any random chip select that gets passed in as pointing to this lower
> device which doesn't seem right.  The fact that this is trying to handle
> mirroring of the chip select to two devices is also raising alarm bells
> here...

This SPI controller has two CS lines and two data bus.
Two devices can be connected to these and either the upper or the
lower or both (Explained below) can be selected.

When two flash devices are used, one of the HW configurations in
which they can be connected is called "parallel" mode where they
share the same CS but use separate 4 bit data bus each
(essentially making it 8 bit bus width).
Another configuration is "stacked" mode where the
same 4 bit data bus is shared and lower or upper CS lines are
used to select the flash.

Nevertheless, we will deal with acceptable ways to add such features
incrementally or maybe send an RFC separately.
For the current patch (which is intended to support a single device),
the above selection of "both" CS wont be necessary.
@Ranjit, please remove it.

<snip>
>> +static void zynqmp_qspi_chipselect(struct spi_device *qspi, bool is_high)
>> +{
>
>> +     if (is_high) {
>> +             /* 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);
>
> No, this is broken - setting the chip select should set the chip select,
> it shouldn't have any impact on transfers.  Transfers should be started
> in the transfer operations.
>

This is the only way to assert the CS. It doesn't start transferring any data.
The controller has a Generic FIFO. All operations to be performed on the
bus have to be given in the form of any entry in this FIFO.
The controller fetches each entry from the FIFO and performs the operations.
And that includes asserting and de-asserting CS. There is no dedicated
register bit to assert or de-assert the CS.

The GEN FIFO entry has fields such as:
- TX or RX
- CS - lower/upper
- data bus - lower/upper
- bytecount
- bus width - x1 or x2 or x4
etc.

@Ranjit It might be useful to describe the GENFIFO format in
the driver at some appropriate place.

Regards,
Harini
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Mark,

On Thu, May 28, 2015 at 9:11 PM, Punnaiah Choudary Kalluri
<punnaiah.choudary.kalluri@xilinx.com> wrote:
> Hi Mark,
>
>> -----Original Message-----
>> From: Mark Brown [mailto:broonie@kernel.org]
>> Sent: Thursday, May 28, 2015 8:34 PM
>> To: Harini Katakam
>> Cc: Ranjit Abhimanyu Waghmode; Rob Herring; Pawel Moll; Mark Rutland;
>> ijc+devicetree@hellion.org.uk; Kumar Gala; Michal Simek; Soren Brinkmann;
>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>> kernel@vger.kernel.org; linux-spi; Punnaiah Choudary Kalluri;
>> ran27jit@gmail.com
>> Subject: Re: [RFC PATCH 2/2] spi: Add support for Zynq Ultrascale+ MPSoC
>> GQSPI controller
>>
>> On Fri, May 22, 2015 at 08:43:54PM +0530, Harini Katakam wrote:
>> > On Fri, May 22, 2015 at 5:28 PM, Mark Brown <broonie@kernel.org> wrote:
>> > > On Wed, May 20, 2015 at 12:57:51PM +0530, Ranjit Waghmode wrote:
>>
>> > > Why is there a default case here?  That's going to men we try to handle
>> > > any random chip select that gets passed in as pointing to this lower
>> > > device which doesn't seem right.  The fact that this is trying to handle
>> > > mirroring of the chip select to two devices is also raising alarm bells
>> > > here...
>>
>> > This SPI controller has two CS lines and two data bus.
>> > Two devices can be connected to these and either the upper or the
>> > lower or both (Explained below) can be selected.
>>
>> > When two flash devices are used, one of the HW configurations in
>> > which they can be connected is called "parallel" mode where they
>>
>> I know what wiring chip selects in parallel is but that's not the
>> question - the question is about the handling of the default case.
>
> Yes, we should not handle default case here. We will change this.
>

Yes, that's right. I was just elaborating the mirroring part.

>>
>> > >> +static void zynqmp_qspi_chipselect(struct spi_device *qspi, bool
>> is_high)
>> > >> +{
>>
>> > >> +     if (is_high) {
>> > >> +             /* 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);
>>
>> > > No, this is broken - setting the chip select should set the chip select,
>> > > it shouldn't have any impact on transfers.  Transfers should be started
>> > > in the transfer operations.
>>
>> > This is the only way to assert the CS. It doesn't start transferring any data.
>>
>> OK, then you can't implement a separate set_cs() operation and shouldn't
>> be trying to do so.  This will break in multiple ways when the framework
>> tries to use the operations separately.  You probably need to implement
>> a single flat transfer() operation.
>
> As we said it will not start any data transfer. In order to set chip select (chip select only) we need to
> 1. Frame a control word with following parameters like the chip select that we would like to set and hold time
>  2. Update the control word to fifo
>
> To have a performance advantage (may be not trivial) we are executing this fifo entry along with the first
>  Data transfer entry in transfer function so that we can avoid waiting for fifo empty in set_cs function.
>
>  We will ensure CS assert by waiting till the fifo empty in set_cs function and justify the what the
> Function supposed do.
>

Yes, if we wait for cs assert to be executed (which is just the time
controller takes to fetch this command
and execute), this set_cs will be independent as expected. The
framework can use it anytime.

Regards,
Harini
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html