[PATCH 0/6] rtc: add rtc_{read,write}8_array and rtc command

[PATCH 0/6] rtc: add rtc_{read,write}8_array and rtc command

[Rasmus Villemoes]

I need access to registers other than just the timekeeping ones of the
pcf2127, so I wanted to implement ->read8 and ->write8. But for
testing these it appeared there was no convenient way to invoke those
from the shell, so I also ended up adding such a command.

Also, it seemed more natural to provide array variants that can read
or write several registers at once, so rtc_ops is expanded a bit.

There are a few things one could do on top, but for now I just want
some feedback, especially on the new _array methods. "rtc set", "rtc
get" and "rtc reset" are rather obvious subsommands to add at some
point. Also, rtc_{read,write}{16,32} can be simplified a bit, along
the lines of

__le16 v;
int ret = rtc_read8_array(dev, reg, &v, 2);
if (ret)
  return ret;
*valuep = __le16_to_cpu(v);
return 0;

Rasmus Villemoes (6):
  rtc: add rtc_read8_array helper and ->read8_array method
  rtc: add rtc_write8_array() helper
  rtc: fall back to ->{read,write}8_array if ->{read,write}8 are not
    provided
  rtc: pcf2127: provide ->read8_array method
  rtc: pcf2127: provide ->write8_array method
  rtc: add rtc command

 cmd/Kconfig              |   6 ++
 cmd/Makefile             |   1 +
 cmd/rtc.c                | 153 +++++++++++++++++++++++++++++++++++++++
 drivers/rtc/pcf2127.c    |  14 +++-
 drivers/rtc/rtc-uclass.c |  53 +++++++++++++-
 include/rtc.h            |  48 ++++++++++++
 6 files changed, 270 insertions(+), 5 deletions(-)
 create mode 100644 cmd/rtc.c

-- 
2.23.0

[PATCH 1/6] rtc: add rtc_read8_array helper and ->read8_array method

[Rasmus Villemoes]

Some users may want to read multiple consecutive 8-bit
registers. Instead of each caller having to implement the loop,
provide a rtc_read8_array() helper. Also, allow a driver to provide a
read8_array method, which can be more efficient than reading one
register at a time.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/rtc/rtc-uclass.c | 19 +++++++++++++++++++
 include/rtc.h            | 24 ++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/drivers/rtc/rtc-uclass.c b/drivers/rtc/rtc-uclass.c
index a0a238aedd..5070fb416d 100644
--- a/drivers/rtc/rtc-uclass.c
+++ b/drivers/rtc/rtc-uclass.c
@@ -49,6 +49,25 @@ int rtc_read8(struct udevice *dev, unsigned int reg)
 	return ops->read8(dev, reg);
 }
 
