| Message ID | 20190219063607.29949-6-vigneshr@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MTD: Add Initial Hyperbus support | expand |
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
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
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 >
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 --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>");
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