Messages in this thread | | | Subject | Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC | From | "Ramuthevar, Vadivel MuruganX" <> | Date | Thu, 30 Apr 2020 15:50:30 +0800 |
| |
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@linux.intel.com> 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@linux.intel.com> 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@linux.intel.com> 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 >
| |