From: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> To: Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Cc: linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Josh Wu <rainyfeeling-1ViLX0X+lBJBDgjK7y7TUQ@public.gmane.org>, Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>, Yang Wenyou <Wenyou.Yang-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> Subject: Re: [PATCH v1 2/5] mtd: atmel_nand: Support variable RB_EDGE interrupts Date: Thu, 14 Jan 2016 11:41:05 +0100 [thread overview] Message-ID: <20160114114105.7f75ba1e@bbrezillon> (raw) In-Reply-To: <CAGkQfmMQTBVzOoE3BTyvg73NKjtGD_0veV_zb_jLutk-6bcqGw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> On Thu, 14 Jan 2016 11:16:19 +0100 Romain Izard <romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > Hi Boris, > > 2016-01-13 19:14 GMT+01:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>: > >> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c > >> index 9d71f9e6a8de..e5d7e7e63f49 100644 > >> --- a/drivers/mtd/nand/atmel_nand.c > >> +++ b/drivers/mtd/nand/atmel_nand.c > >> @@ -67,6 +67,10 @@ struct atmel_nand_caps { > >> bool pmecc_correct_erase_page; > >> }; > >> > >> +struct atmel_nand_nfc_priv { > > > > Can you rename this struct into atmel_nfc_caps to be consistant with the > > atmel_nand_caps? > > > >> + uint32_t rb_edge; > > > > Actually, AFAIU, it's not encoding the type of edges, but the available > > native R/B lines (by native I mean the R/B lines directly connected to > > the NFC IP). > > I suggest renaming this field avail_rb_lines, and making it a bitfield > > (one bit per possible R/B line). > > > > It's already a bitfield in the end: it's the mask for the interrupt status > register. It might have been better if it were called rb_edge_mask. Actually the naming chosen by atmel is a bit misleading, cause the R/B edge selection is done through the RB_RISE/FALL fields, and what they call RB_EDGEX is just the R/B line selection, hence the suggestion to name this field avail_rb_lines. > > >> +}; > >> + > >> /* oob layout for large page size > >> * bad block info is on bytes 0 and 1 > >> * the bytes have to be consecutives to avoid > >> @@ -111,6 +115,7 @@ struct atmel_nfc { > >> /* Point to the sram bank which include readed data via NFC */ > >> void *data_in_sram; > >> bool will_write_sram; > >> + uint32_t rb_edge; > > > > Replace this by > > const struct atmel_nfc_caps *caps; > > > > so that next time you have a per-SoC particularity, you can just add it > > to your struct atmel_nfc_caps without adding new fields to atmel_nfc. > > > > I'm reluctant about this, but I will do it. For me, we are exchanging > a single probe-time copy to indirections in each interrupt handler for some > unspecified future user. Let's hope the tradeoff is worth it. You raised a good point, maybe it's worth keeping this rb_edge field directly in the atmel_nfc struct. If you decide to do so, could you add a comment explaining why you're doing it? > > >> }; > >> static struct atmel_nfc nand_nfc; > >> > >> @@ -1675,9 +1680,9 @@ static irqreturn_t hsmc_interrupt(int irq, void *dev_id) > >> nfc_writel(host->nfc->hsmc_regs, IDR, NFC_SR_XFR_DONE); > >> ret = IRQ_HANDLED; > >> } > >> - if (pending & NFC_SR_RB_EDGE) { > >> + if (pending & host->nfc->rb_edge) { In the other hand, I see that we're already having a lot of indirections here, not sure adding one more will drastically impact the system. And if you really care about interrupt latencies in the system, you should probably register this handler as a threaded irq. > >> complete(&host->nfc->comp_ready); > >> - nfc_writel(host->nfc->hsmc_regs, IDR, NFC_SR_RB_EDGE); > >> + nfc_writel(host->nfc->hsmc_regs, IDR, host->nfc->rb_edge); > > > > How about creating a macro that would generate the appropriate bitmask > > for you? > > > > Something like > > > > #define NFC_SR_RB_EDGE_MASK(x) ((x) << 24) > > > > It's already so verbose, it will make everything longer. Once the member name > contains the 'mask' part, all the information will be there. Makes sense, especially if you keep the pre-calculated nfc->rb_edge_mask field. > > > >> diff --git a/drivers/mtd/nand/atmel_nand_nfc.h b/drivers/mtd/nand/atmel_nand_nfc.h > >> index 4d5d26221a7e..2cd9c57b1e53 100644 > >> --- a/drivers/mtd/nand/atmel_nand_nfc.h > >> +++ b/drivers/mtd/nand/atmel_nand_nfc.h > >> @@ -42,7 +42,10 @@ > >> #define NFC_SR_UNDEF (1 << 21) > >> #define NFC_SR_AWB (1 << 22) > >> #define NFC_SR_ASE (1 << 23) > >> -#define NFC_SR_RB_EDGE (1 << 24) > >> +#define NFC_SR_RB_EDGE0 (1 << 24) > >> +#define NFC_SR_RB_EDGE1 (1 << 25) > >> +#define NFC_SR_RB_EDGE2 (1 << 26) > >> +#define NFC_SR_RB_EDGE3 (1 << 27) > > > > Please replace those 4 definitions by: > > > > #define NFC_SR_RB_EDGE(x) BIT(x + 24) > > > > The whole file could be rewritten in this form, but I see this as a > zero-value proposition. I believe it is best to keep with local > consistency. In fact, I will rather remove RB_EDGE1 & RB_EDGE2, as > those are not mentioned in the datasheets. Hm, I keep thinking using the NFC_SR_RB_EDGE(x) is more future proof, but I'll let Atmel maintainers decide whether it's relevant or not to do so. -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html
WARNING: multiple messages have this Message-ID (diff)
From: Boris Brezillon <boris.brezillon@free-electrons.com> To: Romain Izard <romain.izard.pro@gmail.com> Cc: linux-mtd@lists.infradead.org, devicetree@vger.kernel.org, Josh Wu <rainyfeeling@outlook.com>, Nicolas Ferre <nicolas.ferre@atmel.com>, Yang Wenyou <Wenyou.Yang@atmel.com> Subject: Re: [PATCH v1 2/5] mtd: atmel_nand: Support variable RB_EDGE interrupts Date: Thu, 14 Jan 2016 11:41:05 +0100 [thread overview] Message-ID: <20160114114105.7f75ba1e@bbrezillon> (raw) In-Reply-To: <CAGkQfmMQTBVzOoE3BTyvg73NKjtGD_0veV_zb_jLutk-6bcqGw@mail.gmail.com> On Thu, 14 Jan 2016 11:16:19 +0100 Romain Izard <romain.izard.pro@gmail.com> wrote: > Hi Boris, > > 2016-01-13 19:14 GMT+01:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > >> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c > >> index 9d71f9e6a8de..e5d7e7e63f49 100644 > >> --- a/drivers/mtd/nand/atmel_nand.c > >> +++ b/drivers/mtd/nand/atmel_nand.c > >> @@ -67,6 +67,10 @@ struct atmel_nand_caps { > >> bool pmecc_correct_erase_page; > >> }; > >> > >> +struct atmel_nand_nfc_priv { > > > > Can you rename this struct into atmel_nfc_caps to be consistant with the > > atmel_nand_caps? > > > >> + uint32_t rb_edge; > > > > Actually, AFAIU, it's not encoding the type of edges, but the available > > native R/B lines (by native I mean the R/B lines directly connected to > > the NFC IP). > > I suggest renaming this field avail_rb_lines, and making it a bitfield > > (one bit per possible R/B line). > > > > It's already a bitfield in the end: it's the mask for the interrupt status > register. It might have been better if it were called rb_edge_mask. Actually the naming chosen by atmel is a bit misleading, cause the R/B edge selection is done through the RB_RISE/FALL fields, and what they call RB_EDGEX is just the R/B line selection, hence the suggestion to name this field avail_rb_lines. > > >> +}; > >> + > >> /* oob layout for large page size > >> * bad block info is on bytes 0 and 1 > >> * the bytes have to be consecutives to avoid > >> @@ -111,6 +115,7 @@ struct atmel_nfc { > >> /* Point to the sram bank which include readed data via NFC */ > >> void *data_in_sram; > >> bool will_write_sram; > >> + uint32_t rb_edge; > > > > Replace this by > > const struct atmel_nfc_caps *caps; > > > > so that next time you have a per-SoC particularity, you can just add it > > to your struct atmel_nfc_caps without adding new fields to atmel_nfc. > > > > I'm reluctant about this, but I will do it. For me, we are exchanging > a single probe-time copy to indirections in each interrupt handler for some > unspecified future user. Let's hope the tradeoff is worth it. You raised a good point, maybe it's worth keeping this rb_edge field directly in the atmel_nfc struct. If you decide to do so, could you add a comment explaining why you're doing it? > > >> }; > >> static struct atmel_nfc nand_nfc; > >> > >> @@ -1675,9 +1680,9 @@ static irqreturn_t hsmc_interrupt(int irq, void *dev_id) > >> nfc_writel(host->nfc->hsmc_regs, IDR, NFC_SR_XFR_DONE); > >> ret = IRQ_HANDLED; > >> } > >> - if (pending & NFC_SR_RB_EDGE) { > >> + if (pending & host->nfc->rb_edge) { In the other hand, I see that we're already having a lot of indirections here, not sure adding one more will drastically impact the system. And if you really care about interrupt latencies in the system, you should probably register this handler as a threaded irq. > >> complete(&host->nfc->comp_ready); > >> - nfc_writel(host->nfc->hsmc_regs, IDR, NFC_SR_RB_EDGE); > >> + nfc_writel(host->nfc->hsmc_regs, IDR, host->nfc->rb_edge); > > > > How about creating a macro that would generate the appropriate bitmask > > for you? > > > > Something like > > > > #define NFC_SR_RB_EDGE_MASK(x) ((x) << 24) > > > > It's already so verbose, it will make everything longer. Once the member name > contains the 'mask' part, all the information will be there. Makes sense, especially if you keep the pre-calculated nfc->rb_edge_mask field. > > > >> diff --git a/drivers/mtd/nand/atmel_nand_nfc.h b/drivers/mtd/nand/atmel_nand_nfc.h > >> index 4d5d26221a7e..2cd9c57b1e53 100644 > >> --- a/drivers/mtd/nand/atmel_nand_nfc.h > >> +++ b/drivers/mtd/nand/atmel_nand_nfc.h > >> @@ -42,7 +42,10 @@ > >> #define NFC_SR_UNDEF (1 << 21) > >> #define NFC_SR_AWB (1 << 22) > >> #define NFC_SR_ASE (1 << 23) > >> -#define NFC_SR_RB_EDGE (1 << 24) > >> +#define NFC_SR_RB_EDGE0 (1 << 24) > >> +#define NFC_SR_RB_EDGE1 (1 << 25) > >> +#define NFC_SR_RB_EDGE2 (1 << 26) > >> +#define NFC_SR_RB_EDGE3 (1 << 27) > > > > Please replace those 4 definitions by: > > > > #define NFC_SR_RB_EDGE(x) BIT(x + 24) > > > > The whole file could be rewritten in this form, but I see this as a > zero-value proposition. I believe it is best to keep with local > consistency. In fact, I will rather remove RB_EDGE1 & RB_EDGE2, as > those are not mentioned in the datasheets. Hm, I keep thinking using the NFC_SR_RB_EDGE(x) is more future proof, but I'll let Atmel maintainers decide whether it's relevant or not to do so. -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
next prev parent reply other threads:[~2016-01-14 10:41 UTC|newest] Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-01-13 16:34 [PATCH v1 0/5] mtd: atmel_nand: Add support for NAND Flash on SAMA5D2 Romain Izard 2016-01-13 16:34 ` Romain Izard [not found] ` <1452702857-2240-1-git-send-email-romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-01-13 16:34 ` [PATCH v1 1/5] mtd: atmel_nand: Do not warn on bitflips Romain Izard 2016-01-13 16:34 ` Romain Izard [not found] ` <1452702857-2240-2-git-send-email-romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-01-13 17:54 ` Boris Brezillon 2016-01-13 17:54 ` Boris Brezillon 2016-01-23 20:44 ` Brian Norris 2016-01-23 20:44 ` Brian Norris 2016-01-13 16:34 ` [PATCH v1 2/5] mtd: atmel_nand: Support variable RB_EDGE interrupts Romain Izard 2016-01-13 16:34 ` Romain Izard [not found] ` <1452702857-2240-3-git-send-email-romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-01-13 18:14 ` Boris Brezillon 2016-01-13 18:14 ` Boris Brezillon 2016-01-14 10:16 ` Romain Izard 2016-01-14 10:16 ` Romain Izard [not found] ` <CAGkQfmMQTBVzOoE3BTyvg73NKjtGD_0veV_zb_jLutk-6bcqGw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2016-01-14 10:41 ` Boris Brezillon [this message] 2016-01-14 10:41 ` Boris Brezillon 2016-01-14 1:19 ` Rob Herring 2016-01-14 1:19 ` Rob Herring 2016-01-13 16:34 ` [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2 Romain Izard 2016-01-13 16:34 ` Romain Izard [not found] ` <1452702857-2240-4-git-send-email-romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-01-14 1:12 ` Rob Herring 2016-01-14 1:12 ` Rob Herring 2016-01-14 1:17 ` Yang, Wenyou 2016-01-14 1:17 ` Yang, Wenyou 2016-01-14 13:14 ` Rob Herring 2016-01-14 13:14 ` Rob Herring [not found] ` <CAL_JsqL9kbNYrGYeqT7QCsbMneJbPgoXfaHhCvL8S=t97YaCbg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2016-01-15 1:17 ` Yang, Wenyou 2016-01-15 1:17 ` Yang, Wenyou 2016-01-15 8:54 ` Romain Izard 2016-01-15 8:54 ` Romain Izard [not found] ` <CAGkQfmO94gQ61prZvF52=B_UtOTkbYxd5nnTmDhJam4q-n3T7g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2016-01-17 4:05 ` Rob Herring 2016-01-17 4:05 ` Rob Herring 2016-01-14 9:19 ` Boris Brezillon 2016-01-14 9:19 ` Boris Brezillon 2016-01-15 10:06 ` romain izard 2016-01-15 10:06 ` romain izard 2016-01-13 16:34 ` [PATCH v1 4/5] mtd: atmel_nand: Support 32-bit ECC strength Romain Izard 2016-01-13 16:34 ` Romain Izard [not found] ` <1452702857-2240-5-git-send-email-romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-01-14 1:09 ` Rob Herring 2016-01-14 1:09 ` Rob Herring 2016-01-14 9:26 ` Boris Brezillon 2016-01-14 9:26 ` Boris Brezillon 2016-01-13 16:34 ` [PATCH v1 5/5] ARM: at91/dt: sama5d2: add nand0 and nfc0 nodes Romain Izard 2016-01-13 16:34 ` Romain Izard [not found] ` <1452702857-2240-6-git-send-email-romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-01-13 17:04 ` Nicolas Ferre 2016-01-13 17:04 ` Nicolas Ferre 2016-01-13 17:04 ` Nicolas Ferre
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=20160114114105.7f75ba1e@bbrezillon \ --to=boris.brezillon-wi1+55scjutkeb57/3fjtnbpr1lh4cv8@public.gmane.org \ --cc=Wenyou.Yang-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org \ --cc=devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \ --cc=nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org \ --cc=rainyfeeling-1ViLX0X+lBJBDgjK7y7TUQ@public.gmane.org \ --cc=romain.izard.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \ /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.