+int rtc_read8_array(struct udevice *dev, unsigned int reg,
+		    u8 *buf, unsigned int len)
+{
+	struct rtc_ops *ops = rtc_get_ops(dev);
+
+	assert(ops);
+	if (ops->read8_array)
+		return ops->read8_array(dev, reg, buf, len);
+	if (!ops->read8)
+		return -ENOSYS;
+	while (len--) {
+		int ret = ops->read8(dev, reg++);
+		if (ret < 0)
+			return ret;
+		*buf++ = ret;
+	}
+	return 0;
+}
+
 int rtc_write8(struct udevice *dev, unsigned int reg, int val)
 {
 	struct rtc_ops *ops = rtc_get_ops(dev);
diff --git a/include/rtc.h b/include/rtc.h
index 8aabfc1162..f7f622c1db 100644
--- a/include/rtc.h
+++ b/include/rtc.h
@@ -64,6 +64,18 @@ struct rtc_ops {
 	 */
 	int (*read8)(struct udevice *dev, unsigned int reg);
 
+	/**
+	 * read8_array() - Read multiple 8-bit registers
+	 *
+	 * @dev:	Device to read from
+	 * @reg:	First register to read
+	 * @buf:	Output buffer
+	 * @len:	Number of registers to read
+	 * @return 0 if OK, -ve on error
+	 */
+	int (*read8_array)(struct udevice *dev, unsigned int reg,
+			   u8 *buf, unsigned int len);
+
 	/**
 	* write8() - Write an 8-bit register
 	*
@@ -118,6 +130,18 @@ int dm_rtc_reset(struct udevice *dev);
  */
 int rtc_read8(struct udevice *dev, unsigned int reg);
 
+/**
+ * rtc_read8_array() - Read multiple 8-bit registers
+ *
+ * @dev:	Device to read from
+ * @reg:	First register to read
+ * @buf:	Output buffer
+ * @len:	Number of registers to read
+ * @return 0 if OK, -ve on error
+ */
+int rtc_read8_array(struct udevice *dev, unsigned int reg,
+		    u8 *buf, unsigned int len);
+
 /**
  * rtc_write8() - Write an 8-bit register
  *
-- 
2.23.0

[PATCH 2/6] rtc: add rtc_write8_array() helper

[Rasmus Villemoes]

Similar to the rtc_read8_array(), introduce a helper that allows the
caller to write multiple consecutive 8-bit registers with one call. If
the driver provides the ->write8_array method, use that, otherwise
loop using ->write8.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/rtc/rtc-uclass.c | 18 ++++++++++++++++++
 include/rtc.h            | 24 ++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/rtc/rtc-uclass.c b/drivers/rtc/rtc-uclass.c
index 5070fb416d..56490a876f 100644
--- a/drivers/rtc/rtc-uclass.c
+++ b/drivers/rtc/rtc-uclass.c
@@ -78,6 +78,24 @@ int rtc_write8(struct udevice *dev, unsigned int reg, int val)
 	return ops->write8(dev, reg, val);
 }
 
+int rtc_write8_array(struct udevice *dev, unsigned int reg,
+		     const u8 *buf, unsigned int len)
+{
+	struct rtc_ops *ops = rtc_get_ops(dev);
+
+	assert(ops);
+	if (ops->write8_array)
+		return ops->write8_array(dev, reg, buf, len);
+	if (!ops->write8)
+		return -ENOSYS;
+	while (len--) {
+		int ret = ops->write8(dev, reg++, *buf++);
+		if (ret < 0)
+			return ret;
+	}
+	return 0;
+}
+
 int rtc_read16(struct udevice *dev, unsigned int reg, u16 *valuep)
 {
 	u16 value = 0;
diff --git a/include/rtc.h b/include/rtc.h
index f7f622c1db..08b2a00567 100644
--- a/include/rtc.h
+++ b/include/rtc.h
@@ -85,6 +85,18 @@ struct rtc_ops {
 	* @return 0 if OK, -ve on error
 	*/
 	int (*write8)(struct udevice *dev, unsigned int reg, int val);
+
+	/**
+	 * write8_array() - Write multiple 8-bit registers
+	 *
+	 * @dev:	Device to write to
+	 * @reg:	First register to write
+	 * @buf:	Input buffer
+	 * @len:	Number of registers to write
+	 * @return 0 if OK, -ve on error
+	 */
+	int (*write8_array)(struct udevice *dev, unsigned int reg,
+			    const u8 *buf, unsigned int len);
 };
 
 /* Access the operations for an RTC device */
@@ -152,6 +164,18 @@ int rtc_read8_array(struct udevice *dev, unsigned int reg,
  */
 int rtc_write8(struct udevice *dev, unsigned int reg, int val);
 
+/**
+ * rtc_write8_array() - Write multiple 8-bit registers
+ *
+ * @dev:	Device to write to
+ * @reg:	First register to write
+ * @buf:	Input buffer
+ * @len:	Number of registers to write
+ * @return 0 if OK, -ve on error
+ */
+int rtc_write8_array(struct udevice *dev, unsigned int reg,
+		     const u8 *buf, unsigned int len);
+
 /**
  * rtc_read16() - Read a 16-bit value from the RTC
  *
-- 
2.23.0

[PATCH 3/6] rtc: fall back to ->{read, write}8_array if ->{read, write}8 are not provided

[Rasmus Villemoes]

Similar to how the rtc_{read,write}8_array functions fall back to
using the {read,write}8 methods, do the opposite in the
rtc_{read,write}8 functions.

This way, each driver only needs to provide either ->read8 or
->read8_array to make both rtc_read8() and rtc_read8_array() work -
without this, a driver that provides rtc_read8_array() would most
likely just duplicate the logic here for implementing a ->read8()
method in term of its ->read8_array() method. The same remarks of
course apply to the write case.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/rtc/rtc-uclass.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/rtc/rtc-uclass.c b/drivers/rtc/rtc-uclass.c
index 56490a876f..6bf964f937 100644
--- a/drivers/rtc/rtc-uclass.c
+++ b/drivers/rtc/rtc-uclass.c
@@ -44,9 +44,17 @@ int rtc_read8(struct udevice *dev, unsigned int reg)
 	struct rtc_ops *ops = rtc_get_ops(dev);
 
 	assert(ops);
-	if (!ops->read8)
-		return -ENOSYS;
-	return ops->read8(dev, reg);
+	if (ops->read8)
+		return ops->read8(dev, reg);
+	if (ops->read8_array) {
+		u8 buf[1];
+		int ret = ops->read8_array(dev, reg, buf, 1);
+
+		if (ret < 0)
+			return ret;
+		return buf[0];
+	}
+	return -ENOSYS;
 }
 
 int rtc_read8_array(struct udevice *dev, unsigned int reg,
@@ -73,9 +81,13 @@ int rtc_write8(struct udevice *dev, unsigned int reg, int val)
 	struct rtc_ops *ops = rtc_get_ops(dev);
 
 	assert(ops);
-	if (!ops->write8)
-		return -ENOSYS;
-	return ops->write8(dev, reg, val);
+	if (ops->write8)
+		return ops->write8(dev, reg, val);
+	if (ops->write8_array) {
+		u8 buf[1] = { val };
+		return ops->write8_array(dev, reg, buf, 1);
+	}
+	return -ENOSYS;
 }
 
 int rtc_write8_array(struct udevice *dev, unsigned int reg,
-- 
2.23.0

[PATCH 4/6] rtc: pcf2127: provide ->read8_array method

[Rasmus Villemoes]

This simply consists of renaming the existing pcf2127_read_reg()
helper to follow the naming of the other
methods (i.e. pcf2127_rtc_<method name>) and changing the type of its
"len" parameter.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/rtc/pcf2127.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/pcf2127.c b/drivers/rtc/pcf2127.c
index b34ed63bf0..b3f6de8496 100644
--- a/drivers/rtc/pcf2127.c
+++ b/drivers/rtc/pcf2127.c
@@ -22,8 +22,8 @@
 #define PCF2127_REG_MO		0x08
 #define PCF2127_REG_YR		0x09
 
-static int pcf2127_read_reg(struct udevice *dev, uint offset,
-			    u8 *buffer, int len)
+static int pcf2127_rtc_read8_array(struct udevice *dev, uint offset,
+				   u8 *buffer, uint len)
 {
 	struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
 	struct i2c_msg msg;
@@ -72,7 +72,7 @@ static int pcf2127_rtc_get(struct udevice *dev, struct rtc_time *tm)
 	int ret = 0;
 	uchar buf[10] = { PCF2127_REG_CTRL1 };
 
-	ret = pcf2127_read_reg(dev, PCF2127_REG_CTRL1, buf, sizeof(buf));
+	ret = pcf2127_rtc_read8_array(dev, PCF2127_REG_CTRL1, buf, sizeof(buf));
 	if (ret < 0)
 		return ret;
 
@@ -109,6 +109,7 @@ static const struct rtc_ops pcf2127_rtc_ops = {
 	.get = pcf2127_rtc_get,
 	.set = pcf2127_rtc_set,
 	.reset = pcf2127_rtc_reset,
+	.read8_array = pcf2127_rtc_read8_array,
 };
 
 static const struct udevice_id pcf2127_rtc_ids[] = {
-- 
2.23.0

[PATCH 5/6] rtc: pcf2127: provide ->write8_array method

[Rasmus Villemoes]

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/rtc/pcf2127.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/rtc/pcf2127.c b/drivers/rtc/pcf2127.c
index b3f6de8496..227ab09880 100644
--- a/drivers/rtc/pcf2127.c
+++ b/drivers/rtc/pcf2127.c
@@ -43,6 +43,12 @@ static int pcf2127_rtc_read8_array(struct udevice *dev, uint offset,
 	return dm_i2c_xfer(dev, &msg, 1);
 }
 
+static int pcf2127_rtc_write8_array(struct udevice *dev, uint offset,
+				   const u8 *buffer, uint len)
+{
+	return dm_i2c_write(dev, offset, buffer, len);
+}
+
 static int pcf2127_rtc_set(struct udevice *dev, const struct rtc_time *tm)
 {
 	uchar buf[7] = {0};
@@ -110,6 +116,7 @@ static const struct rtc_ops pcf2127_rtc_ops = {
 	.set = pcf2127_rtc_set,
 	.reset = pcf2127_rtc_reset,
 	.read8_array = pcf2127_rtc_read8_array,
+	.write8_array = pcf2127_rtc_write8_array,
 };
 
 static const struct udevice_id pcf2127_rtc_ids[] = {
-- 
2.23.0

[PATCH 6/6] rtc: add rtc command

[Rasmus Villemoes]

Mostly as an aid for debugging RTC drivers, provide a command that can
be used to read/write arbitrary registers (assuming the driver
provides the read8/write8 methods or their _array variants).

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 cmd/Kconfig  |   6 ++
 cmd/Makefile |   1 +
 cmd/rtc.c    | 153 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 160 insertions(+)
 create mode 100644 cmd/rtc.c

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 6403bc45a5..e5d0e7f7c3 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1673,6 +1673,12 @@ config CMD_DATE
 	  Enable the 'date' command for getting/setting the time/date in RTC
 	  devices.
 
+config CMD_RTC
+	bool "rtc"
+	depends on DM_RTC
+	help
+	  Enable the 'rtc' command for low-level access to RTC devices.
+
 config CMD_TIME
 	bool "time"
 	help
diff --git a/cmd/Makefile b/cmd/Makefile
index f1dd513a4b..871b07f7b2 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -119,6 +119,7 @@ obj-$(CONFIG_CMD_REISER) += reiser.o
 obj-$(CONFIG_CMD_REMOTEPROC) += remoteproc.o
 obj-$(CONFIG_CMD_RNG) += rng.o
 obj-$(CONFIG_CMD_ROCKUSB) += rockusb.o
+obj-$(CONFIG_CMD_RTC) += rtc.o
 obj-$(CONFIG_SANDBOX) += host.o
 obj-$(CONFIG_CMD_SATA) += sata.o
 obj-$(CONFIG_CMD_NVME) += nvme.o
diff --git a/cmd/rtc.c b/cmd/rtc.c
new file mode 100644
index 0000000000..d48c0333fa
--- /dev/null
+++ b/cmd/rtc.c
@@ -0,0 +1,153 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <common.h>
+#include <command.h>
+#include <dm.h>
+#include <hexdump.h>
+#include <i2c.h>
+#include <rtc.h>
+
+#define MAX_RTC_BYTES 32
+
+static int do_rtc_read(struct udevice *dev, int argc, char * const argv[], int mem)
+{
+	u8 buf[MAX_RTC_BYTES];
+	int reg, len, ret;
+	u8 *addr;
+
+	if (argc != 2 + mem)
+		return CMD_RET_USAGE;
+
+	if (mem) {
+		addr = (void*)simple_strtoul(argv[0], NULL, 16);
+		argv++;
+		argv--;
+	} else {
+		addr = buf;
+	}
+	reg = simple_strtoul(argv[0], NULL, 0);
+	len = simple_strtoul(argv[1], NULL, 0);
+
+	if (!mem && len > sizeof(buf)) {
+		printf("can read at most %d registers at a time\n", MAX_RTC_BYTES);
+		return CMD_RET_FAILURE;
+	}
+	ret = rtc_read8_array(dev, reg, buf, len);
+	if (ret) {
+		printf("rtc_read8_array() failed: %d\n", ret);
+		return CMD_RET_FAILURE;
+	}
+	if (!mem) {
+		while (len--)
+			printf("%d: 0x%02x\n", reg++, *addr++);
+	}
+	return CMD_RET_SUCCESS;
+}
+
+static int do_rtc_write(struct udevice *dev, int argc, char * const argv[], int mem)
+{
+	u8 buf[MAX_RTC_BYTES];
+	u8 *addr;
+	int reg, len, ret;
+
+	if (argc != 2 + mem)
+		return CMD_RET_USAGE;
+
+	if (mem) {
+		addr = (void*)simple_strtoul(argv[0], NULL, 16);
+		argv++;
+		argv--;
+	} else {
+		addr = buf;
+	}
+
+	reg = simple_strtoul(argv[0], NULL, 0);
+	if (mem) {
+		len = simple_strtoul(argv[1], NULL, 0);
+	} else {
+		/* Convert hexstring, bail out if too long. */
+		const char *s = argv[1];
+
+		len = strlen(s);
+		if (len > 2*MAX_RTC_BYTES) {
+			printf("hex string too long, can write at most %d bytes\n", MAX_RTC_BYTES);
+			return CMD_RET_FAILURE;
+		}
+		len /= 2;
+		if (hex2bin(addr, s, len)) {
+			printf("invalid hex string\n");
+			return CMD_RET_FAILURE;
+		}
+	}
+
+	ret = rtc_write8_array(dev, reg, buf, len);
+	if (ret) {
+		printf("rtc_write8_array() failed: %d\n", ret);
+		return CMD_RET_FAILURE;
+	}
+	return CMD_RET_SUCCESS;
+}
+
+int do_rtc(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	static int curr_rtc = 0;
+	struct udevice *dev;
+	int ret, idx;
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	argc--;
+	argv++;
+
+	if (!strcmp(argv[0], "list")) {
+		struct uclass *uc;
+		idx = 0;
+
+		uclass_id_foreach_dev(UCLASS_RTC, dev, uc) {
+			printf("RTC #%d - %s\n", idx++, dev->name);
+		}
+		if (!idx) {
+			printf("*** no RTC devices available ***\n");
+			return CMD_RET_FAILURE;
+		}
+		return CMD_RET_SUCCESS;
+	}
+
+	idx = curr_rtc;
+	if (!strcmp(argv[0], "dev") && argc >= 2)
+		idx = simple_strtoul(argv[1], NULL, 10);
+
+	ret = uclass_get_device(UCLASS_RTC, idx, &dev);
+	if (ret) {
+		printf("Cannot find RTC #%d: err=%d\n", idx, ret);
+		return CMD_RET_FAILURE;
+	}
+
+	if (!strcmp(argv[0], "dev")) {
+		/* Show the existing or newly selected RTC */
+		if (argc >= 2)
+			curr_rtc = idx;
+		printf("RTC #%d - %s\n", idx, dev->name);
+		return CMD_RET_SUCCESS;
+	}
+
+	if (!strcmp(argv[0], "read") || !strcmp(argv[0], "readm"))
+		return do_rtc_read(dev, argc - 1, argv + 1, !strcmp(argv[0], "readm"));
+
+	if (!strcmp(argv[0], "write") || !strcmp(argv[0], "writem"))
+		return do_rtc_write(dev, argc - 1, argv + 1, !strcmp(argv[0], "writem"));
+
+	return CMD_RET_USAGE;
+}
+
+U_BOOT_CMD(
+	rtc,	5,	0,	do_rtc,
+	"RTC subsystem",
+	"list                        - show available rtc devices\n"
+	"rtc dev [n]                     - show or set current rtc device\n"
+	"rtc read <reg> <count>          - read and display 8-bit registers starting at <reg>\n"
+	"rtc readm <addr> <reg> <count>  - read 8-bit registers starting at <reg> to memory\n"
+	"rtc write <reg> <hexstring>     - write 8-bit registers starting at <reg>\n"
+	"rtc writem <addr> <reg> <count> - write memory to 8-bit registers starting at <reg>\n"
+);
-- 
2.23.0

[PATCH 2/6] rtc: add rtc_write8_array() helper

[Simon Glass]

Hi Rasmus,

On Mon, 4 May 2020 at 15:20, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Similar to the rtc_read8_array(), introduce a helper that allows the
> caller to write multiple consecutive 8-bit registers with one call. If
> the driver provides the ->write8_array method, use that, otherwise
> loop using ->write8.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/rtc/rtc-uclass.c | 18 ++++++++++++++++++
>  include/rtc.h            | 24 ++++++++++++++++++++++++
>  2 files changed, 42 insertions(+)
>
> diff --git a/drivers/rtc/rtc-uclass.c b/drivers/rtc/rtc-uclass.c
> index 5070fb416d..56490a876f 100644
> --- a/drivers/rtc/rtc-uclass.c
> +++ b/drivers/rtc/rtc-uclass.c
> @@ -78,6 +78,24 @@ int rtc_write8(struct udevice *dev, unsigned int reg, int val)
>         return ops->write8(dev, reg, val);
>  }
>
> +int rtc_write8_array(struct udevice *dev, unsigned int reg,
> +                    const u8 *buf, unsigned int len)

I wonder if we could call this rtc_write() ?

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 1/6] rtc: add rtc_read8_array helper and ->read8_array method

[Simon Glass]

