Re: [PATCH] auxdisplay: charlcd: fix x/y command parsing
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
Re: [PATCH] auxdisplay: charlcd: fix x/y command parsing
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
[of] deadlock b/c of gpiochip and pinctrl dependency
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(>gpio_chip, pc);
if (err) {
dev_err(dev, "could not add GPIO chipn");
return err;
}
...
pc->pctl_dev = devm_pinctrl_register(dev, _pinctrl_desc, pc);
if (IS_ERR(pc->pctl_dev)) {
gpiochip_remove(>gpio_chip);
return PTR_ERR(pc->pctl_dev);
}
...
pinctrl_add_gpio_range(pc->pctl_dev, >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 = <>;
__overlay__ {
myLed {
gpios = <18 GPIO_ACTIVE_HIGH>;
gpio-hog;
output-low;
};
};
};
};
[of] deadlock b/c of gpiochip and pinctrl dependency
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(>gpio_chip, pc);
if (err) {
dev_err(dev, "could not add GPIO chipn");
return err;
}
...
pc->pctl_dev = devm_pinctrl_register(dev, _pinctrl_desc, pc);
if (IS_ERR(pc->pctl_dev)) {
gpiochip_remove(>gpio_chip);
return PTR_ERR(pc->pctl_dev);
}
...
pinctrl_add_gpio_range(pc->pctl_dev, >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 = <>;
__overlay__ {
myLed {
gpios = <18 GPIO_ACTIVE_HIGH>;
gpio-hog;
output-low;
};
};
};
};
[PATCH 1/2] auxdisplay: charlcd: fix x/y address commands
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 <ra...@robertabel.eu>
---
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 = _x;
+ unsigned long *new1 = _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 = _y;
+ new1 = _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, >addr.x) < 0)
- break;
- } else if (*esc == 'y') {
- esc++;
- if (kstrtoul(esc, 10, >addr.y) < 0)
- break;
- } else {
- break;
- }
+ /* If the command is valid, move to the new address */
+ if (parse_xy(esc, >addr.x, >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
[PATCH 1/2] auxdisplay: charlcd: fix x/y address commands
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 = _x;
+ unsigned long *new1 = _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 = _y;
+ new1 = _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, >addr.x) < 0)
- break;
- } else if (*esc == 'y') {
- esc++;
- if (kstrtoul(esc, 10, >addr.y) < 0)
- break;
- } else {
- break;
- }
+ /* If the command is valid, move to the new address */
+ if (parse_xy(esc, >addr.x, >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
[PATCH 2/2] auxdisplay: charlcd: make home command unshift display
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 <ra...@robertabel.eu>
---
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 2/2] auxdisplay: charlcd: make home command unshift display
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 RFC 0/2] auxdisplay: charlcd: fix movement and home commands
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 RFC 0/2] auxdisplay: charlcd: fix movement and home commands
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
Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
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
Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
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
Re: [PATCH RFC v3] auxdisplay: charlcd: Fix and clean up handling of x/y commands
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, >addr.x, >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 RFC v3] auxdisplay: charlcd: Fix and clean up handling of x/y commands
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, >addr.x, >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 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command
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 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command
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 3/4] auxdisplay: charlcd: fix x/y address commands
Hi Miguel, On 26 Feb 2018 17:49, Miguel Ojeda wrote: > On Mon, Feb 26, 2018 at 12:54 AM, Robert Abel <ra...@robertabel.eu> 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 3/4] auxdisplay: charlcd: fix x/y address commands
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 3/4] auxdisplay: charlcd: fix x/y address commands
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
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
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
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
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
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
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
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
[PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
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 <ra...@robertabel.eu>
---
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, >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, _addr.x) < 0)
break;
- } else if (*esc == 'y') {
- esc++;
- if (kstrtoul(esc, 10, >addr.y) < 0)
+ } else if ('y' == cmd) {
+ if (kstrtoul(esc, 10, _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
[PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
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, >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, _addr.x) < 0)
break;
- } else if (*esc == 'y') {
- esc++;
- if (kstrtoul(esc, 10, >addr.y) < 0)
+ } else if ('y' == cmd) {
+ if (kstrtoul(esc, 10, _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
[PATCH 1/4] auxdisplay: charlcd: fix two-line command ^[[LN not marked as processed
Signed-off-by: Robert Abel <ra...@robertabel.eu>
---
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 1/4] auxdisplay: charlcd: fix two-line command ^[[LN not marked as processed
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 2/4] auxdisplay: charlcd: name x/y address struct
Signed-off-by: Robert Abel <ra...@robertabel.eu>
---
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 2/4] auxdisplay: charlcd: name x/y address struct
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 4/4] auxdisplay: charlcd: make home command unshift display
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 <ra...@robertabel.eu>
---
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 4/4] auxdisplay: charlcd: make home command unshift display
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
Re: [PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command
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
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
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
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
Hi Andy, On 15 Feb 2018 11:57, Andy Shevchenko wrote: > On Thu, Feb 15, 2018 at 1:17 AM, Robert Abel <ra...@robertabel.eu> 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 <ra...@robertabel.eu> 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
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
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
Re: [PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command
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 3/3] auxdisplay: charlcd: replace octal literal with form-feed escape sequence
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 <ra...@robertabel.eu> --- 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
[PATCH 3/3] auxdisplay: charlcd: replace octal literal with form-feed escape sequence
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
[PATCH 2/3] auxdisplay: charlcd: use null character instead of zero literal to terminate strings
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 <ra...@robertabel.eu>
---
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
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 <ra...@robertabel.eu>
---
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 2/3] auxdisplay: charlcd: use null character instead of zero literal to terminate strings
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
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 0/3] auxdisplay: charlcd: miscellaneous patches
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 0/3] auxdisplay: charlcd: miscellaneous patches
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
of: dynamic: resource: add WARN in release_resource for DT overlays
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
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 = >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/
of: dynamic: resource: add WARN in release_resource for DT overlays
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
Signed-off-by: Robert ABEL ra...@cit-ec.uni-bielefeld.de
---
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
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/
[PATCH] net: rfkill-regulator: fix compiler warning
pdata char* name = const char* name
Signed-off-by: Robert ABEL ra...@cit-ec.uni-bielefeld.de
---
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 linux/rfkill.h
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 0/8] ARM OMAP2+ GPMC: fixes and bus children
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/
Re: [PATCH 2/8] ARM OMAP2+ GPMC: add bus children
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
Hi Roger, On Tue, Mar 3, 2015 at 1:55 PM, Roger Quadros rog...@ti.com 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/
Re: [PATCH 2/8] ARM OMAP2+ GPMC: add bus children
Hi Roger, On Tue, Mar 3, 2015 at 11:09 AM, Roger Quadros rog...@ti.com 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/
[PATCH 7/8 v4] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug
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",
name,
[PATCH 6/8 v4] ARM OMAP2+ GPMC: calculate GPMCFCLKDIVIDER based on WAITMONITORINGTIME
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(, 0, sizeof(struct gpmc_settings));
if (gpmc_nand_data->of_node)
gpmc_read_settings_dt(gpmc_nand_data->of_node, );
@@ -111,6 +103,15 @@ int gpmc_nand_init(struct omap_nand_platform_data
*gpmc_nand_data,
gpmc_set_legacy(gpmc_nand_data, );
s.device_nand = true;
+
+ 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;
+ }
+ }
+
err = gpmc_cs_program_settings(gpmc_nand_data->cs, );
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, );
+ ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, , _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, );
+ ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, , _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(, _async, _t);
- return gpmc_cs_set_timings(async_cs, );
+ return gpmc_cs_set_timings(async_cs, , _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(, _sync, _t);
- return gpmc_cs_set_timings(sync_cs, );
+ return gpmc_cs_set_timings(sync_cs, , _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->field, #field) < 0) \
return -1
+/**
+ * gpmc_calc_waitmonitoring_divider - calculate proper GPMCFCLKDIVIDER based
on WAITMONIT
[PATCH 8/8 v4] ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters
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 1/8 v4] ARM OMAP2+ GPMC: don't undef DEBUG
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 v4] ARM OMAP2+ GPMC: fixes and bus children
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
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, >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 4/8 v4] ARM OMAP2+ GPMC: change get_gpmc_timing_reg output for DTS
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 2/8 v4] ARM OMAP2+ GPMC: add bus children
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, >dev)) - return 0; + + /* create platform device, NULL on error or when disabled */ + if (!of_platform_device_create(child, NULL, >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, >dev)) + goto err_child_fail; + + return 0; + +err_child_fail: dev_err(>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 3/8 v4] ARM OMAP2+ GPMC: fix debug output alignment
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
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, >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/
Re: [PATCH 4/8] ARM OMAP2+ GPMC: change get_gpmc_timing_reg output for DTS
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 4/8] ARM OMAP2+ GPMC: change get_gpmc_timing_reg output for DTS
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
Hi Roger, On Fri, Feb 27, 2015 at 11:43 AM, Roger Quadros rog...@ti.com 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
On Thu, Feb 26, 2015 at 4:06 PM, Sergei Shtylyov sergei.shtyl...@cogentembedded.com 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
Hi Roger, On Fri, Feb 27, 2015 at 11:24 AM, Roger Quadros rog...@ti.com 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 7/8 v4] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug
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 ra...@cit-ec.uni-bielefeld.de
---
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 = value
* tick format: gpmc,name = value /zwj;* x ns -- y ns; x ticks
*zwj;/
* 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
[PATCH 0/8 v4] ARM OMAP2+ GPMC: fixes and bus children
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
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 ra...@cit-ec.uni-bielefeld.de
---
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
OMAP2+ GPMC driver undefines DEBUG, which makes it unnecessarily hard to turn DEBUG on. Remove the offending lines. Signed-off-by: Robert ABEL ra...@cit-ec.uni-bielefeld.de --- 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 linux/irq.h #include linux/kernel.h #include linux/init.h -- 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
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 ra...@cit-ec.uni-bielefeld.de
---
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-field, #field) 0) \
return -1
+/**
+ * gpmc_calc_waitmonitoring_divider - calculate proper GPMCFCLKDIVIDER based
on WAITMONITORINGTIME
+ * WAITMONITORINGTIME will be _at least_ as long as desired, i.e.
+ * read -- don't sample bus
[PATCH 8/8 v4] ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters
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 ra...@cit-ec.uni-bielefeld.de
---
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 = value
* tick format: gpmc,name = value /zwj;* x ns -- y ns; x ticks
*zwj;/
* 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, 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
[PATCH 3/8 v4] ARM OMAP2+ GPMC: fix debug output alignment
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 ra...@cit-ec.uni-bielefeld.de --- 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
This patch adds support for spawning buses as children of the GPMC. Signed-off-by: Robert ABEL ra...@cit-ec.uni-bielefeld.de --- 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 linux/of_address.h #include linux/of_mtd.h #include linux/of_device.h +#include linux/of_platform.h #include linux/omap-gpmc.h #include linux/mtd/nand.h #include linux/pm_runtime.h @@ -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
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 ra...@cit-ec.uni-bielefeld.de
---
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 = value
+ * tick format: gpmc,name = value /zwj;* x ns -- y ns; x ticks
*zwj;/
+ * 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 4/8] ARM OMAP2+ GPMC: fix debug output alignment
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] ARM OMAP2+ GPMC: add bus children
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, >dev)) - return 0; + + /* create platform device, NULL on error or when disabled */ + if (!of_platform_device_create(child, NULL, >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, >dev)) + goto err_child_fail; + + return 0; + +err_child_fail: dev_err(>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: change get_gpmc_timing_reg output for DTS
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 6/8] ARM OMAP2+ GPMC: calculate GPMCFCLKDIVIDER based on WAITMONITORINGTIME
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(, 0, sizeof(struct gpmc_settings));
if (gpmc_nand_data->of_node)
gpmc_read_settings_dt(gpmc_nand_data->of_node, );
@@ -111,6 +103,15 @@ int gpmc_nand_init(struct omap_nand_platform_data
*gpmc_nand_data,
gpmc_set_legacy(gpmc_nand_data, );
s.device_nand = true;
+
+ 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;
+ }
+ }
+
err = gpmc_cs_program_settings(gpmc_nand_data->cs, );
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, );
+ ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, , _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, );
+ ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, , _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(, _async, _t);
- return gpmc_cs_set_timings(async_cs, );
+ return gpmc_cs_set_timings(async_cs, , _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(, _sync, _t);
- return gpmc_cs_set_timings(sync_cs, );
+ return gpmc_cs_set_timings(sync_cs, , _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->field, #field) < 0) \
return -1
+/**
+ * gpmc_calc_waitmonitoring_divider - calculate proper GPMCFCLKDIVIDER based
on WAITMONIT
[PATCH 5/8] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER
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, >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] ARM OMAP2+ GPMC: fixes and bus children
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 7/8] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug
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
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 8/8] ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters
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 4/8] ARM OMAP2+ GPMC: change get_gpmc_timing_reg output for DTS
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 ra...@cit-ec.uni-bielefeld.de
---
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 = value
+ * tick format: gpmc,name = value /zwj;*(x ns -- y ns]; x ticks
*zwj;/
+ * 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
This patch adds support for spawning buses as children of the GPMC. Signed-off-by: Robert ABEL ra...@cit-ec.uni-bielefeld.de --- 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 linux/of_address.h #include linux/of_mtd.h #include linux/of_device.h +#include linux/of_platform.h #include linux/omap-gpmc.h #include linux/mtd/nand.h #include linux/pm_runtime.h @@ -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
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 ra...@cit-ec.uni-bielefeld.de --- 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 6/8] ARM OMAP2+ GPMC: calculate GPMCFCLKDIVIDER based on WAITMONITORINGTIME
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 ra...@cit-ec.uni-bielefeld.de
---
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-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
[PATCH 5/8] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER
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 ra...@cit-ec.uni-bielefeld.de
---
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 7/8] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug
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 ra...@cit-ec.uni-bielefeld.de
---
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 = value
* tick format: gpmc,name = value /zwj;*(x ns -- y ns]; x ticks
*zwj;/
* 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
OMAP2+ GPMC driver undefines DEBUG, which makes it unnecessarily hard to turn DEBUG on. Remove the offending lines. Signed-off-by: Robert ABEL ra...@cit-ec.uni-bielefeld.de --- 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 linux/irq.h #include linux/kernel.h #include linux/init.h -- 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/
