[of] deadlock b/c of gpiochip and pinctrl dependency

[Robert Abel]

Hi!

I found a dependency issue with gpiochip and pinctrl while using the gpio-hog 
feature that was implemented by Benoit
Parrot in f625d4601759f. Relevant device-tree overlay shown below [3].

I ran into a deadlock situation, because adding a gpiochip requires a 
registered pinctrl with registered ranges, because
the function to add the gpiochip will check for hogged pins and request gpio 
pin functionality for them.
This works in situations when gpiochip and pinctrl functionality don't depend 
on each other. However, consider the
following code, which is from the bcm2835 driver for Raspberry Pi found here 
[1]:

pinctrl-bcm2835.c:
err = gpiochip_add_data(&pc->gpio_chip, pc);
if (err) {
dev_err(dev, "could not add GPIO chipn");
return err;
}

...

pc->pctl_dev = devm_pinctrl_register(dev, &bcm2835_pinctrl_desc, pc);
if (IS_ERR(pc->pctl_dev)) {
gpiochip_remove(&pc->gpio_chip);
return PTR_ERR(pc->pctl_dev);
}

...

pinctrl_add_gpio_range(pc->pctl_dev, &pc->gpio_range);


It's not very obvious, that calling gpiochip_add_data should only be done after 
adding the required ranges to pinctrl,
because the following happens:

gpiochip_add_data(_with_key):
  of_gpiochip_add:
of_gpiochip_scan_gpios:
  for_each_available_child_of_node(chip->of_node, np) with "gpio-hog":
gpiod_hog
  gpiochip_request_own_desc:
gpiod_request_commit:
  status = chip->request(chip, gpio_chip_hwgpio(desc)):
gpiochip_generic_request(...):
  pinctrl_gpio_request:
pinctrl_get_device_gpio_range:
  /* GPIO range not found, because it wasn't registered yet 
*/
  return -EPROBE_DEFER;

This of course deadlocks my system, because everything ends up waiting on the 
gpiochip, which cannot ever be
instantiated. I couldn't find any documentation explicitly mentioning this 
dependency between gpiochip and pinctrl.
My workaround for the moment is to re-order the gpiochip_add_data call after 
the devm_pinctrl_register and
pinctrl_add_gpio_range calls. I haven't observed any ill effect, but I'm not 
sure what other components may already act
on pinctrl while gpiochip is dangling or if any of the other functions that are 
called by add require it as well.

Obviously, this approach won't work for SoCs where setting certain pinctrl 
settings need to touch gpio settings to e.g.
activate pull-ups.

In general, I sought the same functionality that Markus Pargmann was trying to 
implement in 2015 [2].
That is, to name and possibly export gpios through device tree.

It seems disadvantageous that one cannot currently specify pinctrl settings 
while hogging or export by name.
Dynamic device tree overlays also don't work. Perhaps an actual driver instead 
of the hogging mechanism would solve all
dependency issues better?

Regards,

Robert

Please CC, as I'm off-list.


  [1]: 
https://github.com/raspberrypi/linux/blob/83ea2e93/drivers/pinctrl/bcm/pinctrl-bcm2835.c#L1027
  [2]: 
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/357418.html
  [3]: gpio-hog.dtso
   /dts-v1/;
   /plugin/;

   #include 

   / {
   compatible = "brcm,bcm2708";
   fragment@0 {
   target = <&gpio>;
   __overlay__ {
   myLed {
   gpios = <18 GPIO_ACTIVE_HIGH>;
   gpio-hog;
   output-low;
   };
   };
   };
   };

Re: [PATCH] auxdisplay: charlcd: fix x/y command parsing

[Robert Abel]

Hi Miguel,

On 05 Dec 2018 17:47, Miguel Ojeda wrote:> Hi Mans,
>
> [CC'ing a few people involved previously on this]

Thanks for CC'ing me!

On 06 Dec 2018 11:06, Miguel Ojeda wrote [to Mans Rullgard]:
> Are you able to test this?

It's unfortunate that my original comment got ignored back when the breaking 
patch was submitted.
Nevertheless, if somebody posts a patch, I'm happy to test.

Regards,

Robert

[PATCH] SYSFS: fix attribute_group bin file path on removal

[Robert ABEL]

Cody Schafer already fixed binary file creation for attribute groups, 
see [1].

This patch makes the appropriate changes for binary file removal
of attribute groups.
[1]: http://lkml.org/lkml/2014/2/27/832

Signed-off-by: Robert ABEL 
From 706a652d3653470a5caae589f6945498026c94c1 Mon Sep 17 00:00:00 2001
From: Robert ABEL 
Date: Tue, 15 Apr 2014 22:01:35 +0200
Subject: [PATCH] SYSFS: fix attribute_group bin file path on removal

Cody Schafer already fixed binary file creation for attribute groups, see [1].
This patch makes the appropriate changes for binary file removal
of attribute groups.
[1]: http://lkml.org/lkml/2014/2/27/832

Signed-off-by: Robert ABEL 
---
 fs/sysfs/group.c |   10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index aa04068..7d2a860 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -18,7 +18,7 @@
 #include "sysfs.h"
 
 
-static void remove_files(struct kernfs_node *parent, struct kobject *kobj,
+static void remove_files(struct kernfs_node *parent,
 const struct attribute_group *grp)
 {
struct attribute *const *attr;
@@ -29,7 +29,7 @@ static void remove_files(struct kernfs_node *parent, struct 
kobject *kobj,
kernfs_remove_by_name(parent, (*attr)->name);
if (grp->bin_attrs)
for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++)
-   sysfs_remove_bin_file(kobj, *bin_attr);
+   kernfs_remove_by_name(parent, (*bin_attr)->attr.name);
 }
 
 static int create_files(struct kernfs_node *parent, struct kobject *kobj,
@@ -62,7 +62,7 @@ static int create_files(struct kernfs_node *parent, struct 
kobject *kobj,
break;
}
if (error) {
-   remove_files(parent, kobj, grp);
+   remove_files(parent, grp);
goto exit;
}
}
@@ -79,7 +79,7 @@ static int create_files(struct kernfs_node *parent, struct 
kobject *kobj,
break;
}
if (error)
-   remove_files(parent, kobj, grp);
+   remove_files(parent, grp);
}
 exit:
return error;
@@ -224,7 +224,7 @@ void sysfs_remove_group(struct kobject *kobj,
kernfs_get(kn);
}
 
-   remove_files(kn, kobj, grp);
+   remove_files(kn, grp);
if (grp->name)
kernfs_remove(kn);
 
-- 
1.7.9.5

[PATCH RESEND] sysfs: fix attribute_group bin file path on removal

[Robert ABEL]

I sent this patch to the list on 15 Apr 14.
However, no activity since. Should I have sent this patch somewhere else?

Regards,

Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH RESEND] sysfs: fix attribute_group bin file path on removal

[Robert ABEL]

Cody Schafer already fixed binary file creation for attribute groups, see [1].
This patch makes the appropriate changes for binary file removal
of attribute groups.
[1]: http://lkml.org/lkml/2014/2/27/832

Signed-off-by: Robert ABEL 
---
 fs/sysfs/group.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index aa04068..7d2a860 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -18,7 +18,7 @@
 #include "sysfs.h"
 
 
-static void remove_files(struct kernfs_node *parent, struct kobject *kobj,
+static void remove_files(struct kernfs_node *parent,
 const struct attribute_group *grp)
 {
struct attribute *const *attr;
@@ -29,7 +29,7 @@ static void remove_files(struct kernfs_node *parent, struct 
kobject *kobj,
kernfs_remove_by_name(parent, (*attr)->name);
if (grp->bin_attrs)
for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++)
-   sysfs_remove_bin_file(kobj, *bin_attr);
+   kernfs_remove_by_name(parent, (*bin_attr)->attr.name);
 }
 
 static int create_files(struct kernfs_node *parent, struct kobject *kobj,
@@ -62,7 +62,7 @@ static int create_files(struct kernfs_node *parent, struct 
kobject *kobj,
break;
}
if (error) {
-   remove_files(parent, kobj, grp);
+   remove_files(parent, grp);
goto exit;
}
}
@@ -79,7 +79,7 @@ static int create_files(struct kernfs_node *parent, struct 
kobject *kobj,
break;
}
if (error)
-   remove_files(parent, kobj, grp);
+   remove_files(parent, grp);
}
 exit:
return error;
@@ -224,7 +224,7 @@ void sysfs_remove_group(struct kobject *kobj,
kernfs_get(kn);
}
 
-   remove_files(kn, kobj, grp);
+   remove_files(kn, grp);
if (grp->name)
kernfs_remove(kn);
 
-- 
1.9.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: 3.14: WARNING: sysfs group ffffffff81c50ca0 not found for kobject 'target5:0:0'

[Robert Abel]

Hi Olaf,

On 16.05.2014 19:18, Olaf Hering wrote:

I did unplug an USB stick by accident with 3.14.4, the result is this
warning:


[94414.233882] usb 1-3: USB disconnect, device number 4
[94414.245377] scsi 5:0:0:0: rejecting I/O to offline device
[94414.245383] scsi 5:0:0:0: killing request
[94414.370354] FAT-fs (sdc1): unable to read boot sector to mark fs as 
dirty

[94414.370393] [ cut here ]
[94414.370400] WARNING: CPU: 2 PID: 3619 at fs/sysfs/group.c:216 
sysfs_remove_group+0xa1/0xb0()
[94414.370403] sysfs group 81c50ca0 not found for kobject 
'target5:0:0'
This [1] might fix the problem you encountered. I reckon one of your 
drivers exposes a binary attribute file in a named attribute group, 
which it tries to remove when being unloaded. However, the path it looks 
for this attribute file is likely wrong.


[1] http://comments.gmane.org/gmane.linux.kernel/1684070
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH RESEND] sysfs: fix attribute_group bin file path on removal

[Robert ABEL]

Cody Schafer already fixed binary file creation for attribute groups, see [1].
This patch makes the appropriate changes for binary file removal
of attribute groups.
[1]: http://lkml.org/lkml/2014/2/27/832

Signed-off-by: Robert ABEL 
---
 fs/sysfs/group.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index aa04068..7d2a860 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -18,7 +18,7 @@
 #include "sysfs.h"
 
 
-static void remove_files(struct kernfs_node *parent, struct kobject *kobj,
+static void remove_files(struct kernfs_node *parent,
 const struct attribute_group *grp)
 {
struct attribute *const *attr;
@@ -29,7 +29,7 @@ static void remove_files(struct kernfs_node *parent, struct 
kobject *kobj,
kernfs_remove_by_name(parent, (*attr)->name);
if (grp->bin_attrs)
for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++)
-   sysfs_remove_bin_file(kobj, *bin_attr);
+   kernfs_remove_by_name(parent, (*bin_attr)->attr.name);
 }
 
 static int create_files(struct kernfs_node *parent, struct kobject *kobj,
@@ -62,7 +62,7 @@ static int create_files(struct kernfs_node *parent, struct 
kobject *kobj,
break;
}
if (error) {
-   remove_files(parent, kobj, grp);
+   remove_files(parent, grp);
goto exit;
}
}
@@ -79,7 +79,7 @@ static int create_files(struct kernfs_node *parent, struct 
kobject *kobj,
break;
}
if (error)
-   remove_files(parent, kobj, grp);
+   remove_files(parent, grp);
}
 exit:
return error;
@@ -224,7 +224,7 @@ void sysfs_remove_group(struct kobject *kobj,
kernfs_get(kn);
}
 
-   remove_files(kn, kobj, grp);
+   remove_files(kn, grp);
if (grp->name)
kernfs_remove(kn);
 
-- 
1.9.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

of: dynamic: resource: add WARN in release_resource for DT overlays

[Robert ABEL]

This patch prevents a kernel OOPS when removing a DT overlay containing
nodes with reg properties. release_resources is called for these nodes.
However, the resource structs were never initialized, hence the kernel OOPS.

This is obviously a stopgap measure until a proper solution is coded, see [1].


[1]: https://lkml.org/lkml/2014/4/17/359
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] of: resource: add WARN for invalid release_resource calls

[Robert ABEL]

Signed-off-by: Robert ABEL 
---
 kernel/resource.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/kernel/resource.c b/kernel/resource.c
index 0bcebff..b4c9b27 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -236,6 +236,13 @@ static int __release_resource(struct resource *old)
 {
struct resource *tmp, **p;
 
+/* devicetree overlays:
+ * of code doesn't initialize parent, child, sibling
+ * gracefully 'do the right thing' here
+ */
+   if (WARN(!old->parent, "%s: uninitialized resource %s\n", __FUNCTION__, 
old->name))
+   return 0;
+
p = &old->parent->child;
for (;;) {
tmp = *p;
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] net: rfkill-regulator: fix compiler warning

[Robert ABEL]

pdata char* name => const char* name

Signed-off-by: Robert ABEL 
---
 include/linux/rfkill-regulator.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/rfkill-regulator.h b/include/linux/rfkill-regulator.h
index aca36bc..594d8e7 100644
--- a/include/linux/rfkill-regulator.h
+++ b/include/linux/rfkill-regulator.h
@@ -41,7 +41,7 @@
 #include 
 
 struct rfkill_regulator_platform_data {
-   char *name; /* the name for the rfkill switch */
+   const char *name;   /* the name for the rfkill switch */
enum rfkill_type type;  /* the type as specified in rfkill.h */
 };
 
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Robert Abel]

On Tue, Feb 17, 2015 at 3:25 PM, Roger Quadros  wrote:
> one more thing to note is that just specifying sync-clk-ps in DT is not 
> enough for
> asynchronous devices.
>
> The driver doesn't set gpmc_t->sync_clk if "gpmc,sync-read" or 
> "gpmc,sync-write"
> was not set in the DT, which would be the case for asynchronous devices.

You're mistaken. It's always parsed, no matter what. See [1]. But I
did implement your suggestion of computing the divider in the mean
time. I'm going to send the patches rebased to 3.19 tomorrow, after I
tested them.

> What is your proposal to make things better? And what is your use case that 
> doesn't
> work with existing setup?

Well, the current setup was obviously inspired by an asynchronous-only
use-case, where all the timings are in seconds and get converted
on-the-fly. Of course, currently, there is no support to re-compute
them once the gpmc_fclk changes frequency, but I guess that's what the
TODO about the clock framework is about?

Anyway, my synchronous use-case is inherently... synchronous.
Synchronous protocols shouldn't be specified in ns at all. They should
directly be specified in clock ticks. This is also advantageous, as
changes in gpmc_fclk don't need to propagate to the registers.

My main grief with the current setup is its dependency on the
*unknown* gpmc_fclk period at startup when the DT is processed and
GPMC code is called. For instance, my private DT currently operates on
the assumption of 166 Mhz L3 clk (and therefore gpmc_fclk), so all my
timings are in multiples of 6 ns. Should now a colleague of mine think
it would be a splendid idea to change this frequency by means of
bootloader options [to save power, to process data even faster, etc],
everything would break, because the L3 might have to be clocked at 100
MHz (due to divider limits along the clock tree for example) thus
making my gpmc_fclk period 10ns. Now all my synchronous timings are
wrong, because all DT values round to different clock ticks.

One solution would obviously be very verbose: Split synchronous and
asynchronous timings completely. Have a time-ns and a time-tck (where
time is waitmonitoring, or readaccess etc) setting for different use
cases. This would work for truly asynchronous (read _and_ write
asynchronous) as well as truly synchronous (read _and_ write
synchronous) setups.

This 'simple' model breaks for parts where one form of access is
synchronous while the other is asynchronous, e.g. Spansion WS/NS/VS
parts, see [2] pg. 10. There's no easy solution for mixed timings,
because some timing parameters are shared between synchronous and
asynchronous accesses (WAITMONITORINGTIME, CSONTIME, ADVONTIME,
WRDATAONADMUXBUS etc.). One obvious solution is to try to slow the
synchronous side down until asynchronous timings can be met, but that
would make for very slow accesses in most cases.

For instance, I originally started out thinking the GPMC would run at
100 MHz externally -- because it's the max frequency rating -- only to
be surprised when the clock looked weird on the GPMC_CLK pin, because
it was at 166 MHz. I'm not sure how to completely remedy this, but
specifying timings in ns in DT and then depending on the correct board
frequency is kind of shady... :(

Regards,

Robert

[1]: http://lxr.free-electrons.com/source/drivers/memory/omap-gpmc.c#L1509
[2]: http://processors.wiki.ti.com/images/5/56/SpansionNOR-AM35x.pdf ;
("WS/NS/VS can be used for synchronous burst read / asynchronous
single write")
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 3/8 v2] ARM OMAP2+ GPMC: add bus children

[Robert ABEL]

This patch adds support for spawning busses as children of the GPMC.

Signed-off-by: Robert ABEL 
---
 drivers/memory/omap-gpmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 5cabac8..78b78a64 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -1800,7 +1801,7 @@ static int gpmc_probe_generic_child(struct 
platform_device *pdev,
gpmc_cs_enable_mem(cs);
 
 no_timings:
-   if (of_platform_device_create(child, NULL, &pdev->dev))
+   if (!of_platform_populate(child, of_default_bus_match_table, NULL, 
&pdev->dev))
return 0;
 
dev_err(&pdev->dev, "failed to create gpmc child %s\n", child->name);
-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 5/8 v2] ARM OMAP2+ GPMC: change get_gpmc_timing_reg output for DTS

[Robert ABEL]

DTS output was formatted to require additional work when copy-pasting into DTS.
Nano-second timings were removed, because they were not a confidence interval 
nor
an indication what timing values would result in the same #ticks

Signed-off-by: Robert ABEL 
---
 drivers/memory/omap-gpmc.c | 33 +++--
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index b5c8305..ff1a1e7 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -337,32 +337,45 @@ static void gpmc_cs_bool_timings(int cs, const struct 
gpmc_bool_timings *p)
 }
 
 #ifdef DEBUG
+/**
+ * get_gpmc_timing_reg - read a timing parameter and print DTS settings for it.
+ * @cs  Chip Select Region
+ * @reg GPMC_CS_CONFIGn register offset.
+ * @st_bit  Start Bit
+ * @end_bit End Bit. Must be >= @st_bit.
+ * @nameDTS node name, w/o "gpmc,"
+ * @raw Raw Format Option.
+ *  raw format:  gpmc,name = 
+ *  tick format: gpmc,name =  /‍* x ticks *‍/
+ * @noval   Parameter values equal to 0 are not printed.
+ * @shift   Parameter value left shifts @shift, which is then printed instead 
of value.
+ *
+ */
 static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
   bool raw, bool noval, int shift,
   const char *name)
 {
u32 l;
-   int nr_bits, max_value, mask;
+   int nr_bits;
+   int mask;
 
l = gpmc_cs_read_reg(cs, reg);
nr_bits = end_bit - st_bit + 1;
-   max_value = (1 << nr_bits) - 1;
-   mask = max_value << st_bit;
-   l = (l & mask) >> st_bit;
+   mask = (1 << nr_bits) - 1;;
+   l = (l >> st_bit) & mask;
if (shift)
l = (shift << l);
if (noval && (l == 0))
return 0;
if (!raw) {
-   unsigned int time_ns_min, time_ns, time_ns_max;
+   /* DTS tick format for timings in ns */
+   unsigned int time_ns;
 
-   time_ns_min = gpmc_ticks_to_ns(l ? l - 1 : 0);
time_ns = gpmc_ticks_to_ns(l);
-   time_ns_max = gpmc_ticks_to_ns(l + 1 > max_value ?
-  max_value : l + 1);
-   pr_info("gpmc,%s = <%u> (%u - %u ns, %i ticks)\n",
-   name, time_ns, time_ns_min, time_ns_max, l);
+   pr_info("gpmc,%s = <%u> /* %i ticks */\n",
+   name, time_ns, l);
} else {
+   /* raw format */
pr_info("gpmc,%s = <%u>\n", name, l);
}
 
-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 6/8 v2] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Robert ABEL]

The WAITMONITORINGTIME is expressed as a number of GPMC_CLK clock cycles,
even though the access is defined as asynchronous, and no GPMC_CLK clock
is provided to the external device. Still, GPMCFCLKDIVIDER is used as a divider
for the GPMC clock, so it must be programmed to define the
correct WAITMONITORINGTIME delay.

Signed-off-by: Robert ABEL 
---
 drivers/memory/omap-gpmc.c | 15 +--
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index ff1a1e7..a6abaf0 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -566,19 +566,14 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings 
*t)
if (gpmc_capability & GPMC_HAS_WR_ACCESS)
GPMC_SET_ONE(GPMC_CS_CONFIG6, 24, 28, wr_access);
 
