[PATCH v2 0/6] firmware: tegra: Add MRQ support for Tegra264.

[PATCH v2 0/6] firmware: tegra: Add MRQ support for Tegra264.

[Peter De Schrijver]

In Tegra264 the carveouts (GSCs) used to communicate between BPMP and
CPU-NS may reside in DRAM. The location will be signalled using reserved
memory node in DT. Additionally some minor updates to the HSP driver are
done to support the new chip.

Peter De Schrijver (4):
  dt-bindings: mailbox: tegra: Document Tegra264 HSP
  dt-bindings: Add bindings to support DRAM MRQ GSCs
  dt-bindings: memory-region property for tegra186-bpmp
  firmware: tegra: bpmp: Add support for DRAM MRQ GSCs

Stefan Kristiansson (2):
  mailbox: tegra: add support for Tegra264
  soc: tegra: fuse: add support for Tegra264

 .../firmware/nvidia,tegra186-bpmp.yaml        |  37 ++-
 .../bindings/mailbox/nvidia,tegra186-hsp.yaml |   1 +
 .../nvidia,tegra264-bpmp-shmem.yaml           |  45 ++++
 drivers/firmware/tegra/bpmp-tegra186.c        | 214 ++++++++++++------
 drivers/firmware/tegra/bpmp.c                 |   4 +-
 drivers/mailbox/tegra-hsp.c                   |  16 +-
 drivers/soc/tegra/fuse/tegra-apbmisc.c        |   3 +-
 include/soc/tegra/fuse.h                      |   3 +-
 8 files changed, 251 insertions(+), 72 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.yaml

-- 
2.34.1

[PATCH v2 1/6] dt-bindings: mailbox: tegra: Document Tegra264 HSP

[Peter De Schrijver]

Add the compatible string for the HSP block found on the Tegra264 SoC.
The HSP block in Tegra264 is not register compatible with the one in
Tegra194 or Tegra234 hence there is no fallback compatibility string.

Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
---
 .../devicetree/bindings/mailbox/nvidia,tegra186-hsp.yaml         | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.yaml b/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.yaml
index a3e87516d637..2d14fc948999 100644
--- a/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.yaml
+++ b/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.yaml
@@ -66,6 +66,7 @@ properties:
     oneOf:
       - const: nvidia,tegra186-hsp
       - const: nvidia,tegra194-hsp
+      - const: nvidia,tegra264-hsp
       - items:
           - const: nvidia,tegra234-hsp
           - const: nvidia,tegra194-hsp
-- 
2.34.1

Re: [PATCH v2 1/6] dt-bindings: mailbox: tegra: Document Tegra264 HSP

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 521 bytes --]

On Wed, May 10, 2023 at 02:31:26PM +0300, Peter De Schrijver wrote:
> Add the compatible string for the HSP block found on the Tegra264 SoC.
> The HSP block in Tegra264 is not register compatible with the one in
> Tegra194 or Tegra234 hence there is no fallback compatibility string.
> 
> Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
> ---
>  .../devicetree/bindings/mailbox/nvidia,tegra186-hsp.yaml         | 1 +
>  1 file changed, 1 insertion(+)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 2/6] mailbox: tegra: add support for Tegra264

[Peter De Schrijver]

From: Stefan Kristiansson <stefank@nvidia.com>

Tegra264 has a slightly different doorbell register layout than
previous chips.

Signed-off-by: Stefan Kristiansson <stefank@nvidia.com>
Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
---
 drivers/mailbox/tegra-hsp.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/mailbox/tegra-hsp.c b/drivers/mailbox/tegra-hsp.c
index 573481e436f5..7f98e7436d94 100644
--- a/drivers/mailbox/tegra-hsp.c
+++ b/drivers/mailbox/tegra-hsp.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * Copyright (c) 2016-2018, NVIDIA CORPORATION.  All rights reserved.
+ * Copyright (c) 2016-2023, NVIDIA CORPORATION.  All rights reserved.
  */
 
 #include <linux/delay.h>
@@ -97,6 +97,7 @@ struct tegra_hsp_soc {
 	const struct tegra_hsp_db_map *map;
 	bool has_per_mb_ie;
 	bool has_128_bit_mb;
+	unsigned int reg_stride;
 };
 
 struct tegra_hsp {
@@ -279,7 +280,7 @@ tegra_hsp_doorbell_create(struct tegra_hsp *hsp, const char *name,
 		return ERR_PTR(-ENOMEM);
 
 	offset = (1 + (hsp->num_sm / 2) + hsp->num_ss + hsp->num_as) * SZ_64K;
-	offset += index * 0x100;
+	offset += index * hsp->soc->reg_stride;
 
 	db->channel.regs = hsp->regs + offset;
 	db->channel.hsp = hsp;
@@ -916,24 +917,35 @@ static const struct tegra_hsp_soc tegra186_hsp_soc = {
 	.map = tegra186_hsp_db_map,
 	.has_per_mb_ie = false,
 	.has_128_bit_mb = false,
+	.reg_stride = 0x100,
 };
 
 static const struct tegra_hsp_soc tegra194_hsp_soc = {
 	.map = tegra186_hsp_db_map,
 	.has_per_mb_ie = true,
 	.has_128_bit_mb = false,
+	.reg_stride = 0x100,
 };
 
 static const struct tegra_hsp_soc tegra234_hsp_soc = {
 	.map = tegra186_hsp_db_map,
 	.has_per_mb_ie = false,
 	.has_128_bit_mb = true,
+	.reg_stride = 0x100,
+};
+
+static const struct tegra_hsp_soc tegra264_hsp_soc = {
+	.map = tegra186_hsp_db_map,
+	.has_per_mb_ie = false,
+	.has_128_bit_mb = true,
+	.reg_stride = 0x1000,
 };
 
 static const struct of_device_id tegra_hsp_match[] = {
 	{ .compatible = "nvidia,tegra186-hsp", .data = &tegra186_hsp_soc },
 	{ .compatible = "nvidia,tegra194-hsp", .data = &tegra194_hsp_soc },
 	{ .compatible = "nvidia,tegra234-hsp", .data = &tegra234_hsp_soc },
+	{ .compatible = "nvidia,tegra264-hsp", .data = &tegra264_hsp_soc },
 	{ }
 };
 
-- 
2.34.1

Re: [PATCH v2 2/6] mailbox: tegra: add support for Tegra264

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 498 bytes --]

On Wed, May 10, 2023 at 02:31:28PM +0300, Peter De Schrijver wrote:
> From: Stefan Kristiansson <stefank@nvidia.com>
> 
> Tegra264 has a slightly different doorbell register layout than
> previous chips.
> 
> Signed-off-by: Stefan Kristiansson <stefank@nvidia.com>
> Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
> ---
>  drivers/mailbox/tegra-hsp.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 3/6] soc: tegra: fuse: add support for Tegra264

[Peter De Schrijver]

From: Stefan Kristiansson <stefank@nvidia.com>

Add support for Tegra264 to the fuse handling code.

Signed-off-by: Stefan Kristiansson <stefank@nvidia.com>
Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
---
 drivers/soc/tegra/fuse/tegra-apbmisc.c | 3 ++-
 include/soc/tegra/fuse.h               | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/tegra/fuse/tegra-apbmisc.c b/drivers/soc/tegra/fuse/tegra-apbmisc.c
index 4591c5bcb690..eb0a1d924526 100644
--- a/drivers/soc/tegra/fuse/tegra-apbmisc.c
+++ b/drivers/soc/tegra/fuse/tegra-apbmisc.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * Copyright (c) 2014, NVIDIA CORPORATION.  All rights reserved.
+ * Copyright (c) 2014-2023, NVIDIA CORPORATION.  All rights reserved.
  */
 
 #include <linux/export.h>
@@ -62,6 +62,7 @@ bool tegra_is_silicon(void)
 	switch (tegra_get_chip_id()) {
 	case TEGRA194:
 	case TEGRA234:
+	case TEGRA264:
 		if (tegra_get_platform() == 0)
 			return true;
 
diff --git a/include/soc/tegra/fuse.h b/include/soc/tegra/fuse.h
index a63de5da8124..3a513be50243 100644
--- a/include/soc/tegra/fuse.h
+++ b/include/soc/tegra/fuse.h
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 /*
- * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
+ * Copyright (c) 2012-2023, NVIDIA CORPORATION.  All rights reserved.
  */
 
 #ifndef __SOC_TEGRA_FUSE_H__
@@ -17,6 +17,7 @@
 #define TEGRA186	0x18
 #define TEGRA194	0x19
 #define TEGRA234	0x23
+#define TEGRA264	0x26
 
 #define TEGRA_FUSE_SKU_CALIB_0	0xf0
 #define TEGRA30_FUSE_SATA_CALIB	0x124
-- 
2.34.1

Re: [PATCH v2 3/6] soc: tegra: fuse: add support for Tegra264

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 601 bytes --]

