diff mbox series

[V1,1/6] arm64: tegra: Configure QSPI clocks and add DMA

Message ID 20250103060407.1064107-2-va@nvidia.com
State New
Headers show
Series Configure Clocks, Add Native Dma support. | expand

Commit Message

Vishwaroop A Jan. 3, 2025, 6:04 a.m. UTC
Set QSPI0_2X_PM to 199.99 MHz and QSPI0_PM to 99.99 MHz using
PLLC as the parent clock. These frequencies allow Quad IO DT
reads up to 99.99 MHz, which is the fastest that can be
achieved considering various PLL and clock divider constraints.

Populate the DMA and IOMMU properties for the Tegra234 QSPI devices to
enable DMA support.

Change-Id: I1dded904aa8e0f278c89998481e829f1ce474e8c
Signed-off-by: Vishwaroop A <va@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra234.dtsi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Thierry Reding Jan. 9, 2025, 10:40 a.m. UTC | #1
On Fri, Jan 03, 2025 at 06:04:02AM +0000, Vishwaroop A wrote:
> Set QSPI0_2X_PM to 199.99 MHz and QSPI0_PM to 99.99 MHz using
> PLLC as the parent clock. These frequencies allow Quad IO DT
> reads up to 99.99 MHz, which is the fastest that can be
> achieved considering various PLL and clock divider constraints.
> 
> Populate the DMA and IOMMU properties for the Tegra234 QSPI devices to
> enable DMA support.
> 
> Change-Id: I1dded904aa8e0f278c89998481e829f1ce474e8c
> Signed-off-by: Vishwaroop A <va@nvidia.com>
> ---
>  arch/arm64/boot/dts/nvidia/tegra234.dtsi | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra234.dtsi b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> index 984c85eab41a..96d0f13390ae 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> @@ -2948,6 +2948,13 @@
>  				 <&bpmp TEGRA234_CLK_QSPI0_PM>;
>  			clock-names = "qspi", "qspi_out";
>  			resets = <&bpmp TEGRA234_RESET_QSPI0>;
> +			assigned-clocks = <&bpmp TEGRA234_CLK_QSPI0_2X_PM>,
> +					  <&bpmp TEGRA234_CLK_QSPI0_PM>;
> +			assigned-clock-rates = <199999999 99999999>;
> +			assigned-clock-parents = <&bpmp TEGRA234_CLK_PLLC>;
> +			dma-names = "rx", "tx";
> +			dma-coherent;
> +			iommus = <&smmu_niso1 TEGRA234_SID_QSPI0>;
>  			status = "disabled";
>  		};
>  
> @@ -3031,6 +3038,13 @@
>  				 <&bpmp TEGRA234_CLK_QSPI1_PM>;
>  			clock-names = "qspi", "qspi_out";
>  			resets = <&bpmp TEGRA234_RESET_QSPI1>;
> +			assigned-clocks = <&bpmp TEGRA234_CLK_QSPI1_2X_PM>,
> +					  <&bpmp TEGRA234_CLK_QSPI1_PM>;
> +			assigned-clock-rates = <199999999 99999999>;
> +			assigned-clock-parents = <&bpmp TEGRA234_CLK_PLLC>;
> +			dma-names = "rx", "tx";
> +			dma-coherent;
> +			iommus = <&smmu_niso1 TEGRA234_SID_QSPI1>;
>  			status = "disabled";
>  		};
>  

It looks like these are missing the "dmas" properties that go along with
"dma-names".

Thierry
Rob Herring Feb. 12, 2025, 10:08 p.m. UTC | #2
On Wed, 12 Feb 2025 14:46:45 +0000, Vishwaroop A wrote:
> This patch series configures qspi clocks, fixes combined sequence
> programming and introduces native dma support.
> 
> Vishwaroop A (6):
>   arm64: tegra: Configure QSPI clocks and add DMA
>   spi: tegra210-quad: Update dummy sequence configuration
>   spi: tegra210-quad: Fix X1_X2_X4 encoding and support x4 transfers
>   spi: tegra210-quad: remove redundant error handling code
>   spi: tegra210-quad: modify chip select (CS) deactivation
>   spi: tegra210-quad: Introduce native DMA support
> 
>  arch/arm64/boot/dts/nvidia/tegra234.dtsi |  14 ++
>  drivers/spi/spi-tegra210-quad.c          | 273 +++++++++++++----------
>  2 files changed, 174 insertions(+), 113 deletions(-)
> 
> ---
> v1 -> v2:
> 	* Removed Change-IDs from the patches.
>         * Addressed kernel test bot warnings.
> ---
> --
> 2.17.1
> 
> 
> 


My bot found new DTB warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


New warnings running 'make CHECK_DTBS=y for arch/arm64/boot/dts/nvidia/' for 20250212144651.2433086-1-va@nvidia.com:

arch/arm64/boot/dts/nvidia/tegra234-sim-vdk.dtb: spi@3270000: 'dmas' is a dependency of 'dma-names'
	from schema $id: http://devicetree.org/schemas/dma/dma.yaml#
arch/arm64/boot/dts/nvidia/tegra234-p3768-0000+p3767-0000.dtb: spi@3270000: Unevaluated properties are not allowed ('dma-coherent', 'iommus' were unexpected)
	from schema $id: http://devicetree.org/schemas/spi/nvidia,tegra210-quad.yaml#
arch/arm64/boot/dts/nvidia/tegra234-p3768-0000+p3767-0000.dtb: spi@3270000: 'dmas' is a dependency of 'dma-names'
	from schema $id: http://devicetree.org/schemas/dma/dma.yaml#
arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0008.dtb: spi@3270000: Unevaluated properties are not allowed ('dma-coherent', 'iommus' were unexpected)
	from schema $id: http://devicetree.org/schemas/spi/nvidia,tegra210-quad.yaml#
arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0008.dtb: spi@3270000: 'dmas' is a dependency of 'dma-names'
	from schema $id: http://devicetree.org/schemas/dma/dma.yaml#
arch/arm64/boot/dts/nvidia/tegra234-sim-vdk.dtb: spi@3300000: 'dmas' is a dependency of 'dma-names'
	from schema $id: http://devicetree.org/schemas/dma/dma.yaml#
arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dtb: spi@3270000: Unevaluated properties are not allowed ('dma-coherent', 'iommus' were unexpected)
	from schema $id: http://devicetree.org/schemas/spi/nvidia,tegra210-quad.yaml#
arch/arm64/boot/dts/nvidia/tegra234-p3768-0000+p3767-0005.dtb: spi@3270000: Unevaluated properties are not allowed ('dma-coherent', 'iommus' were unexpected)
	from schema $id: http://devicetree.org/schemas/spi/nvidia,tegra210-quad.yaml#
arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dtb: spi@3270000: 'dmas' is a dependency of 'dma-names'
	from schema $id: http://devicetree.org/schemas/dma/dma.yaml#
arch/arm64/boot/dts/nvidia/tegra234-p3768-0000+p3767-0005.dtb: spi@3270000: 'dmas' is a dependency of 'dma-names'
	from schema $id: http://devicetree.org/schemas/dma/dma.yaml#
