Message ID | 1353338956-19420-4-git-send-email-zonque@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
>>>>> "D" == Daniel Mack <zonque@gmail.com> writes:
Hi,
D> This patch adds basic DT bindings for OMAP GPMC.
D> The actual peripherals are instanciated from child nodes within the GPMC
s/instanciated/instantiated/
D> node, and the only type of device that is currently supported is NAND.
D> Code was added to parse the generic GPMC timing parameters and some
D> documentation with examples on how to use them.
D> Successfully tested on an AM33xx board.
D> Signed-off-by: Daniel Mack <zonque@gmail.com>
D> ---
D> .../devicetree/bindings/mtd/gpmc-nand.txt | 84 ++++++++++
D> arch/arm/mach-omap2/gpmc.c | 170 +++++++++++++++++++++
D> 2 files changed, 254 insertions(+)
D> create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-nand.txt
D> diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
D> new file mode 100644
D> index 0000000..20aa5b9
D> --- /dev/null
D> +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
D> @@ -0,0 +1,84 @@
D> +Device tree bindings for GPMC connected NANDs
D> +
D> +GPMC connected NAND (found on OMAP boards) are represented as child nodes of
D> +the GPMC controller with a name of "nand".
D> +
D> +All timing relevant properties as well as generic gpmc child properties are
D> +explained in a separate documents - please refer to
D> +Documentation/devicetree/bindings/bus/ti-gpmc.txt
Which this patch seems to be missing?
D> +
D> +For NAND specific properties such as ECC modes or bus width, please refer to
D> +Documentation/devicetree/bindings/mtd/nand.txt
D> +
D> +
D> +Required properties:
D> +
D> + - reg: The CS line the peripheral is connected to
D> +
D> +Optional properties:
D> +
D> + - nand-bus-width: Set this numeric value to 16 if the hardware
D> + is wired that way. If not specified, a bus
D> + width of 8 is assumed.
D> + - ti,nand-ecc-opt: A string setting the ECC layout to use. One of:
D> +
D> + Layouts for 1-bit ecc: stored at end of spare area:
D> +
D> + "software" Software method (default)
D> + "hw" Hardware method (let gpmc do the error detection)
Nit: why are you spelling out 'software' when everything else is shortened?
D> +
D> + Layouts for 1-bit ecc: stored at beginning of spare area as romcode:
D> +
D> + "hw-romcode" gpmc method & romcode layout
D> + "bch4" 4-bit BCH ecc code
D> + "bch8" 8-bit BCH ecc code
D> +
These are not 1-bit - Well, romcode might be depending on SoC. Looking at
omap2.c it seems to be an alias for hw, so that isn't really helpful on
E.G. am33xx where you should select bch8 to have something compatible
with the romcode.
D> + - ti,nand-ecc-use-elm: Property without value to specify that the
D> + hardware error correction mode should be used.
It's called the error location module. Like you pointed out yourself,
this property isn't really nice. I haven't looked at the elm bindings in
detail, but couldn't the nand driver automatically use the elm if the
device tree provides a node for it? Perhaps the elm should be a subnode
of the nand one?
On Mon, Nov 19, 2012 at 20:59:16, Daniel Mack wrote: > This patch adds basic DT bindings for OMAP GPMC. > ... > + > + if (of_get_property(child, "ti,nand-ecc-use-elm", NULL)) > + gpmc_nand_data->is_elm_used = 1; Can you set to bool value (true) here. Thanks Avinash
Hi Peter, On 19.11.2012 21:52, Peter Korsgaard wrote: >>>>>> "D" == Daniel Mack <zonque@gmail.com> writes: > > Hi, > > D> This patch adds basic DT bindings for OMAP GPMC. > D> The actual peripherals are instanciated from child nodes within the GPMC > > s/instanciated/instantiated/ Thanks, fixed. > > D> node, and the only type of device that is currently supported is NAND. > > D> Code was added to parse the generic GPMC timing parameters and some > D> documentation with examples on how to use them. > > D> Successfully tested on an AM33xx board. > > D> Signed-off-by: Daniel Mack <zonque@gmail.com> > D> --- > D> .../devicetree/bindings/mtd/gpmc-nand.txt | 84 ++++++++++ > D> arch/arm/mach-omap2/gpmc.c | 170 +++++++++++++++++++++ > D> 2 files changed, 254 insertions(+) > D> create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-nand.txt > > D> diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > D> new file mode 100644 > D> index 0000000..20aa5b9 > D> --- /dev/null > D> +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > D> @@ -0,0 +1,84 @@ > D> +Device tree bindings for GPMC connected NANDs > D> + > D> +GPMC connected NAND (found on OMAP boards) are represented as child nodes of > D> +the GPMC controller with a name of "nand". > D> + > D> +All timing relevant properties as well as generic gpmc child properties are > D> +explained in a separate documents - please refer to > D> +Documentation/devicetree/bindings/bus/ti-gpmc.txt > > Which this patch seems to be missing? True - sorry for that. Will be back at v5. > D> + > D> +For NAND specific properties such as ECC modes or bus width, please refer to > D> +Documentation/devicetree/bindings/mtd/nand.txt > D> + > D> + > D> +Required properties: > D> + > D> + - reg: The CS line the peripheral is connected to > D> + > D> +Optional properties: > D> + > D> + - nand-bus-width: Set this numeric value to 16 if the hardware > D> + is wired that way. If not specified, a bus > D> + width of 8 is assumed. > D> + - ti,nand-ecc-opt: A string setting the ECC layout to use. One of: > D> + > D> + Layouts for 1-bit ecc: stored at end of spare area: > D> + > D> + "software" Software method (default) > D> + "hw" Hardware method (let gpmc do the error detection) > > Nit: why are you spelling out 'software' when everything else is shortened? Right. Fixed. > > D> + > D> + Layouts for 1-bit ecc: stored at beginning of spare area as romcode: > D> + > D> + "hw-romcode" gpmc method & romcode layout > D> + "bch4" 4-bit BCH ecc code > D> + "bch8" 8-bit BCH ecc code > D> + > > These are not 1-bit - Well, romcode might be depending on SoC. Looking at > omap2.c it seems to be an alias for hw, so that isn't really helpful on > E.G. am33xx where you should select bch8 to have something compatible > with the romcode. Not sure whether I follow you here. drivers/mtd/nand/omap2.c handles cases for pdata->ecc_opt == OMAP_ECC_BCH8_CODE_HW, so the DT bindings need to offer a way to set it. Or are you purely referring to the comments only? > D> + - ti,nand-ecc-use-elm: Property without value to specify that the > D> + hardware error correction mode should be used. > > It's called the error location module. Like you pointed out yourself, > this property isn't really nice. I haven't looked at the elm bindings in > detail, but couldn't the nand driver automatically use the elm if the > device tree provides a node for it? Perhaps the elm should be a subnode > of the nand one? That is to be discussed for the elm driver then. I'm not sure though whether the elm should always be used when its DT node is present. For now, I would leave it the way it is and have an optional "ti,nand-ecc-use-elm". I just added some more lines of documenation to describe the fact that the elm driver is needed in order to make it work. Daniel
>>>>> "Daniel" == Daniel Mack <zonque@gmail.com> writes: Hi, D> + Layouts for 1-bit ecc: stored at beginning of spare area as romcode: D> + D> + "hw-romcode" gpmc method & romcode layout D> + "bch4" 4-bit BCH ecc code D> + "bch8" 8-bit BCH ecc code D> + >> >> These are not 1-bit - Well, romcode might be depending on SoC. Looking at >> omap2.c it seems to be an alias for hw, so that isn't really helpful on >> E.G. am33xx where you should select bch8 to have something compatible >> with the romcode. Daniel> Not sure whether I follow you here. drivers/mtd/nand/omap2.c handles Daniel> cases for pdata->ecc_opt == OMAP_ECC_BCH8_CODE_HW, so the DT bindings Daniel> need to offer a way to set it. Daniel> Or are you purely referring to the comments only? Yes, but the document states (the first line I'm quotinge) 'Layouts for 1-bit ecc'. The other comment was about hw-romcode not being a very good name, as it apparently means the 1bit hamming code and ECC layout used on the older omap3, and not E.G. the bch8/elm layout used by E.G. am335x. D> + - ti,nand-ecc-use-elm: Property without value to specify that the D> + hardware error correction mode should be used. >> >> It's called the error location module. Like you pointed out yourself, >> this property isn't really nice. I haven't looked at the elm bindings in >> detail, but couldn't the nand driver automatically use the elm if the >> device tree provides a node for it? Perhaps the elm should be a subnode >> of the nand one? Daniel> That is to be discussed for the elm driver then. I'm not sure though Daniel> whether the elm should always be used when its DT node is present. Daniel> For now, I would leave it the way it is and have an optional Daniel> "ti,nand-ecc-use-elm". I just added some more lines of documenation to Daniel> describe the fact that the elm driver is needed in order to make it work. As discussed elsewhere, the elm also changes the ECC layout, so why not just have bchN (software bchN layout) / bchN-elm selections instead of the seperate ti,nand-ecc-use-elm?
On 23.11.2012 11:47, Peter Korsgaard wrote: >>>>>> "Daniel" == Daniel Mack <zonque@gmail.com> writes: > > Hi, > > D> + Layouts for 1-bit ecc: stored at beginning of spare area as romcode: > D> + > D> + "hw-romcode" gpmc method & romcode layout > D> + "bch4" 4-bit BCH ecc code > D> + "bch8" 8-bit BCH ecc code > D> + > >> > >> These are not 1-bit - Well, romcode might be depending on SoC. Looking at > >> omap2.c it seems to be an alias for hw, so that isn't really helpful on > >> E.G. am33xx where you should select bch8 to have something compatible > >> with the romcode. > > Daniel> Not sure whether I follow you here. drivers/mtd/nand/omap2.c handles > Daniel> cases for pdata->ecc_opt == OMAP_ECC_BCH8_CODE_HW, so the DT bindings > Daniel> need to offer a way to set it. > > Daniel> Or are you purely referring to the comments only? > > Yes, but the document states (the first line I'm quotinge) 'Layouts for > 1-bit ecc'. Ok, I guess I'll just remove these comments then. > The other comment was about hw-romcode not being a very good > name, as it apparently means the 1bit hamming code and ECC layout used > on the older omap3, and not E.G. the bch8/elm layout used by > E.G. am335x. So which name would you pick? "bch1"? > As discussed elsewhere, the elm also changes the ECC layout, so why not > just have bchN (software bchN layout) / bchN-elm selections instead of > the seperate ti,nand-ecc-use-elm? Avinash - what do you think? Thanks for the feedback - I'd also like to get these bindings right in the first place. Daniel
>>>>> "Daniel" == Daniel Mack <zonque@gmail.com> writes: Hi, >> The other comment was about hw-romcode not being a very good >> name, as it apparently means the 1bit hamming code and ECC layout used >> on the older omap3, and not E.G. the bch8/elm layout used by >> E.G. am335x. Daniel> So which name would you pick? "bch1"? No, it's hamming code, isn't it? To me it seems similar to the elm discussion, E.G. we have a mix of ECC algorithm + ECC layout + use of hw acceleration or not. The choice of hardware acceleration or not should preferably be transparent to this binding (E.G. it is purely an optimization, ECC strength/format shouldn't change), so the driver should be free to use it if it is available. Daniel> Thanks for the feedback - I'd also like to get these bindings Daniel> right in the first place. Me too!
diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt new file mode 100644 index 0000000..20aa5b9 --- /dev/null +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt @@ -0,0 +1,84 @@ +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 + +Optional properties: + + - nand-bus-width: Set this numeric value to 16 if the hardware + is wired that way. If not specified, a bus + width of 8 is assumed. + - ti,nand-ecc-opt: A string setting the ECC layout to use. One of: + + Layouts for 1-bit ecc: stored at end of spare area: + + "software" Software method (default) + "hw" Hardware method (let gpmc do the error detection) + + Layouts for 1-bit ecc: stored at beginning of spare area as romcode: + + "hw-romcode" gpmc method & romcode layout + "bch4" 4-bit BCH ecc code + "bch8" 8-bit BCH ecc code + + - ti,nand-ecc-use-elm: Property without value to specify that the + hardware error correction mode should be used. + +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>; + num-cs = <8>; + num-waitpins = <8>; + #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>; + ti,nand-ecc-opt = "none"; + ti,nand-ecc-use-elm; + + gpmc,sync-clk = <0>; + gpmc,cs-on = <0>; + gpmc,cs-rd-off = <44>; + gpmc,cs-wr-off = <44>; + gpmc,adv-on = <6>; + gpmc,adv-rd-off = <34>; + gpmc,adv-wr-off = <44>; + gpmc,we-off = <40>; + gpmc,oe-off = <54>; + gpmc,access = <64>; + gpmc,rd-cycle = <82>; + gpmc,wr-cycle = <82>; + gpmc,wr-access = <40>; + 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 7660a56..971a2bf 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> @@ -34,6 +38,7 @@ #include "common.h" #include "omap_device.h" #include "gpmc.h" +#include "gpmc-nand.h" #define DEVICE_NAME "omap-gpmc" @@ -748,6 +753,162 @@ 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 __maybe_unused 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; +} + +#ifdef CONFIG_MTD_NAND + +static const char *nand_ecc_opts[] = { + [OMAP_ECC_HAMMING_CODE_DEFAULT] = "software", + [OMAP_ECC_HAMMING_CODE_HW] = "hw", + [OMAP_ECC_HAMMING_CODE_HW_ROMCODE] = "hw-romcode", + [OMAP_ECC_BCH4_CODE_HW] = "bch4", + [OMAP_ECC_BCH8_CODE_HW] = "bch8", +}; + +static int __devinit gpmc_probe_nand_child(struct platform_device *pdev, + struct device_node *child) +{ + u32 val; + const char *s; + struct gpmc_timings gpmc_t; + 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); + return -ENODEV; + } + + gpmc_nand_data = devm_kzalloc(&pdev->dev, sizeof(*gpmc_nand_data), + GFP_KERNEL); + if (!gpmc_nand_data) + return -ENOMEM; + + gpmc_nand_data->cs = val; + gpmc_nand_data->of_node = child; + + if (!of_property_read_string(child, "ti,nand-ecc-opt", &s)) + for (val = 0; val < ARRAY_SIZE(nand_ecc_opts); val++) + if (!strcasecmp(s, nand_ecc_opts[val])) + gpmc_nand_data->ecc_opt = val; + + if (of_get_property(child, "ti,nand-ecc-use-elm", NULL)) + gpmc_nand_data->is_elm_used = 1; + + 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); + + return 0; +} +#else +static int __devinit gpmc_probe_nand_child(struct platform_device *pdev, + struct device_node *child) +{ + return 0; +} +#endif + +static int __devinit gpmc_probe_dt(struct platform_device *pdev) +{ + int ret; + struct device_node *child; + 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") { + ret = gpmc_probe_nand_child(pdev, child); + of_node_put(child); + if (ret < 0) + return ret; + } + + return 0; +} +#else +static int __devinit gpmc_probe_dt(struct platform_device *pdev) +{ + return 0; +} +#endif + static __devinit int gpmc_probe(struct platform_device *pdev) { int rc; @@ -801,6 +962,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; } @@ -818,6 +987,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> --- .../devicetree/bindings/mtd/gpmc-nand.txt | 84 ++++++++++ arch/arm/mach-omap2/gpmc.c | 170 +++++++++++++++++++++ 2 files changed, 254 insertions(+) create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-nand.txt