diff mbox

[v3,4/4] ARM: OMAP: gpmc: add DT bindings for GPMC timings and NAND

Message ID 1351869956-2787-5-git-send-email-zonque@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Mack Nov. 2, 2012, 3:25 p.m. UTC
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

Comments

avinash philip Nov. 5, 2012, 11:03 a.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Mack Nov. 5, 2012, 12:58 p.m. UTC | #2
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


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
avinash philip Nov. 5, 2012, 1:29 p.m. UTC | #3
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
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Mack Nov. 7, 2012, 9:48 a.m. UTC | #4
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

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
avinash philip Nov. 7, 2012, 3:37 p.m. UTC | #5
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
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Mack Nov. 10, 2012, 6:56 p.m. UTC | #6
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

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Mack Nov. 15, 2012, 12:26 a.m. UTC | #7
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

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
avinash philip Nov. 19, 2012, 6:06 a.m. UTC | #8
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
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Korsgaard Nov. 20, 2012, 3:59 p.m. UTC | #9
>>>>> "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?
Daniel Mack Nov. 21, 2012, 10:15 a.m. UTC | #10
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

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
avinash philip Nov. 23, 2012, 10:43 a.m. UTC | #11
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
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Mack Nov. 27, 2012, 2 p.m. UTC | #12
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

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
avinash philip Nov. 28, 2012, 5:01 a.m. UTC | #13
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
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ivan Djelic Dec. 1, 2012, 9:50 p.m. UTC | #14
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
avinash philip Dec. 5, 2012, 5:15 a.m. UTC | #15
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
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ivan Djelic Dec. 6, 2012, 10:15 a.m. UTC | #16
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
avinash philip Dec. 6, 2012, 1:12 p.m. UTC | #17
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
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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),
 	},
 };