On Wed, May 10, 2023 at 02:31:30PM +0300, Peter De Schrijver wrote:
> From: Stefan Kristiansson <stefank@nvidia.com>
> 
> Add support for Tegra264 to the fuse handling code.
> 
> Signed-off-by: Stefan Kristiansson <stefank@nvidia.com>
> Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
> ---
>  drivers/soc/tegra/fuse/tegra-apbmisc.c | 3 ++-
>  include/soc/tegra/fuse.h               | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)

The preferred subject prefix is "soc/tegra:", but I can fix that up when
applying. Also, capitalize after the last ":".

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 4/6] dt-bindings: Add bindings to support DRAM MRQ GSCs

[Peter De Schrijver]

Add bindings for DRAM MRQ GSC support.

Co-developed-by: Stefan Kristiansson <stefank@nvidia.com>
Signed-off-by: Stefan Kristiansson <stefank@nvidia.com>
Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
---
 .../nvidia,tegra264-bpmp-shmem.yaml           | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.yaml

diff --git a/Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.yaml b/Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.yaml
new file mode 100644
index 000000000000..1f9c2dfbf8c5
--- /dev/null
+++ b/Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reserved-memory/nvidia,tegra264-bpmp-shmem.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Tegra CPU-NS - BPMP IPC reserved memory
+
+maintainers:
+  - Peter De Schrijver <pdeschrijver@nvidia.com>
+
+description: |
+  Define a memory region used for communication between CPU-NS and BPMP.
+  Typically this node is created by the bootloader as the physical address
+  has to be known to both CPU-NS and BPMP for correct IPC operation.
+  The memory region is defined using a child node under /reserved-memory.
+  The sub-node is named shmem@<address>.
+
+allOf:
+  - $ref: reserved-memory.yaml
+
+properties:
+  compatible:
+    const: nvidia,tegra264-bpmp-shmem
+
+  reg:
+    description: The physical address and size of the shared SDRAM region
+
+unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+  - no-map
+
+examples:
+  - |
+    reserved-memory {
+       dram_cpu_bpmp_mail: shmem@f1be0000  {
+           compatible = "nvidia,tegra264-bpmp-shmem";
+           reg = <0x0 0xf1be0000 0x0 0x2000>;
+           no-map;
+       };
+    };
+...
-- 
2.34.1

Re: [PATCH v2 4/6] dt-bindings: Add bindings to support DRAM MRQ GSCs

[Krzysztof Kozlowski]

