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@xxxxxxxxxx> 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@xxxxxxxxxxx>:  
> >> Hi Masahiro,
> >>
> >> On Tue, 26 Jun 2018 11:38:21 +0900, Masahiro Yamada
> >> <yamada.masahiro@xxxxxxxxxxxxx> wrote:
> >>  
> >>> 2018-06-25 23:55 GMT+09:00 Boris Brezillon <boris.brezillon@xxxxxxxxxxx>:  
> >>>> On Mon, 25 Jun 2018 09:50:18 -0500
> >>>> Dinh Nguyen <dinguyen@xxxxxxxxxx> 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@xxxxxx>:  
> >>>>>>>> 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@xxxxxx>
> >>>>>>>> ---
> >>>>>>>> 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.