All of lore.kernel.org
 help / color / mirror / Atom feed
From: Serge Semin <fancer.lancer@gmail.com>
To: Brad Larson <brad@pensando.io>
Cc: linux-arm-kernel@lists.infradead.org, arnd@arndb.de,
	linus.walleij@linaro.org, bgolaszewski@baylibre.com,
	broonie@kernel.org, adrian.hunter@intel.com,
	ulf.hansson@linaro.org, olof@lixom.net,
	linux-gpio@vger.kernel.org, linux-spi@vger.kernel.org,
	linux-mmc@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/8] spi: dw: Add support for Pensando Elba SoC SPI
Date: Thu, 4 Mar 2021 09:44:33 +0300	[thread overview]
Message-ID: <20210304064433.vqyqg3byedvc4quz@mobilestation> (raw)
In-Reply-To: <20210304034141.7062-4-brad@pensando.io>

Hello Brad.
Thanks for the patch. See my comments below.

On Wed, Mar 03, 2021 at 07:41:36PM -0800, Brad Larson wrote:
> The Pensando Elba SoC uses a GPIO based chip select
> for two DW SPI busses with each bus having two
> chip selects.

I see a contradiction here. Normally GPIO-based chip-select is a
property of a platform, but not a SoC/CPU/MCU/etc. Most of the time
SoC SPI interfaces still get to have native CS pins, while at some
platform configurations (like in case of DW APB SPI, which doesn't
provide a way to directly toggle its native CSs) it's easier or even
safer to use GPIOs as CS signals. Of course theoretically a SoC could
be synthesized so it doesn't have native CS output pins, but only some
virtual internal CS flags, but I've never seen such. Anyway according
to the custom CS method below it's not your case because you still
provide support for SPI-devices handled by native CS (else branch in
the if (spi->cs_gpiod) {} else {} statement).

> 
> Signed-off-by: Brad Larson <brad@pensando.io>
> ---
>  drivers/spi/spi-dw-mmio.c | 35 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> index 17c06039a74d..417bd2125c07 100644
> --- a/drivers/spi/spi-dw-mmio.c
> +++ b/drivers/spi/spi-dw-mmio.c
> @@ -56,7 +56,7 @@ struct dw_spi_mscc {
>  /*
>   * The Designware SPI controller (referred to as master in the documentation)
>   * automatically deasserts chip select when the tx fifo is empty. The chip
> - * selects then needs to be either driven as GPIOs or, for the first 4 using the
> + * selects then needs to be either driven as GPIOs or, for the first 4 using
>   * the SPI boot controller registers. the final chip select is an OR gate
>   * between the Designware SPI controller and the SPI boot controller.
>   */
> @@ -237,6 +237,38 @@ static int dw_spi_canaan_k210_init(struct platform_device *pdev,
>  	return 0;
>  }
>
  
> +static void dw_spi_elba_set_cs(struct spi_device *spi, bool enable)
> +{
> +	struct dw_spi *dws = spi_master_get_devdata(spi->master);
> +
> +	if (!enable) {
> +		if (spi->cs_gpiod) {
> +			/*
> +			 * Using a GPIO-based chip-select, the DW SPI
> +			 * controller still needs its own CS bit selected
> +			 * to start the serial engine.  On Elba the specific
> +			 * CS doesn't matter, so use CS0.
> +			 */
> +			dw_writel(dws, DW_SPI_SER, BIT(0));
> +		} else {
> +			/*
> +			 * Using the intrinsic DW chip-select; set the
> +			 * appropriate CS.
> +			 */
> +			dw_writel(dws, DW_SPI_SER, BIT(spi->chip_select));
> +		}
> -	} else
  +	} else {
> +		dw_writel(dws, DW_SPI_SER, 0);
  +	} /* See [1] */
> +}

The custom CS-method above doesn't look much different from the
dw_spi_set_cs() method defined in the spi-dw-core.o driver, except
having at least two problems:
1) It assumes that "enable" argument means the CS-enabling flag, while
in fact it's the CS-level which depending on the SPI_CS_HIGH flag
set/cleared will be 1/0 respectively if CS is supposed to be enabled.
That aspect has already been fixed in the dw_spi_set_cs() method.
2) The method enables CS[0] if GPIO-CS is used for a particular SPI
device. That will cause problems for a GPIO/native CS intermixed case
of having for instance one SPI-device connected to native CS[0] and
another one - to a GPIO. So trying to communicate with the second SPI
device you'll end up having the native CS[0] activated too thus
having an SPI transfer sent to two SPI-device at the same time.
Of course that's not what you'd want.

