Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet
2018-06-22 20:42 GMT+09:00 Boris Brezillon :
> Hi Masahiro,
>
> On Tue, 19 Jun 2018 13:28:11 +0200
> Boris Brezillon wrote:
>
>> Hi Masahiro,
>>
>> On Fri, 15 Jun 2018 10:18:50 +0900
>> Masahiro Yamada wrote:
>>
>> > According to the Denali User's Guide, this IP needs three clocks:
>> >
>> > - clk: controller core clock
>> >
>> > - clk_x: bus interface clock
>> >
>> > - ecc_clk: clock at which ECC circuitry is run
>> >
>> > Currently, denali_dt.c requires a single anonymous clock and its
>> > frequency. However, the driver needs to get the frequency of "clk_x"
>> > not "clk". This is confusing because people tend to assume the
>> > anonymous clock means the core clock. In fact, I got a report of
>> > SOCFPGA breakage because the timing parameters are calculated based
>> > on a wrong frequency.
>> >
>> > Instead of the cheesy implementation, the clocks in the real hardware
>> > should be represented in the driver and the DT-binding.
>> >
>> > However, adding new clocks would break the existing platforms. For the
>> > backward compatibility, the driver still accepts a single clock just as
>> > before. If clk_x is missing, clk_x_rate is set to a hardcoded value.
>> > This is fine for existing DT of Socionext UniPhier, and also fixes the
>> > issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
>> > the bus interface clock.
>> >
>> > Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by
>> > setup_data_interface()")
>>
>> Sorry for changing my mind, but I think this patch is doing more than
>> just fixing a bug. The bugfix should IMO be limited to unconditionally
>> setting ->clk_x_rate to 2 since this is what was done before
>> commit 1bb88666775e. And the remaining changes should go in nand/next.
>
> Do you want me to write this patch? Note that I can't really take this
> patch since Rob asked for a new version with DT bindings changes placed
> in a separate patch.
Sorry for the delay. I was traveling.
Now I am back home, and started to flush my TODO queue.
V4 will be soon.
--
Best Regards
Masahiro Yamada
Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet
2018-06-22 20:42 GMT+09:00 Boris Brezillon :
> Hi Masahiro,
>
> On Tue, 19 Jun 2018 13:28:11 +0200
> Boris Brezillon wrote:
>
>> Hi Masahiro,
>>
>> On Fri, 15 Jun 2018 10:18:50 +0900
>> Masahiro Yamada wrote:
>>
>> > According to the Denali User's Guide, this IP needs three clocks:
>> >
>> > - clk: controller core clock
>> >
>> > - clk_x: bus interface clock
>> >
>> > - ecc_clk: clock at which ECC circuitry is run
>> >
>> > Currently, denali_dt.c requires a single anonymous clock and its
>> > frequency. However, the driver needs to get the frequency of "clk_x"
>> > not "clk". This is confusing because people tend to assume the
>> > anonymous clock means the core clock. In fact, I got a report of
>> > SOCFPGA breakage because the timing parameters are calculated based
>> > on a wrong frequency.
>> >
>> > Instead of the cheesy implementation, the clocks in the real hardware
>> > should be represented in the driver and the DT-binding.
>> >
>> > However, adding new clocks would break the existing platforms. For the
>> > backward compatibility, the driver still accepts a single clock just as
>> > before. If clk_x is missing, clk_x_rate is set to a hardcoded value.
>> > This is fine for existing DT of Socionext UniPhier, and also fixes the
>> > issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
>> > the bus interface clock.
>> >
>> > Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by
>> > setup_data_interface()")
>>
>> Sorry for changing my mind, but I think this patch is doing more than
>> just fixing a bug. The bugfix should IMO be limited to unconditionally
>> setting ->clk_x_rate to 2 since this is what was done before
>> commit 1bb88666775e. And the remaining changes should go in nand/next.
>
> Do you want me to write this patch? Note that I can't really take this
> patch since Rob asked for a new version with DT bindings changes placed
> in a separate patch.
Sorry for the delay. I was traveling.
Now I am back home, and started to flush my TODO queue.
V4 will be soon.
--
Best Regards
Masahiro Yamada
Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet
Hi Masahiro,
On Tue, 19 Jun 2018 13:28:11 +0200
Boris Brezillon wrote:
> Hi Masahiro,
>
> On Fri, 15 Jun 2018 10:18:50 +0900
> Masahiro Yamada wrote:
>
> > According to the Denali User's Guide, this IP needs three clocks:
> >
> > - clk: controller core clock
> >
> > - clk_x: bus interface clock
> >
> > - ecc_clk: clock at which ECC circuitry is run
> >
> > Currently, denali_dt.c requires a single anonymous clock and its
> > frequency. However, the driver needs to get the frequency of "clk_x"
> > not "clk". This is confusing because people tend to assume the
> > anonymous clock means the core clock. In fact, I got a report of
> > SOCFPGA breakage because the timing parameters are calculated based
> > on a wrong frequency.
> >
> > Instead of the cheesy implementation, the clocks in the real hardware
> > should be represented in the driver and the DT-binding.
> >
> > However, adding new clocks would break the existing platforms. For the
> > backward compatibility, the driver still accepts a single clock just as
> > before. If clk_x is missing, clk_x_rate is set to a hardcoded value.
> > This is fine for existing DT of Socionext UniPhier, and also fixes the
> > issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> > the bus interface clock.
> >
> > Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> > setup_data_interface()")
>
> Sorry for changing my mind, but I think this patch is doing more than
> just fixing a bug. The bugfix should IMO be limited to unconditionally
> setting ->clk_x_rate to 2 since this is what was done before
> commit 1bb88666775e. And the remaining changes should go in nand/next.
Do you want me to write this patch? Note that I can't really take this
patch since Rob asked for a new version with DT bindings changes placed
in a separate patch.
Regards,
Boris
Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet
Hi Masahiro,
On Tue, 19 Jun 2018 13:28:11 +0200
Boris Brezillon wrote:
> Hi Masahiro,
>
> On Fri, 15 Jun 2018 10:18:50 +0900
> Masahiro Yamada wrote:
>
> > According to the Denali User's Guide, this IP needs three clocks:
> >
> > - clk: controller core clock
> >
> > - clk_x: bus interface clock
> >
> > - ecc_clk: clock at which ECC circuitry is run
> >
> > Currently, denali_dt.c requires a single anonymous clock and its
> > frequency. However, the driver needs to get the frequency of "clk_x"
> > not "clk". This is confusing because people tend to assume the
> > anonymous clock means the core clock. In fact, I got a report of
> > SOCFPGA breakage because the timing parameters are calculated based
> > on a wrong frequency.
> >
> > Instead of the cheesy implementation, the clocks in the real hardware
> > should be represented in the driver and the DT-binding.
> >
> > However, adding new clocks would break the existing platforms. For the
> > backward compatibility, the driver still accepts a single clock just as
> > before. If clk_x is missing, clk_x_rate is set to a hardcoded value.
> > This is fine for existing DT of Socionext UniPhier, and also fixes the
> > issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> > the bus interface clock.
> >
> > Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> > setup_data_interface()")
>
> Sorry for changing my mind, but I think this patch is doing more than
> just fixing a bug. The bugfix should IMO be limited to unconditionally
> setting ->clk_x_rate to 2 since this is what was done before
> commit 1bb88666775e. And the remaining changes should go in nand/next.
Do you want me to write this patch? Note that I can't really take this
patch since Rob asked for a new version with DT bindings changes placed
in a separate patch.
Regards,
Boris
Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet
On Fri, Jun 15, 2018 at 10:18:50AM +0900, Masahiro Yamada wrote:
> According to the Denali User's Guide, this IP needs three clocks:
>
> - clk: controller core clock
>
> - clk_x: bus interface clock
>
> - ecc_clk: clock at which ECC circuitry is run
>
> Currently, denali_dt.c requires a single anonymous clock and its
> frequency. However, the driver needs to get the frequency of "clk_x"
> not "clk". This is confusing because people tend to assume the
> anonymous clock means the core clock. In fact, I got a report of
> SOCFPGA breakage because the timing parameters are calculated based
> on a wrong frequency.
>
> Instead of the cheesy implementation, the clocks in the real hardware
> should be represented in the driver and the DT-binding.
>
> However, adding new clocks would break the existing platforms. For the
> backward compatibility, the driver still accepts a single clock just as
> before. If clk_x is missing, clk_x_rate is set to a hardcoded value.
> This is fine for existing DT of Socionext UniPhier, and also fixes the
> issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> the bus interface clock.
>
> Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> setup_data_interface()")
> Cc: linux-stable #4.14+
> Reported-by: Richard Weinberger
> Signed-off-by: Masahiro Yamada
> ---
>
> Changes in v3:
> - Change the patch order so that the bug-fix one comes the first
>
> Changes in v2:
> - Split patches into sensible chunks
>
> .../devicetree/bindings/mtd/denali-nand.txt| 5 +++
> drivers/mtd/nand/raw/denali_dt.c | 49
> --
> 2 files changed, 50 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt
> b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> index 0ee8edb..f33da87 100644
> --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> @@ -8,6 +8,9 @@ Required properties:
>- reg : should contain registers location and length for data and reg.
>- reg-names: Should contain the reg names "nand_data" and "denali_reg"
>- interrupts : The interrupt number.
> + - clocks: should contain phandle of the controller core clock, the bus
> +interface clock, and the ECC circuit clock.
> + - clock-names: should contain "nand", "nand_x", "ecc"
The subject says "add more clocks", but it doesn't look like any clocks
were supported before. Or documentation was missing?
Please split bindings to a separate patch.
Rob
Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet
On Fri, Jun 15, 2018 at 10:18:50AM +0900, Masahiro Yamada wrote:
> According to the Denali User's Guide, this IP needs three clocks:
>
> - clk: controller core clock
>
> - clk_x: bus interface clock
>
> - ecc_clk: clock at which ECC circuitry is run
>
> Currently, denali_dt.c requires a single anonymous clock and its
> frequency. However, the driver needs to get the frequency of "clk_x"
> not "clk". This is confusing because people tend to assume the
> anonymous clock means the core clock. In fact, I got a report of
> SOCFPGA breakage because the timing parameters are calculated based
> on a wrong frequency.
>
> Instead of the cheesy implementation, the clocks in the real hardware
> should be represented in the driver and the DT-binding.
>
> However, adding new clocks would break the existing platforms. For the
> backward compatibility, the driver still accepts a single clock just as
> before. If clk_x is missing, clk_x_rate is set to a hardcoded value.
> This is fine for existing DT of Socionext UniPhier, and also fixes the
> issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> the bus interface clock.
>
> Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> setup_data_interface()")
> Cc: linux-stable #4.14+
> Reported-by: Richard Weinberger
> Signed-off-by: Masahiro Yamada
> ---
>
> Changes in v3:
> - Change the patch order so that the bug-fix one comes the first
>
> Changes in v2:
> - Split patches into sensible chunks
>
> .../devicetree/bindings/mtd/denali-nand.txt| 5 +++
> drivers/mtd/nand/raw/denali_dt.c | 49
> --
> 2 files changed, 50 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt
> b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> index 0ee8edb..f33da87 100644
> --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> @@ -8,6 +8,9 @@ Required properties:
>- reg : should contain registers location and length for data and reg.
>- reg-names: Should contain the reg names "nand_data" and "denali_reg"
>- interrupts : The interrupt number.
> + - clocks: should contain phandle of the controller core clock, the bus
> +interface clock, and the ECC circuit clock.
> + - clock-names: should contain "nand", "nand_x", "ecc"
The subject says "add more clocks", but it doesn't look like any clocks
were supported before. Or documentation was missing?
Please split bindings to a separate patch.
Rob
Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet
Hi Masahiro,
On Fri, 15 Jun 2018 10:18:50 +0900
Masahiro Yamada wrote:
> According to the Denali User's Guide, this IP needs three clocks:
>
> - clk: controller core clock
>
> - clk_x: bus interface clock
>
> - ecc_clk: clock at which ECC circuitry is run
>
> Currently, denali_dt.c requires a single anonymous clock and its
> frequency. However, the driver needs to get the frequency of "clk_x"
> not "clk". This is confusing because people tend to assume the
> anonymous clock means the core clock. In fact, I got a report of
> SOCFPGA breakage because the timing parameters are calculated based
> on a wrong frequency.
>
> Instead of the cheesy implementation, the clocks in the real hardware
> should be represented in the driver and the DT-binding.
>
> However, adding new clocks would break the existing platforms. For the
> backward compatibility, the driver still accepts a single clock just as
> before. If clk_x is missing, clk_x_rate is set to a hardcoded value.
> This is fine for existing DT of Socionext UniPhier, and also fixes the
> issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> the bus interface clock.
>
> Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> setup_data_interface()")
Sorry for changing my mind, but I think this patch is doing more than
just fixing a bug. The bugfix should IMO be limited to unconditionally
setting ->clk_x_rate to 2 since this is what was done before
commit 1bb88666775e. And the remaining changes should go in nand/next.
Regards,
Boris
> Cc: linux-stable #4.14+
> Reported-by: Richard Weinberger
> Signed-off-by: Masahiro Yamada
> ---
>
> Changes in v3:
> - Change the patch order so that the bug-fix one comes the first
>
> Changes in v2:
> - Split patches into sensible chunks
>
> .../devicetree/bindings/mtd/denali-nand.txt| 5 +++
> drivers/mtd/nand/raw/denali_dt.c | 49
> --
> 2 files changed, 50 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt
> b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> index 0ee8edb..f33da87 100644
> --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> @@ -8,6 +8,9 @@ Required properties:
>- reg : should contain registers location and length for data and reg.
>- reg-names: Should contain the reg names "nand_data" and "denali_reg"
>- interrupts : The interrupt number.
> + - clocks: should contain phandle of the controller core clock, the bus
> +interface clock, and the ECC circuit clock.
> + - clock-names: should contain "nand", "nand_x", "ecc"
>
> Optional properties:
>- nand-ecc-step-size: see nand.txt for details. If present, the value
> must be
> @@ -31,5 +34,7 @@ nand: nand@ff90 {
> compatible = "altr,socfpga-denali-nand";
> reg = <0xff90 0x20>, <0xffb8 0x1000>;
> reg-names = "nand_data", "denali_reg";
> + clocks = <_clk>, <_x_clk>, <_ecc_clk>;
> + clock-names = "nand", "nand_x", "ecc";
> interrupts = <0 144 4>;
> };
> diff --git a/drivers/mtd/nand/raw/denali_dt.c
> b/drivers/mtd/nand/raw/denali_dt.c
> index cfd33e6..ce6239d 100644
> --- a/drivers/mtd/nand/raw/denali_dt.c
> +++ b/drivers/mtd/nand/raw/denali_dt.c
> @@ -27,7 +27,9 @@
>
> struct denali_dt {
> struct denali_nand_info denali;
> - struct clk *clk;
> + struct clk *clk;/* core clock */
> + struct clk *clk_x; /* bus interface clock */
> + struct clk *clk_ecc;/* ECC circuit clock */
> };
>
> struct denali_dt_data {
> @@ -114,24 +116,61 @@ static int denali_dt_probe(struct platform_device *pdev)
> if (IS_ERR(denali->host))
> return PTR_ERR(denali->host);
>
> - dt->clk = devm_clk_get(>dev, NULL);
> + /*
> + * A single anonymous clock is supported for the backward compatibility.
> + * New platforms should support all the named clocks.
> + */
> + dt->clk = devm_clk_get(>dev, "nand");
> + if (IS_ERR(dt->clk))
> + dt->clk = devm_clk_get(>dev, NULL);
> if (IS_ERR(dt->clk)) {
> dev_err(>dev, "no clk available\n");
> return PTR_ERR(dt->clk);
> }
> +
> + dt->clk_x = devm_clk_get(>dev, "nand_x");
> + if (IS_ERR(dt->clk_x))
> + dt->clk_x = NULL;
> +
> + dt->clk_ecc = devm_clk_get(>dev, "ecc");
> + if (IS_ERR(dt->clk_ecc))
> + dt->clk_ecc = 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->clk_x);
> + if (ret)
> + goto out_disable_clk;
> +
> + ret = clk_prepare_enable(dt->clk_ecc);
> + if (ret)
> + goto out_disable_clk_x;
> +
> + if (dt->clk_x) {
> + denali->clk_x_rate =
Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet
Hi Masahiro,
On Fri, 15 Jun 2018 10:18:50 +0900
Masahiro Yamada wrote:
> According to the Denali User's Guide, this IP needs three clocks:
>
> - clk: controller core clock
>
> - clk_x: bus interface clock
>
> - ecc_clk: clock at which ECC circuitry is run
>
> Currently, denali_dt.c requires a single anonymous clock and its
> frequency. However, the driver needs to get the frequency of "clk_x"
> not "clk". This is confusing because people tend to assume the
> anonymous clock means the core clock. In fact, I got a report of
> SOCFPGA breakage because the timing parameters are calculated based
> on a wrong frequency.
>
> Instead of the cheesy implementation, the clocks in the real hardware
> should be represented in the driver and the DT-binding.
>
> However, adding new clocks would break the existing platforms. For the
> backward compatibility, the driver still accepts a single clock just as
> before. If clk_x is missing, clk_x_rate is set to a hardcoded value.
> This is fine for existing DT of Socionext UniPhier, and also fixes the
> issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> the bus interface clock.
>
> Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> setup_data_interface()")
Sorry for changing my mind, but I think this patch is doing more than
just fixing a bug. The bugfix should IMO be limited to unconditionally
setting ->clk_x_rate to 2 since this is what was done before
commit 1bb88666775e. And the remaining changes should go in nand/next.
Regards,
Boris
> Cc: linux-stable #4.14+
> Reported-by: Richard Weinberger
> Signed-off-by: Masahiro Yamada
> ---
>
> Changes in v3:
> - Change the patch order so that the bug-fix one comes the first
>
> Changes in v2:
> - Split patches into sensible chunks
>
> .../devicetree/bindings/mtd/denali-nand.txt| 5 +++
> drivers/mtd/nand/raw/denali_dt.c | 49
> --
> 2 files changed, 50 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt
> b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> index 0ee8edb..f33da87 100644
> --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> @@ -8,6 +8,9 @@ Required properties:
>- reg : should contain registers location and length for data and reg.
>- reg-names: Should contain the reg names "nand_data" and "denali_reg"
>- interrupts : The interrupt number.
> + - clocks: should contain phandle of the controller core clock, the bus
> +interface clock, and the ECC circuit clock.
> + - clock-names: should contain "nand", "nand_x", "ecc"
>
> Optional properties:
>- nand-ecc-step-size: see nand.txt for details. If present, the value
> must be
> @@ -31,5 +34,7 @@ nand: nand@ff90 {
> compatible = "altr,socfpga-denali-nand";
> reg = <0xff90 0x20>, <0xffb8 0x1000>;
> reg-names = "nand_data", "denali_reg";
> + clocks = <_clk>, <_x_clk>, <_ecc_clk>;
> + clock-names = "nand", "nand_x", "ecc";
> interrupts = <0 144 4>;
> };
> diff --git a/drivers/mtd/nand/raw/denali_dt.c
> b/drivers/mtd/nand/raw/denali_dt.c
> index cfd33e6..ce6239d 100644
> --- a/drivers/mtd/nand/raw/denali_dt.c
> +++ b/drivers/mtd/nand/raw/denali_dt.c
> @@ -27,7 +27,9 @@
>
> struct denali_dt {
> struct denali_nand_info denali;
> - struct clk *clk;
> + struct clk *clk;/* core clock */
> + struct clk *clk_x; /* bus interface clock */
> + struct clk *clk_ecc;/* ECC circuit clock */
> };
>
> struct denali_dt_data {
> @@ -114,24 +116,61 @@ static int denali_dt_probe(struct platform_device *pdev)
> if (IS_ERR(denali->host))
> return PTR_ERR(denali->host);
>
> - dt->clk = devm_clk_get(>dev, NULL);
> + /*
> + * A single anonymous clock is supported for the backward compatibility.
> + * New platforms should support all the named clocks.
> + */
> + dt->clk = devm_clk_get(>dev, "nand");
> + if (IS_ERR(dt->clk))
> + dt->clk = devm_clk_get(>dev, NULL);
> if (IS_ERR(dt->clk)) {
> dev_err(>dev, "no clk available\n");
> return PTR_ERR(dt->clk);
> }
> +
> + dt->clk_x = devm_clk_get(>dev, "nand_x");
> + if (IS_ERR(dt->clk_x))
> + dt->clk_x = NULL;
> +
> + dt->clk_ecc = devm_clk_get(>dev, "ecc");
> + if (IS_ERR(dt->clk_ecc))
> + dt->clk_ecc = 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->clk_x);
> + if (ret)
> + goto out_disable_clk;
> +
> + ret = clk_prepare_enable(dt->clk_ecc);
> + if (ret)
> + goto out_disable_clk_x;
> +
> + if (dt->clk_x) {
> + denali->clk_x_rate =
Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet
Am Dienstag, 19. Juni 2018, 10:07:26 CEST schrieb Masahiro Yamada:
> Hi Boris,
>
>
> 2018-06-18 16:46 GMT+09:00 Boris Brezillon :
> > On Mon, 18 Jun 2018 09:09:02 +0200
> > Richard Weinberger wrote:
> >
> >> Am Freitag, 15. Juni 2018, 03:18:50 CEST schrieb Masahiro Yamada:
> >> > According to the Denali User's Guide, this IP needs three clocks:
> >> >
> >> > - clk: controller core clock
> >> >
> >> > - clk_x: bus interface clock
> >> >
> >> > - ecc_clk: clock at which ECC circuitry is run
> >> >
> >> > Currently, denali_dt.c requires a single anonymous clock and its
> >> > frequency. However, the driver needs to get the frequency of "clk_x"
> >> > not "clk". This is confusing because people tend to assume the
> >> > anonymous clock means the core clock. In fact, I got a report of
> >> > SOCFPGA breakage because the timing parameters are calculated based
> >> > on a wrong frequency.
> >> >
> >> > Instead of the cheesy implementation, the clocks in the real hardware
> >> > should be represented in the driver and the DT-binding.
> >> >
> >> > However, adding new clocks would break the existing platforms. For the
> >> > backward compatibility, the driver still accepts a single clock just as
> >> > before. If clk_x is missing, clk_x_rate is set to a hardcoded value.
> >> > This is fine for existing DT of Socionext UniPhier, and also fixes the
> >> > issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> >> > the bus interface clock.
> >> >
> >> > Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> >> > setup_data_interface()")
> >> > Cc: linux-stable #4.14+
> >> > Reported-by: Richard Weinberger
> >> > Signed-off-by: Masahiro Yamada
> >>
> >> Reviewed-by: Richard Weinberger
> >
> > Maybe a
> >
> > Tested-by: Richard Weinberger
> >
> > ?
> >
> >> Reported-by: Philipp Rosenberger
> >
> > Should I replace your Reported-by by this one or simply add it?
Philipp deserves the Reported-by. :)
>
> I think it is good to have Reported-by
> both from Philipp and Richard.
Patch 1/3 unbreaks v4.14.x on my board.
Tested-by: Richard Weinberger
Thanks,
//richard
Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet
Am Dienstag, 19. Juni 2018, 10:07:26 CEST schrieb Masahiro Yamada:
> Hi Boris,
>
>
> 2018-06-18 16:46 GMT+09:00 Boris Brezillon :
> > On Mon, 18 Jun 2018 09:09:02 +0200
> > Richard Weinberger wrote:
> >
> >> Am Freitag, 15. Juni 2018, 03:18:50 CEST schrieb Masahiro Yamada:
> >> > According to the Denali User's Guide, this IP needs three clocks:
> >> >
> >> > - clk: controller core clock
> >> >
> >> > - clk_x: bus interface clock
> >> >
> >> > - ecc_clk: clock at which ECC circuitry is run
> >> >
> >> > Currently, denali_dt.c requires a single anonymous clock and its
> >> > frequency. However, the driver needs to get the frequency of "clk_x"
> >> > not "clk". This is confusing because people tend to assume the
> >> > anonymous clock means the core clock. In fact, I got a report of
> >> > SOCFPGA breakage because the timing parameters are calculated based
> >> > on a wrong frequency.
> >> >
> >> > Instead of the cheesy implementation, the clocks in the real hardware
> >> > should be represented in the driver and the DT-binding.
> >> >
> >> > However, adding new clocks would break the existing platforms. For the
> >> > backward compatibility, the driver still accepts a single clock just as
> >> > before. If clk_x is missing, clk_x_rate is set to a hardcoded value.
> >> > This is fine for existing DT of Socionext UniPhier, and also fixes the
> >> > issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> >> > the bus interface clock.
> >> >
> >> > Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> >> > setup_data_interface()")
> >> > Cc: linux-stable #4.14+
> >> > Reported-by: Richard Weinberger
> >> > Signed-off-by: Masahiro Yamada
> >>
> >> Reviewed-by: Richard Weinberger
> >
> > Maybe a
> >
> > Tested-by: Richard Weinberger
> >
> > ?
> >
> >> Reported-by: Philipp Rosenberger
> >
> > Should I replace your Reported-by by this one or simply add it?
Philipp deserves the Reported-by. :)
>
> I think it is good to have Reported-by
> both from Philipp and Richard.
Patch 1/3 unbreaks v4.14.x on my board.
Tested-by: Richard Weinberger
Thanks,
//richard
Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet
On Fri, 15 Jun 2018 10:18:50 +0900, Masahiro Yamada
wrote:
> According to the Denali User's Guide, this IP needs three clocks:
>
> - clk: controller core clock
>
> - clk_x: bus interface clock
>
> - ecc_clk: clock at which ECC circuitry is run
>
> Currently, denali_dt.c requires a single anonymous clock and its
> frequency. However, the driver needs to get the frequency of "clk_x"
> not "clk". This is confusing because people tend to assume the
> anonymous clock means the core clock. In fact, I got a report of
> SOCFPGA breakage because the timing parameters are calculated based
> on a wrong frequency.
>
> Instead of the cheesy implementation, the clocks in the real hardware
> should be represented in the driver and the DT-binding.
>
> However, adding new clocks would break the existing platforms. For the
> backward compatibility, the driver still accepts a single clock just as
> before. If clk_x is missing, clk_x_rate is set to a hardcoded value.
> This is fine for existing DT of Socionext UniPhier, and also fixes the
> issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> the bus interface clock.
>
> Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> setup_data_interface()")
> Cc: linux-stable #4.14+
> Reported-by: Richard Weinberger
> Signed-off-by: Masahiro Yamada
> ---
>
Reviewed-by: Miquel Raynal
Thanks,
Miquèl
Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet
On Fri, 15 Jun 2018 10:18:50 +0900, Masahiro Yamada
wrote:
> According to the Denali User's Guide, this IP needs three clocks:
>
> - clk: controller core clock
>
> - clk_x: bus interface clock
>
> - ecc_clk: clock at which ECC circuitry is run
>
> Currently, denali_dt.c requires a single anonymous clock and its
> frequency. However, the driver needs to get the frequency of "clk_x"
> not "clk". This is confusing because people tend to assume the
> anonymous clock means the core clock. In fact, I got a report of
> SOCFPGA breakage because the timing parameters are calculated based
> on a wrong frequency.
>
> Instead of the cheesy implementation, the clocks in the real hardware
> should be represented in the driver and the DT-binding.
>
> However, adding new clocks would break the existing platforms. For the
> backward compatibility, the driver still accepts a single clock just as
> before. If clk_x is missing, clk_x_rate is set to a hardcoded value.
> This is fine for existing DT of Socionext UniPhier, and also fixes the
> issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> the bus interface clock.
>
> Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> setup_data_interface()")
> Cc: linux-stable #4.14+
> Reported-by: Richard Weinberger
> Signed-off-by: Masahiro Yamada
> ---
>
Reviewed-by: Miquel Raynal
Thanks,
Miquèl
Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet
Hi Boris,
2018-06-18 16:46 GMT+09:00 Boris Brezillon :
> On Mon, 18 Jun 2018 09:09:02 +0200
> Richard Weinberger wrote:
>
>> Am Freitag, 15. Juni 2018, 03:18:50 CEST schrieb Masahiro Yamada:
>> > According to the Denali User's Guide, this IP needs three clocks:
>> >
>> > - clk: controller core clock
>> >
>> > - clk_x: bus interface clock
>> >
>> > - ecc_clk: clock at which ECC circuitry is run
>> >
>> > Currently, denali_dt.c requires a single anonymous clock and its
>> > frequency. However, the driver needs to get the frequency of "clk_x"
>> > not "clk". This is confusing because people tend to assume the
>> > anonymous clock means the core clock. In fact, I got a report of
>> > SOCFPGA breakage because the timing parameters are calculated based
>> > on a wrong frequency.
>> >
>> > Instead of the cheesy implementation, the clocks in the real hardware
>> > should be represented in the driver and the DT-binding.
>> >
>> > However, adding new clocks would break the existing platforms. For the
>> > backward compatibility, the driver still accepts a single clock just as
>> > before. If clk_x is missing, clk_x_rate is set to a hardcoded value.
>> > This is fine for existing DT of Socionext UniPhier, and also fixes the
>> > issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
>> > the bus interface clock.
>> >
>> > Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by
>> > setup_data_interface()")
>> > Cc: linux-stable #4.14+
>> > Reported-by: Richard Weinberger
>> > Signed-off-by: Masahiro Yamada
>>
>> Reviewed-by: Richard Weinberger
>
> Maybe a
>
> Tested-by: Richard Weinberger
>
> ?
>
>> Reported-by: Philipp Rosenberger
>
> Should I replace your Reported-by by this one or simply add it?
I think it is good to have Reported-by
both from Philipp and Richard.
Thanks.
--
Best Regards
Masahiro Yamada
Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet
Hi Boris,
2018-06-18 16:46 GMT+09:00 Boris Brezillon :
> On Mon, 18 Jun 2018 09:09:02 +0200
> Richard Weinberger wrote:
>
>> Am Freitag, 15. Juni 2018, 03:18:50 CEST schrieb Masahiro Yamada:
>> > According to the Denali User's Guide, this IP needs three clocks:
>> >
>> > - clk: controller core clock
>> >
>> > - clk_x: bus interface clock
>> >
>> > - ecc_clk: clock at which ECC circuitry is run
>> >
>> > Currently, denali_dt.c requires a single anonymous clock and its
>> > frequency. However, the driver needs to get the frequency of "clk_x"
>> > not "clk". This is confusing because people tend to assume the
>> > anonymous clock means the core clock. In fact, I got a report of
>> > SOCFPGA breakage because the timing parameters are calculated based
>> > on a wrong frequency.
>> >
>> > Instead of the cheesy implementation, the clocks in the real hardware
>> > should be represented in the driver and the DT-binding.
>> >
>> > However, adding new clocks would break the existing platforms. For the
>> > backward compatibility, the driver still accepts a single clock just as
>> > before. If clk_x is missing, clk_x_rate is set to a hardcoded value.
>> > This is fine for existing DT of Socionext UniPhier, and also fixes the
>> > issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
>> > the bus interface clock.
>> >
>> > Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by
>> > setup_data_interface()")
>> > Cc: linux-stable #4.14+
>> > Reported-by: Richard Weinberger
>> > Signed-off-by: Masahiro Yamada
>>
>> Reviewed-by: Richard Weinberger
>
> Maybe a
>
> Tested-by: Richard Weinberger
>
> ?
>
>> Reported-by: Philipp Rosenberger
>
> Should I replace your Reported-by by this one or simply add it?
I think it is good to have Reported-by
both from Philipp and Richard.
Thanks.
--
Best Regards
Masahiro Yamada
Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet
Hi Boris,
On Mon, 18 Jun 2018 09:46:36 +0200, Boris Brezillon
wrote:
> On Mon, 18 Jun 2018 09:09:02 +0200
> Richard Weinberger wrote:
>
> > Am Freitag, 15. Juni 2018, 03:18:50 CEST schrieb Masahiro Yamada:
> > > According to the Denali User's Guide, this IP needs three clocks:
> > >
> > > - clk: controller core clock
> > >
> > > - clk_x: bus interface clock
> > >
> > > - ecc_clk: clock at which ECC circuitry is run
> > >
> > > Currently, denali_dt.c requires a single anonymous clock and its
> > > frequency. However, the driver needs to get the frequency of "clk_x"
> > > not "clk". This is confusing because people tend to assume the
> > > anonymous clock means the core clock. In fact, I got a report of
> > > SOCFPGA breakage because the timing parameters are calculated based
> > > on a wrong frequency.
> > >
> > > Instead of the cheesy implementation, the clocks in the real hardware
> > > should be represented in the driver and the DT-binding.
> > >
> > > However, adding new clocks would break the existing platforms. For the
> > > backward compatibility, the driver still accepts a single clock just as
> > > before. If clk_x is missing, clk_x_rate is set to a hardcoded value.
> > > This is fine for existing DT of Socionext UniPhier, and also fixes the
> > > issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> > > the bus interface clock.
> > >
> > > Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> > > setup_data_interface()")
> > > Cc: linux-stable #4.14+
> > > Reported-by: Richard Weinberger
> > > Signed-off-by: Masahiro Yamada
> >
> > Reviewed-by: Richard Weinberger
>
> Maybe a
>
> Tested-by: Richard Weinberger
>
> ?
>
> > Reported-by: Philipp Rosenberger
>
> Should I replace your Reported-by by this one or simply add it?
>
> Miquel, I'll take this patch in mtd/fixes, and let you take the 2
> others in nand/next. That means you'll have to back merge v4.18-rc2
> into the nand/next tree, or base your tree on v4.18-rc2 instead of
> v4.18-rc1.
Sure.
Miquèl
Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet
Hi Boris,
On Mon, 18 Jun 2018 09:46:36 +0200, Boris Brezillon
wrote:
> On Mon, 18 Jun 2018 09:09:02 +0200
> Richard Weinberger wrote:
>
> > Am Freitag, 15. Juni 2018, 03:18:50 CEST schrieb Masahiro Yamada:
> > > According to the Denali User's Guide, this IP needs three clocks:
> > >
> > > - clk: controller core clock
> > >
> > > - clk_x: bus interface clock
> > >
> > > - ecc_clk: clock at which ECC circuitry is run
> > >
> > > Currently, denali_dt.c requires a single anonymous clock and its
> > > frequency. However, the driver needs to get the frequency of "clk_x"
> > > not "clk". This is confusing because people tend to assume the
> > > anonymous clock means the core clock. In fact, I got a report of
> > > SOCFPGA breakage because the timing parameters are calculated based
> > > on a wrong frequency.
> > >
> > > Instead of the cheesy implementation, the clocks in the real hardware
> > > should be represented in the driver and the DT-binding.
> > >
> > > However, adding new clocks would break the existing platforms. For the
> > > backward compatibility, the driver still accepts a single clock just as
> > > before. If clk_x is missing, clk_x_rate is set to a hardcoded value.
> > > This is fine for existing DT of Socionext UniPhier, and also fixes the
> > > issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> > > the bus interface clock.
> > >
> > > Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> > > setup_data_interface()")
> > > Cc: linux-stable #4.14+
> > > Reported-by: Richard Weinberger
> > > Signed-off-by: Masahiro Yamada
> >
> > Reviewed-by: Richard Weinberger
>
> Maybe a
>
> Tested-by: Richard Weinberger
>
> ?
>
> > Reported-by: Philipp Rosenberger
>
> Should I replace your Reported-by by this one or simply add it?
>
> Miquel, I'll take this patch in mtd/fixes, and let you take the 2
> others in nand/next. That means you'll have to back merge v4.18-rc2
> into the nand/next tree, or base your tree on v4.18-rc2 instead of
> v4.18-rc1.
Sure.
Miquèl
Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet
On Mon, 18 Jun 2018 09:09:02 +0200
Richard Weinberger wrote:
> Am Freitag, 15. Juni 2018, 03:18:50 CEST schrieb Masahiro Yamada:
> > According to the Denali User's Guide, this IP needs three clocks:
> >
> > - clk: controller core clock
> >
> > - clk_x: bus interface clock
> >
> > - ecc_clk: clock at which ECC circuitry is run
> >
> > Currently, denali_dt.c requires a single anonymous clock and its
> > frequency. However, the driver needs to get the frequency of "clk_x"
> > not "clk". This is confusing because people tend to assume the
> > anonymous clock means the core clock. In fact, I got a report of
> > SOCFPGA breakage because the timing parameters are calculated based
> > on a wrong frequency.
> >
> > Instead of the cheesy implementation, the clocks in the real hardware
> > should be represented in the driver and the DT-binding.
> >
> > However, adding new clocks would break the existing platforms. For the
> > backward compatibility, the driver still accepts a single clock just as
> > before. If clk_x is missing, clk_x_rate is set to a hardcoded value.
> > This is fine for existing DT of Socionext UniPhier, and also fixes the
> > issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> > the bus interface clock.
> >
> > Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> > setup_data_interface()")
> > Cc: linux-stable #4.14+
> > Reported-by: Richard Weinberger
> > Signed-off-by: Masahiro Yamada
>
> Reviewed-by: Richard Weinberger
Maybe a
Tested-by: Richard Weinberger
?
> Reported-by: Philipp Rosenberger
Should I replace your Reported-by by this one or simply add it?
Miquel, I'll take this patch in mtd/fixes, and let you take the 2
others in nand/next. That means you'll have to back merge v4.18-rc2
into the nand/next tree, or base your tree on v4.18-rc2 instead of
v4.18-rc1.
Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet
On Mon, 18 Jun 2018 09:09:02 +0200
Richard Weinberger wrote:
> Am Freitag, 15. Juni 2018, 03:18:50 CEST schrieb Masahiro Yamada:
> > According to the Denali User's Guide, this IP needs three clocks:
> >
> > - clk: controller core clock
> >
> > - clk_x: bus interface clock
> >
> > - ecc_clk: clock at which ECC circuitry is run
> >
> > Currently, denali_dt.c requires a single anonymous clock and its
> > frequency. However, the driver needs to get the frequency of "clk_x"
> > not "clk". This is confusing because people tend to assume the
> > anonymous clock means the core clock. In fact, I got a report of
> > SOCFPGA breakage because the timing parameters are calculated based
> > on a wrong frequency.
> >
> > Instead of the cheesy implementation, the clocks in the real hardware
> > should be represented in the driver and the DT-binding.
> >
> > However, adding new clocks would break the existing platforms. For the
> > backward compatibility, the driver still accepts a single clock just as
> > before. If clk_x is missing, clk_x_rate is set to a hardcoded value.
> > This is fine for existing DT of Socionext UniPhier, and also fixes the
> > issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> > the bus interface clock.
> >
> > Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> > setup_data_interface()")
> > Cc: linux-stable #4.14+
> > Reported-by: Richard Weinberger
> > Signed-off-by: Masahiro Yamada
>
> Reviewed-by: Richard Weinberger
Maybe a
Tested-by: Richard Weinberger
?
> Reported-by: Philipp Rosenberger
Should I replace your Reported-by by this one or simply add it?
Miquel, I'll take this patch in mtd/fixes, and let you take the 2
others in nand/next. That means you'll have to back merge v4.18-rc2
into the nand/next tree, or base your tree on v4.18-rc2 instead of
v4.18-rc1.
Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet
Am Freitag, 15. Juni 2018, 03:18:50 CEST schrieb Masahiro Yamada:
> According to the Denali User's Guide, this IP needs three clocks:
>
> - clk: controller core clock
>
> - clk_x: bus interface clock
>
> - ecc_clk: clock at which ECC circuitry is run
>
> Currently, denali_dt.c requires a single anonymous clock and its
> frequency. However, the driver needs to get the frequency of "clk_x"
> not "clk". This is confusing because people tend to assume the
> anonymous clock means the core clock. In fact, I got a report of
> SOCFPGA breakage because the timing parameters are calculated based
> on a wrong frequency.
>
> Instead of the cheesy implementation, the clocks in the real hardware
> should be represented in the driver and the DT-binding.
>
> However, adding new clocks would break the existing platforms. For the
> backward compatibility, the driver still accepts a single clock just as
> before. If clk_x is missing, clk_x_rate is set to a hardcoded value.
> This is fine for existing DT of Socionext UniPhier, and also fixes the
> issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> the bus interface clock.
>
> Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> setup_data_interface()")
> Cc: linux-stable #4.14+
> Reported-by: Richard Weinberger
> Signed-off-by: Masahiro Yamada
Reviewed-by: Richard Weinberger
Reported-by: Philipp Rosenberger
Thanks,
//richard
Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet
Am Freitag, 15. Juni 2018, 03:18:50 CEST schrieb Masahiro Yamada:
> According to the Denali User's Guide, this IP needs three clocks:
>
> - clk: controller core clock
>
> - clk_x: bus interface clock
>
> - ecc_clk: clock at which ECC circuitry is run
>
> Currently, denali_dt.c requires a single anonymous clock and its
> frequency. However, the driver needs to get the frequency of "clk_x"
> not "clk". This is confusing because people tend to assume the
> anonymous clock means the core clock. In fact, I got a report of
> SOCFPGA breakage because the timing parameters are calculated based
> on a wrong frequency.
>
> Instead of the cheesy implementation, the clocks in the real hardware
> should be represented in the driver and the DT-binding.
>
> However, adding new clocks would break the existing platforms. For the
> backward compatibility, the driver still accepts a single clock just as
> before. If clk_x is missing, clk_x_rate is set to a hardcoded value.
> This is fine for existing DT of Socionext UniPhier, and also fixes the
> issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> the bus interface clock.
>
> Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> setup_data_interface()")
> Cc: linux-stable #4.14+
> Reported-by: Richard Weinberger
> Signed-off-by: Masahiro Yamada
Reviewed-by: Richard Weinberger
Reported-by: Philipp Rosenberger
Thanks,
//richard
[PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet
According to the Denali User's Guide, this IP needs three clocks:
- clk: controller core clock
- clk_x: bus interface clock
- ecc_clk: clock at which ECC circuitry is run
Currently, denali_dt.c requires a single anonymous clock and its
frequency. However, the driver needs to get the frequency of "clk_x"
not "clk". This is confusing because people tend to assume the
anonymous clock means the core clock. In fact, I got a report of
SOCFPGA breakage because the timing parameters are calculated based
on a wrong frequency.
Instead of the cheesy implementation, the clocks in the real hardware
should be represented in the driver and the DT-binding.
However, adding new clocks would break the existing platforms. For the
backward compatibility, the driver still accepts a single clock just as
before. If clk_x is missing, clk_x_rate is set to a hardcoded value.
This is fine for existing DT of Socionext UniPhier, and also fixes the
issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
the bus interface clock.
Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by
setup_data_interface()")
Cc: linux-stable #4.14+
Reported-by: Richard Weinberger
Signed-off-by: Masahiro Yamada
---
Changes in v3:
- Change the patch order so that the bug-fix one comes the first
Changes in v2:
- Split patches into sensible chunks
.../devicetree/bindings/mtd/denali-nand.txt| 5 +++
drivers/mtd/nand/raw/denali_dt.c | 49 --
2 files changed, 50 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt
b/Documentation/devicetree/bindings/mtd/denali-nand.txt
index 0ee8edb..f33da87 100644
--- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
@@ -8,6 +8,9 @@ Required properties:
- reg : should contain registers location and length for data and reg.
- reg-names: Should contain the reg names "nand_data" and "denali_reg"
- interrupts : The interrupt number.
+ - clocks: should contain phandle of the controller core clock, the bus
+interface clock, and the ECC circuit clock.
+ - clock-names: should contain "nand", "nand_x", "ecc"
Optional properties:
- nand-ecc-step-size: see nand.txt for details. If present, the value must
be
@@ -31,5 +34,7 @@ nand: nand@ff90 {
compatible = "altr,socfpga-denali-nand";
reg = <0xff90 0x20>, <0xffb8 0x1000>;
reg-names = "nand_data", "denali_reg";
+ clocks = <_clk>, <_x_clk>, <_ecc_clk>;
+ clock-names = "nand", "nand_x", "ecc";
interrupts = <0 144 4>;
};
diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
index cfd33e6..ce6239d 100644
--- a/drivers/mtd/nand/raw/denali_dt.c
+++ b/drivers/mtd/nand/raw/denali_dt.c
@@ -27,7 +27,9 @@
struct denali_dt {
struct denali_nand_info denali;
- struct clk *clk;
+ struct clk *clk;/* core clock */
+ struct clk *clk_x; /* bus interface clock */
+ struct clk *clk_ecc;/* ECC circuit clock */
};
struct denali_dt_data {
@@ -114,24 +116,61 @@ static int denali_dt_probe(struct platform_device *pdev)
if (IS_ERR(denali->host))
return PTR_ERR(denali->host);
- dt->clk = devm_clk_get(>dev, NULL);
+ /*
+* A single anonymous clock is supported for the backward compatibility.
+* New platforms should support all the named clocks.
+*/
+ dt->clk = devm_clk_get(>dev, "nand");
+ if (IS_ERR(dt->clk))
+ dt->clk = devm_clk_get(>dev, NULL);
if (IS_ERR(dt->clk)) {
dev_err(>dev, "no clk available\n");
return PTR_ERR(dt->clk);
}
+
+ dt->clk_x = devm_clk_get(>dev, "nand_x");
+ if (IS_ERR(dt->clk_x))
+ dt->clk_x = NULL;
+
+ dt->clk_ecc = devm_clk_get(>dev, "ecc");
+ if (IS_ERR(dt->clk_ecc))
+ dt->clk_ecc = 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->clk_x);
+ if (ret)
+ goto out_disable_clk;
+
+ ret = clk_prepare_enable(dt->clk_ecc);
+ if (ret)
+ goto out_disable_clk_x;
+
+ if (dt->clk_x) {
+ denali->clk_x_rate = clk_get_rate(dt->clk_x);
+ } else {
+ /*
+* Hardcode the clock rates for the backward compatibility.
+* This works for both SOCFPGA and UniPhier.
+*/
+ dev_notice(>dev,
+ "necessary clock is missing. default clock rates are
used.\n");
+ denali->clk_x_rate = 2;
+ }
ret = denali_init(denali);
if (ret)
- goto out_disable_clk;
+ goto out_disable_clk_ecc;
[PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet
According to the Denali User's Guide, this IP needs three clocks:
- clk: controller core clock
- clk_x: bus interface clock
- ecc_clk: clock at which ECC circuitry is run
Currently, denali_dt.c requires a single anonymous clock and its
frequency. However, the driver needs to get the frequency of "clk_x"
not "clk". This is confusing because people tend to assume the
anonymous clock means the core clock. In fact, I got a report of
SOCFPGA breakage because the timing parameters are calculated based
on a wrong frequency.
Instead of the cheesy implementation, the clocks in the real hardware
should be represented in the driver and the DT-binding.
However, adding new clocks would break the existing platforms. For the
backward compatibility, the driver still accepts a single clock just as
before. If clk_x is missing, clk_x_rate is set to a hardcoded value.
This is fine for existing DT of Socionext UniPhier, and also fixes the
issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
the bus interface clock.
Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by
setup_data_interface()")
Cc: linux-stable #4.14+
Reported-by: Richard Weinberger
Signed-off-by: Masahiro Yamada
---
Changes in v3:
- Change the patch order so that the bug-fix one comes the first
Changes in v2:
- Split patches into sensible chunks
.../devicetree/bindings/mtd/denali-nand.txt| 5 +++
drivers/mtd/nand/raw/denali_dt.c | 49 --
2 files changed, 50 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt
b/Documentation/devicetree/bindings/mtd/denali-nand.txt
index 0ee8edb..f33da87 100644
--- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
@@ -8,6 +8,9 @@ Required properties:
- reg : should contain registers location and length for data and reg.
- reg-names: Should contain the reg names "nand_data" and "denali_reg"
- interrupts : The interrupt number.
+ - clocks: should contain phandle of the controller core clock, the bus
+interface clock, and the ECC circuit clock.
+ - clock-names: should contain "nand", "nand_x", "ecc"
Optional properties:
- nand-ecc-step-size: see nand.txt for details. If present, the value must
be
@@ -31,5 +34,7 @@ nand: nand@ff90 {
compatible = "altr,socfpga-denali-nand";
reg = <0xff90 0x20>, <0xffb8 0x1000>;
reg-names = "nand_data", "denali_reg";
+ clocks = <_clk>, <_x_clk>, <_ecc_clk>;
+ clock-names = "nand", "nand_x", "ecc";
interrupts = <0 144 4>;
};
diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
index cfd33e6..ce6239d 100644
--- a/drivers/mtd/nand/raw/denali_dt.c
+++ b/drivers/mtd/nand/raw/denali_dt.c
@@ -27,7 +27,9 @@
struct denali_dt {
struct denali_nand_info denali;
- struct clk *clk;
+ struct clk *clk;/* core clock */
+ struct clk *clk_x; /* bus interface clock */
+ struct clk *clk_ecc;/* ECC circuit clock */
};
struct denali_dt_data {
@@ -114,24 +116,61 @@ static int denali_dt_probe(struct platform_device *pdev)
if (IS_ERR(denali->host))
return PTR_ERR(denali->host);
- dt->clk = devm_clk_get(>dev, NULL);
+ /*
+* A single anonymous clock is supported for the backward compatibility.
+* New platforms should support all the named clocks.
+*/
+ dt->clk = devm_clk_get(>dev, "nand");
+ if (IS_ERR(dt->clk))
+ dt->clk = devm_clk_get(>dev, NULL);
if (IS_ERR(dt->clk)) {
dev_err(>dev, "no clk available\n");
return PTR_ERR(dt->clk);
}
+
+ dt->clk_x = devm_clk_get(>dev, "nand_x");
+ if (IS_ERR(dt->clk_x))
+ dt->clk_x = NULL;
+
+ dt->clk_ecc = devm_clk_get(>dev, "ecc");
+ if (IS_ERR(dt->clk_ecc))
+ dt->clk_ecc = 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->clk_x);
+ if (ret)
+ goto out_disable_clk;
+
+ ret = clk_prepare_enable(dt->clk_ecc);
+ if (ret)
+ goto out_disable_clk_x;
+
+ if (dt->clk_x) {
+ denali->clk_x_rate = clk_get_rate(dt->clk_x);
+ } else {
+ /*
+* Hardcode the clock rates for the backward compatibility.
+* This works for both SOCFPGA and UniPhier.
+*/
+ dev_notice(>dev,
+ "necessary clock is missing. default clock rates are
used.\n");
+ denali->clk_x_rate = 2;
+ }
ret = denali_init(denali);
if (ret)
- goto out_disable_clk;
+ goto out_disable_clk_ecc;
