Re: [PATCH 7/8] mtd: rawnand: jz4780-bch: Separate top-level and SoC specific code

[Paul Cercueil <paul@crapouillou.net>]

Hi,

On Fri, Jan 18, 2019 at 5:35 AM, Boris Brezillon 
<bbrezillon@kernel.org> wrote:
> On Thu, 17 Jan 2019 22:06:33 -0300
> Paul Cercueil <paul@crapouillou.net <mailto:paul@crapouillou.net>> 
> wrote:
> 
>>  The jz4780-nand driver uses an API provided by the jz4780-bch 
>> driver.
>>  This makes it difficult to support other SoCs in the jz4780-bch 
>> driver.
>>  To work around this, we separate the API functions from the 
>> SoC-specific
>>  code, so that these API functions are SoC-agnostic.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net 
>> <mailto:paul@crapouillou.net>>
>>  ---
>>   drivers/mtd/nand/raw/Makefile              |   3 +-
>>   drivers/mtd/nand/raw/jz4780_bch.c          | 173 
>> +++--------------------------
>>   drivers/mtd/nand/raw/jz4780_bch_common.c   | 172 
>> ++++++++++++++++++++++++++++
>>   drivers/mtd/nand/raw/jz4780_bch_internal.h |  34 ++++++
> 
> What's the overhead of having 4725b and 4780 code in the same source
> file? If we really need to split that up, I'd prefer to have all 
> jz47xx
> sources placed in a subdir (drivers/mtd/nand/raw/jz47xx/).

It's complicated, the hardware changed a lot between the two SoC 
revisions,
to the point where it's not really possible to support the two within 
one
single driver. The registers are different, the behaviour is different, 
etc.

