diff mbox series

[RFC,5/5] mtd: hyperbus: Add driver for TI's Hyperbus memory controller

Message ID 20190219063607.29949-6-vigneshr@ti.com (mailing list archive)
State RFC
Headers show
Series MTD: Add Initial Hyperbus support | expand

Commit Message

Vignesh Raghavendra Feb. 19, 2019, 6:36 a.m. UTC 
Add driver for Hyperbus memory controller on TI's AM654 SoC. Programming
IP is pretty simple and provides direct memory mapped access to
connected Flash devices.

Add basic support for the IP without DMA. Second ChipSelect is not
supported for now.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 drivers/mtd/hyperbus/hbmc_am654.c | 105 ++++++++++++++++++++++++++++++
 1 file changed, 105 insertions(+)
 create mode 100644 drivers/mtd/hyperbus/hbmc_am654.c

Comments

Sergei Shtylyov Feb. 24, 2019, 2:06 p.m. UTC | #1 
Hello!

On 19.02.2019 9:36, Vignesh R (by way of Boris Brezillon
<bbrezillon@kernel.org>) wrote:

> Add driver for Hyperbus memory controller on TI's AM654 SoC. Programming
> IP is pretty simple and provides direct memory mapped access to
> connected Flash devices.
> 
> Add basic support for the IP without DMA. Second ChipSelect is not
> supported for now.
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
>   drivers/mtd/hyperbus/hbmc_am654.c | 105 ++++++++++++++++++++++++++++++
>   1 file changed, 105 insertions(+)
>   create mode 100644 drivers/mtd/hyperbus/hbmc_am654.c
> 
> diff --git a/drivers/mtd/hyperbus/hbmc_am654.c b/drivers/mtd/hyperbus/hbmc_am654.c
> new file mode 100644
> index 000000000000..1f0d2dc52f9f
> --- /dev/null
> +++ b/drivers/mtd/hyperbus/hbmc_am654.c
> @@ -0,0 +1,105 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> +// Author: Vignesh R <vigneshr@ti.com>
> +
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mtd/hyperbus.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/types.h>
> +
> +struct am654_hbmc_priv {
> +	struct hb_device hbdev;
> +	void __iomem	*regbase;
> +};
> +
> +static int am654_hbmc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct am654_hbmc_priv *priv;
> +	struct resource *res;
> +	int err;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (IS_ERR(res)) {

     I don't think that function ever returns error ptrs. It should only 
return NULL on error.

> +		dev_err(&pdev->dev, "failed to get memory resource\n");
> +		return -ENOENT;
> +	}
> +
> +	priv->regbase = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(priv->regbase)) {
> +		dev_err(dev, "Cannot remap controller address.\n");

    The above function already prints its error messages.

[...]
> +module_platform_driver(am654_hbmc_platform_driver);
> +
> +MODULE_DESCRIPTION("HBMC driver for AM654 SoC");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:hbmc-am654");
> +MODULE_AUTHOR("Vignesh R <vigneshr@ti.com>");

MBR, Sergei
Sergei Shtylyov Feb. 25, 2019, 4:29 p.m. UTC | #2 
Hello!

On 02/19/2019 09:36 AM, Vignesh R (by way of Boris Brezillon <bbrezillon@kernel.org>) wrote:

> Add driver for Hyperbus memory controller on TI's AM654 SoC. Programming
> IP is pretty simple and provides direct memory mapped access to
> connected Flash devices.

   Are you sure you posted the _complete_ driver?

> Add basic support for the IP without DMA. Second ChipSelect is not
> supported for now.
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
>  drivers/mtd/hyperbus/hbmc_am654.c | 105 ++++++++++++++++++++++++++++++
>  1 file changed, 105 insertions(+)
>  create mode 100644 drivers/mtd/hyperbus/hbmc_am654.c
> 
> diff --git a/drivers/mtd/hyperbus/hbmc_am654.c b/drivers/mtd/hyperbus/hbmc_am654.c
> new file mode 100644
> index 000000000000..1f0d2dc52f9f
> --- /dev/null
> +++ b/drivers/mtd/hyperbus/hbmc_am654.c
> @@ -0,0 +1,105 @@
[...]
> +static int am654_hbmc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct am654_hbmc_priv *priv;
> +	struct resource *res;
> +	int err;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (IS_ERR(res)) {
> +		dev_err(&pdev->dev, "failed to get memory resource\n");
> +		return -ENOENT;
> +	}
> +
> +	priv->regbase = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(priv->regbase)) {
> +		dev_err(dev, "Cannot remap controller address.\n");
> +		return PTR_ERR(priv->regbase);
> +	}
> +
> +	pm_runtime_enable(&pdev->dev);
> +	err = pm_runtime_get_sync(&pdev->dev);

   That's all nice but where's the code that accesses the actual registers?

