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@xxxxxxxxxx>
> > ---
> >  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.