-   /* caller is expected to have initialized CONFIG1 to cover
-* at least sync vs async
-*/
l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
-   if (l & (GPMC_CONFIG1_READTYPE_SYNC | GPMC_CONFIG1_WRITETYPE_SYNC)) {
 #ifdef DEBUG
-   printk(KERN_INFO "GPMC CS%d CLK period is %lu ns (div %d)\n",
-   cs, (div * gpmc_get_fclk_period()) / 1000, div);
+   printk(KERN_INFO "GPMC CS%d CLK period is %lu ns (div %d)\n",
+   cs, (div * gpmc_get_fclk_period()) / 1000, div);
 #endif
-   l &= ~0x03;
-   l |= (div - 1);
-   gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
-   }
+   l &= ~0x03;
+   l |= (div - 1);
+   gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
 
gpmc_cs_bool_timings(cs, &t->bool_timings);
gpmc_cs_show_timings(cs, "after gpmc_cs_set_timings");
-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/8 v2] ARM OMAP2+ GPMC: fixes and bus children

[Robert ABEL]

These are the changes I proposed in two separate patchsets
#([1], [2]) rebased to 3.19 as well as new changes for little bugs
I noticed while preparing this patchset.

1. DEBUG was undefined in source code --> remove offending lines
2. add capability to have busses as children of the GPMC and multiple
   devices on a bus. See [2] for an example DTS syntax.
3. debug output was unaligned --> align it
4. output for copy-pasting to DTS had erroneous timing outputs and
   made it hard to copy-paste --> remove timing values, add comments
   as DTS comments.
5. WAITMONITORINGTIME is expressed as GPMC_CLK cycles for all accesses.
   GPMCFCLKDIVIDER is used as a divider, so it must always be programmed.
6. GPMCFCLKDIVIDER is calculated according to WAITMONITORINGTIME for
   asynchronous accesses inside the driver --> asynchronous accesses now
   completely decoupled from gpmc,sync-clk-ps.
7. WAITMONITORINGTIME was being programmed/shown in GPMC_FCLK cycles instead
   of GPMC_CLK cycles --> add clock domain information where necessary.
8. Calculated values for WAITMONITORINGTIME and CLKACTIVATIONTIME that were
   outside the defined range would not raise an error.
   DEVICESIZE, ATTACHEDDEVICEPAGELENGTH, WAITMONITORINGTIME and 
   CLKACTIVATIONTIME would not be marked as incorrect on DTS output.
   --> Fix all of these.

[1]: https://lkml.org/lkml/2015/2/12/495
[2]: https://lkml.org/lkml/2015/2/16/337

Robert ABEL (9):
  ARM OMAP2+ GPMC: don't undef DEBUG
  ARM OMAP2+ GPMC: add bus children
  ARM OMAP2+ GPMC: fix debug output alignment
  ARM OMAP2+ GPMC: change get_gpmc_timing_reg output for DTS
  ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER
  ARM OMAP2+ GPMC: calculate GPMCFCLKDIVIDER based on WAITMONITORINGTIME
  ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug
  ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters

 arch/arm/mach-omap2/gpmc-nand.c|  17 +--
 arch/arm/mach-omap2/gpmc-onenand.c |   4 +-
 drivers/memory/Makefile|   2 +
 drivers/memory/omap-gpmc.c | 287 +
 include/linux/omap-gpmc.h  |   2 +-
 5 files changed, 240 insertions(+), 72 deletions(-)

-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 8/8 v2] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

[Robert ABEL]

The WAITMONITORINGTIME is expressed as a number of GPMC_CLK clock cycles,
even though the access is defined as asynchronous, and no GPMC_CLK clock
is provided to the external device. Still, GPMCFCLKDIVIDER is used as a divider
for the GPMC clock, so it must be programmed to define the
correct WAITMONITORINGTIME delay.

This patch correctly computes WAITMONITORINGTIME in GPMC_CLK cycles instead of 
GPMC_FCLK cycles,
both during programming (gpmc_cs_set_timings) and during retrieval 
(gpmc_cs_show_timings).

Signed-off-by: Robert ABEL 
---
 drivers/memory/omap-gpmc.c | 125 +++--
 1 file changed, 99 insertions(+), 26 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 9cf8d21..6591991 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -170,6 +170,11 @@
  */
 #defineGPMC_NR_IRQ 2
 
+enum gpmc_clk_domain {
+   GPMC_CD_FCLK,
+   GPMC_CD_CLK
+};
+
 struct gpmc_cs_data {
const char *name;
 
@@ -268,16 +273,55 @@ static unsigned long gpmc_get_fclk_period(void)
return rate;
 }
 
-static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
+/**
+ * gpmc_get_clk_period - get period of selected clock domain in ps
+ * @cs Chip Select Region.
+ * @cd Clock Domain.
+ *
+ * GPMC_CS_CONFIG1 GPMCFCLKDIVIDER for cs has to be setup
+ * prior to calling this function with GPMC_CD_CLK.
+ * 
+ */
+static unsigned long gpmc_get_clk_period(int cs, enum gpmc_clk_domain cd)
+{
+
+   unsigned long tick_ps = gpmc_get_fclk_period();
+   u32 l;
+   int div;
+
+   switch (cd) {
+   case GPMC_CD_CLK:
+   /* get current clk divider */
+   l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
+   div = (l & 0x03) + 1;
+   /* get GPMC_CLK period */
+   tick_ps *= div;
+   break;
+   case GPMC_CD_FCLK:
+   /* FALL-THROUGH */
+   default:
+   break;
+   }
+
+   return tick_ps;
+
+}
+
+static unsigned int gpmc_ns_to_clk_ticks(unsigned int time_ns, int cs, enum 
gpmc_clk_domain cd)
 {
unsigned long tick_ps;
 
/* Calculate in picosecs to yield more exact results */
-   tick_ps = gpmc_get_fclk_period();
+   tick_ps = gpmc_get_clk_period(cs, cd);
 
return (time_ns * 1000 + tick_ps - 1) / tick_ps;
 }
 
+static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
+{
+   return gpmc_ns_to_clk_ticks(time_ns, /* any CS */ 0, GPMC_CD_FCLK);
+}
+
 static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
 {
unsigned long tick_ps;
@@ -288,9 +332,14 @@ static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
return (time_ps + tick_ps - 1) / tick_ps;
 }
 
+unsigned int gpmc_clk_ticks_to_ns(unsigned ticks, int cs, enum gpmc_clk_domain 
cd)
+{
+   return ticks * gpmc_get_clk_period(cs, cd) / 1000;
+}
+
 unsigned int gpmc_ticks_to_ns(unsigned int ticks)
 {
-   return ticks * gpmc_get_fclk_period() / 1000;
+   return gpmc_clk_ticks_to_ns(ticks, /* any CS */ 0, GPMC_CD_FCLK);
 }
 
 static unsigned int gpmc_ticks_to_ps(unsigned int ticks)
@@ -346,16 +395,22 @@ static void gpmc_cs_bool_timings(int cs, const struct 
gpmc_bool_timings *p)
  * @st_bit  Start Bit
  * @end_bit End Bit. Must be >= @st_bit.
  * @nameDTS node name, w/o "gpmc,"
+ * @cd  Clock Domain of timing parameter.
+ * @shift   Parameter value left shifts @shift, which is then printed instead 
of value.
  * @raw Raw Format Option.
  *  raw format:  gpmc,name = 
  *  tick format: gpmc,name =  /‍* x ticks *‍/
  * @noval   Parameter values equal to 0 are not printed.
- * @shift   Parameter value left shifts @shift, which is then printed instead 
of value.
  *
  */
-static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
-  bool raw, bool noval, int shift,
-  const char *name)
+static int get_gpmc_timing_reg(
+   /* timing specifiers */
+   int cs, int reg, int st_bit, int end_bit,
+   const char *name, const enum gpmc_clk_domain cd,
+   /* value transform */
+   int shift,
+   /* format specifiers */
+   bool raw, bool noval)
 {
u32 l;
int nr_bits;
@@ -373,7 +428,7 @@ static int get_gpmc_timing_reg(int cs, int reg, int st_bit, 
int end_bit,
/* DTS tick format for timings in ns */
unsigned int time_ns;
 
-   time_ns = gpmc_ticks_to_ns(l);
+   time_ns = gpmc_clk_ticks_to_ns(l, cs, cd);
pr_info("gpmc,%s = <%u> /* %i ticks */\n",
name, time_ns, l);
} else {
@@ -388,13 +443,15 @@ static int get_gpmc_timing_reg(int cs, int reg, int 
st_bit, int end_bit,
pr_info("cs%i %s: 0x%08x\n", cs, #config, \
gpmc_cs_read_reg(cs, config))
 #define GPMC_GET_RAW(reg, st, end, field) \

[PATCH 7/8 v2] ARM OMAP2+ GPMC: calculate GPMCFCLKDIVIDER based on WAITMONITORINGTIME

[Robert ABEL]

The WAITMONITORINGTIME is expressed as a number of GPMC_CLK clock cycles,
even though the access is defined as asynchronous, and no GPMC_CLK clock
is provided to the external device. Still, GPMCFCLKDIVIDER is used as a divider
for the GPMC clock, so it must be programmed to define the
correct WAITMONITORINGTIME delay.

Calculate GPMCFCLKDIVIDER independent of gpmc,sync-clk-ps in DT for
truly asynchronous accesses, i.e. both read and write asynchronous.

Signed-off-by: Robert ABEL 
---
 arch/arm/mach-omap2/gpmc-nand.c| 17 -
 arch/arm/mach-omap2/gpmc-onenand.c |  4 +--
 drivers/memory/omap-gpmc.c | 74 ++
 include/linux/omap-gpmc.h  |  2 +-
 4 files changed, 80 insertions(+), 17 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
index d5951b1..e863a59 100644
--- a/arch/arm/mach-omap2/gpmc-nand.c
+++ b/arch/arm/mach-omap2/gpmc-nand.c
@@ -96,14 +96,6 @@ int gpmc_nand_init(struct omap_nand_platform_data 
*gpmc_nand_data,
gpmc_nand_res[1].start = gpmc_get_client_irq(GPMC_IRQ_FIFOEVENTENABLE);
gpmc_nand_res[2].start = gpmc_get_client_irq(GPMC_IRQ_COUNT_EVENT);
 
-   if (gpmc_t) {
-   err = gpmc_cs_set_timings(gpmc_nand_data->cs, gpmc_t);
-   if (err < 0) {
-   pr_err("omap2-gpmc: Unable to set gpmc timings: %d\n", 
err);
-   return err;
-   }
-   }
-
memset(&s, 0, sizeof(struct gpmc_settings));
if (gpmc_nand_data->of_node)
gpmc_read_settings_dt(gpmc_nand_data->of_node, &s);
@@ -111,6 +103,15 @@ int gpmc_nand_init(struct omap_nand_platform_data 
*gpmc_nand_data,
gpmc_set_legacy(gpmc_nand_data, &s);
 
s.device_nand = true;
+
+   if (gpmc_t) {
+   err = gpmc_cs_set_timings(gpmc_nand_data->cs, gpmc_t, &s);
+   if (err < 0) {
+   pr_err("omap2-gpmc: Unable to set gpmc timings: %d\n", 
err);
+   return err;
+   }
+   }
+
err = gpmc_cs_program_settings(gpmc_nand_data->cs, &s);
if (err < 0)
goto out_free_cs;
diff --git a/arch/arm/mach-omap2/gpmc-onenand.c 
b/arch/arm/mach-omap2/gpmc-onenand.c
index 53d197e..f899e77 100644
--- a/arch/arm/mach-omap2/gpmc-onenand.c
+++ b/arch/arm/mach-omap2/gpmc-onenand.c
@@ -293,7 +293,7 @@ static int omap2_onenand_setup_async(void __iomem 
*onenand_base)
if (ret < 0)
return ret;
 
-   ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t);
+   ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t, &onenand_async);
if (ret < 0)
return ret;
 
@@ -331,7 +331,7 @@ static int omap2_onenand_setup_sync(void __iomem 
*onenand_base, int *freq_ptr)
if (ret < 0)
return ret;
 
-   ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t);
+   ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t, &onenand_sync);
if (ret < 0)
return ret;
 
diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index a6abaf0..9cf8d21 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -139,6 +139,8 @@
 #define GPMC_CONFIG1_WAIT_READ_MON  (1 << 22)
 #define GPMC_CONFIG1_WAIT_WRITE_MON (1 << 21)
 #define GPMC_CONFIG1_WAIT_MON_IIME(val) ((val & 3) << 18)
+/** WAITMONITORINGTIME Max Ticks */
+#define GPMC_CONFIG1_WAIT_MON_TIME_MAX  2
 #define GPMC_CONFIG1_WAIT_PIN_SEL(val)  ((val & 3) << 16)
 #define GPMC_CONFIG1_DEVICESIZE(val)((val & 3) << 12)
 #define GPMC_CONFIG1_DEVICESIZE_16  GPMC_CONFIG1_DEVICESIZE(1)
@@ -511,13 +513,41 @@ static int set_gpmc_timing_reg(int cs, int reg, int 
st_bit, int end_bit,
t->field, #field) < 0)  \
return -1
 
+/**
+ * gpmc_calc_waitmonitoring_divider - calculate proper GPMCFCLKDIVIDER based 
on WAITMONITORINGTIME
+ * @wait_monitoring WAITMONITORINGTIME in ns.
+ * @return  -1 on failure to scale, else proper divider > 0.
+ * @noteWAITMONITORINGTIME will be _at least_ as long as desired.
+ *  i.e. read timings should be kept-> don't 
sample bus too early
+ *  while write timings should work out as well -> data is 
longer on bus
+ */
+static int gpmc_calc_waitmonitoring_divider(unsigned int wait_monitoring)
+{
+
+   int div = gpmc_ns_to_ticks(wait_monitoring);
+
+   div += GPMC_CONFIG1_WAIT_MON_TIME_MAX - 1;
+   div /= GPMC_CONFIG1_WAIT_MON_TIME_MAX;
+
+   if (div > 4)
+   return -1;
+   if (div <= 0)
+   div = 1;
+
+   return div;
+
+}
+
+/**
+ * gpmc_calc_divider - calculate GPMC_FCLK divider for sync_clk GPMC_CLK 
period.
+ *

[PATCH 9/8 v2] ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters

[Robert ABEL]

GPMC_CONFIG1_i parameters CLKACTIVATIONTIME and WAITMONITORINGTIME
have reserved values.
Raise an error if calculated timings try to program reserved values.

GPMC_CONFIG1_i ATTCHEDDEVICEPAGELENGTH and DEVICESIZE were already checked
when parsing the DT.

Explicitly comment invalid values on gpmc_cs_show_timings for
-CLKACTIVATIONTIME
-WAITMONITORINGTIME
-DEVICESIZE
-ATTACHEDDEVICEPAGELENGTH

Signed-off-by: Robert ABEL 
---
 drivers/memory/omap-gpmc.c | 69 ++
 1 file changed, 46 insertions(+), 23 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 6591991..cc2e6d0 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -135,6 +135,8 @@
 #define GPMC_CONFIG1_WRITETYPE_ASYNC(0 << 27)
 #define GPMC_CONFIG1_WRITETYPE_SYNC (1 << 27)
 #define GPMC_CONFIG1_CLKACTIVATIONTIME(val) ((val & 3) << 25)
+/** CLKACTIVATIONTIME Max Ticks */
+#define GPMC_CONFIG1_CLKACTIVATIONTIME_MAX 2
 #define GPMC_CONFIG1_PAGE_LEN(val)  ((val & 3) << 23)
 #define GPMC_CONFIG1_WAIT_READ_MON  (1 << 22)
 #define GPMC_CONFIG1_WAIT_WRITE_MON (1 << 21)
@@ -144,6 +146,8 @@
 #define GPMC_CONFIG1_WAIT_PIN_SEL(val)  ((val & 3) << 16)
 #define GPMC_CONFIG1_DEVICESIZE(val)((val & 3) << 12)
 #define GPMC_CONFIG1_DEVICESIZE_16  GPMC_CONFIG1_DEVICESIZE(1)
+/** DEVICESIZE Max Value */
+#define GPMC_CONFIG1_DEVICESIZE_MAX GPMC_CONFIG1_DEVICESIZE_16
 #define GPMC_CONFIG1_DEVICETYPE(val)((val & 3) << 10)
 #define GPMC_CONFIG1_DEVICETYPE_NOR GPMC_CONFIG1_DEVICETYPE(0)
 #define GPMC_CONFIG1_MUXTYPE(val)   ((val & 3) << 8)
@@ -394,18 +398,21 @@ static void gpmc_cs_bool_timings(int cs, const struct 
gpmc_bool_timings *p)
  * @reg GPMC_CS_CONFIGn register offset.
  * @st_bit  Start Bit
  * @end_bit End Bit. Must be >= @st_bit.
+ * @max Maximum parameter value (after optional @shift).
+ *  If 0, maximum is as high as @st_bit and @end_bit allow.
  * @nameDTS node name, w/o "gpmc,"
  * @cd  Clock Domain of timing parameter.
  * @shift   Parameter value left shifts @shift, which is then printed instead 
of value.
  * @raw Raw Format Option.
  *  raw format:  gpmc,name = 
  *  tick format: gpmc,name =  /‍* x ticks *‍/
+ *  When @max is exceeded, "invalid" is printed inside comment.
  * @noval   Parameter values equal to 0 are not printed.
  *
  */
 static int get_gpmc_timing_reg(
/* timing specifiers */
-   int cs, int reg, int st_bit, int end_bit,
+   int cs, int reg, int st_bit, int end_bit, int max,
const char *name, const enum gpmc_clk_domain cd,
/* value transform */
int shift,
@@ -415,11 +422,15 @@ static int get_gpmc_timing_reg(
u32 l;
int nr_bits;
int mask;
+   bool invalid;
 
l = gpmc_cs_read_reg(cs, reg);
nr_bits = end_bit - st_bit + 1;
mask = (1 << nr_bits) - 1;;
l = (l >> st_bit) & mask;
+   if (!max)
+   max = mask;
+   invalid = l > max;
if (shift)
l = (shift << l);
if (noval && (l == 0))
@@ -429,11 +440,11 @@ static int get_gpmc_timing_reg(
unsigned int time_ns;
 
time_ns = gpmc_clk_ticks_to_ns(l, cs, cd);
-   pr_info("gpmc,%s = <%u> /* %i ticks */\n",
-   name, time_ns, l);
+   pr_info("gpmc,%s = <%u> /* %i ticks %s*/\n",
+   name, time_ns, l, invalid ? "; invalid " : "");
} else {
/* raw format */
-   pr_info("gpmc,%s = <%u>\n", name, l);
+   pr_info("gpmc,%s = <%u>%s\n", name, l, invalid ? " /* invalid 
*/" : "");
}
 
return l;
@@ -443,15 +454,19 @@ static int get_gpmc_timing_reg(
pr_info("cs%i %s: 0x%08x\n", cs, #config, \
gpmc_cs_read_reg(cs, config))
 #define GPMC_GET_RAW(reg, st, end, field) \
-   get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 1, 
0)
+   get_gpmc_timing_reg(cs, (reg), (st), (end), 0, field, GPMC_CD_FCLK, 0, 
1, 0)
+#define GPMC_GET_RAW_MAX(reg, st, end, max, field) \
+   get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, GPMC_CD_FCLK, 
0, 1, 0)
 #define GPMC_GET_RAW_BOOL(reg, st, end, field) \
-   get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 1, 
1)
-#define GPMC_GET_RAW_SHIFT(reg, st, end, shift, field) \
-   get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 
(shift), 1, 1)
+   get_gpmc_timing_reg(cs, (reg), (st), (end), 0, field, GPMC_CD_FCLK, 0, 
1, 1)
+#define GPMC_GET_RAW_SHIFT(reg, st, end, shift, max, field) \
+   get_gpmc_timing_reg(cs, (reg), (st), (end

[PATCH 4/8 v2] ARM OMAP2+ GPMC: fix debug output alignment

[Robert ABEL]

GPMC debug output is aligned to 10 characters for field names.
However, some fields have bigger names, screwing up the alignment.
Consequently, alignment was changed to longest field name (17 chars) for now.

Signed-off-by: Robert ABEL 
---
 drivers/memory/omap-gpmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 78b78a64..b5c8305 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -482,7 +482,7 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, 
int end_bit,
l = gpmc_cs_read_reg(cs, reg);
 #ifdef DEBUG
printk(KERN_INFO
-   "GPMC CS%d: %-10s: %3d ticks, %3lu ns (was %3i ticks) %3d ns\n",
+   "GPMC CS%d: %-17s: %3d ticks, %3lu ns (was %3i ticks) %3d ns\n",
   cs, name, ticks, gpmc_get_fclk_period() * ticks / 1000,
(l >> st_bit) & mask, time);
 #endif
-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/8 v2] ARM OMAP2+ GPMC: don't undef DEBUG

[Robert ABEL]

OMAP2+ GPMC driver undefines DEBUG, which makes it unnecessarily
hard to turn DEBUG on. Remove the offending lines.

Signed-off-by: Robert ABEL 
---
 drivers/memory/omap-gpmc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 24696f5..5cabac8 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -12,8 +12,6 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
-#undef DEBUG
-
 #include 
 #include 
 #include 
-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/8 v2] ARM OMAP2+ GPMC: fixes and bus children

[Robert Abel]

On Tue, Feb 24, 2015 at 9:05 PM, Robert ABEL
 wrote:
>
> These are the changes I proposed in two separate patchsets
> #([1], [2]) rebased to 3.19 as well as new changes for little bugs
> I noticed while preparing this patchset.

It seems my m4d s3d sk177z failed me. Disregard the missing Patch 2,
the numbering is off, but the contents are correct.

Regards,

Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/8 v2] ARM OMAP2+ GPMC: add bus children

[Robert Abel]

Hi Roger,

On 25 Feb 2015 13:02, Roger Quadros wrote:
This creates platform devices for the children of child, but what 
about platform device for the child itself?
It seems my first try in the other patch set wasn't so wrong after all. 
Maybe unconditionally call


of_platform_device_create(child,...) and
of_platform_populate(child, ...)?


Shouldn't the bus driver for that bus be responsible for spawning children of 
its bus?

Well, a bus driver doesn't actually do much work here. The standard buses are 
just there for offset and so on.
And that's my use case. Have one CS region with fixed settings and multiple 
devices in it.

Creating code that in effect replicates what simple-bus does isn't worthwhile. 
Basically, it would replicate half the code that
of_platform_populate goes through anyway without good reason.

If complex bus-behavior is needed -- more than single-bus offset shifts --, a 
complex bus driver can always be written.
Since it wouldn't match of_default_match_table, it itself would be responsible 
for creating children.

I think I'll go with my first patch for this one, maybe call 
of_platform_decide_create unconditionally to check whether buses are
available.

Regards,

Robert

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 9/8 v2] ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters

[Robert Abel]

Hi Roger,

On 25 Feb 2015 11:44, Roger Quadros wrote:
typo ATTCHEDDEVICEPAGELENGTH->ATTACHEDDEVICEPAGELENGTH 

Yep.

+/** DEVICESIZE Max Value */
+#define GPMC_CONFIG1_DEVICESIZE_MAX GPMC_CONFIG1_DEVICESIZE_16

Shouldn't this be 1 instead? I'm hoping max value is without the shift
based on GPMC_CONFIG1_CLKACTIVATIONTIME_MAX.

Yes, it should. It's a remnant from the change below...

+ * @max Maximum parameter value (after optional @shift).

max should be absolute value, without the shift.

Yes, forgot to change that.

+#define GPMC_GET_RAW_SHIFT(reg, st, end, shift, max, field) \
+   get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, GPMC_CD_FCLK, 
(shift), 1, 1)

Is it better to rename this to GPMC_GET_RAW_SHIFT_MAX() as it takes the max 
parameter.

Ok.

+   GPMC_GET_RAW_SHIFT(GPMC_CS_CONFIG1, 23, 24, 4, 2, "burst-length");

want to define GPMC_CONFIG1_BURSTLENGTH_MAX?
Yes, for consistency. Originally, i was not going to create defines for 
limits used only in one place, as opposed to WAITMONITORINGTIME and 
CLKACTIVATIONTIME.

-   GPMC_GET_TICKS_CD(GPMC_CS_CONFIG1, 18, 19, "wait-monitoring-ns", 
GPMC_CD_CLK);
-   GPMC_GET_TICKS(GPMC_CS_CONFIG1, 25, 26, "clk-activation-ns");
+   GPMC_GET_TICKS_CD_MAX(GPMC_CS_CONFIG1, 18, 19, GPMC_CONFIG1_WAIT_MON_TIME_MAX, 
"wait-monitoring-ns", GPMC_CD_CLK);

use GPMC_CONFIG1_WAITMONTIME_MAX to have consistent naming.
I used the naming the other define had. Saw no point in changing them. 
They go mostly unused anyway.

Might as well change it.


+#define GPMC_SET_ONE_CD(reg, st, end, field, cd) \
+   GPMC_SET_ONE_CD_MAX(reg, st, end, 0, field, GPMC_CD_FCLK)

last parameter should be (cd) instead of GPMC_CD_FCLK.
Ah, copy-paste, there you are again. Luckily this define goes unused, 
because all GPMC_SET_ONE_CD became GPMC_SET_ONE_CD_MAX anyway.

So I'll delete that. Quite a keen eye, though ;)

Regards,

Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/8 v2] ARM OMAP2+ GPMC: add bus children

[Robert Abel]

Hi Roger,

On 25 Feb 2015 17:18, Roger Quadros wrote:

OK. Would be interesting to see how unconditional call to 
of_platform_decide_create() behaves
for your case.
I'm not able to test today, so results will be in tomorrow. If that 
doesn't work, I'll just resubmit my first patch with the conditional.


I'll be posting a v3 of this entire patch series tomorrow after testing. 
I think I should leave your ACKs out for you to re-ACK?


Regards,

Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 5/8 v2] ARM OMAP2+ GPMC: change get_gpmc_timing_reg output for DTS

[Robert Abel]

Hi Tony,

On 25 Feb 2015 16:23, Tony Lindgren wrote:

* Roger Quadros  [150225 05:28]:

On 24/02/15 22:05, Robert ABEL wrote:

DTS output was formatted to require additional work when copy-pasting into DTS.
Nano-second timings were removed, because they were not a confidence interval 
nor
an indication what timing values would result in the same #ticks

If they were not is it possible to dump min,max timings that will result in
the same ticks?

Yeah my plan was o display the nanosecond range resulting in the
same tick value. That makes it a bit easier to compare the timings
to the data sheet. So maybe show both tick and ns range?

Ah, so my hunch was right. I reimplemented that correctly now.
Basically, your code just went [ticks-1, ticks+1] in ns. But times are 
always ceiling(ns / gpmc_[f]clk), i.e. (ticks - 1, ticks] in ns.


Some other patches change as well because of this change, so a v3 is in 
order =].


Regards,

Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 8/8 v2] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

[Robert Abel]

Hi Roger,

On 25 Feb 2015 17:58, Roger Quadros wrote:

static unsigned int gpmc_ticks_to_ps(unsigned int ticks)
@@ -346,16 +395,22 @@ static void gpmc_cs_bool_timings(int cs, const struct 
gpmc_bool_timings *p)
   * @st_bit  Start Bit
   * @end_bit End Bit. Must be >= @st_bit.
   * @nameDTS node name, w/o "gpmc,"
+ * @cd  Clock Domain of timing parameter.
+ * @shift   Parameter value left shifts @shift, which is then printed instead 
of value.
   * @raw Raw Format Option.
   *  raw format:  gpmc,name = 
   *  tick format: gpmc,name =  /‍* x ticks *‍/
   * @noval   Parameter values equal to 0 are not printed.
- * @shift   Parameter value left shifts @shift, which is then printed instead 
of value.
   *
   */
-static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
-  bool raw, bool noval, int shift,
-  const char *name)
+static int get_gpmc_timing_reg(
+   /* timing specifiers */
+   int cs, int reg, int st_bit, int end_bit,
+   const char *name, const enum gpmc_clk_domain cd,
+   /* value transform */
+   int shift,
+   /* format specifiers */
+   bool raw, bool noval)

now that you are rearranging the parameters, "name" parameter should probably be
at the same position (or last) in get_gpmc_timing_reg() and 
set_gpmc_timing_reg()?
Also clock domain (cd) position could be matched if possible.
I rearranged them primarily, because I wanted to group the specifiers 
according to function, because I found it unnatural to add clock domain 
to the end, when it's "more important" than the format specifiers.
set_gpmc_timing_reg are fine in that regard as it doesn't have format 
specifiers.

+/**
+ * set_gpmc_timing_reg - set a single timing parameter for Chip Select Region.
+ * @cs  Chip Select Region.
+ * @reg GPMC_CS_CONFIGn register offset.
+ * @st_bit  Start Bit
+ * @end_bit End Bit. Must be >= @st_bit.
+ * @timeTiming parameter in ns.
+ * @cd  Timing parameter clock domain.
+ * @nameTiming parameter name.
+ * @noteCaller is expected to have initialized CONFIG1 GPMCFCLKDIVIDER

@note is not a parameter.
Well no, note's a note. This is a doxygen-style comment, so tools should 
put a note in the created documentation. Doxygen will put a box with 
yellow background, for instance.

-   pr_err("%s: GPMC error! CS%d: %s: %d ns, %d ticks > %d\n",
+   pr_err("%s: GPMC CS%d: %s %d ns, %d ticks > %d ticks\n",

any reason for removing the "error!" string?
It's already pr_err, the "error!" in-between "GPMC CS%d" made it hard to 
read and there's a WARN after that statement in all cases, because a 
child _must_ fail if a timing parameter constraint is broken.


Regards,

Robert

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 7/8 v2] ARM OMAP2+ GPMC: calculate GPMCFCLKDIVIDER based on WAITMONITORINGTIME

[Robert Abel]

Hi Roger,

On 25 Feb 2015 17:33, Roger Quadros wrote:

./scripts/checkpatch.pl detects some styling errors.
Well, there's like a million lines over 80 characters. I'm also a 
heathen and don't use an 80 character terminal either.
I'll fix the more serious issues checkpatch finds, but some styling 
errors are even from code already in the driver.



  #define GPMC_CONFIG1_WAIT_MON_IIME(val) ((val & 3) << 18)

Not caused by your patch but we can fix the typo

GPMC_CONFIG1_WAIT_MON_IIME -> GPMC_CONFIG1_WAIT_MON_TIME

I'd rather remove than fix. None of these defines is in active use anymore.

+/**
+ * gpmc_calc_waitmonitoring_divider - calculate proper GPMCFCLKDIVIDER based 
on WAITMONITORINGTIME
+ * @wait_monitoring WAITMONITORINGTIME in ns.
+ * @return  -1 on failure to scale, else proper divider > 0.
+ * @noteWAITMONITORINGTIME will be _at least_ as long as desired.
+ *  i.e. read timings should be kept-> don't 
sample bus too early
+ *  while write timings should work out as well -> data is 
longer on bus
+ */
+static int gpmc_calc_waitmonitoring_divider(unsigned int wait_monitoring)
+{
+
+   int div = gpmc_ns_to_ticks(wait_monitoring);
+
+   div += GPMC_CONFIG1_WAIT_MON_TIME_MAX - 1;
+   div /= GPMC_CONFIG1_WAIT_MON_TIME_MAX;

Sorry I didn't understand how this works. :P
 From the TRM,

waitmonitoringtime_ns = waitmonitoring_ticks x (gpmc_clk_div + 1) x gpmc_fclk_ns

Using that formula:
gpmc_clk_div + 1 = ceil(ceil(waitmonitoringtime_ns / gpmc_fclk_ns) / 
waitmonitoring_ticks).


The reason I use GPMC_CONFIG1_WAIT_MON_TIME_MAX directly, is because 
every other pair is identical:


We have the following cases:
a) WAITMONITORINGTIME = 0, so div doesn't matter (we set div to 1) or
b) WAITMONITORINGTIME = 1, with div = 1 or
c) WAITMONITORINGTIME = 2, with div = n

WAITMONITORINGTIME is never 1 with div = n and div /= 1, because that's 
the same as WAITMONITORINGTIME = 2 with div = n - 1.

Cases a) and b) are caught using the fact that div = 0 after the division.
Case c) is handled by assuming that WAITMONITORINGTIME will always be 2 
(i.e. GPMC_CONFIG1_WAIT_MON_TIME_MAX) for all divs /= 0 at that point.


Took me a while to realize that this works, too.


+   /*
+* See if we need to change the divider for waitmonitoringtime.
+*
+* If DT contains sync-clk-ps for truly asynchronous accesses,
+* ignore it as long as waitmonitoringtime is used.
+*

The comment in the $subject was more easier to understand for me than the above

"Calculate GPMCFCLKDIVIDER independent of gpmc,sync-clk-ps in DT for
truly asynchronous accesses, i.e. both read and write asynchronous."
Well, the comment in $subject was a general description. Here, we don't 
touch div when

a) any access is synchronous
b) all accesses are asynchronous, but WAITMONITORINGTIME is not used, 
i.e. WAITREADMONITORING and WAITWRITEMONITORING are both not set.


So, if WAITMONITORINGTIME is not used, or any access is synchronous, we 
program the sync-clk-ps anyways, because the GPMC won't rely on it for 
any timing.

So the comment is OK, IMHO.

Regards,

Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 7/8 v2] ARM OMAP2+ GPMC: calculate GPMCFCLKDIVIDER based on WAITMONITORINGTIME

[Robert Abel]

On 25 Feb 2015 18:20, Roger Quadros wrote:

Need to patch mach-omap2/usb-tusb6010.c as well. else we get

arch/arm/mach-omap2/usb-tusb6010.c: In function ‘tusb_set_async_mode’:
arch/arm/mach-omap2/usb-tusb6010.c:74:2: error: too few arguments to function 
‘gpmc_cs_set_timings’
In file included from arch/arm/mach-omap2/gpmc.h:14:0,
  from arch/arm/mach-omap2/usb-tusb6010.c:23:
include/linux/omap-gpmc.h:166:12: note: declared here
arch/arm/mach-omap2/usb-tusb6010.c: In function ‘tusb_set_sync_mode’:
arch/arm/mach-omap2/usb-tusb6010.c:101:2: error: too few arguments to function 
‘gpmc_cs_set_timings’
In file included from arch/arm/mach-omap2/gpmc.h:14:0,
  from arch/arm/mach-omap2/usb-tusb6010.c:23:
include/linux/omap-gpmc.h:166:12: note: declared here

Argh... Another one of those exported function calls :|

Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 8/8 v2] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

[Robert Abel]

Hi Roger,

On 25 Feb 2015 18:17, Roger Quadros wrote:

How will the user know by looking at the kernel log that it was really an error?
We don't fail probe if set_gpmc_timing_reg() fails so I feel it is necessary to
clearly show an Error message.

You can probably reword it like "%s: Error!! GPMC CS %d..."


I'll put "error" in there. But just for the record, it's this messaged 
followed by a WARN that explains "failed to add child %s".

So I'd expect the user to put A and B together ;)

Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 8/8] ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters

[Robert ABEL]

GPMC_CONFIG1_i parameters CLKACTIVATIONTIME and WAITMONITORINGTIME
have reserved values.
Raise an error if calculated timings try to program reserved values.

GPMC_CONFIG1_i ATTACHEDDEVICEPAGELENGTH and DEVICESIZE were already checked
when parsing the DT.

Explicitly comment invalid values on gpmc_cs_show_timings for
-CLKACTIVATIONTIME
-WAITMONITORINGTIME
-DEVICESIZE
-ATTACHEDDEVICEPAGELENGTH

Signed-off-by: Robert ABEL 
---
 drivers/memory/omap-gpmc.c | 68 ++
 1 file changed, 45 insertions(+), 23 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 400b0a6..7e5300d 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -135,7 +135,11 @@
 #define GPMC_CONFIG1_WRITETYPE_ASYNC(0 << 27)
 #define GPMC_CONFIG1_WRITETYPE_SYNC (1 << 27)
 #define GPMC_CONFIG1_CLKACTIVATIONTIME(val) ((val & 3) << 25)
+/** CLKACTIVATIONTIME Max Ticks */
+#define GPMC_CONFIG1_CLKACTIVATIONTIME_MAX 2
 #define GPMC_CONFIG1_PAGE_LEN(val)  ((val & 3) << 23)
+/** ATTACHEDDEVICEPAGELENGTH Max Value */
+#define GPMC_CONFIG1_ATTACHEDDEVICEPAGELENGTH_MAX 2
 #define GPMC_CONFIG1_WAIT_READ_MON  (1 << 22)
 #define GPMC_CONFIG1_WAIT_WRITE_MON (1 << 21)
 #define GPMC_CONFIG1_WAIT_MON_TIME(val) ((val & 3) << 18)
@@ -144,6 +148,8 @@
 #define GPMC_CONFIG1_WAIT_PIN_SEL(val)  ((val & 3) << 16)
 #define GPMC_CONFIG1_DEVICESIZE(val)((val & 3) << 12)
 #define GPMC_CONFIG1_DEVICESIZE_16  GPMC_CONFIG1_DEVICESIZE(1)
+/** DEVICESIZE Max Value */
+#define GPMC_CONFIG1_DEVICESIZE_MAX 1
 #define GPMC_CONFIG1_DEVICETYPE(val)((val & 3) << 10)
 #define GPMC_CONFIG1_DEVICETYPE_NOR GPMC_CONFIG1_DEVICETYPE(0)
 #define GPMC_CONFIG1_MUXTYPE(val)   ((val & 3) << 8)
@@ -393,6 +399,8 @@ static void gpmc_cs_bool_timings(int cs, const struct 
gpmc_bool_timings *p)
  * @reg GPMC_CS_CONFIGn register offset.
  * @st_bit  Start Bit
  * @end_bit End Bit. Must be >= @st_bit.
+ * @max Maximum parameter value (before optional @shift).
+ *  If 0, maximum is as high as @st_bit and @end_bit allow.
  * @nameDTS node name, w/o "gpmc,"
  * @cd  Clock Domain of timing parameter.
  * @shift   Parameter value left shifts @shift, which is then printed instead 
of value.
@@ -401,12 +409,13 @@ static void gpmc_cs_bool_timings(int cs, const struct 
gpmc_bool_timings *p)
  *  tick format: gpmc,name =  /‍*(x ns -- y ns]; x ticks 
*‍/
  *  Where (x ns -- y ns] is the half-open interval from x ns to y ns 
that
  *  result in the same tick value.