>>   4 files changed, 222 insertions(+), 160 deletions(-)
>>   create mode 100644 drivers/mtd/nand/raw/jz4780_bch_common.c
>>   create mode 100644 drivers/mtd/nand/raw/jz4780_bch_internal.h
>> 
>>  diff --git a/drivers/mtd/nand/raw/Makefile 
>> b/drivers/mtd/nand/raw/Makefile
>>  index 57159b349054..6dacc9cf38d5 100644
>>  --- a/drivers/mtd/nand/raw/Makefile
>>  +++ b/drivers/mtd/nand/raw/Makefile
>>  @@ -46,7 +46,8 @@ obj-$(CONFIG_MTD_NAND_MPC5121_NFC)	+= 
>> mpc5121_nfc.o
>>   obj-$(CONFIG_MTD_NAND_VF610_NFC)	+= vf610_nfc.o
>>   obj-$(CONFIG_MTD_NAND_RICOH)		+= r852.o
>>   obj-$(CONFIG_MTD_NAND_JZ4740)		+= jz4740_nand.o
>>  -obj-$(CONFIG_MTD_NAND_JZ4780)		+= jz4780_nand.o jz4780_bch.o
>>  +obj-$(CONFIG_MTD_NAND_JZ4780)		+= jz4780_nand.o 
>> jz4780_bch_common.o \
>>  +					   jz4780_bch.o
>>   obj-$(CONFIG_MTD_NAND_GPMI_NAND)	+= gpmi-nand/
>>   obj-$(CONFIG_MTD_NAND_XWAY)		+= xway_nand.o
>>   obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH)	+= bcm47xxnflash/
>>  diff --git a/drivers/mtd/nand/raw/jz4780_bch.c 
>> b/drivers/mtd/nand/raw/jz4780_bch.c
>>  index 161d3821e1c4..1dfc960067b3 100644
>>  --- a/drivers/mtd/nand/raw/jz4780_bch.c
>>  +++ b/drivers/mtd/nand/raw/jz4780_bch.c
>>  @@ -1,25 +1,19 @@
>>   // SPDX-License-Identifier: GPL-2.0
>>   /*
>>  - * JZ4780 BCH controller
>>  + * JZ4780 backend code for the jz4780-bch driver
>>    *
>>    * Copyright (c) 2015 Imagination Technologies
>>    * Author: Alex Smith <alex.smith@imgtec.com 
>> <mailto:alex.smith@imgtec.com>>
>>    */
>> 
>>   #include <linux/bitops.h>
>>  -#include <linux/clk.h>
>>  -#include <linux/delay.h>
>>  -#include <linux/init.h>
>>   #include <linux/iopoll.h>
>>  -#include <linux/module.h>
>>   #include <linux/mutex.h>
>>   #include <linux/of.h>
>>  -#include <linux/of_platform.h>
>>  -#include <linux/platform_device.h>
>>  -#include <linux/sched.h>
>>  -#include <linux/slab.h>
>>  +#include <linux/device.h>
>> 
>>   #include "jz4780_bch.h"
>>  +#include "jz4780_bch_internal.h"
>> 
>>   #define BCH_BHCR			0x0
>>   #define BCH_BHCCR			0x8
>>  @@ -60,13 +54,6 @@
>>   /* Timeout for BCH calculation/correction. */
>>   #define BCH_TIMEOUT_US			100000
>> 
>>  -struct jz4780_bch {
>>  -	struct device *dev;
>>  -	void __iomem *base;
>>  -	struct clk *clk;
>>  -	struct mutex lock;
>>  -};
>>  -
>>   static void jz4780_bch_init(struct jz4780_bch *bch,
>>   			    struct jz4780_bch_params *params, bool encode)
>>   {
>>  @@ -165,18 +152,9 @@ static bool jz4780_bch_wait_complete(struct 
>> jz4780_bch *bch, unsigned int irq,
>>   	return true;
>>   }
>> 
>>  -/**
>>  - * jz4780_bch_calculate() - calculate ECC for a data buffer
>>  - * @bch: BCH device.
>>  - * @params: BCH parameters.
>>  - * @buf: input buffer with raw data.
>>  - * @ecc_code: output buffer with ECC.
>>  - *
>>  - * Return: 0 on success, -ETIMEDOUT if timed out while waiting for 
>> BCH
>>  - * controller.
>>  - */
>>  -int jz4780_bch_calculate(struct jz4780_bch *bch, struct 
>> jz4780_bch_params *params,
>>  -			 const u8 *buf, u8 *ecc_code)
>>  +static int jz4780_calculate(struct jz4780_bch *bch,
>>  +			    struct jz4780_bch_params *params,
>>  +			    const u8 *buf, u8 *ecc_code)
>>   {
>>   	int ret = 0;
>> 
>>  @@ -195,23 +173,10 @@ int jz4780_bch_calculate(struct jz4780_bch 
>> *bch, struct jz4780_bch_params *param
>>   	mutex_unlock(&bch->lock);
>>   	return ret;
>>   }
>>  -EXPORT_SYMBOL(jz4780_bch_calculate);
>>  -
>>  -/**
>>  - * jz4780_bch_correct() - detect and correct bit errors
>>  - * @bch: BCH device.
>>  - * @params: BCH parameters.
>>  - * @buf: raw data read from the chip.
>>  - * @ecc_code: ECC read from the chip.
>>  - *
>>  - * Given the raw data and the ECC read from the NAND device, 
>> detects and
>>  - * corrects errors in the data.
>>  - *
>>  - * Return: the number of bit errors corrected, -EBADMSG if there 
>> are too many
>>  - * errors to correct or -ETIMEDOUT if we timed out waiting for the 
>> controller.
>>  - */
>>  -int jz4780_bch_correct(struct jz4780_bch *bch, struct 
>> jz4780_bch_params *params,
>>  -		       u8 *buf, u8 *ecc_code)
>>  +
>>  +static int jz4780_correct(struct jz4780_bch *bch,
>>  +			  struct jz4780_bch_params *params,
>>  +			  u8 *buf, u8 *ecc_code)
>>   {
>>   	u32 reg, mask, index;
>>   	int i, ret, count;
>>  @@ -257,119 +222,9 @@ int jz4780_bch_correct(struct jz4780_bch 
>> *bch, struct jz4780_bch_params *params,
>>   	mutex_unlock(&bch->lock);
>>   	return ret;
>>   }
>>  -EXPORT_SYMBOL(jz4780_bch_correct);
>>  -
>>  -/**
>>  - * jz4780_bch_get() - get the BCH controller device
>>  - * @np: BCH device tree node.
>>  - *
>>  - * Gets the BCH controller device from the specified device tree 
>> node. The
>>  - * device must be released with jz4780_bch_release() when it is no 
>> longer being
>>  - * used.
>>  - *
>>  - * Return: a pointer to jz4780_bch, errors are encoded into the 
>> pointer.
>>  - * PTR_ERR(-EPROBE_DEFER) if the device hasn't been initialised 
>> yet.
>>  - */
>>  -static struct jz4780_bch *jz4780_bch_get(struct device_node *np)
>>  -{
>>  -	struct platform_device *pdev;
>>  -	struct jz4780_bch *bch;
>>  -
>>  -	pdev = of_find_device_by_node(np);
>>  -	if (!pdev || !platform_get_drvdata(pdev))
>>  -		return ERR_PTR(-EPROBE_DEFER);
>>  -
>>  -	get_device(&pdev->dev);
>>  -
>>  -	bch = platform_get_drvdata(pdev);
>>  -	clk_prepare_enable(bch->clk);
>>  -
>>  -	return bch;
>>  -}
>>  -
>>  -/**
>>  - * of_jz4780_bch_get() - get the BCH controller from a DT node
>>  - * @of_node: the node that contains a bch-controller property.
>>  - *
>>  - * Get the bch-controller property from the given device tree
>>  - * node and pass it to jz4780_bch_get to do the work.
>>  - *
>>  - * Return: a pointer to jz4780_bch, errors are encoded into the 
>> pointer.
>>  - * PTR_ERR(-EPROBE_DEFER) if the device hasn't been initialised 
>> yet.
>>  - */
>>  -struct jz4780_bch *of_jz4780_bch_get(struct device_node *of_node)
>>  -{
>>  -	struct jz4780_bch *bch = NULL;
>>  -	struct device_node *np;
>> 
>>  -	np = of_parse_phandle(of_node, "ingenic,bch-controller", 0);
>>  -
>>  -	if (np) {
>>  -		bch = jz4780_bch_get(np);
>>  -		of_node_put(np);
>>  -	}
>>  -	return bch;
>>  -}
>>  -EXPORT_SYMBOL(of_jz4780_bch_get);
>>  -
>>  -/**
>>  - * jz4780_bch_release() - release the BCH controller device
>>  - * @bch: BCH device.
>>  - */
>>  -void jz4780_bch_release(struct jz4780_bch *bch)
>>  -{
>>  -	clk_disable_unprepare(bch->clk);
>>  -	put_device(bch->dev);
>>  -}
>>  -EXPORT_SYMBOL(jz4780_bch_release);
>>  -
>>  -static int jz4780_bch_probe(struct platform_device *pdev)
>>  -{
>>  -	struct device *dev = &pdev->dev;
>>  -	struct jz4780_bch *bch;
>>  -	struct resource *res;
>>  -
>>  -	bch = devm_kzalloc(dev, sizeof(*bch), GFP_KERNEL);
>>  -	if (!bch)
>>  -		return -ENOMEM;
>>  -
>>  -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>  -	bch->base = devm_ioremap_resource(dev, res);
>>  -	if (IS_ERR(bch->base))
>>  -		return PTR_ERR(bch->base);
>>  -
>>  -	jz4780_bch_disable(bch);
>>  -
>>  -	bch->clk = devm_clk_get(dev, NULL);
>>  -	if (IS_ERR(bch->clk)) {
>>  -		dev_err(dev, "failed to get clock: %ld\n", PTR_ERR(bch->clk));
>>  -		return PTR_ERR(bch->clk);
>>  -	}
>>  -
>>  -	mutex_init(&bch->lock);
>>  -
>>  -	bch->dev = dev;
>>  -	platform_set_drvdata(pdev, bch);
>>  -
>>  -	return 0;
>>  -}
>>  -
>>  -static const struct of_device_id jz4780_bch_dt_match[] = {
>>  -	{ .compatible = "ingenic,jz4780-bch" },
>>  -	{},
>>  +const struct jz4780_bch_ops jz4780_bch_jz4780_ops = {
>>  +	.disable = jz4780_bch_disable,
>>  +	.calculate = jz4780_calculate,
>>  +	.correct = jz4780_correct,
>>   };
>>  -MODULE_DEVICE_TABLE(of, jz4780_bch_dt_match);
>>  -
>>  -static struct platform_driver jz4780_bch_driver = {
>>  -	.probe		= jz4780_bch_probe,
>>  -	.driver	= {
>>  -		.name	= "jz4780-bch",
>>  -		.of_match_table = of_match_ptr(jz4780_bch_dt_match),
>>  -	},
>>  -};
>>  -module_platform_driver(jz4780_bch_driver);
>>  -
>>  -MODULE_AUTHOR("Alex Smith <alex@alex-smith.me.uk 
>> <mailto:alex@alex-smith.me.uk>>");
>>  -MODULE_AUTHOR("Harvey Hunt <harveyhuntnexus@gmail.com 
>> <mailto:harveyhuntnexus@gmail.com>>");
>>  -MODULE_DESCRIPTION("Ingenic JZ4780 BCH error correction driver");
>>  -MODULE_LICENSE("GPL v2");
>>  diff --git a/drivers/mtd/nand/raw/jz4780_bch_common.c 
>> b/drivers/mtd/nand/raw/jz4780_bch_common.c
>>  new file mode 100644
>>  index 000000000000..573b079e6cbe
>>  --- /dev/null
>>  +++ b/drivers/mtd/nand/raw/jz4780_bch_common.c
>>  @@ -0,0 +1,172 @@
>>  +// SPDX-License-Identifier: GPL-2.0
>>  +/*
>>  + * JZ4780 BCH controller
>>  + *
>>  + * Copyright (c) 2015 Imagination Technologies
>>  + * Author: Alex Smith <alex.smith@imgtec.com 
>> <mailto:alex.smith@imgtec.com>>
>>  + */
>>  +
>>  +#include <linux/clk.h>
>>  +#include <linux/init.h>
>>  +#include <linux/module.h>
>>  +#include <linux/of_platform.h>
>>  +#include <linux/platform_device.h>
>>  +
>>  +#include "jz4780_bch_internal.h"
>>  +#include "jz4780_bch.h"
>>  +
>>  +/**
>>  + * jz4780_bch_calculate() - calculate ECC for a data buffer
>>  + * @bch: BCH device.
>>  + * @params: BCH parameters.
>>  + * @buf: input buffer with raw data.
>>  + * @ecc_code: output buffer with ECC.
>>  + *
>>  + * Return: 0 on success, -ETIMEDOUT if timed out while waiting for 
>> BCH
>>  + * controller.
>>  + */
>>  +int jz4780_bch_calculate(struct jz4780_bch *bch, struct 
>> jz4780_bch_params *params,
>>  +			 const u8 *buf, u8 *ecc_code)
>>  +{
>>  +	return bch->ops->calculate(bch, params, buf, ecc_code);
>>  +}
>>  +EXPORT_SYMBOL(jz4780_bch_calculate);
>>  +
>>  +/**
>>  + * jz4780_bch_correct() - detect and correct bit errors
>>  + * @bch: BCH device.
>>  + * @params: BCH parameters.
>>  + * @buf: raw data read from the chip.
>>  + * @ecc_code: ECC read from the chip.
>>  + *
>>  + * Given the raw data and the ECC read from the NAND device, 
>> detects and
>>  + * corrects errors in the data.
>>  + *
>>  + * Return: the number of bit errors corrected, -EBADMSG if there 
>> are too many
>>  + * errors to correct or -ETIMEDOUT if we timed out waiting for the 
>> controller.
>>  + */
>>  +int jz4780_bch_correct(struct jz4780_bch *bch, struct 
>> jz4780_bch_params *params,
>>  +		       u8 *buf, u8 *ecc_code)
>>  +{
>>  +	return bch->ops->correct(bch, params, buf, ecc_code);
>>  +}
>>  +EXPORT_SYMBOL(jz4780_bch_correct);
>>  +
>>  +/**
>>  + * jz4780_bch_get() - get the BCH controller device
>>  + * @np: BCH device tree node.
>>  + *
>>  + * Gets the BCH controller device from the specified device tree 
>> node. The
>>  + * device must be released with jz4780_bch_release() when it is no 
>> longer being
>>  + * used.
>>  + *
>>  + * Return: a pointer to jz4780_bch, errors are encoded into the 
>> pointer.
>>  + * PTR_ERR(-EPROBE_DEFER) if the device hasn't been initialised 
>> yet.
>>  + */
>>  +static struct jz4780_bch *jz4780_bch_get(struct device_node *np)
>>  +{
>>  +	struct platform_device *pdev;
>>  +	struct jz4780_bch *bch;
>>  +
>>  +	pdev = of_find_device_by_node(np);
>>  +	if (!pdev || !platform_get_drvdata(pdev))
>>  +		return ERR_PTR(-EPROBE_DEFER);
>>  +
>>  +	get_device(&pdev->dev);
>>  +
>>  +	bch = platform_get_drvdata(pdev);
>>  +	clk_prepare_enable(bch->clk);
>>  +
>>  +	return bch;
>>  +}
>>  +
>>  +/**
>>  + * of_jz4780_bch_get() - get the BCH controller from a DT node
>>  + * @of_node: the node that contains a bch-controller property.
>>  + *
>>  + * Get the bch-controller property from the given device tree
>>  + * node and pass it to jz4780_bch_get to do the work.
>>  + *
>>  + * Return: a pointer to jz4780_bch, errors are encoded into the 
>> pointer.
>>  + * PTR_ERR(-EPROBE_DEFER) if the device hasn't been initialised 
>> yet.
>>  + */
>>  +struct jz4780_bch *of_jz4780_bch_get(struct device_node *of_node)
>>  +{
>>  +	struct jz4780_bch *bch = NULL;
>>  +	struct device_node *np;
>>  +
>>  +	np = of_parse_phandle(of_node, "ingenic,bch-controller", 0);
>>  +
>>  +	if (np) {
>>  +		bch = jz4780_bch_get(np);
>>  +		of_node_put(np);
>>  +	}
>>  +	return bch;
>>  +}
>>  +EXPORT_SYMBOL(of_jz4780_bch_get);
>>  +
>>  +/**
>>  + * jz4780_bch_release() - release the BCH controller device
>>  + * @bch: BCH device.
>>  + */
>>  +void jz4780_bch_release(struct jz4780_bch *bch)
>>  +{
>>  +	clk_disable_unprepare(bch->clk);
>>  +	put_device(bch->dev);
>>  +}
>>  +EXPORT_SYMBOL(jz4780_bch_release);
>>  +
>>  +static int jz4780_bch_probe(struct platform_device *pdev)
>>  +{
>>  +	struct device *dev = &pdev->dev;
>>  +	struct jz4780_bch *bch;
>>  +	struct resource *res;
>>  +
>>  +	bch = devm_kzalloc(dev, sizeof(*bch), GFP_KERNEL);
>>  +	if (!bch)
>>  +		return -ENOMEM;
>>  +
>>  +	bch->ops = device_get_match_data(dev);
>>  +	if (!bch->ops)
>>  +		return -EINVAL;
>>  +
>>  +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>  +	bch->base = devm_ioremap_resource(dev, res);
>>  +	if (IS_ERR(bch->base))
>>  +		return PTR_ERR(bch->base);
>>  +
>>  +	bch->ops->disable(bch);
>>  +
>>  +	bch->clk = devm_clk_get(dev, NULL);
>>  +	if (IS_ERR(bch->clk)) {
>>  +		dev_err(dev, "failed to get clock: %ld\n", PTR_ERR(bch->clk));
>>  +		return PTR_ERR(bch->clk);
>>  +	}
>>  +
>>  +	mutex_init(&bch->lock);
>>  +
>>  +	bch->dev = dev;
>>  +	platform_set_drvdata(pdev, bch);
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static const struct of_device_id jz4780_bch_dt_match[] = {
>>  +	{ .compatible = "ingenic,jz4780-bch", .data = 
>> &jz4780_bch_jz4780_ops },
>>  +	{},
>>  +};
>>  +MODULE_DEVICE_TABLE(of, jz4780_bch_dt_match);
>>  +
>>  +static struct platform_driver jz4780_bch_driver = {
>>  +	.probe		= jz4780_bch_probe,
>>  +	.driver	= {
>>  +		.name	= "jz4780-bch",
>>  +		.of_match_table = of_match_ptr(jz4780_bch_dt_match),
>>  +	},
>>  +};
>>  +module_platform_driver(jz4780_bch_driver);
>>  +
>>  +MODULE_AUTHOR("Alex Smith <alex@alex-smith.me.uk 
>> <mailto:alex@alex-smith.me.uk>>");
>>  +MODULE_AUTHOR("Harvey Hunt <harveyhuntnexus@gmail.com 
>> <mailto:harveyhuntnexus@gmail.com>>");
>>  +MODULE_DESCRIPTION("Ingenic JZ4780 BCH error correction driver");
>>  +MODULE_LICENSE("GPL v2");
>>  diff --git a/drivers/mtd/nand/raw/jz4780_bch_internal.h 
>> b/drivers/mtd/nand/raw/jz4780_bch_internal.h
>>  new file mode 100644
>>  index 000000000000..7162e4f872f4
>>  --- /dev/null
>>  +++ b/drivers/mtd/nand/raw/jz4780_bch_internal.h
>>  @@ -0,0 +1,34 @@
>>  +/* SPDX-License-Identifier: GPL-2.0 */
>>  +#ifndef __DRIVERS_MTD_NAND_JZ4780_BCH_INTERNAL_H__
>>  +#define __DRIVERS_MTD_NAND_JZ4780_BCH_INTERNAL_H__
>>  +
>>  +#include <linux/compiler_types.h>
>>  +#include <linux/mutex.h>
>>  +#include <linux/types.h>
>>  +
>>  +struct jz4780_bch_params;
>>  +struct jz4780_bch;
>>  +struct device;
>>  +struct clk;
>>  +
>>  +struct jz4780_bch_ops {
>>  +	void (*disable)(struct jz4780_bch *bch);
>>  +	int (*calculate)(struct jz4780_bch *bch,
>>  +			 struct jz4780_bch_params *params,
>>  +			 const u8 *buf, u8 *ecc_code);
>>  +	int (*correct)(struct jz4780_bch *bch,
>>  +			struct jz4780_bch_params *params,
>>  +			u8 *buf, u8 *ecc_code);
>>  +};
>>  +
>>  +struct jz4780_bch {
>>  +	struct device *dev;
>>  +	const struct jz4780_bch_ops *ops;
>>  +	void __iomem *base;
>>  +	struct clk *clk;
>>  +	struct mutex lock;
>>  +};
>>  +
>>  +extern const struct jz4780_bch_ops jz4780_bch_jz4780_ops;
>>  +
>>  +#endif /* __DRIVERS_MTD_NAND_JZ4780_BCH_INTERNAL_H__ */
>