| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 22 Oct 2020 17:14:26 +0800
From: "Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@...ux.intel.com>
To: Pratyush Yadav <p.yadav@...com>
Cc: vigneshr@...com, tudor.ambarus@...rochip.com, broonie@...nel.org,
linux-kernel@...r.kernel.org, linux-spi@...r.kernel.org,
robh+dt@...nel.org, devicetree@...r.kernel.org,
miquel.raynal@...tlin.com, simon.k.r.goldschmidt@...il.com,
dinguyen@...nel.org, richard@....at, cheol.yong.kim@...el.com,
qi-ming.wu@...el.com
Subject: Re: [PATCH v2 2/6] spi: cadence-quadspi: Disable the DAC for Intel
LGM SoC
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@...ux.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@...ux.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 */ }
>>>> };
>>>
>
Powered by blists - more mailing lists