+ *  When @max is exceeded, "invalid" is printed inside comment.
  * @noval   Parameter values equal to 0 are not printed.
  *
  */
 static int get_gpmc_timing_reg(
/* timing specifiers */
-   int cs, int reg, int st_bit, int end_bit,
+   int cs, int reg, int st_bit, int end_bit, int max,
const char *name, const enum gpmc_clk_domain cd,
/* value transform */
int shift,
@@ -416,11 +425,15 @@ static int get_gpmc_timing_reg(
u32 l;
int nr_bits;
int mask;
+   bool invalid;
 
l = gpmc_cs_read_reg(cs, reg);
nr_bits = end_bit - st_bit + 1;
mask = (1 << nr_bits) - 1;
l = (l >> st_bit) & mask;
+   if (!max)
+   max = mask;
+   invalid = l > max;
if (shift)
l = (shift << l);
if (noval && (l == 0))
@@ -432,11 +445,11 @@ static int get_gpmc_timing_reg(
 
time_ns_min = gpmc_clk_ticks_to_ns(l ? l - 1 : 0, cs, cd);
time_ns = gpmc_clk_ticks_to_ns(l, cs, cd);
-   pr_info("gpmc,%s = <%u> /* (%u ns - %u ns]; %i ticks */\n",
-   name, time_ns, time_ns_min, time_ns, l);
+   pr_info("gpmc,%s = <%u> /* (%u ns - %u ns]; %i ticks %s*/\n",
+   name, time_ns, time_ns_min, time_ns, l, invalid ? "; 
invalid " : "");
} else {
/* raw format */
-   pr_info("gpmc,%s = <%u>\n", name, l);
+   pr_info("gpmc,%s = <%u>%s\n", name, l, invalid ? " /* invalid 
*/" : "");
}
 
return l;
@@ -446,15 +459,19 @@ static int get_gpmc_timing_reg(
pr_info("cs%i %s: 0x%08x\n", cs, #config, \
gpmc_cs_read_reg(cs, config))
 #define GPMC_GET_RAW(reg, st, end, field) \
-   get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 1, 
0)
+   get_gpmc_timing_reg(cs, (reg), (st), (end), 0, field, GPMC_CD_FCLK, 0, 
1, 0)
+#define GPMC_GET_RAW_MAX(reg, st, end, max, field) \
+   get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, GPMC_CD_FCLK, 
0, 1, 0)
 #define GPMC_GET_RAW_BOOL(reg, st, end, field

[PATCH 7/8] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

[Robert ABEL]

The WAITMONITORINGTIME is expressed as a number of GPMC_CLK clock cycles,
even though the access is defined as asynchronous, and no GPMC_CLK clock
is provided to the external device. Still, GPMCFCLKDIVIDER is used as a divider
for the GPMC clock, so it must be programmed to define the
correct WAITMONITORINGTIME delay.

This patch correctly computes WAITMONITORINGTIME in GPMC_CLK cycles instead of 
GPMC_FCLK cycles,
both during programming (gpmc_cs_set_timings) and during retrieval 
(gpmc_cs_show_timings).

Signed-off-by: Robert ABEL 
---
 drivers/memory/omap-gpmc.c | 126 +++--
 1 file changed, 99 insertions(+), 27 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index d71ea05..400b0a6 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -170,6 +170,11 @@
  */
 #defineGPMC_NR_IRQ 2
 
+enum gpmc_clk_domain {
+   GPMC_CD_FCLK,
+   GPMC_CD_CLK
+};
+
 struct gpmc_cs_data {
const char *name;
 
@@ -268,16 +273,54 @@ static unsigned long gpmc_get_fclk_period(void)
return rate;
 }
 
-static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
+/**
+ * gpmc_get_clk_period - get period of selected clock domain in ps
+ * @cs Chip Select Region.
+ * @cd Clock Domain.
+ *
+ * GPMC_CS_CONFIG1 GPMCFCLKDIVIDER for cs has to be setup
+ * prior to calling this function with GPMC_CD_CLK.
+ */
+static unsigned long gpmc_get_clk_period(int cs, enum gpmc_clk_domain cd)
+{
+
+   unsigned long tick_ps = gpmc_get_fclk_period();
+   u32 l;
+   int div;
+
+   switch (cd) {
+   case GPMC_CD_CLK:
+   /* get current clk divider */
+   l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
+   div = (l & 0x03) + 1;
+   /* get GPMC_CLK period */
+   tick_ps *= div;
+   break;
+   case GPMC_CD_FCLK:
+   /* FALL-THROUGH */
+   default:
+   break;
+   }
+
+   return tick_ps;
+
+}
+
+static unsigned int gpmc_ns_to_clk_ticks(unsigned int time_ns, int cs, enum 
gpmc_clk_domain cd)
 {
unsigned long tick_ps;
 
/* Calculate in picosecs to yield more exact results */
-   tick_ps = gpmc_get_fclk_period();
+   tick_ps = gpmc_get_clk_period(cs, cd);
 
return (time_ns * 1000 + tick_ps - 1) / tick_ps;
 }
 
+static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
+{
+   return gpmc_ns_to_clk_ticks(time_ns, /* any CS */ 0, GPMC_CD_FCLK);
+}
+
 static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
 {
unsigned long tick_ps;
@@ -288,9 +331,14 @@ static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
return (time_ps + tick_ps - 1) / tick_ps;
 }
 
+unsigned int gpmc_clk_ticks_to_ns(unsigned ticks, int cs, enum gpmc_clk_domain 
cd)
+{
+   return ticks * gpmc_get_clk_period(cs, cd) / 1000;
+}
+
 unsigned int gpmc_ticks_to_ns(unsigned int ticks)
 {
-   return ticks * gpmc_get_fclk_period() / 1000;
+   return gpmc_clk_ticks_to_ns(ticks, /* any CS */ 0, GPMC_CD_FCLK);
 }
 
 static unsigned int gpmc_ticks_to_ps(unsigned int ticks)
@@ -346,18 +394,24 @@ static void gpmc_cs_bool_timings(int cs, const struct 
gpmc_bool_timings *p)
  * @st_bit  Start Bit
  * @end_bit End Bit. Must be >= @st_bit.
  * @nameDTS node name, w/o "gpmc,"
+ * @cd  Clock Domain of timing parameter.
+ * @shift   Parameter value left shifts @shift, which is then printed instead 
of value.
  * @raw Raw Format Option.
  *  raw format:  gpmc,name = 
  *  tick format: gpmc,name =  /‍*(x ns -- y ns]; x ticks 
*‍/
  *  Where (x ns -- y ns] is the half-open interval from x ns to y ns 
that
  *  result in the same tick value.
  * @noval   Parameter values equal to 0 are not printed.
- * @shift   Parameter value left shifts @shift, which is then printed instead 
of value.
  *
  */
-static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
-  bool raw, bool noval, int shift,
-  const char *name)
+static int get_gpmc_timing_reg(
+   /* timing specifiers */
+   int cs, int reg, int st_bit, int end_bit,
+   const char *name, const enum gpmc_clk_domain cd,
+   /* value transform */
+   int shift,
+   /* format specifiers */
+   bool raw, bool noval)
 {
u32 l;
int nr_bits;
@@ -376,8 +430,8 @@ static int get_gpmc_timing_reg(int cs, int reg, int st_bit, 
int end_bit,
unsigned int time_ns;
unsigned int time_ns_min;
 
-   time_ns_min = gpmc_ticks_to_ns(l ? l - 1 : 0);
-   time_ns = gpmc_ticks_to_ns(l);
+   time_ns_min = gpmc_clk_ticks_to_ns(l ? l - 1 : 0, cs, cd);
+   time_ns = gpmc_clk_ticks_to_ns(l, cs, cd);
pr_info("gpmc,%s = <%u> /* (%u ns - %u ns]; %i ticks */\n",
name,

[PATCH 1/8] ARM OMAP2+ GPMC: don't undef DEBUG

[Robert ABEL]

OMAP2+ GPMC driver undefines DEBUG, which makes it unnecessarily
hard to turn DEBUG on. Remove the offending lines.

Signed-off-by: Robert ABEL 
---
 drivers/memory/omap-gpmc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 24696f5..5cabac8 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -12,8 +12,6 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
-#undef DEBUG
-
 #include 
 #include 
 #include 
-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/8] ARM OMAP2+ GPMC: fixes and bus children

[Robert ABEL]

These are the changes I proposed in three separate patchsets
#([1], [2], [3]) rebased to 3.19 as well as new changes for little bugs
I noticed while preparing this patchset.

1. DEBUG was undefined in source code --> remove offending lines
2. add capability to have busses as children of the GPMC and multiple
   devices on a bus. See [2] for an example DTS syntax.
3. debug output was unaligned --> align it
4. output for copy-pasting to DTS had erroneous timing outputs and
   made it hard to copy-paste --> correct timing values, add comments
   as DTS comments.
5. WAITMONITORINGTIME is expressed as GPMC_CLK cycles for all accesses.
   GPMCFCLKDIVIDER is used as a divider, so it must always be programmed.
6. GPMCFCLKDIVIDER is calculated according to WAITMONITORINGTIME for
   asynchronous accesses inside the driver --> asynchronous accesses now
   completely decoupled from gpmc,sync-clk-ps.
7. WAITMONITORINGTIME was being programmed/shown in GPMC_FCLK cycles instead
   of GPMC_CLK cycles --> add clock domain information where necessary.
8. Calculated values for WAITMONITORINGTIME and CLKACTIVATIONTIME that were
   outside the defined range would not raise an error.
   DEVICESIZE, ATTACHEDDEVICEPAGELENGTH, WAITMONITORINGTIME and
   CLKACTIVATIONTIME would not be marked as incorrect on DTS output.
   --> Fix all of these.

[1]: https://lkml.org/lkml/2015/2/12/495
[2]: https://lkml.org/lkml/2015/2/16/337
[3]: https://lkml.org/lkml/2015/2/24/609

Robert ABEL (9):
  ARM OMAP2+ GPMC: don't undef DEBUG
  ARM OMAP2+ GPMC: add bus children
  ARM OMAP2+ GPMC: fix debug output alignment
  ARM OMAP2+ GPMC: change get_gpmc_timing_reg output for DTS
  ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER
  ARM OMAP2+ GPMC: calculate GPMCFCLKDIVIDER based on WAITMONITORINGTIME
  ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug
  ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters

 arch/arm/mach-omap2/gpmc-nand.c|  17 +-
 arch/arm/mach-omap2/gpmc-onenand.c |   4 +-
 arch/arm/mach-omap2/usb-tusb6010.c |   4 +-
 drivers/memory/Makefile|   2 +
 drivers/memory/omap-gpmc.c | 313 +
 include/linux/omap-gpmc.h  |   2 +-
 6 files changed, 265 insertions(+), 77 deletions(-)

-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 5/8] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Robert ABEL]

The WAITMONITORINGTIME is expressed as a number of GPMC_CLK clock cycles,
even though the access is defined as asynchronous, and no GPMC_CLK clock
is provided to the external device. Still, GPMCFCLKDIVIDER is used as a divider
for the GPMC clock, so it must be programmed to define the
correct WAITMONITORINGTIME delay.

Signed-off-by: Robert ABEL 
---
 drivers/memory/omap-gpmc.c | 17 ++---
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 9340e7a..4139f0d 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -498,7 +498,7 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, 
int end_bit,
 
l = gpmc_cs_read_reg(cs, reg);
 #ifdef DEBUG
-   printk(KERN_INFO
+   pr_info(
"GPMC CS%d: %-17s: %3d ticks, %3lu ns (was %3i ticks) %3d ns\n",
   cs, name, ticks, gpmc_get_fclk_period() * ticks / 1000,
(l >> st_bit) & mask, time);
@@ -570,19 +570,14 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings 
*t)
if (gpmc_capability & GPMC_HAS_WR_ACCESS)
GPMC_SET_ONE(GPMC_CS_CONFIG6, 24, 28, wr_access);
 
-   /* caller is expected to have initialized CONFIG1 to cover
-* at least sync vs async
-*/
l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
-   if (l & (GPMC_CONFIG1_READTYPE_SYNC | GPMC_CONFIG1_WRITETYPE_SYNC)) {
 #ifdef DEBUG
-   printk(KERN_INFO "GPMC CS%d CLK period is %lu ns (div %d)\n",
-   cs, (div * gpmc_get_fclk_period()) / 1000, div);
+   pr_info("GPMC CS%d CLK period is %lu ns (div %d)\n",
+   cs, (div * gpmc_get_fclk_period()) / 1000, div);
 #endif
-   l &= ~0x03;
-   l |= (div - 1);
-   gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
-   }
+   l &= ~0x03;
+   l |= (div - 1);
+   gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
 
gpmc_cs_bool_timings(cs, &t->bool_timings);
gpmc_cs_show_timings(cs, "after gpmc_cs_set_timings");
-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 6/8] ARM OMAP2+ GPMC: calculate GPMCFCLKDIVIDER based on WAITMONITORINGTIME

[Robert ABEL]

The WAITMONITORINGTIME is expressed as a number of GPMC_CLK clock cycles,
even though the access is defined as asynchronous, and no GPMC_CLK clock
is provided to the external device. Still, GPMCFCLKDIVIDER is used as a divider
for the GPMC clock, so it must be programmed to define the
correct WAITMONITORINGTIME delay.

Calculate GPMCFCLKDIVIDER independent of gpmc,sync-clk-ps in DT for
pure asynchronous accesses, i.e. both read and write asynchronous.

Signed-off-by: Robert ABEL 
---
 arch/arm/mach-omap2/gpmc-nand.c| 17 
 arch/arm/mach-omap2/gpmc-onenand.c |  4 +-
 arch/arm/mach-omap2/usb-tusb6010.c |  4 +-
 drivers/memory/omap-gpmc.c | 82 ++
 include/linux/omap-gpmc.h  |  2 +-
 5 files changed, 89 insertions(+), 20 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
index d5951b1..e863a59 100644
--- a/arch/arm/mach-omap2/gpmc-nand.c
+++ b/arch/arm/mach-omap2/gpmc-nand.c
@@ -96,14 +96,6 @@ int gpmc_nand_init(struct omap_nand_platform_data 
*gpmc_nand_data,
gpmc_nand_res[1].start = gpmc_get_client_irq(GPMC_IRQ_FIFOEVENTENABLE);
gpmc_nand_res[2].start = gpmc_get_client_irq(GPMC_IRQ_COUNT_EVENT);
 
-   if (gpmc_t) {
-   err = gpmc_cs_set_timings(gpmc_nand_data->cs, gpmc_t);
-   if (err < 0) {
-   pr_err("omap2-gpmc: Unable to set gpmc timings: %d\n", 
err);
-   return err;
-   }
-   }
-
memset(&s, 0, sizeof(struct gpmc_settings));
if (gpmc_nand_data->of_node)
gpmc_read_settings_dt(gpmc_nand_data->of_node, &s);
@@ -111,6 +103,15 @@ int gpmc_nand_init(struct omap_nand_platform_data 
*gpmc_nand_data,
gpmc_set_legacy(gpmc_nand_data, &s);
 
s.device_nand = true;
+
+   if (gpmc_t) {
+   err = gpmc_cs_set_timings(gpmc_nand_data->cs, gpmc_t, &s);
+   if (err < 0) {
+   pr_err("omap2-gpmc: Unable to set gpmc timings: %d\n", 
err);
+   return err;
+   }
+   }
+
err = gpmc_cs_program_settings(gpmc_nand_data->cs, &s);
if (err < 0)
goto out_free_cs;
diff --git a/arch/arm/mach-omap2/gpmc-onenand.c 
b/arch/arm/mach-omap2/gpmc-onenand.c
index 53d197e..f899e77 100644
--- a/arch/arm/mach-omap2/gpmc-onenand.c
+++ b/arch/arm/mach-omap2/gpmc-onenand.c
@@ -293,7 +293,7 @@ static int omap2_onenand_setup_async(void __iomem 
*onenand_base)
if (ret < 0)
return ret;
 
-   ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t);
+   ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t, &onenand_async);
if (ret < 0)
return ret;
 
@@ -331,7 +331,7 @@ static int omap2_onenand_setup_sync(void __iomem 
*onenand_base, int *freq_ptr)
if (ret < 0)
return ret;
 
-   ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t);
+   ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t, &onenand_sync);
if (ret < 0)
return ret;
 
diff --git a/arch/arm/mach-omap2/usb-tusb6010.c 
b/arch/arm/mach-omap2/usb-tusb6010.c
index 8333400..e554d9e 100644
--- a/arch/arm/mach-omap2/usb-tusb6010.c
+++ b/arch/arm/mach-omap2/usb-tusb6010.c
@@ -71,7 +71,7 @@ static int tusb_set_async_mode(unsigned sysclk_ps)
 
gpmc_calc_timings(&t, &tusb_async, &dev_t);
 
-   return gpmc_cs_set_timings(async_cs, &t);
+   return gpmc_cs_set_timings(async_cs, &t, &tusb_async);
 }
 
 static int tusb_set_sync_mode(unsigned sysclk_ps)
@@ -98,7 +98,7 @@ static int tusb_set_sync_mode(unsigned sysclk_ps)
 
gpmc_calc_timings(&t, &tusb_sync, &dev_t);
 
-   return gpmc_cs_set_timings(sync_cs, &t);
+   return gpmc_cs_set_timings(sync_cs, &t, &tusb_sync);
 }
 
 /* tusb driver calls this when it changes the chip's clocking */
diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 4139f0d..d71ea05 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -138,7 +138,9 @@
 #define GPMC_CONFIG1_PAGE_LEN(val)  ((val & 3) << 23)
 #define GPMC_CONFIG1_WAIT_READ_MON  (1 << 22)
 #define GPMC_CONFIG1_WAIT_WRITE_MON (1 << 21)
-#define GPMC_CONFIG1_WAIT_MON_IIME(val) ((val & 3) << 18)
+#define GPMC_CONFIG1_WAIT_MON_TIME(val) ((val & 3) << 18)
+/** WAITMONITORINGTIME Max Ticks */
+#define GPMC_CONFIG1_WAITMONITORINGTIME_MAX  2
 #define GPMC_CONFIG1_WAIT_PIN_SEL(val)  ((val & 3) << 16)
 #define GPMC_CONFIG1_DEVICESIZE(val)((val & 3) << 12)
 #define GPMC_CONFIG1_DEVICESIZE_16  GPMC_CONFIG1_DEVICESIZE(1)
@@ -515,13 +517,46 @@ static int set_gpmc_timing_reg(int cs, int reg, int 
st_bit, int end_bit,
t->f

[PATCH 4/8] ARM OMAP2+ GPMC: change get_gpmc_timing_reg output for DTS

[Robert ABEL]

DTS output was formatted to require additional work when copy-pasting into DTS.
Nano-second timings were replaced with interval of values that produce the same
number of clock ticks.

Signed-off-by: Robert ABEL 
---
 drivers/memory/omap-gpmc.c | 35 ++-
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index dbb6753..9340e7a 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -337,32 +337,49 @@ static void gpmc_cs_bool_timings(int cs, const struct 
gpmc_bool_timings *p)
 }
 
 #ifdef DEBUG
+/**
+ * get_gpmc_timing_reg - read a timing parameter and print DTS settings for it.
+ * @cs  Chip Select Region
+ * @reg GPMC_CS_CONFIGn register offset.
+ * @st_bit  Start Bit
+ * @end_bit End Bit. Must be >= @st_bit.
+ * @nameDTS node name, w/o "gpmc,"
+ * @raw Raw Format Option.
+ *  raw format:  gpmc,name = 
+ *  tick format: gpmc,name =  /‍*(x ns -- y ns]; x ticks 
*‍/
+ *  Where (x ns -- y ns] is the half-open interval from x ns to y ns 
that
+ *  result in the same tick value.
+ * @noval   Parameter values equal to 0 are not printed.
+ * @shift   Parameter value left shifts @shift, which is then printed instead 
of value.
+ *
+ */
 static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
   bool raw, bool noval, int shift,
   const char *name)
 {
u32 l;
-   int nr_bits, max_value, mask;
+   int nr_bits;
+   int mask;
 
l = gpmc_cs_read_reg(cs, reg);
nr_bits = end_bit - st_bit + 1;
-   max_value = (1 << nr_bits) - 1;
-   mask = max_value << st_bit;
-   l = (l & mask) >> st_bit;
+   mask = (1 << nr_bits) - 1;
+   l = (l >> st_bit) & mask;
if (shift)
l = (shift << l);
if (noval && (l == 0))
return 0;
if (!raw) {
-   unsigned int time_ns_min, time_ns, time_ns_max;
+   /* DTS tick format for timings in ns */
+   unsigned int time_ns;
+   unsigned int time_ns_min;
 
time_ns_min = gpmc_ticks_to_ns(l ? l - 1 : 0);
time_ns = gpmc_ticks_to_ns(l);
-   time_ns_max = gpmc_ticks_to_ns(l + 1 > max_value ?
-  max_value : l + 1);
-   pr_info("gpmc,%s = <%u> (%u - %u ns, %i ticks)\n",
-   name, time_ns, time_ns_min, time_ns_max, l);
+   pr_info("gpmc,%s = <%u> /* (%u ns - %u ns]; %i ticks */\n",
+   name, time_ns, time_ns_min, time_ns, l);
} else {
+   /* raw format */
pr_info("gpmc,%s = <%u>\n", name, l);
}
 
-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/8] ARM OMAP2+ GPMC: add bus children

[Robert ABEL]

This patch adds support for spawning buses as children of the GPMC.

Signed-off-by: Robert ABEL 
---
 drivers/memory/omap-gpmc.c | 17 +++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 5cabac8..74a8c52 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -1800,8 +1801,20 @@ static int gpmc_probe_generic_child(struct 
platform_device *pdev,
gpmc_cs_enable_mem(cs);
 
 no_timings:
-   if (of_platform_device_create(child, NULL, &pdev->dev))
-   return 0;
+
+   /* create platform device, NULL on error or when disabled */
+   if (!of_platform_device_create(child, NULL, &pdev->dev))
+   goto err_child_fail;
+
+   /* is child a common bus? */
+   if (of_match_node(of_default_bus_match_table, child))
+   /* create children and other common bus children */
+   if (of_platform_populate(child, of_default_bus_match_table, 
NULL, &pdev->dev))
+   goto err_child_fail;
+
+   return 0;
+
+err_child_fail:
 
dev_err(&pdev->dev, "failed to create gpmc child %s\n", child->name);
ret = -ENODEV;
-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 4/8] ARM OMAP2+ GPMC: fix debug output alignment

[Robert ABEL]

GPMC debug output is aligned to 10 characters for field names.
However, some fields have bigger names, screwing up the alignment.
Consequently, alignment was changed to longest field name (17 chars) for now.

Signed-off-by: Robert ABEL 
---
 drivers/memory/omap-gpmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 74a8c52..dbb6753 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -482,7 +482,7 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, 
int end_bit,
l = gpmc_cs_read_reg(cs, reg);
 #ifdef DEBUG