On Mon, 4 May 2020 at 15:20, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Some users may want to read multiple consecutive 8-bit
> registers. Instead of each caller having to implement the loop,
> provide a rtc_read8_array() helper. Also, allow a driver to provide a
> read8_array method, which can be more efficient than reading one
> register at a time.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/rtc/rtc-uclass.c | 19 +++++++++++++++++++
>  include/rtc.h            | 24 ++++++++++++++++++++++++
>  2 files changed, 43 insertions(+)
>
> diff --git a/drivers/rtc/rtc-uclass.c b/drivers/rtc/rtc-uclass.c
> index a0a238aedd..5070fb416d 100644
> --- a/drivers/rtc/rtc-uclass.c
> +++ b/drivers/rtc/rtc-uclass.c
> @@ -49,6 +49,25 @@ int rtc_read8(struct udevice *dev, unsigned int reg)
>         return ops->read8(dev, reg);
>  }
>
> +int rtc_read8_array(struct udevice *dev, unsigned int reg,
> +                   u8 *buf, unsigned int len)

How about just rtc_read() ?

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 3/6] rtc: fall back to ->{read,write}8_array if ->{read,write}8 are not provided

[Simon Glass]

On Mon, 4 May 2020 at 15:20, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Similar to how the rtc_{read,write}8_array functions fall back to
> using the {read,write}8 methods, do the opposite in the
> rtc_{read,write}8 functions.
>
> This way, each driver only needs to provide either ->read8 or
> ->read8_array to make both rtc_read8() and rtc_read8_array() work -
> without this, a driver that provides rtc_read8_array() would most
> likely just duplicate the logic here for implementing a ->read8()
> method in term of its ->read8_array() method. The same remarks of
> course apply to the write case.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/rtc/rtc-uclass.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Please make sure you add tests for these methods.

[PATCH 4/6] rtc: pcf2127: provide ->read8_array method

[Simon Glass]

On Mon, 4 May 2020 at 15:20, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> This simply consists of renaming the existing pcf2127_read_reg()
> helper to follow the naming of the other
> methods (i.e. pcf2127_rtc_<method name>) and changing the type of its
> "len" parameter.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/rtc/pcf2127.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 5/6] rtc: pcf2127: provide ->write8_array method

[Simon Glass]

On Mon, 4 May 2020 at 15:20, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/rtc/pcf2127.c | 7 +++++++
>  1 file changed, 7 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 6/6] rtc: add rtc command

[Simon Glass]

On Mon, 4 May 2020 at 15:20, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Mostly as an aid for debugging RTC drivers, provide a command that can
> be used to read/write arbitrary registers (assuming the driver
> provides the read8/write8 methods or their _array variants).
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  cmd/Kconfig  |   6 ++
>  cmd/Makefile |   1 +
>  cmd/rtc.c    | 153 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 160 insertions(+)
>  create mode 100644 cmd/rtc.c

Can you also add a sandbox test for this command?

C code:

console_record_reset();
run_command("acpi list", 0);
addr = (ulong)map_to_sysmem(buf);
ut_assert_nextline("ACPI tables start at %lx", addr);
ut_assert_nextline("RSDP %08lx %06lx (v02 U-BOOT)", addr,
   sizeof(struct acpi_rsdp));
...
ut_assert_console_end();

Regards,
Simon

[PATCH 1/6] rtc: add rtc_read8_array helper and ->read8_array method

[Rasmus Villemoes]

On 06/05/2020 05.42, Simon Glass wrote:
> On Mon, 4 May 2020 at 15:20, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>> Some users may want to read multiple consecutive 8-bit
>> registers. Instead of each caller having to implement the loop,
>> provide a rtc_read8_array() helper. Also, allow a driver to provide a
>> read8_array method, which can be more efficient than reading one
>> register at a time.
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>>  drivers/rtc/rtc-uclass.c | 19 +++++++++++++++++++
>>  include/rtc.h            | 24 ++++++++++++++++++++++++
>>  2 files changed, 43 insertions(+)
>>
>> diff --git a/drivers/rtc/rtc-uclass.c b/drivers/rtc/rtc-uclass.c
>> index a0a238aedd..5070fb416d 100644
>> --- a/drivers/rtc/rtc-uclass.c
>> +++ b/drivers/rtc/rtc-uclass.c
>> @@ -49,6 +49,25 @@ int rtc_read8(struct udevice *dev, unsigned int reg)
>>         return ops->read8(dev, reg);
>>  }
>>
>> +int rtc_read8_array(struct udevice *dev, unsigned int reg,
>> +                   u8 *buf, unsigned int len)
> 
> How about just rtc_read() ?

I certainly like a shorter name, and I suppose 8-bit registers are
ubiquitous enough among RTCs. If no-one else speaks up, I'll rename both
the functions and the methods in the next revision.

Thanks,
Rasmus

[PATCH v2 00/10] new rtc methods, rtc command, and tests

[Rasmus Villemoes]

I need access to registers other than just the timekeeping ones of the
pcf2127, so I wanted to implement ->read8 and ->write8. But for
testing these it appeared there was no convenient way to invoke those
from the shell, so I also ended up adding such a command.

Also, it seemed more natural to provide array variants that can read
or write several registers at once, so rtc_ops is expanded a bit.

Changes in v2:

- Use simply "read" and "write" instead of "read8_array",
  "write8_array", both for functions and methods, as suggested by
  Simon.

- The rtc command's interface has been simplified a bit (no separate
  read/readm; the number of arguments determines whether the user
  wants the result on the console or to a memory address)

- Add tests, both of rtc_{read,write}() and of the shell command,
  fixing a few things I stumbled on.

Rasmus Villemoes (10):
  rtc: add rtc_read helper and ->read method
  rtc: add rtc_write() helper
  rtc: fall back to ->{read,write} if ->{read,write}8 are not provided
  rtc: pcf2127: provide ->read method
  rtc: pcf2127: provide ->write method
  rtc: add rtc command
  rtc: sandbox-rtc: fix set method
  rtc: i2c_rtc_emul: catch any write to the "reset" register
  test: dm: rtc: add test of rtc_read, rtc_write
  test: dm: rtc: add tests of rtc shell command

 arch/sandbox/include/asm/rtc.h |   5 ++
 cmd/Kconfig                    |   6 ++
 cmd/Makefile                   |   1 +
 cmd/rtc.c                      | 159 +++++++++++++++++++++++++++++++++
 drivers/rtc/i2c_rtc_emul.c     |   3 +-
 drivers/rtc/pcf2127.c          |  13 ++-
 drivers/rtc/rtc-uclass.c       |  56 +++++++++++-
 drivers/rtc/sandbox_rtc.c      |  65 +++++---------
 include/rtc.h                  |  47 ++++++++++
 test/dm/rtc.c                  | 121 ++++++++++++++++++++++++-
 10 files changed, 426 insertions(+), 50 deletions(-)
 create mode 100644 cmd/rtc.c

-- 
2.23.0

[PATCH v2 01/10] rtc: add rtc_read helper and ->read method

[Rasmus Villemoes]

Some users may want to read multiple consecutive 8-bit
registers. Instead of each caller having to implement the loop,
provide a rtc_read() helper. Also, allow a driver to provide a ->read
method, which can be more efficient than reading one register at a
time.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/rtc/rtc-uclass.c | 18 ++++++++++++++++++
 include/rtc.h            | 23 +++++++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/drivers/rtc/rtc-uclass.c b/drivers/rtc/rtc-uclass.c
index a0a238aedd..44d76bb70f 100644
--- a/drivers/rtc/rtc-uclass.c
+++ b/drivers/rtc/rtc-uclass.c
@@ -39,6 +39,24 @@ int dm_rtc_reset(struct udevice *dev)
 	return ops->reset(dev);
 }
 
