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
