Message-ID: <ZFyhcYyV0qCZqLzE@44189d9-lcedt>
Date:   Thu, 11 May 2023 11:04:01 +0300
From:   Peter De Schrijver <pdeschrijver@...dia.com>
To:     Thierry Reding <thierry.reding@...il.com>
CC:     <jonathanh@...dia.com>, <mperttunen@...dia.com>,
        <sudeep.holla@....com>, <talho@...dia.com>, <robh@...nel.org>,
        <linux-tegra@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <stefank@...dia.com>
Subject: Re: [PATCH v2 6/6] firmware: tegra: bpmp: Add support for DRAM MRQ
 GSCs

On Wed, May 10, 2023 at 04:33:35PM +0200, Thierry Reding wrote:
> On Wed, May 10, 2023 at 02:31:36PM +0300, Peter De Schrijver wrote:
> > Implement support for DRAM MRQ GSCs.
> > 
> > Signed-off-by: Peter De Schrijver <pdeschrijver@...dia.com>
> > ---
> >  drivers/firmware/tegra/bpmp-tegra186.c | 214 +++++++++++++++++--------
> >  drivers/firmware/tegra/bpmp.c          |   4 +-
> >  2 files changed, 153 insertions(+), 65 deletions(-)
> > 
> > diff --git a/drivers/firmware/tegra/bpmp-tegra186.c b/drivers/firmware/tegra/bpmp-tegra186.c
> > index 2e26199041cd..43e2563575fc 100644
> > --- a/drivers/firmware/tegra/bpmp-tegra186.c
> > +++ b/drivers/firmware/tegra/bpmp-tegra186.c
> > @@ -4,8 +4,11 @@
> >   */
> >  
> >  #include <linux/genalloc.h>
> > +#include <linux/io.h>
> >  #include <linux/mailbox_client.h>
> > +#include <linux/of_address.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/range.h>
> 
> Why do we need range.h?
> 

We probably don't need this indeed.

> >  
> >  #include <soc/tegra/bpmp.h>
> >  #include <soc/tegra/bpmp-abi.h>
> > @@ -13,12 +16,13 @@
> >  
> >  #include "bpmp-private.h"
> >  
> > +enum tegra_bpmp_mem_type { TEGRA_INVALID, TEGRA_SRAM, TEGRA_RMEM };
> > +
> 
> This is a strange construct. We can already use the pool pointer to
> determine which type of memory is being used. Your usage of this leads
> to very unintuitive code when you're error checking, etc. and prevents
> you from propagating proper error codes.
> 

No? How would indicate SRAM and DRAM are mutually exclusive?

> >  struct tegra186_bpmp {
> >  	struct tegra_bpmp *parent;
> >  
> >  	struct {
> > -		struct gen_pool *pool;
> > -		void __iomem *virt;
> > +		void *virt;
> 
> I think what we really need is a union that contains both an __iomem
> annotated pointer and a regular one.

And then add a struct gen_pool * in the union and use the enum as a tag.

> 
> >  		dma_addr_t phys;
> >  	} tx, rx;
> >  
> > @@ -26,6 +30,12 @@ struct tegra186_bpmp {
> >  		struct mbox_client client;
> >  		struct mbox_chan *channel;
> >  	} mbox;
> > +
> > +	struct {
> > +		struct gen_pool *tx, *rx;
> > +	} sram;
> 
> Please keep this in the tx/rx structure. This would perhaps be useful if
> there was an equivalent "dram" structure, but as it is there's no
> advantage in keeping this separate from the other memory-related fields.
> 
> > +
> > +	enum tegra_bpmp_mem_type type;
> >  };
> >  
> >  static inline struct tegra_bpmp *
> > @@ -118,8 +128,8 @@ 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);
> > +	iosys_map_set_vaddr_iomem(&rx, (void __iomem *)priv->rx.virt + offset);
> > +	iosys_map_set_vaddr_iomem(&tx, (void __iomem *)priv->tx.virt + offset);
> 
> This completely defies the purpose of using the iosys_map helpers. What
> you really want to do is check if we're using SRAM and use the _iomem
> variant, otherwise, use the plain one, something like:
> 
> 	if (priv->rx.pool)
> 		iosys_map_set_vaddr_iomem(&rx, priv->rx.sram + offset);

Even the current code does not have an rx.sram field, so I don't quite
understand what you mean here.

> 	else
> 		iosys_map_set_vaddr(&rx, priv->rx.dram + offset);
> 
> And repeat that for TX. I suppose you could also do the iosys_map
> assignment for both in the same blocks above since we don't support
> mixing SRAM and DRAM modes.
> 

Currently the code does:

iosys_map_set_vaddr_iomem(&rx, priv->rx.virt + offset);

and priv->rx.virt is initialized by:

priv->rx.virt = (void __iomem *)gen_pool_dma_alloc();

so we cast this today as well?

> >  
> >  	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,64 +168,171 @@ 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_channel_deinit(struct tegra_bpmp *bpmp)
> > +{
> > +	int i;
> 
> Can be unsigned int. The preferred ordering for variable declarations is
> the inverse christmas tree (i.e. sort by length in decreasing order). It
> often matches the result of sorting by importance (i.e. the "priv"
> pointer is more important than the loop variable).
> 
> > +	struct tegra186_bpmp *priv = bpmp->priv;
> > +
> > +	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);
> > +	tegra186_bpmp_channel_cleanup(bpmp->tx_channel);
> > +
> > +	if (priv->type == TEGRA_SRAM) {
> > +		gen_pool_free(priv->sram.tx, (unsigned long)priv->tx.virt, 4096);
> > +		gen_pool_free(priv->sram.rx, (unsigned long)priv->rx.virt, 4096);
> > +	} else if (priv->type == TEGRA_RMEM) {
> > +		memunmap(priv->tx.virt);
> > +	}
> 
> This introduces a bit of an asymmetry because tegra_bpmp_channel_setup()
> doesn't actually set up the pool or reserved-memory. Since the memory is
> only used for the channels, we can probably move the initialization into
> tegra186_bpmp_channel_setup() below.

This is the teardown, not the initialization?

> 
> > +}
> > +
> > +static int tegra186_bpmp_channel_setup(struct tegra_bpmp *bpmp)
> 
> This name could be confusing because we already use the
> tegra186_bpmp_channel_ prefix for functions that operate on individual
> channels, whereas this function operates on the BPMP object.
> 
> Perhaps something like tegra186_bpmp_setup_channels() would better
> reflect what this does.
> 
> The same goes for tegra186_bpmp_channel_deinit() above. Maybe something
> like tegra186_bpmp_cleanup_channels() to make it more obvious that it's
> the counterpart of tegra186_bpmp_setup_channels().
> 
> >  {
> > -	struct tegra186_bpmp *priv;
> >  	unsigned int i;
> >  	int err;
> >  
> > -	priv = devm_kzalloc(bpmp->dev, sizeof(*priv), GFP_KERNEL);
> > -	if (!priv)
> > -		return -ENOMEM;
> > +	err = tegra186_bpmp_channel_init(bpmp->tx_channel, bpmp,
> > +					 bpmp->soc->channels.cpu_tx.offset);
> > +	if (err < 0)
> > +		return err;
> >  
> > -	bpmp->priv = priv;
> > -	priv->parent = bpmp;
> > +	err = tegra186_bpmp_channel_init(bpmp->rx_channel, bpmp,
> > +					 bpmp->soc->channels.cpu_rx.offset);
> > +	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;
> >  
> > -	priv->tx.pool = of_gen_pool_get(bpmp->dev->of_node, "shmem", 0);
> > -	if (!priv->tx.pool) {
> > +		err = tegra186_bpmp_channel_init(&bpmp->threaded_channels[i],
> > +						 bpmp, index);
> > +		if (err < 0)
> > +			break;
> > +	}
> > +
> > +	if (err < 0)
> > +		tegra186_bpmp_channel_deinit(bpmp);
> 
> See how the name is confusing here? This is very close to the call to
> tegra186_bpmp_channel_init() above and the common prefix makes it seem
> like this would undo the effects of the above and then immediately
> raises the question why it's only undoing all of the above channel
> initializations. You then have to actually look at the implementation to
> find out that that's exactly what it does.
> 
> > +
> > +	return err;
> > +}
> > +
> > +static void tegra186_bpmp_reset_channels(struct tegra_bpmp *bpmp)
> > +{
> > +	unsigned int 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]);
> > +}
> 
> I think this now matches the tegra186_bpmp_resume() implementation, so
> it could be reused in that.
> 

Ok.

