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@xxxxxxxxxx>) 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@xxxxxx> >> --- >> 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 > -- Regards Vignesh
