| Message ID | 20211223002225.3738385-2-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | BCMA support for brcmnand | expand |
Hi Florian, f.fainelli@gmail.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@gmail.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. Thanks, Miquèl
On 1/3/2022 8:49 AM, Miquel Raynal wrote: > Hi Florian, > > f.fainelli@gmail.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@gmail.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? -- Florian
Hi Florian, f.fainelli@gmail.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@gmail.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@gmail.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
Hi Miquel, miquel.raynal@bootlin.com wrote on Tue, 4 Jan 2022 09:32:21 +0100: > Hi Florian, > > f.fainelli@gmail.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@gmail.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@gmail.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. Maybe regmaps will actually be slower than these regular if's. Perhaps static keys are the best option? Cheers, Miquèl
On 1/4/22 12:57 AM, Miquel Raynal wrote: > Hi Miquel, > > miquel.raynal@bootlin.com wrote on Tue, 4 Jan 2022 09:32:21 +0100: > >> Hi Florian, >> >> f.fainelli@gmail.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@gmail.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@gmail.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. > > Maybe regmaps will actually be slower than these regular if's. Perhaps > static keys are the best option? OK static keys would probably work. I am not sure that the additional branches for each register access would actually be causing a noticeable performance impact. Pretty much any chip where this controller is used has a DMA interface that you program and kick, the PIO is already assumed to be slow, and each register access is about 200ns on STB chips at least.
Hi Florian, f.fainelli@gmail.com wrote on Tue, 4 Jan 2022 10:34:35 -0800: > On 1/4/22 12:57 AM, Miquel Raynal wrote: > > Hi Miquel, > > > > miquel.raynal@bootlin.com wrote on Tue, 4 Jan 2022 09:32:21 +0100: > > > >> Hi Florian, > >> > >> f.fainelli@gmail.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@gmail.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@gmail.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. > > > > Maybe regmaps will actually be slower than these regular if's. Perhaps > > static keys are the best option? > > OK static keys would probably work. I am not sure that the additional > branches for each register access would actually be causing a noticeable > performance impact. Pretty much any chip where this controller is used > has a DMA interface that you program and kick, the PIO is already > assumed to be slow, and each register access is about 200ns on STB chips > at least. I see. I'll let you decide what you prefer, I won't block if you stick to the regular if/else implementation. Thanks, Miquèl
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); +} + int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc); int brcmnand_remove(struct platform_device *pdev);
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@gmail.com> --- drivers/mtd/nand/raw/brcmnand/brcmnand.c | 14 ++++++++++++-- drivers/mtd/nand/raw/brcmnand/brcmnand.h | 23 +++++++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-)