| Message ID | 1379941608-5068-1-git-send-email-m.krawczuk@partner.samsung.com |
|---|---|
| State | New, archived |
| Headers | show |
Hi Mateusz, On Monday 23 of September 2013 15:06:48 Mateusz Krawczuk wrote: > This patch add clk and device tree nodes for samsung onenand driver. > > since v1: > Updated Documentation according to Mark Rutland notes. > > Signed-off-by: Mateusz Krawczuk <m.krawczuk@partner.samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > --- > .../devicetree/bindings/mtd/samsung-onenand.txt | 44 ++++++++++++++++++++++ > drivers/mtd/onenand/samsung.c | 37 +++++++++++++++++- > 2 files changed, 80 insertions(+), 1 deletion(-) > create mode 100644 Documentation/devicetree/bindings/mtd/samsung-onenand.txt > > diff --git a/Documentation/devicetree/bindings/mtd/samsung-onenand.txt b/Documentation/devicetree/bindings/mtd/samsung-onenand.txt > new file mode 100644 > index 0000000..5bf931c > --- /dev/null > +++ b/Documentation/devicetree/bindings/mtd/samsung-onenand.txt > @@ -0,0 +1,44 @@ > +Device tree bindings for Samsung Onenand > + > +Required properties: > + - compatible: value should be either of the following. > + (a) "samsung,s3c6400-onenand", > + for onenand controller compatible with s3c6400. > + (b) "samsung,s3c6410-onenand", > + for onenand controller compatible with s3c6410. > + (c) "samsung,s5pc100-onenand", > + for onenand controller compatible with s5pc100. > + (d) "samsung,s5pc110-onenand", For consistency with other bindings, I would use s5pv210 here, even if existing driver code refers to it as s5pc110. Device tree bindings are independent from driver internals. > + for s5pc100-like onenand controller used on s5pc110 which supports DMA. nit: inconsistent indentation of (d) case nit2: each subsequent indentation level should use higher indentation: - compatible: value should be either of the following. (a) "samsung,s3c6400-onenand", for onenand controller compatible with s3c6400. > + > +Required properties for s5pc110: > + > + - reg: the offset and length of the control registers. First region describes > + OneNAND interface, second control registers. > + - interrupt-parent, interrupts Onenand memory interrupts Inconsistent formatting of property description. The reg property above used "property: description", while this one uses a single tab. Also please describe both properties separately and specify how many interrupts are required. > + > +Required properties for others: > + > + - reg: the offset and length of the control registers. First region describes > + control registers, second OneNAND interface. > + > +Clocks: > + - gate - clock which output is supplied to external OneNAND flash memory. Inconsistent formatting of property description. > + > + > +For partiton table parsing (optional) please refer to: > + [1] Documentation/devicetree/bindings/mtd/partition.txt > + > +Example for an s5pv210 board: > + > + onenand@b0000000 { > + compatible = "samsung,s5pc110-onenand"; > + reg = <0xb0000000 0x20000>, <0xb0600000 0x2000>; > + interrupt-parent = <&vic1>; > + interrupts = <31>; > + #address-cells = <1>; > + #size-cells = <1>; > + clocks = <&clocks NANDXL>; > + clock-names = "gate"; > + status = "okay"; This status property is not a part of this binding, so it can be safely dropped. > + }; > diff --git a/drivers/mtd/onenand/samsung.c b/drivers/mtd/onenand/samsung.c > index df7400d..5a2cc4b 100644 > --- a/drivers/mtd/onenand/samsung.c > +++ b/drivers/mtd/onenand/samsung.c > @@ -14,6 +14,7 @@ > * S5PC110: use DMA > */ > > +#include <linux/clk.h> > #include <linux/module.h> > #include <linux/platform_device.h> > #include <linux/sched.h> > @@ -24,6 +25,7 @@ > #include <linux/dma-mapping.h> > #include <linux/interrupt.h> > #include <linux/io.h> > +#include <linux/of.h> > > #include <asm/mach/flash.h> > > @@ -133,6 +135,7 @@ enum soc_type { > struct s3c_onenand { > struct mtd_info *mtd; > struct platform_device *pdev; > + struct clk *gate; > enum soc_type type; > void __iomem *base; > struct resource *base_res; > @@ -859,6 +862,19 @@ static void s3c_onenand_setup(struct mtd_info *mtd) > this->write_bufferram = onenand_write_bufferram; > } > > +static const struct of_device_id onenand_s3c_dt_match[] = { > + { .compatible = "samsung,s3c6400-onenand", > + .data = (void *)TYPE_S3C6400 }, > + { .compatible = "samsung,s3c6410-onenand", > + .data = (void *)TYPE_S3C6410 }, > + { .compatible = "samsung,s5pc100-onenand", > + .data = (void *)TYPE_S5PC100 }, > + { .compatible = "samsung,s5pc110-onenand", > + .data = (void *)TYPE_S5PC110 }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, onenand_s3c_dt_match); > + > static int s3c_onenand_probe(struct platform_device *pdev) > { > struct onenand_platform_data *pdata; > @@ -883,12 +899,26 @@ static int s3c_onenand_probe(struct platform_device *pdev) > goto onenand_fail; > } > > + onenand->gate = clk_get(&pdev->dev, "gate"); You can use devm_clk_get() here to remove the need to explicitly put the clock at remove. > + if (IS_ERR(onenand->gate)) > + return PTR_ERR(onenand->gate); > + clk_prepare_enable(onenand->gate); > + This is a separate change that should be submitted as a separate patch. Also you need to add a clk_disable_unprepare() in error path at the bottom of this function. Best regards, Tomasz
On Mon, Sep 23, 2013 at 02:06:48PM +0100, Mateusz Krawczuk wrote: > This patch add clk and device tree nodes for samsung onenand driver. > > since v1: > Updated Documentation according to Mark Rutland notes. > > Signed-off-by: Mateusz Krawczuk <m.krawczuk@partner.samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > --- > .../devicetree/bindings/mtd/samsung-onenand.txt | 44 ++++++++++++++++++++++ > drivers/mtd/onenand/samsung.c | 37 +++++++++++++++++- > 2 files changed, 80 insertions(+), 1 deletion(-) > create mode 100644 Documentation/devicetree/bindings/mtd/samsung-onenand.txt > > diff --git a/Documentation/devicetree/bindings/mtd/samsung-onenand.txt b/Documentation/devicetree/bindings/mtd/samsung-onenand.txt > new file mode 100644 > index 0000000..5bf931c > --- /dev/null > +++ b/Documentation/devicetree/bindings/mtd/samsung-onenand.txt > @@ -0,0 +1,44 @@ > +Device tree bindings for Samsung Onenand > + > +Required properties: > + - compatible: value should be either of the following. > + (a) "samsung,s3c6400-onenand", > + for onenand controller compatible with s3c6400. > + (b) "samsung,s3c6410-onenand", > + for onenand controller compatible with s3c6410. > + (c) "samsung,s5pc100-onenand", > + for onenand controller compatible with s5pc100. > + (d) "samsung,s5pc110-onenand", > + for s5pc100-like onenand controller used on s5pc110 which supports DMA. > + As I asked on the last posting, what are the differences between these implementations? > +Required properties for s5pc110: > + > + - reg: the offset and length of the control registers. First region describes > + OneNAND interface, second control registers. Also, the complete reg description is a bit confusing. How about something like: - reg: two register specifiers: [0] - The OneNAND interface [1] - The control registers Do we expect future OneNAND devices which may require more reg entries to describe? > + - interrupt-parent, interrupts Onenand memory interrupts As it's a common property, I don't think interrupt-parent needs to be described. Is there more than one interrupt? What's it called on the manual? > + > +Required properties for others: > + > + - reg: the offset and length of the control registers. First region describes > + control registers, second OneNAND interface. Why does the s5pc110 OneNAND binding take its registers in the opposite order to the rest of these? Can we not fix up the driver first to make it consistent? Then we only need to describe this once and it's going to be far less of a headache to support. > + > +Clocks: > + - gate - clock which output is supplied to external OneNAND flash memory. How about the following (without the Clocks header): - clocks: clock-specifiers for the clocks named in clock-names, per the common clock bindings. - clock-names: should contain "gate" for the clock to the OneNAND flash memory. Is this the only clock that might be necessary on all platforms? > + > + > +For partiton table parsing (optional) please refer to: > + [1] Documentation/devicetree/bindings/mtd/partition.txt > + > +Example for an s5pv210 board: > + > + onenand@b0000000 { > + compatible = "samsung,s5pc110-onenand"; > + reg = <0xb0000000 0x20000>, <0xb0600000 0x2000>; > + interrupt-parent = <&vic1>; > + interrupts = <31>; > + #address-cells = <1>; > + #size-cells = <1>; > + clocks = <&clocks NANDXL>; > + clock-names = "gate"; > + status = "okay"; > + }; Cheers, Mark.
Hi Mateusz, Mark, On Monday 23 of September 2013 15:08:23 Mark Rutland wrote: > On Mon, Sep 23, 2013 at 02:06:48PM +0100, Mateusz Krawczuk wrote: > > + > > +Required properties: > > + - compatible: value should be either of the following. > > + (a) "samsung,s3c6400-onenand", > > + for onenand controller compatible with s3c6400. > > + (b) "samsung,s3c6410-onenand", > > + for onenand controller compatible with s3c6410. > > + (c) "samsung,s5pc100-onenand", > > + for onenand controller compatible with s5pc100. > > + (d) "samsung,s5pc110-onenand", > > + for s5pc100-like onenand controller used on s5pc110 which supports DMA. > > + > > As I asked on the last posting, what are the differences between these > implementations? (d) is a completely different controller than (a), (b) and (c). They have completely different register layouts. Also see below. > > +Required properties for s5pc110: > > + > > + - reg: the offset and length of the control registers. First region describes > > + OneNAND interface, second control registers. > > Also, the complete reg description is a bit confusing. How about > something like: > > - reg: two register specifiers: > [0] - The OneNAND interface > [1] - The control registers This looks much better, although I wouldn't be afraid of using words instead of symbols to write this as follows instead: - reg: two memory mapped register regions: - first entry: memory mapped OneNAND chip, - second entry: control registers. But... The controller in theory supports more than one OneNAND chip, each mapped at different, so I'd suggest reordering the entries: - reg: memory mapped register regions: - first entry: control registers. - second entry: memory mapped OneNAND chip 0, - ... - Nth entry: memory mapped OneNAND chip (N-2). > Do we expect future OneNAND devices which may require more reg entries > to describe? Nope. In age of eMMC I wouldn't even expect any new OneNAND controller to be handled by these bindings. > > + - interrupt-parent, interrupts Onenand memory interrupts > > As it's a common property, I don't think interrupt-parent needs to > be described. > > Is there more than one interrupt? What's it called on the manual? There is one interrupt per OneNAND chip, so this is what should be represented in the binding. > > + > > +Required properties for others: > > + > > + - reg: the offset and length of the control registers. First region describes > > + control registers, second OneNAND interface. > > Why does the s5pc110 OneNAND binding take its registers in the opposite > order to the rest of these? Can we not fix up the driver first to make > it consistent? Then we only need to describe this once and it's going to > be far less of a headache to support. Both areas in both "major" hardware variants have completely different meaning: - in case of SoCs earlier or equal to S5PC100, first entry represents controller registers that are used to control various operating parameters, while second entry is a command/data interface, which is used to trigger read/write/etc. operations in the controller and push/pull data through it. In this variant there is no direct mapping of OneNAND chip in CPU address space. This variant supports only one memory chip per controller instance, - in case of S5PC110, each OneNAND chip is directly mapped into CPU address space. In addition there is a block of control registers that are used to configure the interface and control internal DMA engine (which is not present in <= S5PC100 variants). This variant supports multiple memory chips per controller instance (2 in case of S5PV210, but current driver handles only one AFAIK). > > + > > +Clocks: > > + - gate - clock which output is supplied to external OneNAND flash memory. > > How about the following (without the Clocks header): > > - clocks: clock-specifiers for the clocks named in clock-names, per the > common clock bindings. > > - clock-names: should contain "gate" for the clock to the OneNAND flash > memory. > > Is this the only clock that might be necessary on all platforms? S3C64xx SoCs have only one OneNAND gate clock per controller. S5PC100 SoC has one IP gate clock and one memory chip gate clock. S5PC110 SoC has one gate that gates both IP and all memory chips. Best regards, Tomasz
On Tue, Sep 24, 2013 at 02:00:01PM +0100, Tomasz Figa wrote: > Hi Mateusz, Mark, Hi, > > On Monday 23 of September 2013 15:08:23 Mark Rutland wrote: > > On Mon, Sep 23, 2013 at 02:06:48PM +0100, Mateusz Krawczuk wrote: > > > + > > > +Required properties: > > > + - compatible: value should be either of the following. > > > + (a) "samsung,s3c6400-onenand", > > > + for onenand controller compatible with s3c6400. > > > + (b) "samsung,s3c6410-onenand", > > > + for onenand controller compatible with s3c6410. > > > + (c) "samsung,s5pc100-onenand", > > > + for onenand controller compatible with s5pc100. > > > + (d) "samsung,s5pc110-onenand", > > > + for s5pc100-like onenand controller used on s5pc110 which supports DMA. > > > + > > > > As I asked on the last posting, what are the differences between these > > implementations? > > (d) is a completely different controller than (a), (b) and (c). They > have completely different register layouts. Also see below. Ok. > > > > +Required properties for s5pc110: > > > + > > > + - reg: the offset and length of the control registers. First region describes > > > + OneNAND interface, second control registers. > > > > Also, the complete reg description is a bit confusing. How about > > something like: > > > > - reg: two register specifiers: > > [0] - The OneNAND interface > > [1] - The control registers > > This looks much better, although I wouldn't be afraid of using words > instead of symbols to write this as follows instead: > > - reg: two memory mapped register regions: > - first entry: memory mapped OneNAND chip, > - second entry: control registers. I'm also happy with that. > > But... The controller in theory supports more than one OneNAND chip, each > mapped at different, so I'd suggest reordering the entries: > > - reg: memory mapped register regions: > - first entry: control registers. > - second entry: memory mapped OneNAND chip 0, > - ... > - Nth entry: memory mapped OneNAND chip (N-2). I agree on the reordering, hence the questions I had below. > > > Do we expect future OneNAND devices which may require more reg entries > > to describe? > > Nope. In age of eMMC I wouldn't even expect any new OneNAND controller > to be handled by these bindings. Ok. > > > > + - interrupt-parent, interrupts Onenand memory interrupts > > > > As it's a common property, I don't think interrupt-parent needs to > > be described. > > > > Is there more than one interrupt? What's it called on the manual? > > There is one interrupt per OneNAND chip, so this is what should be > represented in the binding. Ah. That should be mentioned explicitly, with the required ordering. > > > > + > > > +Required properties for others: > > > + > > > + - reg: the offset and length of the control registers. First region describes > > > + control registers, second OneNAND interface. > > > > Why does the s5pc110 OneNAND binding take its registers in the opposite > > order to the rest of these? Can we not fix up the driver first to make > > it consistent? Then we only need to describe this once and it's going to > > be far less of a headache to support. > > Both areas in both "major" hardware variants have completely different > meaning: > - in case of SoCs earlier or equal to S5PC100, first entry represents > controller registers that are used to control various operating > parameters, while second entry is a command/data interface, which is > used to trigger read/write/etc. operations in the controller and > push/pull data through it. In this variant there is no direct mapping > of OneNAND chip in CPU address space. This variant supports only > one memory chip per controller instance, > - in case of S5PC110, each OneNAND chip is directly mapped into CPU > address space. In addition there is a block of control registers that > are used to configure the interface and control internal DMA engine > (which is not present in <= S5PC100 variants). This variant supports > multiple memory chips per controller instance (2 in case of S5PV210, > but current driver handles only one AFAIK). Ok. Even with that, I think the suggestion to reorder the reg entries above still makes this more consistent, with the controller registers first, then an entry per-chip (which is either the chip itself or the related command/data interface depending on the particular controller). > > > > + > > > +Clocks: > > > + - gate - clock which output is supplied to external OneNAND flash memory. > > > > How about the following (without the Clocks header): > > > > - clocks: clock-specifiers for the clocks named in clock-names, per the > > common clock bindings. > > > > - clock-names: should contain "gate" for the clock to the OneNAND flash > > memory. > > > > Is this the only clock that might be necessary on all platforms? > > S3C64xx SoCs have only one OneNAND gate clock per controller. > S5PC100 SoC has one IP gate clock and one memory chip gate clock. > S5PC110 SoC has one gate that gates both IP and all memory chips. Good to know. Thanks for the thorough description. Cheers, Mark.
diff --git a/Documentation/devicetree/bindings/mtd/samsung-onenand.txt b/Documentation/devicetree/bindings/mtd/samsung-onenand.txt new file mode 100644 index 0000000..5bf931c --- /dev/null +++ b/Documentation/devicetree/bindings/mtd/samsung-onenand.txt @@ -0,0 +1,44 @@ +Device tree bindings for Samsung Onenand + +Required properties: + - compatible: value should be either of the following. + (a) "samsung,s3c6400-onenand", + for onenand controller compatible with s3c6400. + (b) "samsung,s3c6410-onenand", + for onenand controller compatible with s3c6410. + (c) "samsung,s5pc100-onenand", + for onenand controller compatible with s5pc100. + (d) "samsung,s5pc110-onenand", + for s5pc100-like onenand controller used on s5pc110 which supports DMA. + +Required properties for s5pc110: + + - reg: the offset and length of the control registers. First region describes + OneNAND interface, second control registers. + - interrupt-parent, interrupts Onenand memory interrupts + +Required properties for others: + + - reg: the offset and length of the control registers. First region describes + control registers, second OneNAND interface. + +Clocks: + - gate - clock which output is supplied to external OneNAND flash memory. + + +For partiton table parsing (optional) please refer to: + [1] Documentation/devicetree/bindings/mtd/partition.txt + +Example for an s5pv210 board: + + onenand@b0000000 { + compatible = "samsung,s5pc110-onenand"; + reg = <0xb0000000 0x20000>, <0xb0600000 0x2000>; + interrupt-parent = <&vic1>; + interrupts = <31>; + #address-cells = <1>; + #size-cells = <1>; + clocks = <&clocks NANDXL>; + clock-names = "gate"; + status = "okay"; + }; diff --git a/drivers/mtd/onenand/samsung.c b/drivers/mtd/onenand/samsung.c index df7400d..5a2cc4b 100644 --- a/drivers/mtd/onenand/samsung.c +++ b/drivers/mtd/onenand/samsung.c @@ -14,6 +14,7 @@ * S5PC110: use DMA */ +#include <linux/clk.h> #include <linux/module.h> #include <linux/platform_device.h> #include <linux/sched.h> @@ -24,6 +25,7 @@ #include <linux/dma-mapping.h> #include <linux/interrupt.h> #include <linux/io.h> +#include <linux/of.h> #include <asm/mach/flash.h> @@ -133,6 +135,7 @@ enum soc_type { struct s3c_onenand { struct mtd_info *mtd; struct platform_device *pdev; + struct clk *gate; enum soc_type type; void __iomem *base; struct resource *base_res; @@ -859,6 +862,19 @@ static void s3c_onenand_setup(struct mtd_info *mtd) this->write_bufferram = onenand_write_bufferram; } +static const struct of_device_id onenand_s3c_dt_match[] = { + { .compatible = "samsung,s3c6400-onenand", + .data = (void *)TYPE_S3C6400 }, + { .compatible = "samsung,s3c6410-onenand", + .data = (void *)TYPE_S3C6410 }, + { .compatible = "samsung,s5pc100-onenand", + .data = (void *)TYPE_S5PC100 }, + { .compatible = "samsung,s5pc110-onenand", + .data = (void *)TYPE_S5PC110 }, + {}, +}; +MODULE_DEVICE_TABLE(of, onenand_s3c_dt_match); + static int s3c_onenand_probe(struct platform_device *pdev) { struct onenand_platform_data *pdata; @@ -883,12 +899,26 @@ static int s3c_onenand_probe(struct platform_device *pdev) goto onenand_fail; } + onenand->gate = clk_get(&pdev->dev, "gate"); + if (IS_ERR(onenand->gate)) + return PTR_ERR(onenand->gate); + clk_prepare_enable(onenand->gate); + this = (struct onenand_chip *) &mtd[1]; mtd->priv = this; mtd->dev.parent = &pdev->dev; mtd->owner = THIS_MODULE; onenand->pdev = pdev; - onenand->type = platform_get_device_id(pdev)->driver_data; + + if (pdev->dev.of_node) { + const struct of_device_id *match; + + match = of_match_node(onenand_s3c_dt_match, pdev->dev.of_node); + onenand->type = (enum soc_type)match->data; + } else { + onenand->type = platform_get_device_id(pdev)->driver_data; + } + s3c_onenand_setup(mtd); @@ -1077,6 +1107,10 @@ static int s3c_onenand_remove(struct platform_device *pdev) kfree(onenand->page_buf); kfree(onenand); kfree(mtd); + + clk_disable_unprepare(onenand->gate); + clk_put(onenand->gate); + return 0; } @@ -1125,6 +1159,7 @@ MODULE_DEVICE_TABLE(platform, s3c_onenand_driver_ids); static struct platform_driver s3c_onenand_driver = { .driver = { .name = "samsung-onenand", + .of_match_table = of_match_ptr(onenand_s3c_dt_match), .pm = &s3c_pm_ops, }, .id_table = s3c_onenand_driver_ids,