Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC

[Ramuthevar, Vadivel MuruganX]

Hi Boris,

On 5/5/2020 3:00 pm, Boris Brezillon wrote:
> Hello Vadivel,
>
> On Tue, 5 May 2020 13:28:58 +0800
> "Ramuthevar, Vadivel MuruganX"
> <vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx> wrote:
>   
> > > > > > > >       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! for the keep reviewing.
> >
> > SW Address translation is not required, after discussion with HW team ,
> > came to know that 0x17400 and 0x17C00 internal to the SoC.
> >
> > NOC will translate 0xE1XX... to FPI address 0x17X... internally.
> > There is an address translation in the NOC.
> > 0x17X... is not visible to user.
> >
> > so far added hard-coded values to CS0 and CS1 is not at required.
> > I will change the code accordingly and sent to you.
>
> Hm, you told me last week that writing wrong values to this register
> caused the NAND controller to not work properly (you even had code that
> was overwriting the dynamically calculated values by hardcoded ones, so
> I suspect it indeed didn't work) and now you say this write to
> EBU_ADDR_SEL() is optional?! Sorry but it's hard to believe, and I've
> received so many contradictory information from you on that matter that
> I can't really tell which one is correct. Not sure I want to keep
> reviewing new versions of this driver in this context.

oh my bad really sorry , since last week based on the input given from 
person who has worked on legacy IP, but now I have discussed with 
low-level HW team and confirmed. also we don't have proper document 
reference manual since it's new SoC.

Please forgive me , this shouldn't be happen once again, Thanks a lot!!

Regards
Vadivel
> Regards,
>
> Boris