Message ID | 1351869956-2787-5-git-send-email-zonque@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Fri, Nov 02, 2012 at 20:55:56, Daniel Mack wrote: > This patch adds basic DT bindings for OMAP GPMC. > > The actual peripherals are instanciated from child nodes within the GPMC > node, and the only type of device that is currently supported is NAND. > > Code was added to parse the generic GPMC timing parameters and some > documentation with examples on how to use them. > > Successfully tested on an AM33xx board. > > Signed-off-by: Daniel Mack <zonque@gmail.com> [...] > + > + nand@0,0 { > + reg = <0 0 0>; /* CS0, offset 0 */ > + nand-bus-width = <16>; > + nand-ecc-mode = "none"; > + > + gpmc,sync-clk = <0>; > + gpmc,cs-on = <0>; > + gpmc,cs-rd-off = <36>; > + gpmc,cs-wr-off = <36>; > + gpmc,adv-on = <6>; > + gpmc,adv-rd-off = <24>; > + gpmc,adv-wr-off = <36>; > + gpmc,we-off = <30>; > + gpmc,oe-off = <48>; > + gpmc,access = <54>; > + gpmc,rd-cycle = <72>; > + gpmc,wr-cycle = <72>; > + gpmc,wr-access = <30>; > + gpmc,wr-data-mux-bus = <0>; > + > + #address-cells = <1>; > + #size-cells = <1>; > + Can you take the timings (for example) from arago tree. The timings is tested in am335x-evm So the timings can be directly used to populate GPMC timings. Timings can found at http://arago-project.org/git/projects/?p=linux-am33x.git;a=commitdiff; h=66bfbd2c5b35dc81edce0c24843c476161ab5978;hp=370630359cb8db711cf0941cd2a242e28ccfb61e [...] > +static int gpmc_probe_dt(struct platform_device *pdev) Can you take care of the following section mismatch. WARNING: vmlinux.o(.text+0x1e2d0): Section mismatch in reference from the function gpmc_probe_dt() to the function .init.text:gpmc_nand_init(). [...] > + > + val = of_get_nand_ecc_mode(child); > + if (val >= 0) > + gpmc_nand_data->ecc_opt = val; This will fail for BCH. Index of "soft_bch" is 5 & also don't have selection option between for BCH4 & BCH8 also. Can you use the of_property_read_u32 (as done early) to pass the ecc selection from dt file. This will help selection of BCH4 & BCH8 ecc options. Thanks Avinash
On 05.11.2012 12:03, Philip, Avinash wrote: > On Fri, Nov 02, 2012 at 20:55:56, Daniel Mack wrote: >> This patch adds basic DT bindings for OMAP GPMC. >> >> The actual peripherals are instanciated from child nodes within the GPMC >> node, and the only type of device that is currently supported is NAND. >> >> Code was added to parse the generic GPMC timing parameters and some >> documentation with examples on how to use them. >> >> Successfully tested on an AM33xx board. >> >> Signed-off-by: Daniel Mack <zonque@gmail.com> > [...] >> + >> + nand@0,0 { >> + reg = <0 0 0>; /* CS0, offset 0 */ >> + nand-bus-width = <16>; >> + nand-ecc-mode = "none"; >> + >> + gpmc,sync-clk = <0>; >> + gpmc,cs-on = <0>; >> + gpmc,cs-rd-off = <36>; >> + gpmc,cs-wr-off = <36>; >> + gpmc,adv-on = <6>; >> + gpmc,adv-rd-off = <24>; >> + gpmc,adv-wr-off = <36>; >> + gpmc,we-off = <30>; >> + gpmc,oe-off = <48>; >> + gpmc,access = <54>; >> + gpmc,rd-cycle = <72>; >> + gpmc,wr-cycle = <72>; >> + gpmc,wr-access = <30>; >> + gpmc,wr-data-mux-bus = <0>; >> + >> + #address-cells = <1>; >> + #size-cells = <1>; >> + > > Can you take the timings (for example) from arago tree. The timings is tested in am335x-evm > So the timings can be directly used to populate GPMC timings. Timings can found at > > http://arago-project.org/git/projects/?p=linux-am33x.git;a=commitdiff; > h=66bfbd2c5b35dc81edce0c24843c476161ab5978;hp=370630359cb8db711cf0941cd2a242e28ccfb61e > > [...] >> +static int gpmc_probe_dt(struct platform_device *pdev) > > Can you take care of the following section mismatch. > WARNING: vmlinux.o(.text+0x1e2d0): Section mismatch in reference > from the function gpmc_probe_dt() to the function .init.text:gpmc_nand_init(). Sore, both fixed for v4. > [...] >> + >> + val = of_get_nand_ecc_mode(child); >> + if (val >= 0) >> + gpmc_nand_data->ecc_opt = val; > > This will fail for BCH. Index of "soft_bch" is 5 & also don't have selection > option between for BCH4 & BCH8 also. > Can you use the of_property_read_u32 (as done early) to pass the ecc selection > from dt file. This will help selection of BCH4 & BCH8 ecc options. Hmm. Shouldn't we rather teach of_get_nand_ecc_mode() that two modes and bring the enum in sync? Daniel
On Mon, Nov 05, 2012 at 18:28:22, Daniel Mack wrote: > On 05.11.2012 12:03, Philip, Avinash wrote: > > On Fri, Nov 02, 2012 at 20:55:56, Daniel Mack wrote: > >> This patch adds basic DT bindings for OMAP GPMC. > >> > >> The actual peripherals are instanciated from child nodes within the GPMC > >> node, and the only type of device that is currently supported is NAND. > >> > >> Code was added to parse the generic GPMC timing parameters and some > >> documentation with examples on how to use them. > >> > >> Successfully tested on an AM33xx board. > >> > >> Signed-off-by: Daniel Mack <zonque@gmail.com> > > [...] > >> + > >> + nand@0,0 { > >> + reg = <0 0 0>; /* CS0, offset 0 */ > >> + nand-bus-width = <16>; > >> + nand-ecc-mode = "none"; > >> + > >> + gpmc,sync-clk = <0>; > >> + gpmc,cs-on = <0>; > >> + gpmc,cs-rd-off = <36>; > >> + gpmc,cs-wr-off = <36>; > >> + gpmc,adv-on = <6>; > >> + gpmc,adv-rd-off = <24>; > >> + gpmc,adv-wr-off = <36>; > >> + gpmc,we-off = <30>; > >> + gpmc,oe-off = <48>; > >> + gpmc,access = <54>; > >> + gpmc,rd-cycle = <72>; > >> + gpmc,wr-cycle = <72>; > >> + gpmc,wr-access = <30>; > >> + gpmc,wr-data-mux-bus = <0>; > >> + > >> + #address-cells = <1>; > >> + #size-cells = <1>; > >> + > > > > Can you take the timings (for example) from arago tree. The timings is tested in am335x-evm > > So the timings can be directly used to populate GPMC timings. Timings can found at > > > > http://arago-project.org/git/projects/?p=linux-am33x.git;a=commitdiff; > > h=66bfbd2c5b35dc81edce0c24843c476161ab5978;hp=370630359cb8db711cf0941cd2a242e28ccfb61e > > > > [...] > >> +static int gpmc_probe_dt(struct platform_device *pdev) > > > > Can you take care of the following section mismatch. > > WARNING: vmlinux.o(.text+0x1e2d0): Section mismatch in reference > > from the function gpmc_probe_dt() to the function .init.text:gpmc_nand_init(). > > Sore, both fixed for v4. > > > [...] > >> + > >> + val = of_get_nand_ecc_mode(child); > >> + if (val >= 0) > >> + gpmc_nand_data->ecc_opt = val; > > > > This will fail for BCH. Index of "soft_bch" is 5 & also don't have selection > > option between for BCH4 & BCH8 also. > > Can you use the of_property_read_u32 (as done early) to pass the ecc selection > > from dt file. This will help selection of BCH4 & BCH8 ecc options. > > Hmm. Shouldn't we rather teach of_get_nand_ecc_mode() that two modes and > bring the enum in sync? ecc_opt is for selecting different ecc layout and not for selecting ecc mode. In omap2 driver NAND_ECC_HW ecc mode supports 3 ecc layout OMAP_ECC_HAMMING_CODE_HW_ROMCODE OMAP_ECC_BCH4_CODE_HW OMAP_ECC_BCH8_CODE_HW So selection of ecc layout data should come from DT not ecc mode. Thanks Avinash > > > Daniel > > >
On 11/05/2012 08:29 AM, Philip, Avinash wrote: > On Mon, Nov 05, 2012 at 18:28:22, Daniel Mack wrote: >> On 05.11.2012 12:03, Philip, Avinash wrote: >>> On Fri, Nov 02, 2012 at 20:55:56, Daniel Mack wrote: >>>> This patch adds basic DT bindings for OMAP GPMC. >>>> >>>> The actual peripherals are instanciated from child nodes within the GPMC >>>> node, and the only type of device that is currently supported is NAND. >>>> >>>> Code was added to parse the generic GPMC timing parameters and some >>>> documentation with examples on how to use them. >>>> >>>> Successfully tested on an AM33xx board. >>>> >>>> Signed-off-by: Daniel Mack <zonque@gmail.com> >>> [...] >>>> + >>>> + nand@0,0 { >>>> + reg = <0 0 0>; /* CS0, offset 0 */ >>>> + nand-bus-width = <16>; >>>> + nand-ecc-mode = "none"; >>>> + >>>> + gpmc,sync-clk = <0>; >>>> + gpmc,cs-on = <0>; >>>> + gpmc,cs-rd-off = <36>; >>>> + gpmc,cs-wr-off = <36>; >>>> + gpmc,adv-on = <6>; >>>> + gpmc,adv-rd-off = <24>; >>>> + gpmc,adv-wr-off = <36>; >>>> + gpmc,we-off = <30>; >>>> + gpmc,oe-off = <48>; >>>> + gpmc,access = <54>; >>>> + gpmc,rd-cycle = <72>; >>>> + gpmc,wr-cycle = <72>; >>>> + gpmc,wr-access = <30>; >>>> + gpmc,wr-data-mux-bus = <0>; >>>> + >>>> + #address-cells = <1>; >>>> + #size-cells = <1>; >>>> + >>> Can you take the timings (for example) from arago tree. The timings is tested in am335x-evm >>> So the timings can be directly used to populate GPMC timings. Timings can found at >>> >>> http://arago-project.org/git/projects/?p=linux-am33x.git;a=commitdiff; >>> h=66bfbd2c5b35dc81edce0c24843c476161ab5978;hp=370630359cb8db711cf0941cd2a242e28ccfb61e >>> >>> [...] >>>> +static int gpmc_probe_dt(struct platform_device *pdev) >>> Can you take care of the following section mismatch. >>> WARNING: vmlinux.o(.text+0x1e2d0): Section mismatch in reference >>> from the function gpmc_probe_dt() to the function .init.text:gpmc_nand_init(). >> Sore, both fixed for v4. >> >>> [...] >>>> + >>>> + val = of_get_nand_ecc_mode(child); >>>> + if (val >= 0) >>>> + gpmc_nand_data->ecc_opt = val; >>> This will fail for BCH. Index of "soft_bch" is 5 & also don't have selection >>> option between for BCH4 & BCH8 also. >>> Can you use the of_property_read_u32 (as done early) to pass the ecc selection >>> from dt file. This will help selection of BCH4 & BCH8 ecc options. >> Hmm. Shouldn't we rather teach of_get_nand_ecc_mode() that two modes and >> bring the enum in sync? > ecc_opt is for selecting different ecc layout and not for selecting ecc mode. > > In omap2 driver NAND_ECC_HW ecc mode supports 3 ecc layout > OMAP_ECC_HAMMING_CODE_HW_ROMCODE > OMAP_ECC_BCH4_CODE_HW > OMAP_ECC_BCH8_CODE_HW > > So selection of ecc layout data should come from DT not ecc mode. > > Thanks > Avinash > >> >> Daniel >> >> >> > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > Hi, I am working on an AEMIF driver for DaVinci that is similar to this. Is this a memory or mfd device? Murali
On 05.11.2012 14:29, Philip, Avinash wrote: > On Mon, Nov 05, 2012 at 18:28:22, Daniel Mack wrote: >> On 05.11.2012 12:03, Philip, Avinash wrote: >>> On Fri, Nov 02, 2012 at 20:55:56, Daniel Mack wrote: >>>> This patch adds basic DT bindings for OMAP GPMC. >>>> >>>> The actual peripherals are instanciated from child nodes within the GPMC >>>> node, and the only type of device that is currently supported is NAND. >>>> >>>> Code was added to parse the generic GPMC timing parameters and some >>>> documentation with examples on how to use them. >>>> >>>> Successfully tested on an AM33xx board. >>>> >>>> Signed-off-by: Daniel Mack <zonque@gmail.com> >>> [...] >>>> + >>>> + nand@0,0 { >>>> + reg = <0 0 0>; /* CS0, offset 0 */ >>>> + nand-bus-width = <16>; >>>> + nand-ecc-mode = "none"; >>>> + >>>> + gpmc,sync-clk = <0>; >>>> + gpmc,cs-on = <0>; >>>> + gpmc,cs-rd-off = <36>; >>>> + gpmc,cs-wr-off = <36>; >>>> + gpmc,adv-on = <6>; >>>> + gpmc,adv-rd-off = <24>; >>>> + gpmc,adv-wr-off = <36>; >>>> + gpmc,we-off = <30>; >>>> + gpmc,oe-off = <48>; >>>> + gpmc,access = <54>; >>>> + gpmc,rd-cycle = <72>; >>>> + gpmc,wr-cycle = <72>; >>>> + gpmc,wr-access = <30>; >>>> + gpmc,wr-data-mux-bus = <0>; >>>> + >>>> + #address-cells = <1>; >>>> + #size-cells = <1>; >>>> + >>> >>> Can you take the timings (for example) from arago tree. The timings is tested in am335x-evm >>> So the timings can be directly used to populate GPMC timings. Timings can found at >>> >>> http://arago-project.org/git/projects/?p=linux-am33x.git;a=commitdiff; >>> h=66bfbd2c5b35dc81edce0c24843c476161ab5978;hp=370630359cb8db711cf0941cd2a242e28ccfb61e >>> >>> [...] >>>> +static int gpmc_probe_dt(struct platform_device *pdev) >>> >>> Can you take care of the following section mismatch. >>> WARNING: vmlinux.o(.text+0x1e2d0): Section mismatch in reference >>> from the function gpmc_probe_dt() to the function .init.text:gpmc_nand_init(). >> >> Sore, both fixed for v4. >> >>> [...] >>>> + >>>> + val = of_get_nand_ecc_mode(child); >>>> + if (val >= 0) >>>> + gpmc_nand_data->ecc_opt = val; >>> >>> This will fail for BCH. Index of "soft_bch" is 5 & also don't have selection >>> option between for BCH4 & BCH8 also. >>> Can you use the of_property_read_u32 (as done early) to pass the ecc selection >>> from dt file. This will help selection of BCH4 & BCH8 ecc options. >> >> Hmm. Shouldn't we rather teach of_get_nand_ecc_mode() that two modes and >> bring the enum in sync? > > ecc_opt is for selecting different ecc layout and not for selecting ecc mode. > > In omap2 driver NAND_ECC_HW ecc mode supports 3 ecc layout > OMAP_ECC_HAMMING_CODE_HW_ROMCODE > OMAP_ECC_BCH4_CODE_HW > OMAP_ECC_BCH8_CODE_HW > > So selection of ecc layout data should come from DT not ecc mode. Ok, I see. I would still like to set them by string rather than magic numbers that map to enum entries. Valid values would be "none", "hw", "hw-romcode", "bch4" and "bch8". Are you ok with that? Thanks, Daniel
On Wed, Nov 07, 2012 at 15:18:37, Daniel Mack wrote: > On 05.11.2012 14:29, Philip, Avinash wrote: > > On Mon, Nov 05, 2012 at 18:28:22, Daniel Mack wrote: > >> On 05.11.2012 12:03, Philip, Avinash wrote: > >>> On Fri, Nov 02, 2012 at 20:55:56, Daniel Mack wrote: > >>>> This patch adds basic DT bindings for OMAP GPMC. > >>>> > >>>> The actual peripherals are instanciated from child nodes within the GPMC > >>>> node, and the only type of device that is currently supported is NAND. > >>>> > >>>> Code was added to parse the generic GPMC timing parameters and some > >>>> documentation with examples on how to use them. > >>>> > >>>> Successfully tested on an AM33xx board. > >>>> > >>>> Signed-off-by: Daniel Mack <zonque@gmail.com> > >>> [...] > >>>> + > >>>> + nand@0,0 { > >>>> + reg = <0 0 0>; /* CS0, offset 0 */ > >>>> + nand-bus-width = <16>; > >>>> + nand-ecc-mode = "none"; > >>>> + > >>>> + gpmc,sync-clk = <0>; > >>>> + gpmc,cs-on = <0>; > >>>> + gpmc,cs-rd-off = <36>; > >>>> + gpmc,cs-wr-off = <36>; > >>>> + gpmc,adv-on = <6>; > >>>> + gpmc,adv-rd-off = <24>; > >>>> + gpmc,adv-wr-off = <36>; > >>>> + gpmc,we-off = <30>; > >>>> + gpmc,oe-off = <48>; > >>>> + gpmc,access = <54>; > >>>> + gpmc,rd-cycle = <72>; > >>>> + gpmc,wr-cycle = <72>; > >>>> + gpmc,wr-access = <30>; > >>>> + gpmc,wr-data-mux-bus = <0>; > >>>> + > >>>> + #address-cells = <1>; > >>>> + #size-cells = <1>; > >>>> + > >>> > >>> Can you take the timings (for example) from arago tree. The timings is tested in am335x-evm > >>> So the timings can be directly used to populate GPMC timings. Timings can found at > >>> > >>> http://arago-project.org/git/projects/?p=linux-am33x.git;a=commitdiff; > >>> h=66bfbd2c5b35dc81edce0c24843c476161ab5978;hp=370630359cb8db711cf0941cd2a242e28ccfb61e > >>> > >>> [...] > >>>> +static int gpmc_probe_dt(struct platform_device *pdev) > >>> > >>> Can you take care of the following section mismatch. > >>> WARNING: vmlinux.o(.text+0x1e2d0): Section mismatch in reference > >>> from the function gpmc_probe_dt() to the function .init.text:gpmc_nand_init(). > >> > >> Sore, both fixed for v4. > >> > >>> [...] > >>>> + > >>>> + val = of_get_nand_ecc_mode(child); > >>>> + if (val >= 0) > >>>> + gpmc_nand_data->ecc_opt = val; > >>> > >>> This will fail for BCH. Index of "soft_bch" is 5 & also don't have selection > >>> option between for BCH4 & BCH8 also. > >>> Can you use the of_property_read_u32 (as done early) to pass the ecc selection > >>> from dt file. This will help selection of BCH4 & BCH8 ecc options. > >> > >> Hmm. Shouldn't we rather teach of_get_nand_ecc_mode() that two modes and > >> bring the enum in sync? > > > > ecc_opt is for selecting different ecc layout and not for selecting ecc mode. > > > > In omap2 driver NAND_ECC_HW ecc mode supports 3 ecc layout > > OMAP_ECC_HAMMING_CODE_HW_ROMCODE > > OMAP_ECC_BCH4_CODE_HW > > OMAP_ECC_BCH8_CODE_HW > > > > So selection of ecc layout data should come from DT not ecc mode. > > Ok, I see. I would still like to set them by string rather than magic > numbers that map to enum entries. Valid values would be "none", "hw", > "hw-romcode", "bch4" and "bch8". Are you ok with that? Ok, that's nice. Better use ecc_opt instead of ecc_mode. Thanks Avinash > > > Thanks, > Daniel > >
On 07.11.2012 16:37, Philip, Avinash wrote: > On Wed, Nov 07, 2012 at 15:18:37, Daniel Mack wrote: >> On 05.11.2012 14:29, Philip, Avinash wrote: >>> On Mon, Nov 05, 2012 at 18:28:22, Daniel Mack wrote: >>>> On 05.11.2012 12:03, Philip, Avinash wrote: >>>>> On Fri, Nov 02, 2012 at 20:55:56, Daniel Mack wrote: >>>>>> This patch adds basic DT bindings for OMAP GPMC. >>>>>> >>>>>> The actual peripherals are instanciated from child nodes within the GPMC >>>>>> node, and the only type of device that is currently supported is NAND. >>>>>> >>>>>> Code was added to parse the generic GPMC timing parameters and some >>>>>> documentation with examples on how to use them. >>>>>> >>>>>> Successfully tested on an AM33xx board. >>>>>> >>>>>> Signed-off-by: Daniel Mack <zonque@gmail.com> >>>>> [...] >>>>>> + >>>>>> + nand@0,0 { >>>>>> + reg = <0 0 0>; /* CS0, offset 0 */ >>>>>> + nand-bus-width = <16>; >>>>>> + nand-ecc-mode = "none"; >>>>>> + >>>>>> + gpmc,sync-clk = <0>; >>>>>> + gpmc,cs-on = <0>; >>>>>> + gpmc,cs-rd-off = <36>; >>>>>> + gpmc,cs-wr-off = <36>; >>>>>> + gpmc,adv-on = <6>; >>>>>> + gpmc,adv-rd-off = <24>; >>>>>> + gpmc,adv-wr-off = <36>; >>>>>> + gpmc,we-off = <30>; >>>>>> + gpmc,oe-off = <48>; >>>>>> + gpmc,access = <54>; >>>>>> + gpmc,rd-cycle = <72>; >>>>>> + gpmc,wr-cycle = <72>; >>>>>> + gpmc,wr-access = <30>; >>>>>> + gpmc,wr-data-mux-bus = <0>; >>>>>> + >>>>>> + #address-cells = <1>; >>>>>> + #size-cells = <1>; >>>>>> + >>>>> >>>>> Can you take the timings (for example) from arago tree. The timings is tested in am335x-evm >>>>> So the timings can be directly used to populate GPMC timings. Timings can found at >>>>> >>>>> http://arago-project.org/git/projects/?p=linux-am33x.git;a=commitdiff; >>>>> h=66bfbd2c5b35dc81edce0c24843c476161ab5978;hp=370630359cb8db711cf0941cd2a242e28ccfb61e >>>>> >>>>> [...] >>>>>> +static int gpmc_probe_dt(struct platform_device *pdev) >>>>> >>>>> Can you take care of the following section mismatch. >>>>> WARNING: vmlinux.o(.text+0x1e2d0): Section mismatch in reference >>>>> from the function gpmc_probe_dt() to the function .init.text:gpmc_nand_init(). >>>> >>>> Sore, both fixed for v4. >>>> >>>>> [...] >>>>>> + >>>>>> + val = of_get_nand_ecc_mode(child); >>>>>> + if (val >= 0) >>>>>> + gpmc_nand_data->ecc_opt = val; >>>>> >>>>> This will fail for BCH. Index of "soft_bch" is 5 & also don't have selection >>>>> option between for BCH4 & BCH8 also. >>>>> Can you use the of_property_read_u32 (as done early) to pass the ecc selection >>>>> from dt file. This will help selection of BCH4 & BCH8 ecc options. >>>> >>>> Hmm. Shouldn't we rather teach of_get_nand_ecc_mode() that two modes and >>>> bring the enum in sync? >>> >>> ecc_opt is for selecting different ecc layout and not for selecting ecc mode. >>> >>> In omap2 driver NAND_ECC_HW ecc mode supports 3 ecc layout >>> OMAP_ECC_HAMMING_CODE_HW_ROMCODE >>> OMAP_ECC_BCH4_CODE_HW >>> OMAP_ECC_BCH8_CODE_HW >>> >>> So selection of ecc layout data should come from DT not ecc mode. >> >> Ok, I see. I would still like to set them by string rather than magic >> numbers that map to enum entries. Valid values would be "none", "hw", >> "hw-romcode", "bch4" and "bch8". Are you ok with that? > > Ok, that's nice. Better use ecc_opt instead of ecc_mode. I did some more extensive tests that include reading the same nand pages from both U-Boot and the kernel with BCH8 ECC, and it turns out that ->is_elm_used needs to be set in the pdata in order to make this work. So the question is whether we actually want to have a DT property for that or just always enable that bit in case a hardware supported ecc mode is selected. Any opinion on this? That's the last topic before I'm clear to send off v4. Thanks, Daniel
On 11.11.2012 02:56, Daniel Mack wrote: > On 07.11.2012 16:37, Philip, Avinash wrote: >> On Wed, Nov 07, 2012 at 15:18:37, Daniel Mack wrote: >>> On 05.11.2012 14:29, Philip, Avinash wrote: >>>> On Mon, Nov 05, 2012 at 18:28:22, Daniel Mack wrote: >>>>> On 05.11.2012 12:03, Philip, Avinash wrote: >>>>>> On Fri, Nov 02, 2012 at 20:55:56, Daniel Mack wrote: >>>>>>> This patch adds basic DT bindings for OMAP GPMC. >>>>>>> >>>>>>> The actual peripherals are instanciated from child nodes within the GPMC >>>>>>> node, and the only type of device that is currently supported is NAND. >>>>>>> >>>>>>> Code was added to parse the generic GPMC timing parameters and some >>>>>>> documentation with examples on how to use them. >>>>>>> >>>>>>> Successfully tested on an AM33xx board. >>>>>>> >>>>>>> Signed-off-by: Daniel Mack <zonque@gmail.com> >>>>>> [...] >>>>>>> + >>>>>>> + nand@0,0 { >>>>>>> + reg = <0 0 0>; /* CS0, offset 0 */ >>>>>>> + nand-bus-width = <16>; >>>>>>> + nand-ecc-mode = "none"; >>>>>>> + >>>>>>> + gpmc,sync-clk = <0>; >>>>>>> + gpmc,cs-on = <0>; >>>>>>> + gpmc,cs-rd-off = <36>; >>>>>>> + gpmc,cs-wr-off = <36>; >>>>>>> + gpmc,adv-on = <6>; >>>>>>> + gpmc,adv-rd-off = <24>; >>>>>>> + gpmc,adv-wr-off = <36>; >>>>>>> + gpmc,we-off = <30>; >>>>>>> + gpmc,oe-off = <48>; >>>>>>> + gpmc,access = <54>; >>>>>>> + gpmc,rd-cycle = <72>; >>>>>>> + gpmc,wr-cycle = <72>; >>>>>>> + gpmc,wr-access = <30>; >>>>>>> + gpmc,wr-data-mux-bus = <0>; >>>>>>> + >>>>>>> + #address-cells = <1>; >>>>>>> + #size-cells = <1>; >>>>>>> + >>>>>> >>>>>> Can you take the timings (for example) from arago tree. The timings is tested in am335x-evm >>>>>> So the timings can be directly used to populate GPMC timings. Timings can found at >>>>>> >>>>>> http://arago-project.org/git/projects/?p=linux-am33x.git;a=commitdiff; >>>>>> h=66bfbd2c5b35dc81edce0c24843c476161ab5978;hp=370630359cb8db711cf0941cd2a242e28ccfb61e >>>>>> >>>>>> [...] >>>>>>> +static int gpmc_probe_dt(struct platform_device *pdev) >>>>>> >>>>>> Can you take care of the following section mismatch. >>>>>> WARNING: vmlinux.o(.text+0x1e2d0): Section mismatch in reference >>>>>> from the function gpmc_probe_dt() to the function .init.text:gpmc_nand_init(). >>>>> >>>>> Sore, both fixed for v4. >>>>> >>>>>> [...] >>>>>>> + >>>>>>> + val = of_get_nand_ecc_mode(child); >>>>>>> + if (val >= 0) >>>>>>> + gpmc_nand_data->ecc_opt = val; >>>>>> >>>>>> This will fail for BCH. Index of "soft_bch" is 5 & also don't have selection >>>>>> option between for BCH4 & BCH8 also. >>>>>> Can you use the of_property_read_u32 (as done early) to pass the ecc selection >>>>>> from dt file. This will help selection of BCH4 & BCH8 ecc options. >>>>> >>>>> Hmm. Shouldn't we rather teach of_get_nand_ecc_mode() that two modes and >>>>> bring the enum in sync? >>>> >>>> ecc_opt is for selecting different ecc layout and not for selecting ecc mode. >>>> >>>> In omap2 driver NAND_ECC_HW ecc mode supports 3 ecc layout >>>> OMAP_ECC_HAMMING_CODE_HW_ROMCODE >>>> OMAP_ECC_BCH4_CODE_HW >>>> OMAP_ECC_BCH8_CODE_HW >>>> >>>> So selection of ecc layout data should come from DT not ecc mode. >>> >>> Ok, I see. I would still like to set them by string rather than magic >>> numbers that map to enum entries. Valid values would be "none", "hw", >>> "hw-romcode", "bch4" and "bch8". Are you ok with that? >> >> Ok, that's nice. Better use ecc_opt instead of ecc_mode. > > I did some more extensive tests that include reading the same nand pages > from both U-Boot and the kernel with BCH8 ECC, and it turns out that > ->is_elm_used needs to be set in the pdata in order to make this work. > > So the question is whether we actually want to have a DT property for > that or just always enable that bit in case a hardware supported ecc > mode is selected. Any opinion on this? > > That's the last topic before I'm clear to send off v4. Any opionion on this, anyone? Thanks, Daniel
On Sun, Nov 11, 2012 at 00:26:32, Daniel Mack wrote: > On 07.11.2012 16:37, Philip, Avinash wrote: > > On Wed, Nov 07, 2012 at 15:18:37, Daniel Mack wrote: > >> On 05.11.2012 14:29, Philip, Avinash wrote: > >>> On Mon, Nov 05, 2012 at 18:28:22, Daniel Mack wrote: > >>>> On 05.11.2012 12:03, Philip, Avinash wrote: > >>>>> On Fri, Nov 02, 2012 at 20:55:56, Daniel Mack wrote: > >>>>>> This patch adds basic DT bindings for OMAP GPMC. > >>>>>> > >>>>>> The actual peripherals are instanciated from child nodes within the GPMC > >>>>>> node, and the only type of device that is currently supported is NAND. > >>>>>> > >>>>>> Code was added to parse the generic GPMC timing parameters and some > >>>>>> documentation with examples on how to use them. > >>>>>> > >>>>>> Successfully tested on an AM33xx board. > >>>>>> > >>>>>> Signed-off-by: Daniel Mack <zonque@gmail.com> > >>>>> [...] > >>>>>> + > >>>>>> + nand@0,0 { > >>>>>> + reg = <0 0 0>; /* CS0, offset 0 */ > >>>>>> + nand-bus-width = <16>; > >>>>>> + nand-ecc-mode = "none"; > >>>>>> + > >>>>>> + gpmc,sync-clk = <0>; > >>>>>> + gpmc,cs-on = <0>; > >>>>>> + gpmc,cs-rd-off = <36>; > >>>>>> + gpmc,cs-wr-off = <36>; > >>>>>> + gpmc,adv-on = <6>; > >>>>>> + gpmc,adv-rd-off = <24>; > >>>>>> + gpmc,adv-wr-off = <36>; > >>>>>> + gpmc,we-off = <30>; > >>>>>> + gpmc,oe-off = <48>; > >>>>>> + gpmc,access = <54>; > >>>>>> + gpmc,rd-cycle = <72>; > >>>>>> + gpmc,wr-cycle = <72>; > >>>>>> + gpmc,wr-access = <30>; > >>>>>> + gpmc,wr-data-mux-bus = <0>; > >>>>>> + > >>>>>> + #address-cells = <1>; > >>>>>> + #size-cells = <1>; > >>>>>> + > >>>>> > >>>>> Can you take the timings (for example) from arago tree. The timings is tested in am335x-evm > >>>>> So the timings can be directly used to populate GPMC timings. Timings can found at > >>>>> > >>>>> http://arago-project.org/git/projects/?p=linux-am33x.git;a=commitdiff; > >>>>> h=66bfbd2c5b35dc81edce0c24843c476161ab5978;hp=370630359cb8db711cf0941cd2a242e28ccfb61e > >>>>> > >>>>> [...] > >>>>>> +static int gpmc_probe_dt(struct platform_device *pdev) > >>>>> > >>>>> Can you take care of the following section mismatch. > >>>>> WARNING: vmlinux.o(.text+0x1e2d0): Section mismatch in reference > >>>>> from the function gpmc_probe_dt() to the function .init.text:gpmc_nand_init(). > >>>> > >>>> Sore, both fixed for v4. > >>>> > >>>>> [...] > >>>>>> + > >>>>>> + val = of_get_nand_ecc_mode(child); > >>>>>> + if (val >= 0) > >>>>>> + gpmc_nand_data->ecc_opt = val; > >>>>> > >>>>> This will fail for BCH. Index of "soft_bch" is 5 & also don't have selection > >>>>> option between for BCH4 & BCH8 also. > >>>>> Can you use the of_property_read_u32 (as done early) to pass the ecc selection > >>>>> from dt file. This will help selection of BCH4 & BCH8 ecc options. > >>>> > >>>> Hmm. Shouldn't we rather teach of_get_nand_ecc_mode() that two modes and > >>>> bring the enum in sync? > >>> > >>> ecc_opt is for selecting different ecc layout and not for selecting ecc mode. > >>> > >>> In omap2 driver NAND_ECC_HW ecc mode supports 3 ecc layout > >>> OMAP_ECC_HAMMING_CODE_HW_ROMCODE > >>> OMAP_ECC_BCH4_CODE_HW > >>> OMAP_ECC_BCH8_CODE_HW > >>> > >>> So selection of ecc layout data should come from DT not ecc mode. > >> > >> Ok, I see. I would still like to set them by string rather than magic > >> numbers that map to enum entries. Valid values would be "none", "hw", > >> "hw-romcode", "bch4" and "bch8". Are you ok with that? > > > > Ok, that's nice. Better use ecc_opt instead of ecc_mode. > > I did some more extensive tests that include reading the same nand pages > from both U-Boot and the kernel with BCH8 ECC, and it turns out that > ->is_elm_used needs to be set in the pdata in order to make this work. > > So the question is whether we actually want to have a DT property for > that or just always enable that bit in case a hardware supported ecc > mode is selected. Any opinion on this? Yes, is_elm_used data should come from DT. There may be hardware which uses BCH8 ecc scheme even without ELM hardware support with software error correction support. So is_elm_used data should come from DT property. Thanks Avinash > > That's the last topic before I'm clear to send off v4. > > > Thanks, > Daniel > >
>>>>> "Daniel" == Daniel Mack <zonque@gmail.com> writes: Hi, >>>> In omap2 driver NAND_ECC_HW ecc mode supports 3 ecc layout >>>> OMAP_ECC_HAMMING_CODE_HW_ROMCODE >>>> OMAP_ECC_BCH4_CODE_HW >>>> OMAP_ECC_BCH8_CODE_HW >>>> >>>> So selection of ecc layout data should come from DT not ecc mode. >>> >>> Ok, I see. I would still like to set them by string rather than magic >>> numbers that map to enum entries. Valid values would be "none", "hw", >>> "hw-romcode", "bch4" and "bch8". Are you ok with that? >> >> Ok, that's nice. Better use ecc_opt instead of ecc_mode. Daniel> I did some more extensive tests that include reading the same Daniel> nand pages from both U-Boot and the kernel with BCH8 ECC, and Daniel> it turns out that -> is_elm_used needs to be set in the pdata Daniel> in order to make this work. So what you're saying is that the choice of ELM or not is not just an optimization, it really changes the ECC layout? Perhaps it should be treated as a seperate layout (E.G. bch8-elm) then?
On 20.11.2012 16:59, Peter Korsgaard wrote: >>>>>> "Daniel" == Daniel Mack <zonque@gmail.com> writes: > > Hi, > > >>>> In omap2 driver NAND_ECC_HW ecc mode supports 3 ecc layout > >>>> OMAP_ECC_HAMMING_CODE_HW_ROMCODE > >>>> OMAP_ECC_BCH4_CODE_HW > >>>> OMAP_ECC_BCH8_CODE_HW > >>>> > >>>> So selection of ecc layout data should come from DT not ecc mode. > >>> > >>> Ok, I see. I would still like to set them by string rather than magic > >>> numbers that map to enum entries. Valid values would be "none", "hw", > >>> "hw-romcode", "bch4" and "bch8". Are you ok with that? > >> > >> Ok, that's nice. Better use ecc_opt instead of ecc_mode. > > Daniel> I did some more extensive tests that include reading the same > Daniel> nand pages from both U-Boot and the kernel with BCH8 ECC, and > Daniel> it turns out that -> is_elm_used needs to be set in the pdata > Daniel> in order to make this work. > > So what you're saying is that the choice of ELM or not is not just an > optimization, it really changes the ECC layout? Perhaps it should be > treated as a seperate layout (E.G. bch8-elm) then? That is what I experienced, yes. The kernel was unable to parse NAND pages that were written from U-Boot with bch8 hardware mode when the elm module was not active. Maybe someone from TI can explain that? Giving it a dedicated name would also solve the problem with the extra DT property. I'll wait until this is decided before I resend a new set that also fixes the issue with the errornousely forgotten Documentation file. Thanks, Daniel
On Wed, Nov 21, 2012 at 15:45:23, Daniel Mack wrote: > On 20.11.2012 16:59, Peter Korsgaard wrote: > >>>>>> "Daniel" == Daniel Mack <zonque@gmail.com> writes: > > > > Hi, > > > > >>>> In omap2 driver NAND_ECC_HW ecc mode supports 3 ecc layout > > >>>> OMAP_ECC_HAMMING_CODE_HW_ROMCODE > > >>>> OMAP_ECC_BCH4_CODE_HW > > >>>> OMAP_ECC_BCH8_CODE_HW > > >>>> > > >>>> So selection of ecc layout data should come from DT not ecc mode. > > >>> > > >>> Ok, I see. I would still like to set them by string rather than magic > > >>> numbers that map to enum entries. Valid values would be "none", "hw", > > >>> "hw-romcode", "bch4" and "bch8". Are you ok with that? > > >> > > >> Ok, that's nice. Better use ecc_opt instead of ecc_mode. > > > > Daniel> I did some more extensive tests that include reading the same > > Daniel> nand pages from both U-Boot and the kernel with BCH8 ECC, and > > Daniel> it turns out that -> is_elm_used needs to be set in the pdata > > Daniel> in order to make this work. > > > > So what you're saying is that the choice of ELM or not is not just an > > optimization, it really changes the ECC layout? Perhaps it should be > > treated as a seperate layout (E.G. bch8-elm) then? Peter, In patch [1], mtd: nand: omap2: Support for hardware BCH ecc_layout made compatible with Rom Boot Loader ECC layout for am335x. This action is based on is_elm_used flag. Requirement of this flag is to identify the whether the platform ELM module & based on this configure ELM module if present. But ideally BCH8 ecc lay out didn't have a dependency on ELM, it can work with software BCH ecc. RBL compatibility is missing in software BCH because of addition of constant polynomial to ecc vector. If we remove the dependency on erased page handling by ecc vector with constant polynomial, software BCH can do the job of RBL compatibility. Ivan, Do you have any suggestions? Discussion for RBL compatibility found at [2]. It is good that software BCH also support RBL compatibility by suppressing constant polynomial modification. Then ecc layout can be selected from DT entry and error correction can be chosen between software/hardware depending on the availability of ELM hardware. Currently RBL BCH8 compatibility depends on the availability of ELM hardware. Later once software BCH start supporting RBL compatibility, we can remove the check. > > That is what I experienced, yes. The kernel was unable to parse NAND > pages that were written from U-Boot with bch8 hardware mode when the elm > module was not active. Maybe someone from TI can explain that? Giving it > a dedicated name would also solve the problem with the extra DT property. Daniel, Currently BCH8 is supported with software ecc error correction in mainline. The layout for BCH8 ECC layout is 0-1 -> BAD block markers 2-11-> oob free area 12-63-> BCH8 ECC. RBL ECC layout is 0-1 -> BAD block markers 2-57-> BCH8 ecc layout 58-63-> OOB free area As u-boot also maintain RBL ecc layout, write from U-boot and read from Linux requires compatibility with RBL ecc layout. The same is achieved in the patch [1], with by setting is_elm_used to true. 1. https://lkml.org/lkml/2012/10/31/87 2. https://lkml.org/lkml/2012/10/11/20 Thanks Avinash > > I'll wait until this is decided before I resend a new set that also > fixes the issue with the errornousely forgotten Documentation file. > > > Thanks, > Daniel > >
Hi Avinash, Hi Peter, On 23.11.2012 11:43, Philip, Avinash wrote: > On Wed, Nov 21, 2012 at 15:45:23, Daniel Mack wrote: >> On 20.11.2012 16:59, Peter Korsgaard wrote: >>>>>>>> "Daniel" == Daniel Mack <zonque@gmail.com> writes: > Peter, > > In patch [1], mtd: nand: omap2: Support for hardware BCH > > ecc_layout made compatible with Rom Boot Loader ECC layout for am335x. > > This action is based on is_elm_used flag. > > Requirement of this flag is to identify the whether the platform > ELM module & based on this configure ELM module if present. > > But ideally BCH8 ecc lay out didn't have a dependency on ELM, it > can work with software BCH ecc. RBL compatibility is missing > in software BCH because of addition of constant polynomial to > ecc vector. If we remove the dependency on erased page handling > by ecc vector with constant polynomial, software BCH can do the > job of RBL compatibility. > > Ivan, > Do you have any suggestions? > Discussion for RBL compatibility found at [2]. > > It is good that software BCH also support RBL compatibility by suppressing > constant polynomial modification. Then ecc layout can be selected from > DT entry and error correction can be chosen between software/hardware > depending on the availability of ELM hardware. > Currently RBL BCH8 compatibility depends on the availability of ELM > hardware. Later once software BCH start supporting RBL compatibility, > we can remove the check. > >> >> That is what I experienced, yes. The kernel was unable to parse NAND >> pages that were written from U-Boot with bch8 hardware mode when the elm >> module was not active. Maybe someone from TI can explain that? Giving it >> a dedicated name would also solve the problem with the extra DT property. > > Daniel, > > Currently BCH8 is supported with software ecc error correction in mainline. > The layout for BCH8 ECC layout is > 0-1 -> BAD block markers > 2-11-> oob free area > 12-63-> BCH8 ECC. > > RBL ECC layout is > 0-1 -> BAD block markers > 2-57-> BCH8 ecc layout > 58-63-> OOB free area > > As u-boot also maintain RBL ecc layout, write from U-boot > and read from Linux requires compatibility with RBL ecc layout. > The same is achieved in the patch [1], with by setting is_elm_used > to true. > > 1. https://lkml.org/lkml/2012/10/31/87 > 2. https://lkml.org/lkml/2012/10/11/20 So, after reading this, I'm still uncertain what's your preferred way of handling the bindings here. Are you saying we should stick with the is_elm_used flag, and subsequently care for BCH8 software mode compatibility? I would like to submit a new version soon, so it can be queued up for the next merge window, and that decision is the last blocker currently for sending out a new series. Many thanks, Daniel
On Tue, Nov 27, 2012 at 19:30:54, Daniel Mack wrote: > Hi Avinash, > Hi Peter, > > On 23.11.2012 11:43, Philip, Avinash wrote: > > On Wed, Nov 21, 2012 at 15:45:23, Daniel Mack wrote: > >> On 20.11.2012 16:59, Peter Korsgaard wrote: > >>>>>>>> "Daniel" == Daniel Mack <zonque@gmail.com> writes: > > > Peter, > > > > In patch [1], mtd: nand: omap2: Support for hardware BCH > > > > ecc_layout made compatible with Rom Boot Loader ECC layout for am335x. > > > > This action is based on is_elm_used flag. > > > > Requirement of this flag is to identify the whether the platform > > ELM module & based on this configure ELM module if present. > > > > But ideally BCH8 ecc lay out didn't have a dependency on ELM, it > > can work with software BCH ecc. RBL compatibility is missing > > in software BCH because of addition of constant polynomial to > > ecc vector. If we remove the dependency on erased page handling > > by ecc vector with constant polynomial, software BCH can do the > > job of RBL compatibility. > > > > Ivan, > > Do you have any suggestions? > > Discussion for RBL compatibility found at [2]. > > > > It is good that software BCH also support RBL compatibility by suppressing > > constant polynomial modification. Then ecc layout can be selected from > > DT entry and error correction can be chosen between software/hardware > > depending on the availability of ELM hardware. > > Currently RBL BCH8 compatibility depends on the availability of ELM > > hardware. Later once software BCH start supporting RBL compatibility, > > we can remove the check. > > > >> > >> That is what I experienced, yes. The kernel was unable to parse NAND > >> pages that were written from U-Boot with bch8 hardware mode when the elm > >> module was not active. Maybe someone from TI can explain that? Giving it > >> a dedicated name would also solve the problem with the extra DT property. > > > > Daniel, > > > > Currently BCH8 is supported with software ecc error correction in mainline. > > The layout for BCH8 ECC layout is > > 0-1 -> BAD block markers > > 2-11-> oob free area > > 12-63-> BCH8 ECC. > > > > RBL ECC layout is > > 0-1 -> BAD block markers > > 2-57-> BCH8 ecc layout > > 58-63-> OOB free area > > > > As u-boot also maintain RBL ecc layout, write from U-boot > > and read from Linux requires compatibility with RBL ecc layout. > > The same is achieved in the patch [1], with by setting is_elm_used > > to true. > > > > 1. https://lkml.org/lkml/2012/10/31/87 > > 2. https://lkml.org/lkml/2012/10/11/20 > > So, after reading this, I'm still uncertain what's your preferred way of > handling the bindings here. Are you saying we should stick with the > is_elm_used flag, and subsequently care for BCH8 software mode > compatibility? Ideally RBL compatibility didn't depend on the availability of ELM module. So from am335x perspective, RBL compatibility is achieved with ECC error correction with ELM module. I would submit one more revision of BCH8 ELM error correction support which would check for bch8-am335xrbl-compatible. Note: RBL ecc layout can vary from SOC to SOC Daniel, So can you start supporting "bch8-am335xrbl-compatible" and remove usage of is_elm_used in dt. This should come in ecc_opt field. In omap2 NAND driver, AM335x RBL compatibility is achieved depending on ecc_layout and runtime detection of elm module. So options related to can be 1. bch8-am335xrbl-compatible is enabled and run time detection of Of elm module passed, RBL compatibility achieved. 2. bch8-am335xrbl-compatible is enabled and run time detection of of elm module failed, RBL compatibility sacrificed but continue with software BCH8 error correction. Sacrificing RBL compatibility because of constant polynomial addition and usage of 14 byte instead of 13 byte. Ivan, Do you have any plan of removing addition of constant polynomial to ecc data and go for extra byte checking to find erased pages? This way we can start support BCH8 with RBL compatible and kernel Didn't put any restriction of the ecc layout. Thanks Avinash > > I would like to submit a new version soon, so it can be queued up for > the next merge window, and that decision is the last blocker currently > for sending out a new series. > > > Many thanks, > Daniel > >
On Wed, Nov 28, 2012 at 05:01:30AM +0000, Philip, Avinash wrote: (...) > > Daniel, > > So can you start supporting "bch8-am335xrbl-compatible" and remove usage of > is_elm_used in dt. This should come in ecc_opt field. > > In omap2 NAND driver, AM335x RBL compatibility is achieved depending on ecc_layout > and runtime detection of elm module. So options related to can be > 1. bch8-am335xrbl-compatible is enabled and run time detection of > Of elm module passed, RBL compatibility achieved. > 2. bch8-am335xrbl-compatible is enabled and run time detection of > of elm module failed, RBL compatibility sacrificed but continue with > software BCH8 error correction. Sacrificing RBL compatibility > because of constant polynomial addition and usage of 14 byte instead of 13 byte. > > Ivan, > Do you have any plan of removing addition of constant polynomial to ecc data > and go for extra byte checking to find erased pages? > This way we can start support BCH8 with RBL compatible and kernel > Didn't put any restriction of the ecc layout. Hello Avinash, Sorry about the response delay. First a short reminder of pros and cons of the "constant polynomial addition" (let's just call it "CPA") feature: pros: - it elegantly solves all problems related to reading an erased page (no clumsy hack needed) - better performance: when a bitflip appears on an erased page (often this is a "sticky" bitflip), there is no need to perform a full scan+cleanup of the page each time it is read cons: - the ecc vector is not compatible with RBL RBL compatibility is not necessary in my case, because I'm using the OMAP MLC ROM boot mode. Rather than completely removing the CPA feature, I'd like to keep it as an option; it could even be used with the ELM module. I'm OK to submit a patch in this direction, but first I'd like to wait for the dust to settle on arch/arm/mach-omap2 and mtd/nand/omap2.c with Afzal patches and everything; things have become a bit complicated to follow recently :-) Also, I think it would be nice to move BCH code out of omap2.c into a separate file. What do you think ? BR, -- Ivan
On Sun, Dec 02, 2012 at 03:20:14, Ivan Djelic wrote: > On Wed, Nov 28, 2012 at 05:01:30AM +0000, Philip, Avinash wrote: > (...) > > > > Daniel, > > > > So can you start supporting "bch8-am335xrbl-compatible" and remove usage of > > is_elm_used in dt. This should come in ecc_opt field. > > > > In omap2 NAND driver, AM335x RBL compatibility is achieved depending on ecc_layout > > and runtime detection of elm module. So options related to can be > > 1. bch8-am335xrbl-compatible is enabled and run time detection of > > Of elm module passed, RBL compatibility achieved. > > 2. bch8-am335xrbl-compatible is enabled and run time detection of > > of elm module failed, RBL compatibility sacrificed but continue with > > software BCH8 error correction. Sacrificing RBL compatibility > > because of constant polynomial addition and usage of 14 byte instead of 13 byte. > > > > Ivan, > > Do you have any plan of removing addition of constant polynomial to ecc data > > and go for extra byte checking to find erased pages? > > This way we can start support BCH8 with RBL compatible and kernel > > Didn't put any restriction of the ecc layout. > > Hello Avinash, > > Sorry about the response delay. > First a short reminder of pros and cons of the "constant polynomial addition" > (let's just call it "CPA") feature: > > pros: > - it elegantly solves all problems related to reading an erased page (no clumsy hack needed) > - better performance: when a bitflip appears on an erased page (often this is a "sticky" bitflip), > there is no need to perform a full scan+cleanup of the page each time it is read No need for scan + cleanup on each read, as the chances of encountering bit flips in erased page is less. Also to find bit flips in erased page, compare ecc vector for read erased page against a standard ecc vector. Presence of bit flips can find by checking the compare results. In case of mismatch, should go for correction of bit flips in erased page with full scan. So with this approach, we can nullify the effect of CPA for erased page bit flip handling. > > cons: > - the ecc vector is not compatible with RBL > > RBL compatibility is not necessary in my case, because I'm using the OMAP MLC ROM boot mode. > Rather than completely removing the CPA feature, I'd like to keep it as an option; it could > even be used with the ELM module. I agree RBL compatibility depends on the user. But with RBL compatibility, there is no sacrifice of any existing feature. Is it right? Also nand driver get simplified with removal of CPA, so that both HW & SW error correction can go for same ecc calculation. > > I'm OK to submit a patch in this direction, but first I'd like to wait for the dust to settle > on arch/arm/mach-omap2 and mtd/nand/omap2.c with Afzal patches and everything; things have become > a bit complicated to follow recently :-) Afzal's changes are in & are settled now. > Also, I think it would be nice to move BCH code out of omap2.c into a separate file. > What do you think ? With BCH code you mean, omap3_correct_data_bch()? I think this can be part of omap2 driver it self as it is ecc correctable method for nand driver. Thanks Avinash > > BR, > -- > Ivan >
On Wed, Dec 05, 2012 at 05:15:52AM +0000, Philip, Avinash wrote: (...) > > First a short reminder of pros and cons of the "constant polynomial addition" > > (let's just call it "CPA") feature: > > > > pros: > > - it elegantly solves all problems related to reading an erased page (no clumsy hack needed) > > - better performance: when a bitflip appears on an erased page (often this is a "sticky" bitflip), > > there is no need to perform a full scan+cleanup of the page each time it is read > > No need for scan + cleanup on each read, as the chances of encountering bit flips in erased page > is less. Also to find bit flips in erased page, compare ecc vector for read erased page against > a standard ecc vector. Presence of bit flips can find by checking the compare results. In case of > mismatch, should go for correction of bit flips in erased page with full scan. Hi Avinash, I explicitly mentioned the condition "when a bitflip appears on an erased page", in which case you _do_ have to do a full scan upon each read, until you erase the block; and then, the bitflip may still be there (this is what I called a "sticky" bitflip). My experience with recent devices (4-bit/8-bit) is that erased pages with a single bitflip are not uncommon. For those pages, there is undeniably a performance penalty compared to CPA. Benchmarks would be needed to quantify the overall performance impact, but I suspect it is small. > > So with this approach, we can nullify the effect of CPA for erased page bit flip handling. Well, not completely. I would happily drop CPA is that were the case. > > > > cons: > > - the ecc vector is not compatible with RBL > > > > RBL compatibility is not necessary in my case, because I'm using the OMAP MLC ROM boot mode. > > Rather than completely removing the CPA feature, I'd like to keep it as an option; it could > > even be used with the ELM module. > > I agree RBL compatibility depends on the user. But with RBL compatibility, there is no sacrifice > of any existing feature. Is it right? > > Also nand driver get simplified with removal of CPA, so that both HW & SW error correction > can go for same ecc calculation. Indeed that would be a simplification. > > > > I'm OK to submit a patch in this direction, but first I'd like to wait for the dust to settle > > on arch/arm/mach-omap2 and mtd/nand/omap2.c with Afzal patches and everything; things have become > > a bit complicated to follow recently :-) > > Afzal's changes are in & are settled now. What about this set: http://lists.infradead.org/pipermail/linux-mtd/2012-October/044337.html I cannot see it in l2-mtd-2.6 ? Or did I miss something ? Thanks, -- Ivan
On Thu, Dec 06, 2012 at 15:45:41, Ivan Djelic wrote: > On Wed, Dec 05, 2012 at 05:15:52AM +0000, Philip, Avinash wrote: > (...) > > > First a short reminder of pros and cons of the "constant polynomial addition" > > > (let's just call it "CPA") feature: > > > > > > pros: > > > - it elegantly solves all problems related to reading an erased page (no clumsy hack needed) > > > - better performance: when a bitflip appears on an erased page (often this is a "sticky" bitflip), > > > there is no need to perform a full scan+cleanup of the page each time it is read > > > > No need for scan + cleanup on each read, as the chances of encountering bit flips in erased page > > is less. Also to find bit flips in erased page, compare ecc vector for read erased page against > > a standard ecc vector. Presence of bit flips can find by checking the compare results. In case of > > mismatch, should go for correction of bit flips in erased page with full scan. > > Hi Avinash, > > I explicitly mentioned the condition "when a bitflip appears on an erased page", in which case you > _do_ have to do a full scan upon each read, until you erase the block; and then, the bitflip may still > be there (this is what I called a "sticky" bitflip). Bit flips in erased page require full scan. > My experience with recent devices (4-bit/8-bit) is that erased pages with a > single bitflip are not uncommon. What about erased page without bit flips? If we take the probability of bit flips and non-bit flips in erased page, erased pages with bit flips will be very less. So probability of scanning erased page is less. > For those pages, there is undeniably a performance penalty compared to CPA. Pages with bit flips would have better performance with CPA as no explicit scan present. Pages without bit flips, would have better performance if we compare against standard ecc vector than with present implementation of software BCH. The same can be achieved with software BCH also. > Benchmarks would be needed to quantify the overall performance impact, but I suspect it is small. So overall performance would be better with checking with standard ecc vector for finding erased page and handling bit flip in erased page with full scan due to probability that bit flips in erased page is very less than pages without bit flips. But with performance improvement for bit flipped erased page with CPA, we have to sacrifice RBL compatibility. So a common layout across all components would be missing. > > > > > So with this approach, we can nullify the effect of CPA for erased page bit flip handling. > > Well, not completely. I would happily drop CPA is that were the case. So the choices can be 1. Performance drop in case of bit flip in erased page with full scan of page if bit flip in erased page. But can achieve RBL compatibility, simplification of ecc calculation. 2. Sacrifice RBL compatibility with CPA, but still performance can be improved for erased pages without bit flips. But initial discussion in this direction told RBL compatibility would be a better option https://lkml.org/lkml/2012/10/11/240 > > > > > > > cons: > > > - the ecc vector is not compatible with RBL > > > > > > RBL compatibility is not necessary in my case, because I'm using the OMAP MLC ROM boot mode. > > > Rather than completely removing the CPA feature, I'd like to keep it as an option; it could > > > even be used with the ELM module. > > > > I agree RBL compatibility depends on the user. But with RBL compatibility, there is no sacrifice > > of any existing feature. Is it right? > > > > Also nand driver get simplified with removal of CPA, so that both HW & SW error correction > > can go for same ecc calculation. > > Indeed that would be a simplification. > > > > > > > I'm OK to submit a patch in this direction, but first I'd like to wait for the dust to settle > > > on arch/arm/mach-omap2 and mtd/nand/omap2.c with Afzal patches and everything; things have become > > > a bit complicated to follow recently :-) > > > > Afzal's changes are in & are settled now. > > What about this set: http://lists.infradead.org/pipermail/linux-mtd/2012-October/044337.html > I cannot see it in l2-mtd-2.6 ? Or did I miss something ? Yes, Afzal's changes are present in linux-next. I think it will get in next merge window. http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=history;f=drivers/mtd/nand/omap2.c;h=0002d5e94f0d0e3b84f36d2ccb505c95a30b4cdb;hb=cfc4410f5d3de0f68139ddffe065b9889a41d3c0 These changes not present in l2-mtd-2.6 Thanks Avinash > > Thanks, > -- > Ivan >
diff --git a/Documentation/devicetree/bindings/bus/ti-gpmc.txt b/Documentation/devicetree/bindings/bus/ti-gpmc.txt new file mode 100644 index 0000000..4f4a602 --- /dev/null +++ b/Documentation/devicetree/bindings/bus/ti-gpmc.txt @@ -0,0 +1,76 @@ +Device tree bindings for OMAP general purpose memory controllers (GPMC) + +The actual devices are instantiated from the child nodes of a GPMC node. + +Required properties: + + - compatible: Should be set to "ti,gpmc" + - reg: A resource specifier for the register space + (see the example below) + - ti,hwmods: Should be set to "ti,gpmc" until the DT transition is + completed. + - #address-cells: Must be set to 2 to allow memory address translation + - #size-cells: Must be set to 1 to allow CS address passing + - ranges: Must be set up to reflect the memory layout with four + integer values for each chip-select line in use: + + <cs-number> 0 <physical address of mapping> <size> + + Note that this property is not currently parsed. + Calculated values derived from the contents of the + per-CS register GPMC_CONFIG7 (as set up by the + bootloader) are used. That will change in the future, + so be sure to fill the correct values here. + +Timing properties for child nodes. All are optional and default to 0. + + - gpmc,sync-clk: Minimum clock period for synchronous mode, in picoseconds + + Chip-select signal timings corresponding to GPMC_CONFIG2: + - gpmc,cs-on: Assertion time + - gpmc,cs-rd-off: Read deassertion time + - gpmc,cs-wr-off: Write deassertion time + + ADV signal timings corresponding to GPMC_CONFIG3: + - gpmc,adv-on: Assertion time + - gpmc,adv-rd-off: Read deassertion time + - gpmc,adv-wr-off: Write deassertion time + + WE signals timings corresponding to GPMC_CONFIG4: + - gpmc,we-on: Assertion time + - gpmc,we-off: Deassertion time + + OE signals timings corresponding to GPMC_CONFIG4: + - gpmc,oe-on: Assertion time + - gpmc,oe-off: Deassertion time + + Access time and cycle time timings corresponding to GPMC_CONFIG5: + - gpmc,page-burst-access: Multiple access word delay + - gpmc,access: Start-cycle to first data valid delay + - gpmc,rd-cycle: Total read cycle time + - gpmc,wr-cycle: Total write cycle time + +The following are only on OMAP3430: + - gpmc,wr-access + - gpmc,wr-data-mux-bus + + +Example for an AM33xx board: + + gpmc: gpmc@50000000 { + compatible = "ti,gpmc"; + ti,hwmods = "gpmc"; + reg = <0x50000000 0x2000>; + interrupts = <100>; + + #address-cells = <2>; + #size-cells = <1>; + ranges = <0 0 0x08000000 0x10000000>; /* CS0 @addr 0x8000000, size 0x10000000 */ + + /* child nodes go here */ + }; + + + + + diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt new file mode 100644 index 0000000..d78bf49 --- /dev/null +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt @@ -0,0 +1,60 @@ +Device tree bindings for GPMC connected NANDs + +GPMC connected NAND (found on OMAP boards) are represented as child nodes of +the GPMC controller with a name of "nand". + +All timing relevant properties as well as generic gpmc child properties are +explained in a separate documents - please refer to +Documentation/devicetree/bindings/bus/ti-gpmc.txt + +For NAND specific properties such as ECC modes or bus width, please refer to +Documentation/devicetree/bindings/mtd/nand.txt + + +Required properties: + + - reg: The CS line the peripheral is connected to + +For inline partiton table parsing (optional): + + - #address-cells: should be set to 1 + - #size-cells: should be set to 1 + +Example for an AM33xx board: + + gpmc: gpmc@50000000 { + compatible = "ti,gpmc"; + ti,hwmods = "gpmc"; + reg = <0x50000000 0x1000000>; + interrupts = <100>; + #address-cells = <2>; + #size-cells = <1>; + ranges = <0 0 0x08000000 0x10000000>; /* CS0: NAND */ + + nand@0,0 { + reg = <0 0 0>; /* CS0, offset 0 */ + nand-bus-width = <16>; + nand-ecc-mode = "none"; + + gpmc,sync-clk = <0>; + gpmc,cs-on = <0>; + gpmc,cs-rd-off = <36>; + gpmc,cs-wr-off = <36>; + gpmc,adv-on = <6>; + gpmc,adv-rd-off = <24>; + gpmc,adv-wr-off = <36>; + gpmc,we-off = <30>; + gpmc,oe-off = <48>; + gpmc,access = <54>; + gpmc,rd-cycle = <72>; + gpmc,wr-cycle = <72>; + gpmc,wr-access = <30>; + gpmc,wr-data-mux-bus = <0>; + + #address-cells = <1>; + #size-cells = <1>; + + /* partitions go here */ + }; + }; + diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c index 1dcb30c..7ebe4fb 100644 --- a/arch/arm/mach-omap2/gpmc.c +++ b/arch/arm/mach-omap2/gpmc.c @@ -25,6 +25,10 @@ #include <linux/module.h> #include <linux/interrupt.h> #include <linux/platform_device.h> +#include <linux/of.h> +#include <linux/of_mtd.h> +#include <linux/of_device.h> +#include <linux/mtd/nand.h> #include <linux/platform_data/mtd-nand-omap2.h> @@ -37,6 +41,7 @@ #include "soc.h" #include "common.h" #include "gpmc.h" +#include "gpmc-nand.h" #define DEVICE_NAME "omap-gpmc" @@ -751,6 +756,133 @@ static int __devinit gpmc_mem_init(void) return 0; } +#ifdef CONFIG_OF +static struct of_device_id gpmc_dt_ids[] = { + { .compatible = "ti,gpmc" }, + { } +}; +MODULE_DEVICE_TABLE(of, gpmc_dt_ids); + +static void gpmc_read_timings_dt(struct device_node *np, + struct gpmc_timings *gpmc_t) +{ + u32 val; + + memset(gpmc_t, 0, sizeof(*gpmc_t)); + + /* minimum clock period for syncronous mode */ + if (!of_property_read_u32(np, "gpmc,sync-clk", &val)) + gpmc_t->sync_clk = val; + + /* chip select timtings */ + if (!of_property_read_u32(np, "gpmc,cs-on", &val)) + gpmc_t->cs_on = val; + + if (!of_property_read_u32(np, "gpmc,cs-rd-off", &val)) + gpmc_t->cs_rd_off = val; + + if (!of_property_read_u32(np, "gpmc,cs-wr-off", &val)) + gpmc_t->cs_wr_off = val; + + /* ADV signal timings */ + if (!of_property_read_u32(np, "gpmc,adv-on", &val)) + gpmc_t->adv_on = val; + + if (!of_property_read_u32(np, "gpmc,adv-rd-off", &val)) + gpmc_t->adv_rd_off = val; + + if (!of_property_read_u32(np, "gpmc,adv-wr-off", &val)) + gpmc_t->adv_wr_off = val; + + /* WE signal timings */ + if (!of_property_read_u32(np, "gpmc,we-on", &val)) + gpmc_t->we_on = val; + + if (!of_property_read_u32(np, "gpmc,we-off", &val)) + gpmc_t->we_off = val; + + /* OE signal timings */ + if (!of_property_read_u32(np, "gpmc,oe-on", &val)) + gpmc_t->oe_on = val; + + if (!of_property_read_u32(np, "gpmc,oe-off", &val)) + gpmc_t->oe_off = val; + + /* access and cycle timings */ + if (!of_property_read_u32(np, "gpmc,page-burst-access", &val)) + gpmc_t->page_burst_access = val; + + if (!of_property_read_u32(np, "gpmc,access", &val)) + gpmc_t->access = val; + + if (!of_property_read_u32(np, "gpmc,rd-cycle", &val)) + gpmc_t->rd_cycle = val; + + if (!of_property_read_u32(np, "gpmc,wr-cycle", &val)) + gpmc_t->wr_cycle = val; + + /* only for OMAP3430 */ + if (!of_property_read_u32(np, "gpmc,wr-access", &val)) + gpmc_t->wr_access = val; + + if (!of_property_read_u32(np, "gpmc,wr-data-mux-bus", &val)) + gpmc_t->wr_data_mux_bus = val; +} + +static int gpmc_probe_dt(struct platform_device *pdev) +{ + u32 val; + struct device_node *child; + struct gpmc_timings gpmc_t; + const struct of_device_id *of_id = + of_match_device(gpmc_dt_ids, &pdev->dev); + + if (!of_id) + return 0; + + for_each_node_by_name(child, "nand") { + struct omap_nand_platform_data *gpmc_nand_data; + + if (of_property_read_u32(child, "reg", &val) < 0) { + dev_err(&pdev->dev, "%s has no 'reg' property\n", + child->full_name); + continue; + } + + gpmc_nand_data = devm_kzalloc(&pdev->dev, + sizeof(*gpmc_nand_data), + GFP_KERNEL); + if (!gpmc_nand_data) { + dev_err(&pdev->dev, "unable to allocate memory?"); + return -ENOMEM; + } + + gpmc_nand_data->cs = val; + gpmc_nand_data->of_node = child; + + val = of_get_nand_ecc_mode(child); + if (val >= 0) + gpmc_nand_data->ecc_opt = val; + + val = of_get_nand_bus_width(child); + if (val == 16) + gpmc_nand_data->devsize = NAND_BUSWIDTH_16; + + gpmc_read_timings_dt(child, &gpmc_t); + gpmc_nand_init(gpmc_nand_data, &gpmc_t); + + of_node_put(child); + } + + return 0; +} +#else +static int gpmc_probe_dt(struct platform_device *pdev) +{ + return 0; +} +#endif + static __devinit int gpmc_probe(struct platform_device *pdev) { int rc; @@ -804,6 +936,14 @@ static __devinit int gpmc_probe(struct platform_device *pdev) if (IS_ERR_VALUE(gpmc_setup_irq())) dev_warn(gpmc_dev, "gpmc_setup_irq failed\n"); + rc = gpmc_probe_dt(pdev); + if (rc < 0) { + clk_disable_unprepare(gpmc_l3_clk); + clk_put(gpmc_l3_clk); + dev_err(gpmc_dev, "failed to probe DT parameters\n"); + return rc; + } + return 0; } @@ -821,6 +961,7 @@ static struct platform_driver gpmc_driver = { .driver = { .name = DEVICE_NAME, .owner = THIS_MODULE, + .of_match_table = of_match_ptr(gpmc_dt_ids), }, };
This patch adds basic DT bindings for OMAP GPMC. The actual peripherals are instanciated from child nodes within the GPMC node, and the only type of device that is currently supported is NAND. Code was added to parse the generic GPMC timing parameters and some documentation with examples on how to use them. Successfully tested on an AM33xx board. Signed-off-by: Daniel Mack <zonque@gmail.com> --- Documentation/devicetree/bindings/bus/ti-gpmc.txt | 76 +++++++++++ .../devicetree/bindings/mtd/gpmc-nand.txt | 60 +++++++++ arch/arm/mach-omap2/gpmc.c | 141 +++++++++++++++++++++ 3 files changed, 277 insertions(+) create mode 100644 Documentation/devicetree/bindings/bus/ti-gpmc.txt create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-nand.txt