linux-kernel - Re: [PATCH v4 3/3] spi: dw: Add support for master mode selection for DWC SSI controller

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220413130249.ueisqj5xs3komhmr@mobilestation>
Date:   Wed, 13 Apr 2022 16:02:49 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     nandhini.srikandan@...el.com
Cc:     Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        broonie@...nel.org, robh+dt@...nel.org, linux-spi@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        mgross@...ux.intel.com, kris.pan@...el.com,
        kenchappa.demakkanavar@...el.com, furong.zhou@...el.com,
        mallikarjunappa.sangannavar@...el.com, mahesh.r.vaidya@...el.com,
        rashmi.a@...el.com
Subject: Re: [PATCH v4 3/3] spi: dw: Add support for master mode selection
 for DWC SSI controller

Hello Nandhini

AFAICS this patch should go before
[PATCH v4 2/3] spi: dw: Add support for Intel Thunder Bay SPI
controller Thus you'd perform the DWC AHB SSI Master mode conversion
first, then introduce the new controller support. Otherwise without
this patch applied the DW SPI driver is most likely left broken for
the Intel SPI controllers since you drop the DW_SPI_CAP_KEEMBAY_MST
macro usage in [PATCH 2/3] while the new DW AHB SSI Master
functionality is introduced in the next patch [PATCH 3/3]. So please
convert the series to the harmless configuration on each git image
state.

On Tue, Mar 08, 2022 at 06:33:31PM +0800, nandhini.srikandan@...el.com wrote:
> From: Nandhini Srikandan <nandhini.srikandan@...el.com>
> 
> Add support to select the controller mode as master mode by setting
> Bit 31 of CTRLR0 register. This feature is supported for controller
> versions above v1.02.
> 
> Signed-off-by: Nandhini Srikandan <nandhini.srikandan@...el.com>
> ---
>  drivers/spi/spi-dw-core.c | 4 ++--
>  drivers/spi/spi-dw.h      | 7 +++----
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index ecea471ff42c..68bfdf2c4dc7 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -307,8 +307,8 @@ static u32 dw_spi_prepare_cr0(struct dw_spi *dws, struct spi_device *spi)
>  		if (spi->mode & SPI_LOOP)
>  			cr0 |= DW_HSSI_CTRLR0_SRL;
>  

> -		if (dws->caps & DW_SPI_CAP_KEEMBAY_MST)
> -			cr0 |= DW_HSSI_CTRLR0_KEEMBAY_MST;
> +		/* CTRLR0[31] MST */
> +		cr0 |= DW_HSSI_CTRLR0_MST;

Could you please conditionally set that flag here? That's what we
agreed to do in v3:
https://lore.kernel.org/linux-spi/20211116191542.vc42cxvflzn66ien@mobilestation/
like this:
+	/* CTRLR0[31] MST */
+	if (dw_spi_ver_is_ge(dws, HSSI, 102A))
+		cr0 |= DWC_HSSI_CTRLR0_MST;

>  	}
>  
>  	return cr0;
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index d5ee5130601e..2583b7314c41 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -23,7 +23,7 @@
>  	((_dws)->ip == DW_ ## _ip ## _ID)
>  

>  #define __dw_spi_ver_cmp(_dws, _ip, _ver, _op) \
> -	(dw_spi_ip_is(_dws, _ip) && (_dws)->ver _op DW_ ## _ip ## _ver)
> +	(dw_spi_ip_is(_dws, _ip) && (_dws)->ver _op DW_ ## _ip ## _ ## _ver)

Nice catch. My mistake. Could you please move this change into a
dedicated patch with the next fixes tag?
Fixes: 2cc8d9227bbb ("spi: dw: Introduce Synopsys IP-core versions interface")

>  
>  #define dw_spi_ver_is(_dws, _ip, _ver) __dw_spi_ver_cmp(_dws, _ip, _ver, ==)
>  
> @@ -31,8 +31,7 @@
>  
>  /* DW SPI controller capabilities */
>  #define DW_SPI_CAP_CS_OVERRIDE		BIT(0)
> -#define DW_SPI_CAP_KEEMBAY_MST		BIT(1)
> -#define DW_SPI_CAP_DFS32		BIT(2)
> +#define DW_SPI_CAP_DFS32		BIT(1)
>  
>  /* Register offsets (Generic for both DWC APB SSI and DWC SSI IP-cores) */
>  #define DW_SPI_CTRLR0			0x00
> @@ -100,7 +99,7 @@

>   * 0: SSI is slave
>   * 1: SSI is master
>   */
> -#define DW_HSSI_CTRLR0_KEEMBAY_MST		BIT(31)
> +#define DW_HSSI_CTRLR0_MST			BIT(31)

Could you please drop the redundant comment above and join the
macro with the DW_HSSI_* macros group?

-Sergey

>  
>  /* Bit fields in CTRLR1 */
>  #define DW_SPI_NDF_MASK				GENMASK(15, 0)
> -- 
> 2.17.1
>