Anyway I don't really see why you even need a custom CS method here. A
generic method dw_spi_set_cs() shall work for your SPI interface.
If I am wrong, please explain why. Did you try the generic CS method
on your platform?

[1] Placing Braces and Spaces. Chapter 3). Documentation/process/coding-style.rst

> +
> +static int dw_spi_elba_init(struct platform_device *pdev,
> +			    struct dw_spi_mmio *dwsmmio)
> +{
> +	dwsmmio->dws.set_cs = dw_spi_elba_set_cs;
> +
> +	return 0;
> +}
> +
>  static int dw_spi_mmio_probe(struct platform_device *pdev)
>  {
>  	int (*init_func)(struct platform_device *pdev,
> @@ -351,6 +383,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
>  	{ .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
>  	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
>  	{ .compatible = "canaan,k210-spi", dw_spi_canaan_k210_init},

> +	{ .compatible = "pensando,elba-spi", .data = dw_spi_elba_init },

If you agree with me and remove the custom CS-method defined above in
this patch, then all you'll need is just to add "pensando,elba-spi" here
with generic init-callback set - dw_spi_dw_apb_init.

Finally defining new compatible string requires the bindings update.
In the framework of DW APB SPI interface they are defined in:
Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
So you need to have that DT-schema accordingly altered.

The bindings note concerns the rest of the updates in your patchset too.

-Sergey

>  	{ /* end of table */}
>  };
>  MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);
> -- 
> 2.17.1
> 

WARNING: multiple messages have this Message-ID (diff)
From: Serge Semin <fancer.lancer@gmail.com>
To: Brad Larson <brad@pensando.io>
Cc: linux-arm-kernel@lists.infradead.org, arnd@arndb.de,
	linus.walleij@linaro.org, bgolaszewski@baylibre.com,
	broonie@kernel.org, adrian.hunter@intel.com,
	ulf.hansson@linaro.org, olof@lixom.net,
	linux-gpio@vger.kernel.org, linux-spi@vger.kernel.org,
	linux-mmc@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/8] spi: dw: Add support for Pensando Elba SoC SPI
Date: Thu, 4 Mar 2021 09:44:33 +0300	[thread overview]
Message-ID: <20210304064433.vqyqg3byedvc4quz@mobilestation> (raw)
In-Reply-To: <20210304034141.7062-4-brad@pensando.io>

Hello Brad.
Thanks for the patch. See my comments below.

On Wed, Mar 03, 2021 at 07:41:36PM -0800, Brad Larson wrote:
> The Pensando Elba SoC uses a GPIO based chip select
> for two DW SPI busses with each bus having two
> chip selects.

I see a contradiction here. Normally GPIO-based chip-select is a
property of a platform, but not a SoC/CPU/MCU/etc. Most of the time
SoC SPI interfaces still get to have native CS pins, while at some
platform configurations (like in case of DW APB SPI, which doesn't
provide a way to directly toggle its native CSs) it's easier or even
safer to use GPIOs as CS signals. Of course theoretically a SoC could
be synthesized so it doesn't have native CS output pins, but only some
virtual internal CS flags, but I've never seen such. Anyway according
to the custom CS method below it's not your case because you still
provide support for SPI-devices handled by native CS (else branch in
the if (spi->cs_gpiod) {} else {} statement).

> 
> Signed-off-by: Brad Larson <brad@pensando.io>
> ---
>  drivers/spi/spi-dw-mmio.c | 35 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> index 17c06039a74d..417bd2125c07 100644
> --- a/drivers/spi/spi-dw-mmio.c
> +++ b/drivers/spi/spi-dw-mmio.c
> @@ -56,7 +56,7 @@ struct dw_spi_mscc {
>  /*
>   * The Designware SPI controller (referred to as master in the documentation)
>   * automatically deasserts chip select when the tx fifo is empty. The chip
> - * selects then needs to be either driven as GPIOs or, for the first 4 using the
> + * selects then needs to be either driven as GPIOs or, for the first 4 using
>   * the SPI boot controller registers. the final chip select is an OR gate
>   * between the Designware SPI controller and the SPI boot controller.
>   */
> @@ -237,6 +237,38 @@ static int dw_spi_canaan_k210_init(struct platform_device *pdev,
>  	return 0;
>  }
>
  
> +static void dw_spi_elba_set_cs(struct spi_device *spi, bool enable)
> +{
> +	struct dw_spi *dws = spi_master_get_devdata(spi->master);
> +
> +	if (!enable) {
> +		if (spi->cs_gpiod) {
> +			/*
> +			 * Using a GPIO-based chip-select, the DW SPI
> +			 * controller still needs its own CS bit selected
> +			 * to start the serial engine.  On Elba the specific
> +			 * CS doesn't matter, so use CS0.
> +			 */
> +			dw_writel(dws, DW_SPI_SER, BIT(0));
> +		} else {
> +			/*
> +			 * Using the intrinsic DW chip-select; set the
> +			 * appropriate CS.
> +			 */
> +			dw_writel(dws, DW_SPI_SER, BIT(spi->chip_select));
> +		}
> -	} else
  +	} else {
> +		dw_writel(dws, DW_SPI_SER, 0);
  +	} /* See [1] */
> +}

