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

["Ramuthevar, Vadivel MuruganX" <vadivel.muruganx.ramuthevar@linux.intel.com>]

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@linux.intel.com> 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
>