| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20220104093221.6414aab9@xps13>
Date: Tue, 4 Jan 2022 09:32:21 +0100
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Florian Fainelli <f.fainelli@...il.com>
Cc: linux-mtd@...ts.infradead.org,
Rafał Miłecki <zajec5@...il.com>,
Richard Weinberger <richard@....at>,
Vignesh Raghavendra <vigneshr@...com>,
Brian Norris <computersforpeace@...il.com>,
Kamal Dasu <kdasu.kdev@...il.com>,
Arnd Bergmann <arnd@...db.de>,
Cai Huoqing <caihuoqing@...du.com>,
Colin Ian King <colin.king@...el.com>,
open list <linux-kernel@...r.kernel.org>,
"open list:BROADCOM SPECIFIC AMBA DRIVER (BCMA)"
<linux-wireless@...r.kernel.org>,
"open list:BROADCOM STB NAND FLASH DRIVER"
<bcm-kernel-feedback-list@...adcom.com>
Subject: Re: [PATCH 1/9] mtd: rawnand: brcmnand: Allow SoC to provide I/O
operations
Hi Florian,
f.fainelli@...il.com wrote on Mon, 3 Jan 2022 09:24:26 -0800:
> On 1/3/2022 8:49 AM, Miquel Raynal wrote:
> > Hi Florian,
> >
> > f.fainelli@...il.com wrote on Wed, 22 Dec 2021 16:22:17 -0800:
> >
> >> Allow a brcmnand_soc instance to provide a custom set of I/O operations
> >> which we will require when using this driver on a BCMA bus which is not
> >> directly memory mapped I/O. Update the nand_{read,write}_reg accordingly
> >> to use the SoC operations if provided.
> >>
> >> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
> >> ---
> >> drivers/mtd/nand/raw/brcmnand/brcmnand.c | 14 ++++++++++++--
> >> drivers/mtd/nand/raw/brcmnand/brcmnand.h | 23 +++++++++++++++++++++++
> >> 2 files changed, 35 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> index f75929783b94..7a1673b1b1af 100644
> >> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> @@ -594,13 +594,18 @@ enum {
> >> >> static inline u32 nand_readreg(struct brcmnand_controller *ctrl, u32 offs)
> >> {
> >> + if (brcmnand_soc_has_ops(ctrl->soc))
> >> + return brcmnand_soc_read(ctrl->soc, offs);
> >> return brcmnand_readl(ctrl->nand_base + offs);
> >> }
> >> >> static inline void nand_writereg(struct brcmnand_controller *ctrl, u32 offs,
> >> u32 val)
> >> {
> >> - brcmnand_writel(val, ctrl->nand_base + offs);
> >> + if (brcmnand_soc_has_ops(ctrl->soc))
> >> + brcmnand_soc_write(ctrl->soc, val, offs);
> >> + else
> >> + brcmnand_writel(val, ctrl->nand_base + offs);
> >> }
> >> >> static int brcmnand_revision_init(struct brcmnand_controller *ctrl)
> >> @@ -766,13 +771,18 @@ static inline void brcmnand_rmw_reg(struct brcmnand_controller *ctrl,
> >> >> static inline u32 brcmnand_read_fc(struct brcmnand_controller *ctrl, int word)
> >> {
> >> + if (brcmnand_soc_has_ops(ctrl->soc))
> >> + return brcmnand_soc_read(ctrl->soc, ~0);
> >> return __raw_readl(ctrl->nand_fc + word * 4);
> >> }
> >> >> static inline void brcmnand_write_fc(struct brcmnand_controller *ctrl,
> >> int word, u32 val)
> >> {
> >> - __raw_writel(val, ctrl->nand_fc + word * 4);
> >> + if (brcmnand_soc_has_ops(ctrl->soc))
> >> + brcmnand_soc_write(ctrl->soc, val, ~0);
> >> + else
> >> + __raw_writel(val, ctrl->nand_fc + word * 4);
> >> }
> >> >> static inline void edu_writel(struct brcmnand_controller *ctrl,
> >> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.h b/drivers/mtd/nand/raw/brcmnand/brcmnand.h
> >> index eb498fbe505e..a3f2ad5f6572 100644
> >> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.h
> >> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.h
> >> @@ -11,12 +11,19 @@
> >> >> struct platform_device;
> >> struct dev_pm_ops;
> >> +struct brcmnand_io_ops;
> >> >> struct brcmnand_soc {
> >> bool (*ctlrdy_ack)(struct brcmnand_soc *soc);
> >> void (*ctlrdy_set_enabled)(struct brcmnand_soc *soc, bool en);
> >> void (*prepare_data_bus)(struct brcmnand_soc *soc, bool prepare,
> >> bool is_param);
> >> + const struct brcmnand_io_ops *ops;
> >> +};
> >> +
> >> +struct brcmnand_io_ops {
> >> + u32 (*read_reg)(struct brcmnand_soc *soc, u32 offset);
> >> + void (*write_reg)(struct brcmnand_soc *soc, u32 val, u32 offset);
> >> };
> >> >> static inline void brcmnand_soc_data_bus_prepare(struct brcmnand_soc *soc,
> >> @@ -58,6 +65,22 @@ static inline void brcmnand_writel(u32 val, void __iomem *addr)
> >> writel_relaxed(val, addr);
> >> }
> >> >> +static inline bool brcmnand_soc_has_ops(struct brcmnand_soc *soc)
> >> +{
> >> + return soc && soc->ops && soc->ops->read_reg && soc->ops->write_reg;
> >> +}
> >> +
> >> +static inline u32 brcmnand_soc_read(struct brcmnand_soc *soc, u32 offset)
> >> +{
> >> + return soc->ops->read_reg(soc, offset);
> >> +}
> >> +
> >> +static inline void brcmnand_soc_write(struct brcmnand_soc *soc, u32 val,
> >> + u32 offset)
> >> +{
> >> + soc->ops->write_reg(soc, val, offset);
> >> +}
> >> +
> >
> > It might be worth looking into more optimized ways to do these checks,
> > in particular the read/write_reg ones because you're checking against
> > some static data which cannot be optimized out by the compiler but
> > won't change in the lifetime of the kernel.
>
> I suppose I could add an addition if IS_ENABLED(CONFIG_MTD_NAND_BRCMNAND_BCMA) at the front of brcmnand_soc_has_ops(), would that address your concern or you have something else in mind?
I don't like much the #ifdef solution, instead you might think of
static keys, or even better using a regmap. Regmap implementation is
free, you can use either one way or the other and for almost no
overhead compared to the bunch of functions you have here.
Thanks,
Miquèl
Powered by blists - more mailing lists