DENALI: can't detect NAND chip

DENALI: can't detect NAND chip

[Philipp Rosenberger]

Hi,

I'm currently porting Linux 4.14 to a Altera Cyclone V based board. The
Denali NAND Driver isn't able to detect the NAND chip Micron MT29F4G08) correctly.

[    0.862242] nand: device found, Manufacturer ID: 0x00, Chip ID: 0x2c
[    0.868604] nand: Unknown denali-nand
[    0.872253] nand: bus width 8 instead of 16 bits
[    0.876899] nand: No NAND device found
[    0.880688] denali-nand-dt: probe of ff900000.nand failed with error -22

The correct ManufID is 0x2c and the DevID is 0xdc. Currently we are
running a 4.1 kernel with the "old" denali driver without this problem.
While researching the issue I found the following Mail in the
Rocketboards archive, which seems to be the same issue.

https://lists.rocketboards.org/pipermail/rfi/2017-September/003627.html


We also tested Linux 4.16-rc5 without any additional patches with the same behavior.

Best regards,
Philipp

Re: DENALI: can't detect NAND chip

[Masahiro Yamada]

2018-03-12 23:29 GMT+09:00 Philipp Rosenberger <p.rosenberger@linutronix.de>:
> Hi,
>
> I'm currently porting Linux 4.14 to a Altera Cyclone V based board. The
> Denali NAND Driver isn't able to detect the NAND chip Micron MT29F4G08) correctly.
>
> [    0.862242] nand: device found, Manufacturer ID: 0x00, Chip ID: 0x2c
> [    0.868604] nand: Unknown denali-nand
> [    0.872253] nand: bus width 8 instead of 16 bits
> [    0.876899] nand: No NAND device found
> [    0.880688] denali-nand-dt: probe of ff900000.nand failed with error -22
>
> The correct ManufID is 0x2c and the DevID is 0xdc. Currently we are
> running a 4.1 kernel with the "old" denali driver without this problem.
> While researching the issue I found the following Mail in the
> Rocketboards archive, which seems to be the same issue.
>
> https://lists.rocketboards.org/pipermail/rfi/2017-September/003627.html
>
>
> We also tested Linux 4.16-rc5 without any additional patches with the same behavior.
>


I got a similar report before for SOCFPGA.

The reporter found a problem in his clock setting:
http://linux-mtd.infradead.narkive.com/MMBF5wzm/nand-denali-issue-with-4-13



-- 
Best Regards
Masahiro Yamada

Re: DENALI: can't detect NAND chip

[Richard Weinberger]

On Tue, Mar 13, 2018 at 9:48 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> 2018-03-12 23:29 GMT+09:00 Philipp Rosenberger <p.rosenberger@linutronix.de>:
>> Hi,
>>
>> I'm currently porting Linux 4.14 to a Altera Cyclone V based board. The
>> Denali NAND Driver isn't able to detect the NAND chip Micron MT29F4G08) correctly.
>>
>> [    0.862242] nand: device found, Manufacturer ID: 0x00, Chip ID: 0x2c
>> [    0.868604] nand: Unknown denali-nand
>> [    0.872253] nand: bus width 8 instead of 16 bits
>> [    0.876899] nand: No NAND device found
>> [    0.880688] denali-nand-dt: probe of ff900000.nand failed with error -22
>>
>> The correct ManufID is 0x2c and the DevID is 0xdc. Currently we are
>> running a 4.1 kernel with the "old" denali driver without this problem.
>> While researching the issue I found the following Mail in the
>> Rocketboards archive, which seems to be the same issue.
>>
>> https://lists.rocketboards.org/pipermail/rfi/2017-September/003627.html
>>
>>
>> We also tested Linux 4.16-rc5 without any additional patches with the same behavior.
>>
>
>
> I got a similar report before for SOCFPGA.
>
> The reporter found a problem in his clock setting:
> http://linux-mtd.infradead.narkive.com/MMBF5wzm/nand-denali-issue-with-4-13

I got access to that board and did initial debugging of the problem.