> > +
> > +static int tegra186_bpmp_sram_init(struct tegra_bpmp *bpmp)
> > +{
> > +	int err;
> > +	struct tegra186_bpmp *priv = bpmp->priv;
> > +
> > +	priv->sram.tx = of_gen_pool_get(bpmp->dev->of_node, "shmem", 0);
> > +	if (!priv->sram.tx) {
> >  		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);
> > +	priv->tx.virt = gen_pool_dma_alloc(priv->sram.tx, 4096, &priv->tx.phys);
> >  	if (!priv->tx.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->sram.rx = of_gen_pool_get(bpmp->dev->of_node, "shmem", 1);
> > +	if (!priv->sram.rx) {
> >  		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);
> > +	priv->rx.virt = gen_pool_dma_alloc(priv->sram.rx, 4096, &priv->rx.phys);
> >  	if (!priv->rx.virt) {
> >  		dev_err(bpmp->dev, "failed to allocate from RX pool\n");
> >  		err = -ENOMEM;
> >  		goto free_tx;
> >  	}
> >  
> > -	err = tegra186_bpmp_channel_init(bpmp->tx_channel, bpmp,
> > -					 bpmp->soc->channels.cpu_tx.offset);
> > -	if (err < 0)
> > -		goto free_rx;
> > +	priv->type = TEGRA_SRAM;
> >  
> > -	err = tegra186_bpmp_channel_init(bpmp->rx_channel, bpmp,
> > -					 bpmp->soc->channels.cpu_rx.offset);
> > -	if (err < 0)
> > -		goto cleanup_tx_channel;
> > +	return 0;
> >  
> > -	for (i = 0; i < bpmp->threaded.count; i++) {
> > -		unsigned int index = bpmp->soc->channels.thread.offset + i;
> > +free_tx:
> > +	gen_pool_free(priv->sram.tx, (unsigned long)priv->tx.virt, 4096);
> >  
> > -		err = tegra186_bpmp_channel_init(&bpmp->threaded_channels[i],
> > -						 bpmp, index);
> > +	return err;
> > +}
> > +
> > +static enum tegra_bpmp_mem_type tegra186_bpmp_dram_init(struct tegra_bpmp *bpmp)
> > +{
> > +	int err;
> > +	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 TEGRA_INVALID;
> > +
> > +	err = of_address_to_resource(np, 0, &res);
> > +	if (err) {
> > +		dev_warn(bpmp->dev,  "Parsing memory region returned: %d\n", err);
> > +		return TEGRA_INVALID;
> > +	}
> > +
> > +	if ((res.end - res.start + 1) < 0x2000) {
> 
> resource_size(), and maybe use SZ_8K instead of the literal here.
> 
> > +		dev_warn(bpmp->dev,  "DRAM region less than 0x2000 bytes\n");
> 
> Also, better to use a more human-readable string here. While at it,
> perhaps we can make this a bit more assertive, maybe something like:
> 
> 	"DRAM region must be larger than 8 KiB"
> 
> ?
> 
> > +		return TEGRA_INVALID;
> > +	}
> 
> This doesn't allow the caller to differentiate between potentially fatal
> errors and non-fatal ones. For instance, we don't want the absence of a
> "memory-region" property to be fatal (because we want to fall back to
> use SRAM in that case, or at least attempt to), but if "memory-region"
> exists, any of the subsequent errors probably should be fatal. It's
> easier to deal with that situation if you return regular error codes
> here. The !np check above could return -ENODEV, for example, as a way of
> letting the caller know that we don't have DRAM support in DT. For the
> of_address_to_resource() failure we can instead propagate the error code
> and so on.
> 
> Also, I think it'd be better to use a named constant like SZ_8K instead
> of the literal 0x2000 above. 
> 

Will look at this.

> > +
> > +	priv->tx.phys = res.start;
> > +	priv->rx.phys = res.start + 0x1000;
> 
> SZ_4K
> 
> > +
> > +	priv->tx.virt = memremap(priv->tx.phys, res.end - res.start + 1, MEMREMAP_WC);
> 
> Another case where we can use resource_size(). Might be a good idea to
> introduce a local "size" variable.
> 
> > +	if (priv->tx.virt == NULL) {
> > +		dev_warn(bpmp->dev,  "DRAM region mapping failed\n");
> > +		return TEGRA_INVALID;
> > +	}
> > +	priv->rx.virt = priv->tx.virt + 0x1000;
> 
> SZ_4K
> 
> We should probably do the same thing for the SRAM paths, but that should
> be a separate patch and can be done at another time.
> 

Yeah, this is basically the same as 4096 in the sram init but I think
hex is more readable.

> > +
> > +	return TEGRA_RMEM;
> > +}
> > +
> > +static int tegra186_bpmp_init(struct tegra_bpmp *bpmp)
> > +{
> > +	struct tegra186_bpmp *priv;
> > +	int err;
> > +
> > +	priv = devm_kzalloc(bpmp->dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	bpmp->priv = priv;
> > +	priv->parent = bpmp;
> > +
> > +	priv->type = tegra186_bpmp_dram_init(bpmp);
> > +	if (priv->type == TEGRA_INVALID) {
> > +		err = tegra186_bpmp_sram_init(bpmp);
> >  		if (err < 0)
> > -			goto cleanup_channels;
> > +			return err;
> >  	}
> 
> As I mentioned previously, I think we can move the block above into
> tegra186_bpmp_setup_channels() to make it symmetric with the teardown of
> this in tegra186_bpmp_cleanup_channels().
> 
> Thierry
> 

Peter.

Powered by blists - more mailing lists