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

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

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.

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

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

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.

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