The problem seems to be that after your rework the NAND timings get
derived from the NAND clock.
When I make denali_setup_data_interface() a NOP, the NAND is being
detected and works.

Can it be that the function does not calculate the timings correctly
in all cases?
Please see the following debug output for good and bad case.

bad case:
[    0.946757] XXX: denali clk rate: 0x2faf080
[    0.950932] XXX: denali clk phase: 0x0
[    0.954671] XXX: denali clk accuracy: 0x0
[    0.958920] denali-nand-dt ff900000.nand: clk_x_rate: 0x2faf080
[    0.964846] denali-nand-dt ff900000.nand: Dump timing register values:
[    0.964846] acc_clks: 2, re_2_we: 10, re_2_re: 10
[    0.964846] we_2_re: 25, addr_2_data: 20, rdwr_en_lo_cnt: 4
[    0.964846] rdwr_en_hi_cnt: 2, cs_setup_cnt: 3

good case:
[    0.961061] XXX: denali clk rate: 0x2faf080
[    0.965231] XXX: denali clk phase: 0x0
[    0.968967] XXX: denali clk accuracy: 0x0
[    0.973031] denali-nand-dt ff900000.nand: Dump timing register values:
[    0.973031] acc_clks: 4, re_2_we: 20, re_2_re: 20
[    0.973031] we_2_re: 12, addr_2_data: 14, rdwr_en_lo_cnt: 2
[    0.973031] rdwr_en_hi_cnt: 2, cs_setup_cnt: 2

Does this ring a bell?

-- 
Thanks,
//richard

Re: DENALI: can't detect NAND chip

[Boris Brezillon]

On Mon, 4 Jun 2018 21:58:54 +0200
Richard Weinberger <richard.weinberger@gmail.com> wrote:

> On Tue, Mar 13, 2018 at 9:48 AM, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> > 2018-03-12 23:29 GMT+09:00 Philipp Rosenberger <p.rosenberger@linutronix.de>:  
> >> Hi,
> >>
> >> I'm currently porting Linux 4.14 to a Altera Cyclone V based board. The
> >> Denali NAND Driver isn't able to detect the NAND chip Micron MT29F4G08) correctly.
> >>
> >> [    0.862242] nand: device found, Manufacturer ID: 0x00, Chip ID: 0x2c
> >> [    0.868604] nand: Unknown denali-nand
> >> [    0.872253] nand: bus width 8 instead of 16 bits
> >> [    0.876899] nand: No NAND device found
> >> [    0.880688] denali-nand-dt: probe of ff900000.nand failed with error -22
> >>
> >> The correct ManufID is 0x2c and the DevID is 0xdc. Currently we are
> >> running a 4.1 kernel with the "old" denali driver without this problem.
> >> While researching the issue I found the following Mail in the
> >> Rocketboards archive, which seems to be the same issue.
> >>
> >> https://lists.rocketboards.org/pipermail/rfi/2017-September/003627.html
> >>
> >>
> >> We also tested Linux 4.16-rc5 without any additional patches with the same behavior.
> >>  
> >
> >
> > I got a similar report before for SOCFPGA.
> >
> > The reporter found a problem in his clock setting:
> > http://linux-mtd.infradead.narkive.com/MMBF5wzm/nand-denali-issue-with-4-13  
> 
> I got access to that board and did initial debugging of the problem.
> 
> The problem seems to be that after your rework the NAND timings get
> derived from the NAND clock.
> When I make denali_setup_data_interface() a NOP, the NAND is being
> detected and works.
> 
> Can it be that the function does not calculate the timings correctly
> in all cases?
> Please see the following debug output for good and bad case.
> 
> bad case:
> [    0.946757] XXX: denali clk rate: 0x2faf080
> [    0.950932] XXX: denali clk phase: 0x0
> [    0.954671] XXX: denali clk accuracy: 0x0
> [    0.958920] denali-nand-dt ff900000.nand: clk_x_rate: 0x2faf080
> [    0.964846] denali-nand-dt ff900000.nand: Dump timing register values:
> [    0.964846] acc_clks: 2, re_2_we: 10, re_2_re: 10
> [    0.964846] we_2_re: 25, addr_2_data: 20, rdwr_en_lo_cnt: 4
> [    0.964846] rdwr_en_hi_cnt: 2, cs_setup_cnt: 3
> 
> good case:
> [    0.961061] XXX: denali clk rate: 0x2faf080
> [    0.965231] XXX: denali clk phase: 0x0
> [    0.968967] XXX: denali clk accuracy: 0x0
> [    0.973031] denali-nand-dt ff900000.nand: Dump timing register values:
> [    0.973031] acc_clks: 4, re_2_we: 20, re_2_re: 20
> [    0.973031] we_2_re: 12, addr_2_data: 14, rdwr_en_lo_cnt: 2
> [    0.973031] rdwr_en_hi_cnt: 2, cs_setup_cnt: 2
> 
> Does this ring a bell?
> 

