linux-kernel - Re: [PATCH v2 04/19] spi: dw: Fix native CS being unset

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Message-ID: <20200515120514.GX185537@smile.fi.intel.com>
Date:   Fri, 15 May 2020 15:05:14 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Serge Semin <Sergey.Semin@...kalelectronics.ru>
Cc:     Mark Brown <broonie@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Charles Keepax <ckeepax@...nsource.cirrus.com>,
        Serge Semin <fancer.lancer@...il.com>,
        Georgy Vlasov <Georgy.Vlasov@...kalelectronics.ru>,
        Ramil Zaripov <Ramil.Zaripov@...kalelectronics.ru>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Paul Burton <paulburton@...nel.org>,
        Ralf Baechle <ralf@...ux-mips.org>,
        Arnd Bergmann <arnd@...db.de>,
        Allison Randal <allison@...utok.net>,
        Gareth Williams <gareth.williams.jx@...esas.com>,
        Rob Herring <robh+dt@...nel.org>, linux-mips@...r.kernel.org,
        devicetree@...r.kernel.org,
        Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@...el.com>,
        Clement Leger <cleger@...ray.eu>,
        Phil Edworthy <phil.edworthy@...esas.com>,
        "wuxu.wu" <wuxu.wu@...wei.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 04/19] spi: dw: Fix native CS being unset

On Fri, May 15, 2020 at 01:47:43PM +0300, Serge Semin wrote:
> Commit 6e0a32d6f376 ("spi: dw: Fix default polarity of native
> chipselect") attempted to fix the problem when GPIO active-high
> chip-select is utilized to communicate with some SPI slave. It fixed
> the problem, but broke the normal native CS support. At the same time
> the reversion commit ada9e3fcc175 ("spi: dw: Correct handling of native
> chipselect") didn't solve the problem either, since it just inverted
> the set_cs() polarity perception without taking into account that
> CS-high might be applicable. Here is what is done to finally fix the
> problem.
> 
> DW SPI controller demands any native CS being set in order to proceed
> with data transfer. So in order to activate the SPI communications we
> must set any bit in the Slave Select DW SPI controller register no
> matter whether the platform requests the GPIO- or native CS. Preferably
> it should be the bit corresponding to the SPI slave CS number. But
> currently the dw_spi_set_cs() method activates the chip-select
> only if the second argument is false. Since the second argument of the
> set_cs callback is expected to be a boolean with "is-high" semantics
> (actual chip-select pin state value), the bit in the DW SPI Slave
> Select register will be set only if SPI core requests the driver
> to set the CS in the low state. So this will work for active-low
> GPIO-based CS case, and won't work for active-high CS setting
> the bit when SPI core actually needs to deactivate the CS.
> 
> This commit fixes the problem for all described cases. So no matter
> whether an SPI slave needs GPIO- or native-based CS with active-high
> or low signal the corresponding bit will be set in SER.

Nice catch!
Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>

> Signed-off-by: Serge Semin <Sergey.Semin@...kalelectronics.ru>
> Fixes: ada9e3fcc175 ("spi: dw: Correct handling of native chipselect")
> Fixes: 6e0a32d6f376 ("spi: dw: Fix default polarity of native chipselect")
> Reviewed-by: Charles Keepax <ckeepax@...nsource.cirrus.com>
> Acked-by: Linus Walleij <linus.walleij@...aro.org>
> Cc: Georgy Vlasov <Georgy.Vlasov@...kalelectronics.ru>
> Cc: Ramil Zaripov <Ramil.Zaripov@...kalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@...ha.franken.de>
> Cc: Paul Burton <paulburton@...nel.org>
> Cc: Ralf Baechle <ralf@...ux-mips.org>
> Cc: Arnd Bergmann <arnd@...db.de>
> Cc: Allison Randal <allison@...utok.net>
> Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> Cc: Gareth Williams <gareth.williams.jx@...esas.com>
> Cc: Rob Herring <robh+dt@...nel.org>
> Cc: linux-mips@...r.kernel.org
> Cc: devicetree@...r.kernel.org
> 
> ---
> 
> Changelog v2:
> - Add a comment about SER register, that it must be set to activate the
>   SPI communications.
> ---
>  drivers/spi/spi-dw.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
> index 6de196df9c96..450c8218caeb 100644
> --- a/drivers/spi/spi-dw.c
> +++ b/drivers/spi/spi-dw.c
> @@ -124,8 +124,16 @@ static inline void dw_spi_debugfs_remove(struct dw_spi *dws)
>  void dw_spi_set_cs(struct spi_device *spi, bool enable)
>  {
>  	struct dw_spi *dws = spi_controller_get_devdata(spi->controller);
> +	bool cs_high = !!(spi->mode & SPI_CS_HIGH);
>  
> -	if (!enable)
> +	/*
> +	 * DW SPI controller demands any native CS being set in order to
> +	 * proceed with data transfer. So in order to activate the SPI
> +	 * communications we must set a corresponding bit in the Slave
> +	 * Enable register no matter whether the SPI core is configured to
> +	 * support active-high or active-low CS level.
> +	 */
> +	if (cs_high == enable)
>  		dw_writel(dws, DW_SPI_SER, BIT(spi->chip_select));
>  	else if (dws->cs_override)
>  		dw_writel(dws, DW_SPI_SER, 0);
> -- 
> 2.25.1
> 

-- 
With Best Regards,
Andy Shevchenko