> +	if (err < 0) {
> +		pm_runtime_put_noidle(&pdev->dev);

   Calling "put" with previously failed "get" sees strange...

> +		return err;
> +	}
> +
> +	priv->hbdev.needs_calib = true;
> +	priv->hbdev.dev = &pdev->dev;
> +	priv->hbdev.np = of_get_next_child(dev->of_node, NULL);
> +	err = hb_register_device(&priv->hbdev);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to register controller\n");
> +		goto err_destroy;
> +	}
> +
> +	return 0;
> +
> +err_destroy:
> +	hb_unregister_device(&priv->hbdev);

   Calling "unregister" with "register" previously failed also looks strange...

> +	pm_runtime_put_sync(&pdev->dev);

   Why the sync() version?

> +	return err;
> +}
[...]

MBR, Sergei
Vignesh Raghavendra Feb. 26, 2019, 10:18 a.m. UTC | #3 
On 25/02/19 9:59 PM, Sergei Shtylyov wrote:
> Hello!
> 
> On 02/19/2019 09:36 AM, Vignesh R (by way of Boris Brezillon <bbrezillon@kernel.org>) wrote:
> 
>> Add driver for Hyperbus memory controller on TI's AM654 SoC. Programming
>> IP is pretty simple and provides direct memory mapped access to
>> connected Flash devices.
> 
>    Are you sure you posted the _complete_ driver?
> 


Yes, it is... You can find controller doc here[1]. Default values in the
MCR/MTR registers are good enough to simple Hyperflash access.
More perf optimization and timing optimizations will come incrementally

[1] http://www.ti.com/lit/ug/spruid7b/spruid7b.pdf 12.3.3 Hyperbus Interface

>> Add basic support for the IP without DMA. Second ChipSelect is not
>> supported for now.
>>
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>> ---
>>  drivers/mtd/hyperbus/hbmc_am654.c | 105 ++++++++++++++++++++++++++++++
>>  1 file changed, 105 insertions(+)
>>  create mode 100644 drivers/mtd/hyperbus/hbmc_am654.c
>>
>> diff --git a/drivers/mtd/hyperbus/hbmc_am654.c b/drivers/mtd/hyperbus/hbmc_am654.c
>> new file mode 100644
>> index 000000000000..1f0d2dc52f9f
>> --- /dev/null
>> +++ b/drivers/mtd/hyperbus/hbmc_am654.c
>> @@ -0,0 +1,105 @@
> [...]
>> +static int am654_hbmc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct am654_hbmc_priv *priv;
>> +	struct resource *res;
>> +	int err;
>> +
>> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, priv);
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (IS_ERR(res)) {
>> +		dev_err(&pdev->dev, "failed to get memory resource\n");
>> +		return -ENOENT;
>> +	}
>> +
>> +	priv->regbase = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(priv->regbase)) {
>> +		dev_err(dev, "Cannot remap controller address.\n");
>> +		return PTR_ERR(priv->regbase);
>> +	}
>> +
>> +	pm_runtime_enable(&pdev->dev);
>> +	err = pm_runtime_get_sync(&pdev->dev);
> 
>    That's all nice but where's the code that accesses the actual registers?

Interface and functional clk needs to be enabled even to access MMIO
space to read/write data to flash (done by the map framework). So driver
currently just enables everything during probe and disables on remove

> 
>> +	if (err < 0) {
>> +		pm_runtime_put_noidle(&pdev->dev);
> 
>    Calling "put" with previously failed "get" sees strange...
> 

Basically pm_runtime_get_sync() increments usage_count even in case of
failure and pm_runtime_put_noidle() puts it back. You can find many
examples of above piece of code in kernel.

>> +		return err;
>> +	}
>> +
>> +	priv->hbdev.needs_calib = true;
>> +	priv->hbdev.dev = &pdev->dev;
>> +	priv->hbdev.np = of_get_next_child(dev->of_node, NULL);
>> +	err = hb_register_device(&priv->hbdev);
>> +	if (err) {
>> +		dev_err(&pdev->dev, "failed to register controller\n");
>> +		goto err_destroy;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_destroy:
>> +	hb_unregister_device(&priv->hbdev);
> 
>    Calling "unregister" with "register" previously failed also looks strange...
> 

Agree, this is unneeded as hb_register_device() takes care of all
cleanups in err path.

>> +	pm_runtime_put_sync(&pdev->dev);
> 
>    Why the sync() version?
> 

Why not? Since the device is going away, I think its safer to ensure
device has definitely been put to idle state. I see its a common
practice in driver code.

>> +	return err;
>> +}
> [...]
> 
> MBR, Sergei
>
Sergei Shtylyov Feb. 26, 2019, 6:28 p.m. UTC | #4 
On 02/19/2019 09:36 AM, Vignesh R (by way of Boris Brezillon <bbrezillon@kernel.org>) wrote:

> Add driver for Hyperbus memory controller on TI's AM654 SoC. Programming
> IP is pretty simple and provides direct memory mapped access to
> connected Flash devices.
> 
> Add basic support for the IP without DMA. Second ChipSelect is not
> supported for now.
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
>  drivers/mtd/hyperbus/hbmc_am654.c | 105 ++++++++++++++++++++++++++++++
>  1 file changed, 105 insertions(+)
>  create mode 100644 drivers/mtd/hyperbus/hbmc_am654.c
> 
> diff --git a/drivers/mtd/hyperbus/hbmc_am654.c b/drivers/mtd/hyperbus/hbmc_am654.c
> new file mode 100644
> index 000000000000..1f0d2dc52f9f
> --- /dev/null
> +++ b/drivers/mtd/hyperbus/hbmc_am654.c
> @@ -0,0 +1,105 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> +// Author: Vignesh R <vigneshr@ti.com>
> +
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mtd/hyperbus.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/types.h>
> +
> +struct am654_hbmc_priv {
> +	struct hb_device hbdev;
> +	void __iomem	*regbase;

   Isn't regbase a controller's data, not a slave device's?

[...]

MBR, Sergei
diff mbox series

Patch

diff --git a/drivers/mtd/hyperbus/hbmc_am654.c b/drivers/mtd/hyperbus/hbmc_am654.c
new file mode 100644
index 000000000000..1f0d2dc52f9f
--- /dev/null
+++ b/drivers/mtd/hyperbus/hbmc_am654.c
@@ -0,0 +1,105 @@ 
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
+// Author: Vignesh R <vigneshr@ti.com>
+
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mtd/hyperbus.h>
+#include <linux/mtd/mtd.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/types.h>
+
+struct am654_hbmc_priv {
+	struct hb_device hbdev;
+	void __iomem	*regbase;
+};
+
+static int am654_hbmc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct am654_hbmc_priv *priv;
+	struct resource *res;
+	int err;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, priv);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (IS_ERR(res)) {
+		dev_err(&pdev->dev, "failed to get memory resource\n");
+		return -ENOENT;
+	}
+
+	priv->regbase = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->regbase)) {
+		dev_err(dev, "Cannot remap controller address.\n");
+		return PTR_ERR(priv->regbase);
+	}
+
+	pm_runtime_enable(&pdev->dev);
+	err = pm_runtime_get_sync(&pdev->dev);
+	if (err < 0) {
+		pm_runtime_put_noidle(&pdev->dev);
+		return err;
+	}
+
+	priv->hbdev.needs_calib = true;
+	priv->hbdev.dev = &pdev->dev;
+	priv->hbdev.np = of_get_next_child(dev->of_node, NULL);
+	err = hb_register_device(&priv->hbdev);
+	if (err) {
+		dev_err(&pdev->dev, "failed to register controller\n");
+		goto err_destroy;
+	}
+
+	return 0;
+
+err_destroy:
+	hb_unregister_device(&priv->hbdev);
+	pm_runtime_put_sync(&pdev->dev);
+	return err;
+}
+
+static int am654_hbmc_remove(struct platform_device *pdev)
+{
+	struct am654_hbmc_priv *priv = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = hb_unregister_device(&priv->hbdev);
+	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+
+	return ret;
+}
+
+static const struct of_device_id am654_hbmc_dt_ids[] = {
+	{
+		.compatible = "ti,am654-hbmc",
+	},
+	{ /* end of table */ }
+};
+
+MODULE_DEVICE_TABLE(of, am654_hbmc_dt_ids);
+
+static struct platform_driver am654_hbmc_platform_driver = {
+	.probe = am654_hbmc_probe,
+	.remove = am654_hbmc_remove,
+	.driver = {
+		.name = "hbmc-am654",
+		.of_match_table = am654_hbmc_dt_ids,
+	},
+};
+
+module_platform_driver(am654_hbmc_platform_driver);
+
+MODULE_DESCRIPTION("HBMC driver for AM654 SoC");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:hbmc-am654");
+MODULE_AUTHOR("Vignesh R <vigneshr@ti.com>");