Re: [PATCH v2 2/6] spi: cadence-quadspi: Disable the DAC for Intel LGM SoC

[Ramuthevar, Vadivel MuruganX]

Hi,

On 22/10/2020 5:01 pm, Pratyush Yadav wrote:
> On 22/10/20 10:17AM, Ramuthevar, Vadivel MuruganX wrote:
> > Hi Pratyush,
> >
> > On 21/10/2020 11:17 pm, Pratyush Yadav wrote:
> > > Hi,
> > >
> > > On 21/10/20 10:55AM, Ramuthevar,Vadivel MuruganX wrote:
> > > > From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx>
> > > >
> > > > On Intel Lightning Mountain(LGM) SoCs QSPI controller do not use
> > > > Direct Access Controller(DAC).
> > > >
> > > > This patch adds a quirk to disable the Direct Access Controller
> > > > for data transfer instead it uses indirect data transfer.
> > > >
> > > > Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx>
> > > > ---
> > > >    drivers/spi/spi-cadence-quadspi.c | 12 ++++++++++++
> > > >    1 file changed, 12 insertions(+)
> > > >
> > > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> > > > index d7b10c46fa70..3d017b484114 100644
> > > > --- a/drivers/spi/spi-cadence-quadspi.c
> > > > +++ b/drivers/spi/spi-cadence-quadspi.c
> > > > @@ -1106,6 +1106,13 @@ static void cqspi_controller_init(struct cqspi_st *cqspi)
> > > >    	reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
> > > >    	writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
> > > > +	/* Disable direct access controller */
> > > > +	if (!cqspi->use_direct_mode) {
> > > > +		reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
> > > > +		reg &= ~CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
> > > > +		writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
> > > > +	}
> > > > +
> > >
> > > Do you really need to disable the DAC controller? cqspi_read() and
> > > cqspi_write() already check for cqspi->use_direct_mode and avoid using
> > > direct mode if it is false. While I don't think it would do any harm I'm
> > > curious what prompted you to do this instead of just setting the quirk
> > > like cdns_qspi does.
> > >
> > > Anyway, if you do insist on doing it, it does not make any sense to set
> > > a bit and then unset it immediately after. The datasheet I have says
> > > this bit resets to 1 so the block above the code you added should be
> > > removed.
> > Thank you for your review comments..
> > yes, we need this patch to disable DAC for our SoC to avoid any conflicts in
> > future as well since Intel LGM SoC doesn't support DAC at all.
>
> I'm not sure you got my point here.
Got your point, thanks!
 I understand that LGM SoCs don't
> support DAC. I'm not arguing if this _patch_ is needed. I'm arguing if
> this _hunk_ is needed.
Needed, my previous patches added DAC disabled in cqspi_read() and 
cqspi_write() function then Vignesh suggested me to move 
cqspi_controller_init() function part so I have add it now.

you are saying that add hunk at the end of cqspi_controller_init().
that's also okay for me, anyhow DAC should be disabled at any case.

Regards
Vadivel
 Does DAC mode need to be explicitly disabled
> here? Why will the check in cqspi_read() and cqspi_write() not be
> enough?
>
> My other point is that if you absolutely need to disable DAC mode, then
> instead of the code you have added, it would make more sense to do
> something like below in cqspi_controller_init(). Because the bit resets
> to 1 so the block of code to enable it is useless [0].
>
> --- 8< ---
> diff --git a/drivers/spi/spi-cadence-quadspi.c
> b/drivers/spi/spi-cadence-quadspi.c
> index d7ad8b198a11..d2c5d448a944 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -2156,10 +2156,12 @@ static void cqspi_controller_init(struct cqspi_st *cqspi)
>   	writel(cqspi->fifo_depth * cqspi->fifo_width / 8,
>   	       cqspi->iobase + CQSPI_REG_INDIRECTWRWATERMARK);
>   
> -	/* Enable Direct Access Controller */
> -	reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
> -	reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
> -	writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
> +	/* Disable Direct Access Controller */
> +	if (!cqspi->use_dac_mode) {
> +		reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
> +		reg &= ~CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
> +		writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
> +	}
>   
>   	cqspi_controller_enable(cqspi, 1);
>   }
> --- >8 ---
>
> Disclaimer: not tested at all.
>
> [0] Git blames Vignesh for that block of code added in a27f2eaf2b27.
> Vignesh, was this simply an oversight or was there any real reason to
> set the bit?
>   
> > Regards
> > Vadivel
> > > >    	cqspi_controller_enable(cqspi, 1);
> > > >    }
> > > > @@ -1388,6 +1395,10 @@ static const struct cqspi_driver_platdata am654_ospi = {
> > > >    	.quirks = CQSPI_NEEDS_WR_DELAY,
> > > >    };
> > > > +static const struct cqspi_driver_platdata intel_lgm_qspi = {
> > > > +	.quirks = CQSPI_DISABLE_DAC_MODE,
> > > > +};
> > > > +
> > > >    static const struct of_device_id cqspi_dt_ids[] = {
> > > >    	{
> > > >    		.compatible = "cdns,qspi-nor",
> > > > @@ -1403,6 +1414,7 @@ static const struct of_device_id cqspi_dt_ids[] = {
> > > >    	},
> > > >    	{
> > > >    		.compatible = "intel,lgm-qspi",
> > > > +		.data = &intel_lgm_qspi,
> > > >    	},
> > > >    	{ /* end of table */ }
> > > >    };