[PATCH] arm: dts: socfpga: denali needs nand_x_clk too

[PATCH] arm: dts: socfpga: denali needs nand_x_clk too

[Richard Weinberger]

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 <richard@nod.at>
---
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

Re: [PATCH] arm: dts: socfpga: denali needs nand_x_clk too

[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 <richard@nod.at>
> ---
> 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

Re: [PATCH] arm: dts: socfpga: denali needs nand_x_clk too

[Richard Weinberger]

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 <richard@nod.at>
> > ---
> > 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

Re: [PATCH] arm: dts: socfpga: denali needs nand_x_clk too

[Marek Vasut]

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 <richard@nod.at>
>>> ---
>>> 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 <marek.vasut@gmail.com>

-- 
Best regards,
Marek Vasut

Re: [PATCH] arm: dts: socfpga: denali needs nand_x_clk too

[Masahiro Yamada]

Hi Richard,


2018-06-19 21:07 GMT+09:00 Richard Weinberger <richard@nod.at>:
> 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 <richard@nod.at>
> ---
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best Regards
Masahiro Yamada

Re: [PATCH] arm: dts: socfpga: denali needs nand_x_clk too

[Richard Weinberger]

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 <richard@nod.at>:
> > 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 <richard@nod.at>
> > ---
> > 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

Re: [PATCH] arm: dts: socfpga: denali needs nand_x_clk too

[Dinh Nguyen]

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 <richard@nod.at>:
>>> 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 <richard@nod.at>
>>> ---
>>> 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

Re: [PATCH] arm: dts: socfpga: denali needs nand_x_clk too

[Boris Brezillon]

On Mon, 25 Jun 2018 09:50:18 -0500
Dinh Nguyen <dinguyen@kernel.org> 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 <richard@nod.at>:  
> >>> 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 <richard@nod.at>
> >>> ---
> >>> 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?

Re: [PATCH] arm: dts: socfpga: denali needs nand_x_clk too

[Masahiro Yamada]

2018-06-25 23:55 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:
> On Mon, 25 Jun 2018 09:50:18 -0500
> Dinh Nguyen <dinguyen@kernel.org> 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 <richard@nod.at>:
>> >>> 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 <richard@nod.at>
>> >>> ---
>> >>> 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

Re: [PATCH] arm: dts: socfpga: denali needs nand_x_clk too

[Miquel Raynal]

Hi Masahiro,

On Tue, 26 Jun 2018 11:38:21 +0900, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:

> 2018-06-25 23:55 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:
> > On Mon, 25 Jun 2018 09:50:18 -0500
> > Dinh Nguyen <dinguyen@kernel.org> 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 <richard@nod.at>:  
> >> >>> 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 <richard@nod.at>
> >> >>> ---
> >> >>> 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

Re: [PATCH] arm: dts: socfpga: denali needs nand_x_clk too

[Masahiro Yamada]

2018-06-27 3:09 GMT+09:00 Miquel Raynal <miquel.raynal@bootlin.com>:
> Hi Masahiro,
>
> On Tue, 26 Jun 2018 11:38:21 +0900, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>
>> 2018-06-25 23:55 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:
>> > On Mon, 25 Jun 2018 09:50:18 -0500
>> > Dinh Nguyen <dinguyen@kernel.org> 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 <richard@nod.at>:
>> >> >>> 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 <richard@nod.at>
>> >> >>> ---
>> >> >>> 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

Re: [PATCH] arm: dts: socfpga: denali needs nand_x_clk too

[Dinh Nguyen]

Hi Masahiro,

On 06/26/2018 09:52 PM, Masahiro Yamada wrote:
> 2018-06-27 3:09 GMT+09:00 Miquel Raynal <miquel.raynal@bootlin.com>:
>> Hi Masahiro,
>>
>> On Tue, 26 Jun 2018 11:38:21 +0900, Masahiro Yamada
>> <yamada.masahiro@socionext.com> wrote:
>>
>>> 2018-06-25 23:55 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:
>>>> On Mon, 25 Jun 2018 09:50:18 -0500
>>>> Dinh Nguyen <dinguyen@kernel.org> 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 <richard@nod.at>:
>>>>>>>> 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 <richard@nod.at>
>>>>>>>> ---
>>>>>>>> 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

Re: [PATCH] arm: dts: socfpga: denali needs nand_x_clk too

[Boris Brezillon]

On Wed, 27 Jun 2018 09:55:59 -0500
Dinh Nguyen <dinguyen@kernel.org> wrote:

> Hi Masahiro,
> 
> On 06/26/2018 09:52 PM, Masahiro Yamada wrote:
> > 2018-06-27 3:09 GMT+09:00 Miquel Raynal <miquel.raynal@bootlin.com>:  
> >> Hi Masahiro,
> >>
> >> On Tue, 26 Jun 2018 11:38:21 +0900, Masahiro Yamada
> >> <yamada.masahiro@socionext.com> wrote:
> >>  
> >>> 2018-06-25 23:55 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:  
> >>>> On Mon, 25 Jun 2018 09:50:18 -0500
> >>>> Dinh Nguyen <dinguyen@kernel.org> 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 <richard@nod.at>:  
> >>>>>>>> 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 <richard@nod.at>
> >>>>>>>> ---
> >>>>>>>> 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.

Re: [PATCH] arm: dts: socfpga: denali needs nand_x_clk too

[Masahiro Yamada]

Hi Dinh,

2018-06-27 23:55 GMT+09:00 Dinh Nguyen <dinguyen@kernel.org>:
> Hi Masahiro,
>
> On 06/26/2018 09:52 PM, Masahiro Yamada wrote:
>> 2018-06-27 3:09 GMT+09:00 Miquel Raynal <miquel.raynal@bootlin.com>:
>>> Hi Masahiro,
>>>
>>> On Tue, 26 Jun 2018 11:38:21 +0900, Masahiro Yamada
>>> <yamada.masahiro@socionext.com> wrote:
>>>
>>>> 2018-06-25 23:55 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:
>>>>> On Mon, 25 Jun 2018 09:50:18 -0500
>>>>> Dinh Nguyen <dinguyen@kernel.org> 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 <richard@nod.at>:
>>>>>>>>> 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 <richard@nod.at>
>>>>>>>>> ---
>>>>>>>>> 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

Re: [PATCH] arm: dts: socfpga: denali needs nand_x_clk too

[Dinh Nguyen]

On 07/01/2018 08:35 PM, Masahiro Yamada wrote:
> Hi Dinh,
> 
> 2018-06-27 23:55 GMT+09:00 Dinh Nguyen <dinguyen@kernel.org>:
>> Hi Masahiro,
>>
>> On 06/26/2018 09:52 PM, Masahiro Yamada wrote:
>>> 2018-06-27 3:09 GMT+09:00 Miquel Raynal <miquel.raynal@bootlin.com>:
>>>> Hi Masahiro,
>>>>
>>>> On Tue, 26 Jun 2018 11:38:21 +0900, Masahiro Yamada
>>>> <yamada.masahiro@socionext.com> wrote:
>>>>
>>>>> 2018-06-25 23:55 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:
>>>>>> On Mon, 25 Jun 2018 09:50:18 -0500
>>>>>> Dinh Nguyen <dinguyen@kernel.org> 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 <richard@nod.at>:
>>>>>>>>>> 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 <richard@nod.at>
>>>>>>>>>> ---
>>>>>>>>>> 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