Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.117.161 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.117.161; helo=mail.nvidia.com; pr=C
Date:   Tue, 16 May 2023 12:55:03 +0300
From:   Peter De Schrijver <pdeschrijver@nvidia.com>
To:     Thierry Reding <thierry.reding@gmail.com>
CC:     <jonathanh@nvidia.com>, <mperttunen@nvidia.com>,
        <sudeep.holla@arm.com>, <talho@nvidia.com>, <robh@kernel.org>,
        <linux-tegra@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <stefank@nvidia.com>, <krzysztof.kozlowski@linaro.org>
Subject: Re: [PATCH v4 6/6] firmware: tegra: bpmp: Add support for DRAM MRQ
 GSCs
Message-ID: <ZGNS9w+i9Y9gpz6R@44189d9-lcedt>
References: <20230511132048.1122075-1-pdeschrijver@nvidia.com>
 <20230511132048.1122075-7-pdeschrijver@nvidia.com>
 <ZGNOXO3rRtFx_12R@orome>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZGNOXO3rRtFx_12R@orome>
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 May 2023 09:55:14.6034
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: b7d28380-073f-42fe-b5f2-08db55f3a5a3
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.117.161];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: DS1PEPF0000E64A.namprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR12MB4469
X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.