+int rtc_read(struct udevice *dev, unsigned int reg, u8 *buf, unsigned int len)
+{
+	struct rtc_ops *ops = rtc_get_ops(dev);
+
+	assert(ops);
+	if (ops->read)
+		return ops->read(dev, reg, buf, len);
+	if (!ops->read8)
+		return -ENOSYS;
+	while (len--) {
+		int ret = ops->read8(dev, reg++);
+		if (ret < 0)
+			return ret;
+		*buf++ = ret;
+	}
+	return 0;
+}
+
 int rtc_read8(struct udevice *dev, unsigned int reg)
 {
 	struct rtc_ops *ops = rtc_get_ops(dev);
diff --git a/include/rtc.h b/include/rtc.h
index 8aabfc1162..1c9c09fef8 100644
--- a/include/rtc.h
+++ b/include/rtc.h
@@ -55,6 +55,18 @@ struct rtc_ops {
 	 */
 	int (*reset)(struct udevice *dev);
 
+	/**
+	 * read() - Read multiple 8-bit registers
+	 *
+	 * @dev:	Device to read from
+	 * @reg:	First register to read
+	 * @buf:	Output buffer
+	 * @len:	Number of registers to read
+	 * @return 0 if OK, -ve on error
+	 */
+	int (*read)(struct udevice *dev, unsigned int reg,
+			   u8 *buf, unsigned int len);
+
 	/**
 	 * read8() - Read an 8-bit register
 	 *
@@ -109,6 +121,17 @@ int dm_rtc_set(struct udevice *dev, struct rtc_time *time);
  */
 int dm_rtc_reset(struct udevice *dev);
 
+/**
+ * rtc_read() - Read multiple 8-bit registers
+ *
+ * @dev:	Device to read from
+ * @reg:	First register to read
+ * @buf:	Output buffer
+ * @len:	Number of registers to read
+ * @return 0 if OK, -ve on error
+ */
+int rtc_read(struct udevice *dev, unsigned int reg, u8 *buf, unsigned int len);
+
 /**
  * rtc_read8() - Read an 8-bit register
  *
-- 
2.23.0

[PATCH v2 02/10] rtc: add rtc_write() helper

[Rasmus Villemoes]

Similar to rtc_read(), introduce a helper that allows the caller to
write multiple consecutive 8-bit registers with one call. If the
driver provides the ->write method, use that, otherwise loop using
->write8.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/rtc/rtc-uclass.c | 18 ++++++++++++++++++
 include/rtc.h            | 24 ++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/rtc/rtc-uclass.c b/drivers/rtc/rtc-uclass.c
index 44d76bb70f..cc5f9c7baa 100644
--- a/drivers/rtc/rtc-uclass.c
+++ b/drivers/rtc/rtc-uclass.c
@@ -57,6 +57,24 @@ int rtc_read(struct udevice *dev, unsigned int reg, u8 *buf, unsigned int len)
 	return 0;
 }
 
+int rtc_write(struct udevice *dev, unsigned int reg,
+	      const u8 *buf, unsigned int len)
+{
+	struct rtc_ops *ops = rtc_get_ops(dev);
+
+	assert(ops);
+	if (ops->write)
+		return ops->write(dev, reg, buf, len);
+	if (!ops->write8)
+		return -ENOSYS;
+	while (len--) {
+		int ret = ops->write8(dev, reg++, *buf++);
+		if (ret < 0)
+			return ret;
+	}
+	return 0;
+}
+
 int rtc_read8(struct udevice *dev, unsigned int reg)
 {
 	struct rtc_ops *ops = rtc_get_ops(dev);
diff --git a/include/rtc.h b/include/rtc.h
index 1c9c09fef8..8c66d37703 100644
--- a/include/rtc.h
+++ b/include/rtc.h
@@ -67,6 +67,18 @@ struct rtc_ops {
 	int (*read)(struct udevice *dev, unsigned int reg,
 			   u8 *buf, unsigned int len);
 
+	/**
+	 * write() - Write multiple 8-bit registers
+	 *
+	 * @dev:	Device to write to
+	 * @reg:	First register to write
+	 * @buf:	Input buffer
+	 * @len:	Number of registers to write
+	 * @return 0 if OK, -ve on error
+	 */
+	int (*write)(struct udevice *dev, unsigned int reg,
+		     const u8 *buf, unsigned int len);
+
 	/**
 	 * read8() - Read an 8-bit register
 	 *
@@ -132,6 +144,18 @@ int dm_rtc_reset(struct udevice *dev);
  */
 int rtc_read(struct udevice *dev, unsigned int reg, u8 *buf, unsigned int len);
 
+/**
+ * rtc_write() - Write multiple 8-bit registers
+ *
+ * @dev:	Device to write to
+ * @reg:	First register to write
+ * @buf:	Input buffer
+ * @len:	Number of registers to write
+ * @return 0 if OK, -ve on error
+ */
+int rtc_write(struct udevice *dev, unsigned int reg,
+	      const u8 *buf, unsigned int len);
+
 /**
  * rtc_read8() - Read an 8-bit register
  *
-- 
2.23.0

[PATCH v2 03/10] rtc: fall back to ->{read, write} if ->{read, write}8 are not provided

[Rasmus Villemoes]

Similar to how the rtc_{read,write} functions fall back to
using the {read,write}8 methods, do the opposite in the
rtc_{read,write}8 functions.

This way, each driver only needs to provide either ->read8 or ->read
to make both rtc_read8() and rtc_read() work - without this, a driver
that provides ->read() would most likely just duplicate the logic here
for implementing a ->read8() method in term of its ->read()
method. The same remarks of course apply to the write case.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/rtc/rtc-uclass.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/rtc/rtc-uclass.c b/drivers/rtc/rtc-uclass.c
index cc5f9c7baa..3cdeb89084 100644
--- a/drivers/rtc/rtc-uclass.c
+++ b/drivers/rtc/rtc-uclass.c
@@ -80,9 +80,17 @@ int rtc_read8(struct udevice *dev, unsigned int reg)
 	struct rtc_ops *ops = rtc_get_ops(dev);
 
 	assert(ops);
-	if (!ops->read8)
-		return -ENOSYS;
-	return ops->read8(dev, reg);
+	if (ops->read8)
+		return ops->read8(dev, reg);
+	if (ops->read) {
+		u8 buf[1];
+		int ret = ops->read(dev, reg, buf, 1);
+
+		if (ret < 0)
+			return ret;
+		return buf[0];
+	}
+	return -ENOSYS;
 }
 
 int rtc_write8(struct udevice *dev, unsigned int reg, int val)
@@ -90,9 +98,13 @@ int rtc_write8(struct udevice *dev, unsigned int reg, int val)
 	struct rtc_ops *ops = rtc_get_ops(dev);
 
 	assert(ops);
-	if (!ops->write8)
-		return -ENOSYS;
-	return ops->write8(dev, reg, val);
+	if (ops->write8)
+		return ops->write8(dev, reg, val);
+	if (ops->write) {
+		u8 buf[1] = { val };
+		return ops->write(dev, reg, buf, 1);
+	}
+	return -ENOSYS;
 }
 
 int rtc_read16(struct udevice *dev, unsigned int reg, u16 *valuep)
-- 
2.23.0

[PATCH v2 04/10] rtc: pcf2127: provide ->read method

[Rasmus Villemoes]

This simply consists of renaming the existing pcf2127_read_reg()
helper to follow the naming of the other
methods (i.e. pcf2127_rtc_<method name>) and changing the type of its
"len" parameter.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/rtc/pcf2127.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/pcf2127.c b/drivers/rtc/pcf2127.c
index b34ed63bf0..f48cd8cb18 100644
--- a/drivers/rtc/pcf2127.c
+++ b/drivers/rtc/pcf2127.c
@@ -22,8 +22,7 @@
 #define PCF2127_REG_MO		0x08
 #define PCF2127_REG_YR		0x09
 
-static int pcf2127_read_reg(struct udevice *dev, uint offset,
-			    u8 *buffer, int len)
+static int pcf2127_rtc_read(struct udevice *dev, uint offset, u8 *buffer, uint len)
 {
 	struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
 	struct i2c_msg msg;
@@ -72,7 +71,7 @@ static int pcf2127_rtc_get(struct udevice *dev, struct rtc_time *tm)
 	int ret = 0;
 	uchar buf[10] = { PCF2127_REG_CTRL1 };
 
-	ret = pcf2127_read_reg(dev, PCF2127_REG_CTRL1, buf, sizeof(buf));
+	ret = pcf2127_rtc_read(dev, PCF2127_REG_CTRL1, buf, sizeof(buf));
 	if (ret < 0)
 		return ret;
 
@@ -109,6 +108,7 @@ static const struct rtc_ops pcf2127_rtc_ops = {
 	.get = pcf2127_rtc_get,
 	.set = pcf2127_rtc_set,
 	.reset = pcf2127_rtc_reset,
+	.read = pcf2127_rtc_read,
 };
 
 static const struct udevice_id pcf2127_rtc_ids[] = {
-- 
2.23.0

[PATCH v2 05/10] rtc: pcf2127: provide ->write method

[Rasmus Villemoes]

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/rtc/pcf2127.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/rtc/pcf2127.c b/drivers/rtc/pcf2127.c
index f48cd8cb18..a3faf62ee0 100644
--- a/drivers/rtc/pcf2127.c
+++ b/drivers/rtc/pcf2127.c
@@ -42,6 +42,12 @@ static int pcf2127_rtc_read(struct udevice *dev, uint offset, u8 *buffer, uint l
 	return dm_i2c_xfer(dev, &msg, 1);
 }
 
+static int pcf2127_rtc_write(struct udevice *dev, uint offset,
+			     const u8 *buffer, uint len)
+{
+	return dm_i2c_write(dev, offset, buffer, len);
+}
+
 static int pcf2127_rtc_set(struct udevice *dev, const struct rtc_time *tm)
 {
 	uchar buf[7] = {0};
@@ -109,6 +115,7 @@ static const struct rtc_ops pcf2127_rtc_ops = {
 	.set = pcf2127_rtc_set,
 	.reset = pcf2127_rtc_reset,
 	.read = pcf2127_rtc_read,
+	.write = pcf2127_rtc_write,
 };
 
 static const struct udevice_id pcf2127_rtc_ids[] = {
-- 
2.23.0

[PATCH v2 06/10] rtc: add rtc command

[Rasmus Villemoes]

Mostly as an aid for debugging RTC drivers, provide a command that can
be used to read/write arbitrary registers (assuming the driver
provides the read/write methods or their single-register-at-a-time
variants).

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 cmd/Kconfig  |   6 ++
 cmd/Makefile |   1 +
 cmd/rtc.c    | 159 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 166 insertions(+)
 create mode 100644 cmd/rtc.c

diff --git a/cmd/Kconfig b/cmd/Kconfig
index f9be1988f6..7eea25facd 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1715,6 +1715,12 @@ config CMD_DATE
 	  Enable the 'date' command for getting/setting the time/date in RTC
 	  devices.
 
+config CMD_RTC
+	bool "rtc"
+	depends on DM_RTC
+	help
+	  Enable the 'rtc' command for low-level access to RTC devices.
+
 config CMD_TIME
 	bool "time"
 	help
diff --git a/cmd/Makefile b/cmd/Makefile
index 974ad48b0a..c7992113e4 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -120,6 +120,7 @@ obj-$(CONFIG_CMD_REISER) += reiser.o
 obj-$(CONFIG_CMD_REMOTEPROC) += remoteproc.o
 obj-$(CONFIG_CMD_RNG) += rng.o
 obj-$(CONFIG_CMD_ROCKUSB) += rockusb.o
+obj-$(CONFIG_CMD_RTC) += rtc.o
 obj-$(CONFIG_SANDBOX) += host.o
 obj-$(CONFIG_CMD_SATA) += sata.o
 obj-$(CONFIG_CMD_NVME) += nvme.o
diff --git a/cmd/rtc.c b/cmd/rtc.c
new file mode 100644
index 0000000000..e26b7ca18f
--- /dev/null
+++ b/cmd/rtc.c
@@ -0,0 +1,159 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <common.h>
+#include <command.h>
+#include <dm.h>
+#include <hexdump.h>
+#include <i2c.h>
+#include <mapmem.h>
+#include <rtc.h>
+
+#define MAX_RTC_BYTES 32
+
+static int do_rtc_read(struct udevice *dev, int argc, char * const argv[])
+{
+	u8 buf[MAX_RTC_BYTES];
+	int reg, len, ret;
+	u8 *addr;
+
+	if (argc < 2 || argc > 3)
+		return CMD_RET_USAGE;
+
+	reg = simple_strtoul(argv[0], NULL, 0);
+	len = simple_strtoul(argv[1], NULL, 0);
+	if (argc == 3) {
+		addr = map_sysmem(simple_strtoul(argv[2], NULL, 16), len);
+	} else {
+		if (len > sizeof(buf)) {
+			printf("can read at most %d registers at a time\n", MAX_RTC_BYTES);
+			return CMD_RET_FAILURE;
+		}
+		addr = buf;
+	}
+
+	ret = rtc_read(dev, reg, addr, len);
+	if (ret) {
+		printf("rtc_read() failed: %d\n", ret);
+		ret = CMD_RET_FAILURE;
+		goto out;
+	} else {
+		ret = CMD_RET_SUCCESS;
+	}
+
+	if (argc == 2) {
+		while (len--)
+			printf("%d: 0x%02x\n", reg++, *addr++);
+	}
+out:
+	if (argc == 3)
+		unmap_sysmem(addr);
+	return ret;
+}
+
+static int do_rtc_write(struct udevice *dev, int argc, char * const argv[])
+{
+	u8 buf[MAX_RTC_BYTES];
+	u8 *addr;
+	int reg, len, ret;
+
+	if (argc < 2 || argc > 3)
+		return CMD_RET_USAGE;
+
+	reg = simple_strtoul(argv[0], NULL, 0);
+
+	if (argc == 3) {
+		len = simple_strtoul(argv[1], NULL, 0);
+		addr = map_sysmem(simple_strtoul(argv[2], NULL, 16), len);
+	} else {
+		const char *s = argv[1];
+
+		/* Convert hexstring, bail out if too long. */
+		addr = buf;
+		len = strlen(s);
+		if (len > 2*MAX_RTC_BYTES) {
+			printf("hex string too long, can write at most %d bytes\n", MAX_RTC_BYTES);
+			return CMD_RET_FAILURE;
+		}
+		len /= 2;
+		if (hex2bin(addr, s, len)) {
+			printf("invalid hex string\n");
+			return CMD_RET_FAILURE;
+		}
+	}
+
+	ret = rtc_write(dev, reg, addr, len);
+	if (ret) {
+		printf("rtc_write() failed: %d\n", ret);
+		ret = CMD_RET_FAILURE;
+	} else {
+		ret = CMD_RET_SUCCESS;
+	}
+
+	if (argc == 3)
+		unmap_sysmem(addr);
+	return ret;
+}
+
+int do_rtc(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	static int curr_rtc = 0;
+	struct udevice *dev;
+	int ret, idx;
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	argc--;
+	argv++;
+
+	if (!strcmp(argv[0], "list")) {
+		struct uclass *uc;
+		idx = 0;
+
+		uclass_id_foreach_dev(UCLASS_RTC, dev, uc) {
+			printf("RTC #%d - %s\n", idx++, dev->name);
+		}
+		if (!idx) {
+			printf("*** no RTC devices available ***\n");
+			return CMD_RET_FAILURE;
+		}
+		return CMD_RET_SUCCESS;
+	}
+
+	idx = curr_rtc;
+	if (!strcmp(argv[0], "dev") && argc >= 2)
+		idx = simple_strtoul(argv[1], NULL, 10);
+
+	ret = uclass_get_device(UCLASS_RTC, idx, &dev);
+	if (ret) {
+		printf("Cannot find RTC #%d: err=%d\n", idx, ret);
+		return CMD_RET_FAILURE;
+	}
+
+	if (!strcmp(argv[0], "dev")) {
+		/* Show the existing or newly selected RTC */
+		if (argc >= 2)
+			curr_rtc = idx;
+		printf("RTC #%d - %s\n", idx, dev->name);
+		return CMD_RET_SUCCESS;
+	}
+
+	if (!strcmp(argv[0], "read"))
+		return do_rtc_read(dev, argc - 1, argv + 1);
+
+	if (!strcmp(argv[0], "write"))
+		return do_rtc_write(dev, argc - 1, argv + 1);
+
+	return CMD_RET_USAGE;
+}
+
+U_BOOT_CMD(
+	rtc,	5,	0,	do_rtc,
+	"RTC subsystem",
+	"list                        - show available rtc devices\n"
+	"rtc dev [n]                     - show or set current rtc device\n"
+	"rtc read <reg> <count>          - read and display 8-bit registers starting at <reg>\n"
+	"rtc read <reg> <count> <addr>   - read 8-bit registers starting at <reg> to memory <addr>\n"
+	"rtc write <reg> <hexstring>     - write 8-bit registers starting at <reg>\n"
+	"rtc write <reg> <count> <addr>  - write from memory <addr> to 8-bit registers starting at <reg>\n"
+);
-- 
2.23.0

