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

[Boris Brezillon]

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.

Regards,

Boris