printk(KERN_INFO
-   "GPMC CS%d: %-10s: %3d ticks, %3lu ns (was %3i ticks) %3d ns\n",
+   "GPMC CS%d: %-17s: %3d ticks, %3lu ns (was %3i ticks) %3d ns\n",
   cs, name, ticks, gpmc_get_fclk_period() * ticks / 1000,
(l >> st_bit) & mask, time);
 #endif
-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/8] ARM OMAP2+ GPMC: add bus children

[Robert Abel]

Hi Roger,

On Tue, Mar 3, 2015 at 11:09 AM, Roger Quadros  wrote:
> If that is the case then I'd rather not check for return value of 
> of_platform_populate().
> Failure in populating GPMC child's children is already out of scope of GPMC 
> driver.

Well, I'd rather leave it in for now. If something *does* break in the
future, the user will at least get a message about it.

Regards,

Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/8] ARM OMAP2+ GPMC: fixes and bus children

[Robert Abel]

Hi Roger,

On Tue, Mar 3, 2015 at 1:55 PM, Roger Quadros  wrote:
> I'm OK with this version.
>
> Tony, after you ACK these I will queue them for v4.1.

Please use v4 of my patches. The DTS output has been changed and the
comments have their colon.

Regards,

Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] ARM OMAP2 GPMC undefines DEBUG

[Robert ABEL]

The GPMC driver in OMAP2 undef's DEBUG, which makes it unnecessarily hard to 
turn DEBUG on,
because that's not expected behavior.
My proposed patch gets rid of the line undef'ing DEBUG.

Robert ABEL (1):
  ARM OMAP2 GPMC: don't undef DEBUG

 arch/arm/mach-omap2/gpmc.c | 2 --
 1 file changed, 2 deletions(-)

-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] ARM OMAP2 GPMC: don't undef DEBUG

[Robert ABEL]

Signed-off-by: Robert ABEL 
---
 arch/arm/mach-omap2/gpmc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index ab43755..1ab6bc0 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -12,8 +12,6 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
-#undef DEBUG
-
 #include 
 #include 
 #include 
-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/8] ARM OMAP2+ GPMC: change get_gpmc_timing_reg output for DTS

[Robert Abel]

Hi Roger,

On Fri, Feb 27, 2015 at 11:43 AM, Roger Quadros  wrote:
>>   time_ns_min = gpmc_ticks_to_ns(l ? l - 1 : 0);
>
> should be
> time_ns_min = l ? gpmc_ticks_to_ns(l - 1) + 1 : 0;
That's a micro-optimization.
>
> + 1ns since we don't want to fall into the previous tick 
> bracket.
> for l == 0 we have t_min as 0. no need to pass it through 
> gpmc_ticks_to_ns() or add 1 ns.
That's why the invervals are half-open. I can make them closed, no problem.

Regards,

Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/8] ARM OMAP2+ GPMC: change get_gpmc_timing_reg output for DTS

[Robert Abel]

On Thu, Feb 26, 2015 at 4:06 PM, Sergei Shtylyov
 wrote:
>Documentation/kernel-doc-nano-HOWTO.txt requires colons after the
> parameter names, doesn't it?

Jesus Christ, you guys are killing me...
I've already spent way more time on this patch series than I intended
to anyway...

>> +   mask = (1 << nr_bits) - 1;
>
>
>BIT(nr_bits) - 1, perhaps?

Not happening... BIT macro obscures what's actually going on.

Regards,

Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/8] ARM OMAP2+ GPMC: add bus children

[Robert Abel]

Hi Roger,

On Fri, Feb 27, 2015 at 11:24 AM, Roger Quadros  wrote:
>> + /* is child a common bus? */
>> + if (of_match_node(of_default_bus_match_table, child))
>> + /* create children and other common bus children */
>> + if (of_platform_populate(child, of_default_bus_match_table, 
>> NULL, &pdev->dev))
>> + goto err_child_fail;
>
> this would print "failed to create gpmc child" but we have already created
> the gpmc child in the first of_platform_device_create() call.
> A more appropriate message would be "failed to populate all children of 
> child->name"
>
> Also do you want to return failure?
> it will result in of_node_put() of the child and another print message
> about "probing gpmc child %s failed" in gpmc_probe_dt().
>
> IMO if the GPMC node's child was created fine then we shouldn't return error.

As of_platform_populate _always_ return 0 no matter what, the only way
to reach that message is if probing the child failed.
As I cannot see into the future when of_platform_populate might
actually be changed to return meaningful codes, we shouldn't try to
foresee what the actual problem might be today either. This is a
battle for another day.

Regards,

Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 3/8 v4] ARM OMAP2+ GPMC: fix debug output alignment

[Robert ABEL]

GPMC debug output is aligned to 10 characters for field names.
However, some fields have bigger names, screwing up the alignment.
Consequently, alignment was changed to longest field name (17 chars) for now.

Signed-off-by: Robert ABEL 
---
 drivers/memory/omap-gpmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 74a8c52..dbb6753 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -482,7 +482,7 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, 
int end_bit,
l = gpmc_cs_read_reg(cs, reg);
 #ifdef DEBUG
printk(KERN_INFO
-   "GPMC CS%d: %-10s: %3d ticks, %3lu ns (was %3i ticks) %3d ns\n",
+   "GPMC CS%d: %-17s: %3d ticks, %3lu ns (was %3i ticks) %3d ns\n",
   cs, name, ticks, gpmc_get_fclk_period() * ticks / 1000,
(l >> st_bit) & mask, time);
 #endif
-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/8 v4] ARM OMAP2+ GPMC: add bus children

[Robert ABEL]

This patch adds support for spawning buses as children of the GPMC.

Signed-off-by: Robert ABEL 
---
 drivers/memory/omap-gpmc.c | 17 +++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 5cabac8..74a8c52 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -1800,8 +1801,20 @@ static int gpmc_probe_generic_child(struct 
platform_device *pdev,
gpmc_cs_enable_mem(cs);
 
 no_timings:
-   if (of_platform_device_create(child, NULL, &pdev->dev))
-   return 0;
+
+   /* create platform device, NULL on error or when disabled */
+   if (!of_platform_device_create(child, NULL, &pdev->dev))
+   goto err_child_fail;
+
+   /* is child a common bus? */
+   if (of_match_node(of_default_bus_match_table, child))
+   /* create children and other common bus children */
+   if (of_platform_populate(child, of_default_bus_match_table, 
NULL, &pdev->dev))
+   goto err_child_fail;
+
+   return 0;
+
+err_child_fail:
 
dev_err(&pdev->dev, "failed to create gpmc child %s\n", child->name);
ret = -ENODEV;
-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 4/8 v4] ARM OMAP2+ GPMC: change get_gpmc_timing_reg output for DTS

[Robert ABEL]

DTS output was formatted to require additional work when copy-pasting into DTS.
Nano-second timings were replaced with interval of values that produce the same
number of clock ticks.

Signed-off-by: Robert ABEL 
---
 drivers/memory/omap-gpmc.c | 38 --
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index dbb6753..432e638 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -337,32 +337,50 @@ static void gpmc_cs_bool_timings(int cs, const struct 
gpmc_bool_timings *p)
 }
 
 #ifdef DEBUG
+/**
+ * get_gpmc_timing_reg - read a timing parameter and print DTS settings for it.
+ * @cs:  Chip Select Region
+ * @reg: GPMC_CS_CONFIGn register offset.
+ * @st_bit:  Start Bit
+ * @end_bit: End Bit. Must be >= @st_bit.
+ * @name:DTS node name, w/o "gpmc,"
+ * @raw: Raw Format Option.
+ *   raw format:  gpmc,name = 
+ *   tick format: gpmc,name =  /‍* x ns -- y ns; x ticks 
*‍/
+ *   Where x ns -- y ns result in the same tick value.
+ * @noval:   Parameter values equal to 0 are not printed.
+ * @shift:   Parameter value left shifts @shift, which is then printed instead 
of value.
+ * @return:  Specified timing parameter (after optional @shift).
+ *
+ */
 static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
   bool raw, bool noval, int shift,
   const char *name)
 {
u32 l;
-   int nr_bits, max_value, mask;
+   int nr_bits;
+   int mask;
 
l = gpmc_cs_read_reg(cs, reg);
nr_bits = end_bit - st_bit + 1;
-   max_value = (1 << nr_bits) - 1;
-   mask = max_value << st_bit;
-   l = (l & mask) >> st_bit;
+   mask = (1 << nr_bits) - 1;
+   l = (l >> st_bit) & mask;
if (shift)
l = (shift << l);
if (noval && (l == 0))
return 0;
if (!raw) {
-   unsigned int time_ns_min, time_ns, time_ns_max;
+   /* DTS tick format for timings in ns */
+   unsigned int time_ns;
+   unsigned int time_ns_min = 0;
 
-   time_ns_min = gpmc_ticks_to_ns(l ? l - 1 : 0);
+   if (l)
+   time_ns_min = gpmc_ticks_to_ns(l - 1) + 1;
time_ns = gpmc_ticks_to_ns(l);
-   time_ns_max = gpmc_ticks_to_ns(l + 1 > max_value ?
-  max_value : l + 1);
-   pr_info("gpmc,%s = <%u> (%u - %u ns, %i ticks)\n",
-   name, time_ns, time_ns_min, time_ns_max, l);
+   pr_info("gpmc,%s = <%u> /* %u ns - %u ns; %i ticks */\n",
+   name, time_ns, time_ns_min, time_ns, l);
} else {
+   /* raw format */
pr_info("gpmc,%s = <%u>\n", name, l);
}
 
-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/8 v4] ARM OMAP2+ GPMC: fixes and bus children

[Robert ABEL]

These are the changes I proposed in these patch series: [1], [2], [3], [4]
rebased to 3.19 as well as new changes for little bugs I noticed while
preparing this patch series as well as changes introduced via comments.

1. DEBUG was undefined in source code --> remove offending lines
2. add capability to have busses as children of the GPMC and multiple
   devices on a bus. See [2] for an example DTS syntax.
3. debug output was unaligned --> align it
4. output for copy-pasting to DTS had erroneous timing outputs and
   made it hard to copy-paste --> correct timing values, add comments
   as DTS comments.
5. WAITMONITORINGTIME is expressed as GPMC_CLK cycles for all accesses.
   GPMCFCLKDIVIDER is used as a divider, so it must always be programmed.
6. GPMCFCLKDIVIDER is calculated according to WAITMONITORINGTIME for
   asynchronous accesses inside the driver --> asynchronous accesses now
   completely decoupled from gpmc,sync-clk-ps.
7. WAITMONITORINGTIME was being programmed/shown in GPMC_FCLK cycles instead
   of GPMC_CLK cycles --> add clock domain information where necessary.
8. Calculated values for WAITMONITORINGTIME and CLKACTIVATIONTIME that were
   outside the defined range would not raise an error.
   DEVICESIZE, ATTACHEDDEVICEPAGELENGTH, WAITMONITORINGTIME and
   CLKACTIVATIONTIME would not be marked as incorrect on DTS output.
   --> Fix all of these.

[1]: https://lkml.org/lkml/2015/2/12/495
[2]: https://lkml.org/lkml/2015/2/16/337
[3]: https://lkml.org/lkml/2015/2/24/609
[4]: https://lkml.org/lkml/2015/2/26/387

Robert ABEL (9):
  ARM OMAP2+ GPMC: don't undef DEBUG
  ARM OMAP2+ GPMC: add bus children
  ARM OMAP2+ GPMC: fix debug output alignment
  ARM OMAP2+ GPMC: change get_gpmc_timing_reg output for DTS
  ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER
  ARM OMAP2+ GPMC: calculate GPMCFCLKDIVIDER based on WAITMONITORINGTIME
  ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug
  ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters

 arch/arm/mach-omap2/gpmc-nand.c|  17 +-
 arch/arm/mach-omap2/gpmc-onenand.c |   4 +-
 arch/arm/mach-omap2/usb-tusb6010.c |   4 +-
 drivers/memory/Makefile|   2 +
 drivers/memory/omap-gpmc.c | 313 +
 include/linux/omap-gpmc.h  |   2 +-
 6 files changed, 265 insertions(+), 77 deletions(-)

-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 5/8 v4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Robert ABEL]

The WAITMONITORINGTIME is expressed as a number of GPMC_CLK clock cycles,
even though the access is defined as asynchronous, and no GPMC_CLK clock
is provided to the external device. Still, GPMCFCLKDIVIDER is used as a divider
for the GPMC clock, so it must be programmed to define the
correct WAITMONITORINGTIME delay.

Signed-off-by: Robert ABEL 
---
 drivers/memory/omap-gpmc.c | 17 ++---
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 432e638..02e5228 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -499,7 +499,7 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, 
int end_bit,
 
l = gpmc_cs_read_reg(cs, reg);
 #ifdef DEBUG
-   printk(KERN_INFO
+   pr_info(
"GPMC CS%d: %-17s: %3d ticks, %3lu ns (was %3i ticks) %3d ns\n",
   cs, name, ticks, gpmc_get_fclk_period() * ticks / 1000,
(l >> st_bit) & mask, time);
@@ -571,19 +571,14 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings 
*t)
if (gpmc_capability & GPMC_HAS_WR_ACCESS)
GPMC_SET_ONE(GPMC_CS_CONFIG6, 24, 28, wr_access);
 
-   /* caller is expected to have initialized CONFIG1 to cover
-* at least sync vs async
-*/
l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
-   if (l & (GPMC_CONFIG1_READTYPE_SYNC | GPMC_CONFIG1_WRITETYPE_SYNC)) {
 #ifdef DEBUG
-   printk(KERN_INFO "GPMC CS%d CLK period is %lu ns (div %d)\n",
-   cs, (div * gpmc_get_fclk_period()) / 1000, div);
+   pr_info("GPMC CS%d CLK period is %lu ns (div %d)\n",
+   cs, (div * gpmc_get_fclk_period()) / 1000, div);
 #endif
-   l &= ~0x03;
-   l |= (div - 1);
-   gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
-   }
+   l &= ~0x03;
+   l |= (div - 1);
+   gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
 
gpmc_cs_bool_timings(cs, &t->bool_timings);
gpmc_cs_show_timings(cs, "after gpmc_cs_set_timings");
-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/8 v4] ARM OMAP2+ GPMC: don't undef DEBUG

[Robert ABEL]

OMAP2+ GPMC driver undefines DEBUG, which makes it unnecessarily
hard to turn DEBUG on. Remove the offending lines.

Signed-off-by: Robert ABEL 
---
 drivers/memory/omap-gpmc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 24696f5..5cabac8 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -12,8 +12,6 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
-#undef DEBUG
-
 #include 
 #include 
 #include 
-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 6/8 v4] ARM OMAP2+ GPMC: calculate GPMCFCLKDIVIDER based on WAITMONITORINGTIME

[Robert ABEL]

The WAITMONITORINGTIME is expressed as a number of GPMC_CLK clock cycles,
even though the access is defined as asynchronous, and no GPMC_CLK clock
is provided to the external device. Still, GPMCFCLKDIVIDER is used as a divider
for the GPMC clock, so it must be programmed to define the
correct WAITMONITORINGTIME delay.

Calculate GPMCFCLKDIVIDER independent of gpmc,sync-clk-ps in DT for
pure asynchronous accesses, i.e. both read and write asynchronous.

Signed-off-by: Robert ABEL 
---
 arch/arm/mach-omap2/gpmc-nand.c| 17 
 arch/arm/mach-omap2/gpmc-onenand.c |  4 +-
 arch/arm/mach-omap2/usb-tusb6010.c |  4 +-
 drivers/memory/omap-gpmc.c | 85 ++
 include/linux/omap-gpmc.h  |  2 +-
 5 files changed, 92 insertions(+), 20 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
index d5951b1..e863a59 100644
--- a/arch/arm/mach-omap2/gpmc-nand.c
+++ b/arch/arm/mach-omap2/gpmc-nand.c
@@ -96,14 +96,6 @@ int gpmc_nand_init(struct omap_nand_platform_data 
*gpmc_nand_data,
gpmc_nand_res[1].start = gpmc_get_client_irq(GPMC_IRQ_FIFOEVENTENABLE);
gpmc_nand_res[2].start = gpmc_get_client_irq(GPMC_IRQ_COUNT_EVENT);
 
-   if (gpmc_t) {
-   err = gpmc_cs_set_timings(gpmc_nand_data->cs, gpmc_t);
-   if (err < 0) {
-   pr_err("omap2-gpmc: Unable to set gpmc timings: %d\n", 
err);
-   return err;
-   }
-   }
-
memset(&s, 0, sizeof(struct gpmc_settings));
if (gpmc_nand_data->of_node)
gpmc_read_settings_dt(gpmc_nand_data->of_node, &s);
@@ -111,6 +103,15 @@ int gpmc_nand_init(struct omap_nand_platform_data 
*gpmc_nand_data,
gpmc_set_legacy(gpmc_nand_data, &s);
 
s.device_nand = true;
+
+   if (gpmc_t) {
+   err = gpmc_cs_set_timings(gpmc_nand_data->cs, gpmc_t, &s);
+   if (err < 0) {
+   pr_err("omap2-gpmc: Unable to set gpmc timings: %d\n", 
err);
+   return err;
+   }
+   }
+
err = gpmc_cs_program_settings(gpmc_nand_data->cs, &s);
if (err < 0)
goto out_free_cs;
diff --git a/arch/arm/mach-omap2/gpmc-onenand.c 
b/arch/arm/mach-omap2/gpmc-onenand.c
index 53d197e..f899e77 100644
--- a/arch/arm/mach-omap2/gpmc-onenand.c
+++ b/arch/arm/mach-omap2/gpmc-onenand.c
@@ -293,7 +293,7 @@ static int omap2_onenand_setup_async(void __iomem 
*onenand_base)
if (ret < 0)
return ret;
 
-   ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t);
+   ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t, &onenand_async);
if (ret < 0)
return ret;
 
@@ -331,7 +331,7 @@ static int omap2_onenand_setup_sync(void __iomem 
*onenand_base, int *freq_ptr)
if (ret < 0)
return ret;
 
-   ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t);
+   ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t, &onenand_sync);
if (ret < 0)
return ret;
 
diff --git a/arch/arm/mach-omap2/usb-tusb6010.c 
b/arch/arm/mach-omap2/usb-tusb6010.c
index 8333400..e554d9e 100644
--- a/arch/arm/mach-omap2/usb-tusb6010.c
+++ b/arch/arm/mach-omap2/usb-tusb6010.c
@@ -71,7 +71,7 @@ static int tusb_set_async_mode(unsigned sysclk_ps)
 
gpmc_calc_timings(&t, &tusb_async, &dev_t);
 
-   return gpmc_cs_set_timings(async_cs, &t);
+   return gpmc_cs_set_timings(async_cs, &t, &tusb_async);
 }
 
 static int tusb_set_sync_mode(unsigned sysclk_ps)
@@ -98,7 +98,7 @@ static int tusb_set_sync_mode(unsigned sysclk_ps)
 
gpmc_calc_timings(&t, &tusb_sync, &dev_t);
 
-   return gpmc_cs_set_timings(sync_cs, &t);
+   return gpmc_cs_set_timings(sync_cs, &t, &tusb_sync);
 }
 
 /* tusb driver calls this when it changes the chip's clocking */
diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 02e5228..8ee335d 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -138,7 +138,9 @@
 #define GPMC_CONFIG1_PAGE_LEN(val)  ((val & 3) << 23)
 #define GPMC_CONFIG1_WAIT_READ_MON  (1 << 22)
 #define GPMC_CONFIG1_WAIT_WRITE_MON (1 << 21)
-#define GPMC_CONFIG1_WAIT_MON_IIME(val) ((val & 3) << 18)
+#define GPMC_CONFIG1_WAIT_MON_TIME(val) ((val & 3) << 18)
+/** WAITMONITORINGTIME Max Ticks */
+#define GPMC_CONFIG1_WAITMONITORINGTIME_MAX  2
 #define GPMC_CONFIG1_WAIT_PIN_SEL(val)  ((val & 3) << 16)
 #define GPMC_CONFIG1_DEVICESIZE(val)((val & 3) << 12)
 #define GPMC_CONFIG1_DEVICESIZE_16  GPMC_CONFIG1_DEVICESIZE(1)