On Wed, 10 May 2023 14:31:32 +0300, Peter De Schrijver wrote:
> Add bindings for DRAM MRQ GSC support.
> 
> Co-developed-by: Stefan Kristiansson <stefank@nvidia.com>
> Signed-off-by: Stefan Kristiansson <stefank@nvidia.com>
> Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
> ---
>  .../nvidia,tegra264-bpmp-shmem.yaml           | 45 +++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.example.dts:21.16-50: Warning (reg_format): /example-0/reserved-memory/shmem@f1be0000:reg: property has invalid length (16 bytes) (#address-cells == 2, #size-cells == 1)
Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.example.dtb: Warning (pci_device_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.example.dtb: Warning (pci_device_bus_num): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.example.dtb: Warning (simple_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.example.dtb: Warning (i2c_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.example.dtb: Warning (spi_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.example.dts:19.47-23.14: Warning (avoid_default_addr_size): /example-0/reserved-memory/shmem@f1be0000: Relying on default #address-cells value
Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.example.dts:19.47-23.14: Warning (avoid_default_addr_size): /example-0/reserved-memory/shmem@f1be0000: Relying on default #size-cells value
Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.example.dtb: Warning (unique_unit_address_if_enabled): Failed prerequisite 'avoid_default_addr_size'

See https://patchwork.ozlabs.org/patch/1779405

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

[PATCH v2 5/6] dt-bindings: memory-region property for tegra186-bpmp

[Peter De Schrijver]

Add memory-region property to the tegra186-bpmp binding to support
DRAM MRQ GSCs.

Co-developed-by: Stefan Kristiansson <stefank@nvidia.com>
Signed-off-by: Stefan Kristiansson <stefank@nvidia.com>
Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
---
 .../firmware/nvidia,tegra186-bpmp.yaml        | 37 +++++++++++++++++--
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml
index 833c07f1685c..f3e02c9d090d 100644
--- a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml
+++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.yaml
@@ -57,8 +57,11 @@ description: |
   "#address-cells" or "#size-cells" property.
 
   The shared memory area for the IPC TX and RX between CPU and BPMP are
-  predefined and work on top of sysram, which is an SRAM inside the
-  chip. See ".../sram/sram.yaml" for the bindings.
+  predefined and work on top of either sysram, which is an SRAM inside the
+  chip, or in normal SDRAM.
+  See ".../sram/sram.yaml" for the bindings for the SRAM case.
+  See "../reserved-memory/nvidia,tegra264-bpmp-shmem.yaml" for bindings for
+  the SDRAM case.
 
 properties:
   compatible:
@@ -81,6 +84,11 @@ properties:
     minItems: 2
     maxItems: 2
 
+  memory-region:
+    description: phandle to reserved memory region used for IPC between
+      CPU-NS and BPMP.
+    maxItems: 1
+
   "#clock-cells":
     const: 1
 
@@ -115,10 +123,15 @@ properties:
 
 additionalProperties: false
 
+oneOf:
+  - required:
+      - memory-region
+  - required:
+      - shmem
+
 required:
   - compatible
   - mboxes
-  - shmem
   - "#clock-cells"
   - "#power-domain-cells"
   - "#reset-cells"
@@ -184,3 +197,21 @@ examples:
             #thermal-sensor-cells = <1>;
         };
     };
+
+  - |
+    #include <dt-bindings/mailbox/tegra186-hsp.h>
+
+    bpmp {
+        compatible = "nvidia,tegra186-bpmp";
+        interconnects = <&mc TEGRA186_MEMORY_CLIENT_BPMPR &emc>,
+                        <&mc TEGRA186_MEMORY_CLIENT_BPMPW &emc>,
+                        <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAR &emc>,
+                        <&mc TEGRA186_MEMORY_CLIENT_BPMPDMAW &emc>;
+        interconnect-names = "read", "write", "dma-mem", "dma-write";
+        mboxes = <&hsp_top1 TEGRA_HSP_MBOX_TYPE_DB
+                            TEGRA_HSP_DB_MASTER_BPMP>;
+        memory-region = <&dram_cpu_bpmp_mail>;
+        #clock-cells = <1>;
+        #power-domain-cells = <1>;
+        #reset-cells = <1>;
+    };
-- 
2.34.1

Re: [PATCH v2 5/6] dt-bindings: memory-region property for tegra186-bpmp

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 848 bytes --]

On Wed, May 10, 2023 at 02:31:34PM +0300, Peter De Schrijver wrote:
> Add memory-region property to the tegra186-bpmp binding to support
> DRAM MRQ GSCs.
> 
> Co-developed-by: Stefan Kristiansson <stefank@nvidia.com>
> Signed-off-by: Stefan Kristiansson <stefank@nvidia.com>
> Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
> ---
>  .../firmware/nvidia,tegra186-bpmp.yaml        | 37 +++++++++++++++++--
>  1 file changed, 34 insertions(+), 3 deletions(-)

We usually use a longer subject prefix, even though that makes the whole
subject usually become longer than the recommended 50 (or so)
characters.

Maybe you also want to add a verb to the subject to make it more
descriptive. As it is the subject doesn't indicate what's happening with
the memory-region property. It could equally well get removed.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 6/6] firmware: tegra: bpmp: Add support for DRAM MRQ GSCs

[Peter De Schrijver]

Implement support for DRAM MRQ GSCs.

Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
---
 drivers/firmware/tegra/bpmp-tegra186.c | 214 +++++++++++++++++--------
 drivers/firmware/tegra/bpmp.c          |   4 +-
 2 files changed, 153 insertions(+), 65 deletions(-)

diff --git a/drivers/firmware/tegra/bpmp-tegra186.c b/drivers/firmware/tegra/bpmp-tegra186.c
index 2e26199041cd..43e2563575fc 100644
--- a/drivers/firmware/tegra/bpmp-tegra186.c
+++ b/drivers/firmware/tegra/bpmp-tegra186.c
@@ -4,8 +4,11 @@
  */
 
 #include <linux/genalloc.h>
+#include <linux/io.h>
 #include <linux/mailbox_client.h>
+#include <linux/of_address.h>
 #include <linux/platform_device.h>
+#include <linux/range.h>
 
 #include <soc/tegra/bpmp.h>
 #include <soc/tegra/bpmp-abi.h>
@@ -13,12 +16,13 @@
 
 #include "bpmp-private.h"
 
+enum tegra_bpmp_mem_type { TEGRA_INVALID, TEGRA_SRAM, TEGRA_RMEM };
+
 struct tegra186_bpmp {
 	struct tegra_bpmp *parent;
 
 	struct {
-		struct gen_pool *pool;
-		void __iomem *virt;
+		void *virt;
 		dma_addr_t phys;
 	} tx, rx;
 
@@ -26,6 +30,12 @@ struct tegra186_bpmp {
 		struct mbox_client client;
 		struct mbox_chan *channel;
 	} mbox;
+
+	struct {
+		struct gen_pool *tx, *rx;
+	} sram;
+
+	enum tegra_bpmp_mem_type type;
 };
 
 static inline struct tegra_bpmp *
@@ -118,8 +128,8 @@ static int tegra186_bpmp_channel_init(struct tegra_bpmp_channel *channel,
 	queue_size = tegra_ivc_total_queue_size(message_size);
 	offset = queue_size * index;
 
-	iosys_map_set_vaddr_iomem(&rx, priv->rx.virt + offset);
-	iosys_map_set_vaddr_iomem(&tx, priv->tx.virt + offset);
+	iosys_map_set_vaddr_iomem(&rx, (void __iomem *)priv->rx.virt + offset);
+	iosys_map_set_vaddr_iomem(&tx, (void __iomem *)priv->tx.virt + offset);
 
 	err = tegra_ivc_init(channel->ivc, NULL, &rx, priv->rx.phys + offset, &tx,
 			     priv->tx.phys + offset, 1, message_size, tegra186_bpmp_ivc_notify,
@@ -158,64 +168,171 @@ static void mbox_handle_rx(struct mbox_client *client, void *data)
 	tegra_bpmp_handle_rx(bpmp);
 }
 
-static int tegra186_bpmp_init(struct tegra_bpmp *bpmp)
+static void tegra186_bpmp_channel_deinit(struct tegra_bpmp *bpmp)
+{
+	int i;
+	struct tegra186_bpmp *priv = bpmp->priv;
+
+	for (i = 0; i < bpmp->threaded.count; i++) {
+		if (!bpmp->threaded_channels[i].bpmp)
+			continue;
+
+		tegra186_bpmp_channel_cleanup(&bpmp->threaded_channels[i]);
+	}
+
+	tegra186_bpmp_channel_cleanup(bpmp->rx_channel);
+	tegra186_bpmp_channel_cleanup(bpmp->tx_channel);
+
+	if (priv->type == TEGRA_SRAM) {
+		gen_pool_free(priv->sram.tx, (unsigned long)priv->tx.virt, 4096);
+		gen_pool_free(priv->sram.rx, (unsigned long)priv->rx.virt, 4096);
+	} else if (priv->type == TEGRA_RMEM) {
+		memunmap(priv->tx.virt);
+	}
+}
+
+static int tegra186_bpmp_channel_setup(struct tegra_bpmp *bpmp)
 {
-	struct tegra186_bpmp *priv;
 	unsigned int i;
 	int err;
 
-	priv = devm_kzalloc(bpmp->dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
+	err = tegra186_bpmp_channel_init(bpmp->tx_channel, bpmp,
+					 bpmp->soc->channels.cpu_tx.offset);
+	if (err < 0)
+		return err;
 
-	bpmp->priv = priv;
-	priv->parent = bpmp;
+	err = tegra186_bpmp_channel_init(bpmp->rx_channel, bpmp,
+					 bpmp->soc->channels.cpu_rx.offset);
+	if (err < 0) {
+		tegra186_bpmp_channel_cleanup(bpmp->tx_channel);
+		return err;
+	}
+
+	for (i = 0; i < bpmp->threaded.count; i++) {
+		unsigned int index = bpmp->soc->channels.thread.offset + i;
 
-	priv->tx.pool = of_gen_pool_get(bpmp->dev->of_node, "shmem", 0);
-	if (!priv->tx.pool) {
+		err = tegra186_bpmp_channel_init(&bpmp->threaded_channels[i],
+						 bpmp, index);
+		if (err < 0)
+			break;
+	}
+
+	if (err < 0)
+		tegra186_bpmp_channel_deinit(bpmp);
+
+	return err;
+}
+
+static void tegra186_bpmp_reset_channels(struct tegra_bpmp *bpmp)
+{
+	unsigned int i;
+
+	tegra186_bpmp_channel_reset(bpmp->tx_channel);
+	tegra186_bpmp_channel_reset(bpmp->rx_channel);
+
+	for (i = 0; i < bpmp->threaded.count; i++)
+		tegra186_bpmp_channel_reset(&bpmp->threaded_channels[i]);
+}
+
+static int tegra186_bpmp_sram_init(struct tegra_bpmp *bpmp)
+{
+	int err;
+	struct tegra186_bpmp *priv = bpmp->priv;
+
+	priv->sram.tx = of_gen_pool_get(bpmp->dev->of_node, "shmem", 0);
+	if (!priv->sram.tx) {
 		dev_err(bpmp->dev, "TX shmem pool not found\n");
 		return -EPROBE_DEFER;
 	}
 
-	priv->tx.virt = (void __iomem *)gen_pool_dma_alloc(priv->tx.pool, 4096, &priv->tx.phys);
+	priv->tx.virt = gen_pool_dma_alloc(priv->sram.tx, 4096, &priv->tx.phys);
 	if (!priv->tx.virt) {
 		dev_err(bpmp->dev, "failed to allocate from TX pool\n");
 		return -ENOMEM;
 	}
 
-	priv->rx.pool = of_gen_pool_get(bpmp->dev->of_node, "shmem", 1);
-	if (!priv->rx.pool) {
+	priv->sram.rx = of_gen_pool_get(bpmp->dev->of_node, "shmem", 1);
+	if (!priv->sram.rx) {
 		dev_err(bpmp->dev, "RX shmem pool not found\n");
 		err = -EPROBE_DEFER;
 		goto free_tx;
 	}
 
-	priv->rx.virt = (void __iomem *)gen_pool_dma_alloc(priv->rx.pool, 4096, &priv->rx.phys);
+	priv->rx.virt = gen_pool_dma_alloc(priv->sram.rx, 4096, &priv->rx.phys);
 	if (!priv->rx.virt) {
 		dev_err(bpmp->dev, "failed to allocate from RX pool\n");
 		err = -ENOMEM;
 		goto free_tx;
 	}
 
-	err = tegra186_bpmp_channel_init(bpmp->tx_channel, bpmp,
-					 bpmp->soc->channels.cpu_tx.offset);
-	if (err < 0)
-		goto free_rx;
+	priv->type = TEGRA_SRAM;
 
-	err = tegra186_bpmp_channel_init(bpmp->rx_channel, bpmp,
-					 bpmp->soc->channels.cpu_rx.offset);
-	if (err < 0)
-		goto cleanup_tx_channel;
+	return 0;
 
-	for (i = 0; i < bpmp->threaded.count; i++) {
-		unsigned int index = bpmp->soc->channels.thread.offset + i;
+free_tx:
+	gen_pool_free(priv->sram.tx, (unsigned long)priv->tx.virt, 4096);
 
-		err = tegra186_bpmp_channel_init(&bpmp->threaded_channels[i],
-						 bpmp, index);
+	return err;
+}
+
+static enum tegra_bpmp_mem_type tegra186_bpmp_dram_init(struct tegra_bpmp *bpmp)
+{
+	int err;
+	struct resource res;
+	struct device_node *np;
+	struct tegra186_bpmp *priv = bpmp->priv;
+
+	np = of_parse_phandle(bpmp->dev->of_node, "memory-region", 0);
+	if (!np)
+		return TEGRA_INVALID;
+
+	err = of_address_to_resource(np, 0, &res);
+	if (err) {
+		dev_warn(bpmp->dev,  "Parsing memory region returned: %d\n", err);
+		return TEGRA_INVALID;
+	}
+
+	if ((res.end - res.start + 1) < 0x2000) {
+		dev_warn(bpmp->dev,  "DRAM region less than 0x2000 bytes\n");
+		return TEGRA_INVALID;
+	}
+
+	priv->tx.phys = res.start;
+	priv->rx.phys = res.start + 0x1000;
+
+	priv->tx.virt = memremap(priv->tx.phys, res.end - res.start + 1, MEMREMAP_WC);
+	if (priv->tx.virt == NULL) {
+		dev_warn(bpmp->dev,  "DRAM region mapping failed\n");
+		return TEGRA_INVALID;
+	}
+	priv->rx.virt = priv->tx.virt + 0x1000;
+
+	return TEGRA_RMEM;
+}
+
+static int tegra186_bpmp_init(struct tegra_bpmp *bpmp)
+{
+	struct tegra186_bpmp *priv;
+	int err;
+
+	priv = devm_kzalloc(bpmp->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	bpmp->priv = priv;
+	priv->parent = bpmp;
+
+	priv->type = tegra186_bpmp_dram_init(bpmp);
+	if (priv->type == TEGRA_INVALID) {
+		err = tegra186_bpmp_sram_init(bpmp);
 		if (err < 0)
-			goto cleanup_channels;
+			return err;
 	}
 
+	err = tegra186_bpmp_channel_setup(bpmp);
+	if (err < 0)
+		return err;
+
 	/* mbox registration */
 	priv->mbox.client.dev = bpmp->dev;
 	priv->mbox.client.rx_callback = mbox_handle_rx;
@@ -226,51 +343,22 @@ static int tegra186_bpmp_init(struct tegra_bpmp *bpmp)
 	if (IS_ERR(priv->mbox.channel)) {
 		err = PTR_ERR(priv->mbox.channel);
 		dev_err(bpmp->dev, "failed to get HSP mailbox: %d\n", err);
-		goto cleanup_channels;
+		tegra186_bpmp_channel_deinit(bpmp);
+		return err;
 	}
 
-	tegra186_bpmp_channel_reset(bpmp->tx_channel);
-	tegra186_bpmp_channel_reset(bpmp->rx_channel);
-
-	for (i = 0; i < bpmp->threaded.count; i++)
-		tegra186_bpmp_channel_reset(&bpmp->threaded_channels[i]);
+	tegra186_bpmp_reset_channels(bpmp);
 
 	return 0;
-
-cleanup_channels:
-	for (i = 0; i < bpmp->threaded.count; i++) {
-		if (!bpmp->threaded_channels[i].bpmp)
-			continue;
-
-		tegra186_bpmp_channel_cleanup(&bpmp->threaded_channels[i]);
-	}
-
-	tegra186_bpmp_channel_cleanup(bpmp->rx_channel);
-cleanup_tx_channel:
-	tegra186_bpmp_channel_cleanup(bpmp->tx_channel);
-free_rx:
-	gen_pool_free(priv->rx.pool, (unsigned long)priv->rx.virt, 4096);
-free_tx:
-	gen_pool_free(priv->tx.pool, (unsigned long)priv->tx.virt, 4096);
-
-	return err;
 }
 
 static void tegra186_bpmp_deinit(struct tegra_bpmp *bpmp)
 {
 	struct tegra186_bpmp *priv = bpmp->priv;
-	unsigned int i;
 
 	mbox_free_channel(priv->mbox.channel);
 
-	for (i = 0; i < bpmp->threaded.count; i++)
-		tegra186_bpmp_channel_cleanup(&bpmp->threaded_channels[i]);
-
-	tegra186_bpmp_channel_cleanup(bpmp->rx_channel);
-	tegra186_bpmp_channel_cleanup(bpmp->tx_channel);
-
-	gen_pool_free(priv->rx.pool, (unsigned long)priv->rx.virt, 4096);
-	gen_pool_free(priv->tx.pool, (unsigned long)priv->tx.virt, 4096);
+	tegra186_bpmp_channel_deinit(bpmp);
 }
 
 static int tegra186_bpmp_resume(struct tegra_bpmp *bpmp)
diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
index 8b5e5daa9fae..17bd3590aaa2 100644
--- a/drivers/firmware/tegra/bpmp.c
+++ b/drivers/firmware/tegra/bpmp.c
@@ -735,6 +735,8 @@ static int tegra_bpmp_probe(struct platform_device *pdev)
 	if (!bpmp->threaded_channels)
 		return -ENOMEM;
 
+	platform_set_drvdata(pdev, bpmp);
+
 	err = bpmp->soc->ops->init(bpmp);
 	if (err < 0)
 		return err;
@@ -758,8 +760,6 @@ static int tegra_bpmp_probe(struct platform_device *pdev)
 
 	dev_info(&pdev->dev, "firmware: %.*s\n", (int)sizeof(tag), tag);
 
-	platform_set_drvdata(pdev, bpmp);
-
 	err = of_platform_default_populate(pdev->dev.of_node, NULL, &pdev->dev);
 	if (err < 0)
 		goto free_mrq;
-- 
2.34.1

Re: [PATCH v2 6/6] firmware: tegra: bpmp: Add support for DRAM MRQ GSCs

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 14988 bytes --]

On Wed, May 10, 2023 at 02:31:36PM +0300, Peter De Schrijver wrote:
> Implement support for DRAM MRQ GSCs.
> 
> Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
> ---
>  drivers/firmware/tegra/bpmp-tegra186.c | 214 +++++++++++++++++--------
>  drivers/firmware/tegra/bpmp.c          |   4 +-
>  2 files changed, 153 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/firmware/tegra/bpmp-tegra186.c b/drivers/firmware/tegra/bpmp-tegra186.c
> index 2e26199041cd..43e2563575fc 100644
> --- a/drivers/firmware/tegra/bpmp-tegra186.c
> +++ b/drivers/firmware/tegra/bpmp-tegra186.c
> @@ -4,8 +4,11 @@
>   */
>  
>  #include <linux/genalloc.h>
> +#include <linux/io.h>
>  #include <linux/mailbox_client.h>
> +#include <linux/of_address.h>
>  #include <linux/platform_device.h>
> +#include <linux/range.h>

Why do we need range.h?

>  
>  #include <soc/tegra/bpmp.h>
>  #include <soc/tegra/bpmp-abi.h>
> @@ -13,12 +16,13 @@
>  
>  #include "bpmp-private.h"
>  
> +enum tegra_bpmp_mem_type { TEGRA_INVALID, TEGRA_SRAM, TEGRA_RMEM };
> +

This is a strange construct. We can already use the pool pointer to
determine which type of memory is being used. Your usage of this leads
to very unintuitive code when you're error checking, etc. and prevents
you from propagating proper error codes.

>  struct tegra186_bpmp {
>  	struct tegra_bpmp *parent;
>  
>  	struct {
> -		struct gen_pool *pool;
> -		void __iomem *virt;
> +		void *virt;

I think what we really need is a union that contains both an __iomem
annotated pointer and a regular one.

>  		dma_addr_t phys;
>  	} tx, rx;
>  
> @@ -26,6 +30,12 @@ struct tegra186_bpmp {
>  		struct mbox_client client;
>  		struct mbox_chan *channel;
>  	} mbox;
> +
> +	struct {
> +		struct gen_pool *tx, *rx;
> +	} sram;

Please keep this in the tx/rx structure. This would perhaps be useful if
there was an equivalent "dram" structure, but as it is there's no
advantage in keeping this separate from the other memory-related fields.

> +
> +	enum tegra_bpmp_mem_type type;
>  };
>  
>  static inline struct tegra_bpmp *
> @@ -118,8 +128,8 @@ static int tegra186_bpmp_channel_init(struct tegra_bpmp_channel *channel,
>  	queue_size = tegra_ivc_total_queue_size(message_size);
>  	offset = queue_size * index;
>  
> -	iosys_map_set_vaddr_iomem(&rx, priv->rx.virt + offset);
> -	iosys_map_set_vaddr_iomem(&tx, priv->tx.virt + offset);
> +	iosys_map_set_vaddr_iomem(&rx, (void __iomem *)priv->rx.virt + offset);
> +	iosys_map_set_vaddr_iomem(&tx, (void __iomem *)priv->tx.virt + offset);

This completely defies the purpose of using the iosys_map helpers. What
you really want to do is check if we're using SRAM and use the _iomem
variant, otherwise, use the plain one, something like:

	if (priv->rx.pool)
		iosys_map_set_vaddr_iomem(&rx, priv->rx.sram + offset);
	else
		iosys_map_set_vaddr(&rx, priv->rx.dram + offset);

And repeat that for TX. I suppose you could also do the iosys_map
assignment for both in the same blocks above since we don't support
mixing SRAM and DRAM modes.

>  
>  	err = tegra_ivc_init(channel->ivc, NULL, &rx, priv->rx.phys + offset, &tx,
>  			     priv->tx.phys + offset, 1, message_size, tegra186_bpmp_ivc_notify,
> @@ -158,64 +168,171 @@ static void mbox_handle_rx(struct mbox_client *client, void *data)
>  	tegra_bpmp_handle_rx(bpmp);
>  }
>  
> -static int tegra186_bpmp_init(struct tegra_bpmp *bpmp)
> +static void tegra186_bpmp_channel_deinit(struct tegra_bpmp *bpmp)
> +{
> +	int i;

Can be unsigned int. The preferred ordering for variable declarations is
the inverse christmas tree (i.e. sort by length in decreasing order). It
often matches the result of sorting by importance (i.e. the "priv"
pointer is more important than the loop variable).

> +	struct tegra186_bpmp *priv = bpmp->priv;
> +
> +	for (i = 0; i < bpmp->threaded.count; i++) {
> +		if (!bpmp->threaded_channels[i].bpmp)
> +			continue;
> +
> +		tegra186_bpmp_channel_cleanup(&bpmp->threaded_channels[i]);
> +	}
> +
> +	tegra186_bpmp_channel_cleanup(bpmp->rx_channel);
> +	tegra186_bpmp_channel_cleanup(bpmp->tx_channel);
> +
> +	if (priv->type == TEGRA_SRAM) {
> +		gen_pool_free(priv->sram.tx, (unsigned long)priv->tx.virt, 4096);
> +		gen_pool_free(priv->sram.rx, (unsigned long)priv->rx.virt, 4096);
> +	} else if (priv->type == TEGRA_RMEM) {
> +		memunmap(priv->tx.virt);
> +	}

This introduces a bit of an asymmetry because tegra_bpmp_channel_setup()
doesn't actually set up the pool or reserved-memory. Since the memory is
only used for the channels, we can probably move the initialization into
tegra186_bpmp_channel_setup() below.

> +}
> +
> +static int tegra186_bpmp_channel_setup(struct tegra_bpmp *bpmp)

This name could be confusing because we already use the
tegra186_bpmp_channel_ prefix for functions that operate on individual
channels, whereas this function operates on the BPMP object.

Perhaps something like tegra186_bpmp_setup_channels() would better
reflect what this does.

The same goes for tegra186_bpmp_channel_deinit() above. Maybe something
like tegra186_bpmp_cleanup_channels() to make it more obvious that it's
the counterpart of tegra186_bpmp_setup_channels().

>  {
> -	struct tegra186_bpmp *priv;
>  	unsigned int i;
>  	int err;
>  
> -	priv = devm_kzalloc(bpmp->dev, sizeof(*priv), GFP_KERNEL);
> -	if (!priv)
> -		return -ENOMEM;
> +	err = tegra186_bpmp_channel_init(bpmp->tx_channel, bpmp,
> +					 bpmp->soc->channels.cpu_tx.offset);
> +	if (err < 0)
> +		return err;
>  
> -	bpmp->priv = priv;
> -	priv->parent = bpmp;
> +	err = tegra186_bpmp_channel_init(bpmp->rx_channel, bpmp,
> +					 bpmp->soc->channels.cpu_rx.offset);
> +	if (err < 0) {
> +		tegra186_bpmp_channel_cleanup(bpmp->tx_channel);
> +		return err;
> +	}
> +
> +	for (i = 0; i < bpmp->threaded.count; i++) {
> +		unsigned int index = bpmp->soc->channels.thread.offset + i;
>  
> -	priv->tx.pool = of_gen_pool_get(bpmp->dev->of_node, "shmem", 0);
> -	if (!priv->tx.pool) {
> +		err = tegra186_bpmp_channel_init(&bpmp->threaded_channels[i],
> +						 bpmp, index);
> +		if (err < 0)
> +			break;
> +	}
> +
> +	if (err < 0)
> +		tegra186_bpmp_channel_deinit(bpmp);

See how the name is confusing here? This is very close to the call to
tegra186_bpmp_channel_init() above and the common prefix makes it seem
like this would undo the effects of the above and then immediately
raises the question why it's only undoing all of the above channel
initializations. You then have to actually look at the implementation to
find out that that's exactly what it does.

> +
> +	return err;
> +}
> +
> +static void tegra186_bpmp_reset_channels(struct tegra_bpmp *bpmp)
> +{
> +	unsigned int i;
> +
> +	tegra186_bpmp_channel_reset(bpmp->tx_channel);
> +	tegra186_bpmp_channel_reset(bpmp->rx_channel);
> +
> +	for (i = 0; i < bpmp->threaded.count; i++)
> +		tegra186_bpmp_channel_reset(&bpmp->threaded_channels[i]);
> +}

I think this now matches the tegra186_bpmp_resume() implementation, so
it could be reused in that.

> +
> +static int tegra186_bpmp_sram_init(struct tegra_bpmp *bpmp)
> +{
> +	int err;
> +	struct tegra186_bpmp *priv = bpmp->priv;
> +
> +	priv->sram.tx = of_gen_pool_get(bpmp->dev->of_node, "shmem", 0);
> +	if (!priv->sram.tx) {
>  		dev_err(bpmp->dev, "TX shmem pool not found\n");
>  		return -EPROBE_DEFER;
>  	}
>  
> -	priv->tx.virt = (void __iomem *)gen_pool_dma_alloc(priv->tx.pool, 4096, &priv->tx.phys);
> +	priv->tx.virt = gen_pool_dma_alloc(priv->sram.tx, 4096, &priv->tx.phys);
>  	if (!priv->tx.virt) {
>  		dev_err(bpmp->dev, "failed to allocate from TX pool\n");
>  		return -ENOMEM;
>  	}
>  
> -	priv->rx.pool = of_gen_pool_get(bpmp->dev->of_node, "shmem", 1);
> -	if (!priv->rx.pool) {
> +	priv->sram.rx = of_gen_pool_get(bpmp->dev->of_node, "shmem", 1);
> +	if (!priv->sram.rx) {
>  		dev_err(bpmp->dev, "RX shmem pool not found\n");
>  		err = -EPROBE_DEFER;
>  		goto free_tx;
>  	}
>  
> -	priv->rx.virt = (void __iomem *)gen_pool_dma_alloc(priv->rx.pool, 4096, &priv->rx.phys);
> +	priv->rx.virt = gen_pool_dma_alloc(priv->sram.rx, 4096, &priv->rx.phys);
>  	if (!priv->rx.virt) {
>  		dev_err(bpmp->dev, "failed to allocate from RX pool\n");
>  		err = -ENOMEM;
>  		goto free_tx;
>  	}
>  
> -	err = tegra186_bpmp_channel_init(bpmp->tx_channel, bpmp,
> -					 bpmp->soc->channels.cpu_tx.offset);
> -	if (err < 0)
> -		goto free_rx;
> +	priv->type = TEGRA_SRAM;
>  
> -	err = tegra186_bpmp_channel_init(bpmp->rx_channel, bpmp,
> -					 bpmp->soc->channels.cpu_rx.offset);
> -	if (err < 0)
> -		goto cleanup_tx_channel;
> +	return 0;
>  
> -	for (i = 0; i < bpmp->threaded.count; i++) {
> -		unsigned int index = bpmp->soc->channels.thread.offset + i;
> +free_tx:
> +	gen_pool_free(priv->sram.tx, (unsigned long)priv->tx.virt, 4096);
>  
> -		err = tegra186_bpmp_channel_init(&bpmp->threaded_channels[i],
> -						 bpmp, index);
> +	return err;
> +}
> +
> +static enum tegra_bpmp_mem_type tegra186_bpmp_dram_init(struct tegra_bpmp *bpmp)
> +{
> +	int err;
> +	struct resource res;
> +	struct device_node *np;
> +	struct tegra186_bpmp *priv = bpmp->priv;
> +
> +	np = of_parse_phandle(bpmp->dev->of_node, "memory-region", 0);
> +	if (!np)
> +		return TEGRA_INVALID;
> +
> +	err = of_address_to_resource(np, 0, &res);
> +	if (err) {
> +		dev_warn(bpmp->dev,  "Parsing memory region returned: %d\n", err);
> +		return TEGRA_INVALID;
> +	}
> +
> +	if ((res.end - res.start + 1) < 0x2000) {

resource_size(), and maybe use SZ_8K instead of the literal here.

> +		dev_warn(bpmp->dev,  "DRAM region less than 0x2000 bytes\n");

Also, better to use a more human-readable string here. While at it,
perhaps we can make this a bit more assertive, maybe something like:

	"DRAM region must be larger than 8 KiB"

?

> +		return TEGRA_INVALID;
> +	}

This doesn't allow the caller to differentiate between potentially fatal
errors and non-fatal ones. For instance, we don't want the absence of a
"memory-region" property to be fatal (because we want to fall back to
use SRAM in that case, or at least attempt to), but if "memory-region"
exists, any of the subsequent errors probably should be fatal. It's
easier to deal with that situation if you return regular error codes
here. The !np check above could return -ENODEV, for example, as a way of
letting the caller know that we don't have DRAM support in DT. For the
of_address_to_resource() failure we can instead propagate the error code
and so on.

Also, I think it'd be better to use a named constant like SZ_8K instead
of the literal 0x2000 above. 

> +
> +	priv->tx.phys = res.start;
> +	priv->rx.phys = res.start + 0x1000;

SZ_4K

> +
> +	priv->tx.virt = memremap(priv->tx.phys, res.end - res.start + 1, MEMREMAP_WC);

Another case where we can use resource_size(). Might be a good idea to
introduce a local "size" variable.

> +	if (priv->tx.virt == NULL) {
> +		dev_warn(bpmp->dev,  "DRAM region mapping failed\n");
> +		return TEGRA_INVALID;
> +	}
> +	priv->rx.virt = priv->tx.virt + 0x1000;

SZ_4K

We should probably do the same thing for the SRAM paths, but that should
be a separate patch and can be done at another time.

> +
> +	return TEGRA_RMEM;
> +}
> +
> +static int tegra186_bpmp_init(struct tegra_bpmp *bpmp)
> +{
> +	struct tegra186_bpmp *priv;
> +	int err;
> +
> +	priv = devm_kzalloc(bpmp->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	bpmp->priv = priv;
> +	priv->parent = bpmp;
> +
> +	priv->type = tegra186_bpmp_dram_init(bpmp);
> +	if (priv->type == TEGRA_INVALID) {
> +		err = tegra186_bpmp_sram_init(bpmp);
>  		if (err < 0)
> -			goto cleanup_channels;
> +			return err;
>  	}

As I mentioned previously, I think we can move the block above into
tegra186_bpmp_setup_channels() to make it symmetric with the teardown of
this in tegra186_bpmp_cleanup_channels().

Thierry

>  
> +	err = tegra186_bpmp_channel_setup(bpmp);
> +	if (err < 0)
> +		return err;
> +
>  	/* mbox registration */
>  	priv->mbox.client.dev = bpmp->dev;
>  	priv->mbox.client.rx_callback = mbox_handle_rx;
> @@ -226,51 +343,22 @@ static int tegra186_bpmp_init(struct tegra_bpmp *bpmp)
>  	if (IS_ERR(priv->mbox.channel)) {
>  		err = PTR_ERR(priv->mbox.channel);
>  		dev_err(bpmp->dev, "failed to get HSP mailbox: %d\n", err);
> -		goto cleanup_channels;
> +		tegra186_bpmp_channel_deinit(bpmp);
> +		return err;
>  	}
>  
> -	tegra186_bpmp_channel_reset(bpmp->tx_channel);
> -	tegra186_bpmp_channel_reset(bpmp->rx_channel);
> -
> -	for (i = 0; i < bpmp->threaded.count; i++)
> -		tegra186_bpmp_channel_reset(&bpmp->threaded_channels[i]);
> +	tegra186_bpmp_reset_channels(bpmp);
>  
>  	return 0;
> -
> -cleanup_channels:
> -	for (i = 0; i < bpmp->threaded.count; i++) {
> -		if (!bpmp->threaded_channels[i].bpmp)
> -			continue;
> -
> -		tegra186_bpmp_channel_cleanup(&bpmp->threaded_channels[i]);
> -	}
> -
> -	tegra186_bpmp_channel_cleanup(bpmp->rx_channel);
> -cleanup_tx_channel:
> -	tegra186_bpmp_channel_cleanup(bpmp->tx_channel);
> -free_rx:
> -	gen_pool_free(priv->rx.pool, (unsigned long)priv->rx.virt, 4096);
> -free_tx:
> -	gen_pool_free(priv->tx.pool, (unsigned long)priv->tx.virt, 4096);
> -
> -	return err;
>  }
>  
>  static void tegra186_bpmp_deinit(struct tegra_bpmp *bpmp)
>  {
>  	struct tegra186_bpmp *priv = bpmp->priv;
> -	unsigned int i;
>  
>  	mbox_free_channel(priv->mbox.channel);
>  
> -	for (i = 0; i < bpmp->threaded.count; i++)
> -		tegra186_bpmp_channel_cleanup(&bpmp->threaded_channels[i]);
> -
> -	tegra186_bpmp_channel_cleanup(bpmp->rx_channel);
> -	tegra186_bpmp_channel_cleanup(bpmp->tx_channel);
> -
> -	gen_pool_free(priv->rx.pool, (unsigned long)priv->rx.virt, 4096);
> -	gen_pool_free(priv->tx.pool, (unsigned long)priv->tx.virt, 4096);
> +	tegra186_bpmp_channel_deinit(bpmp);
>  }
>  
>  static int tegra186_bpmp_resume(struct tegra_bpmp *bpmp)
> diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
> index 8b5e5daa9fae..17bd3590aaa2 100644
> --- a/drivers/firmware/tegra/bpmp.c
> +++ b/drivers/firmware/tegra/bpmp.c
> @@ -735,6 +735,8 @@ static int tegra_bpmp_probe(struct platform_device *pdev)
>  	if (!bpmp->threaded_channels)
>  		return -ENOMEM;
>  
> +	platform_set_drvdata(pdev, bpmp);
> +
>  	err = bpmp->soc->ops->init(bpmp);
>  	if (err < 0)
>  		return err;
> @@ -758,8 +760,6 @@ static int tegra_bpmp_probe(struct platform_device *pdev)
>  
>  	dev_info(&pdev->dev, "firmware: %.*s\n", (int)sizeof(tag), tag);
>  
> -	platform_set_drvdata(pdev, bpmp);
> -
>  	err = of_platform_default_populate(pdev->dev.of_node, NULL, &pdev->dev);
>  	if (err < 0)
>  		goto free_mrq;
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 6/6] firmware: tegra: bpmp: Add support for DRAM MRQ GSCs

[Peter De Schrijver]

On Wed, May 10, 2023 at 04:33:35PM +0200, Thierry Reding wrote:
> On Wed, May 10, 2023 at 02:31:36PM +0300, Peter De Schrijver wrote:
> > Implement support for DRAM MRQ GSCs.
> > 
> > Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
> > ---
> >  drivers/firmware/tegra/bpmp-tegra186.c | 214 +++++++++++++++++--------
> >  drivers/firmware/tegra/bpmp.c          |   4 +-
> >  2 files changed, 153 insertions(+), 65 deletions(-)
> > 
> > diff --git a/drivers/firmware/tegra/bpmp-tegra186.c b/drivers/firmware/tegra/bpmp-tegra186.c
> > index 2e26199041cd..43e2563575fc 100644
> > --- a/drivers/firmware/tegra/bpmp-tegra186.c
> > +++ b/drivers/firmware/tegra/bpmp-tegra186.c
> > @@ -4,8 +4,11 @@
> >   */
> >  
> >  #include <linux/genalloc.h>
> > +#include <linux/io.h>
> >  #include <linux/mailbox_client.h>
> > +#include <linux/of_address.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/range.h>
> 
> Why do we need range.h?
> 

We probably don't need this indeed.

> >  
> >  #include <soc/tegra/bpmp.h>
> >  #include <soc/tegra/bpmp-abi.h>
> > @@ -13,12 +16,13 @@
> >  
> >  #include "bpmp-private.h"
> >  
> > +enum tegra_bpmp_mem_type { TEGRA_INVALID, TEGRA_SRAM, TEGRA_RMEM };
> > +
> 
> This is a strange construct. We can already use the pool pointer to
> determine which type of memory is being used. Your usage of this leads
> to very unintuitive code when you're error checking, etc. and prevents
> you from propagating proper error codes.
> 

No? How would indicate SRAM and DRAM are mutually exclusive?

> >  struct tegra186_bpmp {
> >  	struct tegra_bpmp *parent;
> >  
> >  	struct {
> > -		struct gen_pool *pool;
> > -		void __iomem *virt;
> > +		void *virt;
> 
> I think what we really need is a union that contains both an __iomem
> annotated pointer and a regular one.

And then add a struct gen_pool * in the union and use the enum as a tag.

> 
> >  		dma_addr_t phys;
> >  	} tx, rx;
> >  
> > @@ -26,6 +30,12 @@ struct tegra186_bpmp {
> >  		struct mbox_client client;
> >  		struct mbox_chan *channel;
> >  	} mbox;
> > +
> > +	struct {
> > +		struct gen_pool *tx, *rx;
> > +	} sram;
> 
> Please keep this in the tx/rx structure. This would perhaps be useful if
> there was an equivalent "dram" structure, but as it is there's no
> advantage in keeping this separate from the other memory-related fields.
> 
> > +
> > +	enum tegra_bpmp_mem_type type;
> >  };
> >  
> >  static inline struct tegra_bpmp *
> > @@ -118,8 +128,8 @@ static int tegra186_bpmp_channel_init(struct tegra_bpmp_channel *channel,
> >  	queue_size = tegra_ivc_total_queue_size(message_size);
> >  	offset = queue_size * index;
> >  
> > -	iosys_map_set_vaddr_iomem(&rx, priv->rx.virt + offset);
> > -	iosys_map_set_vaddr_iomem(&tx, priv->tx.virt + offset);
> > +	iosys_map_set_vaddr_iomem(&rx, (void __iomem *)priv->rx.virt + offset);
> > +	iosys_map_set_vaddr_iomem(&tx, (void __iomem *)priv->tx.virt + offset);
> 
> This completely defies the purpose of using the iosys_map helpers. What
> you really want to do is check if we're using SRAM and use the _iomem
> variant, otherwise, use the plain one, something like:
> 
> 	if (priv->rx.pool)
> 		iosys_map_set_vaddr_iomem(&rx, priv->rx.sram + offset);

Even the current code does not have an rx.sram field, so I don't quite
understand what you mean here.

> 	else
> 		iosys_map_set_vaddr(&rx, priv->rx.dram + offset);
> 
> And repeat that for TX. I suppose you could also do the iosys_map
> assignment for both in the same blocks above since we don't support
> mixing SRAM and DRAM modes.
> 

Currently the code does:

iosys_map_set_vaddr_iomem(&rx, priv->rx.virt + offset);

and priv->rx.virt is initialized by:

priv->rx.virt = (void __iomem *)gen_pool_dma_alloc();

so we cast this today as well?

> >  
> >  	err = tegra_ivc_init(channel->ivc, NULL, &rx, priv->rx.phys + offset, &tx,
> >  			     priv->tx.phys + offset, 1, message_size, tegra186_bpmp_ivc_notify,
> > @@ -158,64 +168,171 @@ static void mbox_handle_rx(struct mbox_client *client, void *data)
> >  	tegra_bpmp_handle_rx(bpmp);
> >  }
> >  
> > -static int tegra186_bpmp_init(struct tegra_bpmp *bpmp)
> > +static void tegra186_bpmp_channel_deinit(struct tegra_bpmp *bpmp)
> > +{
> > +	int i;
> 
> Can be unsigned int. The preferred ordering for variable declarations is
> the inverse christmas tree (i.e. sort by length in decreasing order). It
> often matches the result of sorting by importance (i.e. the "priv"
> pointer is more important than the loop variable).
> 
> > +	struct tegra186_bpmp *priv = bpmp->priv;
> > +
> > +	for (i = 0; i < bpmp->threaded.count; i++) {
> > +		if (!bpmp->threaded_channels[i].bpmp)
> > +			continue;
> > +
> > +		tegra186_bpmp_channel_cleanup(&bpmp->threaded_channels[i]);
> > +	}
> > +
> > +	tegra186_bpmp_channel_cleanup(bpmp->rx_channel);
> > +	tegra186_bpmp_channel_cleanup(bpmp->tx_channel);
> > +
> > +	if (priv->type == TEGRA_SRAM) {
> > +		gen_pool_free(priv->sram.tx, (unsigned long)priv->tx.virt, 4096);
> > +		gen_pool_free(priv->sram.rx, (unsigned long)priv->rx.virt, 4096);
> > +	} else if (priv->type == TEGRA_RMEM) {
> > +		memunmap(priv->tx.virt);
> > +	}
> 
> This introduces a bit of an asymmetry because tegra_bpmp_channel_setup()
> doesn't actually set up the pool or reserved-memory. Since the memory is
> only used for the channels, we can probably move the initialization into
> tegra186_bpmp_channel_setup() below.

This is the teardown, not the initialization?

> 
> > +}
> > +
> > +static int tegra186_bpmp_channel_setup(struct tegra_bpmp *bpmp)
> 
> This name could be confusing because we already use the
> tegra186_bpmp_channel_ prefix for functions that operate on individual
> channels, whereas this function operates on the BPMP object.
> 
> Perhaps something like tegra186_bpmp_setup_channels() would better
> reflect what this does.
> 
> The same goes for tegra186_bpmp_channel_deinit() above. Maybe something
> like tegra186_bpmp_cleanup_channels() to make it more obvious that it's
> the counterpart of tegra186_bpmp_setup_channels().
> 
> >  {
> > -	struct tegra186_bpmp *priv;
> >  	unsigned int i;
> >  	int err;
> >  
> > -	priv = devm_kzalloc(bpmp->dev, sizeof(*priv), GFP_KERNEL);
> > -	if (!priv)
> > -		return -ENOMEM;
> > +	err = tegra186_bpmp_channel_init(bpmp->tx_channel, bpmp,
> > +					 bpmp->soc->channels.cpu_tx.offset);
> > +	if (err < 0)
> > +		return err;
> >  
> > -	bpmp->priv = priv;
> > -	priv->parent = bpmp;
> > +	err = tegra186_bpmp_channel_init(bpmp->rx_channel, bpmp,
> > +					 bpmp->soc->channels.cpu_rx.offset);
> > +	if (err < 0) {
> > +		tegra186_bpmp_channel_cleanup(bpmp->tx_channel);
> > +		return err;
> > +	}
> > +
> > +	for (i = 0; i < bpmp->threaded.count; i++) {
> > +		unsigned int index = bpmp->soc->channels.thread.offset + i;
> >  
> > -	priv->tx.pool = of_gen_pool_get(bpmp->dev->of_node, "shmem", 0);
> > -	if (!priv->tx.pool) {
> > +		err = tegra186_bpmp_channel_init(&bpmp->threaded_channels[i],
> > +						 bpmp, index);
> > +		if (err < 0)
> > +			break;
> > +	}
> > +
> > +	if (err < 0)
> > +		tegra186_bpmp_channel_deinit(bpmp);
> 
> See how the name is confusing here? This is very close to the call to
> tegra186_bpmp_channel_init() above and the common prefix makes it seem
> like this would undo the effects of the above and then immediately
> raises the question why it's only undoing all of the above channel
> initializations. You then have to actually look at the implementation to
> find out that that's exactly what it does.
> 
> > +
> > +	return err;
> > +}
> > +
> > +static void tegra186_bpmp_reset_channels(struct tegra_bpmp *bpmp)
> > +{
> > +	unsigned int i;
> > +
> > +	tegra186_bpmp_channel_reset(bpmp->tx_channel);
> > +	tegra186_bpmp_channel_reset(bpmp->rx_channel);
> > +
> > +	for (i = 0; i < bpmp->threaded.count; i++)
> > +		tegra186_bpmp_channel_reset(&bpmp->threaded_channels[i]);
> > +}
> 
> I think this now matches the tegra186_bpmp_resume() implementation, so
> it could be reused in that.
> 

Ok.

> > +
> > +static int tegra186_bpmp_sram_init(struct tegra_bpmp *bpmp)
> > +{
> > +	int err;
> > +	struct tegra186_bpmp *priv = bpmp->priv;
> > +
> > +	priv->sram.tx = of_gen_pool_get(bpmp->dev->of_node, "shmem", 0);
> > +	if (!priv->sram.tx) {
> >  		dev_err(bpmp->dev, "TX shmem pool not found\n");
> >  		return -EPROBE_DEFER;
> >  	}
> >  
> > -	priv->tx.virt = (void __iomem *)gen_pool_dma_alloc(priv->tx.pool, 4096, &priv->tx.phys);
> > +	priv->tx.virt = gen_pool_dma_alloc(priv->sram.tx, 4096, &priv->tx.phys);
> >  	if (!priv->tx.virt) {
> >  		dev_err(bpmp->dev, "failed to allocate from TX pool\n");
> >  		return -ENOMEM;
> >  	}
> >  
> > -	priv->rx.pool = of_gen_pool_get(bpmp->dev->of_node, "shmem", 1);
> > -	if (!priv->rx.pool) {
> > +	priv->sram.rx = of_gen_pool_get(bpmp->dev->of_node, "shmem", 1);
> > +	if (!priv->sram.rx) {
> >  		dev_err(bpmp->dev, "RX shmem pool not found\n");
> >  		err = -EPROBE_DEFER;
> >  		goto free_tx;
> >  	}
> >  
> > -	priv->rx.virt = (void __iomem *)gen_pool_dma_alloc(priv->rx.pool, 4096, &priv->rx.phys);
> > +	priv->rx.virt = gen_pool_dma_alloc(priv->sram.rx, 4096, &priv->rx.phys);
> >  	if (!priv->rx.virt) {
> >  		dev_err(bpmp->dev, "failed to allocate from RX pool\n");
> >  		err = -ENOMEM;
> >  		goto free_tx;
> >  	}
> >  
> > -	err = tegra186_bpmp_channel_init(bpmp->tx_channel, bpmp,
> > -					 bpmp->soc->channels.cpu_tx.offset);
> > -	if (err < 0)
> > -		goto free_rx;
> > +	priv->type = TEGRA_SRAM;
> >  
> > -	err = tegra186_bpmp_channel_init(bpmp->rx_channel, bpmp,
> > -					 bpmp->soc->channels.cpu_rx.offset);
> > -	if (err < 0)
> > -		goto cleanup_tx_channel;
> > +	return 0;
> >  
> > -	for (i = 0; i < bpmp->threaded.count; i++) {
> > -		unsigned int index = bpmp->soc->channels.thread.offset + i;
> > +free_tx:
> > +	gen_pool_free(priv->sram.tx, (unsigned long)priv->tx.virt, 4096);
> >  
> > -		err = tegra186_bpmp_channel_init(&bpmp->threaded_channels[i],
> > -						 bpmp, index);
> > +	return err;
> > +}
> > +
> > +static enum tegra_bpmp_mem_type tegra186_bpmp_dram_init(struct tegra_bpmp *bpmp)
> > +{
> > +	int err;
> > +	struct resource res;
> > +	struct device_node *np;
> > +	struct tegra186_bpmp *priv = bpmp->priv;
> > +
> > +	np = of_parse_phandle(bpmp->dev->of_node, "memory-region", 0);
> > +	if (!np)
> > +		return TEGRA_INVALID;
> > +
> > +	err = of_address_to_resource(np, 0, &res);
> > +	if (err) {
> > +		dev_warn(bpmp->dev,  "Parsing memory region returned: %d\n", err);
> > +		return TEGRA_INVALID;
> > +	}
> > +
> > +	if ((res.end - res.start + 1) < 0x2000) {
> 
> resource_size(), and maybe use SZ_8K instead of the literal here.
> 
> > +		dev_warn(bpmp->dev,  "DRAM region less than 0x2000 bytes\n");
> 
> Also, better to use a more human-readable string here. While at it,
> perhaps we can make this a bit more assertive, maybe something like:
> 
> 	"DRAM region must be larger than 8 KiB"
> 
> ?
> 
> > +		return TEGRA_INVALID;
> > +	}
> 
> This doesn't allow the caller to differentiate between potentially fatal
> errors and non-fatal ones. For instance, we don't want the absence of a
> "memory-region" property to be fatal (because we want to fall back to
> use SRAM in that case, or at least attempt to), but if "memory-region"
> exists, any of the subsequent errors probably should be fatal. It's
> easier to deal with that situation if you return regular error codes
> here. The !np check above could return -ENODEV, for example, as a way of
> letting the caller know that we don't have DRAM support in DT. For the
> of_address_to_resource() failure we can instead propagate the error code
> and so on.
> 
> Also, I think it'd be better to use a named constant like SZ_8K instead
> of the literal 0x2000 above. 
> 

Will look at this.

> > +
> > +	priv->tx.phys = res.start;
> > +	priv->rx.phys = res.start + 0x1000;
> 
> SZ_4K
> 
> > +
> > +	priv->tx.virt = memremap(priv->tx.phys, res.end - res.start + 1, MEMREMAP_WC);
> 
> Another case where we can use resource_size(). Might be a good idea to
> introduce a local "size" variable.
> 
> > +	if (priv->tx.virt == NULL) {
> > +		dev_warn(bpmp->dev,  "DRAM region mapping failed\n");
> > +		return TEGRA_INVALID;
> > +	}
> > +	priv->rx.virt = priv->tx.virt + 0x1000;
> 
> SZ_4K
> 
> We should probably do the same thing for the SRAM paths, but that should
> be a separate patch and can be done at another time.
> 

Yeah, this is basically the same as 4096 in the sram init but I think
hex is more readable.

> > +
> > +	return TEGRA_RMEM;
> > +}
> > +
> > +static int tegra186_bpmp_init(struct tegra_bpmp *bpmp)
> > +{
> > +	struct tegra186_bpmp *priv;
> > +	int err;
> > +
> > +	priv = devm_kzalloc(bpmp->dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	bpmp->priv = priv;
> > +	priv->parent = bpmp;
> > +
> > +	priv->type = tegra186_bpmp_dram_init(bpmp);
> > +	if (priv->type == TEGRA_INVALID) {
> > +		err = tegra186_bpmp_sram_init(bpmp);
> >  		if (err < 0)
> > -			goto cleanup_channels;
> > +			return err;
> >  	}
> 
> As I mentioned previously, I think we can move the block above into
> tegra186_bpmp_setup_channels() to make it symmetric with the teardown of
> this in tegra186_bpmp_cleanup_channels().
> 
> Thierry
> 

Peter.

Re: [PATCH v2 0/6] firmware: tegra: Add MRQ support for Tegra264.

[Peter De Schrijver]

On Wed, May 10, 2023 at 02:31:24PM +0300, Peter De Schrijver wrote:
> In Tegra264 the carveouts (GSCs) used to communicate between BPMP and
> CPU-NS may reside in DRAM. The location will be signalled using reserved
> memory node in DT. Additionally some minor updates to the HSP driver are
> done to support the new chip.
> 
> Peter De Schrijver (4):
>   dt-bindings: mailbox: tegra: Document Tegra264 HSP
>   dt-bindings: Add bindings to support DRAM MRQ GSCs
>   dt-bindings: memory-region property for tegra186-bpmp
>   firmware: tegra: bpmp: Add support for DRAM MRQ GSCs
> 
> Stefan Kristiansson (2):
>   mailbox: tegra: add support for Tegra264
>   soc: tegra: fuse: add support for Tegra264

Changes in v2:

- Added signoff messages
- Updated bindings to support DRAM MRQ GSCs
- Split out memory-region property for tegra186-bpmp
- Addressed sparse errors in bpmp-tegra186.c