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.