Could it be that clk_get_rate() returns an invalid value? Looks like
all good timings are almost equal to bad timings multiplied by 2, so
maybe the ->recalc_rate() method of clk driver returns freq / 2 instead
of freq.

Re: DENALI: can't detect NAND chip

[Richard Weinberger]

Am Montag, 4. Juni 2018, 22:34:53 CEST schrieb Boris Brezillon:
> > I got access to that board and did initial debugging of the problem.
> > 
> > The problem seems to be that after your rework the NAND timings get
> > derived from the NAND clock.
> > When I make denali_setup_data_interface() a NOP, the NAND is being
> > detected and works.
> > 
> > Can it be that the function does not calculate the timings correctly
> > in all cases?
> > Please see the following debug output for good and bad case.
> > 
> > bad case:
> > [    0.946757] XXX: denali clk rate: 0x2faf080
> > [    0.950932] XXX: denali clk phase: 0x0
> > [    0.954671] XXX: denali clk accuracy: 0x0
> > [    0.958920] denali-nand-dt ff900000.nand: clk_x_rate: 0x2faf080
> > [    0.964846] denali-nand-dt ff900000.nand: Dump timing register values:
> > [    0.964846] acc_clks: 2, re_2_we: 10, re_2_re: 10
> > [    0.964846] we_2_re: 25, addr_2_data: 20, rdwr_en_lo_cnt: 4
> > [    0.964846] rdwr_en_hi_cnt: 2, cs_setup_cnt: 3
> > 
> > good case:
> > [    0.961061] XXX: denali clk rate: 0x2faf080
> > [    0.965231] XXX: denali clk phase: 0x0
> > [    0.968967] XXX: denali clk accuracy: 0x0
> > [    0.973031] denali-nand-dt ff900000.nand: Dump timing register values:
> > [    0.973031] acc_clks: 4, re_2_we: 20, re_2_re: 20
> > [    0.973031] we_2_re: 12, addr_2_data: 14, rdwr_en_lo_cnt: 2
> > [    0.973031] rdwr_en_hi_cnt: 2, cs_setup_cnt: 2
> > 
> > Does this ring a bell?
> > 
> 
> Could it be that clk_get_rate() returns an invalid value? Looks like
> all good timings are almost equal to bad timings multiplied by 2, so
> maybe the ->recalc_rate() method of clk driver returns freq / 2 instead
> of freq.

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. :)

Thanks,
//richard

-- 
sigma star gmbh - Eduard-Bodem-Gasse 6 - 6020 Innsbruck - Austria
ATU66964118 - FN 374287y

Re: DENALI: can't detect NAND chip

[Boris Brezillon]

On Mon, 04 Jun 2018 22:41:40 +0200
Richard Weinberger <richard@sigma-star.at> wrote:

> Am Montag, 4. Juni 2018, 22:34:53 CEST schrieb Boris Brezillon:
> > > I got access to that board and did initial debugging of the problem.
> > > 
> > > The problem seems to be that after your rework the NAND timings get
> > > derived from the NAND clock.
> > > When I make denali_setup_data_interface() a NOP, the NAND is being
> > > detected and works.
> > > 
> > > Can it be that the function does not calculate the timings correctly
> > > in all cases?
> > > Please see the following debug output for good and bad case.
> > > 
> > > bad case:
> > > [    0.946757] XXX: denali clk rate: 0x2faf080
> > > [    0.950932] XXX: denali clk phase: 0x0
> > > [    0.954671] XXX: denali clk accuracy: 0x0
> > > [    0.958920] denali-nand-dt ff900000.nand: clk_x_rate: 0x2faf080
> > > [    0.964846] denali-nand-dt ff900000.nand: Dump timing register values:
> > > [    0.964846] acc_clks: 2, re_2_we: 10, re_2_re: 10
> > > [    0.964846] we_2_re: 25, addr_2_data: 20, rdwr_en_lo_cnt: 4
> > > [    0.964846] rdwr_en_hi_cnt: 2, cs_setup_cnt: 3
> > > 
> > > good case:
> > > [    0.961061] XXX: denali clk rate: 0x2faf080
> > > [    0.965231] XXX: denali clk phase: 0x0
> > > [    0.968967] XXX: denali clk accuracy: 0x0
> > > [    0.973031] denali-nand-dt ff900000.nand: Dump timing register values:
> > > [    0.973031] acc_clks: 4, re_2_we: 20, re_2_re: 20
> > > [    0.973031] we_2_re: 12, addr_2_data: 14, rdwr_en_lo_cnt: 2
> > > [    0.973031] rdwr_en_hi_cnt: 2, cs_setup_cnt: 2
> > > 
> > > Does this ring a bell?
> > >   
> > 
> > Could it be that clk_get_rate() returns an invalid value? Looks like
> > all good timings are almost equal to bad timings multiplied by 2, so
> > maybe the ->recalc_rate() method of clk driver returns freq / 2 instead
> > of freq.  
> 
> 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.

Re: DENALI: can't detect NAND chip

[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.

Thanks,
//richard

-- 
sigma star gmbh - Eduard-Bodem-Gasse 6 - 6020 Innsbruck - Austria
ATU66964118 - FN 374287y

Re: DENALI: can't detect NAND chip

[Richard Weinberger]

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.

Thanks,
//richard

-- 
sigma star gmbh - Eduard-Bodem-Gasse 6 - 6020 Innsbruck - Austria
ATU66964118 - FN 374287y

Re: DENALI: can't detect NAND chip

[Masahiro Yamada]

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";
clocks = <...>, <...>, <...>;


This might be an annoying churn, though...


-- 
Best Regards
Masahiro Yamada

Re: DENALI: can't detect NAND chip

[Boris Brezillon]

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;

Re: DENALI: can't detect NAND chip

[Richard Weinberger]

Am Dienstag, 5. Juni 2018, 09:36:27 CEST schrieb Boris Brezillon:
> On Tue, 5 Jun 2018 10:43:49 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> > 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).

I agree this makes sense and unbreaks existing users that upgrade their kernels.

> --->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;

We should emit a message when a hardcored value is used.

Thanks,
//richard

-- 
sigma star gmbh - Eduard-Bodem-Gasse 6 - 6020 Innsbruck - Austria
ATU66964118 - FN 374287y

Re: DENALI: can't detect NAND chip

[Richard Weinberger]

Am Dienstag, 5. Juni 2018, 09:36:27 CEST schrieb Boris Brezillon:
> 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).

[...]

Masahiro, which fix do you prefer?

We could also just fix the dts file.
e.g.
commit c0097d0b498bdefb427c352d615432e97f1998aa
Author: Richard Weinberger <richard@nod.at>
Date:   Tue Jun 12 11:16:22 2018 +0200

    arm: dts: socfpga: Use right denali clock
    
    To calculate the timing parameters a 200MHz clock is needed, in case of
    socfpga this is nand_x_clk and not nand_clk.
    
    Cc: stable@kernel.org
    Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by setup_data_interface()")
    Signed-off-by: Richard Weinberger <richard@nod.at>

diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 486d4e7433ed..8fbddc2a5c33 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -754,7 +754,7 @@
 			reg-names = "nand_data", "denali_reg";
 			interrupts = <0x0 0x90 0x4>;
 			dma-mask = <0xffffffff>;