The custom CS-method above doesn't look much different from the
dw_spi_set_cs() method defined in the spi-dw-core.o driver, except
having at least two problems:
1) It assumes that "enable" argument means the CS-enabling flag, while
in fact it's the CS-level which depending on the SPI_CS_HIGH flag
set/cleared will be 1/0 respectively if CS is supposed to be enabled.
That aspect has already been fixed in the dw_spi_set_cs() method.
2) The method enables CS[0] if GPIO-CS is used for a particular SPI
device. That will cause problems for a GPIO/native CS intermixed case
of having for instance one SPI-device connected to native CS[0] and
another one - to a GPIO. So trying to communicate with the second SPI
device you'll end up having the native CS[0] activated too thus
having an SPI transfer sent to two SPI-device at the same time.
Of course that's not what you'd want.

Anyway I don't really see why you even need a custom CS method here. A
generic method dw_spi_set_cs() shall work for your SPI interface.
If I am wrong, please explain why. Did you try the generic CS method
on your platform?

[1] Placing Braces and Spaces. Chapter 3). Documentation/process/coding-style.rst

> +
> +static int dw_spi_elba_init(struct platform_device *pdev,
> +			    struct dw_spi_mmio *dwsmmio)
> +{
> +	dwsmmio->dws.set_cs = dw_spi_elba_set_cs;
> +
> +	return 0;
> +}
> +
>  static int dw_spi_mmio_probe(struct platform_device *pdev)
>  {
>  	int (*init_func)(struct platform_device *pdev,
> @@ -351,6 +383,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
>  	{ .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
>  	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
>  	{ .compatible = "canaan,k210-spi", dw_spi_canaan_k210_init},

> +	{ .compatible = "pensando,elba-spi", .data = dw_spi_elba_init },

If you agree with me and remove the custom CS-method defined above in
this patch, then all you'll need is just to add "pensando,elba-spi" here
with generic init-callback set - dw_spi_dw_apb_init.

Finally defining new compatible string requires the bindings update.
In the framework of DW APB SPI interface they are defined in:
Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
So you need to have that DT-schema accordingly altered.

The bindings note concerns the rest of the updates in your patchset too.

-Sergey

>  	{ /* end of table */}
>  };
>  MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2021-03-04  6:46 UTC|newest]

Thread overview: 102+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-04  3:41 [PATCH 0/8] Support Pensando Elba SoC Brad Larson
2021-03-04  3:41 ` Brad Larson
2021-03-04  3:41 ` [PATCH 1/8] gpio: Add Elba SoC gpio driver for spi cs control Brad Larson
2021-03-04  3:41   ` Brad Larson
2021-03-04  8:29   ` Linus Walleij
2021-03-04  8:29     ` Linus Walleij
2021-03-04  9:10     ` Serge Semin
2021-03-04  9:10       ` Serge Semin
2021-03-04 13:38       ` Linus Walleij
2021-03-04 13:38         ` Linus Walleij
2021-08-23  1:05         ` Brad Larson
2021-08-23  1:05           ` Brad Larson
2021-08-29 21:09           ` Linus Walleij
2021-08-29 21:09             ` Linus Walleij
2021-10-04 16:46             ` Brad Larson
2021-10-04 16:46               ` Brad Larson
2021-10-12 23:51               ` Linus Walleij
2021-10-12 23:51                 ` Linus Walleij
2021-10-14 20:06                 ` Brad Larson
2021-10-14 20:06                   ` Brad Larson
2021-03-30  2:44     ` Brad Larson
2021-03-30  2:44       ` Brad Larson
2021-08-23  1:05     ` Brad Larson
2021-08-23  1:05       ` Brad Larson
2021-03-04 20:43   ` Elliott, Robert (Servers)
2021-03-04 20:43     ` Elliott, Robert (Servers)
2021-08-23  1:06     ` Brad Larson
2021-08-23  1:06       ` Brad Larson
2021-03-05 11:25   ` Krzysztof Kozlowski
2021-03-05 11:25     ` Krzysztof Kozlowski
2021-08-23  1:07     ` Brad Larson
2021-08-23  1:07       ` Brad Larson
2021-03-05 13:57   ` Geert Uytterhoeven
2021-03-05 13:57     ` Geert Uytterhoeven
2021-08-23  1:08     ` Brad Larson
2021-08-23  1:08       ` Brad Larson
2021-03-07 19:21   ` Andy Shevchenko
2021-03-07 19:21     ` Andy Shevchenko
2021-03-29  1:19     ` Brad Larson
2021-03-29  1:19       ` Brad Larson
2021-03-29 10:39       ` Andy Shevchenko
2021-03-29 10:39         ` Andy Shevchenko
2021-08-23  1:13         ` Brad Larson
2021-08-23  1:13           ` Brad Larson
2021-08-23  7:50           ` Geert Uytterhoeven
2021-08-23  7:50             ` Geert Uytterhoeven
2021-08-23 16:30             ` Brad Larson
2021-08-23 16:30               ` Brad Larson
2021-08-23 20:11               ` Geert Uytterhoeven
2021-08-23 20:11                 ` Geert Uytterhoeven
2021-10-04 17:14                 ` Brad Larson
2021-10-04 17:14                   ` Brad Larson
2021-10-04 17:16                   ` Geert Uytterhoeven
2021-10-04 17:16                     ` Geert Uytterhoeven
2021-08-23  1:10     ` Brad Larson
2021-08-23  1:10       ` Brad Larson
2021-03-04  3:41 ` [PATCH 2/8] spi: cadence-quadspi: Add QSPI support for Pensando Elba SoC Brad Larson
2021-03-04  3:41   ` Brad Larson
2021-03-04  9:29   ` Arnd Bergmann
2021-03-04  9:29     ` Arnd Bergmann
2021-03-04  3:41 ` [PATCH 3/8] spi: dw: Add support for Pensando Elba SoC SPI Brad Larson
2021-03-04  3:41   ` Brad Larson
2021-03-04  6:44   ` Serge Semin [this message]
2021-03-04  6:44     ` Serge Semin
2021-08-23  1:17     ` Brad Larson
2021-08-23  1:17       ` Brad Larson
2021-03-04  8:48   ` Linus Walleij
2021-03-04  8:48     ` Linus Walleij
2021-03-10  3:52     ` Brad Larson
2021-03-10  3:52       ` Brad Larson
2021-03-04  3:41 ` [PATCH 4/8] spidev: Add Pensando CPLD compatible Brad Larson
2021-03-04  3:41   ` Brad Larson
2021-03-04  9:33   ` Arnd Bergmann
2021-03-04  9:33     ` Arnd Bergmann
2021-03-04  3:41 ` [PATCH 5/8] mmc: sdhci-cadence: Add Pensando Elba SoC support Brad Larson
2021-03-04  3:41   ` Brad Larson
2021-03-04  9:41   ` Arnd Bergmann
2021-03-04  9:41     ` Arnd Bergmann
2021-03-04  3:41 ` [PATCH 6/8] arm64: Add config for Pensando SoC platforms Brad Larson
2021-03-04  3:41   ` Brad Larson
2021-03-04  9:42   ` Arnd Bergmann
2021-03-04  9:42     ` Arnd Bergmann
2021-03-04  3:41 ` [PATCH 7/8] arm64: dts: Add Pensando Elba SoC support Brad Larson
2021-03-04  3:41   ` Brad Larson
2021-03-04  8:03   ` Serge Semin
2021-03-04  8:03     ` Serge Semin
2021-03-29  1:07     ` Brad Larson
2021-03-29  1:07       ` Brad Larson
2021-08-23  0:54     ` Brad Larson
2021-08-23  0:54       ` Brad Larson
2021-03-04  8:51   ` Linus Walleij
2021-03-04  8:51     ` Linus Walleij
2021-03-29  0:54     ` Brad Larson
2021-03-29  0:54       ` Brad Larson
2021-03-04  9:06   ` Arnd Bergmann
2021-03-04  9:06     ` Arnd Bergmann
2021-03-04 20:47   ` Rob Herring
2021-03-04 20:47     ` Rob Herring
2021-03-05 11:22   ` Krzysztof Kozlowski
2021-03-05 11:22     ` Krzysztof Kozlowski
2021-03-04  3:41 ` [PATCH 8/8] MAINTAINERS: Add entry for PENSANDO Brad Larson
2021-03-04  3:41   ` Brad Larson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210304064433.vqyqg3byedvc4quz@mobilestation \
    --to=fancer.lancer@gmail.com \
    --cc=adrian.hunter@intel.com \
    --cc=arnd@arndb.de \
    --cc=bgolaszewski@baylibre.com \
    --cc=brad@pensando.io \
    --cc=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mmc@vger.kernel.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=olof@lixom.net \
    --cc=ulf.hansson@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.