On Sat, Sep 17, 2016 at 07:31:23PM +0200, Boris Brezillon wrote:
> Hi Sergio,
>
> On Sat, 17 Sep 2016 12:22:40 -0300
> Sergio Prado <sergio.prado@xxxxxxxxxxxxxx> wrote:
>
> > Tested on FriendlyARM Mini2440
> >
> > Signed-off-by: Sergio Prado <sergio.prado@xxxxxxxxxxxxxx>
> > ---
> > .../devicetree/bindings/mtd/samsung-s3c2410.txt | 70 +++++++++++
>
> DT maintainers usually ask people to keep the DT bindings doc in a
> separate patch.
Right. I'll send the DT bindings doc in a separate patch.
>
> > drivers/mtd/nand/s3c2410.c | 129 ++++++++++++++++++++-
> > include/linux/platform_data/mtd-nand-s3c2410.h | 1 +
> > 3 files changed, 195 insertions(+), 5 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/mtd/samsung-s3c2410.txt
> >
> > diff --git a/Documentation/devicetree/bindings/mtd/samsung-s3c2410.txt b/Documentation/devicetree/bindings/mtd/samsung-s3c2410.txt
> > new file mode 100644
> > index 000000000000..1c39f6cf483b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/samsung-s3c2410.txt
> > @@ -0,0 +1,70 @@
> > +* Samsung S3C2410 and compatible NAND flash controller
> > +
> > +Required properties:
> > +- compatible : The possible values are:
> > + "samsung,s3c2410-nand"
> > + "samsung,s3c2412-nand"
> > + "samsung,s3c2440-nand"
> > + "samsung,s3c6400-nand"
> > +- reg : register's location and length.
> > +- #address-cells, #size-cells : see nand.txt
> > +- clocks : phandle to the nand controller clock
> > +- clock-names : must contain "nand"
> > +
> > +Optional properties:
> > +- samsung,tacls : time for active CLE/ALE to nWE/nOE
> > +- samsung,twrph0 : active time for nWE/nOE
> > +- samsung,twrph1 : time for release CLE/ALE from nWE/nOE inactive
>
> Can you try to extract these information from the nand_sdr_timings
> struct instead of passing it in your DT?
OK. Looks like it is possible to extract the timings from
nand_sdr_timings. I'll try to do it.
>
> > +- samsung,ignore_unset_ecc : boolean to ignore error when we have
> > + 0xff,0xff,0xff read ECC, on the
> > + assumption that it is an un-eccd page
> > +
> > +Optional children nodes:
> > +Children nodes representing the available nand chips.
> > +
> > +Optional children properties:
> > +- nand-ecc-mode : see nand.txt
> > +- nand-on-flash-bbt : see nand.txt
> > +
> > +Each children device node may optionally contain a 'partitions' sub-node,
> > +which further contains sub-nodes describing the flash partition mapping.
> > +See partition.txt for more detail.
> > +
> > +Example:
> > +
> > +nand@4e000000 {
>
> s/nand/nand-controller/
Ops. My bad.
>
> > + compatible = "samsung,s3c2440-nand";
> > + reg = <0x4e000000 0x40>;
> > +
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + clocks = <&clocks HCLK_NAND>;
> > + clock-names = "nand";
> > +
> > + samsung,tacls = <0>;
> > + samsung,twrph0 = <25>;
> > + samsung,twrph1 = <15>;
>
> As said above, I think these timings can be extracted from the
> nand_sdr_timings struct, which is know automatically attached to
> nand_chip at detection time.
Right.
>
> > + samsung,ignore_unset_ecc;
>
> Just discovered this ->ignore_unset_ecc property, and I don't
> understand why it's here for...
> Either you don't want to have ECC, and in this case you should set
> NAND_ECC_NONE, or you want to have ECC calculated, and in this case,
> the only valid situation where ECC bytes are 0xff is when the page is
> erased.
>
> If I'm right, please fix the driver instead of adding this DT property.
> If I'm wrong, could you explain in more detail when you have ECC bytes
> set to 0xff?
That's what I initially thought, but I must confess I just focused on
keeping the same interface. I'll try to understand better if this check is
really necessary.
>
> > +
> > + nand@0 {
>
> @0 implies having a reg property. I don't see any in your example, and
> it's not document in the required property list.
>
> Is your controller able to connect to different CS?
No, it is not necessary. I'll remove @0.
>
> > + nand-ecc-mode = "soft";
> > + nand-on-flash-bbt;
> > +
> > + partitions {
> > + compatible = "fixed-partitions";
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > +
> > + partition@0 {
> > + label = "u-boot";
> > + reg = <0 0x040000>;
> > + };
> > +
> > + partition@40000 {
> > + label = "kernel";
> > + reg = <0x040000 0x500000>;
> > + };
> > + };
> > + };
> > +};
> > diff --git a/drivers/mtd/nand/s3c2410.c b/drivers/mtd/nand/s3c2410.c
> > index d9309cf0ce2e..ecbb9c9c1e9a 100644
> > --- a/drivers/mtd/nand/s3c2410.c
> > +++ b/drivers/mtd/nand/s3c2410.c
> > @@ -39,6 +39,8 @@
> > #include <linux/slab.h>
> > #include <linux/clk.h>
> > #include <linux/cpufreq.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> >
> > #include <linux/mtd/mtd.h>
> > #include <linux/mtd/nand.h>
> > @@ -185,6 +187,26 @@ struct s3c2410_nand_info {
> > #endif
> > };
> >
> > +struct s3c24XX_nand_devtype_data {
> > + enum s3c_cpu_type type;
> > +};
> > +
> > +struct s3c24XX_nand_devtype_data s3c2410_nand_devtype_data = {
> > + .type = TYPE_S3C2410,
> > +};
> > +
> > +struct s3c24XX_nand_devtype_data s3c2412_nand_devtype_data = {
> > + .type = TYPE_S3C2412,
> > +};
> > +
> > +struct s3c24XX_nand_devtype_data s3c2440_nand_devtype_data = {
> > + .type = TYPE_S3C2440,
> > +};
> > +
> > +struct s3c24XX_nand_devtype_data s3c6400_nand_devtype_data = {
> > + .type = TYPE_S3C2412,
> > +};
> > +
> > /* conversion functions */
> >
> > static struct s3c2410_nand_mtd *s3c2410_nand_mtd_toours(struct mtd_info *mtd)
> > @@ -813,6 +835,8 @@ static void s3c2410_nand_init_chip(struct s3c2410_nand_info *info,
> > struct nand_chip *chip = &nmtd->chip;
> > void __iomem *regs = info->regs;
> >
> > + nand_set_flash_node(chip, set->of_node);
> > +
> > chip->write_buf = s3c2410_nand_write_buf;
> > chip->read_buf = s3c2410_nand_read_buf;
> > chip->select_chip = s3c2410_nand_select_chip;
> > @@ -947,6 +971,96 @@ static void s3c2410_nand_update_chip(struct s3c2410_nand_info *info,
> > }
> > }
> >
> > +#ifdef CONFIG_OF_MTD
>
> Hm, I thought this symbol had been dropped, but apparently I forgot to
> remove it. You should make it dependent on CONFIG_OF, not CONFIG_OF_MTD.
>
> Anyway, I'm not even sure this ifdef is needed. Just test if
> pdev->dev.of_node is NULL in s3c24xx_nand_probe_dt() and return -1 in
> this case.
Right. I'll remove this ifdef.
>
> > +static const struct of_device_id s3c24xx_nand_dt_ids[] = {
> > + {
> > + .compatible = "samsung,s3c2410-nand",
> > + .data = &s3c2410_nand_devtype_data,
> > + }, {
> > + .compatible = "samsung,s3c2412-nand",
> > + .data = &s3c2412_nand_devtype_data,
> > + }, {
> > + .compatible = "samsung,s3c2440-nand",
> > + .data = &s3c2440_nand_devtype_data,
> > + }, {
> > + .compatible = "samsung,s3c6400-nand",
> > + .data = &s3c6400_nand_devtype_data,
> > + },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, s3c24xx_nand_dt_ids);
> > +
> > +static int s3c24xx_nand_probe_dt(struct platform_device *pdev)
> > +{
> > + const struct s3c24XX_nand_devtype_data *devtype_data;
> > + struct s3c2410_platform_nand *pdata;
> > + struct s3c2410_nand_info *info = platform_get_drvdata(pdev);
> > + struct device_node *np = pdev->dev.of_node, *child;
> > + const struct of_device_id *of_id;
> > + struct s3c2410_nand_set *sets;
> > +
> > + of_id = of_match_device(s3c24xx_nand_dt_ids, &pdev->dev);
> > + if (!of_id)
> > + return 1;
> > +
> > + devtype_data = of_id->data;
> > + info->cpu_type = devtype_data->type;
> > +
> > + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> > + if (!pdata)
> > + return -ENOMEM;
> > +
> > + pdev->dev.platform_data = pdata;
> > +
> > + of_property_read_u32(np, "samsung,tacls", &pdata->tacls);
> > + of_property_read_u32(np, "samsung,twrph0", &pdata->twrph0);
> > + of_property_read_u32(np, "samsung,twrph1", &pdata->twrph1);
> > +
> > + if (of_get_property(np, "samsung,ignore_unset_ecc", NULL))
> > + pdata->ignore_unset_ecc = 1;
> > +
> > + pdata->nr_sets = of_get_child_count(np);
> > + if (!pdata->nr_sets)
> > + return 0;
> > +
> > + sets = devm_kzalloc(&pdev->dev, sizeof(*sets) * pdata->nr_sets,
> > + GFP_KERNEL);
> > + if (!sets)
> > + return -ENOMEM;
> > +
> > + pdata->sets = sets;
> > +
> > + for_each_available_child_of_node(np, child) {
> > +
> > + sets->name = (char *)child->name;
> > + sets->of_node = child;
> > + sets->nr_chips = 1;
> > +
> > + if (!of_property_match_string(child, "nand-ecc-mode", "none"))
> > + sets->disable_ecc = 1;
> > +
> > + if (of_get_property(child, "nand-on-flash-bbt", NULL))
> > + sets->flash_bbt = 1;
> > +
>
> These properties are automatically extracted in nand_scan_ident(), why
> do you need to parse them twice?
Looks like the driver uses this properties before they are extracted in
nand_scan_ident(). But I'll try to understand better what the driver is
doing and prevent from parsing these properties twice.
>
> > + sets++;
> > + }
> > +
> > + return 0;
> > +}
> > +#else
> > +static int s3c24xx_nand_probe_dt(struct platform_device *pdev)
> > +{
> > + return 1;
> > +}
> > +#endif
> > +
> > +static void s3c24xx_nand_probe_pdata(struct platform_device *pdev)
> > +{
> > + struct s3c2410_nand_info *info = platform_get_drvdata(pdev);
> > +
> > + info->cpu_type = platform_get_device_id(pdev)->driver_data;
> > +}
> > +
> > /* s3c24xx_nand_probe
> > *
> > * called by device layer when it finds a device matching
> > @@ -956,8 +1070,7 @@ static void s3c2410_nand_update_chip(struct s3c2410_nand_info *info,
> > */
> > static int s3c24xx_nand_probe(struct platform_device *pdev)
> > {
> > - struct s3c2410_platform_nand *plat = to_nand_plat(pdev);
> > - enum s3c_cpu_type cpu_type;
> > + struct s3c2410_platform_nand *plat;
> > struct s3c2410_nand_info *info;
> > struct s3c2410_nand_mtd *nmtd;
> > struct s3c2410_nand_set *sets;
> > @@ -967,8 +1080,6 @@ static int s3c24xx_nand_probe(struct platform_device *pdev)
> > int nr_sets;
> > int setno;
> >
> > - cpu_type = platform_get_device_id(pdev)->driver_data;
> > -
> > info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
> > if (info == NULL) {
> > err = -ENOMEM;
> > @@ -991,6 +1102,14 @@ static int s3c24xx_nand_probe(struct platform_device *pdev)
> >
> > s3c2410_nand_clk_set_state(info, CLOCK_ENABLE);
> >
> > + err = s3c24xx_nand_probe_dt(pdev);
> > + if (err > 0)
> > + s3c24xx_nand_probe_pdata(pdev);
> > + else if (err < 0)
> > + goto exit_error;
> > +
> > + plat = to_nand_plat(pdev);
> > +
> > /* allocate and map the resource */
> >
> > /* currently we assume we have the one resource */
> > @@ -999,7 +1118,6 @@ static int s3c24xx_nand_probe(struct platform_device *pdev)
> >
> > info->device = &pdev->dev;
> > info->platform = plat;
> > - info->cpu_type = cpu_type;
> >
> > info->regs = devm_ioremap_resource(&pdev->dev, res);
> > if (IS_ERR(info->regs)) {
> > @@ -1156,6 +1274,7 @@ static struct platform_driver s3c24xx_nand_driver = {
> > .id_table = s3c24xx_driver_ids,
> > .driver = {
> > .name = "s3c24xx-nand",
> > + .of_match_table = s3c24xx_nand_dt_ids,
>
> If you keep the #ifdef CONFIG_OF section
>
> .of_match_table = of_match_ptr(s3c24xx_nand_dt_ids),
I'll remove the ifdef.
>
> > },
> > };
> >
> > diff --git a/include/linux/platform_data/mtd-nand-s3c2410.h b/include/linux/platform_data/mtd-nand-s3c2410.h
> > index c55e42ee57fa..9d20871e4bbd 100644
> > --- a/include/linux/platform_data/mtd-nand-s3c2410.h
> > +++ b/include/linux/platform_data/mtd-nand-s3c2410.h
> > @@ -40,6 +40,7 @@ struct s3c2410_nand_set {
> > char *name;
> > int *nr_map;
> > struct mtd_partition *partitions;
> > + struct device_node *of_node;
> > };
> >
> > struct s3c2410_platform_nand {
>
--
Sergio Prado
Embedded Labworks
Office: +55 11 2628-3461
Mobile: +55 11 97123-3420
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