-			clocks = <&nand_clk>;
+			clocks = <&nand_x_clk>;
 			status = "disabled";
 		};
 
Thanks,
//richard

-- 
sigma star gmbh - Eduard-Bodem-Gasse 6 - 6020 Innsbruck - Austria
ATU66964118 - FN 374287y

Re: DENALI: can't detect NAND chip

[Masahiro Yamada]

2018-06-12 18:21 GMT+09:00 Richard Weinberger <richard@sigma-star.at>:
> Am Dienstag, 5. Juni 2018, 09:36:27 CEST schrieb Boris Brezillon:
>> 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).
>
> [...]
>
> Masahiro, which fix do you prefer?
>
> We could also just fix the dts file.
> e.g.
> commit c0097d0b498bdefb427c352d615432e97f1998aa
> Author: Richard Weinberger <richard@nod.at>
> Date:   Tue Jun 12 11:16:22 2018 +0200
>
>     arm: dts: socfpga: Use right denali clock
>
>     To calculate the timing parameters a 200MHz clock is needed, in case of
>     socfpga this is nand_x_clk and not nand_clk.
>
>     Cc: stable@kernel.org
>     Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by setup_data_interface()")
>     Signed-off-by: Richard Weinberger <richard@nod.at>
>
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index 486d4e7433ed..8fbddc2a5c33 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -754,7 +754,7 @@
>                         reg-names = "nand_data", "denali_reg";
>                         interrupts = <0x0 0x90 0x4>;
>                         dma-mask = <0xffffffff>;
> -                       clocks = <&nand_clk>;
> +                       clocks = <&nand_x_clk>;
>                         status = "disabled";
>                 };
>
> Thanks,
> //richard
>


I almost finishing the denali driver update.
I am now writing commit log.   Will post it soon.

(By taking care of the backward compatibility in the driver,
you do not need to rush to fix your DT.)



-- 
Best Regards
Masahiro Yamada

Re: DENALI: can't detect NAND chip

[Richard Weinberger]

Am Dienstag, 12. Juni 2018, 11:24:54 CEST schrieb Masahiro Yamada:
> I almost finishing the denali driver update.
> I am now writing commit log.   Will post it soon.
> 
> (By taking care of the backward compatibility in the driver,
> you do not need to rush to fix your DT.)

Awesome!

Thanks,
//richard

-- 
sigma star gmbh - Eduard-Bodem-Gasse 6 - 6020 Innsbruck - Austria
ATU66964118 - FN 374287y

Re: DENALI: can't detect NAND chip

[Boris Brezillon]

On Tue, 12 Jun 2018 11:21:03 +0200
Richard Weinberger <richard@sigma-star.at> wrote:

> Am Dienstag, 5. Juni 2018, 09:36:27 CEST schrieb Boris Brezillon:
> > 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).  
> 
> [...]
> 
> Masahiro, which fix do you prefer?

I prefer the solution where we properly describe all the clks that are
needed by the NAND controller in the DT + the workaround to still
support existing DTs.

> 
> We could also just fix the dts file.
> e.g.
> commit c0097d0b498bdefb427c352d615432e97f1998aa
> Author: Richard Weinberger <richard@nod.at>
> Date:   Tue Jun 12 11:16:22 2018 +0200
> 
>     arm: dts: socfpga: Use right denali clock
>     
>     To calculate the timing parameters a 200MHz clock is needed, in case of
>     socfpga this is nand_x_clk and not nand_clk.
>     
>     Cc: stable@kernel.org
>     Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by setup_data_interface()")
>     Signed-off-by: Richard Weinberger <richard@nod.at>
> 
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index 486d4e7433ed..8fbddc2a5c33 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -754,7 +754,7 @@
>  			reg-names = "nand_data", "denali_reg";
>  			interrupts = <0x0 0x90 0x4>;
>  			dma-mask = <0xffffffff>;
> -			clocks = <&nand_clk>;
> +			clocks = <&nand_x_clk>;
>  			status = "disabled";
>  		};
>  
> Thanks,
> //richard
>