From: "Ramuthevar, Vadivel MuruganX" <vadivel.muruganx.ramuthevar@linux.intel.com> To: Boris Brezillon <boris.brezillon@collabora.com> Cc: qi-ming.wu@intel.com, linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org, devicetree@vger.kernel.org, cheol.yong.kim@intel.com, hauke.mehrtens@intel.com, anders.roxell@linaro.org, vigneshr@ti.com, arnd@arndb.de, richard@nod.at, brendanhiggins@google.com, linux-mips@vger.kernel.org, robh+dt@kernel.org, miquel.raynal@bootlin.com, tglx@linutronix.de, masonccyang@mxic.com.tw, andriy.shevchenko@intel.com Subject: Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC Date: Thu, 30 Apr 2020 15:50:30 +0800 [thread overview] Message-ID: <1de9ba29-30f1-6829-27e0-6f141e9bb1e6@linux.intel.com> (raw) In-Reply-To: <20200429173107.5c6d2f55@collabora.com> 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 >
WARNING: multiple messages have this Message-ID (diff)
From: "Ramuthevar, Vadivel MuruganX" <vadivel.muruganx.ramuthevar@linux.intel.com> To: Boris Brezillon <boris.brezillon@collabora.com> Cc: cheol.yong.kim@intel.com, devicetree@vger.kernel.org, masonccyang@mxic.com.tw, anders.roxell@linaro.org, vigneshr@ti.com, arnd@arndb.de, hauke.mehrtens@intel.com, richard@nod.at, brendanhiggins@google.com, linux-kernel@vger.kernel.org, linux-mips@vger.kernel.org, robh+dt@kernel.org, linux-mtd@lists.infradead.org, miquel.raynal@bootlin.com, tglx@linutronix.de, qi-ming.wu@intel.com, andriy.shevchenko@intel.com Subject: Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC Date: Thu, 30 Apr 2020 15:50:30 +0800 [thread overview] Message-ID: <1de9ba29-30f1-6829-27e0-6f141e9bb1e6@linux.intel.com> (raw) In-Reply-To: <20200429173107.5c6d2f55@collabora.com> 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 > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2020-04-30 7:50 UTC|newest] Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-04-29 10:42 [PATCH v4 0/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC Ramuthevar,Vadivel MuruganX 2020-04-29 10:42 ` Ramuthevar, Vadivel MuruganX 2020-04-29 10:42 ` [PATCH v4 1/2] dt-bindings: mtd: Add YAML for Nand Flash Controller support Ramuthevar,Vadivel MuruganX 2020-04-29 10:42 ` Ramuthevar, Vadivel MuruganX 2020-04-29 15:34 ` Boris Brezillon 2020-04-29 15:34 ` Boris Brezillon 2020-04-30 1:07 ` Ramuthevar, Vadivel MuruganX 2020-04-30 1:07 ` Ramuthevar, Vadivel MuruganX 2020-04-29 10:42 ` [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC Ramuthevar,Vadivel MuruganX 2020-04-29 10:42 ` Ramuthevar, Vadivel MuruganX 2020-04-29 11:33 ` Boris Brezillon 2020-04-29 11:33 ` Boris Brezillon 2020-04-29 13:29 ` Ramuthevar, Vadivel MuruganX 2020-04-29 13:29 ` Ramuthevar, Vadivel MuruganX 2020-04-29 13:32 ` Boris Brezillon 2020-04-29 13:32 ` Boris Brezillon 2020-04-29 14:26 ` Ramuthevar, Vadivel MuruganX 2020-04-29 14:26 ` Ramuthevar, Vadivel MuruganX 2020-04-29 14:22 ` Boris Brezillon 2020-04-29 14:22 ` Boris Brezillon 2020-04-29 14:33 ` Ramuthevar, Vadivel MuruganX 2020-04-29 14:33 ` Ramuthevar, Vadivel MuruganX 2020-04-29 14:48 ` Boris Brezillon 2020-04-29 14:48 ` Boris Brezillon 2020-04-29 15:18 ` Ramuthevar, Vadivel MuruganX 2020-04-29 15:18 ` Ramuthevar, Vadivel MuruganX 2020-04-29 15:29 ` Ramuthevar, Vadivel MuruganX 2020-04-29 15:29 ` Ramuthevar, Vadivel MuruganX 2020-04-29 15:31 ` Boris Brezillon 2020-04-29 15:31 ` Boris Brezillon 2020-04-30 7:50 ` Ramuthevar, Vadivel MuruganX [this message] 2020-04-30 7:50 ` Ramuthevar, Vadivel MuruganX 2020-04-30 8:21 ` Boris Brezillon 2020-04-30 8:21 ` Boris Brezillon 2020-04-30 8:30 ` Ramuthevar, Vadivel MuruganX 2020-04-30 8:30 ` Ramuthevar, Vadivel MuruganX 2020-04-30 8:36 ` Boris Brezillon 2020-04-30 8:36 ` Boris Brezillon 2020-04-30 9:07 ` Ramuthevar, Vadivel MuruganX 2020-04-30 9:07 ` Ramuthevar, Vadivel MuruganX 2020-04-30 12:36 ` Boris Brezillon 2020-04-30 12:36 ` Boris Brezillon 2020-04-30 13:01 ` Boris Brezillon 2020-04-30 13:01 ` Boris Brezillon 2020-05-04 1:58 ` Ramuthevar, Vadivel MuruganX 2020-05-04 1:58 ` Ramuthevar, Vadivel MuruganX 2020-05-04 2:02 ` Ramuthevar, Vadivel MuruganX 2020-05-04 2:02 ` Ramuthevar, Vadivel MuruganX 2020-05-04 7:08 ` Boris Brezillon 2020-05-04 7:08 ` Boris Brezillon 2020-05-04 7:15 ` Ramuthevar, Vadivel MuruganX 2020-05-04 7:15 ` Ramuthevar, Vadivel MuruganX 2020-05-04 7:17 ` Boris Brezillon 2020-05-04 7:17 ` Boris Brezillon 2020-05-04 8:50 ` Ramuthevar, Vadivel MuruganX 2020-05-04 8:50 ` Ramuthevar, Vadivel MuruganX 2020-05-04 8:58 ` Boris Brezillon 2020-05-04 8:58 ` Boris Brezillon 2020-05-04 9:17 ` Ramuthevar, Vadivel MuruganX 2020-05-04 9:17 ` Ramuthevar, Vadivel MuruganX 2020-05-05 5:28 ` Ramuthevar, Vadivel MuruganX 2020-05-05 5:28 ` Ramuthevar, Vadivel MuruganX 2020-05-05 7:00 ` Boris Brezillon 2020-05-05 7:00 ` Boris Brezillon 2020-05-05 7:17 ` Ramuthevar, Vadivel MuruganX 2020-05-05 7:17 ` Ramuthevar, Vadivel MuruganX 2020-05-04 1:54 ` Ramuthevar, Vadivel MuruganX 2020-05-04 1:54 ` Ramuthevar, Vadivel MuruganX
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1de9ba29-30f1-6829-27e0-6f141e9bb1e6@linux.intel.com \ --to=vadivel.muruganx.ramuthevar@linux.intel.com \ --cc=anders.roxell@linaro.org \ --cc=andriy.shevchenko@intel.com \ --cc=arnd@arndb.de \ --cc=boris.brezillon@collabora.com \ --cc=brendanhiggins@google.com \ --cc=cheol.yong.kim@intel.com \ --cc=devicetree@vger.kernel.org \ --cc=hauke.mehrtens@intel.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mips@vger.kernel.org \ --cc=linux-mtd@lists.infradead.org \ --cc=masonccyang@mxic.com.tw \ --cc=miquel.raynal@bootlin.com \ --cc=qi-ming.wu@intel.com \ --cc=richard@nod.at \ --cc=robh+dt@kernel.org \ --cc=tglx@linutronix.de \ --cc=vigneshr@ti.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.