diff mbox series

[1/9] mtd: rawnand: brcmnand: Allow SoC to provide I/O operations

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

Commit Message

Florian Fainelli Dec. 23, 2021, 12:22 a.m. UTC
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(-)

Comments

Miquel Raynal Jan. 3, 2022, 4:49 p.m. UTC | #1
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
Florian Fainelli Jan. 3, 2022, 5:24 p.m. UTC | #2
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
Miquel Raynal Jan. 4, 2022, 8:32 a.m. UTC | #3
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
Miquel Raynal Jan. 4, 2022, 8:57 a.m. UTC | #4
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
Florian Fainelli Jan. 4, 2022, 6:34 p.m. UTC | #5
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.
Miquel Raynal Jan. 4, 2022, 6:37 p.m. UTC | #6
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 mbox series

Patch

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);