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

[Ramuthevar, Vadivel MuruganX]

Hi Boris,

  Thank you very much for the review comments and your time...
On 7/5/2020 2:48 pm, Boris Brezillon wrote:
> On Thu, 7 May 2020 14:38:52 +0800
> "Ramuthevar, Vadivel MuruganX"
> <vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx> wrote:
>
>> Hi Boris,
>>
>>    Thank you very much for the review comments and your time...
>>
>> On 7/5/2020 2:27 pm, Boris Brezillon wrote:
>>> On Thu, 7 May 2020 14:13:42 +0800
>>> "Ramuthevar, Vadivel MuruganX"
>>> <vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx> wrote:
>>>
>>>> Hi Boris,
>>>>
>>>>      Thank you very much for the review comments and your time...
>>>>
>>>> On 7/5/2020 1:28 pm, Boris Brezillon wrote:
>>>>> On Thu,  7 May 2020 08:15:37 +0800
>>>>> "Ramuthevar,Vadivel MuruganX"
>>>>> <vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx> wrote:
>>>>>
>>>>>> +	reg = readl(ebu_host->ebu + EBU_ADDR_SEL(ebu_host->cs_num));
>>>>>> +	writel(reg | EBU_ADDR_MASK(5) | EBU_ADDR_SEL_REGEN,
>>>>>> +	       ebu_host->ebu + EBU_ADDR_SEL(ebu_host->cs_num));
>>>>> Seriously, did you really think I would not notice what you're doing
>>>>> here?
>>>> Yes , I know that you have very good understanding about this.
>>>>    You're reading the previous value which either contains a default
>>>>> mapping or has the mapping set by the bootloader, and write it 
back to
>>>>> the register along with a new mask and the REGEN bit set (which
>>>>> BTW is wrong since you don't mask out other fields before updating
>>>>> them).
>>>> There is no other field get overwritten
>>>>    This confirms that this Core -> FPI address translation exists
>>>>> and has to be set properly, so please stop lying about that.
>>>> Sorry, there is no SW translation, as I have mentioned that it's
>>>> optional only, for safer side , reading and writing the default 
values.
>>> Then write EBU_ADDR_SEL_REGEN and we'll if see that works. I suspect it
>>> won't.
>> You mean, without reading just writing EBU_ADDR_SEL_REGEN bit alone in
>> EBU_ADDR_SELx , as you said it won't work because it overwrites 0x174
>> with 0x0 values so BASE is lost.
> Which confirms that this mapping has to be defined.
Sure, Noted.
>> either we can leave it or read & write with ORed | EBU_ADDR_SEL_REGEN
> None of this is acceptable IMO. You have to build the value based on the
> address translation described in the DT. Why are you so reluctant to
> this approach?
Agreed!, will derive the values(0x174/0x17C) to be written into these 
registers based on the chip select (CS0/CS1)
Address_sel0_register: 0xE0F0_0020
Address_sel1_register: 0xE0F0_0024
Bits : 31...12|11...8| 7..4 |3..2|  1   |  0
flags:  BASE  |------| MASK | -- | MRME | REGEN

BASE : 0x17400 /0x17C00 to be written into 31:12 based on the chip selection
MASK: 5: bits 26:22 to included address comparison
MRME: Memory Region Memory Enable
REGEN: Memory Region Access Enable

As you have suggested to get the above base values from DT and update in 
driver, will do that.

Thanks!
Regards
Vadivel
>> Please correct me if anything is wrong, Thanks!
>>>
>>>> The memory region to enabled that's my concern so written the same
>>>> register values.
>>> I don't buy that, sorry.
>>>
>>>> This will not be impact other fields, so please see below for 
reference
>>>>
>>>> The EBU Address Select Registers EBU_ADDR_SEL_0 to EBU_ADDSEL3 
establish
>>>> and control memory regions for external accesses.
>>>>
>>>> Reset Value: 17400001H
>>> See, as suspected the reset value is exactly what you expect.
>> Yes , that's the reason said being optional.
> Then it's not optional. It just works because you use the default
> va