arch/arm64/boot/dts/nvidia/tegra234-p3740-0002+p3701-0008.dtb: spi@3270000: Unevaluated properties are not allowed ('dma-coherent', 'iommus' were unexpected)
	from schema $id: http://devicetree.org/schemas/spi/nvidia,tegra210-quad.yaml#
arch/arm64/boot/dts/nvidia/tegra234-p3768-0000+p3767-0000.dtb: spi@3300000: 'dmas' is a dependency of 'dma-names'
	from schema $id: http://devicetree.org/schemas/dma/dma.yaml#
arch/arm64/boot/dts/nvidia/tegra234-p3740-0002+p3701-0008.dtb: spi@3270000: 'dmas' is a dependency of 'dma-names'
	from schema $id: http://devicetree.org/schemas/dma/dma.yaml#
arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0008.dtb: spi@3300000: 'dmas' is a dependency of 'dma-names'
	from schema $id: http://devicetree.org/schemas/dma/dma.yaml#
arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dtb: spi@3300000: 'dmas' is a dependency of 'dma-names'
	from schema $id: http://devicetree.org/schemas/dma/dma.yaml#
arch/arm64/boot/dts/nvidia/tegra234-p3768-0000+p3767-0005.dtb: spi@3300000: 'dmas' is a dependency of 'dma-names'
	from schema $id: http://devicetree.org/schemas/dma/dma.yaml#
arch/arm64/boot/dts/nvidia/tegra234-p3740-0002+p3701-0008.dtb: spi@3300000: 'dmas' is a dependency of 'dma-names'
	from schema $id: http://devicetree.org/schemas/dma/dma.yaml#
Thierry Reding Feb. 27, 2025, 10:39 a.m. UTC | #3
On Wed, Feb 12, 2025 at 02:46:46PM +0000, Vishwaroop A wrote:
> Set QSPI0_2X_PM to 199.99 MHz and QSPI0_PM to 99.99 MHz using
> PLLC as the parent clock. These frequencies allow Quad IO DT
> reads up to 99.99 MHz, which is the fastest that can be
> achieved considering various PLL and clock divider constraints.
> 
> Populate the DMA and IOMMU properties for the Tegra234 QSPI devices to
> enable DMA support.
> 
> Signed-off-by: Vishwaroop A <va@nvidia.com>
> ---
>  arch/arm64/boot/dts/nvidia/tegra234.dtsi | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra234.dtsi b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> index 2601b43b2d8c..0ac2d3aba930 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> @@ -2948,6 +2948,13 @@
>  				 <&bpmp TEGRA234_CLK_QSPI0_PM>;
>  			clock-names = "qspi", "qspi_out";
>  			resets = <&bpmp TEGRA234_RESET_QSPI0>;
> +			assigned-clocks = <&bpmp TEGRA234_CLK_QSPI0_2X_PM>,
> +					  <&bpmp TEGRA234_CLK_QSPI0_PM>;
> +			assigned-clock-rates = <199999999 99999999>;
> +			assigned-clock-parents = <&bpmp TEGRA234_CLK_PLLC>;
> +			dma-names = "rx", "tx";

Other than the dma-names property which was already discussed, this
looks fine.

Thierry
Thierry Reding Feb. 27, 2025, 10:42 a.m. UTC | #4
On Wed, Feb 12, 2025 at 02:46:47PM +0000, Vishwaroop A wrote:
> Adding support for the dummy sequence configuration. The dummy sequence
> introduces a delay between the command and the data phases of a
> transfer. This delay, measured in clock cycles, allows the slave
> device to prepare for data transmission, ensuring data integrity and
> proper synchronization.
> 
> Signed-off-by: Vishwaroop A <va@nvidia.com>
> ---
>  drivers/spi/spi-tegra210-quad.c | 31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)

Acked-by: Thierry Reding <treding@nvidia.com>
Thierry Reding Feb. 27, 2025, 10:45 a.m. UTC | #5
On Wed, Feb 12, 2025 at 02:46:48PM +0000, Vishwaroop A wrote:
> This patch corrects the QSPI_COMMAND_X1_X2_X4 and QSPI_ADDRESS_X1_X2_X4
> macros to properly encode the bus width for x1, x2, and x4 transfers.
> Although these macros were previously incorrect, they were not being
> used in the driver, so no functionality was affected.
> 
> The patch updates tegra_qspi_cmd_config() and tegra_qspi_addr_config()
> function calls to use the actual bus width from the transfer, instead of
> hardcoding it to 0 (which implied x1 mode). This change enables proper
> support for x1, x2, and x4 data transfers by correctly configuring the
> interface width for commands and addresses.
> 
> These modifications improve the QSPI driver's flexibility and prepare it
> for future use cases that may require different bus widths for commands
> and addresses.
> 
> Fixes: 1b8342cc4a38 ("spi: tegra210-quad: combined sequence mode")
> 
> Signed-off-by: Vishwaroop A <va@nvidia.com>

With that blank line between Fixes: and S-o-b: dropped, this is:

Acked-by: Thierry Reding <treding@nvidia.com>
Thierry Reding Feb. 27, 2025, 10:45 a.m. UTC | #6
On Wed, Feb 12, 2025 at 02:46:49PM +0000, Vishwaroop A wrote:
> Remove unnecessary error handling code that terminated transfers and
> executed delay on errors. This code was redundant as error handling is
> already done at a higher level in the SPI core.
> 
> Fixes: 1b8342cc4a38 ("spi: tegra210-quad: combined sequence mode")
> 
> Signed-off-by: Vishwaroop A <va@nvidia.com>

Again, the blank line can be dropped. Otherwise:

Acked-by: Thierry Reding <treding@nvidia.com>
Thierry Reding Feb. 27, 2025, 10:46 a.m. UTC | #7
On Wed, Feb 12, 2025 at 02:46:50PM +0000, Vishwaroop A wrote:
> Modify the chip select (CS) deactivation and inter-transfer delay
> execution to be performed only during the DATA_TRANSFER phase when
> the cs_change flag is not set. This ensures proper CS handling and
> timing between transfers while eliminating redundant operations.
> 
> Fixes: 1b8342cc4a38 ("spi: tegra210-quad: combined sequence mode")
> 
> Signed-off-by: Vishwaroop A <va@nvidia.com>
> ---
>  drivers/spi/spi-tegra210-quad.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-tegra210-quad.c b/drivers/spi/spi-tegra210-quad.c
> index 2d7a2e3da337..04f41e92c1e2 100644
> --- a/drivers/spi/spi-tegra210-quad.c
> +++ b/drivers/spi/spi-tegra210-quad.c
> @@ -1171,16 +1171,16 @@ static int tegra_qspi_combined_seq_xfer(struct tegra_qspi *tqspi,
>  				ret = -EIO;
>  				goto exit;
>  			}
> -			if (!xfer->cs_change) {
> -				tegra_qspi_transfer_end(spi);
> -				spi_transfer_delay_exec(xfer);
> -			}
>  			break;
>  		default:
>  			ret = -EINVAL;
>  			goto exit;
>  		}
>  		msg->actual_length += xfer->len;
> +		if (!xfer->cs_change && transfer_phase == DATA_TRANSFER) {
> +			tegra_qspi_transfer_end(spi);
> +			spi_transfer_delay_exec(xfer);
> +		}
>  		transfer_phase++;

