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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Tue, 4 Jan 2022 19:37:36 +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 Tue, 4 Jan 2022 10:34:35 -0800:

> On 1/4/22 12:57 AM, Miquel Raynal wrote:
> > Hi Miquel,
> > 
> > miquel.raynal@...tlin.com wrote on Tue, 4 Jan 2022 09:32:21 +0100:
> >   
> >> 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.  
> > 
> > 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

Powered by blists - more mailing lists