| Message ID | 20180619120719.26921-1-richard@nod.at |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | arm: dts: socfpga: denali needs nand_x_clk too | expand |
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"; > }; > >
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
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>
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
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
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
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?
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?
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
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/
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
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.
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.
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
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"; };
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(-)