I'd prefer blank lines around the "if" block to make this less
cluttered, but since this was always like this, either way:

Acked-by: Thierry Reding <treding@nvidia.com>
Jon Hunter Feb. 27, 2025, 11:09 a.m. UTC | #8
On 12/02/2025 14:46, Vishwaroop A wrote:
> This patch series configures qspi clocks, fixes combined sequence
> programming and introduces native dma support.
> 
> Vishwaroop A (6):
>    arm64: tegra: Configure QSPI clocks and add DMA
>    spi: tegra210-quad: Update dummy sequence configuration
>    spi: tegra210-quad: Fix X1_X2_X4 encoding and support x4 transfers
>    spi: tegra210-quad: remove redundant error handling code
>    spi: tegra210-quad: modify chip select (CS) deactivation
>    spi: tegra210-quad: Introduce native DMA support

Can you re-order these so that all fixes are first and then new features 
are 2nd?

Thanks
Jon
Jon Hunter Feb. 27, 2025, 11:13 a.m. UTC | #9
On 12/02/2025 14:46, Vishwaroop A wrote:
> Set QSPI0_2X_PM to 199.99 MHz and QSPI0_PM to 99.99 MHz using
> PLLC as the parent clock. These frequencies allow Quad IO DT
> reads up to 99.99 MHz, which is the fastest that can be
> achieved considering various PLL and clock divider constraints.
> 
> Populate the DMA and IOMMU properties for the Tegra234 QSPI devices to
> enable DMA support.
> 
> Signed-off-by: Vishwaroop A <va@nvidia.com>
> ---
>   arch/arm64/boot/dts/nvidia/tegra234.dtsi | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra234.dtsi b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> index 2601b43b2d8c..0ac2d3aba930 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> @@ -2948,6 +2948,13 @@
>   				 <&bpmp TEGRA234_CLK_QSPI0_PM>;
>   			clock-names = "qspi", "qspi_out";
>   			resets = <&bpmp TEGRA234_RESET_QSPI0>;
> +			assigned-clocks = <&bpmp TEGRA234_CLK_QSPI0_2X_PM>,
> +					  <&bpmp TEGRA234_CLK_QSPI0_PM>;
> +			assigned-clock-rates = <199999999 99999999>;
> +			assigned-clock-parents = <&bpmp TEGRA234_CLK_PLLC>;
> +			dma-names = "rx", "tx";
> +			dma-coherent;
> +			iommus = <&smmu_niso1 TEGRA234_SID_QSPI0>;
>   			status = "disabled";
>   		};
>   
> @@ -3031,6 +3038,13 @@
>   				 <&bpmp TEGRA234_CLK_QSPI1_PM>;
>   			clock-names = "qspi", "qspi_out";
>   			resets = <&bpmp TEGRA234_RESET_QSPI1>;
> +			assigned-clocks = <&bpmp TEGRA234_CLK_QSPI1_2X_PM>,
> +					  <&bpmp TEGRA234_CLK_QSPI1_PM>;
> +			assigned-clock-rates = <199999999 99999999>;
> +			assigned-clock-parents = <&bpmp TEGRA234_CLK_PLLC>;
> +			dma-names = "rx", "tx";
> +			dma-coherent;
> +			iommus = <&smmu_niso1 TEGRA234_SID_QSPI1>;
>   			status = "disabled";
>   		};
>   


Make sure you CC the DT mailing list on this.

With this change I am seeing the following warnings ...

arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dtb: spi@3270000: Unevaluated properties are not allowed ('dma-coherent', 'iommus' were unexpected)
	from schema $id: http://devicetree.org/schemas/spi/nvidia,tegra210-quad.yaml#
   DTC [C] arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0008.dtb
arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0008.dtb: spi@3270000: Unevaluated properties are not allowed ('dma-coherent', 'iommus' were unexpected)
	from schema $id: http://devicetree.org/schemas/spi/nvidia,tegra210-quad.yaml#
   DTC [C] arch/arm64/boot/dts/nvidia/tegra234-p3740-0002+p3701-0008.dtb
arch/arm64/boot/dts/nvidia/tegra234-p3740-0002+p3701-0008.dtb: spi@3270000: Unevaluated properties are not allowed ('dma-coherent', 'iommus' were unexpected)
	from schema $id: http://devicetree.org/schemas/spi/nvidia,tegra210-quad.yaml#
   DTC [C] arch/arm64/boot/dts/nvidia/tegra234-p3768-0000+p3767-0000.dtb
arch/arm64/boot/dts/nvidia/tegra234-p3768-0000+p3767-0000.dtb: spi@3270000: Unevaluated properties are not allowed ('dma-coherent', 'iommus' were unexpected)
	from schema $id: http://devicetree.org/schemas/spi/nvidia,tegra210-quad.yaml#
   DTC [C] arch/arm64/boot/dts/nvidia/tegra234-p3768-0000+p3767-0005.dtb
arch/arm64/boot/dts/nvidia/tegra234-p3768-0000+p3767-0005.dtb: spi@3270000: Unevaluated properties are not allowed ('dma-coherent', 'iommus' were unexpected)
	from schema $id: http://devicetree.org/schemas/spi/nvidia,tegra210-quad.yaml#

Jon
Thierry Reding Feb. 27, 2025, 11:14 a.m. UTC | #10
On Wed, Feb 12, 2025 at 02:46:51PM +0000, Vishwaroop A wrote:
> Previous generations of Tegra supported DMA operations by an external
> DMA controller, but the QSPI on Tegra234 devices now have an internal
> DMA controller.
> 
> Internal DMA: Uses the QSPI controller's built-in DMA engine, which is
> limited in capabilities and tied directly to the QSPI module.
> 
> External DMA: Utilizes a separate, GPCDMA DMA controller that can
> transfer data between QSPI and any memory location.
> 
> Native DMA Initialization: Introduce routines to initialize and
> configure native DMA channels for both transmit and receive paths.
> Set up DMA mapping functions to manage buffer addresses effectively.
> 
> Enhance Transfer Logic: Implement logic to choose between CPU-based
> and DMA-based transfers based on data size.
> 
> Signed-off-by: Vishwaroop A <va@nvidia.com>
> ---
>  drivers/spi/spi-tegra210-quad.c | 218 ++++++++++++++++++--------------
>  1 file changed, 126 insertions(+), 92 deletions(-)
> 
> diff --git a/drivers/spi/spi-tegra210-quad.c b/drivers/spi/spi-tegra210-quad.c
> index 04f41e92c1e2..7463b00b1ffb 100644
> --- a/drivers/spi/spi-tegra210-quad.c
> +++ b/drivers/spi/spi-tegra210-quad.c
> @@ -111,6 +111,9 @@
>  #define QSPI_DMA_BLK				0x024
>  #define QSPI_DMA_BLK_SET(x)			(((x) & 0xffff) << 0)
>  
> +#define QSPI_DMA_MEM_ADDRESS_REG		0x028
> +#define QSPI_DMA_HI_ADDRESS_REG			0x02c

I'd drop the _REG suffix since we don't use it on any of the other
register definitions.

