Re: DENALI: can't detect NAND chip

Boris Brezillon
2018-06-05T07:36:52Z

On Tue, 5 Jun 2018 10:43:49 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> 2018-06-05 7:01 GMT+09:00 Richard Weinberger <richard@sigma-star.at>:
> > Am Montag, 4. Juni 2018, 22:57:32 CEST schrieb Richard Weinberger:
> >> Am Montag, 4. Juni 2018, 22:51:17 CEST schrieb Boris Brezillon:
> >> > > According to the datasheet, the NAND clock rate should be 50Mhz, which is what
> >> > > is returned in both cases.
> >> > > So this does not really look invalid to me. :)
> >> >
> >> > Did you try to put a scope on the RE or WE pin? The x2 factor looks too
> >> > perfect to be just a coincidence.
> >>
> >> Not yet, but you are right the x2 factor is really interesting.
> >
> > The NFC uses two clocks, nand_x_clk and nand_clk.
> > nand_clk is nand_x_clk / 4.
> > In the device tree nand_clk is referenced, and therefore used to calculate
> > the timings. So we might need to use the 200Mhz clock instead of the 50Mhz
> > for the calculation.
> >
>
>
> Yes.
>
>
> Strictly speaking, the Denali IP takes three clocks.
>
> [1] clk : core clock
> [2] clk_x: bus interface clock
> [3] ecc_clk: for ECC engine
>
>
>
> Also in my SoCs (Socionext UniPhier),
>
> clk = 50MHz,
> clk_x = ecc_clk = 200MHz
>
>
>
> In this case, the ->setup_data_interface hook
> is interested in the frequency of [2] clk_x.
>
> However, clk_x is not a core clock.
> I admit this is confusing.
>
>
> If we really want to make the DT-binding precise,
> we can change the clock requirement like follows:
>
> clock-names = "clk", "clk_x", "ecc_clk";

How about:

clock-names = "nand", "nand_x", "ecc";

The clk prefix/suffix is really redundant here.

> clocks = <...>, <...>, <...>;

If the IP really takes 3 different clks in input, then it should be
represented like that.

>
>
> This might be an annoying churn, though...
>
>

I guess you're talking about backward compat with existing dtb. This
should be a problem, and we can even fix the Richard's problem for
those dtbs if we hardcode the nand_x_clk rate to 200Mhz when nand_x_clk
is missing (see below).

--->8---
diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
index cfd33e6ca77f..91ee1ce1843a 100644
--- a/drivers/mtd/nand/raw/denali_dt.c
+++ b/drivers/mtd/nand/raw/denali_dt.c
@@ -28,6 +28,8 @@
struct denali_dt {
struct denali_nand_info denali;
struct clk *clk;
+ struct clk *x_clk;
+ struct clk *ecc_clk;
};

struct denali_dt_data {
@@ -114,24 +116,56 @@ static int denali_dt_probe(struct platform_device *pdev)
if (IS_ERR(denali->host))
return PTR_ERR(denali->host);

- dt->clk = devm_clk_get(&pdev->dev, NULL);
+ dt->clk = devm_clk_get(&pdev->dev, "nand");
+ if (IS_ERR(dt->clk))
+ dt->clk = devm_clk_get(&pdev->dev, NULL);
if (IS_ERR(dt->clk)) {
dev_err(&pdev->dev, "no clk available\n");
return PTR_ERR(dt->clk);
}
+
+ dt->x_clk = devm_clk_get(&pdev->dev, "nand_x");
+ if (IS_ERR(dt->x_clk))
+ dt->x_clk = NULL;
+
+ dt->ecc_clk = devm_clk_get(&pdev->dev, "ecc");
+ if (IS_ERR(dt->ecc_clk))
+ dt->ecc_clk = NULL;
+
ret = clk_prepare_enable(dt->clk);
if (ret)
return ret;

- denali->clk_x_rate = clk_get_rate(dt->clk);
+ ret = clk_prepare_enable(dt->x_clk);
+ if (ret)
+ goto out_disable_clk;
+
+ ret = clk_prepare_enable(dt->ecc_clk);
+ if (ret)
+ goto out_disable_x_clk;
+
+ /*
+ * Hardcode clk_x_rate to 200Mhz when ->x_clk is missing, as was done
+ * by the driver before Linux 4.13.
+ */
+ if (dt->x_clk)
+ denali->clk_x_rate = clk_get_rate(dt->x_clk);
+ else
+ denali->clk_x_rate = 200000000;

ret = denali_init(denali);
if (ret)
- goto out_disable_clk;
+ goto out_disable_ecc_clk;

platform_set_drvdata(pdev, dt);
return 0;

+out_disable_ecc_clk:
+ clk_disable_unprepare(dt->ecc_clk);
+
+out_disable_x_clk:
+ clk_disable_unprepare(dt->x_clk);
+
out_disable_clk:
clk_disable_unprepare(dt->clk);

@@ -143,6 +177,8 @@ static int denali_dt_remove(struct platform_device *pdev)
struct denali_dt *dt = platform_get_drvdata(pdev);

denali_remove(&dt->denali);
+ clk_disable_unprepare(dt->ecc_clk);
+ clk_disable_unprepare(dt->x_clk);
clk_disable_unprepare(dt->clk);

return 0;