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

[Punnaiah Choudary Kalluri]

Hi Mark,

> -----Original Message-----
> From: Mark Brown [mailto:broonie@xxxxxxxxxx]
> Sent: Thursday, May 28, 2015 8:34 PM
> To: Harini Katakam
> Cc: Ranjit Abhimanyu Waghmode; Rob Herring; Pawel Moll; Mark Rutland;
> ijc+devicetree@xxxxxxxxxxxxxx; Kumar Gala; Michal Simek; Soren Brinkmann;
> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-spi; Punnaiah Choudary Kalluri;
> ran27jit@xxxxxxxxx
> 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@xxxxxxxxxx> 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. 

> 
> > >> +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.

Regards,
Punnaiah
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/