> +
>  #define QSPI_TX_FIFO				0x108
>  #define QSPI_RX_FIFO				0x188
>  
> @@ -167,9 +170,9 @@ enum tegra_qspi_transfer_type {
>  };
>  
>  struct tegra_qspi_soc_data {
> -	bool has_dma;
>  	bool cmb_xfer_capable;
>  	bool supports_tpm;
> +	bool has_ext_dma;
>  	unsigned int cs_count;
>  };
>  
> @@ -605,17 +608,21 @@ static void tegra_qspi_dma_unmap_xfer(struct tegra_qspi *tqspi, struct spi_trans
>  
>  	len = DIV_ROUND_UP(tqspi->curr_dma_words * tqspi->bytes_per_word, 4) * 4;
>  
> -	dma_unmap_single(tqspi->dev, t->tx_dma, len, DMA_TO_DEVICE);
> -	dma_unmap_single(tqspi->dev, t->rx_dma, len, DMA_FROM_DEVICE);
> +	if (t->tx_buf)
> +		dma_unmap_single(tqspi->dev, t->tx_dma, len, DMA_TO_DEVICE);
> +	if (t->rx_buf)
> +		dma_unmap_single(tqspi->dev, t->rx_dma, len, DMA_FROM_DEVICE);
>  }
>  
>  static int tegra_qspi_start_dma_based_transfer(struct tegra_qspi *tqspi, struct spi_transfer *t)
>  {
>  	struct dma_slave_config dma_sconfig = { 0 };
> +	dma_addr_t rx_dma_phys, tx_dma_phys;
>  	unsigned int len;
>  	u8 dma_burst;
>  	int ret = 0;
>  	u32 val;
> +	bool has_ext_dma = tqspi->soc_data->has_ext_dma;
>  
>  	if (tqspi->is_packed) {
>  		ret = tegra_qspi_dma_map_xfer(tqspi, t);
> @@ -634,60 +641,85 @@ static int tegra_qspi_start_dma_based_transfer(struct tegra_qspi *tqspi, struct
>  		len = tqspi->curr_dma_words * 4;
>  
>  	/* set attention level based on length of transfer */
> -	val = 0;
> -	if (len & 0xf) {
> -		val |= QSPI_TX_TRIG_1 | QSPI_RX_TRIG_1;
> -		dma_burst = 1;
> -	} else if (((len) >> 4) & 0x1) {
> -		val |= QSPI_TX_TRIG_4 | QSPI_RX_TRIG_4;
> -		dma_burst = 4;
> -	} else {
> -		val |= QSPI_TX_TRIG_8 | QSPI_RX_TRIG_8;
> -		dma_burst = 8;
> +	if (has_ext_dma) {
> +		val = 0;
> +		if (len & 0xf) {
> +			val |= QSPI_TX_TRIG_1 | QSPI_RX_TRIG_1;
> +			dma_burst = 1;
> +		} else if (((len) >> 4) & 0x1) {
> +			val |= QSPI_TX_TRIG_4 | QSPI_RX_TRIG_4;
> +			dma_burst = 4;
> +		} else {
> +			val |= QSPI_TX_TRIG_8 | QSPI_RX_TRIG_8;
> +			dma_burst = 8;
> +		}
> +
> +		tegra_qspi_writel(tqspi, val, QSPI_DMA_CTL);
>  	}
>  
> -	tegra_qspi_writel(tqspi, val, QSPI_DMA_CTL);
>  	tqspi->dma_control_reg = val;
>  
>  	dma_sconfig.device_fc = true;
> -	if (tqspi->cur_direction & DATA_DIR_TX) {
> -		dma_sconfig.dst_addr = tqspi->phys + QSPI_TX_FIFO;
> -		dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> -		dma_sconfig.dst_maxburst = dma_burst;
> -		ret = dmaengine_slave_config(tqspi->tx_dma_chan, &dma_sconfig);
> -		if (ret < 0) {
> -			dev_err(tqspi->dev, "failed DMA slave config: %d\n", ret);
> -			return ret;
> -		}
>  
> -		tegra_qspi_copy_client_txbuf_to_qspi_txbuf(tqspi, t);
> -		ret = tegra_qspi_start_tx_dma(tqspi, t, len);
> -		if (ret < 0) {
> -			dev_err(tqspi->dev, "failed to starting TX DMA: %d\n", ret);
> -			return ret;
> +	if ((tqspi->cur_direction & DATA_DIR_TX)) {
> +		if (has_ext_dma) {
> +			dma_sconfig.dst_addr = tqspi->phys + QSPI_TX_FIFO;
> +			dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> +			dma_sconfig.dst_maxburst = dma_burst;
> +			ret = dmaengine_slave_config(tqspi->tx_dma_chan, &dma_sconfig);
> +			if (ret < 0) {
> +				dev_err(tqspi->dev, "failed DMA slave config: %d\n", ret);
> +				return ret;
> +			}
> +
> +			tegra_qspi_copy_client_txbuf_to_qspi_txbuf(tqspi, t);
> +			ret = tegra_qspi_start_tx_dma(tqspi, t, len);
> +			if (ret < 0) {
> +				dev_err(tqspi->dev, "failed to starting TX DMA: %d\n", ret);
> +				return ret;
> +			}
> +		} else {
> +			if (tqspi->is_packed)
> +				tx_dma_phys = t->tx_dma;
> +			else
> +				tx_dma_phys = tqspi->tx_dma_phys;
> +			tegra_qspi_copy_client_txbuf_to_qspi_txbuf(tqspi, t);
> +			tegra_qspi_writel(tqspi, lower_32_bits(tx_dma_phys),
> +					  QSPI_DMA_MEM_ADDRESS_REG);
> +			tegra_qspi_writel(tqspi, (upper_32_bits(tx_dma_phys) & 0xff),
> +					  QSPI_DMA_HI_ADDRESS_REG);
>  		}
>  	}
>  
>  	if (tqspi->cur_direction & DATA_DIR_RX) {
> -		dma_sconfig.src_addr = tqspi->phys + QSPI_RX_FIFO;
> -		dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> -		dma_sconfig.src_maxburst = dma_burst;
> -		ret = dmaengine_slave_config(tqspi->rx_dma_chan, &dma_sconfig);
> -		if (ret < 0) {
> -			dev_err(tqspi->dev, "failed DMA slave config: %d\n", ret);
> -			return ret;
> -		}
> -
> -		dma_sync_single_for_device(tqspi->dev, tqspi->rx_dma_phys,
> -					   tqspi->dma_buf_size,
> -					   DMA_FROM_DEVICE);
> -
> -		ret = tegra_qspi_start_rx_dma(tqspi, t, len);
> -		if (ret < 0) {
> -			dev_err(tqspi->dev, "failed to start RX DMA: %d\n", ret);
> -			if (tqspi->cur_direction & DATA_DIR_TX)
> -				dmaengine_terminate_all(tqspi->tx_dma_chan);
> -			return ret;
> +		if (has_ext_dma) {
> +			dma_sconfig.src_addr = tqspi->phys + QSPI_RX_FIFO;
> +			dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> +			dma_sconfig.src_maxburst = dma_burst;
> +			ret = dmaengine_slave_config(tqspi->rx_dma_chan, &dma_sconfig);
> +			if (ret < 0) {
> +				dev_err(tqspi->dev, "failed DMA slave config: %d\n", ret);
> +				return ret;
> +			}
> +			dma_sync_single_for_device(tqspi->dev, tqspi->rx_dma_phys,
> +						   tqspi->dma_buf_size, DMA_FROM_DEVICE);
> +			ret = tegra_qspi_start_rx_dma(tqspi, t, len);
> +			if (ret < 0) {
> +				dev_err(tqspi->dev, "failed to start RX DMA: %d\n", ret);
> +				if (tqspi->cur_direction & DATA_DIR_TX)
> +					dmaengine_terminate_all(tqspi->tx_dma_chan);
> +				return ret;
> +			}

Please keep the whitespace that was there before (maybe even add a few
blank lines) to make this less cluttered.

> +		} else {
> +			if (tqspi->is_packed)
> +				rx_dma_phys = t->rx_dma;
> +			else
> +				rx_dma_phys = tqspi->rx_dma_phys;
> +
> +			tegra_qspi_writel(tqspi, lower_32_bits(rx_dma_phys),
> +					  QSPI_DMA_MEM_ADDRESS_REG);
> +			tegra_qspi_writel(tqspi, (upper_32_bits(rx_dma_phys) & 0xff),
> +					  QSPI_DMA_HI_ADDRESS_REG);
>  		}

This doesn't look right. You're passing a memory buffer to hardware
here, so this needs DMA sync operations, too.

[...]
> @@ -1388,30 +1420,32 @@ static irqreturn_t handle_dma_based_xfer(struct tegra_qspi *tqspi)
>  
>  	if (tqspi->cur_direction & DATA_DIR_TX) {
>  		if (tqspi->tx_status) {
> -			dmaengine_terminate_all(tqspi->tx_dma_chan);
> -			err += 1;
> -		} else {
> +			if (tqspi->tx_dma_chan)
> +				dmaengine_terminate_all(tqspi->tx_dma_chan);
> +			err++;
> +		} else if (tqspi->tx_dma_chan) {
>  			wait_status = wait_for_completion_interruptible_timeout(
>  				&tqspi->tx_dma_complete, QSPI_DMA_TIMEOUT);
>  			if (wait_status <= 0) {
>  				dmaengine_terminate_all(tqspi->tx_dma_chan);
>  				dev_err(tqspi->dev, "failed TX DMA transfer\n");
> -				err += 1;
> +				err++;
>  			}
>  		}
>  	}
>  
>  	if (tqspi->cur_direction & DATA_DIR_RX) {
>  		if (tqspi->rx_status) {
> -			dmaengine_terminate_all(tqspi->rx_dma_chan);
> -			err += 2;
> -		} else {
> +			if (tqspi->rx_dma_chan)
> +				dmaengine_terminate_all(tqspi->rx_dma_chan);
> +			err++;
> +		} else if (tqspi->rx_dma_chan) {
>  			wait_status = wait_for_completion_interruptible_timeout(
>  				&tqspi->rx_dma_complete, QSPI_DMA_TIMEOUT);
>  			if (wait_status <= 0) {
>  				dmaengine_terminate_all(tqspi->rx_dma_chan);
>  				dev_err(tqspi->dev, "failed RX DMA transfer\n");
> -				err += 2;
> +				err++;

Maybe we should change the "err" variable to something like "errors" or
"num_errors" to make it clear what this does. As it is, it's easily
mistaken to be a negative error code, in which case ++ wouldn't make
sense.

Thierry
Jon Hunter Feb. 27, 2025, 11:17 a.m. UTC | #11
On 12/02/2025 14:46, Vishwaroop A wrote:
> Previous generations of Tegra supported DMA operations by an external
> DMA controller, but the QSPI on Tegra234 devices now have an internal
> DMA controller.
> 
> Internal DMA: Uses the QSPI controller's built-in DMA engine, which is
> limited in capabilities and tied directly to the QSPI module.
> 
> External DMA: Utilizes a separate, GPCDMA DMA controller that can
> transfer data between QSPI and any memory location.
> 
> Native DMA Initialization: Introduce routines to initialize and
> configure native DMA channels for both transmit and receive paths.
> Set up DMA mapping functions to manage buffer addresses effectively.
> 
> Enhance Transfer Logic: Implement logic to choose between CPU-based
> and DMA-based transfers based on data size.
> 
> Signed-off-by: Vishwaroop A <va@nvidia.com>
> ---
>   drivers/spi/spi-tegra210-quad.c | 218 ++++++++++++++++++--------------
>   1 file changed, 126 insertions(+), 92 deletions(-)
> 
> diff --git a/drivers/spi/spi-tegra210-quad.c b/drivers/spi/spi-tegra210-quad.c
> index 04f41e92c1e2..7463b00b1ffb 100644
> --- a/drivers/spi/spi-tegra210-quad.c
> +++ b/drivers/spi/spi-tegra210-quad.c
> @@ -111,6 +111,9 @@
>   #define QSPI_DMA_BLK				0x024
>   #define QSPI_DMA_BLK_SET(x)			(((x) & 0xffff) << 0)
>   
> +#define QSPI_DMA_MEM_ADDRESS_REG		0x028
> +#define QSPI_DMA_HI_ADDRESS_REG			0x02c
> +
>   #define QSPI_TX_FIFO				0x108
>   #define QSPI_RX_FIFO				0x188
>   
> @@ -167,9 +170,9 @@ enum tegra_qspi_transfer_type {
>   };
>   
>   struct tegra_qspi_soc_data {
> -	bool has_dma;
>   	bool cmb_xfer_capable;
>   	bool supports_tpm;
> +	bool has_ext_dma;
>   	unsigned int cs_count;
>   };
>   
> @@ -605,17 +608,21 @@ static void tegra_qspi_dma_unmap_xfer(struct tegra_qspi *tqspi, struct spi_trans
>   
>   	len = DIV_ROUND_UP(tqspi->curr_dma_words * tqspi->bytes_per_word, 4) * 4;
>   
> -	dma_unmap_single(tqspi->dev, t->tx_dma, len, DMA_TO_DEVICE);
> -	dma_unmap_single(tqspi->dev, t->rx_dma, len, DMA_FROM_DEVICE);
> +	if (t->tx_buf)
> +		dma_unmap_single(tqspi->dev, t->tx_dma, len, DMA_TO_DEVICE);
> +	if (t->rx_buf)
> +		dma_unmap_single(tqspi->dev, t->rx_dma, len, DMA_FROM_DEVICE);
>   }
>   
>   static int tegra_qspi_start_dma_based_transfer(struct tegra_qspi *tqspi, struct spi_transfer *t)
>   {
>   	struct dma_slave_config dma_sconfig = { 0 };
> +	dma_addr_t rx_dma_phys, tx_dma_phys;
>   	unsigned int len;
>   	u8 dma_burst;
>   	int ret = 0;
>   	u32 val;
> +	bool has_ext_dma = tqspi->soc_data->has_ext_dma;
>   
>   	if (tqspi->is_packed) {
>   		ret = tegra_qspi_dma_map_xfer(tqspi, t);
> @@ -634,60 +641,85 @@ static int tegra_qspi_start_dma_based_transfer(struct tegra_qspi *tqspi, struct
>   		len = tqspi->curr_dma_words * 4;
>   
>   	/* set attention level based on length of transfer */
> -	val = 0;
> -	if (len & 0xf) {
> -		val |= QSPI_TX_TRIG_1 | QSPI_RX_TRIG_1;
> -		dma_burst = 1;
> -	} else if (((len) >> 4) & 0x1) {
> -		val |= QSPI_TX_TRIG_4 | QSPI_RX_TRIG_4;
> -		dma_burst = 4;
> -	} else {
> -		val |= QSPI_TX_TRIG_8 | QSPI_RX_TRIG_8;
> -		dma_burst = 8;
> +	if (has_ext_dma) {
> +		val = 0;
> +		if (len & 0xf) {
> +			val |= QSPI_TX_TRIG_1 | QSPI_RX_TRIG_1;
> +			dma_burst = 1;
> +		} else if (((len) >> 4) & 0x1) {
> +			val |= QSPI_TX_TRIG_4 | QSPI_RX_TRIG_4;
> +			dma_burst = 4;
> +		} else {
> +			val |= QSPI_TX_TRIG_8 | QSPI_RX_TRIG_8;
> +			dma_burst = 8;
> +		}
> +
> +		tegra_qspi_writel(tqspi, val, QSPI_DMA_CTL);
>   	}
>   
> -	tegra_qspi_writel(tqspi, val, QSPI_DMA_CTL);
>   	tqspi->dma_control_reg = val;
>   
>   	dma_sconfig.device_fc = true;
> -	if (tqspi->cur_direction & DATA_DIR_TX) {
> -		dma_sconfig.dst_addr = tqspi->phys + QSPI_TX_FIFO;
> -		dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> -		dma_sconfig.dst_maxburst = dma_burst;
> -		ret = dmaengine_slave_config(tqspi->tx_dma_chan, &dma_sconfig);
> -		if (ret < 0) {
> -			dev_err(tqspi->dev, "failed DMA slave config: %d\n", ret);
> -			return ret;
> -		}
>   
> -		tegra_qspi_copy_client_txbuf_to_qspi_txbuf(tqspi, t);
> -		ret = tegra_qspi_start_tx_dma(tqspi, t, len);
> -		if (ret < 0) {
> -			dev_err(tqspi->dev, "failed to starting TX DMA: %d\n", ret);
> -			return ret;
> +	if ((tqspi->cur_direction & DATA_DIR_TX)) {
> +		if (has_ext_dma) {

For consistency with 'handle_dma_based_xfer' I think it is better to 
use 'tqspi->tx_dma_chan' here.

> +			dma_sconfig.dst_addr = tqspi->phys + QSPI_TX_FIFO;
> +			dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> +			dma_sconfig.dst_maxburst = dma_burst;
> +			ret = dmaengine_slave_config(tqspi->tx_dma_chan, &dma_sconfig);
> +			if (ret < 0) {
> +				dev_err(tqspi->dev, "failed DMA slave config: %d\n", ret);
> +				return ret;
> +			}
> +
> +			tegra_qspi_copy_client_txbuf_to_qspi_txbuf(tqspi, t);
> +			ret = tegra_qspi_start_tx_dma(tqspi, t, len);
> +			if (ret < 0) {
> +				dev_err(tqspi->dev, "failed to starting TX DMA: %d\n", ret);
> +				return ret;
> +			}
> +		} else {
> +			if (tqspi->is_packed)
> +				tx_dma_phys = t->tx_dma;
> +			else
> +				tx_dma_phys = tqspi->tx_dma_phys;
> +			tegra_qspi_copy_client_txbuf_to_qspi_txbuf(tqspi, t);
> +			tegra_qspi_writel(tqspi, lower_32_bits(tx_dma_phys),
> +					  QSPI_DMA_MEM_ADDRESS_REG);
> +			tegra_qspi_writel(tqspi, (upper_32_bits(tx_dma_phys) & 0xff),
> +					  QSPI_DMA_HI_ADDRESS_REG);
>   		}
>   	}
>   
>   	if (tqspi->cur_direction & DATA_DIR_RX) {
> -		dma_sconfig.src_addr = tqspi->phys + QSPI_RX_FIFO;
> -		dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> -		dma_sconfig.src_maxburst = dma_burst;
> -		ret = dmaengine_slave_config(tqspi->rx_dma_chan, &dma_sconfig);
> -		if (ret < 0) {
> -			dev_err(tqspi->dev, "failed DMA slave config: %d\n", ret);
> -			return ret;
> -		}
> -
> -		dma_sync_single_for_device(tqspi->dev, tqspi->rx_dma_phys,
> -					   tqspi->dma_buf_size,
> -					   DMA_FROM_DEVICE);
> -
> -		ret = tegra_qspi_start_rx_dma(tqspi, t, len);
> -		if (ret < 0) {
> -			dev_err(tqspi->dev, "failed to start RX DMA: %d\n", ret);
> -			if (tqspi->cur_direction & DATA_DIR_TX)
> -				dmaengine_terminate_all(tqspi->tx_dma_chan);
> -			return ret;
> +		if (has_ext_dma) {

Same here.

> +			dma_sconfig.src_addr = tqspi->phys + QSPI_RX_FIFO;
> +			dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> +			dma_sconfig.src_maxburst = dma_burst;
> +			ret = dmaengine_slave_config(tqspi->rx_dma_chan, &dma_sconfig);
> +			if (ret < 0) {
> +				dev_err(tqspi->dev, "failed DMA slave config: %d\n", ret);
> +				return ret;
> +			}
> +			dma_sync_single_for_device(tqspi->dev, tqspi->rx_dma_phys,
> +						   tqspi->dma_buf_size, DMA_FROM_DEVICE);
> +			ret = tegra_qspi_start_rx_dma(tqspi, t, len);
> +			if (ret < 0) {
> +				dev_err(tqspi->dev, "failed to start RX DMA: %d\n", ret);
> +				if (tqspi->cur_direction & DATA_DIR_TX)
> +					dmaengine_terminate_all(tqspi->tx_dma_chan);
> +				return ret;
> +			}
> +		} else {
> +			if (tqspi->is_packed)
> +				rx_dma_phys = t->rx_dma;
> +			else
> +				rx_dma_phys = tqspi->rx_dma_phys;
> +
> +			tegra_qspi_writel(tqspi, lower_32_bits(rx_dma_phys),
> +					  QSPI_DMA_MEM_ADDRESS_REG);
> +			tegra_qspi_writel(tqspi, (upper_32_bits(rx_dma_phys) & 0xff),
> +					  QSPI_DMA_HI_ADDRESS_REG);
>   		}
>   	}
>   
> @@ -726,9 +758,6 @@ static int tegra_qspi_start_cpu_based_transfer(struct tegra_qspi *qspi, struct s
>   
>   static void tegra_qspi_deinit_dma(struct tegra_qspi *tqspi)
>   {
> -	if (!tqspi->soc_data->has_dma)
> -		return;
> -
>   	if (tqspi->tx_dma_buf) {
>   		dma_free_coherent(tqspi->dev, tqspi->dma_buf_size,
>   				  tqspi->tx_dma_buf, tqspi->tx_dma_phys);
> @@ -759,16 +788,26 @@ static int tegra_qspi_init_dma(struct tegra_qspi *tqspi)
>   	u32 *dma_buf;
>   	int err;
>   
> -	if (!tqspi->soc_data->has_dma)
> -		return 0;
> +	if (tqspi->soc_data->has_ext_dma) {
> +		dma_chan = dma_request_chan(tqspi->dev, "rx");
> +		if (IS_ERR(dma_chan)) {
> +			err = PTR_ERR(dma_chan);
> +			goto err_out;
> +		}
>   
> -	dma_chan = dma_request_chan(tqspi->dev, "rx");
> -	if (IS_ERR(dma_chan)) {
> -		err = PTR_ERR(dma_chan);
> -		goto err_out;
> -	}
> +		tqspi->rx_dma_chan = dma_chan;
>   
> -	tqspi->rx_dma_chan = dma_chan;
> +		dma_chan = dma_request_chan(tqspi->dev, "tx");
> +		if (IS_ERR(dma_chan)) {
> +			err = PTR_ERR(dma_chan);
> +			goto err_out;
> +		}
> +
> +		tqspi->tx_dma_chan = dma_chan;
> +	} else {
> +		tqspi->rx_dma_chan = NULL;
> +		tqspi->tx_dma_chan = NULL;
> +	}
>   
>   	dma_buf = dma_alloc_coherent(tqspi->dev, tqspi->dma_buf_size, &dma_phys, GFP_KERNEL);
>   	if (!dma_buf) {
> @@ -779,14 +818,6 @@ static int tegra_qspi_init_dma(struct tegra_qspi *tqspi)
>   	tqspi->rx_dma_buf = dma_buf;
>   	tqspi->rx_dma_phys = dma_phys;
>   
> -	dma_chan = dma_request_chan(tqspi->dev, "tx");
> -	if (IS_ERR(dma_chan)) {
> -		err = PTR_ERR(dma_chan);
> -		goto err_out;
> -	}
> -
> -	tqspi->tx_dma_chan = dma_chan;
> -
>   	dma_buf = dma_alloc_coherent(tqspi->dev, tqspi->dma_buf_size, &dma_phys, GFP_KERNEL);
>   	if (!dma_buf) {
>   		err = -ENOMEM;
> @@ -1056,6 +1087,7 @@ static int tegra_qspi_combined_seq_xfer(struct tegra_qspi *tqspi,
>   					struct spi_message *msg)
>   {
>   	bool is_first_msg = true;
> +	bool has_ext_dma = tqspi->soc_data->has_ext_dma;
>   	struct spi_transfer *xfer;
>   	struct spi_device *spi = msg->spi;
>   	u8 transfer_phase = 0;
> @@ -1128,15 +1160,12 @@ static int tegra_qspi_combined_seq_xfer(struct tegra_qspi *tqspi,
>   			if (WARN_ON(ret == 0)) {
>   				dev_err(tqspi->dev, "QSPI Transfer failed with timeout: %d\n",
>   					ret);
> -				if (tqspi->is_curr_dma_xfer &&
> -				    (tqspi->cur_direction & DATA_DIR_TX))
> -					dmaengine_terminate_all
> -						(tqspi->tx_dma_chan);
> -
> -				if (tqspi->is_curr_dma_xfer &&
> -				    (tqspi->cur_direction & DATA_DIR_RX))
> -					dmaengine_terminate_all
> -						(tqspi->rx_dma_chan);
> +				if (tqspi->is_curr_dma_xfer && has_ext_dma) {
> +					if (tqspi->cur_direction & DATA_DIR_TX)
> +						dmaengine_terminate_all(tqspi->tx_dma_chan);
> +					if (tqspi->cur_direction & DATA_DIR_RX)
> +						dmaengine_terminate_all(tqspi->rx_dma_chan);
> +				}
>   
>   				/* Abort transfer by resetting pio/dma bit */
>   				if (!tqspi->is_curr_dma_xfer) {
> @@ -1197,6 +1226,7 @@ static int tegra_qspi_non_combined_seq_xfer(struct tegra_qspi *tqspi,
>   	struct spi_device *spi = msg->spi;
>   	struct spi_transfer *transfer;
>   	bool is_first_msg = true;
> +	bool has_ext_dma = tqspi->soc_data->has_ext_dma;
>   	int ret = 0, val = 0;
>   
>   	msg->status = 0;
> @@ -1251,10 +1281,12 @@ static int tegra_qspi_non_combined_seq_xfer(struct tegra_qspi *tqspi,
>   						  QSPI_DMA_TIMEOUT);
>   		if (WARN_ON(ret == 0)) {
>   			dev_err(tqspi->dev, "transfer timeout\n");
> -			if (tqspi->is_curr_dma_xfer && (tqspi->cur_direction & DATA_DIR_TX))
> -				dmaengine_terminate_all(tqspi->tx_dma_chan);
> -			if (tqspi->is_curr_dma_xfer && (tqspi->cur_direction & DATA_DIR_RX))
> -				dmaengine_terminate_all(tqspi->rx_dma_chan);
> +			if (tqspi->is_curr_dma_xfer && has_ext_dma) {

And here.

> +				if (tqspi->cur_direction & DATA_DIR_TX)
> +					dmaengine_terminate_all(tqspi->tx_dma_chan);
> +				if (tqspi->cur_direction & DATA_DIR_RX)
> +					dmaengine_terminate_all(tqspi->rx_dma_chan);
> +			}
>   			tegra_qspi_handle_error(tqspi);
>   			ret = -EIO;
>   			goto complete_xfer;
> @@ -1323,7 +1355,7 @@ static bool tegra_qspi_validate_cmb_seq(struct tegra_qspi *tqspi,
>   			return false;
>   		xfer = list_next_entry(xfer, transfer_list);
>   	}
> -	if (!tqspi->soc_data->has_dma && xfer->len > (QSPI_FIFO_DEPTH << 2))
> +	if (!tqspi->soc_data->has_ext_dma && xfer->len > (QSPI_FIFO_DEPTH << 2))
>   		return false;
>   
>   	return true;
> @@ -1388,30 +1420,32 @@ static irqreturn_t handle_dma_based_xfer(struct tegra_qspi *tqspi)
>   
>   	if (tqspi->cur_direction & DATA_DIR_TX) {
>   		if (tqspi->tx_status) {
> -			dmaengine_terminate_all(tqspi->tx_dma_chan);
> -			err += 1;
> -		} else {
> +			if (tqspi->tx_dma_chan)
> +				dmaengine_terminate_all(tqspi->tx_dma_chan);
> +			err++;
> +		} else if (tqspi->tx_dma_chan) {
>   			wait_status = wait_for_completion_interruptible_timeout(
>   				&tqspi->tx_dma_complete, QSPI_DMA_TIMEOUT);
>   			if (wait_status <= 0) {
>   				dmaengine_terminate_all(tqspi->tx_dma_chan);
>   				dev_err(tqspi->dev, "failed TX DMA transfer\n");
> -				err += 1;
> +				err++;
>   			}
>   		}
>   	}
>   
>   	if (tqspi->cur_direction & DATA_DIR_RX) {
>   		if (tqspi->rx_status) {
> -			dmaengine_terminate_all(tqspi->rx_dma_chan);
> -			err += 2;
> -		} else {
> +			if (tqspi->rx_dma_chan)
> +				dmaengine_terminate_all(tqspi->rx_dma_chan);
> +			err++;
> +		} else if (tqspi->rx_dma_chan) {
>   			wait_status = wait_for_completion_interruptible_timeout(
>   				&tqspi->rx_dma_complete, QSPI_DMA_TIMEOUT);
>   			if (wait_status <= 0) {
>   				dmaengine_terminate_all(tqspi->rx_dma_chan);
>   				dev_err(tqspi->dev, "failed RX DMA transfer\n");
> -				err += 2;
> +				err++;
>   			}
>   		}
>   	}
> @@ -1474,28 +1508,28 @@ static irqreturn_t tegra_qspi_isr_thread(int irq, void *context_data)
>   }
>   
>   static struct tegra_qspi_soc_data tegra210_qspi_soc_data = {
> -	.has_dma = true,
> +	.has_ext_dma = true,
>   	.cmb_xfer_capable = false,
>   	.supports_tpm = false,
>   	.cs_count = 1,
>   };
>   
>   static struct tegra_qspi_soc_data tegra186_qspi_soc_data = {
> -	.has_dma = true,
> +	.has_ext_dma = true,
>   	.cmb_xfer_capable = true,
>   	.supports_tpm = false,
>   	.cs_count = 1,
>   };
>   
>   static struct tegra_qspi_soc_data tegra234_qspi_soc_data = {
> -	.has_dma = false,
> +	.has_ext_dma = false,
>   	.cmb_xfer_capable = true,
>   	.supports_tpm = true,
>   	.cs_count = 1,
>   };
>   
>   static struct tegra_qspi_soc_data tegra241_qspi_soc_data = {
> -	.has_dma = false,
> +	.has_ext_dma = true,
>   	.cmb_xfer_capable = true,
>   	.supports_tpm = true,
>   	.cs_count = 4,
Jon Hunter March 7, 2025, 1:28 p.m. UTC | #12
On 12/02/2025 14:39, Vishwaroop A wrote:
> 
> 
> ________________________________________
> From: Thierry Reding
> Sent: Thursday, January 09, 2025 16:10
> To: Vishwaroop A
> Cc: Jon Hunter; Sowjanya Komatineni; Laxman Dewangan; broonie@kernel.org; linux-spi@vger.kernel.org; linux-tegra@vger.kernel.org; linux-kernel@vger.kernel.org; Krishna Yarlagadda; Suresh Mangipudi
> Subject: Re: [PATCH V1 1/6] arm64: tegra: Configure QSPI clocks and add DMA
> 
> On Fri, Jan 03, 2025 at 06:04:02AM +0000, Vishwaroop A wrote:
>> Set QSPI0_2X_PM to 199.99 MHz and QSPI0_PM to 99.99 MHz using
>> PLLC as the parent clock. These frequencies allow Quad IO DT
>> reads up to 99.99 MHz, which is the fastest that can be
>> achieved considering various PLL and clock divider constraints.
>>
>> Populate the DMA and IOMMU properties for the Tegra234 QSPI devices to
>> enable DMA support.
>>
>> Change-Id: I1dded904aa8e0f278c89998481e829f1ce474e8c
>> Signed-off-by: Vishwaroop A <va@nvidia.com>
>> ---
>>   arch/arm64/boot/dts/nvidia/tegra234.dtsi | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/nvidia/tegra234.dtsi b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
>> index 984c85eab41a..96d0f13390ae 100644
>> --- a/arch/arm64/boot/dts/nvidia/tegra234.dtsi
>> +++ b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
>> @@ -2948,6 +2948,13 @@
>>                                 <&bpmp TEGRA234_CLK_QSPI0_PM>;
>>                        clock-names = "qspi", "qspi_out";
>>                        resets = <&bpmp TEGRA234_RESET_QSPI0>;
>> +                     assigned-clocks = <&bpmp TEGRA234_CLK_QSPI0_2X_PM>,
>> +                                       <&bpmp TEGRA234_CLK_QSPI0_PM>;
>> +                     assigned-clock-rates = <199999999 99999999>;
>> +                     assigned-clock-parents = <&bpmp TEGRA234_CLK_PLLC>;
>> +                     dma-names = "rx", "tx";
>> +                     dma-coherent;
>> +                     iommus = <&smmu_niso1 TEGRA234_SID_QSPI0>;
>>                        status = "disabled";
>>                };
>>
>> @@ -3031,6 +3038,13 @@
>>                                 <&bpmp TEGRA234_CLK_QSPI1_PM>;
>>                        clock-names = "qspi", "qspi_out";
>>                        resets = <&bpmp TEGRA234_RESET_QSPI1>;
>> +                     assigned-clocks = <&bpmp TEGRA234_CLK_QSPI1_2X_PM>,
>> +                                       <&bpmp TEGRA234_CLK_QSPI1_PM>;
>> +                     assigned-clock-rates = <199999999 99999999>;
>> +                     assigned-clock-parents = <&bpmp TEGRA234_CLK_PLLC>;
>> +                     dma-names = "rx", "tx";
>> +                     dma-coherent;
>> +                     iommus = <&smmu_niso1 TEGRA234_SID_QSPI1>;
>>                        status = "disabled";
>>                };
>>
> 
>>> It looks like these are missing the "dmas" properties that go along with "dma-names".
> [Vishwaroop A] dmas property is not required as QSPI uses native dma engine. dmas property is used for assigning  the dma channels. In case of QSPI it has own native DMA engine.


OK so that means we don't need the 'dma-names' here either. Please 
remove this.

Jon
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/nvidia/tegra234.dtsi b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
index 984c85eab41a..96d0f13390ae 100644
--- a/arch/arm64/boot/dts/nvidia/tegra234.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
@@ -2948,6 +2948,13 @@ 
 				 <&bpmp TEGRA234_CLK_QSPI0_PM>;
 			clock-names = "qspi", "qspi_out";
 			resets = <&bpmp TEGRA234_RESET_QSPI0>;
+			assigned-clocks = <&bpmp TEGRA234_CLK_QSPI0_2X_PM>,
+					  <&bpmp TEGRA234_CLK_QSPI0_PM>;
+			assigned-clock-rates = <199999999 99999999>;
+			assigned-clock-parents = <&bpmp TEGRA234_CLK_PLLC>;
+			dma-names = "rx", "tx";
+			dma-coherent;
+			iommus = <&smmu_niso1 TEGRA234_SID_QSPI0>;
 			status = "disabled";
 		};
 
@@ -3031,6 +3038,13 @@ 
 				 <&bpmp TEGRA234_CLK_QSPI1_PM>;
 			clock-names = "qspi", "qspi_out";
 			resets = <&bpmp TEGRA234_RESET_QSPI1>;
+			assigned-clocks = <&bpmp TEGRA234_CLK_QSPI1_2X_PM>,
+					  <&bpmp TEGRA234_CLK_QSPI1_PM>;
+			assigned-clock-rates = <199999999 99999999>;
+			assigned-clock-parents = <&bpmp TEGRA234_CLK_PLLC>;
+			dma-names = "rx", "tx";
+			dma-coherent;
+			iommus = <&smmu_niso1 TEGRA234_SID_QSPI1>;
 			status = "disabled";
 		};