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
Implement support for DRAM MRQ GSCs.
Signed-off-by: Peter De Schrijver <[email protected]>
---
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
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 <[email protected]>
Acked-by: Thierry Reding <[email protected]>
Signed-off-by: Peter De Schrijver <[email protected]>
---
.../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
From: Stefan Kristiansson <[email protected]>
Add support for Tegra264 to the fuse handling code.
Signed-off-by: Stefan Kristiansson <[email protected]>
Signed-off-by: Peter De Schrijver <[email protected]>
---
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
On Thu, May 11, 2023 at 04:20:48PM +0300, Peter De Schrijver wrote:
> From: Stefan Kristiansson <[email protected]>
>
> Add support for Tegra264 to the fuse handling code.
>
> Signed-off-by: Stefan Kristiansson <[email protected]>
> Signed-off-by: Peter De Schrijver <[email protected]>
> ---
> drivers/soc/tegra/fuse/tegra-apbmisc.c | 3 ++-
> include/soc/tegra/fuse.h | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
Applied, thanks.
Thierry
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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.
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 <[email protected]>
> > > ---
> > > 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
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 <[email protected]>
> > > > ---
> > > > 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.
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.
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 <[email protected]>
> > > > > ---
> > > > > 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
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