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

["Ramuthevar, Vadivel MuruganX" <vadivel.muruganx.ramuthevar@linux.intel.com>]

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@linux.intel.com>
>>>>
>>>> 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@linux.intel.com>
>>>> ---
>>>>    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 */ }
>>>>    };
>>>
>