The denali NAND flash controller needs at least two clocks to operate,
nand_clk and nand_x_clk.
Since 1bb88666775e ("mtd: nand: denali: handle timing parameters by
setup_data_interface()") nand_x_clk is used to derive timing settings.
Signed-off-by: Richard Weinberger <[email protected]>
---
Strictly speaking denali needs a ecc_clk too, but AFAIK such a clock
is not present on this SoC.
But my SoCFPGA knowledge is very limited.
Thanks,
//richard
---
arch/arm/boot/dts/socfpga.dtsi | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 486d4e7433ed..562f7b375bbd 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -754,7 +754,8 @@
reg-names = "nand_data", "denali_reg";
interrupts = <0x0 0x90 0x4>;
dma-mask = <0xffffffff>;
- clocks = <&nand_clk>;
+ clocks = <&nand_clk>, <&nand_x_clk>;
+ clock-names = "nand", "nand_x";
status = "disabled";
};
--
2.17.1
On 06/19/2018 02:07 PM, Richard Weinberger wrote:
> The denali NAND flash controller needs at least two clocks to operate,
> nand_clk and nand_x_clk.
> Since 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> setup_data_interface()") nand_x_clk is used to derive timing settings.
>
> Signed-off-by: Richard Weinberger <[email protected]>
> ---
> Strictly speaking denali needs a ecc_clk too, but AFAIK such a clock
> is not present on this SoC.
> But my SoCFPGA knowledge is very limited.
>
> Thanks,
> //richard
It looks sane, but I cannot test it right now, since I'm on vacation.
I hope Dinh/Chin can jump in.
> ---
> arch/arm/boot/dts/socfpga.dtsi | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index 486d4e7433ed..562f7b375bbd 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -754,7 +754,8 @@
> reg-names = "nand_data", "denali_reg";
> interrupts = <0x0 0x90 0x4>;
> dma-mask = <0xffffffff>;
> - clocks = <&nand_clk>;
> + clocks = <&nand_clk>, <&nand_x_clk>;
> + clock-names = "nand", "nand_x";
> status = "disabled";
> };
>
>
--
Best regards,
Marek Vasut
Marek,
Am Mittwoch, 20. Juni 2018, 06:52:09 CEST schrieb Marek Vasut:
> On 06/19/2018 02:07 PM, Richard Weinberger wrote:
> > The denali NAND flash controller needs at least two clocks to operate,
> > nand_clk and nand_x_clk.
> > Since 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> > setup_data_interface()") nand_x_clk is used to derive timing settings.
> >
> > Signed-off-by: Richard Weinberger <[email protected]>
> > ---
> > Strictly speaking denali needs a ecc_clk too, but AFAIK such a clock
> > is not present on this SoC.
> > But my SoCFPGA knowledge is very limited.
> >
> > Thanks,
> > //richard
>
> It looks sane, but I cannot test it right now, since I'm on vacation.
> I hope Dinh/Chin can jump in.
The patch was tested by me. So, at least it is not completely untested.
BTW: I forgot to mention that it depends on Masahiro's Denali fixes.
Thanks,
//richard
On 06/20/2018 07:27 AM, Richard Weinberger wrote:
> Marek,
>
> Am Mittwoch, 20. Juni 2018, 06:52:09 CEST schrieb Marek Vasut:
>> On 06/19/2018 02:07 PM, Richard Weinberger wrote:
>>> The denali NAND flash controller needs at least two clocks to operate,
>>> nand_clk and nand_x_clk.
>>> Since 1bb88666775e ("mtd: nand: denali: handle timing parameters by
>>> setup_data_interface()") nand_x_clk is used to derive timing settings.
>>>
>>> Signed-off-by: Richard Weinberger <[email protected]>
>>> ---
>>> Strictly speaking denali needs a ecc_clk too, but AFAIK such a clock
>>> is not present on this SoC.
>>> But my SoCFPGA knowledge is very limited.
>>>
>>> Thanks,
>>> //richard
>>
>> It looks sane, but I cannot test it right now, since I'm on vacation.
>> I hope Dinh/Chin can jump in.
>
> The patch was tested by me. So, at least it is not completely untested.
> BTW: I forgot to mention that it depends on Masahiro's Denali fixes.
Then that's perfect.
Reviewed-by: Marek Vasut <[email protected]>
--
Best regards,
Marek Vasut
Hi Richard,
2018-06-19 21:07 GMT+09:00 Richard Weinberger <[email protected]>:
> The denali NAND flash controller needs at least two clocks to operate,
> nand_clk and nand_x_clk.
> Since 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> setup_data_interface()") nand_x_clk is used to derive timing settings.
>
> Signed-off-by: Richard Weinberger <[email protected]>
> ---
> Strictly speaking denali needs a ecc_clk too, but AFAIK such a clock
> is not present on this SoC.
> But my SoCFPGA knowledge is very limited.
>
> Thanks,
> //richard
> ---
> arch/arm/boot/dts/socfpga.dtsi | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index 486d4e7433ed..562f7b375bbd 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -754,7 +754,8 @@
> reg-names = "nand_data", "denali_reg";
> interrupts = <0x0 0x90 0x4>;
> dma-mask = <0xffffffff>;
> - clocks = <&nand_clk>;
> + clocks = <&nand_clk>, <&nand_x_clk>;
> + clock-names = "nand", "nand_x";
IMHO, this should be
clocks = <&nand_clk>, <&nand_x_clk>, <&nand_x_clk>;
clock-names = "nand", "nand_x", "ecc";
A clock consumer (Denali in this case) should generally
take the same number of clocks across SoCs.
It is just some SoCs tie clocks together.
It is the case for my UniPhier platform;
"nand_x" and "ecc" are tied up because they are both 200MHz.
SOCFPGA supports HW ECC correction, thus it surely needs ecc clock.
> status = "disabled";
> };
>
> --
> 2.17.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards
Masahiro Yamada
Masahiro,
Am Freitag, 22. Juni 2018, 16:37:21 CEST schrieb Masahiro Yamada:
> Hi Richard,
>
>
> 2018-06-19 21:07 GMT+09:00 Richard Weinberger <[email protected]>:
> > The denali NAND flash controller needs at least two clocks to operate,
> > nand_clk and nand_x_clk.
> > Since 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> > setup_data_interface()") nand_x_clk is used to derive timing settings.
> >
> > Signed-off-by: Richard Weinberger <[email protected]>
> > ---
> > Strictly speaking denali needs a ecc_clk too, but AFAIK such a clock
> > is not present on this SoC.
> > But my SoCFPGA knowledge is very limited.
> >
> > Thanks,
> > //richard
> > ---
> > arch/arm/boot/dts/socfpga.dtsi | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> > index 486d4e7433ed..562f7b375bbd 100644
> > --- a/arch/arm/boot/dts/socfpga.dtsi
> > +++ b/arch/arm/boot/dts/socfpga.dtsi
> > @@ -754,7 +754,8 @@
> > reg-names = "nand_data", "denali_reg";
> > interrupts = <0x0 0x90 0x4>;
> > dma-mask = <0xffffffff>;
> > - clocks = <&nand_clk>;
> > + clocks = <&nand_clk>, <&nand_x_clk>;
> > + clock-names = "nand", "nand_x";
>
>
> IMHO, this should be
>
> clocks = <&nand_clk>, <&nand_x_clk>, <&nand_x_clk>;
> clock-names = "nand", "nand_x", "ecc";
>
>
>
> A clock consumer (Denali in this case) should generally
> take the same number of clocks across SoCs.
>
> It is just some SoCs tie clocks together.
>
> It is the case for my UniPhier platform;
> "nand_x" and "ecc" are tied up because they are both 200MHz.
>
>
> SOCFPGA supports HW ECC correction, thus it surely needs ecc clock.
Good point!
Thanks,
//richard
On 06/22/2018 10:58 AM, Richard Weinberger wrote:
> Masahiro,
>
> Am Freitag, 22. Juni 2018, 16:37:21 CEST schrieb Masahiro Yamada:
>> Hi Richard,
>>
>>
>> 2018-06-19 21:07 GMT+09:00 Richard Weinberger <[email protected]>:
>>> The denali NAND flash controller needs at least two clocks to operate,
>>> nand_clk and nand_x_clk.
>>> Since 1bb88666775e ("mtd: nand: denali: handle timing parameters by
>>> setup_data_interface()") nand_x_clk is used to derive timing settings.
>>>
>>> Signed-off-by: Richard Weinberger <[email protected]>
>>> ---
>>> Strictly speaking denali needs a ecc_clk too, but AFAIK such a clock
>>> is not present on this SoC.
>>> But my SoCFPGA knowledge is very limited.
>>>
>>> Thanks,
>>> //richard
>>> ---
>>> arch/arm/boot/dts/socfpga.dtsi | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>>> index 486d4e7433ed..562f7b375bbd 100644
>>> --- a/arch/arm/boot/dts/socfpga.dtsi
>>> +++ b/arch/arm/boot/dts/socfpga.dtsi
>>> @@ -754,7 +754,8 @@
>>> reg-names = "nand_data", "denali_reg";
>>> interrupts = <0x0 0x90 0x4>;
>>> dma-mask = <0xffffffff>;
>>> - clocks = <&nand_clk>;
>>> + clocks = <&nand_clk>, <&nand_x_clk>;
>>> + clock-names = "nand", "nand_x";
>>
>>
>> IMHO, this should be
>>
>> clocks = <&nand_clk>, <&nand_x_clk>, <&nand_x_clk>;
>> clock-names = "nand", "nand_x", "ecc";
No, it should be just the nand_x and ecc.
There's already a patch to use the nand_x_clk and not the nand_clk.
https://git.kernel.org/pub/scm/linux/kernel/git/dinguyen/linux.git/commit/?h=socfpga_for_next_v4.19_fixes_v1&id=1709ab58eb79b19bceb2287d111bf1bd2df1cf6d
Dinh
On Mon, 25 Jun 2018 09:50:18 -0500
Dinh Nguyen <[email protected]> wrote:
> On 06/22/2018 10:58 AM, Richard Weinberger wrote:
> > Masahiro,
> >
> > Am Freitag, 22. Juni 2018, 16:37:21 CEST schrieb Masahiro Yamada:
> >> Hi Richard,
> >>
> >>
> >> 2018-06-19 21:07 GMT+09:00 Richard Weinberger <[email protected]>:
> >>> The denali NAND flash controller needs at least two clocks to operate,
> >>> nand_clk and nand_x_clk.
> >>> Since 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> >>> setup_data_interface()") nand_x_clk is used to derive timing settings.
> >>>
> >>> Signed-off-by: Richard Weinberger <[email protected]>
> >>> ---
> >>> Strictly speaking denali needs a ecc_clk too, but AFAIK such a clock
> >>> is not present on this SoC.
> >>> But my SoCFPGA knowledge is very limited.
> >>>
> >>> Thanks,
> >>> //richard
> >>> ---
> >>> arch/arm/boot/dts/socfpga.dtsi | 3 ++-
> >>> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> >>> index 486d4e7433ed..562f7b375bbd 100644
> >>> --- a/arch/arm/boot/dts/socfpga.dtsi
> >>> +++ b/arch/arm/boot/dts/socfpga.dtsi
> >>> @@ -754,7 +754,8 @@
> >>> reg-names = "nand_data", "denali_reg";
> >>> interrupts = <0x0 0x90 0x4>;
> >>> dma-mask = <0xffffffff>;
> >>> - clocks = <&nand_clk>;
> >>> + clocks = <&nand_clk>, <&nand_x_clk>;
> >>> + clock-names = "nand", "nand_x";
> >>
> >>
> >> IMHO, this should be
> >>
> >> clocks = <&nand_clk>, <&nand_x_clk>, <&nand_x_clk>;
> >> clock-names = "nand", "nand_x", "ecc";
>
> No, it should be just the nand_x and ecc.
>
> There's already a patch to use the nand_x_clk and not the nand_clk.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/dinguyen/linux.git/commit/?h=socfpga_for_next_v4.19_fixes_v1&id=1709ab58eb79b19bceb2287d111bf1bd2df1cf6d
Hm, are you sure this is accurate? I might be wrong but I find it weird
that the denali NAND controller IP has been adapted by Xilinx to only
take one clk. Isn't that the same clk is feeding all clk inputs of the
denali block?
2018-06-25 23:55 GMT+09:00 Boris Brezillon <[email protected]>:
> On Mon, 25 Jun 2018 09:50:18 -0500
> Dinh Nguyen <[email protected]> wrote:
>
>> On 06/22/2018 10:58 AM, Richard Weinberger wrote:
>> > Masahiro,
>> >
>> > Am Freitag, 22. Juni 2018, 16:37:21 CEST schrieb Masahiro Yamada:
>> >> Hi Richard,
>> >>
>> >>
>> >> 2018-06-19 21:07 GMT+09:00 Richard Weinberger <[email protected]>:
>> >>> The denali NAND flash controller needs at least two clocks to operate,
>> >>> nand_clk and nand_x_clk.
>> >>> Since 1bb88666775e ("mtd: nand: denali: handle timing parameters by
>> >>> setup_data_interface()") nand_x_clk is used to derive timing settings.
>> >>>
>> >>> Signed-off-by: Richard Weinberger <[email protected]>
>> >>> ---
>> >>> Strictly speaking denali needs a ecc_clk too, but AFAIK such a clock
>> >>> is not present on this SoC.
>> >>> But my SoCFPGA knowledge is very limited.
>> >>>
>> >>> Thanks,
>> >>> //richard
>> >>> ---
>> >>> arch/arm/boot/dts/socfpga.dtsi | 3 ++-
>> >>> 1 file changed, 2 insertions(+), 1 deletion(-)
>> >>>
>> >>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>> >>> index 486d4e7433ed..562f7b375bbd 100644
>> >>> --- a/arch/arm/boot/dts/socfpga.dtsi
>> >>> +++ b/arch/arm/boot/dts/socfpga.dtsi
>> >>> @@ -754,7 +754,8 @@
>> >>> reg-names = "nand_data", "denali_reg";
>> >>> interrupts = <0x0 0x90 0x4>;
>> >>> dma-mask = <0xffffffff>;
>> >>> - clocks = <&nand_clk>;
>> >>> + clocks = <&nand_clk>, <&nand_x_clk>;
>> >>> + clock-names = "nand", "nand_x";
>> >>
>> >>
>> >> IMHO, this should be
>> >>
>> >> clocks = <&nand_clk>, <&nand_x_clk>, <&nand_x_clk>;
>> >> clock-names = "nand", "nand_x", "ecc";
>>
>> No, it should be just the nand_x and ecc.
>>
>> There's already a patch to use the nand_x_clk and not the nand_clk.
Different people try to fix the problem in different ways.
I think it is due to miscommunication across sub-systems.
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/dinguyen/linux.git/commit/?h=socfpga_for_next_v4.19_fixes_v1&id=1709ab58eb79b19bceb2287d111bf1bd2df1cf6d
This does not break your board.
However, it is not helpful any more.
I already fix the Denali driver
to use the hard-coded clock frequency
if the old binding is used.
BTW, Marek issued Reviewed-by to this patch.
> Hm, are you sure this is accurate? I might be wrong but I find it weird
> that the denali NAND controller IP has been adapted by Xilinx to only
Xilinx?
Do you mean Altera (Intel)?
> take one clk. Isn't that the same clk is feeding all clk inputs of the
> denali block?
--
Best Regards
Masahiro Yamada
Hi Masahiro,
On Tue, 26 Jun 2018 11:38:21 +0900, Masahiro Yamada
<[email protected]> wrote:
> 2018-06-25 23:55 GMT+09:00 Boris Brezillon <[email protected]>:
> > On Mon, 25 Jun 2018 09:50:18 -0500
> > Dinh Nguyen <[email protected]> wrote:
> >
> >> On 06/22/2018 10:58 AM, Richard Weinberger wrote:
> >> > Masahiro,
> >> >
> >> > Am Freitag, 22. Juni 2018, 16:37:21 CEST schrieb Masahiro Yamada:
> >> >> Hi Richard,
> >> >>
> >> >>
> >> >> 2018-06-19 21:07 GMT+09:00 Richard Weinberger <[email protected]>:
> >> >>> The denali NAND flash controller needs at least two clocks to operate,
> >> >>> nand_clk and nand_x_clk.
> >> >>> Since 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> >> >>> setup_data_interface()") nand_x_clk is used to derive timing settings.
> >> >>>
> >> >>> Signed-off-by: Richard Weinberger <[email protected]>
> >> >>> ---
> >> >>> Strictly speaking denali needs a ecc_clk too, but AFAIK such a clock
> >> >>> is not present on this SoC.
> >> >>> But my SoCFPGA knowledge is very limited.
> >> >>>
> >> >>> Thanks,
> >> >>> //richard
> >> >>> ---
> >> >>> arch/arm/boot/dts/socfpga.dtsi | 3 ++-
> >> >>> 1 file changed, 2 insertions(+), 1 deletion(-)
> >> >>>
> >> >>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> >> >>> index 486d4e7433ed..562f7b375bbd 100644
> >> >>> --- a/arch/arm/boot/dts/socfpga.dtsi
> >> >>> +++ b/arch/arm/boot/dts/socfpga.dtsi
> >> >>> @@ -754,7 +754,8 @@
> >> >>> reg-names = "nand_data", "denali_reg";
> >> >>> interrupts = <0x0 0x90 0x4>;
> >> >>> dma-mask = <0xffffffff>;
> >> >>> - clocks = <&nand_clk>;
> >> >>> + clocks = <&nand_clk>, <&nand_x_clk>;
> >> >>> + clock-names = "nand", "nand_x";
> >> >>
> >> >>
> >> >> IMHO, this should be
> >> >>
> >> >> clocks = <&nand_clk>, <&nand_x_clk>, <&nand_x_clk>;
> >> >> clock-names = "nand", "nand_x", "ecc";
> >>
> >> No, it should be just the nand_x and ecc.
> >>
> >> There's already a patch to use the nand_x_clk and not the nand_clk.
>
>
> Different people try to fix the problem in different ways.
>
> I think it is due to miscommunication across sub-systems.
Is the series named
mtd: rawnand: denali: add new clocks and improve
setup_data_interface
still valid?
I am about to apply it (patches 2/5 to 5/5) but it looks like the
discussion is still ongoing.
Thanks,
Miquèl
2018-06-27 3:09 GMT+09:00 Miquel Raynal <[email protected]>:
> Hi Masahiro,
>
> On Tue, 26 Jun 2018 11:38:21 +0900, Masahiro Yamada
> <[email protected]> wrote:
>
>> 2018-06-25 23:55 GMT+09:00 Boris Brezillon <[email protected]>:
>> > On Mon, 25 Jun 2018 09:50:18 -0500
>> > Dinh Nguyen <[email protected]> wrote:
>> >
>> >> On 06/22/2018 10:58 AM, Richard Weinberger wrote:
>> >> > Masahiro,
>> >> >
>> >> > Am Freitag, 22. Juni 2018, 16:37:21 CEST schrieb Masahiro Yamada:
>> >> >> Hi Richard,
>> >> >>
>> >> >>
>> >> >> 2018-06-19 21:07 GMT+09:00 Richard Weinberger <[email protected]>:
>> >> >>> The denali NAND flash controller needs at least two clocks to operate,
>> >> >>> nand_clk and nand_x_clk.
>> >> >>> Since 1bb88666775e ("mtd: nand: denali: handle timing parameters by
>> >> >>> setup_data_interface()") nand_x_clk is used to derive timing settings.
>> >> >>>
>> >> >>> Signed-off-by: Richard Weinberger <[email protected]>
>> >> >>> ---
>> >> >>> Strictly speaking denali needs a ecc_clk too, but AFAIK such a clock
>> >> >>> is not present on this SoC.
>> >> >>> But my SoCFPGA knowledge is very limited.
>> >> >>>
>> >> >>> Thanks,
>> >> >>> //richard
>> >> >>> ---
>> >> >>> arch/arm/boot/dts/socfpga.dtsi | 3 ++-
>> >> >>> 1 file changed, 2 insertions(+), 1 deletion(-)
>> >> >>>
>> >> >>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>> >> >>> index 486d4e7433ed..562f7b375bbd 100644
>> >> >>> --- a/arch/arm/boot/dts/socfpga.dtsi
>> >> >>> +++ b/arch/arm/boot/dts/socfpga.dtsi
>> >> >>> @@ -754,7 +754,8 @@
>> >> >>> reg-names = "nand_data", "denali_reg";
>> >> >>> interrupts = <0x0 0x90 0x4>;
>> >> >>> dma-mask = <0xffffffff>;
>> >> >>> - clocks = <&nand_clk>;
>> >> >>> + clocks = <&nand_clk>, <&nand_x_clk>;
>> >> >>> + clock-names = "nand", "nand_x";
>> >> >>
>> >> >>
>> >> >> IMHO, this should be
>> >> >>
>> >> >> clocks = <&nand_clk>, <&nand_x_clk>, <&nand_x_clk>;
>> >> >> clock-names = "nand", "nand_x", "ecc";
>> >>
>> >> No, it should be just the nand_x and ecc.
>> >>
>> >> There's already a patch to use the nand_x_clk and not the nand_clk.
>>
>>
>> Different people try to fix the problem in different ways.
>>
>> I think it is due to miscommunication across sub-systems.
>
> Is the series named
>
> mtd: rawnand: denali: add new clocks and improve
> setup_data_interface
>
> still valid?
Yes.
I believe V4 is valid.
Information for Dinh Nguyen:
http://patchwork.ozlabs.org/patch/933507/
http://patchwork.ozlabs.org/patch/933487/
http://patchwork.ozlabs.org/patch/933494/
http://patchwork.ozlabs.org/patch/933506/
If he is not convinced, I am open to discussion, though.
> I am about to apply it (patches 2/5 to 5/5) but it looks like the
> discussion is still ongoing.
>
> Thanks,
> Miquèl
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
--
Best Regards
Masahiro Yamada
Hi Masahiro,
On 06/26/2018 09:52 PM, Masahiro Yamada wrote:
> 2018-06-27 3:09 GMT+09:00 Miquel Raynal <[email protected]>:
>> Hi Masahiro,
>>
>> On Tue, 26 Jun 2018 11:38:21 +0900, Masahiro Yamada
>> <[email protected]> wrote:
>>
>>> 2018-06-25 23:55 GMT+09:00 Boris Brezillon <[email protected]>:
>>>> On Mon, 25 Jun 2018 09:50:18 -0500
>>>> Dinh Nguyen <[email protected]> wrote:
>>>>
>>>>> On 06/22/2018 10:58 AM, Richard Weinberger wrote:
>>>>>> Masahiro,
>>>>>>
>>>>>> Am Freitag, 22. Juni 2018, 16:37:21 CEST schrieb Masahiro Yamada:
>>>>>>> Hi Richard,
>>>>>>>
>>>>>>>
>>>>>>> 2018-06-19 21:07 GMT+09:00 Richard Weinberger <[email protected]>:
>>>>>>>> The denali NAND flash controller needs at least two clocks to operate,
>>>>>>>> nand_clk and nand_x_clk.
>>>>>>>> Since 1bb88666775e ("mtd: nand: denali: handle timing parameters by
>>>>>>>> setup_data_interface()") nand_x_clk is used to derive timing settings.
>>>>>>>>
>>>>>>>> Signed-off-by: Richard Weinberger <[email protected]>
>>>>>>>> ---
>>>>>>>> Strictly speaking denali needs a ecc_clk too, but AFAIK such a clock
>>>>>>>> is not present on this SoC.
>>>>>>>> But my SoCFPGA knowledge is very limited.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> //richard
>>>>>>>> ---
>>>>>>>> arch/arm/boot/dts/socfpga.dtsi | 3 ++-
>>>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>>>>>>>> index 486d4e7433ed..562f7b375bbd 100644
>>>>>>>> --- a/arch/arm/boot/dts/socfpga.dtsi
>>>>>>>> +++ b/arch/arm/boot/dts/socfpga.dtsi
>>>>>>>> @@ -754,7 +754,8 @@
>>>>>>>> reg-names = "nand_data", "denali_reg";
>>>>>>>> interrupts = <0x0 0x90 0x4>;
>>>>>>>> dma-mask = <0xffffffff>;
>>>>>>>> - clocks = <&nand_clk>;
>>>>>>>> + clocks = <&nand_clk>, <&nand_x_clk>;
>>>>>>>> + clock-names = "nand", "nand_x";
>>>>>>>
>>>>>>>
>>>>>>> IMHO, this should be
>>>>>>>
>>>>>>> clocks = <&nand_clk>, <&nand_x_clk>, <&nand_x_clk>;
>>>>>>> clock-names = "nand", "nand_x", "ecc";
>>>>>
>>>>> No, it should be just the nand_x and ecc.
>>>>>
>>>>> There's already a patch to use the nand_x_clk and not the nand_clk.
>>>
>>>
>>> Different people try to fix the problem in different ways.
>>>
>>> I think it is due to miscommunication across sub-systems.
>>
>> Is the series named
>>
>> mtd: rawnand: denali: add new clocks and improve
>> setup_data_interface
>>
>> still valid?
>
> Yes.
> I believe V4 is valid.
>
>
> Information for Dinh Nguyen:
>
> http://patchwork.ozlabs.org/patch/933507/
> http://patchwork.ozlabs.org/patch/933487/
> http://patchwork.ozlabs.org/patch/933494/
> http://patchwork.ozlabs.org/patch/933506/
>
>
> If he is not convinced, I am open to discussion, though.
I wasn't aware of these patches. This patch is staged to go into
v4.17-rc3 through the arm-soc:
https://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git/commit/arch/arm/boot/dts/socfpga.dtsi?h=fixes&id=4eda9b766b042ea38d84df91581b03f6145a2ab0
I think your patch will handle a case where only 1 clock is passed in,
so it should be okay right?
Dinh
On Wed, 27 Jun 2018 09:55:59 -0500
Dinh Nguyen <[email protected]> wrote:
> Hi Masahiro,
>
> On 06/26/2018 09:52 PM, Masahiro Yamada wrote:
> > 2018-06-27 3:09 GMT+09:00 Miquel Raynal <[email protected]>:
> >> Hi Masahiro,
> >>
> >> On Tue, 26 Jun 2018 11:38:21 +0900, Masahiro Yamada
> >> <[email protected]> wrote:
> >>
> >>> 2018-06-25 23:55 GMT+09:00 Boris Brezillon <[email protected]>:
> >>>> On Mon, 25 Jun 2018 09:50:18 -0500
> >>>> Dinh Nguyen <[email protected]> wrote:
> >>>>
> >>>>> On 06/22/2018 10:58 AM, Richard Weinberger wrote:
> >>>>>> Masahiro,
> >>>>>>
> >>>>>> Am Freitag, 22. Juni 2018, 16:37:21 CEST schrieb Masahiro Yamada:
> >>>>>>> Hi Richard,
> >>>>>>>
> >>>>>>>
> >>>>>>> 2018-06-19 21:07 GMT+09:00 Richard Weinberger <[email protected]>:
> >>>>>>>> The denali NAND flash controller needs at least two clocks to operate,
> >>>>>>>> nand_clk and nand_x_clk.
> >>>>>>>> Since 1bb88666775e ("mtd: nand: denali: handle timing parameters by
> >>>>>>>> setup_data_interface()") nand_x_clk is used to derive timing settings.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Richard Weinberger <[email protected]>
> >>>>>>>> ---
> >>>>>>>> Strictly speaking denali needs a ecc_clk too, but AFAIK such a clock
> >>>>>>>> is not present on this SoC.
> >>>>>>>> But my SoCFPGA knowledge is very limited.
> >>>>>>>>
> >>>>>>>> Thanks,
> >>>>>>>> //richard
> >>>>>>>> ---
> >>>>>>>> arch/arm/boot/dts/socfpga.dtsi | 3 ++-
> >>>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>>>>>
> >>>>>>>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> >>>>>>>> index 486d4e7433ed..562f7b375bbd 100644
> >>>>>>>> --- a/arch/arm/boot/dts/socfpga.dtsi
> >>>>>>>> +++ b/arch/arm/boot/dts/socfpga.dtsi
> >>>>>>>> @@ -754,7 +754,8 @@
> >>>>>>>> reg-names = "nand_data", "denali_reg";
> >>>>>>>> interrupts = <0x0 0x90 0x4>;
> >>>>>>>> dma-mask = <0xffffffff>;
> >>>>>>>> - clocks = <&nand_clk>;
> >>>>>>>> + clocks = <&nand_clk>, <&nand_x_clk>;
> >>>>>>>> + clock-names = "nand", "nand_x";
> >>>>>>>
> >>>>>>>
> >>>>>>> IMHO, this should be
> >>>>>>>
> >>>>>>> clocks = <&nand_clk>, <&nand_x_clk>, <&nand_x_clk>;
> >>>>>>> clock-names = "nand", "nand_x", "ecc";
> >>>>>
> >>>>> No, it should be just the nand_x and ecc.
> >>>>>
> >>>>> There's already a patch to use the nand_x_clk and not the nand_clk.
> >>>
> >>>
> >>> Different people try to fix the problem in different ways.
> >>>
> >>> I think it is due to miscommunication across sub-systems.
> >>
> >> Is the series named
> >>
> >> mtd: rawnand: denali: add new clocks and improve
> >> setup_data_interface
> >>
> >> still valid?
> >
> > Yes.
> > I believe V4 is valid.
> >
> >
> > Information for Dinh Nguyen:
> >
> > http://patchwork.ozlabs.org/patch/933507/
> > http://patchwork.ozlabs.org/patch/933487/
> > http://patchwork.ozlabs.org/patch/933494/
> > http://patchwork.ozlabs.org/patch/933506/
> >
> >
> > If he is not convinced, I am open to discussion, though.
>
> I wasn't aware of these patches. This patch is staged to go into
> v4.17-rc3 through the arm-soc:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git/commit/arch/arm/boot/dts/socfpga.dtsi?h=fixes&id=4eda9b766b042ea38d84df91581b03f6145a2ab0
I don't think that's a problem. Masahiro's changes are targeting 4.19,
so these new DT changes should go in after that anyway.
>
> I think your patch will handle a case where only 1 clock is passed in,
> so it should be okay right?
Just because the DT changes you made fixed the problem doesn't mean it's
the right solution. DT should represent HW blocks and HW connections.
According to Masahiro, the denali NAND controller takes 3 clk signals
in input, so those 3 clks should be represented in the DT with 3
entries in the "clocks" and "clock-names" props. The fact that, in the
socfpga case, all clk input signals are actually connected to the same
clk provider does not mean you should have a single entry in "clocks"
and "clock-names". Instead, you should have all 3 entries of clocks
pointing to the same clk provider.
Hi Dinh,
2018-06-27 23:55 GMT+09:00 Dinh Nguyen <[email protected]>:
> Hi Masahiro,
>
> On 06/26/2018 09:52 PM, Masahiro Yamada wrote:
>> 2018-06-27 3:09 GMT+09:00 Miquel Raynal <[email protected]>:
>>> Hi Masahiro,
>>>
>>> On Tue, 26 Jun 2018 11:38:21 +0900, Masahiro Yamada
>>> <[email protected]> wrote:
>>>
>>>> 2018-06-25 23:55 GMT+09:00 Boris Brezillon <[email protected]>:
>>>>> On Mon, 25 Jun 2018 09:50:18 -0500
>>>>> Dinh Nguyen <[email protected]> wrote:
>>>>>
>>>>>> On 06/22/2018 10:58 AM, Richard Weinberger wrote:
>>>>>>> Masahiro,
>>>>>>>
>>>>>>> Am Freitag, 22. Juni 2018, 16:37:21 CEST schrieb Masahiro Yamada:
>>>>>>>> Hi Richard,
>>>>>>>>
>>>>>>>>
>>>>>>>> 2018-06-19 21:07 GMT+09:00 Richard Weinberger <[email protected]>:
>>>>>>>>> The denali NAND flash controller needs at least two clocks to operate,
>>>>>>>>> nand_clk and nand_x_clk.
>>>>>>>>> Since 1bb88666775e ("mtd: nand: denali: handle timing parameters by
>>>>>>>>> setup_data_interface()") nand_x_clk is used to derive timing settings.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Richard Weinberger <[email protected]>
>>>>>>>>> ---
>>>>>>>>> Strictly speaking denali needs a ecc_clk too, but AFAIK such a clock
>>>>>>>>> is not present on this SoC.
>>>>>>>>> But my SoCFPGA knowledge is very limited.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> //richard
>>>>>>>>> ---
>>>>>>>>> arch/arm/boot/dts/socfpga.dtsi | 3 ++-
>>>>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>>>>>>>>> index 486d4e7433ed..562f7b375bbd 100644
>>>>>>>>> --- a/arch/arm/boot/dts/socfpga.dtsi
>>>>>>>>> +++ b/arch/arm/boot/dts/socfpga.dtsi
>>>>>>>>> @@ -754,7 +754,8 @@
>>>>>>>>> reg-names = "nand_data", "denali_reg";
>>>>>>>>> interrupts = <0x0 0x90 0x4>;
>>>>>>>>> dma-mask = <0xffffffff>;
>>>>>>>>> - clocks = <&nand_clk>;
>>>>>>>>> + clocks = <&nand_clk>, <&nand_x_clk>;
>>>>>>>>> + clock-names = "nand", "nand_x";
>>>>>>>>
>>>>>>>>
>>>>>>>> IMHO, this should be
>>>>>>>>
>>>>>>>> clocks = <&nand_clk>, <&nand_x_clk>, <&nand_x_clk>;
>>>>>>>> clock-names = "nand", "nand_x", "ecc";
>>>>>>
>>>>>> No, it should be just the nand_x and ecc.
>>>>>>
>>>>>> There's already a patch to use the nand_x_clk and not the nand_clk.
>>>>
>>>>
>>>> Different people try to fix the problem in different ways.
>>>>
>>>> I think it is due to miscommunication across sub-systems.
>>>
>>> Is the series named
>>>
>>> mtd: rawnand: denali: add new clocks and improve
>>> setup_data_interface
>>>
>>> still valid?
>>
>> Yes.
>> I believe V4 is valid.
>>
>>
>> Information for Dinh Nguyen:
>>
>> http://patchwork.ozlabs.org/patch/933507/
>> http://patchwork.ozlabs.org/patch/933487/
>> http://patchwork.ozlabs.org/patch/933494/
>> http://patchwork.ozlabs.org/patch/933506/
>>
>>
>> If he is not convinced, I am open to discussion, though.
>
> I wasn't aware of these patches. This patch is staged to go into
> v4.17-rc3 through the arm-soc:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git/commit/arch/arm/boot/dts/socfpga.dtsi?h=fixes&id=4eda9b766b042ea38d84df91581b03f6145a2ab0
>
> I think your patch will handle a case where only 1 clock is passed in,
> so it should be okay right?
>
I should be OK,
but please consider the proper fix for v4.19
as Boris suggested.
--
Best Regards
Masahiro Yamada
On 07/01/2018 08:35 PM, Masahiro Yamada wrote:
> Hi Dinh,
>
> 2018-06-27 23:55 GMT+09:00 Dinh Nguyen <[email protected]>:
>> Hi Masahiro,
>>
>> On 06/26/2018 09:52 PM, Masahiro Yamada wrote:
>>> 2018-06-27 3:09 GMT+09:00 Miquel Raynal <[email protected]>:
>>>> Hi Masahiro,
>>>>
>>>> On Tue, 26 Jun 2018 11:38:21 +0900, Masahiro Yamada
>>>> <[email protected]> wrote:
>>>>
>>>>> 2018-06-25 23:55 GMT+09:00 Boris Brezillon <[email protected]>:
>>>>>> On Mon, 25 Jun 2018 09:50:18 -0500
>>>>>> Dinh Nguyen <[email protected]> wrote:
>>>>>>
>>>>>>> On 06/22/2018 10:58 AM, Richard Weinberger wrote:
>>>>>>>> Masahiro,
>>>>>>>>
>>>>>>>> Am Freitag, 22. Juni 2018, 16:37:21 CEST schrieb Masahiro Yamada:
>>>>>>>>> Hi Richard,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 2018-06-19 21:07 GMT+09:00 Richard Weinberger <[email protected]>:
>>>>>>>>>> The denali NAND flash controller needs at least two clocks to operate,
>>>>>>>>>> nand_clk and nand_x_clk.
>>>>>>>>>> Since 1bb88666775e ("mtd: nand: denali: handle timing parameters by
>>>>>>>>>> setup_data_interface()") nand_x_clk is used to derive timing settings.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Richard Weinberger <[email protected]>
>>>>>>>>>> ---
>>>>>>>>>> Strictly speaking denali needs a ecc_clk too, but AFAIK such a clock
>>>>>>>>>> is not present on this SoC.
>>>>>>>>>> But my SoCFPGA knowledge is very limited.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> //richard
>>>>>>>>>> ---
>>>>>>>>>> arch/arm/boot/dts/socfpga.dtsi | 3 ++-
>>>>>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>>>>>>>>>> index 486d4e7433ed..562f7b375bbd 100644
>>>>>>>>>> --- a/arch/arm/boot/dts/socfpga.dtsi
>>>>>>>>>> +++ b/arch/arm/boot/dts/socfpga.dtsi
>>>>>>>>>> @@ -754,7 +754,8 @@
>>>>>>>>>> reg-names = "nand_data", "denali_reg";
>>>>>>>>>> interrupts = <0x0 0x90 0x4>;
>>>>>>>>>> dma-mask = <0xffffffff>;
>>>>>>>>>> - clocks = <&nand_clk>;
>>>>>>>>>> + clocks = <&nand_clk>, <&nand_x_clk>;
>>>>>>>>>> + clock-names = "nand", "nand_x";
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> IMHO, this should be
>>>>>>>>>
>>>>>>>>> clocks = <&nand_clk>, <&nand_x_clk>, <&nand_x_clk>;
>>>>>>>>> clock-names = "nand", "nand_x", "ecc";
>>>>>>>
>>>>>>> No, it should be just the nand_x and ecc.
>>>>>>>
>>>>>>> There's already a patch to use the nand_x_clk and not the nand_clk.
>>>>>
>>>>>
>>>>> Different people try to fix the problem in different ways.
>>>>>
>>>>> I think it is due to miscommunication across sub-systems.
>>>>
>>>> Is the series named
>>>>
>>>> mtd: rawnand: denali: add new clocks and improve
>>>> setup_data_interface
>>>>
>>>> still valid?
>>>
>>> Yes.
>>> I believe V4 is valid.
>>>
>>>
>>> Information for Dinh Nguyen:
>>>
>>> http://patchwork.ozlabs.org/patch/933507/
>>> http://patchwork.ozlabs.org/patch/933487/
>>> http://patchwork.ozlabs.org/patch/933494/
>>> http://patchwork.ozlabs.org/patch/933506/
>>>
>>>
>>> If he is not convinced, I am open to discussion, though.
>>
>> I wasn't aware of these patches. This patch is staged to go into
>> v4.17-rc3 through the arm-soc:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git/commit/arch/arm/boot/dts/socfpga.dtsi?h=fixes&id=4eda9b766b042ea38d84df91581b03f6145a2ab0
>>
>> I think your patch will handle a case where only 1 clock is passed in,
>> so it should be okay right?
>>
>
> I should be OK,
> but please consider the proper fix for v4.19
> as Boris suggested.
>
Yes, I will spin up a patch.
Dinh