[PATCH v2 07/10] rtc: sandbox-rtc: fix set method

[Rasmus Villemoes]

The current set method is broken; a simple test case is to first set
the date to something in April, then change the date to 31st May:

=> date 040412122020.34
Date: 2020-04-04 (Saturday)    Time: 12:12:34
=> date 053112122020.34
Date: 2020-05-01 (Friday)    Time: 12:12:34

or via the amending of the existing rtc_set_get test case similarly:

$ ./u-boot -T -v
=> ut dm rtc_set_get
Test: dm_test_rtc_set_get: rtc.c
expected: 31/08/2004 18:18:00
actual: 01/08/2004 18:18:00

The problem is that after each register write,
sandbox_i2c_rtc_complete_write() gets called and sets the internal
time from the current set of registers. However, when we get to
writing 31 to mday, the registers are in an inconsistent state (mon is
still 4), so the mktime machinery ends up translating April 31st to
May 1st. Upon the next register write, the registers are populated by
sandbox_i2c_rtc_prepare_read(), so the 31 we just wrote to mday gets
overwritten by a 1.

Fix it by writing all registers at once, and for consistency, update
the get method to retrieve them all with one "i2c transfer".

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/rtc/sandbox_rtc.c | 65 +++++++++++++++------------------------
 test/dm/rtc.c             | 15 ++++++++-
 2 files changed, 38 insertions(+), 42 deletions(-)

diff --git a/drivers/rtc/sandbox_rtc.c b/drivers/rtc/sandbox_rtc.c
index b08d758a74..77065e49c7 100644
--- a/drivers/rtc/sandbox_rtc.c
+++ b/drivers/rtc/sandbox_rtc.c
@@ -14,55 +14,38 @@
 
 static int sandbox_rtc_get(struct udevice *dev, struct rtc_time *time)
 {
-	time->tm_sec = dm_i2c_reg_read(dev, REG_SEC);
-	if (time->tm_sec < 0)
-		return time->tm_sec;
-	time->tm_min = dm_i2c_reg_read(dev, REG_MIN);
-	if (time->tm_min < 0)
-		return time->tm_min;
-	time->tm_hour = dm_i2c_reg_read(dev, REG_HOUR);
-	if (time->tm_hour < 0)
-		return time->tm_hour;
-	time->tm_mday = dm_i2c_reg_read(dev, REG_MDAY);
-	if (time->tm_mday < 0)
-		return time->tm_mday;
-	time->tm_mon = dm_i2c_reg_read(dev, REG_MON);
-	if (time->tm_mon < 0)
-		return time->tm_mon;
-	time->tm_year = dm_i2c_reg_read(dev, REG_YEAR);
-	if (time->tm_year < 0)
-		return time->tm_year;
-	time->tm_year += 1900;
-	time->tm_wday = dm_i2c_reg_read(dev, REG_WDAY);
-	if (time->tm_wday < 0)
-		return time->tm_wday;
+	u8 buf[7];
+	int ret;
+
+	ret = dm_i2c_read(dev, REG_SEC, buf, sizeof(buf));
+	if (ret < 0)
+		return ret;
+
+	time->tm_sec  = buf[REG_SEC - REG_SEC];
+	time->tm_min  = buf[REG_MIN - REG_SEC];
+	time->tm_hour = buf[REG_HOUR - REG_SEC];
+	time->tm_mday = buf[REG_MDAY - REG_SEC];
+	time->tm_mon  = buf[REG_MON - REG_SEC];
+	time->tm_year = buf[REG_YEAR - REG_SEC] + 1900;
+	time->tm_wday = buf[REG_WDAY - REG_SEC];
 
 	return 0;
 }
 
 static int sandbox_rtc_set(struct udevice *dev, const struct rtc_time *time)
 {
+	u8 buf[7];
 	int ret;
 
-	ret = dm_i2c_reg_write(dev, REG_SEC, time->tm_sec);
-	if (ret < 0)
-		return ret;
-	ret = dm_i2c_reg_write(dev, REG_MIN, time->tm_min);
-	if (ret < 0)
-		return ret;
-	ret = dm_i2c_reg_write(dev, REG_HOUR, time->tm_hour);
-	if (ret < 0)
-		return ret;
-	ret = dm_i2c_reg_write(dev, REG_MDAY, time->tm_mday);
-	if (ret < 0)
-		return ret;
-	ret = dm_i2c_reg_write(dev, REG_MON, time->tm_mon);
-	if (ret < 0)
-		return ret;
-	ret = dm_i2c_reg_write(dev, REG_YEAR, time->tm_year - 1900);
-	if (ret < 0)
-		return ret;
-	ret = dm_i2c_reg_write(dev, REG_WDAY, time->tm_wday);
+	buf[REG_SEC - REG_SEC]  = time->tm_sec;
+	buf[REG_MIN - REG_SEC]  = time->tm_min;
+	buf[REG_HOUR - REG_SEC] = time->tm_hour;
+	buf[REG_MDAY - REG_SEC] = time->tm_mday;
+	buf[REG_MON  - REG_SEC] = time->tm_mon;
+	buf[REG_YEAR - REG_SEC] = time->tm_year - 1900;
+	buf[REG_WDAY - REG_SEC] = time->tm_wday;
+
+	ret = dm_i2c_write(dev, REG_SEC, buf, sizeof(buf));
 	if (ret < 0)
 		return ret;
 
diff --git a/test/dm/rtc.c b/test/dm/rtc.c
index 7188742764..e5454139cd 100644
--- a/test/dm/rtc.c
+++ b/test/dm/rtc.c
@@ -69,7 +69,20 @@ static int dm_test_rtc_set_get(struct unit_test_state *uts)
 	old_base_time = sandbox_i2c_rtc_get_set_base_time(emul, -1);
 
 	memset(&time, '\0', sizeof(time));
-	time.tm_mday = 25;
+	time.tm_mday = 3;
+	time.tm_mon = 6;
+	time.tm_year = 2004;
+	time.tm_sec = 0;
+	time.tm_min = 18;
+	time.tm_hour = 18;
+	ut_assertok(dm_rtc_set(dev, &time));
+
+	memset(&cmp, '\0', sizeof(cmp));
+	ut_assertok(dm_rtc_get(dev, &cmp));
+	ut_assertok(cmp_times(&time, &cmp, true));
+
+	memset(&time, '\0', sizeof(time));
+	time.tm_mday = 31;
 	time.tm_mon = 8;
 	time.tm_year = 2004;
 	time.tm_sec = 0;
-- 
2.23.0

[PATCH v2 08/10] rtc: i2c_rtc_emul: catch any write to the "reset" register

[Rasmus Villemoes]

It's more natural that any write that happens to touch the reset
register should cause a reset, rather than just a write that starts at
that offset.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/rtc/i2c_rtc_emul.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/i2c_rtc_emul.c b/drivers/rtc/i2c_rtc_emul.c
index d4b33e59d6..3a7f1fe53e 100644
--- a/drivers/rtc/i2c_rtc_emul.c
+++ b/drivers/rtc/i2c_rtc_emul.c
@@ -196,7 +196,8 @@ static int sandbox_i2c_rtc_xfer(struct udevice *emul, struct i2c_msg *msg,
 
 			/* Write the register */
 			memcpy(plat->reg + offset, ptr, len);
-			if (offset == REG_RESET)
+			/* If the reset register was written to, do reset. */
+			if (offset <= REG_RESET && REG_RESET < offset + len)
 				reset_time(emul);
 		}
 	}
-- 
2.23.0

[PATCH v2 09/10] test: dm: rtc: add test of rtc_read, rtc_write

