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

[PATCH v4 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 support for DRAM MRQ GSCs
  dt-bindings: Add support for tegra186-bpmp DRAM MRQ GSCs
  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 into its own patch
- Addressed sparse errors in bpmp-tegra186.c

Changes in v3:

- Added #address-cells = <2> and #size-cells = <2> to
  nvidia,tegra264-bpmp-shmem binding example.

Changes in v4:

- Added lost Acked-by tags to patch 1 and 2.
- Updated topic for patch 3 to 'soc/tegra: fuse:'.
- Updated topic for patch 4 to 'dt-bindings: Add support for DRAM MRQ GSCs'.
- Updated topic for patch 5 to 'dt-bindings: Add support for tegra186-bpmp DRAM MRQ GSCs'.
- Removed unneeded include statements in patch 6.
- Renamed some functions in patch 6 for more consistent naming.
- Improved handling of void * vs void __iomem * in patch6 .

 .../firmware/nvidia,tegra186-bpmp.yaml        |  37 ++-
 .../bindings/mailbox/nvidia,tegra186-hsp.yaml |   1 +
 .../nvidia,tegra264-bpmp-shmem.yaml           |  47 ++++
 drivers/firmware/tegra/bpmp-tegra186.c        | 232 +++++++++++++-----
 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, 268 insertions(+), 75 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.yaml

-- 
2.34.1

[PATCH v4 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.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Acked-by: Thierry Reding <treding@nvidia.com>
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

[PATCH v4 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.

Acked-by: Thierry Reding <treding@nvidia.com>
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

[PATCH v4 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 v4 3/6] soc/tegra: fuse: Add support for Tegra264

[Thierry Reding]

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

On Thu, May 11, 2023 at 04:20:48PM +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(-)

Applied, thanks.

Thierry

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

[PATCH v4 4/6] dt-bindings: Add support for 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           | 47 +++++++++++++++++++
 1 file changed, 47 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..4087459c01db
--- /dev/null
+++ b/Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.yaml
@@ -0,0 +1,47 @@
+# 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 {
+       #address-cells = <2>;
+       #size-cells = <2>;
+       dram_cpu_bpmp_mail: shmem@f1be0000  {
+           compatible = "nvidia,tegra264-bpmp-shmem";
+           reg = <0x0 0xf1be0000 0x0 0x2000>;
+           no-map;
+       };
+    };
+...
-- 
2.34.1

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

[Conor Dooley]

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

On Thu, May 11, 2023 at 04:20:49PM +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>

Perhaps Krzysztof will disagree, but looks fine to me, with some minor
remarks below.
Just to note, I didn't get the cover letter & therefore didn't get the
changelog :/
I know you had a back and forth with him about that, *my* €0.02 is that
either you put the changelog in the cover & send it to everyone, or you
put it in each patch.

> ---
>  .../nvidia,tegra264-bpmp-shmem.yaml           | 47 +++++++++++++++++++
>  1 file changed, 47 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..4087459c01db
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.yaml
> @@ -0,0 +1,47 @@
> +# 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: |

You don't appear to have any formatting to preserve, so the | is not
needed.

> +  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 {
> +       #address-cells = <2>;
> +       #size-cells = <2>;

You also do not need these size/address-cells, because...

> +       dram_cpu_bpmp_mail: shmem@f1be0000  {
(nit: double space                          ^^)

> +           compatible = "nvidia,tegra264-bpmp-shmem";
> +           reg = <0x0 0xf1be0000 0x0 0x2000>;

...the 64-bit registers here are both 0x0.

With those fixed:
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> +           no-map;
> +       };
> +    };
> +...
> -- 
> 2.34.1
> 

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

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

[Krzysztof Kozlowski]

On 11/05/2023 21:21, Conor Dooley wrote:
> On Thu, May 11, 2023 at 04:20:49PM +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>
> 
> Perhaps Krzysztof will disagree, but looks fine to me, with some minor
> remarks below.
> Just to note, I didn't get the cover letter & therefore didn't get the
> changelog :/

Me neither... and in v3 I asked for it or for proper changelog in the patch

> I know you had a back and forth with him about that, *my* €0.02 is that
> either you put the changelog in the cover & send it to everyone, or you
> put it in each patch.



Best regards,
Krzysztof

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

[Thierry Reding]

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

On Thu, May 11, 2023 at 08:21:07PM +0100, Conor Dooley wrote:
> On Thu, May 11, 2023 at 04:20:49PM +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>
> 
> Perhaps Krzysztof will disagree, but looks fine to me, with some minor
> remarks below.
> Just to note, I didn't get the cover letter & therefore didn't get the
> changelog :/
> I know you had a back and forth with him about that, *my* €0.02 is that
> either you put the changelog in the cover & send it to everyone, or you
> put it in each patch.
> 
> > ---
> >  .../nvidia,tegra264-bpmp-shmem.yaml           | 47 +++++++++++++++++++
> >  1 file changed, 47 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..4087459c01db
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/reserved-memory/nvidia,tegra264-bpmp-shmem.yaml
> > @@ -0,0 +1,47 @@
> > +# 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: |
> 
> You don't appear to have any formatting to preserve, so the | is not
> needed.
> 
> > +  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 {
> > +       #address-cells = <2>;
> > +       #size-cells = <2>;
> 
> You also do not need these size/address-cells, because...
> 
> > +       dram_cpu_bpmp_mail: shmem@f1be0000  {
> (nit: double space                          ^^)
> 
> > +           compatible = "nvidia,tegra264-bpmp-shmem";
> > +           reg = <0x0 0xf1be0000 0x0 0x2000>;
> 
> ...the 64-bit registers here are both 0x0.

I think Peter had to add these explicitly because the defaults are 2 and
1, respectively, and DTC was warning about this. I suppose the "reg"
property could be adjusted to use the defaults, but on the other hand I
find that it's good if the examples match reality and we need size-cells
to be 2 on Tegra.

Either way is fine with me, though.

Thierry

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

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

[Conor Dooley]

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

On Tue, May 16, 2023 at 11:12:50AM +0200, Thierry Reding wrote:
> I think Peter had to add these explicitly because the defaults are 2 and
> 1, respectively, and DTC was warning about this. I suppose the "reg"
> property could be adjusted to use the defaults, but on the other hand I
> find that it's good if the examples match reality and we need size-cells
> to be 2 on Tegra.

Huh, caught out by an abnormal example!
If it avoids an error & matches the use-case it seems like a good idea to
leave it as-is. Here's an unqualified
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
instead of the previous qualified one.

Thanks,
Conor.

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

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

[Krzysztof Kozlowski]

On 11/05/2023 15:20, 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>
> ---

Same comments as before:
1. Missing subject prefix, so:
Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

2. I don't get why you decided to send changelog to different address -
it takes some time to find it - and to skip other maintainers...

Best regards,
Krzysztof

[PATCH v4 5/6] dt-bindings: Add support for tegra186-bpmp DRAM MRQ GSCs

[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 v4 5/6] dt-bindings: Add support for tegra186-bpmp DRAM MRQ GSCs

[Conor Dooley]

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

On Thu, May 11, 2023 at 04:20:50PM +0300, Peter De Schrijver wrote:

> +    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>;

FWIW, this fits on one line - although you've just copy-pasted what was
already there in the other example.

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>


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

Re: [PATCH v4 5/6] dt-bindings: Add support for tegra186-bpmp DRAM MRQ GSCs

[Krzysztof Kozlowski]

On 11/05/2023 15:20, Peter De Schrijver wrote:
> Add memory-region property to the tegra186-bpmp binding to support
> DRAM MRQ GSCs.

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

> 
> 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>;

I am not sure if difference with one property justifies new example...

Best regards,
Krzysztof

Re: [PATCH v4 5/6] dt-bindings: Add support for tegra186-bpmp DRAM MRQ GSCs

[Thierry Reding]

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

On Fri, May 12, 2023 at 08:45:22AM +0200, Krzysztof Kozlowski wrote:
> On 11/05/2023 15:20, Peter De Schrijver wrote:
> > Add memory-region property to the tegra186-bpmp binding to support
> > DRAM MRQ GSCs.
> 
> Use subject prefixes matching the subsystem (which you can get for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching).
> 
> > 
> > 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>;
> 
> I am not sure if difference with one property justifies new example...

It makes sense in this case, in my opinion, because both memory-region
and shmem properties are mutually exclusive, so this is a good way to
make sure both validation paths are tested.

Thierry

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

[PATCH v4 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 | 232 ++++++++++++++++++-------
 drivers/firmware/tegra/bpmp.c          |   4 +-
 2 files changed, 168 insertions(+), 68 deletions(-)

diff --git a/drivers/firmware/tegra/bpmp-tegra186.c b/drivers/firmware/tegra/bpmp-tegra186.c
index 2e26199041cd..74575c9f0014 100644
--- a/drivers/firmware/tegra/bpmp-tegra186.c
+++ b/drivers/firmware/tegra/bpmp-tegra186.c
@@ -4,7 +4,9 @@
  */
 
 #include <linux/genalloc.h>
+#include <linux/io.h>
 #include <linux/mailbox_client.h>
+#include <linux/of_address.h>
 #include <linux/platform_device.h>
 
 #include <soc/tegra/bpmp.h>
@@ -13,12 +15,21 @@
 
 #include "bpmp-private.h"
 
+enum tegra_bpmp_mem_type { TEGRA_INVALID, TEGRA_SRAM, TEGRA_DRAM };
+
 struct tegra186_bpmp {
 	struct tegra_bpmp *parent;
 
 	struct {
-		struct gen_pool *pool;
-		void __iomem *virt;
+		union {
+			struct {
+				void __iomem *virt;
+				struct gen_pool *pool;
+			} sram;
+			struct {
+				void *virt;
+			} dram;
+		};
 		dma_addr_t phys;
 	} tx, rx;
 
@@ -26,6 +37,8 @@ struct tegra186_bpmp {
 		struct mbox_client client;
 		struct mbox_chan *channel;
 	} mbox;
+
+	enum tegra_bpmp_mem_type type;
 };
 
 static inline struct tegra_bpmp *
@@ -118,8 +131,17 @@ 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);
+	if (priv->type == TEGRA_SRAM) {
+		iosys_map_set_vaddr_iomem(&rx, priv->rx.sram.virt + offset);
+		iosys_map_set_vaddr_iomem(&tx, priv->tx.sram.virt + offset);
+	} else if (priv->type == TEGRA_DRAM) {
+		iosys_map_set_vaddr(&rx, priv->rx.dram.virt + offset);
+		iosys_map_set_vaddr(&tx, priv->tx.dram.virt + offset);
+	} else {
+		dev_err(bpmp->dev, "Inconsistent state %d of priv->type detected in %s\n",
+				priv->type, __func__);
+		return -EINVAL;
+	}
 
 	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,54 +180,135 @@ 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_teardown_channels(struct tegra_bpmp *bpmp)
 {
-	struct tegra186_bpmp *priv;
-	unsigned int i;
-	int err;
+	size_t i;
+	struct tegra186_bpmp *priv = bpmp->priv;
 
-	priv = devm_kzalloc(bpmp->dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
+	for (i = 0; i < bpmp->threaded.count; i++) {
+		if (!bpmp->threaded_channels[i].bpmp)
+			continue;
 
-	bpmp->priv = priv;
-	priv->parent = bpmp;
+		tegra186_bpmp_channel_cleanup(&bpmp->threaded_channels[i]);
+	}
+
+	tegra186_bpmp_channel_cleanup(bpmp->rx_channel);
+	tegra186_bpmp_channel_cleanup(bpmp->tx_channel);
 
-	priv->tx.pool = of_gen_pool_get(bpmp->dev->of_node, "shmem", 0);
-	if (!priv->tx.pool) {
+	if (priv->type == TEGRA_SRAM) {
+		gen_pool_free(priv->tx.sram.pool, (unsigned long)priv->tx.sram.virt, 4096);
+		gen_pool_free(priv->rx.sram.pool, (unsigned long)priv->rx.sram.virt, 4096);
+	} else if (priv->type == TEGRA_DRAM) {
+		memunmap(priv->tx.dram.virt);
+	}
+}
+
+static int tegra186_bpmp_sram_init(struct tegra_bpmp *bpmp)
+{
+	int err;
+	struct tegra186_bpmp *priv = bpmp->priv;
+
+	priv->tx.sram.pool = of_gen_pool_get(bpmp->dev->of_node, "shmem", 0);
+	if (!priv->tx.sram.pool) {
 		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);
-	if (!priv->tx.virt) {
+	priv->tx.sram.virt = (void __iomem *)gen_pool_dma_alloc(priv->tx.sram.pool, 4096,
+								&priv->tx.phys);
+	if (!priv->tx.sram.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->rx.sram.pool = of_gen_pool_get(bpmp->dev->of_node, "shmem", 1);
+	if (!priv->rx.sram.pool) {
 		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);
-	if (!priv->rx.virt) {
+	priv->rx.sram.virt = (void __iomem *)gen_pool_dma_alloc(priv->rx.sram.pool, 4096,
+								&priv->rx.phys);
+	if (!priv->rx.sram.virt) {
 		dev_err(bpmp->dev, "failed to allocate from RX pool\n");
 		err = -ENOMEM;
 		goto free_tx;
 	}
 
+	priv->type = TEGRA_SRAM;
+
+	return 0;
+
+free_tx:
+	gen_pool_free(priv->tx.sram.pool, (unsigned long)priv->tx.sram.virt, 4096);
+
+	return err;
+}
+
+static int tegra186_bpmp_dram_init(struct tegra_bpmp *bpmp)
+{
+	int err;
+	resource_size_t size;
+	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 -ENOENT;
+
+	err = of_address_to_resource(np, 0, &res);
+	if (err) {
+		dev_warn(bpmp->dev,  "Parsing memory region returned: %d\n", err);
+		return -EINVAL;
+	}
+
+	size = resource_size(&res);
+	if (size < SZ_8K) {
+		dev_warn(bpmp->dev,  "DRAM region must be larger than 8 KiB\n");
+		return -EINVAL;
+	}
+
+	priv->tx.phys = res.start;
+	priv->rx.phys = res.start + SZ_4K;
+
+	priv->tx.dram.virt = memremap(priv->tx.phys, size, MEMREMAP_WC);
+	if (priv->tx.dram.virt == NULL) {
+		dev_warn(bpmp->dev,  "DRAM region mapping failed\n");
+		return -EINVAL;
+	}
+	priv->rx.dram.virt = priv->tx.dram.virt + SZ_4K;
+	priv->type = TEGRA_DRAM;
+
+	return 0;
+}
+
+static int tegra186_bpmp_setup_channels(struct tegra_bpmp *bpmp)
+{
+	int err;
+	size_t i;
+	struct tegra186_bpmp *priv = bpmp->priv;
+
+	priv->type = TEGRA_INVALID;
+
+	err = tegra186_bpmp_dram_init(bpmp);
+	if (err == -ENOENT)
+		err = tegra186_bpmp_sram_init(bpmp);
+	if (err < 0)
+		return err;
+
 	err = tegra186_bpmp_channel_init(bpmp->tx_channel, bpmp,
 					 bpmp->soc->channels.cpu_tx.offset);
 	if (err < 0)
-		goto free_rx;
+		return err;
 
 	err = tegra186_bpmp_channel_init(bpmp->rx_channel, bpmp,
 					 bpmp->soc->channels.cpu_rx.offset);
-	if (err < 0)
-		goto cleanup_tx_channel;
+	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;
@@ -213,9 +316,42 @@ static int tegra186_bpmp_init(struct tegra_bpmp *bpmp)
 		err = tegra186_bpmp_channel_init(&bpmp->threaded_channels[i],
 						 bpmp, index);
 		if (err < 0)
-			goto cleanup_channels;
+			break;
 	}
 
+	if (err < 0)
+		tegra186_bpmp_teardown_channels(bpmp);
+
+	return err;
+}
+
+static void tegra186_bpmp_reset_channels(struct tegra_bpmp *bpmp)
+{
+	size_t 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_init(struct tegra_bpmp *bpmp)
+{
+	int err;
+	struct tegra186_bpmp *priv;
+
+	priv = devm_kzalloc(bpmp->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	bpmp->priv = priv;
+	priv->parent = bpmp;
+
+	err = tegra186_bpmp_setup_channels(bpmp);
+	if (err < 0)
+		return err;
+
 	/* mbox registration */
 	priv->mbox.client.dev = bpmp->dev;
 	priv->mbox.client.rx_callback = mbox_handle_rx;
@@ -226,63 +362,27 @@ 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_teardown_channels(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_teardown_channels(bpmp);
 }
 
 static int tegra186_bpmp_resume(struct tegra_bpmp *bpmp)
 {
-	unsigned int i;
-
-	/* reset message channels */
-	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;
 }
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 v4 6/6] firmware: tegra: bpmp: Add support for DRAM MRQ GSCs

[Thierry Reding]

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

On Thu, May 11, 2023 at 04:20:51PM +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 | 232 ++++++++++++++++++-------
>  drivers/firmware/tegra/bpmp.c          |   4 +-
>  2 files changed, 168 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/firmware/tegra/bpmp-tegra186.c b/drivers/firmware/tegra/bpmp-tegra186.c
> index 2e26199041cd..74575c9f0014 100644
> --- a/drivers/firmware/tegra/bpmp-tegra186.c
> +++ b/drivers/firmware/tegra/bpmp-tegra186.c
> @@ -4,7 +4,9 @@
>   */
>  
>  #include <linux/genalloc.h>
> +#include <linux/io.h>
>  #include <linux/mailbox_client.h>
> +#include <linux/of_address.h>
>  #include <linux/platform_device.h>
>  
>  #include <soc/tegra/bpmp.h>
> @@ -13,12 +15,21 @@
>  
>  #include "bpmp-private.h"
>  
> +enum tegra_bpmp_mem_type { TEGRA_INVALID, TEGRA_SRAM, TEGRA_DRAM };

Still not convinced about this one.

> +
>  struct tegra186_bpmp {
>  	struct tegra_bpmp *parent;
>  
>  	struct {
> -		struct gen_pool *pool;
> -		void __iomem *virt;
> +		union {
> +			struct {
> +				void __iomem *virt;
> +				struct gen_pool *pool;
> +			} sram;
> +			struct {
> +				void *virt;
> +			} dram;
> +		};

The drawback of these unions is that they can lead to ambiguity, so you
need the tegra_bpmp_mem_type enum to differentiate between the two.

If you change this to something like:

	struct {
		struct gen_pool *pool;
		void __iomem *sram;
		void *dram;
		dma_addr_t phys;
	} tx, rx;

you eliminate all ambiguity because you can either have pool and sram
set, or you can have dram set, and depending on which are set you know
which type of memory you're dealing with.

Plus you then don't need the extra enum to differentiate between them.

Another alternative would be to use something like:

	union {
		void __iomem *sram;
		void *dram;
	} virt;

if you want to avoid the extra 8 bytes. But to be honest, I wouldn't
bother.

>  		dma_addr_t phys;
>  	} tx, rx;
>  
> @@ -26,6 +37,8 @@ struct tegra186_bpmp {
>  		struct mbox_client client;
>  		struct mbox_chan *channel;
>  	} mbox;
> +
> +	enum tegra_bpmp_mem_type type;
>  };
>  
>  static inline struct tegra_bpmp *
> @@ -118,8 +131,17 @@ 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);
> +	if (priv->type == TEGRA_SRAM) {
> +		iosys_map_set_vaddr_iomem(&rx, priv->rx.sram.virt + offset);
> +		iosys_map_set_vaddr_iomem(&tx, priv->tx.sram.virt + offset);
> +	} else if (priv->type == TEGRA_DRAM) {
> +		iosys_map_set_vaddr(&rx, priv->rx.dram.virt + offset);
> +		iosys_map_set_vaddr(&tx, priv->tx.dram.virt + offset);
> +	} else {
> +		dev_err(bpmp->dev, "Inconsistent state %d of priv->type detected in %s\n",
> +				priv->type, __func__);
> +		return -EINVAL;
> +	}

With an enum you need to do this because theoretically it could happen.
But practically it will never happen and you can just rely on the pool
variable, for example, to distinguish.

Thierry

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

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

[Peter De Schrijver]

On Tue, May 16, 2023 at 11:35:24AM +0200, Thierry Reding wrote:
> On Thu, May 11, 2023 at 04:20:51PM +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 | 232 ++++++++++++++++++-------
> >  drivers/firmware/tegra/bpmp.c          |   4 +-
> >  2 files changed, 168 insertions(+), 68 deletions(-)
> > 
> > diff --git a/drivers/firmware/tegra/bpmp-tegra186.c b/drivers/firmware/tegra/bpmp-tegra186.c
> > index 2e26199041cd..74575c9f0014 100644
> > --- a/drivers/firmware/tegra/bpmp-tegra186.c
> > +++ b/drivers/firmware/tegra/bpmp-tegra186.c
> > @@ -4,7 +4,9 @@
> >   */
> >  
> >  #include <linux/genalloc.h>
> > +#include <linux/io.h>
> >  #include <linux/mailbox_client.h>
> > +#include <linux/of_address.h>
> >  #include <linux/platform_device.h>
> >  
> >  #include <soc/tegra/bpmp.h>
> > @@ -13,12 +15,21 @@
> >  
> >  #include "bpmp-private.h"
> >  
> > +enum tegra_bpmp_mem_type { TEGRA_INVALID, TEGRA_SRAM, TEGRA_DRAM };
> 
> Still not convinced about this one.
> 
> > +
> >  struct tegra186_bpmp {
> >  	struct tegra_bpmp *parent;
> >  
> >  	struct {
> > -		struct gen_pool *pool;
> > -		void __iomem *virt;
> > +		union {
> > +			struct {
> > +				void __iomem *virt;
> > +				struct gen_pool *pool;
> > +			} sram;
> > +			struct {
> > +				void *virt;
> > +			} dram;
> > +		};
> 
> The drawback of these unions is that they can lead to ambiguity, so you
> need the tegra_bpmp_mem_type enum to differentiate between the two.
> 

No, on the contrary, now it's clear you can either have void __iomem *
and struct gen_pool * or void *virt but not both.

> If you change this to something like:
> 
> 	struct {
> 		struct gen_pool *pool;
> 		void __iomem *sram;
> 		void *dram;
> 		dma_addr_t phys;
> 	} tx, rx;
> 
> you eliminate all ambiguity because you can either have pool and sram
> set, or you can have dram set, and depending on which are set you know
> which type of memory you're dealing with.
> 

No. You just add ambiguity. It's not clear from looking at the data
structure which fields are valid for which case.

> Plus you then don't need the extra enum to differentiate between them.
> 

That is a limitation of the C programming language yes..
What you would really want is something like this:

struct sram {
	void __iomem *virt;
	struct gen_pool *pool;
};

struct dram {
	void *virt;
};

enum half_channel {
	sram(struct sram),
	dram(struct dram),
};

struct tegra186_bpmp {
	struct tegra_bpmp *parent;
	...
	enum half_channel tx,rx;
}

> Another alternative would be to use something like:
> 
> 	union {
> 		void __iomem *sram;
> 		void *dram;
> 	} virt;
> 
> if you want to avoid the extra 8 bytes. But to be honest, I wouldn't
> bother.
> 
> >  		dma_addr_t phys;
> >  	} tx, rx;
> >  
> > @@ -26,6 +37,8 @@ struct tegra186_bpmp {
> >  		struct mbox_client client;
> >  		struct mbox_chan *channel;
> >  	} mbox;
> > +
> > +	enum tegra_bpmp_mem_type type;
> >  };
> >  
> >  static inline struct tegra_bpmp *
> > @@ -118,8 +131,17 @@ 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);
> > +	if (priv->type == TEGRA_SRAM) {
> > +		iosys_map_set_vaddr_iomem(&rx, priv->rx.sram.virt + offset);
> > +		iosys_map_set_vaddr_iomem(&tx, priv->tx.sram.virt + offset);
> > +	} else if (priv->type == TEGRA_DRAM) {
> > +		iosys_map_set_vaddr(&rx, priv->rx.dram.virt + offset);
> > +		iosys_map_set_vaddr(&tx, priv->tx.dram.virt + offset);
> > +	} else {
> > +		dev_err(bpmp->dev, "Inconsistent state %d of priv->type detected in %s\n",
> > +				priv->type, __func__);
> > +		return -EINVAL;
> > +	}
> 
> With an enum you need to do this because theoretically it could happen.
> But practically it will never happen and you can just rely on the pool
> variable, for example, to distinguish.
> 
> Thierry

Peter.

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

[Thierry Reding]

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

On Tue, May 16, 2023 at 12:55:03PM +0300, Peter De Schrijver wrote:
> On Tue, May 16, 2023 at 11:35:24AM +0200, Thierry Reding wrote:
> > On Thu, May 11, 2023 at 04:20:51PM +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 | 232 ++++++++++++++++++-------
> > >  drivers/firmware/tegra/bpmp.c          |   4 +-
> > >  2 files changed, 168 insertions(+), 68 deletions(-)
> > > 
> > > diff --git a/drivers/firmware/tegra/bpmp-tegra186.c b/drivers/firmware/tegra/bpmp-tegra186.c
> > > index 2e26199041cd..74575c9f0014 100644
> > > --- a/drivers/firmware/tegra/bpmp-tegra186.c
> > > +++ b/drivers/firmware/tegra/bpmp-tegra186.c
> > > @@ -4,7 +4,9 @@
> > >   */
> > >  
> > >  #include <linux/genalloc.h>
> > > +#include <linux/io.h>
> > >  #include <linux/mailbox_client.h>
> > > +#include <linux/of_address.h>
> > >  #include <linux/platform_device.h>
> > >  
> > >  #include <soc/tegra/bpmp.h>
> > > @@ -13,12 +15,21 @@
> > >  
> > >  #include "bpmp-private.h"
> > >  
> > > +enum tegra_bpmp_mem_type { TEGRA_INVALID, TEGRA_SRAM, TEGRA_DRAM };
> > 
> > Still not convinced about this one.
> > 
> > > +
> > >  struct tegra186_bpmp {
> > >  	struct tegra_bpmp *parent;
> > >  
> > >  	struct {
> > > -		struct gen_pool *pool;
> > > -		void __iomem *virt;
> > > +		union {
> > > +			struct {
> > > +				void __iomem *virt;
> > > +				struct gen_pool *pool;
> > > +			} sram;
> > > +			struct {
> > > +				void *virt;
> > > +			} dram;
> > > +		};
> > 
> > The drawback of these unions is that they can lead to ambiguity, so you
> > need the tegra_bpmp_mem_type enum to differentiate between the two.
> > 
> 
> No, on the contrary, now it's clear you can either have void __iomem *
> and struct gen_pool * or void *virt but not both.

No, it's not clear. You can have one part of your driver write the
sram.virt field and another read dram.virt and they'll end up pointing
at the same memory location but with different meaning. That's why you
need to introduce the enumeration in order to specify which one of the
two you want to pick.

And that's exactly where you start introducing the potential for
inconsistency: now you need to be extra careful that the enumeration and
the unions are set correctly. You effectively have two sources of truth
and they don't necessarily match. You can also end up (at least
theoretically) with the invalid value, so you need an extra check for
that too.

You can avoid all of those inconsistencies if you reduce this to one
source of truth, namely the pointers that you're going to use.

Your variant would be slightly better if you omitted the invalid value
because then you could still have an internal inconsistency, but the
likelihood is much reduced.

> > If you change this to something like:
> > 
> > 	struct {
> > 		struct gen_pool *pool;
> > 		void __iomem *sram;
> > 		void *dram;
> > 		dma_addr_t phys;
> > 	} tx, rx;
> > 
> > you eliminate all ambiguity because you can either have pool and sram
> > set, or you can have dram set, and depending on which are set you know
> > which type of memory you're dealing with.
> > 
> 
> No. You just add ambiguity. It's not clear from looking at the data
> structure which fields are valid for which case.

That's easily fixed by adding comments explaining what you use them for.
But the code should make that pretty clear already.

Thierry

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

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

[Peter De Schrijver]

On Wed, Jun 07, 2023 at 05:57:39PM +0200, Thierry Reding wrote:
> On Tue, May 16, 2023 at 12:55:03PM +0300, Peter De Schrijver wrote:
> > On Tue, May 16, 2023 at 11:35:24AM +0200, Thierry Reding wrote:
> > > On Thu, May 11, 2023 at 04:20:51PM +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 | 232 ++++++++++++++++++-------
> > > >  drivers/firmware/tegra/bpmp.c          |   4 +-
> > > >  2 files changed, 168 insertions(+), 68 deletions(-)
> > > > 
> > > > diff --git a/drivers/firmware/tegra/bpmp-tegra186.c b/drivers/firmware/tegra/bpmp-tegra186.c
> > > > index 2e26199041cd..74575c9f0014 100644
> > > > --- a/drivers/firmware/tegra/bpmp-tegra186.c
> > > > +++ b/drivers/firmware/tegra/bpmp-tegra186.c
> > > > @@ -4,7 +4,9 @@
> > > >   */
> > > >  
> > > >  #include <linux/genalloc.h>
> > > > +#include <linux/io.h>
> > > >  #include <linux/mailbox_client.h>
> > > > +#include <linux/of_address.h>
> > > >  #include <linux/platform_device.h>
> > > >  
> > > >  #include <soc/tegra/bpmp.h>
> > > > @@ -13,12 +15,21 @@
> > > >  
> > > >  #include "bpmp-private.h"
> > > >  
> > > > +enum tegra_bpmp_mem_type { TEGRA_INVALID, TEGRA_SRAM, TEGRA_DRAM };
> > > 
> > > Still not convinced about this one.
> > > 
> > > > +
> > > >  struct tegra186_bpmp {
> > > >  	struct tegra_bpmp *parent;
> > > >  
> > > >  	struct {
> > > > -		struct gen_pool *pool;
> > > > -		void __iomem *virt;
> > > > +		union {
> > > > +			struct {
> > > > +				void __iomem *virt;
> > > > +				struct gen_pool *pool;
> > > > +			} sram;
> > > > +			struct {
> > > > +				void *virt;
> > > > +			} dram;
> > > > +		};
> > > 
> > > The drawback of these unions is that they can lead to ambiguity, so you
> > > need the tegra_bpmp_mem_type enum to differentiate between the two.
> > > 
> > 
> > No, on the contrary, now it's clear you can either have void __iomem *
> > and struct gen_pool * or void *virt but not both.
> 
> No, it's not clear. You can have one part of your driver write the
> sram.virt field and another read dram.virt and they'll end up pointing
> at the same memory location but with different meaning. That's why you

No. You can't the union in combination with the discriminating enum
tells you you should only either sram or dram.

> need to introduce the enumeration in order to specify which one of the
> two you want to pick.
> 
> And that's exactly where you start introducing the potential for
> inconsistency: now you need to be extra careful that the enumeration and
> the unions are set correctly. You effectively have two sources of truth
> and they don't necessarily match. You can also end up (at least
> theoretically) with the invalid value, so you need an extra check for
> that too.
> 
> You can avoid all of those inconsistencies if you reduce this to one
> source of truth, namely the pointers that you're going to use.
> 

I don't think pointers should be used as a discriminator.

> Your variant would be slightly better if you omitted the invalid value
> because then you could still have an internal inconsistency, but the
> likelihood is much reduced.
> 
> > > If you change this to something like:
> > > 
> > > 	struct {
> > > 		struct gen_pool *pool;
> > > 		void __iomem *sram;
> > > 		void *dram;
> > > 		dma_addr_t phys;
> > > 	} tx, rx;
> > > 
> > > you eliminate all ambiguity because you can either have pool and sram
> > > set, or you can have dram set, and depending on which are set you know
> > > which type of memory you're dealing with.
> > > 
> > 
> > No. You just add ambiguity. It's not clear from looking at the data
> > structure which fields are valid for which case.
> 
> That's easily fixed by adding comments explaining what you use them for.
> But the code should make that pretty clear already.

No. The code should follow the data structures, that's why unions exist.

Peter.

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

[Thierry Reding]

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

On Thu, Jun 08, 2023 at 12:06:58PM +0300, Peter De Schrijver wrote:
> On Wed, Jun 07, 2023 at 05:57:39PM +0200, Thierry Reding wrote:
> > On Tue, May 16, 2023 at 12:55:03PM +0300, Peter De Schrijver wrote:
> > > On Tue, May 16, 2023 at 11:35:24AM +0200, Thierry Reding wrote:
> > > > On Thu, May 11, 2023 at 04:20:51PM +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 | 232 ++++++++++++++++++-------
> > > > >  drivers/firmware/tegra/bpmp.c          |   4 +-
> > > > >  2 files changed, 168 insertions(+), 68 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/firmware/tegra/bpmp-tegra186.c b/drivers/firmware/tegra/bpmp-tegra186.c
> > > > > index 2e26199041cd..74575c9f0014 100644
> > > > > --- a/drivers/firmware/tegra/bpmp-tegra186.c
> > > > > +++ b/drivers/firmware/tegra/bpmp-tegra186.c
> > > > > @@ -4,7 +4,9 @@
> > > > >   */
> > > > >  
> > > > >  #include <linux/genalloc.h>
> > > > > +#include <linux/io.h>
> > > > >  #include <linux/mailbox_client.h>
> > > > > +#include <linux/of_address.h>
> > > > >  #include <linux/platform_device.h>
> > > > >  
> > > > >  #include <soc/tegra/bpmp.h>
> > > > > @@ -13,12 +15,21 @@
> > > > >  
> > > > >  #include "bpmp-private.h"
> > > > >  
> > > > > +enum tegra_bpmp_mem_type { TEGRA_INVALID, TEGRA_SRAM, TEGRA_DRAM };
> > > > 
> > > > Still not convinced about this one.
> > > > 
> > > > > +
> > > > >  struct tegra186_bpmp {
> > > > >  	struct tegra_bpmp *parent;
> > > > >  
> > > > >  	struct {
> > > > > -		struct gen_pool *pool;
> > > > > -		void __iomem *virt;
> > > > > +		union {
> > > > > +			struct {
> > > > > +				void __iomem *virt;
> > > > > +				struct gen_pool *pool;
> > > > > +			} sram;
> > > > > +			struct {
> > > > > +				void *virt;
> > > > > +			} dram;
> > > > > +		};
> > > > 
> > > > The drawback of these unions is that they can lead to ambiguity, so you
> > > > need the tegra_bpmp_mem_type enum to differentiate between the two.
> > > > 
> > > 
> > > No, on the contrary, now it's clear you can either have void __iomem *
> > > and struct gen_pool * or void *virt but not both.
> > 
> > No, it's not clear. You can have one part of your driver write the
> > sram.virt field and another read dram.virt and they'll end up pointing
> > at the same memory location but with different meaning. That's why you
> 
> No. You can't the union in combination with the discriminating enum
> tells you you should only either sram or dram.

That's precisely my point. This only works in conjunction with the
additional enum and it unnecessarily complicates things.

> > need to introduce the enumeration in order to specify which one of the
> > two you want to pick.
> > 
> > And that's exactly where you start introducing the potential for
> > inconsistency: now you need to be extra careful that the enumeration and
> > the unions are set correctly. You effectively have two sources of truth
> > and they don't necessarily match. You can also end up (at least
> > theoretically) with the invalid value, so you need an extra check for
> > that too.
> > 
> > You can avoid all of those inconsistencies if you reduce this to one
> > source of truth, namely the pointers that you're going to use.
> > 
> 
> I don't think pointers should be used as a discriminator.

I don't think we should extra data to discriminate when we can already
discriminate using the existing data.

Thierry

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

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

[Peter De Schrijver]

On Wed, Jun 07, 2023 at 05:57:39PM +0200, Thierry Reding wrote:
> > No, on the contrary, now it's clear you can either have void __iomem *
> > and struct gen_pool * or void *virt but not both.
> 
> No, it's not clear. You can have one part of your driver write the
> sram.virt field and another read dram.virt and they'll end up pointing
> at the same memory location but with different meaning. That's why you
> need to introduce the enumeration in order to specify which one of the
> two you want to pick.
> 
> And that's exactly where you start introducing the potential for
> inconsistency: now you need to be extra careful that the enumeration and
> the unions are set correctly. You effectively have two sources of truth
> and they don't necessarily match. You can also end up (at least
> theoretically) with the invalid value, so you need an extra check for
> that too.
> 
> You can avoid all of those inconsistencies if you reduce this to one
> source of truth, namely the pointers that you're going to use.
> 

There are 4 possible states for these pointers:
both NULL
both non-NULL
sram pointer NULL, dram pointer non-NULL
dram pointer NULL, sram pointer non-NULL

So how is this one source of truth?

Peter.

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

[Thierry Reding]

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

On Thu, Jun 08, 2023 at 02:22:23PM +0300, Peter De Schrijver wrote:
> On Wed, Jun 07, 2023 at 05:57:39PM +0200, Thierry Reding wrote:
> > > No, on the contrary, now it's clear you can either have void __iomem *
> > > and struct gen_pool * or void *virt but not both.
> > 
> > No, it's not clear. You can have one part of your driver write the
> > sram.virt field and another read dram.virt and they'll end up pointing
> > at the same memory location but with different meaning. That's why you
> > need to introduce the enumeration in order to specify which one of the
> > two you want to pick.
> > 
> > And that's exactly where you start introducing the potential for
> > inconsistency: now you need to be extra careful that the enumeration and
> > the unions are set correctly. You effectively have two sources of truth
> > and they don't necessarily match. You can also end up (at least
> > theoretically) with the invalid value, so you need an extra check for
> > that too.
> > 
> > You can avoid all of those inconsistencies if you reduce this to one
> > source of truth, namely the pointers that you're going to use.
> > 
> 
> There are 4 possible states for these pointers:
> both NULL
> both non-NULL
> sram pointer NULL, dram pointer non-NULL
> dram pointer NULL, sram pointer non-NULL
> 
> So how is this one source of truth?

If you add a tristate enum you turn this into 6 possible states, how is
that any better?

My point is that the pointer contents are enough to determine which mode
we use. In either case we have to make sure that the state is consistent
so we can't end up with both non-NULL. The difference is that without
the enum we only have to make sure that the pointers are correct. With
the additional enum we also need to make sure that that value is
consistent with the values that we store in the pointers.

Anyway, time is running out, so I'll just apply the series and "fix"
this up myself.

Thierry

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