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

From: Ramuthevar, Vadivel MuruganX
Date: Thu Apr 30 2020 - 03:50:49 EST


Hi Boris,

Thank you very much for keep reviewing the patches and more queries...

On 29/4/2020 11:31 pm, Boris Brezillon wrote:
On Wed, 29 Apr 2020 23:18:31 +0800
"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx> wrote:

Hi Boris,

On 29/4/2020 10:48 pm, Boris Brezillon wrote:
On Wed, 29 Apr 2020 22:33:37 +0800
"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx> wrote:
Hi Boris,

On 29/4/2020 10:22 pm, Boris Brezillon wrote:
On Wed, 29 Apr 2020 18:42:05 +0800
"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx> wrote:
+
+#define EBU_ADDR_SEL(n) (0x20 + (n) * 4)
+#define EBU_ADDR_MASK (5 << 4)

It's still unclear what ADDR_MASK is for. Can you add a comment
explaining what it does?

Thank you Boris, keep review and giving inputs, will update.

Can you please explain it here before sending a new version?

Memory Region Address Mask:
Specifies the number of right-most bits in the base address that should
be included in the address comparison. bits positions(7:4).

Okay, then the macro should be

#define EBU_ADDR_MASK(x) ((x) << 4)

And now I'd like you to explain why 5 is the right value for that field
(I guess that has to do with the position of the CS/ALE/CLE pins).

5 : bit 26, 25, 24, 23, 22 to be included for comparison in the ADDR_SELx , it compares only 5 bits.



+#define EBU_ADDR_SEL_REGEN 0x1

+
+ writel(lower_32_bits(ebu_host->cs[ebu_host->cs_num].nand_pa) |
+ EBU_ADDR_SEL_REGEN | EBU_ADDR_MASK,
+ ebu_host->ebu + EBU_ADDR_SEL(reg));

You set EBU_ADDR_SEL(reg) once here...

+
+ writel(EBU_MEM_BASE_CS_0 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN,
+ ebu_host->ebu + EBU_ADDR_SEL(0));
+ writel(EBU_MEM_BASE_CS_1 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN,
+ ebu_host->ebu + EBU_ADDR_SEL(reg));

... and a second time here. That sounds like overwriting the
EBU_ADDR_SEL(reg) register to me.


That's super weird. You seem to set EBU_ADDR_SEL(reg) twice. Are you
sure that's needed, and are we setting EBU_ADDR_SEL(0) here?

You are right, its weird only, but we need it, since different chip
select has different memory region access address.

Well, that doesn't make any sense, the second write to
EBU_ADDR_SEL(reg) overrides the first one, meaning that nand_pa is
actually never written to ADDR_SEL(reg).

it will not overwrite the first one, since two different registers
EBU_ADDR_SEL_0 EBU_ADDR_SEL 20H
EBU_ADDR_SEL_1 EBU_ADDR_SEL 24H

See my above.


it is an internal address selection w.r.t chip select for nand physical
address update.



Yes , we are setting both CS0 and CS1 memory access region, if you have
any concern to optimize, please suggest me, Thanks!

If you want to setup both CS, and the address written in EBU_ADDR_SEL(x)
is really related to the nand_pa address, then retrieve resources for
all CS ranges.
If it's not related, please explain what those
EBU_MEM_BASE_CS_X values encode.

Memory Region Base Address
FPI Bus addresses are compared to this base address in conjunction with
the mask control(EBU_ADDR_MASK). Driver need to program this field!

That's not explaining what the base address should be. Is 'nand_pa' the
value we should have there?

The one prorgrammed in the addr_sel register is used by the HW controller, it remaps to 0x174XX-> CS0 and 0x17CXX->CS1.
The hardware itself, decodes only for 1740xx/17c0xx, other random values cannot be programmed

Regards
Vadivel