[Rasmus Villemoes]

Define a few aux registers and check that they can be read/written
individually. Also check that one can access the time-keeping
registers directly and get the expected results.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 arch/sandbox/include/asm/rtc.h |  5 ++++
 test/dm/rtc.c                  | 45 ++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/arch/sandbox/include/asm/rtc.h b/arch/sandbox/include/asm/rtc.h
index 1fbfea7999..5bb032f59f 100644
--- a/arch/sandbox/include/asm/rtc.h
+++ b/arch/sandbox/include/asm/rtc.h
@@ -21,6 +21,11 @@ enum {
 
 	REG_RESET	= 0x20,
 
+	REG_AUX0	= 0x30,
+	REG_AUX1,
+	REG_AUX2,
+	REG_AUX3,
+
 	REG_COUNT	= 0x80,
 };
 
diff --git a/test/dm/rtc.c b/test/dm/rtc.c
index e5454139cd..5301805d19 100644
--- a/test/dm/rtc.c
+++ b/test/dm/rtc.c
@@ -9,6 +9,7 @@
 #include <i2c.h>
 #include <rtc.h>
 #include <asm/io.h>
+#include <asm/rtc.h>
 #include <asm/test.h>
 #include <dm/test.h>
 #include <test/ut.h>
@@ -129,6 +130,50 @@ static int dm_test_rtc_set_get(struct unit_test_state *uts)
 }
 DM_TEST(dm_test_rtc_set_get, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
 
+static int dm_test_rtc_read_write(struct unit_test_state *uts)
+{
+	struct rtc_time time;
+	struct udevice *dev, *emul;
+	long old_offset;
+	u8 buf[4], reg;
+
+	ut_assertok(uclass_get_device(UCLASS_RTC, 0, &dev));
+
+	memcpy(buf, "car", 4);
+	ut_assertok(rtc_write(dev, REG_AUX0, buf, 4));
+	memset(buf, '\0', sizeof(buf));
+	ut_assertok(rtc_read(dev, REG_AUX0, buf, 4));
+	ut_asserteq(memcmp(buf, "car", 4), 0);
+
+	reg = 'b';
+	ut_assertok(rtc_write(dev, REG_AUX0, &reg, 1));
+	memset(buf, '\0', sizeof(buf));
+	ut_assertok(rtc_read(dev, REG_AUX0, buf, 4));
+	ut_asserteq(memcmp(buf, "bar", 4), 0);
+
+	reg = 't';
+	ut_assertok(rtc_write(dev, REG_AUX2, &reg, 1));
+	memset(buf, '\0', sizeof(buf));
+	ut_assertok(rtc_read(dev, REG_AUX1, buf, 3));
+	ut_asserteq(memcmp(buf, "at", 3), 0);
+
+	ut_assertok(i2c_emul_find(dev, &emul));
+	ut_assert(emul != NULL);
+
+	old_offset = sandbox_i2c_rtc_set_offset(emul, false, 0);
+	ut_assertok(dm_rtc_get(dev, &time));
+
+	ut_assertok(rtc_read(dev, REG_SEC, &reg, 1));
+	ut_asserteq(time.tm_sec, reg);
+	ut_assertok(rtc_read(dev, REG_MDAY, &reg, 1));
+	ut_asserteq(time.tm_mday, reg);
+
+	sandbox_i2c_rtc_set_offset(emul, true, old_offset);
+
+	return 0;
+}
+DM_TEST(dm_test_rtc_read_write, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
+
 /* Reset the time */
 static int dm_test_rtc_reset(struct unit_test_state *uts)
 {
-- 
2.23.0

[PATCH v2 10/10] test: dm: rtc: add tests of rtc shell command

[Rasmus Villemoes]

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 test/dm/rtc.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/test/dm/rtc.c b/test/dm/rtc.c
index 5301805d19..d1d8ff0375 100644
--- a/test/dm/rtc.c
+++ b/test/dm/rtc.c
@@ -5,6 +5,7 @@
  */
 
 #include <common.h>
+#include <console.h>
 #include <dm.h>
 #include <i2c.h>
 #include <rtc.h>
@@ -174,6 +175,66 @@ static int dm_test_rtc_read_write(struct unit_test_state *uts)
 }
 DM_TEST(dm_test_rtc_read_write, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
 
+/* Test 'rtc list' command */
+static int dm_test_rtc_cmd_list(struct unit_test_state *uts)
+{
+	console_record_reset();
+
+	run_command("rtc list", 0);
+	ut_assert_nextline("RTC #0 - rtc at 43");
+	ut_assert_nextline("RTC #1 - rtc at 61");
+	ut_assert_console_end();
+
+	return 0;
+}
+DM_TEST(dm_test_rtc_cmd_list, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
+
+/* Test 'rtc read' and 'rtc write' commands */
+static int dm_test_rtc_cmd_rw(struct unit_test_state *uts)
+{
+	console_record_reset();
+
+	run_command("rtc dev 0", 0);
+	ut_assert_nextline("RTC #0 - rtc at 43");
+	ut_assert_console_end();
+
+	run_command("rtc write 0x30 aabb", 0);
+	ut_assert_console_end();
+
+	run_command("rtc read 0x30 2", 0);
+	ut_assert_nextline("48: 0xaa");
+	ut_assert_nextline("49: 0xbb");
+	ut_assert_console_end();
+
+	run_command("rtc dev 1", 0);
+	ut_assert_nextline("RTC #1 - rtc at 61");
+	ut_assert_console_end();
+
+	run_command("rtc write 0x30 ccdd", 0);
+	ut_assert_console_end();
+
+	run_command("rtc read 0x30 2", 0);
+	ut_assert_nextline("48: 0xcc");
+	ut_assert_nextline("49: 0xdd");
+	ut_assert_console_end();
+
+	/*
+	 * Switch back to device #0, check that its aux registers
+	 * still have the same values.
+	 */
+	run_command("rtc dev 0", 0);
+	ut_assert_nextline("RTC #0 - rtc at 43");
+	ut_assert_console_end();
+
+	run_command("rtc read 0x30 2", 0);
+	ut_assert_nextline("48: 0xaa");
+	ut_assert_nextline("49: 0xbb");
+	ut_assert_console_end();
+
+	return 0;
+}
+DM_TEST(dm_test_rtc_cmd_rw, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
+
 /* Reset the time */
 static int dm_test_rtc_reset(struct unit_test_state *uts)
 {
-- 
2.23.0

[PATCH v2 06/10] rtc: add rtc command

[Simon Glass]

Hi Rasmus,

On Tue, 19 May 2020 at 16:01, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Mostly as an aid for debugging RTC drivers, provide a command that can
> be used to read/write arbitrary registers (assuming the driver
> provides the read/write methods or their single-register-at-a-time
> variants).
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  cmd/Kconfig  |   6 ++
>  cmd/Makefile |   1 +
>  cmd/rtc.c    | 159 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 166 insertions(+)
>  create mode 100644 cmd/rtc.c
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index f9be1988f6..7eea25facd 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1715,6 +1715,12 @@ config CMD_DATE
>           Enable the 'date' command for getting/setting the time/date in RTC
>           devices.
>
> +config CMD_RTC
> +       bool "rtc"
> +       depends on DM_RTC
> +       help
> +         Enable the 'rtc' command for low-level access to RTC devices.
> +
>  config CMD_TIME
>         bool "time"
>         help
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 974ad48b0a..c7992113e4 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -120,6 +120,7 @@ obj-$(CONFIG_CMD_REISER) += reiser.o
>  obj-$(CONFIG_CMD_REMOTEPROC) += remoteproc.o
>  obj-$(CONFIG_CMD_RNG) += rng.o
>  obj-$(CONFIG_CMD_ROCKUSB) += rockusb.o
> +obj-$(CONFIG_CMD_RTC) += rtc.o
>  obj-$(CONFIG_SANDBOX) += host.o
>  obj-$(CONFIG_CMD_SATA) += sata.o
>  obj-$(CONFIG_CMD_NVME) += nvme.o
> diff --git a/cmd/rtc.c b/cmd/rtc.c
> new file mode 100644
> index 0000000000..e26b7ca18f
> --- /dev/null
> +++ b/cmd/rtc.c
> @@ -0,0 +1,159 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <common.h>
> +#include <command.h>
> +#include <dm.h>
> +#include <hexdump.h>
> +#include <i2c.h>
> +#include <mapmem.h>
> +#include <rtc.h>
> +
> +#define MAX_RTC_BYTES 32
> +
> +static int do_rtc_read(struct udevice *dev, int argc, char * const argv[])
> +{
> +       u8 buf[MAX_RTC_BYTES];
> +       int reg, len, ret;
> +       u8 *addr;
> +
> +       if (argc < 2 || argc > 3)
> +               return CMD_RET_USAGE;
> +
> +       reg = simple_strtoul(argv[0], NULL, 0);

I think these should be hex (i.e. 16), since that is the norm in U-Boot.

> +       len = simple_strtoul(argv[1], NULL, 0);
> +       if (argc == 3) {
> +               addr = map_sysmem(simple_strtoul(argv[2], NULL, 16), len);
> +       } else {
> +               if (len > sizeof(buf)) {
> +                       printf("can read at most %d registers at a time\n", MAX_RTC_BYTES);

It would be better to loop like print_buffer() does.

> +                       return CMD_RET_FAILURE;
> +               }
> +               addr = buf;
> +       }
> +
> +       ret = rtc_read(dev, reg, addr, len);
> +       if (ret) {
> +               printf("rtc_read() failed: %d\n", ret);
> +               ret = CMD_RET_FAILURE;
> +               goto out;
> +       } else {
> +               ret = CMD_RET_SUCCESS;
> +       }
> +
> +       if (argc == 2) {
> +               while (len--)
> +                       printf("%d: 0x%02x\n", reg++, *addr++);

Perhaps use print_buffer()?

> +       }
> +out:
> +       if (argc == 3)
> +               unmap_sysmem(addr);
> +       return ret;
> +}
> +
> +static int do_rtc_write(struct udevice *dev, int argc, char * const argv[])
> +{
> +       u8 buf[MAX_RTC_BYTES];
> +       u8 *addr;
> +       int reg, len, ret;
> +
> +       if (argc < 2 || argc > 3)
> +               return CMD_RET_USAGE;
> +
> +       reg = simple_strtoul(argv[0], NULL, 0);
> +
> +       if (argc == 3) {
> +               len = simple_strtoul(argv[1], NULL, 0);
> +               addr = map_sysmem(simple_strtoul(argv[2], NULL, 16), len);
> +       } else {
> +               const char *s = argv[1];
> +
> +               /* Convert hexstring, bail out if too long. */
> +               addr = buf;
> +               len = strlen(s);
> +               if (len > 2*MAX_RTC_BYTES) {

Spaces around *

> +                       printf("hex string too long, can write at most %d bytes\n", MAX_RTC_BYTES);

Please can you try checkpatch or patman? This lines seems too long

> +                       return CMD_RET_FAILURE;
> +               }
> +               len /= 2;
> +               if (hex2bin(addr, s, len)) {
> +                       printf("invalid hex string\n");
> +                       return CMD_RET_FAILURE;
> +               }
> +       }
> +
> +       ret = rtc_write(dev, reg, addr, len);
> +       if (ret) {
> +               printf("rtc_write() failed: %d\n", ret);
> +               ret = CMD_RET_FAILURE;
> +       } else {
> +               ret = CMD_RET_SUCCESS;
> +       }
> +
> +       if (argc == 3)
> +               unmap_sysmem(addr);
> +       return ret;
> +}
> +
> +int do_rtc(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +       static int curr_rtc = 0;
> +       struct udevice *dev;
> +       int ret, idx;
> +
> +       if (argc < 2)
> +               return CMD_RET_USAGE;
> +
> +       argc--;
> +       argv++;
> +
> +       if (!strcmp(argv[0], "list")) {

It is comment in U-Boot to just check the letters that are needed. So
here you could do (*argv[0] == 'l')
> +               struct uclass *uc;
> +               idx = 0;
> +
> +               uclass_id_foreach_dev(UCLASS_RTC, dev, uc) {
> +                       printf("RTC #%d - %s\n", idx++, dev->name);
> +               }
> +               if (!idx) {
> +                       printf("*** no RTC devices available ***\n");
> +                       return CMD_RET_FAILURE;
> +               }
> +               return CMD_RET_SUCCESS;
> +       }
> +
> +       idx = curr_rtc;
> +       if (!strcmp(argv[0], "dev") && argc >= 2)
> +               idx = simple_strtoul(argv[1], NULL, 10);
> +
> +       ret = uclass_get_device(UCLASS_RTC, idx, &dev);
> +       if (ret) {
> +               printf("Cannot find RTC #%d: err=%d\n", idx, ret);
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       if (!strcmp(argv[0], "dev")) {
> +               /* Show the existing or newly selected RTC */
> +               if (argc >= 2)
> +                       curr_rtc = idx;
> +               printf("RTC #%d - %s\n", idx, dev->name);
> +               return CMD_RET_SUCCESS;
> +       }
> +
> +       if (!strcmp(argv[0], "read"))
> +               return do_rtc_read(dev, argc - 1, argv + 1);
> +
> +       if (!strcmp(argv[0], "write"))
> +               return do_rtc_write(dev, argc - 1, argv + 1);
> +
> +       return CMD_RET_USAGE;
> +}
> +
> +U_BOOT_CMD(
> +       rtc,    5,      0,      do_rtc,
> +       "RTC subsystem",
> +       "list                        - show available rtc devices\n"
> +       "rtc dev [n]                     - show or set current rtc device\n"
> +       "rtc read <reg> <count>          - read and display 8-bit registers starting at <reg>\n"
> +       "rtc read <reg> <count> <addr>   - read 8-bit registers starting at <reg> to memory <addr>\n"
> +       "rtc write <reg> <hexstring>     - write 8-bit registers starting at <reg>\n"
> +       "rtc write <reg> <count> <addr>  - write from memory <addr> to 8-bit registers starting at <reg>\n"
> +);
> --
> 2.23.0
>

Regards,
Simon

[PATCH v2 07/10] rtc: sandbox-rtc: fix set method

[Simon Glass]

On Tue, 19 May 2020 at 16:01, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> The current set method is broken; a simple test case is to first set
> the date to something in April, then change the date to 31st May:
>
> => date 040412122020.34
> Date: 2020-04-04 (Saturday)    Time: 12:12:34
> => date 053112122020.34
> Date: 2020-05-01 (Friday)    Time: 12:12:34
>
> or via the amending of the existing rtc_set_get test case similarly:
>
> $ ./u-boot -T -v
> => ut dm rtc_set_get
> Test: dm_test_rtc_set_get: rtc.c
> expected: 31/08/2004 18:18:00
> actual: 01/08/2004 18:18:00
>
> The problem is that after each register write,
> sandbox_i2c_rtc_complete_write() gets called and sets the internal
> time from the current set of registers. However, when we get to
> writing 31 to mday, the registers are in an inconsistent state (mon is
> still 4), so the mktime machinery ends up translating April 31st to
> May 1st. Upon the next register write, the registers are populated by
> sandbox_i2c_rtc_prepare_read(), so the 31 we just wrote to mday gets
> overwritten by a 1.
>
> Fix it by writing all registers at once, and for consistency, update
> the get method to retrieve them all with one "i2c transfer".
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/rtc/sandbox_rtc.c | 65 +++++++++++++++------------------------
>  test/dm/rtc.c             | 15 ++++++++-
>  2 files changed, 38 insertions(+), 42 deletions(-)

Nice fix!

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 08/10] rtc: i2c_rtc_emul: catch any write to the "reset" register

[Simon Glass]

On Tue, 19 May 2020 at 16:01, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> It's more natural that any write that happens to touch the reset
> register should cause a reset, rather than just a write that starts at
> that offset.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/rtc/i2c_rtc_emul.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 09/10] test: dm: rtc: add test of rtc_read, rtc_write

[Simon Glass]

On Tue, 19 May 2020 at 16:01, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Define a few aux registers and check that they can be read/written
> individually. Also check that one can access the time-keeping
> registers directly and get the expected results.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  arch/sandbox/include/asm/rtc.h |  5 ++++
>  test/dm/rtc.c                  | 45 ++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 10/10] test: dm: rtc: add tests of rtc shell command

[Simon Glass]

On Tue, 19 May 2020 at 16:01, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  test/dm/rtc.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

But please add a commit message.

[PATCH v2 06/10] rtc: add rtc command

[Rasmus Villemoes]

On 31/05/2020 16.07, Simon Glass wrote:
> Hi Rasmus,
> 
> On Tue, 19 May 2020 at 16:01, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>

>> +static int do_rtc_read(struct udevice *dev, int argc, char * const argv[])
>> +{
>> +       u8 buf[MAX_RTC_BYTES];
>> +       int reg, len, ret;
>> +       u8 *addr;
>> +
>> +       if (argc < 2 || argc > 3)
>> +               return CMD_RET_USAGE;
>> +
>> +       reg = simple_strtoul(argv[0], NULL, 0);
> 
> I think these should be hex (i.e. 16), since that is the norm in U-Boot.

OK.

>> +       len = simple_strtoul(argv[1], NULL, 0);
>> +       if (argc == 3) {
>> +               addr = map_sysmem(simple_strtoul(argv[2], NULL, 16), len);
>> +       } else {
>> +               if (len > sizeof(buf)) {
>> +                       printf("can read at most %d registers at a time\n", MAX_RTC_BYTES);
> 
> It would be better to loop like print_buffer() does.

Both read and write have been rewritten to avoid that arbitrary limit.

>> +
>> +       if (argc == 2) {
>> +               while (len--)
>> +                       printf("%d: 0x%02x\n", reg++, *addr++);
> 
> Perhaps use print_buffer()?

Done.

>> +               const char *s = argv[1];
>> +
>> +               /* Convert hexstring, bail out if too long. */
>> +               addr = buf;
>> +               len = strlen(s);
>> +               if (len > 2*MAX_RTC_BYTES) {
> 
> Spaces around *
> 
>> +                       printf("hex string too long, can write at most %d bytes\n", MAX_RTC_BYTES);
> 
> Please can you try checkpatch or patman? This lines seems too long

The rewrite to avoid the 32 byte limit made me handle the "argc==3" case
separately (it wasn't worth the complexity trying to stick to just one
rtc_{read,write} call, which also automatically dealt with this one.

>> +
>> +int do_rtc(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> +       static int curr_rtc = 0;
>> +       struct udevice *dev;
>> +       int ret, idx;
>> +
>> +       if (argc < 2)
>> +               return CMD_RET_USAGE;
>> +
>> +       argc--;
>> +       argv++;
>> +
>> +       if (!strcmp(argv[0], "list")) {
> 
> It is comment in U-Boot to just check the letters that are needed. So
> here you could do (*argv[0] == 'l')

Yes, and I consider that an anti-pattern. It makes it impossible to
later introduce another (sub)command which starts with a
previously-unique prefix. Now, if that "just type a unique prefix"
wasn't official, so scripts were always supposed to use the full names,
it wouldn't be that big a problem (scripts written for later versions of
U-Boot, or U-Boots configured with more (sub)commands, could still fail
silently if used on an earlier U-Boot or one with fewer (sub)commands
instead of producing a "usage" error message), but
https://www.denx.de/wiki/view/DULG/UBootCommandLineInterface explicitly
mentions that as a feature (and says h can be used for help, which it
can't when the hash command is built in, perfectly exemplifying what I'm
talking about).

Thanks,
Rasmus

[PATCH v2 10/10] test: dm: rtc: add tests of rtc shell command

[Rasmus Villemoes]

On 31/05/2020 16.07, Simon Glass wrote:
> On Tue, 19 May 2020 at 16:01, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>>  test/dm/rtc.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 61 insertions(+)
>>
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> But please add a commit message.

Not sure there's anything more to say than $subject, but sure, I can
repeat that in the body.

Thanks,
Rasmus

[PATCH v2 06/10] rtc: add rtc command

[Simon Glass]

Hi Rasmus,

On Tue, 2 Jun 2020 at 03:13, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 31/05/2020 16.07, Simon Glass wrote:
> > Hi Rasmus,
> >
> > On Tue, 19 May 2020 at 16:01, Rasmus Villemoes
> > <rasmus.villemoes@prevas.dk> wrote:
> >>

[..]

> >> +int do_rtc(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >> +{
> >> +       static int curr_rtc = 0;
> >> +       struct udevice *dev;
> >> +       int ret, idx;
> >> +
> >> +       if (argc < 2)
> >> +               return CMD_RET_USAGE;
> >> +
> >> +       argc--;
> >> +       argv++;
> >> +
> >> +       if (!strcmp(argv[0], "list")) {
> >
> > It is comment in U-Boot to just check the letters that are needed. So
> > here you could do (*argv[0] == 'l')
>
> Yes, and I consider that an anti-pattern. It makes it impossible to
> later introduce another (sub)command which starts with a
> previously-unique prefix. Now, if that "just type a unique prefix"
> wasn't official, so scripts were always supposed to use the full names,
> it wouldn't be that big a problem (scripts written for later versions of
> U-Boot, or U-Boots configured with more (sub)commands, could still fail
> silently if used on an earlier U-Boot or one with fewer (sub)commands
> instead of producing a "usage" error message), but
> https://www.denx.de/wiki/view/DULG/UBootCommandLineInterface explicitly
> mentions that as a feature (and says h can be used for help, which it
> can't when the hash command is built in, perfectly exemplifying what I'm
> talking about).

Hah funny. Using an abbreviation is only possible if no other command
starts with the same leters.

It is certainly very risky to use abbreviations in scripts. I would
not recommend it. Abbreviations are for interactive use. If you have
auto-completion on you can use tab.

But here we are talking about a sub-command, which is a bit more
controlled, in that it doesn't depend on what other commands the user
enables.

Anyway, it's up to you what you want to do here.

Regards,
Simon

[PATCH v2 06/10] rtc: add rtc command

[Rasmus Villemoes]

On 02/06/2020 15.22, Simon Glass wrote:
> Hi Rasmus,
> 
> On Tue, 2 Jun 2020 at 03:13, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>> On 31/05/2020 16.07, Simon Glass wrote:
>>> Hi Rasmus,
>>>
>>> On Tue, 19 May 2020 at 16:01, Rasmus Villemoes
>>> <rasmus.villemoes@prevas.dk> wrote:
>>>>
> 
> [..]
> 
>>>> +int do_rtc(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>> +{
>>>> +       static int curr_rtc = 0;
>>>> +       struct udevice *dev;
>>>> +       int ret, idx;
>>>> +
>>>> +       if (argc < 2)
>>>> +               return CMD_RET_USAGE;
>>>> +
>>>> +       argc--;
>>>> +       argv++;
>>>> +
>>>> +       if (!strcmp(argv[0], "list")) {
>>>
>>> It is comment in U-Boot to just check the letters that are needed. So
>>> here you could do (*argv[0] == 'l')
>>
>> Yes, and I consider that an anti-pattern. It makes it impossible to
>> later introduce another (sub)command which starts with a
>> previously-unique prefix. Now, if that "just type a unique prefix"
>> wasn't official, so scripts were always supposed to use the full names,
>> it wouldn't be that big a problem (scripts written for later versions of
>> U-Boot, or U-Boots configured with more (sub)commands, could still fail
>> silently if used on an earlier U-Boot or one with fewer (sub)commands
>> instead of producing a "usage" error message), but
>> https://www.denx.de/wiki/view/DULG/UBootCommandLineInterface explicitly
>> mentions that as a feature (and says h can be used for help, which it
>> can't when the hash command is built in, perfectly exemplifying what I'm
>> talking about).
> 
> Hah funny. Using an abbreviation is only possible if no other command
> starts with the same leters.
> 
> It is certainly very risky to use abbreviations in scripts. I would
> not recommend it. Abbreviations are for interactive use. If you have
> auto-completion on you can use tab.

Exactly, so the ability to use the abbreviated form doesn't really buy
anything - it's risky in scripts, and interactively, it merely saves a
tab keystroke (and that's all lost in the cognitive overhead of having
to remember just what abbrev is enough).

> But here we are talking about a sub-command, which is a bit more
> controlled, in that it doesn't depend on what other commands the user
> enables.

True, but the same point applies; if I allowed "rtc w", one couldn't
easily later add an "rtc wobble" subcommand (ok, my imagination is
lacking, but you get the idea).

> Anyway, it's up to you what you want to do here.

In that case I'll keep checking for the full name of subcommands.

Thanks,
Rasmus

[PATCH v2 00/10] new rtc methods, rtc command, and tests

[Rasmus Villemoes]

On 20/05/2020 00.01, Rasmus Villemoes wrote:
> I need access to registers other than just the timekeeping ones of the
> pcf2127, so I wanted to implement ->read8 and ->write8. But for
> testing these it appeared there was no convenient way to invoke those
> from the shell, so I also ended up adding such a command.
> 
> Also, it seemed more natural to provide array variants that can read
> or write several registers at once, so rtc_ops is expanded a bit.
> 
> Changes in v2:
> 
> - Use simply "read" and "write" instead of "read8_array",
>   "write8_array", both for functions and methods, as suggested by
>   Simon.

Urgh. The name rtc_read() is already used for a local helper by a number
of rtc drivers (also rtc_write, for somewhat fewer drivers). So I can
still call the methods ->read and ->write, but the functions will need
another name. Probably dm_rtc_read/dm_rtc_write, since this is only for
DM-enabled drivers anyway, and matches the existing dm_rtc_get/dm_rtc_set.

Rasmus

[PATCH v2 06/10] rtc: add rtc command

[Simon Glass]

Hi Rasmus,

On Tue, 2 Jun 2020 at 08:36, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 02/06/2020 15.22, Simon Glass wrote:
> > Hi Rasmus,
> >
> > On Tue, 2 Jun 2020 at 03:13, Rasmus Villemoes
> > <rasmus.villemoes@prevas.dk> wrote:
> >>
> >> On 31/05/2020 16.07, Simon Glass wrote:
> >>> Hi Rasmus,
> >>>
> >>> On Tue, 19 May 2020 at 16:01, Rasmus Villemoes
> >>> <rasmus.villemoes@prevas.dk> wrote:
> >>>>
> >
> > [..]
> >
> >>>> +int do_rtc(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >>>> +{
> >>>> +       static int curr_rtc = 0;
> >>>> +       struct udevice *dev;
> >>>> +       int ret, idx;
> >>>> +
> >>>> +       if (argc < 2)
> >>>> +               return CMD_RET_USAGE;
> >>>> +
> >>>> +       argc--;
> >>>> +       argv++;
> >>>> +
> >>>> +       if (!strcmp(argv[0], "list")) {
> >>>
> >>> It is comment in U-Boot to just check the letters that are needed. So
> >>> here you could do (*argv[0] == 'l')
> >>
> >> Yes, and I consider that an anti-pattern. It makes it impossible to
> >> later introduce another (sub)command which starts with a
> >> previously-unique prefix. Now, if that "just type a unique prefix"
> >> wasn't official, so scripts were always supposed to use the full names,
> >> it wouldn't be that big a problem (scripts written for later versions of
> >> U-Boot, or U-Boots configured with more (sub)commands, could still fail
> >> silently if used on an earlier U-Boot or one with fewer (sub)commands
> >> instead of producing a "usage" error message), but
> >> https://www.denx.de/wiki/view/DULG/UBootCommandLineInterface explicitly
> >> mentions that as a feature (and says h can be used for help, which it
> >> can't when the hash command is built in, perfectly exemplifying what I'm
> >> talking about).
> >
> > Hah funny. Using an abbreviation is only possible if no other command
> > starts with the same leters.
> >
> > It is certainly very risky to use abbreviations in scripts. I would
> > not recommend it. Abbreviations are for interactive use. If you have
> > auto-completion on you can use tab.
>
> Exactly, so the ability to use the abbreviated form doesn't really buy
> anything - it's risky in scripts, and interactively, it merely saves a
> tab keystroke (and that's all lost in the cognitive overhead of having
> to remember just what abbrev is enough).

Not quite:
- tab is an extra, unnecessary keystroke
- many comments don't implement auto-complete (e.g. try 'gpio i'
- auto-complete adds to code size
- fully checking the string adds to code size

>
> > But here we are talking about a sub-command, which is a bit more
> > controlled, in that it doesn't depend on what other commands the user
> > enables.
>
> True, but the same point applies; if I allowed "rtc w", one couldn't
> easily later add an "rtc wobble" subcommand (ok, my imagination is
> lacking, but you get the idea).
>
> > Anyway, it's up to you what you want to do here.
>
> In that case I'll keep checking for the full name of subcommands.

OK.

Regards,
Simon

[PATCH v2 00/10] new rtc methods, rtc command, and tests

[Simon Glass]

Hi Rasmus,

On Tue, 2 Jun 2020 at 12:40, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 20/05/2020 00.01, Rasmus Villemoes wrote:
> > I need access to registers other than just the timekeeping ones of the
> > pcf2127, so I wanted to implement ->read8 and ->write8. But for
> > testing these it appeared there was no convenient way to invoke those
> > from the shell, so I also ended up adding such a command.
> >
> > Also, it seemed more natural to provide array variants that can read
> > or write several registers at once, so rtc_ops is expanded a bit.
> >
> > Changes in v2:
> >
> > - Use simply "read" and "write" instead of "read8_array",
> >   "write8_array", both for functions and methods, as suggested by
> >   Simon.
>
> Urgh. The name rtc_read() is already used for a local helper by a number
> of rtc drivers (also rtc_write, for somewhat fewer drivers). So I can
> still call the methods ->read and ->write, but the functions will need
> another name. Probably dm_rtc_read/dm_rtc_write, since this is only for
> DM-enabled drivers anyway, and matches the existing dm_rtc_get/dm_rtc_set.

The conflict is OK, since at some point those drivers will be updated
to DM or removed. I'd rather avoid the dm_ prefix if not necessary.

Regards,
Simon

[PATCH v2 00/10] new rtc methods, rtc command, and tests

[Rasmus Villemoes]

On 02/06/2020 21.29, Simon Glass wrote:
> Hi Rasmus,
> 
> On Tue, 2 Jun 2020 at 12:40, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>> Urgh. The name rtc_read() is already used for a local helper by a number
>> of rtc drivers (also rtc_write, for somewhat fewer drivers). So I can
>> still call the methods ->read and ->write, but the functions will need
>> another name. Probably dm_rtc_read/dm_rtc_write, since this is only for
>> DM-enabled drivers anyway, and matches the existing dm_rtc_get/dm_rtc_set.
> 
> The conflict is OK, since at some point those drivers will be updated
> to DM or removed. I'd rather avoid the dm_ prefix if not necessary.

There are some DM-enabled drivers that still use those names as local
helpers, e.g. rx8025.c and pt7c4338.c.

Rasmus

[PATCH v2 00/10] new rtc methods, rtc command, and tests

[Simon Glass]

Hi Rasmus,

On Tue, 2 Jun 2020 at 13:44, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 02/06/2020 21.29, Simon Glass wrote:
> > Hi Rasmus,
> >
> > On Tue, 2 Jun 2020 at 12:40, Rasmus Villemoes
> > <rasmus.villemoes@prevas.dk> wrote:
> >>
> >> Urgh. The name rtc_read() is already used for a local helper by a number
> >> of rtc drivers (also rtc_write, for somewhat fewer drivers). So I can
> >> still call the methods ->read and ->write, but the functions will need
> >> another name. Probably dm_rtc_read/dm_rtc_write, since this is only for
> >> DM-enabled drivers anyway, and matches the existing dm_rtc_get/dm_rtc_set.
> >
> > The conflict is OK, since at some point those drivers will be updated
> > to DM or removed. I'd rather avoid the dm_ prefix if not necessary.
>
> There are some DM-enabled drivers that still use those names as local
> helpers, e.g. rx8025.c and pt7c4338.c.

OK then they probably need a prefix of the driver name on those functions.

Regards,
Simon