Re: DENALI: can't detect NAND chip

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