Messages in this thread | | | Subject | Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC | From | "Ramuthevar, Vadivel MuruganX" <> | Date | Mon, 4 May 2020 17:17:03 +0800 |
| |
Hi Boris,
On 4/5/2020 4:58 pm, Boris Brezillon wrote: > On Mon, 4 May 2020 16:50:08 +0800 > "Ramuthevar, Vadivel MuruganX" > <vadivel.muruganx.ramuthevar@linux.intel.com> wrote: > >> Hi Boris, >> >> On 4/5/2020 3:17 pm, Boris Brezillon wrote: >>> On Mon, 4 May 2020 15:15:08 +0800 >>> "Ramuthevar, Vadivel MuruganX" >>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote: >>> >>>> Hi Boris, >>>> >>>> Thank you very much for the prompt review and suggestions... >>>> >>>> On 4/5/2020 3:08 pm, Boris Brezillon wrote: >>>>> On Mon, 4 May 2020 10:02:35 +0800 >>>>> "Ramuthevar, Vadivel MuruganX" >>>>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote: >>>>> >>>>>> Hi Boris, >>>>>> >>>>>> On 30/4/2020 9:01 pm, Boris Brezillon wrote: >>>>>>> On Thu, 30 Apr 2020 14:36:00 +0200 >>>>>>> Boris Brezillon<boris.brezillon@collabora.com> wrote: >>>>>>> >>>>>>>> On Thu, 30 Apr 2020 17:07:03 +0800 >>>>>>>> "Ramuthevar, Vadivel MuruganX" >>>>>>>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote: >>>>>>>> >>>>>>>>>>>> The question is, is it the same value we have in nand_pa or it is >>>>>>>>>>>> different? >>>>>>>>>>>> >>>>>>>>>>> Different address which is 0xE1400000 NAND_BASE_PHY address. >>>>>>>>>> Then why didn't you tell me they didn't match when I suggested to pass >>>>>>>>> sorry, because you keep asking nand_pa after that only I realized that. >>>>>>>>> >>>>>>>>>> nand_pa? So now the question is, what does this address represent? >>>>>>>>> EBU-MODULE >>>>>>>>> _________ _______________________ >>>>>>>>> | | | |NAND CTRL | >>>>>>>>> | FPI BUS |==>| CS0(0x174) | 0xE100 ( 0xE14/0xE1C) NAND_PHY_BASE >>>>>>>>> |_________| |_CS1(0x17C)_|__________ | >>>>>>>>> >>>>>>>>> EBU_CONRTROLLER_BASE : 0xE0F0_0000 >>>>>>>>> HSNAND_BASE: 0xE100_0000 >>>>>>>>> NAND_CS0: 0xE140_0000 >>>>>>>>> NAND_CS1: 0xE1C0_0000 >>>>>>>>> >>>>>>>>> MEM_REGION_BASE_CS0: 0x17400 (internal to ebu controller ) >>>>>>>>> MEM_REGION_BASE_CS1: 0x17C00 >>>>>>>>> >>>>>>>> Hm, I wonder if we shouldn't use a 'ranges' property to describe this >>>>>>>> address translation. Something like >>>>>>>> >>>>>>>> ebu@xxx { >>>>>>>> ranges = <0x17400000 0xe1400000 0x1000>, >>>>>>>> <0x17c00000 0xe1c00000 0x1000>; >>>>>>>> reg = <0x17400000>, <0x17c00000>; >>>>>>>> reg-names = "cs-0", "cs-1"; >>>>>>>> } >>>>>>>> >>>>>>>> The translated address (0xE1X00000) will be available in res->start, >>>>>>>> and the non-translated one (0x17X00000) can be retrieved with >>>>>>>> of_get_address(). All you'd have to do then would be calculate the >>>>>>>> mask: >>>>>>>> >>>>>>>> mask = (translated_address & original_address) >> 22; >>>>>>>> num_comp_bits = fls(mask); >>>>>>>> WARN_ON(mask != GENMASK(num_comp_bits - 1, 0)); >>>>>>>> >>>>>>>> Which allows you to properly set the ADDR_SEL() register without >>>>>>>> relying on some hardcoded values: >>>>>>>> >>>>>>>> writel(original_address | EBU_ADDR_SEL_REGEN | >>>>>>>> EBU_ADDR_COMP_BITS(num_comp_bits), >>>>>>>> ebu_host->ebu + EBU_ADDR_SEL(csid)); >>>>>>>> >>>>>>>> That's quite important if we want to merge the xway NAND driver with >>>>>>>> this one. >>>>>>> Looks like the translation is done at the FPI bus declaration level (see >>>>>>> [1]). We really need to see the big picture to take a wise decision >>>>>>> about the bindings. Would you mind pasting your dsti/dts files >>>>>>> somewhere? It feels like the NAND controller is a sub-part of a more >>>>>>> generic 'memory' controller, in which case the NAND controller should be >>>>>>> declared as a child of this generic memory bus (called localbus in [1], >>>>>>> but maybe EBU is more accurate). >>>>>>> >>>>>>> [1]https://github.com/xieyaxiongfly/Atheros_CSI_tool_OpenWRT_src/blob/master/target/linux/lantiq/files-4.14/arch/mips/boot/dts/vr9.dtsi#L162 >>>>>> >>>>>> ebu_nand: ebu_nand@e0f00000 { >>>>>> compatible = "intel,lgm-ebu-nand"; >>>>>> reg = <0xe0f00000 0x100 >>>>>> 0xe1000000 0x300 >>>>>> 0xe1400000 0x80000 >>>>>> 0xe1c00000 0x10000>; >>>>>> reg-names = "ebunand", "hsnand", "nand_cs0", nand_cs1"; >>>>>> dmas = <&dma0 8>, <&dma0 9>; >>>>>> dma-names = "ebu_rx", "ebu_tx"; >>>>>> clocks = <&cgu0 LGM_GCLK_EBU>; >>>>>> }; >>>>>> >>>>>> >>>>>> &ebu_nand { >>>>>> status = "disabled"; >>>>>> nand,cs = <1>; >>>>>> nand-ecc-mode = "hw"; >>>>>> pinctrl-names = "default"; >>>>>> pinctrl-0 = <&ebu_nand_base &ebu_cs1>; >>>>>> }; >>>>>> >>>>>>> >>>>> Ok. If I understand the SoC topology correctly it should actually be >>>>> something like that: >>>>> >>>>> { >>>>> ... >>>>> fpi@xxxxx { >>>>> compatible = "intel,lgm-fpi", "simple-bus"; >>>>> >>>>> /* You might have other ranges to define here */ >>>>> ranges = <0x16000000 0xe0000000 0x1000000>; >>>>> >>>>> ... >>>> >>>> Sorry, we do not have fpi tree node in our dts/dtsi file instead we have >>>> the below one.. , that also not included the major peripherals >>>> controllers node. >>>> /* Special part from CPU core */ >>>> core: core { >>>> compatible = "intel,core", "simple-bus"; >>>> #address-cells = <1>; >>>> #size-cells = <1>; >>>> ranges; >>>> >>>> ioapic1: interrupt-controller@fec00000 { >>>> #interrupt-cells = <2>; >>>> #address-cells = <0>; >>>> compatible = "intel,ce4100-ioapic"; >>>> interrupt-controller; >>>> reg = <0xfec00000 0x1000>; >>>> nr_entries = <256>; >>>> }; >>>> >>>> hpet: timer@fed00000 { >>>> compatible = "intel,ce4100-hpet"; >>>> reg = <0xfed00000 0x400>; >>>> }; >>>> >>>> lapic0: interrupt-controller@fee00000 { >>>> compatible = "intel,ce4100-lapic"; >>>> reg = <0xfee00000 0x1000>; >>>> no_pic_mode; >>>> }; >>>> }; >>>> >>>> other than this, rest all in independent node . >>> >>> But you do have an FPI bus, right? If this is the case it should be >>> represented. >> >> Yes, FPI bus is slave to core which connects all the peripherals. >> >> Or is the "intel,core" bus actually the FPI bus that you >>> named differently? >> >> FPI slave bus connects to core bus by OCP bridge, so here it is named >> FPI bus, but SW perspective didn't have root tree which has all >> sub-nodes, as of now each peripheral has its own node. > > Duh, not sure that's a good idea to hide that, especially since you > have to describe the address translation that happens when crossing the > FPI bus (the ranges thing I mentioned previously).
Thanks Boris, Sure I will do as you have suggested.
can I proceed to send next patch-set.
Regards Vadivel
| |