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

[Florian Fainelli <f.fainelli@gmail.com>]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/