@@ -516,13 +518,48 @@ static int set_gpmc_timing_reg(int cs, int reg, int 
st_bit, int end_bit,
t->fie

[PATCH 8/8 v4] ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters

[Robert ABEL]

GPMC_CONFIG1_i parameters CLKACTIVATIONTIME and WAITMONITORINGTIME
have reserved values.
Raise an error if calculated timings try to program reserved values.

GPMC_CONFIG1_i ATTACHEDDEVICEPAGELENGTH and DEVICESIZE were already checked
when parsing the DT.

Explicitly comment invalid values on gpmc_cs_show_timings for
-CLKACTIVATIONTIME
-WAITMONITORINGTIME
-DEVICESIZE
-ATTACHEDDEVICEPAGELENGTH

Signed-off-by: Robert ABEL 
---
 drivers/memory/omap-gpmc.c | 68 ++
 1 file changed, 45 insertions(+), 23 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index d091065..750c655 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -135,7 +135,11 @@
 #define GPMC_CONFIG1_WRITETYPE_ASYNC(0 << 27)
 #define GPMC_CONFIG1_WRITETYPE_SYNC (1 << 27)
 #define GPMC_CONFIG1_CLKACTIVATIONTIME(val) ((val & 3) << 25)
+/** CLKACTIVATIONTIME Max Ticks */
+#define GPMC_CONFIG1_CLKACTIVATIONTIME_MAX 2
 #define GPMC_CONFIG1_PAGE_LEN(val)  ((val & 3) << 23)
+/** ATTACHEDDEVICEPAGELENGTH Max Value */
+#define GPMC_CONFIG1_ATTACHEDDEVICEPAGELENGTH_MAX 2
 #define GPMC_CONFIG1_WAIT_READ_MON  (1 << 22)
 #define GPMC_CONFIG1_WAIT_WRITE_MON (1 << 21)
 #define GPMC_CONFIG1_WAIT_MON_TIME(val) ((val & 3) << 18)
@@ -144,6 +148,8 @@
 #define GPMC_CONFIG1_WAIT_PIN_SEL(val)  ((val & 3) << 16)
 #define GPMC_CONFIG1_DEVICESIZE(val)((val & 3) << 12)
 #define GPMC_CONFIG1_DEVICESIZE_16  GPMC_CONFIG1_DEVICESIZE(1)
+/** DEVICESIZE Max Value */
+#define GPMC_CONFIG1_DEVICESIZE_MAX 1
 #define GPMC_CONFIG1_DEVICETYPE(val)((val & 3) << 10)
 #define GPMC_CONFIG1_DEVICETYPE_NOR GPMC_CONFIG1_DEVICETYPE(0)
 #define GPMC_CONFIG1_MUXTYPE(val)   ((val & 3) << 8)
@@ -393,6 +399,8 @@ static void gpmc_cs_bool_timings(int cs, const struct 
gpmc_bool_timings *p)
  * @reg: GPMC_CS_CONFIGn register offset.
  * @st_bit:  Start Bit
  * @end_bit: End Bit. Must be >= @st_bit.
+ * @ma:x Maximum parameter value (before optional @shift).
+ *   If 0, maximum is as high as @st_bit and @end_bit allow.
  * @name:DTS node name, w/o "gpmc,"
  * @cd:  Clock Domain of timing parameter.
  * @shift:   Parameter value left shifts @shift, which is then printed instead 
of value.
@@ -400,13 +408,14 @@ static void gpmc_cs_bool_timings(int cs, const struct 
gpmc_bool_timings *p)
  *   raw format:  gpmc,name = 
  *   tick format: gpmc,name =  /‍* x ns -- y ns; x ticks 
*‍/
  *   Where x ns -- y ns result in the same tick value.
+ *   When @max is exceeded, "invalid" is printed inside comment.
  * @noval:   Parameter values equal to 0 are not printed.
  * @return:  Specified timing parameter (after optional @shift).
  *
  */
 static int get_gpmc_timing_reg(
/* timing specifiers */
-   int cs, int reg, int st_bit, int end_bit,
+   int cs, int reg, int st_bit, int end_bit, int max,
const char *name, const enum gpmc_clk_domain cd,
/* value transform */
int shift,
@@ -416,11 +425,15 @@ static int get_gpmc_timing_reg(
u32 l;
int nr_bits;
int mask;
+   bool invalid;
 
l = gpmc_cs_read_reg(cs, reg);
nr_bits = end_bit - st_bit + 1;
mask = (1 << nr_bits) - 1;
l = (l >> st_bit) & mask;
+   if (!max)
+   max = mask;
+   invalid = l > max;
if (shift)
l = (shift << l);
if (noval && (l == 0))
@@ -433,11 +446,11 @@ static int get_gpmc_timing_reg(
if (l)
time_ns_min = gpmc_clk_ticks_to_ns(l - 1, cs, cd) + 1;
time_ns = gpmc_clk_ticks_to_ns(l, cs, cd);
-   pr_info("gpmc,%s = <%u> /* %u ns - %u ns; %i ticks */\n",
-   name, time_ns, time_ns_min, time_ns, l);
+   pr_info("gpmc,%s = <%u> /* %u ns - %u ns; %i ticks%s*/\n",
+   name, time_ns, time_ns_min, time_ns, l, invalid ? "; 
invalid " : " ");
} else {
/* raw format */
-   pr_info("gpmc,%s = <%u>\n", name, l);
+   pr_info("gpmc,%s = <%u>%s\n", name, l, invalid ? " /* invalid 
*/" : "");
}
 
return l;
@@ -447,15 +460,19 @@ static int get_gpmc_timing_reg(
pr_info("cs%i %s: 0x%08x\n", cs, #config, \
gpmc_cs_read_reg(cs, config))
 #define GPMC_GET_RAW(reg, st, end, field) \
-   get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 1, 
0)
+   get_gpmc_timing_reg(cs, (reg), (st), (end), 0, field, GPMC_CD_FCLK, 0, 
1, 0)
+#define GPMC_GET_RAW_MAX(reg, st, end, max, field) \
+   get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, 

[PATCH 7/8 v4] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

[Robert ABEL]

The WAITMONITORINGTIME is expressed as a number of GPMC_CLK clock cycles,
even though the access is defined as asynchronous, and no GPMC_CLK clock
is provided to the external device. Still, GPMCFCLKDIVIDER is used as a divider
for the GPMC clock, so it must be programmed to define the
correct WAITMONITORINGTIME delay.

This patch correctly computes WAITMONITORINGTIME in GPMC_CLK cycles instead of 
GPMC_FCLK cycles,
both during programming (gpmc_cs_set_timings) and during retrieval 
(gpmc_cs_show_timings).

Signed-off-by: Robert ABEL 
---
 drivers/memory/omap-gpmc.c | 128 +++--
 1 file changed, 101 insertions(+), 27 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 8ee335d..d091065 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -170,6 +170,11 @@
  */
 #defineGPMC_NR_IRQ 2
 
+enum gpmc_clk_domain {
+   GPMC_CD_FCLK,
+   GPMC_CD_CLK
+};
+
 struct gpmc_cs_data {
const char *name;
 
@@ -268,16 +273,54 @@ static unsigned long gpmc_get_fclk_period(void)
return rate;
 }
 
-static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
+/**
+ * gpmc_get_clk_period - get period of selected clock domain in ps
+ * @cs Chip Select Region.
+ * @cd Clock Domain.
+ *
+ * GPMC_CS_CONFIG1 GPMCFCLKDIVIDER for cs has to be setup
+ * prior to calling this function with GPMC_CD_CLK.
+ */
+static unsigned long gpmc_get_clk_period(int cs, enum gpmc_clk_domain cd)
+{
+
+   unsigned long tick_ps = gpmc_get_fclk_period();
+   u32 l;
+   int div;
+
+   switch (cd) {
+   case GPMC_CD_CLK:
+   /* get current clk divider */
+   l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
+   div = (l & 0x03) + 1;
+   /* get GPMC_CLK period */
+   tick_ps *= div;
+   break;
+   case GPMC_CD_FCLK:
+   /* FALL-THROUGH */
+   default:
+   break;
+   }
+
+   return tick_ps;
+
+}
+
+static unsigned int gpmc_ns_to_clk_ticks(unsigned int time_ns, int cs, enum 
gpmc_clk_domain cd)
 {
unsigned long tick_ps;
 
/* Calculate in picosecs to yield more exact results */
-   tick_ps = gpmc_get_fclk_period();
+   tick_ps = gpmc_get_clk_period(cs, cd);
 
return (time_ns * 1000 + tick_ps - 1) / tick_ps;
 }
 
+static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
+{
+   return gpmc_ns_to_clk_ticks(time_ns, /* any CS */ 0, GPMC_CD_FCLK);
+}
+
 static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
 {
unsigned long tick_ps;
@@ -288,9 +331,14 @@ static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
return (time_ps + tick_ps - 1) / tick_ps;
 }
 
+unsigned int gpmc_clk_ticks_to_ns(unsigned ticks, int cs, enum gpmc_clk_domain 
cd)
+{
+   return ticks * gpmc_get_clk_period(cs, cd) / 1000;
+}
+
 unsigned int gpmc_ticks_to_ns(unsigned int ticks)
 {
-   return ticks * gpmc_get_fclk_period() / 1000;
+   return gpmc_clk_ticks_to_ns(ticks, /* any CS */ 0, GPMC_CD_FCLK);
 }
 
 static unsigned int gpmc_ticks_to_ps(unsigned int ticks)
@@ -346,18 +394,24 @@ static void gpmc_cs_bool_timings(int cs, const struct 
gpmc_bool_timings *p)
  * @st_bit:  Start Bit
  * @end_bit: End Bit. Must be >= @st_bit.
  * @name:DTS node name, w/o "gpmc,"
+ * @cd:  Clock Domain of timing parameter.
+ * @shift:   Parameter value left shifts @shift, which is then printed instead 
of value.
  * @raw: Raw Format Option.
  *   raw format:  gpmc,name = 
  *   tick format: gpmc,name =  /‍* x ns -- y ns; x ticks 
*‍/
  *   Where x ns -- y ns result in the same tick value.
  * @noval:   Parameter values equal to 0 are not printed.
- * @shift:   Parameter value left shifts @shift, which is then printed instead 
of value.
  * @return:  Specified timing parameter (after optional @shift).
  *
  */
-static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
-  bool raw, bool noval, int shift,
-  const char *name)
+static int get_gpmc_timing_reg(
+   /* timing specifiers */
+   int cs, int reg, int st_bit, int end_bit,
+   const char *name, const enum gpmc_clk_domain cd,
+   /* value transform */
+   int shift,
+   /* format specifiers */
+   bool raw, bool noval)
 {
u32 l;
int nr_bits;
@@ -377,8 +431,8 @@ static int get_gpmc_timing_reg(int cs, int reg, int st_bit, 
int end_bit,
unsigned int time_ns_min = 0;
 
if (l)
-   time_ns_min = gpmc_ticks_to_ns(l - 1) + 1;
-   time_ns = gpmc_ticks_to_ns(l);
+   time_ns_min = gpmc_clk_ticks_to_ns(l - 1, cs, cd) + 1;
+   time_ns = gpmc_clk_ticks_to_ns(l, cs, cd);
pr_info("gpmc,%s = <%u> /* %u ns - %u ns; %i ticks */\n",
na

[PATCH 3/4] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

[Robert ABEL]

WAITMONITORINGTIME is expressed in GPMC_CLK cycles (even for asynchronous 
accesses).
However the driver currently converts them to GPMC_FCLK cycles, thus 
waitmonitoringtime in dt
had to be predivided by divider as a workaround. This patch fixes the issue by 
reading the
current GPMCFCLKDIVIDER and adjusting the divisor for WAITMONITORINGTIME 
accordingly.

Signed-off-by: Robert ABEL 
---
 arch/arm/mach-omap2/gpmc.c | 78 +-
 1 file changed, 64 insertions(+), 14 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index bae4a20..4fa5ff1 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -113,6 +113,9 @@
  */
 #defineGPMC_NR_IRQ 2
 
+#define GPMC_FCLK   0
+#define GPMC_CLK1
+
 struct gpmc_client_irq {
unsignedirq;
u32 bitmask;
@@ -208,16 +211,55 @@ static unsigned long gpmc_get_fclk_period(void)
return rate;
 }
 
-static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
+/**
+ * gpmc_get_clk_period - get period of selected clk in ps
+ * @cs : affected chip select
+ * @clk_sel: either one of GPMC_CLK or GPMC_FCLK
+ *
+ * GPMC_CS_CONFIG1 GPMCFCLKDIVIDER for cs has to be setup
+ * prior to calling this function with GPMC_CLK.
+ * 
+ */
+static unsigned long gpmc_get_clk_period(int cs, unsigned int clk_sel)
+{
+
+   unsigned long tick_ps = gpmc_get_fclk_period();
+   u32 l;
+   int div;
+
+   switch (clk_sel) {
+   case GPMC_CLK:
+   /* get current clk divider */
+   l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
+   div = (l & 0x03) + 1;
+   /* get GPMC_CLK period */
+   tick_ps *= div;
+   break;
+   case GPMC_FCLK:
+   /* FALL-THROUGH */
+   default:
+   break;
+   }
+
+   return tick_ps;
+
+}
+
+static unsigned int gpmc_ns_to_clk_ticks(unsigned int time_ns, int cs, 
unsigned int clk_sel)
 {
unsigned long tick_ps;
 
/* Calculate in picosecs to yield more exact results */
-   tick_ps = gpmc_get_fclk_period();
+   tick_ps = gpmc_get_clk_period(cs, clk_sel);
 
return (time_ns * 1000 + tick_ps - 1) / tick_ps;
 }
 
+static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
+{
+   return gpmc_ns_to_clk_ticks(time_ns, 0, GPMC_FCLK);
+}
+
 static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
 {
unsigned long tick_ps;
@@ -280,10 +322,10 @@ static void gpmc_cs_bool_timings(int cs, const struct 
gpmc_bool_timings *p)
 
 #ifdef DEBUG
 static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
-  int time, const char *name)
+  int time, unsigned int clk_sel, const char *name)
 #else
 static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
-  int time)
+  int time, unsigned int clk_sel)
 #endif
 {
u32 l;
@@ -292,7 +334,7 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, 
int end_bit,
if (time == 0)
ticks = 0;
else
-   ticks = gpmc_ns_to_ticks(time);
+   ticks = gpmc_ns_to_clk_ticks(time, cs, clk_sel);
nr_bits = end_bit - st_bit + 1;
if (ticks >= 1 << nr_bits) {
 #ifdef DEBUG
@@ -307,7 +349,7 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, 
int end_bit,
 #ifdef DEBUG
printk(KERN_INFO
"GPMC CS%d: %-17s: %3d ticks, %3lu ns (was %3i ticks) %3d ns\n",
-  cs, name, ticks, gpmc_get_fclk_period() * ticks / 1000,
+  cs, name, ticks, gpmc_get_clk_period(cs, clk_sel) * ticks / 1000,
(l >> st_bit) & mask, time);
 #endif
l &= ~(mask << st_bit);
@@ -320,12 +362,19 @@ static int set_gpmc_timing_reg(int cs, int reg, int 
st_bit, int end_bit,
 #ifdef DEBUG
 #define GPMC_SET_ONE(reg, st, end, field) \
if (set_gpmc_timing_reg(cs, (reg), (st), (end), \
-   t->field, #field) < 0)  \
+   t->field, GPMC_FCLK, #field) < 0)   
\
return -1
+#define GPMC_SET_ONE_C(reg, st, end, field, clk_sel) \
+   if (set_gpmc_timing_reg(cs, (reg), (st), (end), \
+t->field, clk_sel, #field) < 0) \
+return -1
 #else
 #define GPMC_SET_ONE(reg, st, end, field) \
-   if (set_gpmc_timing_reg(cs, (reg), (st), (end), t->field) < 0) \
+   if (set_gpmc_timing_reg(cs, (reg), (st), (end), t->field, GPMC_FCLK) < 
0) \
return -1
+#define GPMC_SET_ONE_C(reg, st, end, field, clk_sel) \
+   if (set_gpmc_timing_reg(cs, (reg), (st), (end), t->field, clk_sel) < 0) 
  \
+return -1
 #endif
 
 int gpmc_calc_divider(unsigned i

[PATCH 4/4] ARM OMAP2+ GPMC: add bus children

[Robert ABEL]

This patch adds support for spawning busses as children of the GPMC.

Signed-off-by: Robert ABEL 
---
 arch/arm/mach-omap2/gpmc.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 4fa5ff1..c6c8543 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -1589,9 +1590,20 @@ static int gpmc_probe_generic_child(struct 
platform_device *pdev,
gpmc_cs_set_timings(cs, &gpmc_t);
 
 no_timings:
-   if (of_platform_device_create(child, NULL, &pdev->dev))
+
+   if (of_match_node(of_default_bus_match_table, child)) {
+
+   /* ignore return code, because 0 is ambiguous */
+   of_platform_populate(child, of_default_bus_match_table, NULL, 
&pdev->dev);
return 0;
 
+   } else {
+
+   if (of_platform_device_create(child, NULL, &pdev->dev))
+   return 0;
+
+   }
+
dev_err(&pdev->dev, "failed to create gpmc child %s\n", child->name);
ret = -ENODEV;
 
-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Robert ABEL]

GPMC uses GPMCFCLKDIVIDER during synchronous as well as asynchronous accesses
in conjunction with WAITMONITORINGTIME. Thus, it's wrong to only program it for
synchronous accesses. Remove the conditional.

Signed-off-by: Robert ABEL 
---
 arch/arm/mach-omap2/gpmc.c | 15 +--
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 5c3639c..bae4a20 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -382,19 +382,14 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings 
*t)
if (gpmc_capability & GPMC_HAS_WR_ACCESS)
GPMC_SET_ONE(GPMC_CS_CONFIG6, 24, 28, wr_access);
 
-   /* caller is expected to have initialized CONFIG1 to cover
-* at least sync vs async
-*/
l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
-   if (l & (GPMC_CONFIG1_READTYPE_SYNC | GPMC_CONFIG1_WRITETYPE_SYNC)) {
 #ifdef DEBUG
-   printk(KERN_INFO "GPMC CS%d CLK period is %lu ns (div %d)\n",
-   cs, (div * gpmc_get_fclk_period()) / 1000, div);
+   printk(KERN_INFO "GPMC CS%d CLK period is %lu ns (div %d)\n",
+  cs, (div * gpmc_get_fclk_period()) / 1000, div);
 #endif
-   l &= ~0x03;
-   l |= (div - 1);
-   gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
-   }
+   l &= ~0x03;
+   l |= (div - 1);
+   gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
 
gpmc_cs_bool_timings(cs, &t->bool_timings);
 
-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/4] ARM OMAP2+ GPMC: fix debug output alignment

[Robert ABEL]

GPMC debug output is aligned to 10 characters for field names.
However, some fields have bigger names, screwing up the alignment.
Consequently, alignment was changed to longest field name (17 chars) for now.

Signed-off-by: Robert ABEL 
---
 arch/arm/mach-omap2/gpmc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 1ab6bc0..5c3639c 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -296,7 +296,7 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, 
int end_bit,
nr_bits = end_bit - st_bit + 1;
if (ticks >= 1 << nr_bits) {
 #ifdef DEBUG
-   printk(KERN_INFO "GPMC CS%d: %-10s* %3d ns, %3d ticks >= %d\n",
+   printk(KERN_INFO "GPMC CS%d: %-17s* %3d ns, %3d ticks >= %d\n",
cs, name, time, ticks, 1 << nr_bits);
 #endif
return -1;
@@ -306,7 +306,7 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, 
int end_bit,
l = gpmc_cs_read_reg(cs, reg);
 #ifdef DEBUG
printk(KERN_INFO
-   "GPMC CS%d: %-10s: %3d ticks, %3lu ns (was %3i ticks) %3d ns\n",
+   "GPMC CS%d: %-17s: %3d ticks, %3lu ns (was %3i ticks) %3d ns\n",
   cs, name, ticks, gpmc_get_fclk_period() * ticks / 1000,
(l >> st_bit) & mask, time);
 #endif
-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/4] ARM OMAP2+ GPMC: various fixes and bus children

[Robert ABEL]

I found a few more bugs in the GPMC driver. The first three patches should be 
self-explanatory:

*debug output was unaligned --> align it
*GPMCFCLKDIVIDER is used in all use cases --> always program it
*WAITMONITORINGTIME was computed in FCLK ticks instead of CLK ticks --> compute 
in CLK ticks

The last patch adds bus children to the GPMC. Currently, the GPMC driver
only supports direct children, which means one child with unique settings per 
CS region.

This patch adds explicit code inside the GPMC driver to probe for busses and
instantiate their children. (As opposed to adding the GPMC to 
of_default_bus_match_table
so it would be matched when the DT is first populated.)

GPMC properties apply to busses as well as direct children and map to whole CS 
regions.

Example:

&gpmc {
ranges = ;

my_bus: nor@0,0x { /* CS0; offset 0x */

compatible = "simple-bus";
#address-cells = <1>;
#size-cells = <1>;
ranges = ;
reg = ;
bank-width = <2>; /* GPMC_DEVWIDTH_16BIT */

gpmc,sync-read;
gpmc,sync-write;
gpmc,sync-clk-ps = ...;
... etc. ...
reg-io-width = <2>;

my_subdev0: subdev@0x { /* address 0x */

compatible = "my,driver0";
reg = ;

};
... etc. ...
my_subdevN: subdev@0x8000 { /* address 0x8000 */

compatible = "my,driverN";
        reg = ;

};

};

};


Robert ABEL (4):
  ARM OMAP2+ GPMC: fix debug output alignment
  ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER
  ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug
  ARM OMAP2+ GPMC: add bus children

 arch/arm/mach-omap2/gpmc.c | 105 ++---
 1 file changed, 81 insertions(+), 24 deletions(-)

-- 
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Robert Abel]

Hi Tony,

On 16 Feb 2015 18:10, Tony Lindgren wrote:

* Robert ABEL  [150216 07:52]:

GPMC uses GPMCFCLKDIVIDER during synchronous as well as asynchronous accesses
in conjunction with WAITMONITORINGTIME. Thus, it's wrong to only program it for
synchronous accesses. Remove the conditional.

Do you have some test case that gets fixed by this? Or does this
fix some regression?

The reason why I'm asking is that AFAIK we've had async access
working all along?
Maybe to clarify: This is only affects asynchronous accesses with 
WAITREADMONITORING and/or WAITWRITEMONITORING enabled. Regular 
asynchronous accesses without WAIT pin monitoring were never affected to 
begin with and are not affected by this change.


I don't have an off-hand test case and seeing as the current 
implementation is faulty, it's probable nobody actively uses this 
feature. Cf. OMAP35xx TRM (SPRUF98X) pp. 1103. Basically, 
WAITMONITORINGTIME extends the number of cycles from the end of WAIT 
asserted to when the data is provided/sampled and the read/write access 
(should) take place. While this behavior is esoteric, it's not a reason 
not to fix this oversight.


Regards,

Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Robert Abel]

Hi Roger,

On Tue, Feb 17, 2015 at 9:12 AM, Roger Quadros  wrote:
>
> Can you use the following wording from TRM instead?
>
> as per am335x TRM (spruh73i.pdf), section 7.1.3.3.8.3.2
>
> The WAITMONITORINGTIME is expressed as a number of GPMC_CLK clock cycles,
> even though the access is defined as asynchronous, and no GPMC_CLK clock
> is provided to the external device. Still, GPMCFCLKDIVIDER is used as a 
> divider
> for the GPMC clock, so it must be programmed to define the
> correct WAITMONITORINGTIME delay.


Verbatim? Sure can. Gonna do that with a rebase to 3.19, I guess.

>
> Instead of this can we explicitly set the GPMC_CLK divider to 1 and hence
> corresponding divider bits to 0 in the asynchronous case?
> This is because the previously calculated "div" depends on synchronous clock 
> which
> might not be properly initialized for asynchronous devices.


No, we shouldn't. If WAITREADMONITORING and/or WAITWRITEMONITORING is
enabled, sync_clk must be set in order to use WAITMONITORINGTIME
correctly. If it's not explicitly set, it's set to 0, which yields div
1 anyways.
The reason being that a fixed divider of 1 will limit a user's ability
to prolong the #WAIT-deassert --> *access delay for no good reason. If
working with a slow device, this will inconvenience users.

>
> AFAIK t->sync_clk is always 0 for asynchronous devices and 
> gpmc_calc_divider(0)
> will return 1 and your patch will work but still we shouldn't depend on 
> sync_clk for
> asynchronous devices so let's set this explicitly.


See above. Hardware depends on divider, divider is set by sync_clk, so
we shouldn't limit what a user can program in DT.
Having said that, I'm not aware that sync_clk is always 0 for async
devices. The code always parses it and sets the appropriate field in
gpmc_t, which is passed to gpmc_cs_set_timings.
Now, there is generally a lack of checking for optional/required DT
properties, so I didn't add extra checking.

Regards,

Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/4] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

[Robert Abel]

Hi Roger,

On Tue, Feb 17, 2015 at 10:27 AM, Roger Quadros  wrote:
>
> Can you please point out which DT had used this pre-divided workaround? We 
> will have to
> fix those then.

I didn't check. A cursory glance reveals all DTS in arch/arm/boot/dts
to use a value of 0.

>
> Can you please rebase your patches on top of v3.19?
> gpmc.c has been moved to drivers/memory/omap-gpmc.c


Will do.

>
> So all the below occurrences of "unsigned int clk_sel" become "enum 
> gpmc_clksel".

Yes, I should have opted for a cleaner solution from the start.

> > +static unsigned int gpmc_ns_to_clk_ticks(unsigned int time_ns, int cs, 
> > unsigned int clk_sel)
> >  {
> >   unsigned long tick_ps;
> >
> >   /* Calculate in picosecs to yield more exact results */
> > - tick_ps = gpmc_get_fclk_period();
> > + tick_ps = gpmc_get_clk_period(cs, clk_sel);
> >
> >   return (time_ns * 1000 + tick_ps - 1) / tick_ps;
> >  }
> >
> > +static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
> > +{
> > + return gpmc_ns_to_clk_ticks(time_ns, 0, GPMC_FCLK);
>
> This function should have been unchanged since we're dealing with GPMC_FCLK
> and it was using gpmc_get_fclk_period().


Which function? gpmc_ns_to_ticks? It still uses FCLK. I merely did not
want to have the picosecond formula in two places. I don't see a good
reason to do so now either...

>
> Let's call this GPMC_SET_ONE_CLKSEL()


Will do.


>
> You will also need to correct gpmc_cs_show_timings() to show the
> correct timing for "wait-monitoring-ns" based on GPMC_CLK.


I was working off 3.14, checked if it worked for 3.17, so both didn't
have gpmc_cs_show_timings. I'll take a look.

Regards,

Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/4] ARM OMAP2+ GPMC: add bus children

[Robert Abel]

Hi Roger,

On Tue, Feb 17, 2015 at 10:41 AM, Roger Quadros  wrote:>
> Can we simply use only
> of_platform_populate(child, NULL, NULL, &pdev->dev)
>
> That way we get rid of the if..else and let OF core take care of
> creating buses or devices.

Surely, you mean

>of_platform_populate(child, of_default_bus_match_table, NULL, &pdev->dev);

? If matches is NULL, we won't match any busses. Or did you mean to
just replace the second conditional?

I'll try that and see it works.

Regards,

Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Robert Abel]

Hi Roger,

On Tue, Feb 17, 2015 at 2:52 PM, Roger Quadros  wrote:
> nobody stops the DT binding from specifying a large enough 
> "gpmc,wait-monitoring-ns" value.
> The driver must use that to scale the GPMC_CLK if it doesn't fit in the 
> GPMC_FCLK.
> This feature can come separately though. So for now I was suggesting to set 
> the divisor to 1.
> [...]
> AFAIK "gpmc,sync-clk-ps" is not specified for asynchronous devices so it 
> defaults to 0
> in the driver.

As you have rightly pointed out, sync-clk-ps defaults to 0, i.e.
divider 1. My solution would work for people /now/ with different
gpmc,wait-monitoring-ns requirements. Of course, in general you're
right, the driver could compute that on its own. However, this
influences sampling behavior of the GPMC, which is somewhat strange
anyway. Since I lack a proper test setup and time to experiment with
the GPMC, I'd compromise on leaving sync-clk-ps default to 0, divider
defaults to 1. If somebody feels up to implementing driver-side
GPMC_CLK scaling, they might as well nix the dependency at that point
in time. Right now, keeping the dependency seems more useful to users
than killing it right away.

> What I'm stressing on is that there shouldn't be any dependency on 
> "gpmc,sync-clk-ps" for
> asynchronous devices. It also becomes easier to specify the 
> wait-monitoring-ns as we don't need
> to cross reference with "sync-clk-ps".

As an aside: There shouldn't be a dependency on the FCLK for
synchronous accesses either. The GPMC driver is in a somewhat terrible
state that synchronous protocols have to specify in ns, which get
scaled by the startup FCLK period... So this wrongful dependency
doesn't make my top ten, especially since it right now would fit a use
case.

Regards,

Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command

[Robert Abel]

On 13 Feb 2018 14:36, Andy Shevchenko wrote:
> I understand that we have a huge and hopefully nice library in the
> kernel, but the question still the same, what prevents a developer or
> maintainer to look at it from time to time?
> 
> For, I dare to say, ages we have hex_to_bin() and hex2bin().
> Can we use it?

hex_to_bin look fine to me, although personally I'm not a big fan of its
use of tolower.

The current parser implementation is much more lenient than hex2bin,
however. hex2bin won't parse strings containing illegal characters
(which are currently skipped) or hexadecimal strings with an odd number
of digits (which are currently allowed and the final digit will be ignored).

I noticed the only part of the code that does make use of library
functions, parsing x and y coordinates using kstrtoul, is broken.
Apparently it used to use simple_strtoul, which worked and then got
replaced. So apparently looking over the kernel lib from time to time
can also do some harm ;)
Patch incoming :)

Regards,

Robert

[PATCH 0/3] auxdisplay: charlcd: miscellaneous patches

[Robert Abel]

While looking at charlcd I noticed some little bits here and there that might
be corrected or improved.

Robert Abel (3):
  auxdisplay: charlcd: fix hex literal ranges for graphics command
  auxdisplay: charlcd: use null character instead of zero literal to
terminate strings
  auxdisplay: charlcd: replace octal literal with form-feed escape
sequence

 drivers/auxdisplay/charlcd.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.11.0

[PATCH 2/3] auxdisplay: charlcd: use null character instead of zero literal to terminate strings

[Robert Abel]

Using '\0' instead of plain 0 makes the intent clearer that this is indeed a 
string and not a series of integers.

Signed-off-by: Robert Abel 
---
 drivers/auxdisplay/charlcd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index 324d02f9f1c5..92549c8344a4 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -527,7 +527,7 @@ static void charlcd_write_char(struct charlcd *lcd, char c)
if ((c != '\n') && priv->esc_seq.len >= 0) {
/* yes, let's add this char to the buffer */
priv->esc_seq.buf[priv->esc_seq.len++] = c;
-   priv->esc_seq.buf[priv->esc_seq.len] = 0;
+   priv->esc_seq.buf[priv->esc_seq.len] = '\0';
} else {
/* aborts any previous escape sequence */
priv->esc_seq.len = -1;
@@ -536,7 +536,7 @@ static void charlcd_write_char(struct charlcd *lcd, char c)
case LCD_ESCAPE_CHAR:
/* start of an escape sequence */
priv->esc_seq.len = 0;
-   priv->esc_seq.buf[priv->esc_seq.len] = 0;
+   priv->esc_seq.buf[priv->esc_seq.len] = '\0';
break;
case '\b':
/* go back one char and clear it */
-- 
2.11.0

[PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command

[Robert Abel]

The graphics command expects 16 hexadecimal literals, but would allow 
characters in range [0-9a-zA-Z] instead of [0-9a-fA-F].

Signed-off-by: Robert Abel 
---
 drivers/auxdisplay/charlcd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index 642afd88870b..324d02f9f1c5 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -441,9 +441,9 @@ static inline int handle_lcd_special_code(struct charlcd 
*lcd)
shift ^= 4;
if (*esc >= '0' && *esc <= '9') {
value |= (*esc - '0') << shift;
-   } else if (*esc >= 'A' && *esc <= 'Z') {
+   } else if (*esc >= 'A' && *esc <= 'F') {
value |= (*esc - 'A' + 10) << shift;
-   } else if (*esc >= 'a' && *esc <= 'z') {
+   } else if (*esc >= 'a' && *esc <= 'f') {
value |= (*esc - 'a' + 10) << shift;
} else {
esc++;
-- 
2.11.0

[PATCH 3/3] auxdisplay: charlcd: replace octal literal with form-feed escape sequence

[Robert Abel]

There is no need to resort to octal escape sequence for the form feed character 
when an established escape sequence exists.

Signed-off-by: Robert Abel 
---
 drivers/auxdisplay/charlcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index 92549c8344a4..a3486db03d81 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -555,7 +555,7 @@ static void charlcd_write_char(struct charlcd *lcd, char c)
/* back one char again */
lcd->ops->write_cmd(lcd, LCD_CMD_SHIFT);
break;
-   case '\014':
+   case '\f':
/* quickly clear the display */
charlcd_clear_fast(lcd);
break;
-- 
2.11.0

Re: [PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command

[Robert Abel]

Hi Andy,

On 15 Feb 2018 11:57, Andy Shevchenko wrote:
> On Thu, Feb 15, 2018 at 1:17 AM, Robert Abel  wrote:
>> hex_to_bin look fine to me, although personally I'm not a big fan of its
>> use of tolower.
> 
> Let's duplicate then over and over?

I was speaking more generally about performance here. There is a reason
for kstrtox.c:57
(https://elixir.bootlin.com/linux/v4.14.7/source/lib/kstrtox.c#L57)
> unsigned int lc = c | 0x20; /* don't tolower() this line */

> Can you point to the documentation where user can easily (w/o reading
> the code) get how it suppose to be?

Unfortunately not. I read the code myself to know how it is supposed to
work. That's definitely a gap in documentation.

> On Thu, Feb 15, 2018 at 1:17 AM, Robert Abel  wrote:
>> I noticed the only part of the code that does make use of library
>> functions, parsing x and y coordinates using kstrtoul, is broken.
>> Apparently it used to use simple_strtoul, which worked and then got
>> replaced.
> By which commit?

129957069e6af42a6e021d90679c56662c95f7e1
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=129957069e6af42a6e021d90679c56662c95f7e1)

I'll try to answer to this email with relevant patches for charlcd.c.

Regards,

Robert

Re: [PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command

[Robert Abel]

The following patch set fixes the x/y coordinate addressing issue I mentioned 
earlier
as well as not marking the two-line command sequence as processed resulting in 
a confusing
state.

I implemented the logic around using kstrtoul by terminating the substrings 
that contain
numbers for each command separately and then restoring the command character 
following the
number that was overwritten.

Additionally, the code now doesn't write live to the x and y address fields and 
will
bail out if an unknown command character or an invalid number was encountered.

I did try to keep it backwards compatible, so sequences of the form 
^[[Lx0y1x2y4; will
still work (and result in x2y4).

Additionally, I noticed that the ^[[H home function was just resetting the x/y 
coordinates
but not the display shift, which results in a situation where the display 
cannot be
programmatically unshifted (except for init ^[[LI). So I implemented the HOME 
command 0x02
for charlcd_home. The old behavior can be simulated using ^[[Lx0y0; (just reset 
x/y, don't
unshift).

I tested on my Raspberry Pi Zero W with a clone 1602 display.

Robert Abel (4):
  auxdisplay: charlcd: fix two-line command ^[[LN not marked as
processed
  auxdisplay: charlcd: name x/y address struct
  auxdisplay: charlcd: fix x/y address commands
  auxdisplay: charlcd: make home command unshift display

 drivers/auxdisplay/charlcd.c | 76 ++--
 1 file changed, 60 insertions(+), 16 deletions(-)

-- 
2.11.0

Re: [PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command

[Robert Abel]

The following patch set fixes the x/y coordinate addressing issue I mentioned 
earlier
as well as not marking the two-line command sequence as processed resulting in 
a confusing
state.

I implemented the logic around using kstrtoul by terminating the substrings 
that contain
numbers for each command separately and then restoring the command character 
following the
number that was overwritten.

Additionally, the code now doesn't write live to the x and y address fields and 
will
bail out if an unknown command character or an invalid number was encountered.

I did try to keep it backwards compatible, so sequences of the form 
^[[Lx0y1x2y4; will
still work (and result in x2y4).

Additionally, I noticed that the ^[[H home function was just resetting the x/y 
coordinates
but not the display shift, which results in a situation where the display 
cannot be
programmatically unshifted (except for init ^[[LI). So I implemented the HOME 
command 0x02
for charlcd_home. The old behavior can be simulated using ^[[Lx0y0; (just reset 
x/y, don't
unshift).

I tested on my Raspberry Pi Zero W with a clone 1602 display.

Robert Abel (4):
  auxdisplay: charlcd: fix two-line command ^[[LN not marked as
processed
  auxdisplay: charlcd: name x/y address struct
  auxdisplay: charlcd: fix x/y address commands
  auxdisplay: charlcd: make home command unshift display

 drivers/auxdisplay/charlcd.c | 76 ++--
 1 file changed, 60 insertions(+), 16 deletions(-)

-- 
2.11.0

[PATCH 4/4] auxdisplay: charlcd: make home command unshift display

[Robert Abel]

A user has no way of unshifting the display programmatically once shifted.
Users cannot rely on ^[[H (home) to result in their message being seen either.
Use the actual HOME command 0x02 instead of only returning to x0/y0.
Users can still do ^[[Lx0y0; (go to x/y) if they wish to use the 'old' home
function.
Implement fast clearing of LCD by going to x0/y0 first, clearing display and
then calling home to possibly unshift display possibly avoiding artifacts by
not unshifting before clearing display.

Signed-off-by: Robert Abel 
---
 drivers/auxdisplay/charlcd.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index 24cabe88c7f0..41f9aa4a73d4 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -43,6 +43,8 @@
 /* LCD commands */
 #define LCD_CMD_DISPLAY_CLEAR  0x01/* Clear entire display */
 
+#define LCD_CMD_HOME0x02/* Set DDRAM address to 0 and unshift 
display */
+
 #define LCD_CMD_ENTRY_MODE 0x04/* Set entry mode */
 #define LCD_CMD_CURSOR_INC 0x02/* Increment cursor */
 
@@ -182,7 +184,8 @@ static void charlcd_home(struct charlcd *lcd)
 
priv->addr.x = 0;
priv->addr.y = 0;
-   charlcd_gotoxy(lcd);
+   lcd->ops->write_cmd(lcd, LCD_CMD_HOME);
+   long_sleep(2);
 }
 
 static void charlcd_print(struct charlcd *lcd, char c)
@@ -202,9 +205,12 @@ static void charlcd_print(struct charlcd *lcd, char c)
 
 static void charlcd_clear_fast(struct charlcd *lcd)
 {
+   struct charlcd_priv *priv = to_priv(lcd);
int pos;
 
-   charlcd_home(lcd);
+   priv->addr.x = 0;
+   priv->addr.y = 0;
+   charlcd_gotoxy(lcd);
 
if (lcd->ops->clear_fast)
lcd->ops->clear_fast(lcd);
-- 
2.11.0

[PATCH 2/4] auxdisplay: charlcd: name x/y address struct

[Robert Abel]

Signed-off-by: Robert Abel 
---
 drivers/auxdisplay/charlcd.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index e3b2fd15c5a3..a3d364e6c666 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -67,6 +67,11 @@
 #define LCD_ESCAPE_LEN 24  /* Max chars for LCD escape command */
 #define LCD_ESCAPE_CHAR27  /* Use char 27 for escape 
command */
 
+struct charlcd_priv_addr {
+   unsigned long int x;
+   unsigned long int y;
+};
+
 struct charlcd_priv {
struct charlcd lcd;
 
@@ -80,12 +85,9 @@ struct charlcd_priv {
unsigned long int flags;
 
/* Contains the LCD X and Y offset */
-   struct {
-   unsigned long int x;
-   unsigned long int y;
-   } addr;
+   struct charlcd_priv_addr addr;
 
-   /* Current escape sequence and it's length or -1 if outside */
+   /* Current escape sequence and its length or -1 if outside */
struct {
char buf[LCD_ESCAPE_LEN + 1];
int len;
-- 
2.11.0

[PATCH 1/4] auxdisplay: charlcd: fix two-line command ^[[LN not marked as processed

[Robert Abel]

Signed-off-by: Robert Abel 
---
 drivers/auxdisplay/charlcd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index a3486db03d81..e3b2fd15c5a3 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -362,6 +362,7 @@ static inline int handle_lcd_special_code(struct charlcd 
*lcd)
break;
case 'N':   /* Two Lines */
priv->flags |= LCD_FLAG_N;
+   processed = 1;
break;
case 'l':   /* Shift Cursor Left */
if (priv->addr.x > 0) {
-- 
2.11.0

[PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Robert Abel]

NUL-terminate each individual number to be parsed.
To do this, the next command character and a pointer to its argument
are found and stored. The command character is then overwritten by NUL
before kstr* functions are called on the buffer.

Signed-off-by: Robert Abel 
---
 drivers/auxdisplay/charlcd.c | 53 
 1 file changed, 44 insertions(+), 9 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index a3d364e6c666..24cabe88c7f0 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -471,28 +471,63 @@ static inline int handle_lcd_special_code(struct charlcd 
*lcd)
break;
}
case 'x':   /* gotoxy : LxXXX[yYYY]; */
-   case 'y':   /* gotoxy : LyYYY[xXXX]; */
+   case 'y': { /* gotoxy : LyYYY[xXXX]; */
+
+   char* nxt_esc;
+   char  nxt_cmd;
+   char  cmd;
+   struct charlcd_priv_addr tmp_addr;
+
if (!strchr(esc, ';'))
break;
 
-   while (*esc) {
-   if (*esc == 'x') {
-   esc++;
-   if (kstrtoul(esc, 10, &priv->addr.x) < 0)
+   /* sequence is processed whether legal or illegal */
+   processed = 1;
+
+   /* copy current address to temporary buffer */
+   tmp_addr = priv->addr;
+
+   nxt_cmd = *esc++;
+   nxt_esc = esc;
+
+   while ('\0' != *esc) {
+
+   cmd = nxt_cmd;
+   esc = nxt_esc;
+   nxt_esc = strpbrk(esc, "xy;");
+   if (NULL != nxt_esc) {
+   nxt_cmd = *nxt_esc;
+   /* terminate current sequence with NUL */
+   *nxt_esc++ = '\0';
+   }
+
+   if ('x' == cmd) {
+   if (kstrtoul(esc, 10, &tmp_addr.x) < 0)
break;
-   } else if (*esc == 'y') {
-   esc++;
-   if (kstrtoul(esc, 10, &priv->addr.y) < 0)
+   } else if ('y' == cmd) {
+   if (kstrtoul(esc, 10, &tmp_addr.y) < 0)
break;
} else {
+   /* break on unknown command or ';' */
break;
}
+
}
 
+   /* unknown commands in sequence will be followed by at least 
';' */
+   if ('\0' != *esc)
+   break;
+
+   /* clamp new x/y coordinates */
+   if (tmp_addr.x >= lcd->width)
+   tmp_addr.x = lcd->width - 1;
+   tmp_addr.y %= lcd->height;
+
+   priv->addr = tmp_addr;
charlcd_gotoxy(lcd);
-   processed = 1;
break;
}
+   }
 
/* TODO: This indent party here got ugly, clean it! */
/* Check whether one flag was changed */
-- 
2.11.0

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Robert Abel]

Hi Geert,

On 26 Feb 2018 09:46, Geert Uytterhoeven wrote:
>> +
>> +   nxt_cmd = *esc++;
>> +   nxt_esc = esc;
>> +
>> +   while ('\0' != *esc) {
>> +
>> +   cmd = nxt_cmd;
>> +   esc = nxt_esc;
>> +   nxt_esc = strpbrk(esc, "xy;");
>> +   if (NULL != nxt_esc) {
>> +   nxt_cmd = *nxt_esc;
>> +   /* terminate current sequence with NUL */
>> +   *nxt_esc++ = '\0';
>> +   }
> 
> So if none of "x", "y", or ";" is found, nxt_cmd will still contain
> the current command?
> Shouldn't it be reset to '\0' or so?

I originally had a comment there, but it then felt kinda silly.
Since one of the conditions of the code is that a semicolon ';' is
detected at the end of the stream, the situation you describe never
occurs until the end of the command string.

nxt_esc == NULL only when cmd == ';' and *esc == NULL. Since there is
nothing to terminate and the code won't run again (due to while (*esc)),
I didn't reset nxt_cmd. Other than that, I also don't know that there is
a sensible reset value.

Regards,

Robert

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Robert Abel]

Hi Andy, Hi Miguel,

On 26 Feb 2018 12:44, Andy Shevchenko wrote:
> Can we avoid yoda style of programming?
On 26 Feb 2018 17:49, Miguel Ojeda wrote:
> Please do not change the style of the code w.r.t to the rest of the
> file, which writes tests with the non-lvalue on the right-hand side
> and do not compare against '\0'. Same for the rest.

I am actually a fan of yoda-style programming, although its value is
diminished with modern IDEs, which we all use, right?

On 26 Feb 2018 17:54, Miguel Ojeda wrote:
> On Mon, Feb 26, 2018 at 12:44 PM, Andy Shevchenko
>> Perhaps instead of dancing around kstrtox() better to switch to
>> simple_strtoul() ?
>
> It seems deprecated:
>
> /* Obsolete, do not use.  Use kstrto instead */
> extern unsigned long simple_strtoul(const char *,char **,unsigned int);

I thought the whole point was that simple_strtoul is deprecated and on
the kill list? Isn't that kind of against the whole argument of
re-inventing the wheel?
If using simple_strtoul is an option, it might be best to bring it back
and not touch the buffer at all.

Regards,

Robert

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Robert Abel]

On 26 Feb 2018 17:49, Miguel Ojeda wrote:
> On a general note, the code seems a bit convoluted for what it does,
> specially without the comment written in the commit message :-) Isn't
> it simpler to use a tiny array in the stack and put the numbers to be
> converted instead of modifying the input sequence and dancing with
> pointers?

That's what I felt at first, too. If we can drop the backwards
compatibility of repeated xy commands, the whole affair gets much
easier, but will unfortunately break existing use.

Ex. ^[[Lx004y002x006; --> x6y2, because repeats of x would just
overwrite earlier values. That's what the while loop allowed in the
first place.

I suspect the while loop to parse was just a clever way of parsing y
followed by x and x followed by y using the same code and the
overwriting behavior is actually an unaccounted-for side-effect.

Regards,

Robert

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Robert Abel]

Hi @ll,

this is a discussion stemming from drivers/auxdisplay about the usage
and possible deprecation of simple_strto* set of functions found in
lib/vsprintf.
I cc'ed everybody who signed off and also everybody who
signed/authored/added/removed more than 20% of lib/vsprintf.

This has come up, because some auxdisplay functionality was broken by a
well-intentioned patch to switch from simple_strtoul to kstrtoul in
129957069e6af42a6e021d90679c56662c95f7e1
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=129957069e6af42a6e021d90679c56662c95f7e1)
and now there is discussion about whether the simple_strto* functions
are really obsolete.

On 26 Feb 2018 18:26, Miguel Ojeda wrote:
> On Mon, Feb 26, 2018 at 6:09 PM, Andy Shevchenko wrote:
>> On Mon, Feb 26, 2018 at 6:54 PM, Miguel Ojeda wrote:
>>> On Mon, Feb 26, 2018 at 12:44 PM, Andy Shevchenko wrote:
 Perhaps instead of dancing around kstrtox() better to switch to
 simple_strtoul() ?
>>>
>>> It seems deprecated:
>>>
>>> /* Obsolete, do not use.  Use kstrto instead */
>>> extern unsigned long simple_strtoul(const char *,char **,unsigned int);
>>
>> It has been discussed several times. The comment is simple wrong.
>>
>> Because of the requirement of kstrtox() to have a \0 or \n followed by
>> \0 as "end of field".
>> simple_strto*() is suitable to be run in place.
> 
> I agree that in-place versions of these kind of string functions are
> very useful, don't get me wrong! But unless someone changes the
> "official" comment, we shouldn't add new code relying on them.

So can we get clarity in form of a patch here, or not?
I'm hesitant to move back to simple_kstroul, because to me it seems that
general consensus (at least when the comment was put there) was to
remove it:

> commit 462e471107624fe9bd8b6353ac13e06305c3f3fd
> Author: Eldad Zack <>
> Date:   Mon Dec 17 16:03:05 2012 -0800
> 
> simple_strto*: annotate function as obsolete
> 
> Update the documentation for simple_strto* to reflect that it has been
> obsoleted and advise the usage of kstrto*.
> 
> Signed-off-by: Eldad Zack <>
> Cc: J. Bruce Fields <>
> Cc: Joe Perches <>
> Cc: Randy Dunlap <>
> Cc: Alexey Dobriyan <>
> Cc: Rob Landley <>
> Signed-off-by: Andrew Morton <>
> Signed-off-by: Linus Torvalds <>

One big advantage simple_kstro* functions have over their kstrto*
cousins is that strings don't need to be null-terminated. Instead, they
will be parsed as far as they can be and a pointer to the end of the
respective number will be returned.

Regards,

Robert

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Robert Abel]

Hi Miguel,

On 26 Feb 2018 17:49, Miguel Ojeda wrote:
> On Mon, Feb 26, 2018 at 12:54 AM, Robert Abel  wrote:
>> +   /* clamp new x/y coordinates */
>> +   if (tmp_addr.x >= lcd->width)
>> +   tmp_addr.x = lcd->width - 1;
> 
> tmp_addr.x = min(tmp_addr.x, lcd->width - 1);

Had throught of that, too. However, it introduces a warning about type
mismatch, because lcd->width is int while tmp_addr.x is unsigned long
int. I didn't fell like saving a line warranted much bigger changes to
the lcd struct.

Regards,

Robert

Re: [PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command

[Robert Abel]

On 26 Feb 2018 00:54, Robert Abel wrote:> Robert Abel (4):
>   auxdisplay: charlcd: fix two-line command ^[[LN not marked as
> processed
>   auxdisplay: charlcd: name x/y address struct
>   auxdisplay: charlcd: fix x/y address commands
>   auxdisplay: charlcd: make home command unshift display

So I think as it stands, only the first patch is being picked up, while
the other three will need rework, which is fine.

I think I'm going to wait a little while with the x/y fix until the
simple_strto* situation has settled.

For the home command basically only more comments in code should be
necessary.
More comments will also be good for the x/y issue once I change the
implementation.

Regards,

Robert

Re: [PATCH RFC v3] auxdisplay: charlcd: Fix and clean up handling of x/y commands

[Robert Abel]

On 27 Feb 2018 23:09, Miguel Ojeda wrote:> @@ -469,24 +543,11 @@ static
inline int handle_lcd_special_code(struct charlcd *lcd)
>   }
>   case 'x':   /* gotoxy : LxXXX[yYYY]; */
>   case 'y':   /* gotoxy : LyYYY[xXXX]; */
> - if (!strchr(esc, ';'))
> - break;

Might want to keep this. It's in line with all other cases and prevents
calling parse_xy with input that has no chance of being correct due to
missing final ';'.

> + /* If the command is valid, move to the new address */
> + if (parse_xy(esc, &priv->addr.x, &priv->addr.y))
> + charlcd_gotoxy(lcd);

While not in the original code, the inputs are now not clamped to width
and height.
That means for a two-line display ^[[Ly02; will actually end up on line
y = 1, not y = 2 % 2 = 0, because the four-line display logic bumps the
address up.
The same goes for going over lcd->width/lcd->bwidth in the x coordinate.
For four-line displays that ends up going to the line y + 2, because the
buffer is split in the middle.
The distinction between lcd->width and lcd->bwidth depends on whether it
makes sense to put the cursor outside the visible area or not.

Regards,

Robert

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Robert Abel]

Hi Willy,

On 27 Feb 2018 06:19, Willy Tarreau wrote:
> Well actually I don't see a problem there at all. The principle is simply
> to accept any sequence assigning x or y or both. If you write x4y2x6, it
> simply means that you changed your mind regarding x and that the last
> value (6) is the one you want. Just as if you wrote "^[[Lx4;^[[y2;^[[x6;".
> The while loop doesn't even try to do anything clever, it simply parses
> everything matching x and y followed by digits. I think the only reason
> for having both x and y processed in the same loop was to call
> charlcd_gotoxy() only once for both axes.

I didn't say it is a problem. It is however an edge case that incurs a
lot of code for little to no functionality.
I'd much prefer if we broke backwards compatibility here and actually
only parse the format that is indicated in the comment:

> case 'x':   /* gotoxy : LxXXX[yYYY]; */
> case 'y':   /* gotoxy : LyYYY[xXXX]; */
>  

Exactly one x command followed exactly by zero or one y command or
vice-versa.

If somebody changes their mind during the escape sequence, they can just
issue a new one instead of appending to the current one.

I'll post an example patch.

Regards,

Robert

[PATCH RFC 0/2] auxdisplay: charlcd: fix movement and home commands

[Robert Abel]

I reworked the previous patches 2-4 into these two patches.
The first patch proproses a different solution to the movement command
parse code that reduces the movement command two at most two movement
subcommands (x or y standalone, x followed by y or vice-versa).

The second patch add comments in code to the home and clear_fast functions
and explains their behavior a bit more.

Robert Abel (2):
  auxdisplay: charlcd: fix x/y address commands
  auxdisplay: charlcd: make home command unshift display

 drivers/auxdisplay/charlcd.c | 137 ++-
 1 file changed, 122 insertions(+), 15 deletions(-)

-- 
2.11.0

[PATCH 2/2] auxdisplay: charlcd: make home command unshift display

[Robert Abel]

A user has no way of unshifting the display programmatically once shifted.
Users cannot rely on ^[[H (home) to result in their message being seen either.
Use the actual HOME command 0x02 instead of only returning to x0/y0.
Users can still do ^[[Lx0y0; (go to x/y) if they wish to use the 'old' home
function.
Implement fast clearing of LCD by going to x0/y0 first, clearing display and
then calling home to possibly unshift display possibly avoiding artifacts by
not unshifting before clearing display.

Signed-off-by: Robert Abel 
---
 drivers/auxdisplay/charlcd.c | 28 ++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index ae078f414539..fdb3aa6423bf 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -43,6 +43,8 @@
 /* LCD commands */
 #define LCD_CMD_DISPLAY_CLEAR  0x01/* Clear entire display */
 
+#define LCD_CMD_HOME0x02/* Set DDRAM address to 0 and unshift 
display */
+
 #define LCD_CMD_ENTRY_MODE 0x04/* Set entry mode */
 #define LCD_CMD_CURSOR_INC 0x02/* Increment cursor */
 
@@ -174,13 +176,24 @@ static void charlcd_gotoxy(struct charlcd *lcd)
lcd->ops->write_cmd(lcd, LCD_CMD_SET_DDRAM_ADDR | addr);
 }
 
+/**
+ * charlcd_home() - Return to DDRAM address 0 and unshift the display.
+ * @lcd: LCD descriptor structure.
+ *
+ * Return to coordinates x0/y0 and unshift the display.
+ *
+ * Note: Use the movement command ^[[Lx0y0; instead of home
+ * in case the display should not be unshifted.
+ */
 static void charlcd_home(struct charlcd *lcd)
 {
struct charlcd_priv *priv = to_priv(lcd);
 
+   /* return to x0/y0 and unshift the display */
priv->addr.x = 0;
priv->addr.y = 0;
-   charlcd_gotoxy(lcd);
+   lcd->ops->write_cmd(lcd, LCD_CMD_HOME);
+   long_sleep(2);
 }
 
 static void charlcd_print(struct charlcd *lcd, char c)
@@ -198,11 +211,21 @@ static void charlcd_print(struct charlcd *lcd, char c)
charlcd_gotoxy(lcd);
 }
 
+/**
+ * charlcd_clear_fast() - Perform a fast clear of the display and return home.
+ * @lcd: LCD descriptor structure.
+ *
+ * The display will be unshifted and at coordinates x0/y0 after fast clear.
+ */
 static void charlcd_clear_fast(struct charlcd *lcd)
 {
+   struct charlcd_priv *priv = to_priv(lcd);
int pos;
 
-   charlcd_home(lcd);
+   /* avoid artifacts by going to x0/y0 without unshifting the display */
+   priv->addr.x = 0;
+   priv->addr.y = 0;
+   charlcd_gotoxy(lcd);
 
if (lcd->ops->clear_fast)
lcd->ops->clear_fast(lcd);
@@ -210,6 +233,7 @@ static void charlcd_clear_fast(struct charlcd *lcd)
for (pos = 0; pos < min(2, lcd->height) * lcd->hwidth; pos++)
lcd->ops->write_data(lcd, ' ');
 
+   /* return to x0/y0 and unshift the display */
charlcd_home(lcd);
 }
 
-- 
2.11.0

[PATCH 1/2] auxdisplay: charlcd: fix x/y address commands

[Robert Abel]

The current version does not parse x/y commands at all.
Simplify the x/y command syntax to the one indicated in
the comment all along and introduce a parsing function
that handles parsing a sequence of one or two subcommands
where each subcommand must appear at most once.

Signed-off-by: Robert Abel 
---
 drivers/auxdisplay/charlcd.c | 109 +--
 1 file changed, 96 insertions(+), 13 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index e3b2fd15c5a3..ae078f414539 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -292,6 +292,96 @@ static int charlcd_init_display(struct charlcd *lcd)
return 0;
 }
 
+/**
+ * parse_xy() - Parse coordinates of a movement command.
+ * @s: Pointer to null-terminated movement command string.
+ * @x: Pointer to x position.
+ * @y: Pointer to y position.
+ *
+ * Parses a movement command of the form "([xy][0-9]+){1,2};",
+ * where each group must begin with a different subcommand.
+ *
+ * The positions will only be updated when their respective
+ * subcommand is encountered and when the whole movement
+ * command is valid.
+ *
+ * The movement command string must contain a ';' at the end.
+ *
+ * For instance:
+ *   - ";"  fails.
+ *   - "x1;"returns (1, ).
+ *   - "y2x1;"  returns (1, 2).
+ *   - "x12y34x56;" fails.
+ *   - ""   fails.
+ *   - "x"  illegal input.
+ *   - "x;" fails.
+ *   - "x1" illegal input.
+ *   - "xy12;"  fails.
+ *   - "x12yy12;"   fails.
+ *   - "xx" illegal input.
+ *
+ * Return: Returns whether the command is valid. The position arguments are
+ * only written if the parsing was successful.
+ */
+static bool parse_xy(const char *s, unsigned long *x, unsigned long *y)
+{
+
+   unsigned long new_x = *x;
+   unsigned long new_y = *y;
+
+   char xcoord[LCD_ESCAPE_LEN];
+   char ycoord[LCD_ESCAPE_LEN];
+   const char *split = strpbrk(s + 1, "xy");
+   const char *end = strchr(s, ';');
+   char *coord0 = xcoord;
+   char *coord1 = ycoord;
+   unsigned long *new0 = &new_x;
+   unsigned long *new1 = &new_y;
+
+   memset(xcoord, 0, sizeof(xcoord));
+   memset(ycoord, 0, sizeof(ycoord));
+
+   /* validate input */
+   switch (*s) {
+   case 'x':
+   if (split != NULL && *split != 'y')
+   return false;
+   break;
+   case 'y':
+   /* swap coordinates */
+   coord0 = ycoord;
+   coord1 = xcoord;
+   new0 = &new_y;
+   new1 = &new_x;
+
+   if (split != NULL && *split != 'x')
+   return false;
+   break;
+   default:
+   return false;
+   }
+
+   /* parse coordinate 0 and 1 */
+   if (split == NULL) {
+   memcpy(coord0, s + 1, end - s - 1);
+   if (kstrtoul(coord0, 10, new0) < 0)
+   return false;
+   } else {
+   memcpy(coord0, s + 1, split - s - 1);
+   memcpy(coord1, split + 1, end - split - 1);
+   if (kstrtoul(coord0, 10, new0) < 0)
+   return false;
+   if (kstrtoul(coord1, 10, new1) < 0)
+   return false;
+   }
+
+   /* update coordinates on success */
+   *x = new_x;
+   *y = new_y;
+   return true;
+
+}
+
 /*
  * These are the file operation function for user access to /dev/lcd
  * This function can also be called from inside the kernel, by
@@ -473,21 +563,14 @@ static inline int handle_lcd_special_code(struct charlcd 
*lcd)
if (!strchr(esc, ';'))
break;
 
-   while (*esc) {
-   if (*esc == 'x') {
-   esc++;
-   if (kstrtoul(esc, 10, &priv->addr.x) < 0)
-   break;
-   } else if (*esc == 'y') {
-   esc++;
-   if (kstrtoul(esc, 10, &priv->addr.y) < 0)
-   break;
-   } else {
-   break;
-   }
+   /* If the command is valid, move to the new address */
+   if (parse_xy(esc, &priv->addr.x, &priv->addr.y)) {
+   priv->addr.x = min_t(unsigned long, priv->addr.x, 
lcd->bwidth - 1);
+   priv->addr.y %= lcd->height;
+   charlcd_gotoxy(lcd);
}
 
-   charlcd_gotoxy(lcd);
+   /* Regardless of its validity, mark as processed */
processed = 1;
break;
}
-- 
2.11.0