[PATCH 0/4] reboot SiFive Unmatched via PMIC

[PATCH 0/4] reboot SiFive Unmatched via PMIC

[Nikita Shubin]

From: Nikita Shubin <n.shubin@yadro.com>

This series introduce rebooting via i2c PMIC, currently on
SiFive Unmatched board.

teseted via Linux with reset extension and direct ecall from 
u-boot.

With this it becomes possible to reboot/shutdown the board 
if Linux or u-boot is accompanied with OpenSBI reset extension.

"gpio-poweroff" is required to be removed from u-boot dts file, as
it overrides fdt_reset_da9063.

fdt_reset_da9063 is loaded if "dlg,da9063" is present in device tree.

Moreover it's look like we require a convenient "sleep" functionality,
currently i've implemented sleep via CSR_MCYCLE, assuming we don't normally
communicate with device while operational only on start/shutdown.

```
static inline void wait_cycles(unsigned long cycles)
{
	csr_write(CSR_MCYCLE, 0);
	while (cycles > csr_read_num(CSR_MCYCLE));
}
```

The original sequence was discovered by Alexandre Ghiti posted a patch on 
linux-riscv mail-lists.

https://patchwork.kernel.org/project/linux-riscv/patch/20210921053356.1705833-1-alexandre.ghiti at canonical.com/

These magick numbers are readed as:
- select PAGE0
- set WAKE_UP bit in CONTROL_A reg
- mask setting POWER1, POWER domain and STANDBY, and poweroff SYSTEM domain

Nikita Shubin (4):
  lib: utils/i2c: Add generic I2C configuration library
  lib: utils/gpio: Add simple FDT based I2C framework
  lib: utils/gpio: Add minimal SiFive I2C driver
  lib: utils/reset: Add generic da9063 reset driver

 include/sbi_utils/i2c/fdt_i2c.h    |  26 +++
 include/sbi_utils/i2c/i2c.h        |  65 +++++++
 lib/utils/i2c/fdt_i2c.c            | 110 +++++++++++
 lib/utils/i2c/fdt_i2c_sifive.c     | 289 +++++++++++++++++++++++++++++
 lib/utils/i2c/i2c.c                | 102 ++++++++++
 lib/utils/i2c/objects.mk           |  12 ++
 lib/utils/reset/fdt_reset.c        |   2 +
 lib/utils/reset/fdt_reset_da9063.c | 203 ++++++++++++++++++++
 lib/utils/reset/objects.mk         |   1 +
 9 files changed, 810 insertions(+)
 create mode 100644 include/sbi_utils/i2c/fdt_i2c.h
 create mode 100644 include/sbi_utils/i2c/i2c.h
 create mode 100644 lib/utils/i2c/fdt_i2c.c
 create mode 100644 lib/utils/i2c/fdt_i2c_sifive.c
 create mode 100644 lib/utils/i2c/i2c.c
 create mode 100644 lib/utils/i2c/objects.mk
 create mode 100644 lib/utils/reset/fdt_reset_da9063.c

-- 
2.31.1

[PATCH 1/4] lib: utils/i2c: Add generic I2C configuration library

[Nikita Shubin]

From: Nikita Shubin <n.shubin@yadro.com>

Helper library to keep track of registered I2C adapters,
identified by dts offset, basic send/read functions and
adapter configuration (enable, set dividers, etc...).

Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
---
 include/sbi_utils/i2c/i2c.h |  65 +++++++++++++++++++++++
 lib/utils/i2c/i2c.c         | 102 ++++++++++++++++++++++++++++++++++++
 lib/utils/i2c/objects.mk    |  10 ++++
 3 files changed, 177 insertions(+)
 create mode 100644 include/sbi_utils/i2c/i2c.h
 create mode 100644 lib/utils/i2c/i2c.c
 create mode 100644 lib/utils/i2c/objects.mk

diff --git a/include/sbi_utils/i2c/i2c.h b/include/sbi_utils/i2c/i2c.h
new file mode 100644
index 0000000..86dd582
--- /dev/null
+++ b/include/sbi_utils/i2c/i2c.h
@@ -0,0 +1,65 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright (c) 2021 YADRO
+ *
+ * Authors:
+ *   Nikita Shubin <nshubin@yadro.com>
+ */
+
+#ifndef __I2C_H__
+#define __I2C_H__
+
+#include <sbi/sbi_types.h>
+
+/** Representation of a I2C adapter */
+struct i2c_adapter {
+	/** Pointer to I2C driver owning this I2C adapter */
+	void *driver;
+
+	/** Uniquie ID of the I2C adapter assigned by the driver */
+	int id;
+
+	/**
+	 * Configure I2C adapter
+	 *
+	 * Enable, set dividers, etc...
+	 *
+	 * @return 0 on success and negative error code on failure
+	 */
+	int (*configure)(struct i2c_adapter *ia);
+
+	/**
+	 * Send byte to given address, register
+	 *
+	 * @return 0 on success and negative error code on failure
+	 */
+	int (*send)(struct i2c_adapter *ia, uint8_t addr, uint8_t reg, uint8_t value);
+
+	/**
+	 * Read byte from given address, register
+	 *
+	 * @return 0 on success and negative error code on failure
+	 */
+	int (*read)(struct i2c_adapter *ia, uint8_t addr, uint8_t reg, uint8_t *value);
+};
+
+/** Find a registered I2C adapter */
+struct i2c_adapter *i2c_adapter_find(int id);
+
+/** Register I2C adapter */
+int i2c_adapter_add(struct i2c_adapter *ia);
+
+/** Un-register I2C adapter */
+void i2c_adapter_remove(struct i2c_adapter *ia);
+
+/** Configure I2C adapter prior to send/read */
+int i2c_adapter_configure(struct i2c_adapter *ia);
+
+/** Send to device on I2C adapter bus */
+int i2c_adapter_send(struct i2c_adapter *ia, uint8_t addr, uint8_t reg, uint8_t value);
+
+/** Read from device on I2C adapter bus */
+int i2c_adapter_read(struct i2c_adapter *ia, uint8_t addr, uint8_t reg, uint8_t *value);
+
+#endif
diff --git a/lib/utils/i2c/i2c.c b/lib/utils/i2c/i2c.c
new file mode 100644
index 0000000..9bcb129
--- /dev/null
+++ b/lib/utils/i2c/i2c.c
@@ -0,0 +1,102 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright (c) 2021 YADRO
+ *
+ * Authors:
+ *   Nikita Shubin <nshubin@yadro.com>
+ *
+ * derivate: lib/utils/gpio/gpio.c
+ * Authors:
+ *   Anup Patel <anup.patel@wdc.com>
+ */
+
+#include <sbi/sbi_error.h>
+#include <sbi_utils/i2c/i2c.h>
+
+#define I2C_ADAPTER_MAX		16
+
+static struct i2c_adapter *i2c_array[I2C_ADAPTER_MAX];
+
+struct i2c_adapter *i2c_adapter_find(int id)
+{
+	unsigned int i;
+	struct i2c_adapter *ret = NULL;
+
+	for (i = 0; i < I2C_ADAPTER_MAX; i++) {
+		if (i2c_array[i] && i2c_array[i]->id == id) {
+			ret = i2c_array[i];
+			break;
+		}
+	}
+
+	return ret;
+}
+
+int i2c_adapter_add(struct i2c_adapter *ia)
+{
+	int i, ret = SBI_ENOSPC;
+
+	if (!ia)
+		return SBI_EINVAL;
+	if (i2c_adapter_find(ia->id))
+		return SBI_EALREADY;
+
+	for (i = 0; i < I2C_ADAPTER_MAX; i++) {
+		if (!i2c_array[i]) {
+			i2c_array[i] = ia;
+			ret = 0;
+			break;
+		}
+	}
+
+	return ret;
+}
+
+void i2c_adapter_remove(struct i2c_adapter *ia)
+{
+	int i;
+
+	if (!ia)
+		return;
+
+	for (i = 0; i < I2C_ADAPTER_MAX; i++) {
+		if (i2c_array[i] == ia) {
+			i2c_array[i] = NULL;
+			break;
+		}
+	}
+}
+
+int i2c_adapter_configure(struct i2c_adapter *ia)
+{
+	if (!ia)
+		return SBI_EINVAL;
+	if (!ia->configure)
+		return 0;
+
+	return ia->configure(ia);
+}
+
+int i2c_adapter_send(struct i2c_adapter *ia, uint8_t addr,
+		     uint8_t reg, uint8_t value)
+{
+	if (!ia)
+		return SBI_EINVAL;
+	if (!ia->send)
+		return SBI_ENOSYS;
+
+	return ia->send(ia, addr, reg, value);
+}
+
+
+int i2c_adapter_read(struct i2c_adapter *ia, uint8_t addr,
+		     uint8_t reg, uint8_t *value)
+{
+	if (!ia)
+		return SBI_EINVAL;
+	if (!ia->read)
+		return SBI_ENOSYS;
+
+	return ia->read(ia, addr, reg, value);
+}
diff --git a/lib/utils/i2c/objects.mk b/lib/utils/i2c/objects.mk
new file mode 100644
index 0000000..16a70da
--- /dev/null
+++ b/lib/utils/i2c/objects.mk
@@ -0,0 +1,10 @@
+#
+# SPDX-License-Identifier: BSD-2-Clause
+#
+# Copyright (c) 2021 YADRO
+#
+# Authors:
+#   Nikita Shubin <nshubin@yadro.com>
+#
+
+libsbiutils-objs-y += i2c/i2c.o
-- 
2.31.1

[PATCH 1/4] lib: utils/i2c: Add generic I2C configuration library

[Xiang W]

? 2021-09-24???? 14:33 +0300?Nikita Shubin???
> From: Nikita Shubin <n.shubin@yadro.com>
> 
> Helper library to keep track of registered I2C adapters,
> identified by dts offset, basic send/read functions and
> adapter configuration (enable, set dividers, etc...).
> 
> Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
> ---
> ?include/sbi_utils/i2c/i2c.h |? 65 +++++++++++++++++++++++
> ?lib/utils/i2c/i2c.c???????? | 102
> ++++++++++++++++++++++++++++++++++++
> ?lib/utils/i2c/objects.mk??? |? 10 ++++
> ?3 files changed, 177 insertions(+)
> ?create mode 100644 include/sbi_utils/i2c/i2c.h
> ?create mode 100644 lib/utils/i2c/i2c.c
> ?create mode 100644 lib/utils/i2c/objects.mk
> 
> diff --git a/include/sbi_utils/i2c/i2c.h
> b/include/sbi_utils/i2c/i2c.h
> new file mode 100644
> index 0000000..86dd582
> --- /dev/null
> +++ b/include/sbi_utils/i2c/i2c.h
> @@ -0,0 +1,65 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright (c) 2021 YADRO
> + *
> + * Authors:
> + *?? Nikita Shubin <nshubin@yadro.com>
> + */
> +
> +#ifndef __I2C_H__
> +#define __I2C_H__
> +
> +#include <sbi/sbi_types.h>
> +
> +/** Representation of a I2C adapter */
> +struct i2c_adapter {
I suggest adding an object sbi_dlist to form a linked list

> +???????/** Pointer to I2C driver owning this I2C adapter */
> +???????void *driver;
> +
> +???????/** Uniquie ID of the I2C adapter assigned by the driver */
> +???????int id;
> +
> +???????/**
> +??????? * Configure I2C adapter
> +??????? *
> +??????? * Enable, set dividers, etc...
> +??????? *
> +??????? * @return 0 on success and negative error code on failure
> +??????? */
> +???????int (*configure)(struct i2c_adapter *ia);
> +
> +???????/**
> +??????? * Send byte to given address, register
> +??????? *
> +??????? * @return 0 on success and negative error code on failure
> +??????? */
> +???????int (*send)(struct i2c_adapter *ia, uint8_t addr, uint8_t
> reg, uint8_t value);
> +
> +???????/**
> +??????? * Read byte from given address, register
> +??????? *
> +??????? * @return 0 on success and negative error code on failure
> +??????? */
> +???????int (*read)(struct i2c_adapter *ia, uint8_t addr, uint8_t
> reg, uint8_t *value);
> +};
> +
> +/** Find a registered I2C adapter */
> +struct i2c_adapter *i2c_adapter_find(int id);
> +
> +/** Register I2C adapter */
> +int i2c_adapter_add(struct i2c_adapter *ia);
> +
> +/** Un-register I2C adapter */
> +void i2c_adapter_remove(struct i2c_adapter *ia);
> +
> +/** Configure I2C adapter prior to send/read */
> +int i2c_adapter_configure(struct i2c_adapter *ia);
> +
> +/** Send to device on I2C adapter bus */
> +int i2c_adapter_send(struct i2c_adapter *ia, uint8_t addr, uint8_t
> reg, uint8_t value);
> +
> +/** Read from device on I2C adapter bus */
> +int i2c_adapter_read(struct i2c_adapter *ia, uint8_t addr, uint8_t
> reg, uint8_t *value);
> +
> +#endif
> diff --git a/lib/utils/i2c/i2c.c b/lib/utils/i2c/i2c.c
> new file mode 100644
> index 0000000..9bcb129
> --- /dev/null
> +++ b/lib/utils/i2c/i2c.c
> @@ -0,0 +1,102 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright (c) 2021 YADRO
> + *
> + * Authors:
> + *?? Nikita Shubin <nshubin@yadro.com>
> + *
> + * derivate: lib/utils/gpio/gpio.c
> + * Authors:
> + *?? Anup Patel <anup.patel@wdc.com>
> + */
> +
> +#include <sbi/sbi_error.h>
> +#include <sbi_utils/i2c/i2c.h>
> +
> +#define I2C_ADAPTER_MAX????????????????16
If it is implemented through a linked list, there will be no limit on
the number

Regards,
Xiang W
> +
> +static struct i2c_adapter *i2c_array[I2C_ADAPTER_MAX];
> +
> +struct i2c_adapter *i2c_adapter_find(int id)
> +{
> +???????unsigned int i;
> +???????struct i2c_adapter *ret = NULL;
> +
> +???????for (i = 0; i < I2C_ADAPTER_MAX; i++) {
> +???????????????if (i2c_array[i] && i2c_array[i]->id == id) {
> +???????????????????????ret = i2c_array[i];
> +???????????????????????break;
> +???????????????}
> +???????}
> +
> +???????return ret;
> +}
> +
> +int i2c_adapter_add(struct i2c_adapter *ia)
> +{
> +???????int i, ret = SBI_ENOSPC;
> +
> +???????if (!ia)
> +???????????????return SBI_EINVAL;
> +???????if (i2c_adapter_find(ia->id))
> +???????????????return SBI_EALREADY;
> +
> +???????for (i = 0; i < I2C_ADAPTER_MAX; i++) {
> +???????????????if (!i2c_array[i]) {
> +???????????????????????i2c_array[i] = ia;
> +???????????????????????ret = 0;
> +???????????????????????break;
> +???????????????}
> +???????}
> +
> +???????return ret;
> +}
> +
> +void i2c_adapter_remove(struct i2c_adapter *ia)
> +{
> +???????int i;
> +
> +???????if (!ia)
> +???????????????return;
> +
> +???????for (i = 0; i < I2C_ADAPTER_MAX; i++) {
> +???????????????if (i2c_array[i] == ia) {
> +???????????????????????i2c_array[i] = NULL;
> +???????????????????????break;
> +???????????????}
> +???????}
> +}
> +
> +int i2c_adapter_configure(struct i2c_adapter *ia)
> +{
> +???????if (!ia)
> +???????????????return SBI_EINVAL;
> +???????if (!ia->configure)
> +???????????????return 0;
> +
> +???????return ia->configure(ia);
> +}
> +
> +int i2c_adapter_send(struct i2c_adapter *ia, uint8_t addr,
> +??????????????????? uint8_t reg, uint8_t value)
> +{
> +???????if (!ia)
> +???????????????return SBI_EINVAL;
> +???????if (!ia->send)
> +???????????????return SBI_ENOSYS;
> +
> +???????return ia->send(ia, addr, reg, value);
> +}
> +
> +
> +int i2c_adapter_read(struct i2c_adapter *ia, uint8_t addr,
> +??????????????????? uint8_t reg, uint8_t *value)
> +{
> +???????if (!ia)
> +???????????????return SBI_EINVAL;
> +???????if (!ia->read)
> +???????????????return SBI_ENOSYS;
> +
> +???????return ia->read(ia, addr, reg, value);
> +}
> diff --git a/lib/utils/i2c/objects.mk b/lib/utils/i2c/objects.mk
> new file mode 100644
> index 0000000..16a70da
> --- /dev/null
> +++ b/lib/utils/i2c/objects.mk
> @@ -0,0 +1,10 @@
> +#
> +# SPDX-License-Identifier: BSD-2-Clause
> +#
> +# Copyright (c) 2021 YADRO
> +#
> +# Authors:
> +#?? Nikita Shubin <nshubin@yadro.com>
> +#
> +
> +libsbiutils-objs-y += i2c/i2c.o
> -- 
> 2.31.1
> 
> 

[PATCH 1/4] lib: utils/i2c: Add generic I2C configuration library

[Nikita Shubin]

Hello Xiang!

On Sat, 25 Sep 2021 12:19:43 +0800
Xiang W <wxjstz@126.com> wrote:

> > +
> > +#include <sbi/sbi_error.h>
> > +#include <sbi_utils/i2c/i2c.h>
> > +
> > +#define I2C_ADAPTER_MAX????????????????16  
> If it is implemented through a linked list, there will be no limit on
> the number

You are proposing double linked lists like in Linux/BSD? That makes
sense.


Yours,
Nikita Shubin

[PATCH 1/4] lib: utils/i2c: Add generic I2C configuration library

[Alexandre ghiti]

Hi Nikita,

Thanks for jumping in and coming up with this patchset :)

Below my review:

On 9/24/21 1:33 PM, Nikita Shubin wrote:
> From: Nikita Shubin <n.shubin@yadro.com>
>
> Helper library to keep track of registered I2C adapters,
> identified by dts offset, basic send/read functions and
> adapter configuration (enable, set dividers, etc...).
>
> Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
> ---
>  include/sbi_utils/i2c/i2c.h |  65 +++++++++++++++++++++++
>  lib/utils/i2c/i2c.c         | 102 ++++++++++++++++++++++++++++++++++++
>  lib/utils/i2c/objects.mk    |  10 ++++
>  3 files changed, 177 insertions(+)
>  create mode 100644 include/sbi_utils/i2c/i2c.h
>  create mode 100644 lib/utils/i2c/i2c.c
>  create mode 100644 lib/utils/i2c/objects.mk
>
> diff --git a/include/sbi_utils/i2c/i2c.h b/include/sbi_utils/i2c/i2c.h
> new file mode 100644
> index 0000000..86dd582
> --- /dev/null
> +++ b/include/sbi_utils/i2c/i2c.h
> @@ -0,0 +1,65 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright (c) 2021 YADRO
> + *
> + * Authors:
> + *   Nikita Shubin <nshubin@yadro.com>
> + */
> +
> +#ifndef __I2C_H__
> +#define __I2C_H__
> +
> +#include <sbi/sbi_types.h>
> +
> +/** Representation of a I2C adapter */
> +struct i2c_adapter {
> +	/** Pointer to I2C driver owning this I2C adapter */
> +	void *driver;
> +
> +	/** Uniquie ID of the I2C adapter assigned by the driver */


s/Uniquie/Unique


> +	int id;
> +
> +	/**
> +	 * Configure I2C adapter
> +	 *
> +	 * Enable, set dividers, etc...
> +	 *
> +	 * @return 0 on success and negative error code on failure
> +	 */
> +	int (*configure)(struct i2c_adapter *ia);
> +
> +	/**
> +	 * Send byte to given address, register
> +	 *
> +	 * @return 0 on success and negative error code on failure
> +	 */
> +	int (*send)(struct i2c_adapter *ia, uint8_t addr, uint8_t reg, uint8_t value);


send is limited to sending only 1B value? Shouldn't we handle an array
of bytes with a size?


> +
> +	/**
> +	 * Read byte from given address, register
> +	 *
> +	 * @return 0 on success and negative error code on failure
> +	 */
> +	int (*read)(struct i2c_adapter *ia, uint8_t addr, uint8_t reg, uint8_t *value);


Same here.


> +};


The API should be symmetrical i2c_send/i2c_recv or i2c_read/i2c_write
and what about describing the arguments?


> +
> +/** Find a registered I2C adapter */
> +struct i2c_adapter *i2c_adapter_find(int id);
> +
> +/** Register I2C adapter */
> +int i2c_adapter_add(struct i2c_adapter *ia);
> +
> +/** Un-register I2C adapter */
> +void i2c_adapter_remove(struct i2c_adapter *ia);
> +
> +/** Configure I2C adapter prior to send/read */
> +int i2c_adapter_configure(struct i2c_adapter *ia);
> +
> +/** Send to device on I2C adapter bus */
> +int i2c_adapter_send(struct i2c_adapter *ia, uint8_t addr, uint8_t reg, uint8_t value);
> +
> +/** Read from device on I2C adapter bus */
> +int i2c_adapter_read(struct i2c_adapter *ia, uint8_t addr, uint8_t reg, uint8_t *value);
> +
> +#endif
> diff --git a/lib/utils/i2c/i2c.c b/lib/utils/i2c/i2c.c
> new file mode 100644
> index 0000000..9bcb129
> --- /dev/null
> +++ b/lib/utils/i2c/i2c.c
> @@ -0,0 +1,102 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright (c) 2021 YADRO
> + *
> + * Authors:
> + *   Nikita Shubin <nshubin@yadro.com>
> + *
> + * derivate: lib/utils/gpio/gpio.c
> + * Authors:
> + *   Anup Patel <anup.patel@wdc.com>
> + */
> +
> +#include <sbi/sbi_error.h>
> +#include <sbi_utils/i2c/i2c.h>
> +
> +#define I2C_ADAPTER_MAX		16
> +
> +static struct i2c_adapter *i2c_array[I2C_ADAPTER_MAX];
> +
> +struct i2c_adapter *i2c_adapter_find(int id)
> +{
> +	unsigned int i;
> +	struct i2c_adapter *ret = NULL;
> +
> +	for (i = 0; i < I2C_ADAPTER_MAX; i++) {
> +		if (i2c_array[i] && i2c_array[i]->id == id) {
> +			ret = i2c_array[i];
> +			break;


I would have returned directly here: return i2c_array(i].


> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +int i2c_adapter_add(struct i2c_adapter *ia)
> +{
> +	int i, ret = SBI_ENOSPC;
> +
> +	if (!ia)
> +		return SBI_EINVAL;
> +	if (i2c_adapter_find(ia->id))
> +		return SBI_EALREADY;
> +
> +	for (i = 0; i < I2C_ADAPTER_MAX; i++) {
> +		if (!i2c_array[i]) {
> +			i2c_array[i] = ia;
> +			ret = 0;
> +			break;


Same here.


> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +void i2c_adapter_remove(struct i2c_adapter *ia)
> +{
> +	int i;
> +
> +	if (!ia)
> +		return;
> +
> +	for (i = 0; i < I2C_ADAPTER_MAX; i++) {
> +		if (i2c_array[i] == ia) {
> +			i2c_array[i] = NULL;
> +			break;


Same here.


> +		}
> +	}
> +}
> +
> +int i2c_adapter_configure(struct i2c_adapter *ia)
> +{
> +	if (!ia)
> +		return SBI_EINVAL;
> +	if (!ia->configure)
> +		return 0;
> +
> +	return ia->configure(ia);
> +}
> +
> +int i2c_adapter_send(struct i2c_adapter *ia, uint8_t addr,
> +		     uint8_t reg, uint8_t value)
> +{
> +	if (!ia)
> +		return SBI_EINVAL;
> +	if (!ia->send)
> +		return SBI_ENOSYS;
> +
> +	return ia->send(ia, addr, reg, value);
> +}
> +
> +


?2 new lines.


> +int i2c_adapter_read(struct i2c_adapter *ia, uint8_t addr,
> +		     uint8_t reg, uint8_t *value)
> +{
> +	if (!ia)
> +		return SBI_EINVAL;
> +	if (!ia->read)
> +		return SBI_ENOSYS;
> +
> +	return ia->read(ia, addr, reg, value);
> +}


IMO, those helpers are not very helpful. I would have imagined an API
more "device-centric":

int i2c_send(struct i2c_device *dev, uint8_t reg, uint8_t *value)

{

??? return dev->i2c_adapter->send(dev->address, reg, value);

}

And I don't think the configure callback is even needed: it should be
done at the initialization of the adapter, in its own driver, I don't
think the device should care about that.


> diff --git a/lib/utils/i2c/objects.mk b/lib/utils/i2c/objects.mk
> new file mode 100644
> index 0000000..16a70da
> --- /dev/null
> +++ b/lib/utils/i2c/objects.mk
> @@ -0,0 +1,10 @@
> +#
> +# SPDX-License-Identifier: BSD-2-Clause
> +#
> +# Copyright (c) 2021 YADRO
> +#
> +# Authors:
> +#   Nikita Shubin <nshubin@yadro.com>
> +#
> +
> +libsbiutils-objs-y += i2c/i2c.o

[PATCH 1/4] lib: utils/i2c: Add generic I2C configuration library

[Nikita Shubin]

Hi Alexander,

Thank you for your attention.

On Mon, 27 Sep 2021 17:41:18 +0200
Alexandre ghiti <alex@ghiti.fr> wrote:

> And I don't think the configure callback is even needed: it should be
> done at the initialization of the adapter, in its own driver, I don't
> think the device should care about that.

As long as i2c frame is supposed to do something more complicated then
sending a few bytes at the very end and even, may be, at the start of
booting, it is definitely required to configure - setup clocks, enable
the device.

Yours,
Nikita Shubin

[PATCH 1/4] lib: utils/i2c: Add generic I2C configuration library

[Alexandre Ghiti]

On Tue, Sep 28, 2021 at 12:52 PM Nikita Shubin
<nikita.shubin@maquefel.me> wrote:
>
> Hi Alexander,
>
> Thank you for your attention.
>
> On Mon, 27 Sep 2021 17:41:18 +0200
> Alexandre ghiti <alex@ghiti.fr> wrote:
>
> > And I don't think the configure callback is even needed: it should be
> > done at the initialization of the adapter, in its own driver, I don't
> > think the device should care about that.
>
> As long as i2c frame is supposed to do something more complicated then
> sending a few bytes at the very end and even, may be, at the start of
> booting, it is definitely required to configure - setup clocks, enable
> the device.

I agree that some configurations need to be done but the i2c device
should not care about that, it should only ask to send/recv, the
adapter should do the necessary configuration behind the scenes.

>
> Yours,
> Nikita Shubin
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH 2/4] lib: utils/gpio: Add simple FDT based I2C framework

[Nikita Shubin]

From: Nikita Shubin <n.shubin@yadro.com>

FDT based I2C framework on the top of I2C library.

The drivers are probed on demand by fdt_i2c_adapter_get
function.

Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
---
 include/sbi_utils/i2c/fdt_i2c.h |  26 ++++++++
 lib/utils/i2c/fdt_i2c.c         | 107 ++++++++++++++++++++++++++++++++
 lib/utils/i2c/objects.mk        |   1 +
 3 files changed, 134 insertions(+)
 create mode 100644 include/sbi_utils/i2c/fdt_i2c.h
 create mode 100644 lib/utils/i2c/fdt_i2c.c

diff --git a/include/sbi_utils/i2c/fdt_i2c.h b/include/sbi_utils/i2c/fdt_i2c.h
new file mode 100644
index 0000000..1f41a0f
--- /dev/null
+++ b/include/sbi_utils/i2c/fdt_i2c.h
@@ -0,0 +1,26 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright (c) 2021 YADRO
+ *
+ * Authors:
+ *   Nikita Shubin <nshubin@yadro.com>
+ */
+
+#ifndef __FDT_I2C_H__
+#define __FDT_I2C_H__
+
+#include <sbi_utils/i2c/i2c.h>
+
+/** FDT based I2C adapter driver */
+struct fdt_i2c_adapter {
+	const struct fdt_match *match_table;
+	int (*init)(void *fdt, int nodeoff,
+		    const struct fdt_match *match);
+};
+
+/** Get I2C adapter identified by nodeoff */
+int fdt_i2c_adapter_get(void *fdt, int nodeoff,
+			struct i2c_adapter **out_adapter);
+
+#endif
diff --git a/lib/utils/i2c/fdt_i2c.c b/lib/utils/i2c/fdt_i2c.c
new file mode 100644
index 0000000..27e28a4
--- /dev/null
+++ b/lib/utils/i2c/fdt_i2c.c
@@ -0,0 +1,107 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright (c) 2021 YADRO
+ *
+ * Authors:
+ *   Nikita Shubin <nshubin@yadro.com>
+ *
+ * derivate: lib/utils/gpio/fdt_gpio.c
+ * Authors:
+ *   Anup Patel <anup.patel@wdc.com>
+ */
+
+#include <libfdt.h>
+#include <sbi/sbi_error.h>
+#include <sbi_utils/fdt/fdt_helper.h>
+#include <sbi_utils/i2c/fdt_i2c.h>
+
+#include <sbi/sbi_console.h>
+
+static struct fdt_i2c_adapter *i2c_adapter_drivers[] = {
+};
+
+static struct fdt_i2c_adapter *fdt_i2c_adapter_driver(struct i2c_adapter *adapter)
+{
+	int pos;
+
+	if (!adapter)
+		return NULL;
+
+	for (pos = 0; pos < array_size(i2c_adapter_drivers); pos++) {
+		if (adapter->driver == i2c_adapter_drivers[pos])
+			return i2c_adapter_drivers[pos];
+	}
+
+	return NULL;
+}
+
+static int fdt_i2c_adapter_init(void *fdt, int nodeoff)
+{
+	int pos, rc;
+	struct fdt_i2c_adapter *drv;
+	const struct fdt_match *match;
+
+	/* Try all I2C drivers one-by-one */
+	for (pos = 0; pos < array_size(i2c_adapter_drivers); pos++) {
+		drv = i2c_adapter_drivers[pos];
+		match = fdt_match_node(fdt, nodeoff, drv->match_table);
+		if (match && drv->init) {
+			rc = drv->init(fdt, nodeoff, match);
+			if (rc == SBI_ENODEV)
+				continue;
+			if (rc)
+				return rc;
+			return 0;
+		}
+	}
+
+	return SBI_ENOSYS;
+}
+
+static int fdt_i2c_adapter_find(void *fdt, int nodeoff,
+				struct i2c_adapter **out_adapter)
+{
+	int rc;
+	struct i2c_adapter *adapter = i2c_adapter_find(nodeoff);
+
+	if (!adapter) {
+		/* I2C adapter not found so initialize matching driver */
+		rc = fdt_i2c_adapter_init(fdt, nodeoff);
+		if (rc)
+			return rc;
+
+		/* Try to find I2C adapter again */
+		adapter = i2c_adapter_find(nodeoff);
+		if (!adapter)
+			return SBI_ENOSYS;
+	}
+
+	if (out_adapter)
+		*out_adapter = adapter;
+
+	return 0;
+}
+
+int fdt_i2c_adapter_get(void *fdt, int nodeoff,
+			struct i2c_adapter **out_adapter)
+{
+	int rc;
+	struct i2c_adapter *adapter;
+	struct fdt_i2c_adapter *drv;
+
+	if (!fdt || (nodeoff < 0) || !out_adapter)
+		return SBI_EINVAL;
+
+	rc = fdt_i2c_adapter_find(fdt, nodeoff, &adapter);
+	if (rc)
+		return rc;
+
+	drv = fdt_i2c_adapter_driver(adapter);
+	if (!drv)
+		return SBI_ENOSYS;
+
+	*out_adapter = adapter;
+
+	return 0;
+}
diff --git a/lib/utils/i2c/objects.mk b/lib/utils/i2c/objects.mk
index 16a70da..06baa65 100644
--- a/lib/utils/i2c/objects.mk
+++ b/lib/utils/i2c/objects.mk
@@ -8,3 +8,4 @@
 #
 
 libsbiutils-objs-y += i2c/i2c.o
+libsbiutils-objs-y += i2c/fdt_i2c.o
-- 
2.31.1

[PATCH 2/4] lib: utils/gpio: Add simple FDT based I2C framework

[Xiang W]

? 2021-09-24???? 14:33 +0300?Nikita Shubin???
> From: Nikita Shubin <n.shubin@yadro.com>
> 
> FDT based I2C framework on the top of I2C library.
> 
> The drivers are probed on demand by fdt_i2c_adapter_get
> function.
> 
> Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
> ---
> ?include/sbi_utils/i2c/fdt_i2c.h |? 26 ++++++++
> ?lib/utils/i2c/fdt_i2c.c???????? | 107
> ++++++++++++++++++++++++++++++++
> ?lib/utils/i2c/objects.mk??????? |?? 1 +
> ?3 files changed, 134 insertions(+)
> ?create mode 100644 include/sbi_utils/i2c/fdt_i2c.h
> ?create mode 100644 lib/utils/i2c/fdt_i2c.c
> 
> diff --git a/include/sbi_utils/i2c/fdt_i2c.h
> b/include/sbi_utils/i2c/fdt_i2c.h
> new file mode 100644
> index 0000000..1f41a0f
> --- /dev/null
> +++ b/include/sbi_utils/i2c/fdt_i2c.h
> @@ -0,0 +1,26 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright (c) 2021 YADRO
> + *
> + * Authors:
> + *?? Nikita Shubin <nshubin@yadro.com>
> + */
> +
> +#ifndef __FDT_I2C_H__
> +#define __FDT_I2C_H__
> +
> +#include <sbi_utils/i2c/i2c.h>
> +
> +/** FDT based I2C adapter driver */
> +struct fdt_i2c_adapter {
> +???????const struct fdt_match *match_table;
> +???????int (*init)(void *fdt, int nodeoff,
> +?????????????????? const struct fdt_match *match);
> +};
> +
> +/** Get I2C adapter identified by nodeoff */
> +int fdt_i2c_adapter_get(void *fdt, int nodeoff,
> +???????????????????????struct i2c_adapter **out_adapter);
> +
> +#endif
> diff --git a/lib/utils/i2c/fdt_i2c.c b/lib/utils/i2c/fdt_i2c.c
> new file mode 100644
> index 0000000..27e28a4
> --- /dev/null
> +++ b/lib/utils/i2c/fdt_i2c.c
> @@ -0,0 +1,107 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright (c) 2021 YADRO
> + *
> + * Authors:
> + *?? Nikita Shubin <nshubin@yadro.com>
> + *
> + * derivate: lib/utils/gpio/fdt_gpio.c
> + * Authors:
> + *?? Anup Patel <anup.patel@wdc.com>
> + */
> +
> +#include <libfdt.h>
> +#include <sbi/sbi_error.h>
> +#include <sbi_utils/fdt/fdt_helper.h>
> +#include <sbi_utils/i2c/fdt_i2c.h>
> +
> +#include <sbi/sbi_console.h>
> +
> +static struct fdt_i2c_adapter *i2c_adapter_drivers[] = {
> +};
> +
> +static struct fdt_i2c_adapter *fdt_i2c_adapter_driver(struct
> i2c_adapter *adapter)
> +{
> +???????int pos;
> +
> +???????if (!adapter)
> +???????????????return NULL;
> +
> +???????for (pos = 0; pos < array_size(i2c_adapter_drivers); pos++) {
> +???????????????if (adapter->driver == i2c_adapter_drivers[pos])
> +???????????????????????return i2c_adapter_drivers[pos];
> +???????}
> +
> +???????return NULL;
> +}
> +
> +static int fdt_i2c_adapter_init(void *fdt, int nodeoff)
> +{
> +???????int pos, rc;
> +???????struct fdt_i2c_adapter *drv;
> +???????const struct fdt_match *match;
> +
> +???????/* Try all I2C drivers one-by-one */
> +???????for (pos = 0; pos < array_size(i2c_adapter_drivers); pos++) {
> +???????????????drv = i2c_adapter_drivers[pos];
> +???????????????match = fdt_match_node(fdt, nodeoff, drv-
> >match_table);
> +???????????????if (match && drv->init) {
> +???????????????????????rc = drv->init(fdt, nodeoff, match);
> +???????????????????????if (rc == SBI_ENODEV)
> +???????????????????????????????continue;
> +???????????????????????if (rc)
> +???????????????????????????????return rc;
> +???????????????????????return 0;
> +???????????????}
> +???????}
> +
> +???????return SBI_ENOSYS;
> +}
> +
> +static int fdt_i2c_adapter_find(void *fdt, int nodeoff,
> +???????????????????????????????struct i2c_adapter **out_adapter)
> +{
> +???????int rc;
> +???????struct i2c_adapter *adapter = i2c_adapter_find(nodeoff);
> +
> +???????if (!adapter) {
> +???????????????/* I2C adapter not found so initialize matching
> driver */
Reinitializing the I2C adapter may affect other adapters that have
already been initialized. So I suggest to add a static variable,
through this control the initial operation is executed once.

Regards,
Xiang W
> +???????????????rc = fdt_i2c_adapter_init(fdt, nodeoff);
> +???????????????if (rc)
> +???????????????????????return rc;
> +
> +???????????????/* Try to find I2C adapter again */
> +???????????????adapter = i2c_adapter_find(nodeoff);
> +???????????????if (!adapter)
> +???????????????????????return SBI_ENOSYS;
> +???????}
> +
> +???????if (out_adapter)
> +???????????????*out_adapter = adapter;
> +
> +???????return 0;
> +}
> +
> +int fdt_i2c_adapter_get(void *fdt, int nodeoff,
> +???????????????????????struct i2c_adapter **out_adapter)
> +{
> +???????int rc;
> +???????struct i2c_adapter *adapter;
> +???????struct fdt_i2c_adapter *drv;
> +
> +???????if (!fdt || (nodeoff < 0) || !out_adapter)
> +???????????????return SBI_EINVAL;
> +
> +???????rc = fdt_i2c_adapter_find(fdt, nodeoff, &adapter);
> +???????if (rc)
> +???????????????return rc;
> +
> +???????drv = fdt_i2c_adapter_driver(adapter);
> +???????if (!drv)
> +???????????????return SBI_ENOSYS;
> +
> +???????*out_adapter = adapter;
> +
> +???????return 0;
> +}
> diff --git a/lib/utils/i2c/objects.mk b/lib/utils/i2c/objects.mk
> index 16a70da..06baa65 100644
> --- a/lib/utils/i2c/objects.mk
> +++ b/lib/utils/i2c/objects.mk
> @@ -8,3 +8,4 @@
> ?#
> ?
> ?libsbiutils-objs-y += i2c/i2c.o
> +libsbiutils-objs-y += i2c/fdt_i2c.o
> -- 
> 2.31.1
> 
> 

[PATCH 2/4] lib: utils/gpio: Add simple FDT based I2C framework

[Alexandre ghiti]

Patch title uses 'gpio' instead of 'i2c'.

On 9/24/21 1:33 PM, Nikita Shubin wrote:
> From: Nikita Shubin <n.shubin@yadro.com>
>
> FDT based I2C framework on the top of I2C library.
>
> The drivers are probed on demand by fdt_i2c_adapter_get
> function.
>
> Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
> ---
>  include/sbi_utils/i2c/fdt_i2c.h |  26 ++++++++
>  lib/utils/i2c/fdt_i2c.c         | 107 ++++++++++++++++++++++++++++++++
>  lib/utils/i2c/objects.mk        |   1 +
>  3 files changed, 134 insertions(+)
>  create mode 100644 include/sbi_utils/i2c/fdt_i2c.h
>  create mode 100644 lib/utils/i2c/fdt_i2c.c
>
> diff --git a/include/sbi_utils/i2c/fdt_i2c.h b/include/sbi_utils/i2c/fdt_i2c.h
> new file mode 100644
> index 0000000..1f41a0f
> --- /dev/null
> +++ b/include/sbi_utils/i2c/fdt_i2c.h
> @@ -0,0 +1,26 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright (c) 2021 YADRO
> + *
> + * Authors:
> + *   Nikita Shubin <nshubin@yadro.com>
> + */
> +
> +#ifndef __FDT_I2C_H__
> +#define __FDT_I2C_H__
> +
> +#include <sbi_utils/i2c/i2c.h>
> +
> +/** FDT based I2C adapter driver */
> +struct fdt_i2c_adapter {
> +	const struct fdt_match *match_table;
> +	int (*init)(void *fdt, int nodeoff,
> +		    const struct fdt_match *match);
> +};


This structure name could contain "driver", because we don't really know
what it is as is.


> +
> +/** Get I2C adapter identified by nodeoff */
> +int fdt_i2c_adapter_get(void *fdt, int nodeoff,
> +			struct i2c_adapter **out_adapter);
> +
> +#endif
> diff --git a/lib/utils/i2c/fdt_i2c.c b/lib/utils/i2c/fdt_i2c.c
> new file mode 100644
> index 0000000..27e28a4
> --- /dev/null
> +++ b/lib/utils/i2c/fdt_i2c.c
> @@ -0,0 +1,107 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright (c) 2021 YADRO
> + *
> + * Authors:
> + *   Nikita Shubin <nshubin@yadro.com>
> + *
> + * derivate: lib/utils/gpio/fdt_gpio.c
> + * Authors:
> + *   Anup Patel <anup.patel@wdc.com>
> + */
> +
> +#include <libfdt.h>
> +#include <sbi/sbi_error.h>
> +#include <sbi_utils/fdt/fdt_helper.h>
> +#include <sbi_utils/i2c/fdt_i2c.h>
> +
> +#include <sbi/sbi_console.h>
> +
> +static struct fdt_i2c_adapter *i2c_adapter_drivers[] = {
> +};
> +
> +static struct fdt_i2c_adapter *fdt_i2c_adapter_driver(struct i2c_adapter *adapter)
> +{
> +	int pos;
> +
> +	if (!adapter)
> +		return NULL;
> +
> +	for (pos = 0; pos < array_size(i2c_adapter_drivers); pos++) {
> +		if (adapter->driver == i2c_adapter_drivers[pos])
> +			return i2c_adapter_drivers[pos];
> +	}
> +
> +	return NULL;
> +}


Ok I may be missing something here, but isn't it the same as a simple
return adapter->driver?


> +
> +static int fdt_i2c_adapter_init(void *fdt, int nodeoff)
> +{
> +	int pos, rc;
> +	struct fdt_i2c_adapter *drv;
> +	const struct fdt_match *match;
> +
> +	/* Try all I2C drivers one-by-one */
> +	for (pos = 0; pos < array_size(i2c_adapter_drivers); pos++) {
> +		drv = i2c_adapter_drivers[pos];
> +		match = fdt_match_node(fdt, nodeoff, drv->match_table);
> +		if (match && drv->init) {
> +			rc = drv->init(fdt, nodeoff, match);
> +			if (rc == SBI_ENODEV)
> +				continue;
> +			if (rc)
> +				return rc;
> +			return 0;
> +		}
> +	}
> +
> +	return SBI_ENOSYS;
> +}
> +
> +static int fdt_i2c_adapter_find(void *fdt, int nodeoff,
> +				struct i2c_adapter **out_adapter)
> +{
> +	int rc;
> +	struct i2c_adapter *adapter = i2c_adapter_find(nodeoff);
> +
> +	if (!adapter) {
> +		/* I2C adapter not found so initialize matching driver */
> +		rc = fdt_i2c_adapter_init(fdt, nodeoff);
> +		if (rc)
> +			return rc;
> +
> +		/* Try to find I2C adapter again */
> +		adapter = i2c_adapter_find(nodeoff);
> +		if (!adapter)
> +			return SBI_ENOSYS;
> +	}
> +
> +	if (out_adapter)
> +		*out_adapter = adapter;
> +
> +	return 0;
> +}
> +
> +int fdt_i2c_adapter_get(void *fdt, int nodeoff,
> +			struct i2c_adapter **out_adapter)
> +{
> +	int rc;
> +	struct i2c_adapter *adapter;
> +	struct fdt_i2c_adapter *drv;
> +
> +	if (!fdt || (nodeoff < 0) || !out_adapter)
> +		return SBI_EINVAL;
> +
> +	rc = fdt_i2c_adapter_find(fdt, nodeoff, &adapter);
> +	if (rc)
> +		return rc;
> +
> +	drv = fdt_i2c_adapter_driver(adapter);
> +	if (!drv)
> +		return SBI_ENOSYS;
> +
> +	*out_adapter = adapter;
> +
> +	return 0;
> +}
> diff --git a/lib/utils/i2c/objects.mk b/lib/utils/i2c/objects.mk
> index 16a70da..06baa65 100644
> --- a/lib/utils/i2c/objects.mk
> +++ b/lib/utils/i2c/objects.mk
> @@ -8,3 +8,4 @@
>  #
>  
>  libsbiutils-objs-y += i2c/i2c.o
> +libsbiutils-objs-y += i2c/fdt_i2c.o

[PATCH 3/4] lib: utils/gpio: Add minimal SiFive I2C driver

[Nikita Shubin]

From: Nikita Shubin <n.shubin@yadro.com>

Minimum SiFive I2C driver to read/send bytes over I2C bus.

This allows querying information and perform operation of onboard PMIC,
as well as power-off and reset.

Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
---
 lib/utils/i2c/fdt_i2c.c        |   3 +
 lib/utils/i2c/fdt_i2c_sifive.c | 289 +++++++++++++++++++++++++++++++++
 lib/utils/i2c/objects.mk       |   1 +
 3 files changed, 293 insertions(+)
 create mode 100644 lib/utils/i2c/fdt_i2c_sifive.c

diff --git a/lib/utils/i2c/fdt_i2c.c b/lib/utils/i2c/fdt_i2c.c
index 27e28a4..46434da 100644
--- a/lib/utils/i2c/fdt_i2c.c
+++ b/lib/utils/i2c/fdt_i2c.c
@@ -18,7 +18,10 @@
 
 #include <sbi/sbi_console.h>
 
+extern struct fdt_i2c_adapter fdt_i2c_adapter_sifive;
+
 static struct fdt_i2c_adapter *i2c_adapter_drivers[] = {
+	&fdt_i2c_adapter_sifive
 };
 
 static struct fdt_i2c_adapter *fdt_i2c_adapter_driver(struct i2c_adapter *adapter)
diff --git a/lib/utils/i2c/fdt_i2c_sifive.c b/lib/utils/i2c/fdt_i2c_sifive.c
new file mode 100644
index 0000000..ff294f4
--- /dev/null
+++ b/lib/utils/i2c/fdt_i2c_sifive.c
@@ -0,0 +1,289 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright (c) 2021 YADRO
+ *
+ * Authors:
+ *   Nikita Shubin <nshubin@yadro.com>
+ */
+
+#include <sbi/riscv_io.h>
+#include <sbi/sbi_error.h>
+#include <sbi_utils/fdt/fdt_helper.h>
+#include <sbi_utils/i2c/fdt_i2c.h>
+
+#include <sbi/riscv_asm.h>
+#include <sbi/riscv_encoding.h>
+
+#define SIFIVE_I2C_ADAPTER_MAX	2
+
+#define SIFIVE_I2C_PRELO	0x00
+#define SIFIVE_I2C_PREHI	0x04
+#define SIFIVE_I2C_CTR		0x08
+#define SIFIVE_I2C_TXR		0x00c
+#define SIFIVE_I2C_RXR		SIFIVE_I2C_TXR
+#define SIFIVE_I2C_CR		0x010
+#define SIFIVE_I2C_SR		SIFIVE_I2C_CR
+
+#define SIFIVE_I2C_CTR_IEN	(1 << 6)
+#define SIFIVE_I2C_CTR_EN	(1 << 7)
+
+#define SIFIVE_I2C_CMD_IACK	(1 << 0)
+#define SIFIVE_I2C_CMD_ACK	(1 << 3)
+#define SIFIVE_I2C_CMD_WR	(1 << 4)
+#define SIFIVE_I2C_CMD_RD	(1 << 5)
+#define SIFIVE_I2C_CMD_STO	(1 << 6)
+#define SIFIVE_I2C_CMD_STA	(1 << 7)
+
+#define SIFIVE_I2C_STATUS_IF	(1 << 0)
+#define SIFIVE_I2C_STATUS_TIP	(1 << 1)
+#define SIFIVE_I2C_STATUS_AL	(1 << 5)
+#define SIFIVE_I2C_STATUS_BUSY	(1 << 6)
+#define SIFIVE_I2C_STATUS_RXACK	(1 << 7)
+
+#define SIFIVE_I2C_WRITE_BIT	(0 << 0)
+#define SIFIVE_I2C_READ_BIT	(1 << 0)
+
+struct sifive_i2c_adapter {
+	unsigned long addr;
+	struct i2c_adapter adapter;
+};
+
+static unsigned int sifive_i2c_adapter_count;
+static struct sifive_i2c_adapter sifive_i2c_adapter_array[SIFIVE_I2C_ADAPTER_MAX];
+
+extern struct fdt_i2c_adapter fdt_i2c_adapter_sifive;
+
+static inline void setreg(struct sifive_i2c_adapter *adap, int reg, u8 value)
+{
+	writel(value, (volatile void *)adap->addr + reg);
+}
+
+static inline u8 getreg(struct sifive_i2c_adapter *adap, int reg)
+{
+	return readl((volatile void *)adap->addr + reg);
+}
+
+static int sifive_i2c_adapter_rxack(struct sifive_i2c_adapter *adap)
+{
+	uint8_t val = getreg(adap, SIFIVE_I2C_SR);
+
+	if (val & SIFIVE_I2C_STATUS_RXACK)
+		return SBI_EIO;
+
+	return 0;
+}
+
+static inline void wait_cycles(unsigned long cycles)
+{
+	csr_write(CSR_MCYCLE, 0);
+	while
+		(cycles > csr_read_num(CSR_MCYCLE));
+}
+
+static int sifive_i2c_adapter_poll(struct sifive_i2c_adapter *adap, uint32_t mask)
+{
+	int max_retry = 5;
+	uint8_t val;
+
+	do {
+		val = getreg(adap, SIFIVE_I2C_SR);
+		wait_cycles(100000);
+	} while ((val & mask) && (max_retry--) > 0);
+
+	if (max_retry <= 0)
+		return SBI_ETIMEDOUT;
+
+	return 0;
+}
+
+#define sifive_i2c_adapter_poll_tip(adap) sifive_i2c_adapter_poll(adap, SIFIVE_I2C_STATUS_TIP)
+#define sifive_i2c_adapter_poll_busy(adap) sifive_i2c_adapter_poll(adap, SIFIVE_I2C_STATUS_BUSY)
+
+static int sifive_i2c_adapter_start(struct sifive_i2c_adapter *adap, uint8_t addr, uint8_t bit)
+{
+	uint8_t val = (addr << 1) | bit;
+
+	setreg(adap, SIFIVE_I2C_TXR, val);
+	val = SIFIVE_I2C_CMD_STA | SIFIVE_I2C_CMD_WR | SIFIVE_I2C_CMD_IACK;
+	setreg(adap, SIFIVE_I2C_CR, val);
+
+	return sifive_i2c_adapter_poll_tip(adap);
+}
+
+static int sifive_i2c_adapter_sendb(struct sifive_i2c_adapter *adap, uint8_t addr, uint8_t reg, uint8_t value)
+{
+	int rc = sifive_i2c_adapter_start(adap, addr, SIFIVE_I2C_WRITE_BIT);
+
+	if (rc)
+		return rc;
+
+	rc = sifive_i2c_adapter_rxack(adap);
+	if (rc)
+		return rc;
+
+	/* set register address */
+	setreg(adap, SIFIVE_I2C_TXR, reg);
+	setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_WR | SIFIVE_I2C_CMD_IACK);
+	rc = sifive_i2c_adapter_poll_tip(adap);
+	if (rc)
+		return rc;
+
+	rc = sifive_i2c_adapter_rxack(adap);
+	if (rc)
+		return rc;
+
+	/* set value */
+	setreg(adap, SIFIVE_I2C_TXR, value);
+	setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_WR | SIFIVE_I2C_CMD_IACK);
+
+	rc = sifive_i2c_adapter_poll_tip(adap);
+	if (rc)
+		return rc;
+
+	rc = sifive_i2c_adapter_rxack(adap);
+	if (rc)
+		return rc;
+
+	setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_STO | SIFIVE_I2C_CMD_IACK);
+
+	/* poll BUSY instead of ACK*/
+	rc = sifive_i2c_adapter_poll_busy(adap);
+	if (rc)
+		return rc;
+
+	setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_IACK);
+
+	return 0;
+}
+
+static int sifive_i2c_adapter_readb(struct sifive_i2c_adapter *adap, uint8_t addr, uint8_t reg, uint8_t *value)
+{
+	int rc;
+	uint8_t val;
+
+	rc = sifive_i2c_adapter_start(adap, addr, SIFIVE_I2C_WRITE_BIT);
+	if (rc)
+		return rc;
+
+	rc = sifive_i2c_adapter_rxack(adap);
+	if (rc)
+		return rc;
+
+	setreg(adap, SIFIVE_I2C_TXR, reg);
+	setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_WR | SIFIVE_I2C_CMD_IACK);
+
+	rc = sifive_i2c_adapter_poll_tip(adap);
+	if (rc)
+		return rc;
+
+	rc = sifive_i2c_adapter_rxack(adap);
+	if (rc)
+		return rc;
+
+	/* setting addr with high 0 bit */
+	val = (addr << 1) | SIFIVE_I2C_READ_BIT;
+	setreg(adap, SIFIVE_I2C_TXR, val);
+	setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_STA | SIFIVE_I2C_CMD_WR | SIFIVE_I2C_CMD_IACK);
+
+	rc = sifive_i2c_adapter_poll_tip(adap);
+	if (rc)
+		return rc;
+
+	rc = sifive_i2c_adapter_rxack(adap);
+	if (rc)
+		return rc;
+
+	setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_ACK | SIFIVE_I2C_CMD_RD | SIFIVE_I2C_CMD_IACK);
+
+	rc = sifive_i2c_adapter_poll_tip(adap);
+	if (rc)
+		return rc;
+
+	*value = getreg(adap, SIFIVE_I2C_RXR);
+
+	setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_STO | SIFIVE_I2C_CMD_IACK);
+	rc = sifive_i2c_adapter_poll_busy(adap);
+	if (rc)
+		return rc;
+
+	setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_IACK);
+
+	return 0;
+}
+
+static int sifive_i2c_adapter_send(struct i2c_adapter *ia, uint8_t addr, uint8_t reg, uint8_t value)
+{
+	struct sifive_i2c_adapter *adapter =
+		container_of(ia, struct sifive_i2c_adapter, adapter);
+
+	return sifive_i2c_adapter_sendb(adapter, addr, reg, value);
+}
+
+static int sifive_i2c_adapter_read(struct i2c_adapter *ia, uint8_t addr, uint8_t reg, uint8_t *value)
+{
+	int rc;
+	uint8_t val;
+	struct sifive_i2c_adapter *adapter =
+		container_of(ia, struct sifive_i2c_adapter, adapter);
+
+	rc = sifive_i2c_adapter_readb(adapter, addr, reg, &val);
+	if (rc)
+		return rc;
+
+	if (value)
+		*value = val;
+
+	return 0;
+}
+
+static int sifive_i2c_adapter_configure(struct i2c_adapter *ia)
+{
+	struct sifive_i2c_adapter *adap =
+		container_of(ia, struct sifive_i2c_adapter, adapter);
+
+	/* enable controller/disable interrupts */
+	setreg(adap, SIFIVE_I2C_CTR, SIFIVE_I2C_CTR_EN);
+
+	return 0;
+}
+
+static int sifive_i2c_init(void *fdt, int nodeoff,
+			    const struct fdt_match *match)
+{
+	int rc;
+	struct sifive_i2c_adapter *adapter;
+	uint64_t addr;
+
+	if (sifive_i2c_adapter_count >= SIFIVE_I2C_ADAPTER_MAX)
+		return SBI_ENOSPC;
+
+	adapter = &sifive_i2c_adapter_array[sifive_i2c_adapter_count];
+
+	rc = fdt_get_node_addr_size(fdt, nodeoff, 0, &addr, NULL);
+	if (rc)
+		return rc;
+
+	adapter->addr = addr;
+	adapter->adapter.driver = &fdt_i2c_adapter_sifive;
+	adapter->adapter.id = nodeoff;
+	adapter->adapter.send = sifive_i2c_adapter_send;
+	adapter->adapter.read = sifive_i2c_adapter_read;
+	adapter->adapter.configure = sifive_i2c_adapter_configure;
+	rc = i2c_adapter_add(&adapter->adapter);
+	if (rc)
+		return rc;
+
+	sifive_i2c_adapter_count++;
+	return 0;
+}
+
+static const struct fdt_match sifive_i2c_match[] = {
+	{ .compatible = "sifive,i2c0" },
+	{ },
+};
+
+struct fdt_i2c_adapter fdt_i2c_adapter_sifive = {
+	.match_table = sifive_i2c_match,
+	.init = sifive_i2c_init,
+};
diff --git a/lib/utils/i2c/objects.mk b/lib/utils/i2c/objects.mk
index 06baa65..d52ab18 100644
--- a/lib/utils/i2c/objects.mk
+++ b/lib/utils/i2c/objects.mk
@@ -9,3 +9,4 @@
 
 libsbiutils-objs-y += i2c/i2c.o
 libsbiutils-objs-y += i2c/fdt_i2c.o
+libsbiutils-objs-y += i2c/fdt_i2c_sifive.o
-- 
2.31.1

[PATCH 3/4] lib: utils/gpio: Add minimal SiFive I2C driver

[Alexandre ghiti]

Patch title uses 'gpio' instead of 'i2c'.

On 9/24/21 1:33 PM, Nikita Shubin wrote:
> From: Nikita Shubin <n.shubin@yadro.com>
>
> Minimum SiFive I2C driver to read/send bytes over I2C bus.
>
> This allows querying information and perform operation of onboard PMIC,
> as well as power-off and reset.
>
> Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
> ---
>  lib/utils/i2c/fdt_i2c.c        |   3 +
>  lib/utils/i2c/fdt_i2c_sifive.c | 289 +++++++++++++++++++++++++++++++++
>  lib/utils/i2c/objects.mk       |   1 +
>  3 files changed, 293 insertions(+)
>  create mode 100644 lib/utils/i2c/fdt_i2c_sifive.c
>
> diff --git a/lib/utils/i2c/fdt_i2c.c b/lib/utils/i2c/fdt_i2c.c
> index 27e28a4..46434da 100644
> --- a/lib/utils/i2c/fdt_i2c.c
> +++ b/lib/utils/i2c/fdt_i2c.c
> @@ -18,7 +18,10 @@
>  
>  #include <sbi/sbi_console.h>
>  
> +extern struct fdt_i2c_adapter fdt_i2c_adapter_sifive;
> +
>  static struct fdt_i2c_adapter *i2c_adapter_drivers[] = {
> +	&fdt_i2c_adapter_sifive
>  };
>  
>  static struct fdt_i2c_adapter *fdt_i2c_adapter_driver(struct i2c_adapter *adapter)
> diff --git a/lib/utils/i2c/fdt_i2c_sifive.c b/lib/utils/i2c/fdt_i2c_sifive.c
> new file mode 100644
> index 0000000..ff294f4
> --- /dev/null
> +++ b/lib/utils/i2c/fdt_i2c_sifive.c
> @@ -0,0 +1,289 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright (c) 2021 YADRO
> + *
> + * Authors:
> + *   Nikita Shubin <nshubin@yadro.com>
> + */
> +
> +#include <sbi/riscv_io.h>
> +#include <sbi/sbi_error.h>
> +#include <sbi_utils/fdt/fdt_helper.h>
> +#include <sbi_utils/i2c/fdt_i2c.h>
> +
> +#include <sbi/riscv_asm.h>
> +#include <sbi/riscv_encoding.h>
> +
> +#define SIFIVE_I2C_ADAPTER_MAX	2
> +
> +#define SIFIVE_I2C_PRELO	0x00
> +#define SIFIVE_I2C_PREHI	0x04
> +#define SIFIVE_I2C_CTR		0x08
> +#define SIFIVE_I2C_TXR		0x00c
> +#define SIFIVE_I2C_RXR		SIFIVE_I2C_TXR
> +#define SIFIVE_I2C_CR		0x010
> +#define SIFIVE_I2C_SR		SIFIVE_I2C_CR
> +
> +#define SIFIVE_I2C_CTR_IEN	(1 << 6)
> +#define SIFIVE_I2C_CTR_EN	(1 << 7)
> +
> +#define SIFIVE_I2C_CMD_IACK	(1 << 0)
> +#define SIFIVE_I2C_CMD_ACK	(1 << 3)
> +#define SIFIVE_I2C_CMD_WR	(1 << 4)
> +#define SIFIVE_I2C_CMD_RD	(1 << 5)
> +#define SIFIVE_I2C_CMD_STO	(1 << 6)
> +#define SIFIVE_I2C_CMD_STA	(1 << 7)
> +
> +#define SIFIVE_I2C_STATUS_IF	(1 << 0)
> +#define SIFIVE_I2C_STATUS_TIP	(1 << 1)
> +#define SIFIVE_I2C_STATUS_AL	(1 << 5)
> +#define SIFIVE_I2C_STATUS_BUSY	(1 << 6)
> +#define SIFIVE_I2C_STATUS_RXACK	(1 << 7)
> +
> +#define SIFIVE_I2C_WRITE_BIT	(0 << 0)
> +#define SIFIVE_I2C_READ_BIT	(1 << 0)
> +
> +struct sifive_i2c_adapter {
> +	unsigned long addr;
> +	struct i2c_adapter adapter;
> +};
> +
> +static unsigned int sifive_i2c_adapter_count;
> +static struct sifive_i2c_adapter sifive_i2c_adapter_array[SIFIVE_I2C_ADAPTER_MAX];
> +
> +extern struct fdt_i2c_adapter fdt_i2c_adapter_sifive;
> +
> +static inline void setreg(struct sifive_i2c_adapter *adap, int reg, u8 value)
> +{
> +	writel(value, (volatile void *)adap->addr + reg);
> +}
> +
> +static inline u8 getreg(struct sifive_i2c_adapter *adap, int reg)
> +{
> +	return readl((volatile void *)adap->addr + reg);
> +}


Both functions should be prefixed by "sifive_i2c_", like all other
functions.


> +
> +static int sifive_i2c_adapter_rxack(struct sifive_i2c_adapter *adap)
> +{
> +	uint8_t val = getreg(adap, SIFIVE_I2C_SR);
> +
> +	if (val & SIFIVE_I2C_STATUS_RXACK)
> +		return SBI_EIO;
> +
> +	return 0;
> +}
> +
> +static inline void wait_cycles(unsigned long cycles)
> +{
> +	csr_write(CSR_MCYCLE, 0);
> +	while
> +		(cycles > csr_read_num(CSR_MCYCLE));
> +}


A patch that implements this recently landed in master (or is about to).


> +
> +static int sifive_i2c_adapter_poll(struct sifive_i2c_adapter *adap, uint32_t mask)
> +{
> +	int max_retry = 5;
> +	uint8_t val;
> +
> +	do {
> +		val = getreg(adap, SIFIVE_I2C_SR);
> +		wait_cycles(100000);
> +	} while ((val & mask) && (max_retry--) > 0);
> +
> +	if (max_retry <= 0)
> +		return SBI_ETIMEDOUT;
> +
> +	return 0;
> +}
> +
> +#define sifive_i2c_adapter_poll_tip(adap) sifive_i2c_adapter_poll(adap, SIFIVE_I2C_STATUS_TIP)
> +#define sifive_i2c_adapter_poll_busy(adap) sifive_i2c_adapter_poll(adap, SIFIVE_I2C_STATUS_BUSY)
> +
> +static int sifive_i2c_adapter_start(struct sifive_i2c_adapter *adap, uint8_t addr, uint8_t bit)
> +{
> +	uint8_t val = (addr << 1) | bit;
> +
> +	setreg(adap, SIFIVE_I2C_TXR, val);
> +	val = SIFIVE_I2C_CMD_STA | SIFIVE_I2C_CMD_WR | SIFIVE_I2C_CMD_IACK;
> +	setreg(adap, SIFIVE_I2C_CR, val);
> +
> +	return sifive_i2c_adapter_poll_tip(adap);
> +}
> +
> +static int sifive_i2c_adapter_sendb(struct sifive_i2c_adapter *adap, uint8_t addr, uint8_t reg, uint8_t value)
> +{
> +	int rc = sifive_i2c_adapter_start(adap, addr, SIFIVE_I2C_WRITE_BIT);
> +
> +	if (rc)
> +		return rc;
> +
> +	rc = sifive_i2c_adapter_rxack(adap);
> +	if (rc)
> +		return rc;
> +
> +	/* set register address */
> +	setreg(adap, SIFIVE_I2C_TXR, reg);
> +	setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_WR | SIFIVE_I2C_CMD_IACK);
> +	rc = sifive_i2c_adapter_poll_tip(adap);
> +	if (rc)
> +		return rc;
> +
> +	rc = sifive_i2c_adapter_rxack(adap);
> +	if (rc)
> +		return rc;
> +
> +	/* set value */
> +	setreg(adap, SIFIVE_I2C_TXR, value);
> +	setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_WR | SIFIVE_I2C_CMD_IACK);
> +
> +	rc = sifive_i2c_adapter_poll_tip(adap);
> +	if (rc)
> +		return rc;
> +
> +	rc = sifive_i2c_adapter_rxack(adap);
> +	if (rc)
> +		return rc;
> +
> +	setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_STO | SIFIVE_I2C_CMD_IACK);
> +
> +	/* poll BUSY instead of ACK*/
> +	rc = sifive_i2c_adapter_poll_busy(adap);
> +	if (rc)
> +		return rc;
> +
> +	setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_IACK);
> +
> +	return 0;
> +}
> +
> +static int sifive_i2c_adapter_readb(struct sifive_i2c_adapter *adap, uint8_t addr, uint8_t reg, uint8_t *value)
> +{
> +	int rc;
> +	uint8_t val;
> +
> +	rc = sifive_i2c_adapter_start(adap, addr, SIFIVE_I2C_WRITE_BIT);
> +	if (rc)
> +		return rc;
> +
> +	rc = sifive_i2c_adapter_rxack(adap);
> +	if (rc)
> +		return rc;
> +
> +	setreg(adap, SIFIVE_I2C_TXR, reg);
> +	setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_WR | SIFIVE_I2C_CMD_IACK);
> +
> +	rc = sifive_i2c_adapter_poll_tip(adap);
> +	if (rc)
> +		return rc;
> +
> +	rc = sifive_i2c_adapter_rxack(adap);
> +	if (rc)
> +		return rc;
> +
> +	/* setting addr with high 0 bit */
> +	val = (addr << 1) | SIFIVE_I2C_READ_BIT;
> +	setreg(adap, SIFIVE_I2C_TXR, val);
> +	setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_STA | SIFIVE_I2C_CMD_WR | SIFIVE_I2C_CMD_IACK);
> +
> +	rc = sifive_i2c_adapter_poll_tip(adap);
> +	if (rc)
> +		return rc;
> +
> +	rc = sifive_i2c_adapter_rxack(adap);
> +	if (rc)
> +		return rc;
> +
> +	setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_ACK | SIFIVE_I2C_CMD_RD | SIFIVE_I2C_CMD_IACK);
> +
> +	rc = sifive_i2c_adapter_poll_tip(adap);
> +	if (rc)
> +		return rc;
> +
> +	*value = getreg(adap, SIFIVE_I2C_RXR);
> +
> +	setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_STO | SIFIVE_I2C_CMD_IACK);
> +	rc = sifive_i2c_adapter_poll_busy(adap);
> +	if (rc)
> +		return rc;
> +
> +	setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_IACK);
> +
> +	return 0;
> +}
> +
> +static int sifive_i2c_adapter_send(struct i2c_adapter *ia, uint8_t addr, uint8_t reg, uint8_t value)


This function is limited to sending 1B immediate value (as pointed in a
previous patch review): let's call it sifive_i2c_adapter_send_imm8 or
something similar and make it a helper of the more general function.


> +{
> +	struct sifive_i2c_adapter *adapter =
> +		container_of(ia, struct sifive_i2c_adapter, adapter);
> +
> +	return sifive_i2c_adapter_sendb(adapter, addr, reg, value);
> +}
> +
> +static int sifive_i2c_adapter_read(struct i2c_adapter *ia, uint8_t addr, uint8_t reg, uint8_t *value)

Same here.


> +{
> +	int rc;
> +	uint8_t val;
> +	struct sifive_i2c_adapter *adapter =
> +		container_of(ia, struct sifive_i2c_adapter, adapter);
> +
> +	rc = sifive_i2c_adapter_readb(adapter, addr, reg, &val);
> +	if (rc)
> +		return rc;
> +
> +	if (value)
> +		*value = val;
> +
> +	return 0;
> +}
> +
> +static int sifive_i2c_adapter_configure(struct i2c_adapter *ia)
> +{
> +	struct sifive_i2c_adapter *adap =
> +		container_of(ia, struct sifive_i2c_adapter, adapter);
> +
> +	/* enable controller/disable interrupts */
> +	setreg(adap, SIFIVE_I2C_CTR, SIFIVE_I2C_CTR_EN);
> +
> +	return 0;
> +}
> +
> +static int sifive_i2c_init(void *fdt, int nodeoff,
> +			    const struct fdt_match *match)
> +{
> +	int rc;
> +	struct sifive_i2c_adapter *adapter;
> +	uint64_t addr;
> +
> +	if (sifive_i2c_adapter_count >= SIFIVE_I2C_ADAPTER_MAX)
> +		return SBI_ENOSPC;


It's a bit weird to limit here the number of sifive i2c adapters to
SIFIVE_I2C_ADAPTER_MAX whereas we already have another limit in i2c.c to
I2C_ADAPTER_MAX, that limits the number of sifive adapters to 2 for no
technical reason right?

One thing that could prevent this could be to totally get rid of
sifive_i2c_adapter structure and just put the "addr" field into
i2c_adapter structure "void *data" field (which does not exist yet: this
new field could be used for whatever purpose that serves the i2c adapter
driver). And then adding adapter would be a simple call to
i2c_adapter_add(&fdt_i2c_adapter_sifive, nodeoff,
sifive_i2c_adapter_send, sifive_i2c_adapter_read, (void *)addr).


> +
> +	adapter = &sifive_i2c_adapter_array[sifive_i2c_adapter_count];
> +
> +	rc = fdt_get_node_addr_size(fdt, nodeoff, 0, &addr, NULL);
> +	if (rc)
> +		return rc;
> +
> +	adapter->addr = addr;
> +	adapter->adapter.driver = &fdt_i2c_adapter_sifive;
> +	adapter->adapter.id = nodeoff;
> +	adapter->adapter.send = sifive_i2c_adapter_send;
> +	adapter->adapter.read = sifive_i2c_adapter_read;
> +	adapter->adapter.configure = sifive_i2c_adapter_configure;
> +	rc = i2c_adapter_add(&adapter->adapter);
> +	if (rc)
> +		return rc;
> +
> +	sifive_i2c_adapter_count++;
> +	return 0;
> +}
> +
> +static const struct fdt_match sifive_i2c_match[] = {
> +	{ .compatible = "sifive,i2c0" },
> +	{ },
> +};
> +
> +struct fdt_i2c_adapter fdt_i2c_adapter_sifive = {
> +	.match_table = sifive_i2c_match,
> +	.init = sifive_i2c_init,
> +};
> diff --git a/lib/utils/i2c/objects.mk b/lib/utils/i2c/objects.mk
> index 06baa65..d52ab18 100644
> --- a/lib/utils/i2c/objects.mk
> +++ b/lib/utils/i2c/objects.mk
> @@ -9,3 +9,4 @@
>  
>  libsbiutils-objs-y += i2c/i2c.o
>  libsbiutils-objs-y += i2c/fdt_i2c.o
> +libsbiutils-objs-y += i2c/fdt_i2c_sifive.o

[PATCH 4/4] lib: utils/reset: Add generic da9063 reset driver

[Nikita Shubin]

From: Nikita Shubin <n.shubin@yadro.com>

da9063 PMIC can be used to reset/shutdown the
Sifive Unmatched board.

shutdown is done simply by writing SHUDOWN bit to
DA9063_REG_CONTROL_F register.

reset via setting WAKEUP bit in DA9063_REG_CONTROL_F
register followed by masking POWER and POWER1 domains
and setting STANDBY bit in DA9063_REG_CONTROL_A,
originally discovered by Alexandre Ghiti on linux-riscv
maillists.

Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
---
 lib/utils/reset/fdt_reset.c        |   2 +
 lib/utils/reset/fdt_reset_da9063.c | 203 +++++++++++++++++++++++++++++
 lib/utils/reset/objects.mk         |   1 +
 3 files changed, 206 insertions(+)
 create mode 100644 lib/utils/reset/fdt_reset_da9063.c

diff --git a/lib/utils/reset/fdt_reset.c b/lib/utils/reset/fdt_reset.c
index 168bb0c..92f37b0 100644
--- a/lib/utils/reset/fdt_reset.c
+++ b/lib/utils/reset/fdt_reset.c
@@ -18,6 +18,7 @@ extern struct fdt_reset fdt_reset_htif;
 extern struct fdt_reset fdt_reset_sifive_test;
 extern struct fdt_reset fdt_reset_sunxi_wdt;
 extern struct fdt_reset fdt_reset_thead;
+extern struct fdt_reset fdt_reset_da9063;
 
 static struct fdt_reset *reset_drivers[] = {
 	&fdt_poweroff_gpio,
@@ -26,6 +27,7 @@ static struct fdt_reset *reset_drivers[] = {
 	&fdt_reset_sifive_test,
 	&fdt_reset_sunxi_wdt,
 	&fdt_reset_thead,
+	&fdt_reset_da9063,
 };
 
 int fdt_reset_init(void)
diff --git a/lib/utils/reset/fdt_reset_da9063.c b/lib/utils/reset/fdt_reset_da9063.c
new file mode 100644
index 0000000..6c96968
--- /dev/null
+++ b/lib/utils/reset/fdt_reset_da9063.c
@@ -0,0 +1,203 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright (c) 2021 YADRO
+ *
+ * Authors:
+ *   Nikita Shubin <nshubin@yadro.com>
+ */
+
+#include <libfdt.h>
+#include <sbi/sbi_error.h>
+#include <sbi/sbi_console.h>
+#include <sbi/sbi_ecall_interface.h>
+#include <sbi/sbi_hart.h>
+#include <sbi/sbi_system.h>
+#include <sbi_utils/fdt/fdt_helper.h>
+#include <sbi_utils/i2c/fdt_i2c.h>
+#include <sbi_utils/reset/fdt_reset.h>
+
+#define DA9063_REG_PAGE_CON		0x00
+#define DA9063_REG_CONTROL_A		0x0e
+#define DA9063_REG_CONTROL_C		0x10
+#define DA9063_REG_CONTROL_F		0x13
+#define DA9063_REG_DEVICE_ID		0x81
+
+#define DA9063_CONTROL_A_CP_EN		(1 << 7)
+#define DA9063_CONTROL_A_M_POWER1_EN	(1 << 6)
+#define DA9063_CONTROL_A_M_POWER_EN	(1 << 5)
+#define DA9063_CONTROL_A_M_SYSTEM_EN	(1 << 4)
+#define DA9063_CONTROL_A_STANDBY	(1 << 3)
+#define DA9063_CONTROL_A_POWER1_EN	(1 << 2)
+#define DA9063_CONTROL_A_POWER_EN	(1 << 1)
+#define DA9063_CONTROL_A_SYSTEM_EN	(1 << 0)
+
+#define DA9063_CONTROL_F_WAKEUP		(1 << 2)
+#define DA9063_CONTROL_F_SHUTDOWN	(1 << 1)
+#define DA9063_CONTROL_F_WATCHDOG	(1 << 0)
+
+#define DA9063_CONTROL_C_DEF_SUPPLY	(1 << 7)
+#define DA9063_CONTROL_C_SLEW_RATE	(1 << 4)
+#define DA9063_CONTROL_C_OTPREAD_EN	(1 << 3)
+#define DA9063_CONTROL_C_AUTO_BOOT	(1 << 2)
+#define DA9063_CONTROL_C_DEBOUNCING	(1 << 0)
+
+#define PMIC_CHIP_ID_DA9063		0x61
+
+static struct {
+	struct i2c_adapter *adapter;
+	uint32_t reg;
+	int last_error;
+} da9063;
+
+
+
+static int da9063_system_reset_check(u32 type, u32 reason)
+{
+	switch (type) {
+	case SBI_SRST_RESET_TYPE_SHUTDOWN:
+	case SBI_SRST_RESET_TYPE_COLD_REBOOT:
+	case SBI_SRST_RESET_TYPE_WARM_REBOOT:
+		return 1;
+	}
+
+	return 0;
+}
+
+static inline int da9063_sanity_check(struct i2c_adapter *adap, uint32_t reg)
+{
+	uint8_t val;
+	int rc = i2c_adapter_send(adap, reg, DA9063_REG_PAGE_CON, 0x02);
+
+	if (rc)
+		return rc;
+
+	/* check set page*/
+	rc = i2c_adapter_read(adap, reg, 0x0, &val);
+	if (rc)
+		return rc;
+
+	if (val != 0x02)
+		return SBI_ENODEV;
+
+	/* read and check device id */
+	rc = i2c_adapter_read(adap, reg, DA9063_REG_DEVICE_ID, &val);
+	if (rc)
+		return rc;
+
+	if (val != PMIC_CHIP_ID_DA9063)
+		return SBI_ENODEV;
+
+	return 0;
+}
+
+static inline int da9063_shutdown(struct i2c_adapter *adap, uint32_t reg)
+{
+	int rc = i2c_adapter_send(adap, da9063.reg, DA9063_REG_PAGE_CON, 0x00);
+
+	if (rc)
+		return rc;
+
+	return i2c_adapter_send(adap, da9063.reg,
+				DA9063_REG_CONTROL_F, DA9063_CONTROL_F_SHUTDOWN);
+}
+
+static inline int da9063_reset(struct i2c_adapter *adap, uint32_t reg)
+{
+	int rc = i2c_adapter_send(adap, da9063.reg, DA9063_REG_PAGE_CON, 0x00);
+
+	if (rc)
+		return rc;
+
+	rc = i2c_adapter_send(adap, da9063.reg,
+			      DA9063_REG_CONTROL_F, DA9063_CONTROL_F_WAKEUP);
+	if (rc)
+		return rc;
+
+	return i2c_adapter_send(adap, da9063.reg,
+				DA9063_REG_CONTROL_A,
+				DA9063_CONTROL_A_M_POWER1_EN |
+				DA9063_CONTROL_A_M_POWER_EN |
+				DA9063_CONTROL_A_STANDBY);
+}
+
+static void da9063_system_reset(u32 type, u32 reason)
+{
+	struct i2c_adapter *adap = da9063.adapter;
+	uint32_t reg = da9063.reg;
+	int rc;
+
+	if (adap) {
+		/* may include clock init */
+		i2c_adapter_configure(adap);
+
+		/* sanity check */
+		rc = da9063_sanity_check(adap, reg);
+		if (rc) {
+			sbi_printf("%s: chip is not da9063 PMIC\n", __func__);
+			goto skip_reset;
+		}
+
+		switch (type) {
+		case SBI_SRST_RESET_TYPE_SHUTDOWN:
+			da9063_shutdown(adap, reg);
+			break;
+		case SBI_SRST_RESET_TYPE_COLD_REBOOT:
+		case SBI_SRST_RESET_TYPE_WARM_REBOOT:
+			da9063_reset(adap, reg);
+			break;
+		}
+
+		while
+			(1);
+
+skip_reset:
+		sbi_hart_hang();
+	}
+}
+
+static struct sbi_system_reset_device da9063_reset_i2c = {
+	.name = "da9063-reset",
+	.system_reset_check = da9063_system_reset_check,
+	.system_reset = da9063_system_reset
+};
+
+static int da9063_reset_init(void *fdt, int nodeoff,
+			   const struct fdt_match *match)
+{
+	int rc, i2c_bus;
+	struct i2c_adapter *adapter;
+	uint64_t addr;
+
+	/* find i2c bus parent node */
+	i2c_bus = fdt_parent_offset(fdt, nodeoff);
+	if (i2c_bus < 0)
+		return i2c_bus;
+
+	/* i2c adapter get */
+	rc = fdt_i2c_adapter_get(fdt, i2c_bus, &adapter);
+	if (rc)
+		return rc;
+
+	da9063.adapter = adapter;
+
+	rc = fdt_get_node_addr_size(fdt, nodeoff, 0, &addr, NULL);
+	if (rc)
+		return rc;
+
+	da9063.reg = addr;
+
+	sbi_system_reset_set_device(&da9063_reset_i2c);
+
+	return 0;
+}
+
+static const struct fdt_match da9063_reset_match[] = {
+	{ .compatible = "dlg,da9063", .data = (void *)TRUE },
+	{ },
+};
+
+struct fdt_reset fdt_reset_da9063 = {
+	.match_table = da9063_reset_match,
+	.init = da9063_reset_init,
+};
diff --git a/lib/utils/reset/objects.mk b/lib/utils/reset/objects.mk
index 6c95db3..cfe4c09 100644
--- a/lib/utils/reset/objects.mk
+++ b/lib/utils/reset/objects.mk
@@ -14,3 +14,4 @@ libsbiutils-objs-y += reset/fdt_reset_sifive_test.o
 libsbiutils-objs-y += reset/fdt_reset_sunxi_wdt.o
 libsbiutils-objs-y += reset/fdt_reset_thead.o
 libsbiutils-objs-y += reset/fdt_reset_thead_asm.o
+libsbiutils-objs-y += reset/fdt_reset_da9063.o
-- 
2.31.1

[PATCH 4/4] lib: utils/reset: Add generic da9063 reset driver

[Alexandre ghiti]

On 9/24/21 1:33 PM, Nikita Shubin wrote:
> From: Nikita Shubin <n.shubin@yadro.com>
>
> da9063 PMIC can be used to reset/shutdown the
> Sifive Unmatched board.
>
> shutdown is done simply by writing SHUDOWN bit to
> DA9063_REG_CONTROL_F register.


s/SHUDOWN/SHUTDOWN


>
> reset via setting WAKEUP bit in DA9063_REG_CONTROL_F
> register followed by masking POWER and POWER1 domains
> and setting STANDBY bit in DA9063_REG_CONTROL_A,
> originally discovered by Alexandre Ghiti on linux-riscv
> maillists.


Someone from Dialog answered this patch and will get back to us after
talking to the hardware team.


>
> Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
> ---
>  lib/utils/reset/fdt_reset.c        |   2 +
>  lib/utils/reset/fdt_reset_da9063.c | 203 +++++++++++++++++++++++++++++
>  lib/utils/reset/objects.mk         |   1 +
>  3 files changed, 206 insertions(+)
>  create mode 100644 lib/utils/reset/fdt_reset_da9063.c
>
> diff --git a/lib/utils/reset/fdt_reset.c b/lib/utils/reset/fdt_reset.c
> index 168bb0c..92f37b0 100644
> --- a/lib/utils/reset/fdt_reset.c
> +++ b/lib/utils/reset/fdt_reset.c
> @@ -18,6 +18,7 @@ extern struct fdt_reset fdt_reset_htif;
>  extern struct fdt_reset fdt_reset_sifive_test;
>  extern struct fdt_reset fdt_reset_sunxi_wdt;
>  extern struct fdt_reset fdt_reset_thead;
> +extern struct fdt_reset fdt_reset_da9063;
>  
>  static struct fdt_reset *reset_drivers[] = {
>  	&fdt_poweroff_gpio,
> @@ -26,6 +27,7 @@ static struct fdt_reset *reset_drivers[] = {
>  	&fdt_reset_sifive_test,
>  	&fdt_reset_sunxi_wdt,
>  	&fdt_reset_thead,
> +	&fdt_reset_da9063,
>  };
>  
>  int fdt_reset_init(void)
> diff --git a/lib/utils/reset/fdt_reset_da9063.c b/lib/utils/reset/fdt_reset_da9063.c
> new file mode 100644
> index 0000000..6c96968
> --- /dev/null
> +++ b/lib/utils/reset/fdt_reset_da9063.c
> @@ -0,0 +1,203 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright (c) 2021 YADRO
> + *
> + * Authors:
> + *   Nikita Shubin <nshubin@yadro.com>
> + */
> +
> +#include <libfdt.h>
> +#include <sbi/sbi_error.h>
> +#include <sbi/sbi_console.h>
> +#include <sbi/sbi_ecall_interface.h>
> +#include <sbi/sbi_hart.h>
> +#include <sbi/sbi_system.h>
> +#include <sbi_utils/fdt/fdt_helper.h>
> +#include <sbi_utils/i2c/fdt_i2c.h>
> +#include <sbi_utils/reset/fdt_reset.h>
> +
> +#define DA9063_REG_PAGE_CON		0x00
> +#define DA9063_REG_CONTROL_A		0x0e
> +#define DA9063_REG_CONTROL_C		0x10
> +#define DA9063_REG_CONTROL_F		0x13
> +#define DA9063_REG_DEVICE_ID		0x81
> +
> +#define DA9063_CONTROL_A_CP_EN		(1 << 7)
> +#define DA9063_CONTROL_A_M_POWER1_EN	(1 << 6)
> +#define DA9063_CONTROL_A_M_POWER_EN	(1 << 5)
> +#define DA9063_CONTROL_A_M_SYSTEM_EN	(1 << 4)
> +#define DA9063_CONTROL_A_STANDBY	(1 << 3)
> +#define DA9063_CONTROL_A_POWER1_EN	(1 << 2)
> +#define DA9063_CONTROL_A_POWER_EN	(1 << 1)
> +#define DA9063_CONTROL_A_SYSTEM_EN	(1 << 0)
> +
> +#define DA9063_CONTROL_F_WAKEUP		(1 << 2)
> +#define DA9063_CONTROL_F_SHUTDOWN	(1 << 1)
> +#define DA9063_CONTROL_F_WATCHDOG	(1 << 0)
> +
> +#define DA9063_CONTROL_C_DEF_SUPPLY	(1 << 7)
> +#define DA9063_CONTROL_C_SLEW_RATE	(1 << 4)
> +#define DA9063_CONTROL_C_OTPREAD_EN	(1 << 3)
> +#define DA9063_CONTROL_C_AUTO_BOOT	(1 << 2)
> +#define DA9063_CONTROL_C_DEBOUNCING	(1 << 0)


Not all above defines are used, we should stick to the strict minimum as
the only functionality we exploit from this device is the reset.


> +
> +#define PMIC_CHIP_ID_DA9063		0x61
> +
> +static struct {
> +	struct i2c_adapter *adapter;
> +	uint32_t reg;
> +	int last_error;
> +} da9063;
> +
> +
> +


3 new lines here


> +static int da9063_system_reset_check(u32 type, u32 reason)
> +{
> +	switch (type) {
> +	case SBI_SRST_RESET_TYPE_SHUTDOWN:
> +	case SBI_SRST_RESET_TYPE_COLD_REBOOT:
> +	case SBI_SRST_RESET_TYPE_WARM_REBOOT:
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +static inline int da9063_sanity_check(struct i2c_adapter *adap, uint32_t reg)


No need for the struct i2c_adapter argument as it is globally accessible.


> +{
> +	uint8_t val;
> +	int rc = i2c_adapter_send(adap, reg, DA9063_REG_PAGE_CON, 0x02);
> +
> +	if (rc)
> +		return rc;
> +
> +	/* check set page*/
> +	rc = i2c_adapter_read(adap, reg, 0x0, &val);
> +	if (rc)
> +		return rc;
> +
> +	if (val != 0x02)
> +		return SBI_ENODEV;
> +
> +	/* read and check device id */
> +	rc = i2c_adapter_read(adap, reg, DA9063_REG_DEVICE_ID, &val);
> +	if (rc)
> +		return rc;
> +
> +	if (val != PMIC_CHIP_ID_DA9063)
> +		return SBI_ENODEV;
> +
> +	return 0;
> +}
> +
> +static inline int da9063_shutdown(struct i2c_adapter *adap, uint32_t reg)


Same here regarding struct i2c_adapter argument.


> +{
> +	int rc = i2c_adapter_send(adap, da9063.reg, DA9063_REG_PAGE_CON, 0x00);
> +
> +	if (rc)
> +		return rc;
> +
> +	return i2c_adapter_send(adap, da9063.reg,
> +				DA9063_REG_CONTROL_F, DA9063_CONTROL_F_SHUTDOWN);
> +}
> +
> +static inline int da9063_reset(struct i2c_adapter *adap, uint32_t reg)


Same here regarding struct i2c_adapter argument.


> +{
> +	int rc = i2c_adapter_send(adap, da9063.reg, DA9063_REG_PAGE_CON, 0x00);
> +
> +	if (rc)
> +		return rc;
> +
> +	rc = i2c_adapter_send(adap, da9063.reg,
> +			      DA9063_REG_CONTROL_F, DA9063_CONTROL_F_WAKEUP);
> +	if (rc)
> +		return rc;
> +
> +	return i2c_adapter_send(adap, da9063.reg,
> +				DA9063_REG_CONTROL_A,
> +				DA9063_CONTROL_A_M_POWER1_EN |
> +				DA9063_CONTROL_A_M_POWER_EN |
> +				DA9063_CONTROL_A_STANDBY);
> +}
> +
> +static void da9063_system_reset(u32 type, u32 reason)
> +{
> +	struct i2c_adapter *adap = da9063.adapter;
> +	uint32_t reg = da9063.reg;
> +	int rc;
> +
> +	if (adap) {
> +		/* may include clock init */
> +		i2c_adapter_configure(adap);


Why should the da9063 device care about the adapter configuration here?
I think It should just rely on an already configured/initialized i2c
controller.


> +
> +		/* sanity check */
> +		rc = da9063_sanity_check(adap, reg);
> +		if (rc) {
> +			sbi_printf("%s: chip is not da9063 PMIC\n", __func__);
> +			goto skip_reset;
> +		}
> +
> +		switch (type) {
> +		case SBI_SRST_RESET_TYPE_SHUTDOWN:
> +			da9063_shutdown(adap, reg);
> +			break;
> +		case SBI_SRST_RESET_TYPE_COLD_REBOOT:
> +		case SBI_SRST_RESET_TYPE_WARM_REBOOT:
> +			da9063_reset(adap, reg);
> +			break;
> +		}
> +
> +		while
> +			(1);
> +
> +skip_reset:
> +		sbi_hart_hang();
> +	}
> +}
> +
> +static struct sbi_system_reset_device da9063_reset_i2c = {
> +	.name = "da9063-reset",
> +	.system_reset_check = da9063_system_reset_check,
> +	.system_reset = da9063_system_reset
> +};
> +
> +static int da9063_reset_init(void *fdt, int nodeoff,
> +			   const struct fdt_match *match)
> +{
> +	int rc, i2c_bus;
> +	struct i2c_adapter *adapter;
> +	uint64_t addr;
> +
> +	/* find i2c bus parent node */
> +	i2c_bus = fdt_parent_offset(fdt, nodeoff);
> +	if (i2c_bus < 0)
> +		return i2c_bus;
> +
> +	/* i2c adapter get */
> +	rc = fdt_i2c_adapter_get(fdt, i2c_bus, &adapter);
> +	if (rc)
> +		return rc;
> +
> +	da9063.adapter = adapter;


Establishing the link between the i2c device and its adapter should
somehow be implicitly done by the i2c library, IMO the device should not
care about its controller.


> +
> +	rc = fdt_get_node_addr_size(fdt, nodeoff, 0, &addr, NULL);
> +	if (rc)
> +		return rc;
> +
> +	da9063.reg = addr;
> +
> +	sbi_system_reset_set_device(&da9063_reset_i2c);
> +
> +	return 0;
> +}
> +
> +static const struct fdt_match da9063_reset_match[] = {
> +	{ .compatible = "dlg,da9063", .data = (void *)TRUE },
> +	{ },
> +};
> +
> +struct fdt_reset fdt_reset_da9063 = {
> +	.match_table = da9063_reset_match,
> +	.init = da9063_reset_init,
> +};
> diff --git a/lib/utils/reset/objects.mk b/lib/utils/reset/objects.mk
> index 6c95db3..cfe4c09 100644
> --- a/lib/utils/reset/objects.mk
> +++ b/lib/utils/reset/objects.mk
> @@ -14,3 +14,4 @@ libsbiutils-objs-y += reset/fdt_reset_sifive_test.o
>  libsbiutils-objs-y += reset/fdt_reset_sunxi_wdt.o
>  libsbiutils-objs-y += reset/fdt_reset_thead.o
>  libsbiutils-objs-y += reset/fdt_reset_thead_asm.o
> +libsbiutils-objs-y += reset/fdt_reset_da9063.o

[PATCH 4/4] lib: utils/reset: Add generic da9063 reset driver

[Nikita Shubin]

Hello Alexander!

On Mon, 27 Sep 2021 17:42:24 +0200
Alexandre ghiti <alex@ghiti.fr> wrote:

> On 9/24/21 1:33 PM, Nikita Shubin wrote:
> > From: Nikita Shubin <n.shubin@yadro.com>
> >
> > da9063 PMIC can be used to reset/shutdown the
> > Sifive Unmatched board.
> >
> > shutdown is done simply by writing SHUDOWN bit to
> > DA9063_REG_CONTROL_F register.  
> 
> 
> s/SHUDOWN/SHUTDOWN
> 
> 
> >
> > reset via setting WAKEUP bit in DA9063_REG_CONTROL_F
> > register followed by masking POWER and POWER1 domains
> > and setting STANDBY bit in DA9063_REG_CONTROL_A,
> > originally discovered by Alexandre Ghiti on linux-riscv
> > maillists.  
> 
> 
> Someone from Dialog answered this patch and will get back to us after
> talking to the hardware team.

You mean Adam Thomson - right ? Looking forward to his answer. Anyway
it seems that reseting via watchdog is a more reliable way to him. May
this is really a better way.

Yours,
Nikita Shubin

[PATCH 0/4] reboot SiFive Unmatched via PMIC

[Jessica Clarke]

On 24 Sep 2021, at 12:33, Nikita Shubin <nikita.shubin@maquefel.me> wrote:
> 
> From: Nikita Shubin <n.shubin@yadro.com>
> 
> This series introduce rebooting via i2c PMIC, currently on
> SiFive Unmatched board.
> 
> teseted via Linux with reset extension and direct ecall from 
> u-boot.
> 
> With this it becomes possible to reboot/shutdown the board 
> if Linux or u-boot is accompanied with OpenSBI reset extension.
> 
> "gpio-poweroff" is required to be removed from u-boot dts file, as
> it overrides fdt_reset_da9063.

I disagree with this. The GPIO has been provided by SiFive specifically
for powering off and so should be used. The DA9063 is only used for
reset because there is no reset GPIO, but it?s a bit of a hacky way of
resetting. IMO the DA9063 driver should be the lowest priority and only
used for poweroff or reset when there is no alternative.

Jess

[PATCH 0/4] reboot SiFive Unmatched via PMIC

[Anup Patel]

?On 24/09/21, 9:24 PM, "Jessica Clarke" <jrtc27@jrtc27.com> wrote:

    On 24 Sep 2021, at 12:33, Nikita Shubin <nikita.shubin@maquefel.me> wrote:
    > 
    > From: Nikita Shubin <n.shubin@yadro.com>
    > 
    > This series introduce rebooting via i2c PMIC, currently on
    > SiFive Unmatched board.
    > 
    > teseted via Linux with reset extension and direct ecall from 
    > u-boot.
    > 
    > With this it becomes possible to reboot/shutdown the board 
    > if Linux or u-boot is accompanied with OpenSBI reset extension.
    > 
    > "gpio-poweroff" is required to be removed from u-boot dts file, as
    > it overrides fdt_reset_da9063.

    I disagree with this. The GPIO has been provided by SiFive specifically
    for powering off and so should be used. The DA9063 is only used for
    reset because there is no reset GPIO, but it?s a bit of a hacky way of
    resetting. IMO the DA9063 driver should be the lowest priority and only
    used for poweroff or reset when there is no alternative.

We need to improve sbi_system.h APIs so that platform/drivers can
register callback for particular reset types. This will help reset types
provided different reset drivers and we can use GPIO (for poweroff)
+ DA9063 (for reset) on SiFive Unmatched.

Regards,
Anup

    Jess

[PATCH 0/4] reboot SiFive Unmatched via PMIC

[Nikita Shubin]

On Fri, 24 Sep 2021 16:02:07 +0000
Anup Patel <Anup.Patel@wdc.com> wrote:

> We need to improve sbi_system.h APIs so that platform/drivers can
> register callback for particular reset types. This will help reset
> types provided different reset drivers and we can use GPIO (for
> poweroff)
> + DA9063 (for reset) on SiFive Unmatched.

It's hard to disagree with this. With no one objecting - i am eager to
look into this.

Moreover i noticed that with no reset available at all the boot fails
with no visible objections.

Yours,
Nikita Shubin

[PATCH 0/4] reboot SiFive Unmatched via PMIC

[Anup Patel]

On Fri, Sep 24, 2021 at 9:36 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> On Fri, 24 Sep 2021 16:02:07 +0000
> Anup Patel <Anup.Patel@wdc.com> wrote:
>
> > We need to improve sbi_system.h APIs so that platform/drivers can
> > register callback for particular reset types. This will help reset
> > types provided different reset drivers and we can use GPIO (for
> > poweroff)
> > + DA9063 (for reset) on SiFive Unmatched.
>
> It's hard to disagree with this. With no one objecting - i am eager to
> look into this.

Sure, go ahead and include this as separate patches in your next
revision. You will have to update existing reset drivers as well.

>
> Moreover i noticed that with no reset available at all the boot fails
> with no visible objections.

Can you elaborate further ?

>
> Yours,
> Nikita Shubin
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

Regards,
Anup

[PATCH 0/4] reboot SiFive Unmatched via PMIC

[Nikita Shubin]

On Fri, 24 Sep 2021 21:39:09 +0530
Anup Patel <anup@brainfault.org> wrote:

> On Fri, Sep 24, 2021 at 9:36 PM Nikita Shubin
> <nikita.shubin@maquefel.me> wrote:
> >
> > On Fri, 24 Sep 2021 16:02:07 +0000
> > Anup Patel <Anup.Patel@wdc.com> wrote:
> >  
> > > We need to improve sbi_system.h APIs so that platform/drivers can
> > > register callback for particular reset types. This will help reset
> > > types provided different reset drivers and we can use GPIO (for
> > > poweroff)
> > > + DA9063 (for reset) on SiFive Unmatched.  
> >
> > It's hard to disagree with this. With no one objecting - i am eager
> > to look into this.  
> 
> Sure, go ahead and include this as separate patches in your next
> revision. You will have to update existing reset drivers as well.
> 
> >
> > Moreover i noticed that with no reset available at all the boot
> > fails with no visible objections.  
> 
> Can you elaborate further ?

Of course, why not.

Yours,
Nikita Shubin

> 
> >
> > Yours,
> > Nikita Shubin
> >
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi  
> 
> Regards,
> Anup

[PATCH 0/4] reboot SiFive Unmatched via PMIC

[Nikita Shubin]

On Fri, 24 Sep 2021 21:39:09 +0530
Anup Patel <anup@brainfault.org> wrote:

> On Fri, Sep 24, 2021 at 9:36 PM Nikita Shubin
> <nikita.shubin@maquefel.me> wrote:
> >
> > On Fri, 24 Sep 2021 16:02:07 +0000
> > Anup Patel <Anup.Patel@wdc.com> wrote:
> >  
> > > We need to improve sbi_system.h APIs so that platform/drivers can
> > > register callback for particular reset types. This will help reset
> > > types provided different reset drivers and we can use GPIO (for
> > > poweroff)
> > > + DA9063 (for reset) on SiFive Unmatched.  
> >
> > It's hard to disagree with this. With no one objecting - i am eager
> > to look into this.  
> 
> Sure, go ahead and include this as separate patches in your next
> revision. You will have to update existing reset drivers as well.
> 
> >
> > Moreover i noticed that with no reset available at all the boot
> > fails with no visible objections.  
> 
> Can you elaborate further ?

I noticed that if no reset driver is present at all the board won't
boot, but looking into the code - may be it's just in case drv->init
return an error, i will look further into it.

Yours,
Nikita Shubin

> 
> >
> > Yours,
> > Nikita Shubin
> >
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi  
> 
> Regards,
> Anup

[PATCH 0/4] reboot SiFive Unmatched via PMIC

[Bin Meng]

On Fri, Sep 24, 2021 at 7:34 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> From: Nikita Shubin <n.shubin@yadro.com>
>
> This series introduce rebooting via i2c PMIC, currently on
> SiFive Unmatched board.
>
> teseted via Linux with reset extension and direct ecall from
> u-boot.
>
> With this it becomes possible to reboot/shutdown the board
> if Linux or u-boot is accompanied with OpenSBI reset extension.
>
> "gpio-poweroff" is required to be removed from u-boot dts file, as
> it overrides fdt_reset_da9063.
>
> fdt_reset_da9063 is loaded if "dlg,da9063" is present in device tree.
>
> Moreover it's look like we require a convenient "sleep" functionality,
> currently i've implemented sleep via CSR_MCYCLE, assuming we don't normally
> communicate with device while operational only on start/shutdown.
>
> ```
> static inline void wait_cycles(unsigned long cycles)
> {
>         csr_write(CSR_MCYCLE, 0);
>         while (cycles > csr_read_num(CSR_MCYCLE));
> }
> ```
>
> The original sequence was discovered by Alexandre Ghiti posted a patch on
> linux-riscv mail-lists.
>
> https://patchwork.kernel.org/project/linux-riscv/patch/20210921053356.1705833-1-alexandre.ghiti at canonical.com/
>
> These magick numbers are readed as:
> - select PAGE0
> - set WAKE_UP bit in CONTROL_A reg
> - mask setting POWER1, POWER domain and STANDBY, and poweroff SYSTEM domain
>
> Nikita Shubin (4):
>   lib: utils/i2c: Add generic I2C configuration library
>   lib: utils/gpio: Add simple FDT based I2C framework
>   lib: utils/gpio: Add minimal SiFive I2C driver
>   lib: utils/reset: Add generic da9063 reset driver
>

It looks like we will end up having lots of reset drivers in OpenSBI
in the future, no surprise, as this is what the spec requires, sigh.

Alexandre Ghiti has posted U-Boot patches to reset Unmatched via PMIC as well.
http://patchwork.ozlabs.org/project/uboot/patch/20210924084231.3311216-5-alexandre.ghiti at canonical.com/

Regards,
Bin

[PATCH 0/4] reboot SiFive Unmatched via PMIC

[Anup Patel]

On Sat, Sep 25, 2021 at 6:40 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Fri, Sep 24, 2021 at 7:34 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
> >
> > From: Nikita Shubin <n.shubin@yadro.com>
> >
> > This series introduce rebooting via i2c PMIC, currently on
> > SiFive Unmatched board.
> >
> > teseted via Linux with reset extension and direct ecall from
> > u-boot.
> >
> > With this it becomes possible to reboot/shutdown the board
> > if Linux or u-boot is accompanied with OpenSBI reset extension.
> >
> > "gpio-poweroff" is required to be removed from u-boot dts file, as
> > it overrides fdt_reset_da9063.
> >
> > fdt_reset_da9063 is loaded if "dlg,da9063" is present in device tree.
> >
> > Moreover it's look like we require a convenient "sleep" functionality,
> > currently i've implemented sleep via CSR_MCYCLE, assuming we don't normally
> > communicate with device while operational only on start/shutdown.
> >
> > ```
> > static inline void wait_cycles(unsigned long cycles)
> > {
> >         csr_write(CSR_MCYCLE, 0);
> >         while (cycles > csr_read_num(CSR_MCYCLE));
> > }
> > ```
> >
> > The original sequence was discovered by Alexandre Ghiti posted a patch on
> > linux-riscv mail-lists.
> >
> > https://patchwork.kernel.org/project/linux-riscv/patch/20210921053356.1705833-1-alexandre.ghiti at canonical.com/
> >
> > These magick numbers are readed as:
> > - select PAGE0
> > - set WAKE_UP bit in CONTROL_A reg
> > - mask setting POWER1, POWER domain and STANDBY, and poweroff SYSTEM domain
> >
> > Nikita Shubin (4):
> >   lib: utils/i2c: Add generic I2C configuration library
> >   lib: utils/gpio: Add simple FDT based I2C framework
> >   lib: utils/gpio: Add minimal SiFive I2C driver
> >   lib: utils/reset: Add generic da9063 reset driver
> >
>
> It looks like we will end up having lots of reset drivers in OpenSBI
> in the future, no surprise, as this is what the spec requires, sigh.
>
> Alexandre Ghiti has posted U-Boot patches to reset Unmatched via PMIC as well.
> http://patchwork.ozlabs.org/project/uboot/patch/20210924084231.3311216-5-alexandre.ghiti at canonical.com/

This diversity with reset drivers is not a problem just for OpenSBI
but it applies to all OSes (Linux, FreeBSD, VxWorks, ....),
Bootloaders (U-Boot, EDK2, Coreboot, ....), and Hypervisors
(Xvisor, Xen, Bao, ....).

Just imagine, each hypervisor having its own reset mechanism
for Guest/VM and suddenly we start having hypervisor specific
reset drivers in various OSes and Bootloaders.

Even more critical issue is TEE where we can't let non-secure
domains directly reset the system without the secure domain
knowing about it.

With SBI SRST extension the above issues are handled
as follows:
1) The diversity in platform specific resets is restricted to
M-mode runtime firmware (OpenSBI)
2) All hypervisors provide SBI SRST to Guest/VM as the
standard reset mechanism
3) Reset requests from TEE non-secure domains come to
M-mode runtime firmware (OpenSBI) which can then
notify the TEE secure domain to take necessary action.
Also, reset request from the TEE secure domain will
directly reset the system.

Point#3 above has been already implemented in OpenSBI
domains.

We have been discussing SBI SRST for almost 2 years now.

We also had extensive discussion about this at LPC 2020.
https://linuxplumbersconf.org/event/7/contributions/810/attachments/631/1144/RISCV_EBBR_lpc2020.pdf

Regards,
Anup

>
> Regards,
> Bin
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH 0/4] reboot SiFive Unmatched via PMIC

[Bin Meng]

On Sat, Sep 25, 2021 at 11:56 AM Anup Patel <anup@brainfault.org> wrote:
>
> On Sat, Sep 25, 2021 at 6:40 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Fri, Sep 24, 2021 at 7:34 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
> > >
> > > From: Nikita Shubin <n.shubin@yadro.com>
> > >
> > > This series introduce rebooting via i2c PMIC, currently on
> > > SiFive Unmatched board.
> > >
> > > teseted via Linux with reset extension and direct ecall from
> > > u-boot.
> > >
> > > With this it becomes possible to reboot/shutdown the board
> > > if Linux or u-boot is accompanied with OpenSBI reset extension.
> > >
> > > "gpio-poweroff" is required to be removed from u-boot dts file, as
> > > it overrides fdt_reset_da9063.
> > >
> > > fdt_reset_da9063 is loaded if "dlg,da9063" is present in device tree.
> > >
> > > Moreover it's look like we require a convenient "sleep" functionality,
> > > currently i've implemented sleep via CSR_MCYCLE, assuming we don't normally
> > > communicate with device while operational only on start/shutdown.
> > >
> > > ```
> > > static inline void wait_cycles(unsigned long cycles)
> > > {
> > >         csr_write(CSR_MCYCLE, 0);
> > >         while (cycles > csr_read_num(CSR_MCYCLE));
> > > }
> > > ```
> > >
> > > The original sequence was discovered by Alexandre Ghiti posted a patch on
> > > linux-riscv mail-lists.
> > >
> > > https://patchwork.kernel.org/project/linux-riscv/patch/20210921053356.1705833-1-alexandre.ghiti at canonical.com/
> > >
> > > These magick numbers are readed as:
> > > - select PAGE0
> > > - set WAKE_UP bit in CONTROL_A reg
> > > - mask setting POWER1, POWER domain and STANDBY, and poweroff SYSTEM domain
> > >
> > > Nikita Shubin (4):
> > >   lib: utils/i2c: Add generic I2C configuration library
> > >   lib: utils/gpio: Add simple FDT based I2C framework
> > >   lib: utils/gpio: Add minimal SiFive I2C driver
> > >   lib: utils/reset: Add generic da9063 reset driver
> > >
> >
> > It looks like we will end up having lots of reset drivers in OpenSBI
> > in the future, no surprise, as this is what the spec requires, sigh.
> >
> > Alexandre Ghiti has posted U-Boot patches to reset Unmatched via PMIC as well.
> > http://patchwork.ozlabs.org/project/uboot/patch/20210924084231.3311216-5-alexandre.ghiti at canonical.com/
>
> This diversity with reset drivers is not a problem just for OpenSBI
> but it applies to all OSes (Linux, FreeBSD, VxWorks, ....),
> Bootloaders (U-Boot, EDK2, Coreboot, ....), and Hypervisors
> (Xvisor, Xen, Bao, ....).
>
> Just imagine, each hypervisor having its own reset mechanism
> for Guest/VM and suddenly we start having hypervisor specific
> reset drivers in various OSes and Bootloaders.
>
> Even more critical issue is TEE where we can't let non-secure
> domains directly reset the system without the secure domain
> knowing about it.
>
> With SBI SRST extension the above issues are handled
> as follows:
> 1) The diversity in platform specific resets is restricted to
> M-mode runtime firmware (OpenSBI)
> 2) All hypervisors provide SBI SRST to Guest/VM as the
> standard reset mechanism
> 3) Reset requests from TEE non-secure domains come to
> M-mode runtime firmware (OpenSBI) which can then
> notify the TEE secure domain to take necessary action.
> Also, reset request from the TEE secure domain will
> directly reset the system.
>
> Point#3 above has been already implemented in OpenSBI
> domains.
>
> We have been discussing SBI SRST for almost 2 years now.
>
> We also had extensive discussion about this at LPC 2020.
> https://linuxplumbersconf.org/event/7/contributions/810/attachments/631/1144/RISCV_EBBR_lpc2020.pdf
>

I understand the rationale behind this. I just have a concern about
various reset drivers being put in the OpenSBI, which will bloat its
size (the generic firmware) very quickly.

We've spent lots of time standlizing the ACLINT/AIA stuff as we don't
want each hardware vendor to invent his own interrupt system. Could we
do something similar to the system reset controller to go a step
further?

I don't have a historical reason why the Arm ecosystem has not come to
such a path regarding system reset controllers. I am sure I missed
something. x86 does this with a hardware that conforms with the ACPI
programming interface so there is only one reset driver.

Regards,
Bin

[PATCH 0/4] reboot SiFive Unmatched via PMIC

[Anup Patel]

On Sat, Sep 25, 2021 at 10:16 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sat, Sep 25, 2021 at 11:56 AM Anup Patel <anup@brainfault.org> wrote:
> >
> > On Sat, Sep 25, 2021 at 6:40 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > On Fri, Sep 24, 2021 at 7:34 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
> > > >
> > > > From: Nikita Shubin <n.shubin@yadro.com>
> > > >
> > > > This series introduce rebooting via i2c PMIC, currently on
> > > > SiFive Unmatched board.
> > > >
> > > > teseted via Linux with reset extension and direct ecall from
> > > > u-boot.
> > > >
> > > > With this it becomes possible to reboot/shutdown the board
> > > > if Linux or u-boot is accompanied with OpenSBI reset extension.
> > > >
> > > > "gpio-poweroff" is required to be removed from u-boot dts file, as
> > > > it overrides fdt_reset_da9063.
> > > >
> > > > fdt_reset_da9063 is loaded if "dlg,da9063" is present in device tree.
> > > >
> > > > Moreover it's look like we require a convenient "sleep" functionality,
> > > > currently i've implemented sleep via CSR_MCYCLE, assuming we don't normally
> > > > communicate with device while operational only on start/shutdown.
> > > >
> > > > ```
> > > > static inline void wait_cycles(unsigned long cycles)
> > > > {
> > > >         csr_write(CSR_MCYCLE, 0);
> > > >         while (cycles > csr_read_num(CSR_MCYCLE));
> > > > }
> > > > ```
> > > >
> > > > The original sequence was discovered by Alexandre Ghiti posted a patch on
> > > > linux-riscv mail-lists.
> > > >
> > > > https://patchwork.kernel.org/project/linux-riscv/patch/20210921053356.1705833-1-alexandre.ghiti at canonical.com/
> > > >
> > > > These magick numbers are readed as:
> > > > - select PAGE0
> > > > - set WAKE_UP bit in CONTROL_A reg
> > > > - mask setting POWER1, POWER domain and STANDBY, and poweroff SYSTEM domain
> > > >
> > > > Nikita Shubin (4):
> > > >   lib: utils/i2c: Add generic I2C configuration library
> > > >   lib: utils/gpio: Add simple FDT based I2C framework
> > > >   lib: utils/gpio: Add minimal SiFive I2C driver
> > > >   lib: utils/reset: Add generic da9063 reset driver
> > > >
> > >
> > > It looks like we will end up having lots of reset drivers in OpenSBI
> > > in the future, no surprise, as this is what the spec requires, sigh.
> > >
> > > Alexandre Ghiti has posted U-Boot patches to reset Unmatched via PMIC as well.
> > > http://patchwork.ozlabs.org/project/uboot/patch/20210924084231.3311216-5-alexandre.ghiti at canonical.com/
> >
> > This diversity with reset drivers is not a problem just for OpenSBI
> > but it applies to all OSes (Linux, FreeBSD, VxWorks, ....),
> > Bootloaders (U-Boot, EDK2, Coreboot, ....), and Hypervisors
> > (Xvisor, Xen, Bao, ....).
> >
> > Just imagine, each hypervisor having its own reset mechanism
> > for Guest/VM and suddenly we start having hypervisor specific
> > reset drivers in various OSes and Bootloaders.
> >
> > Even more critical issue is TEE where we can't let non-secure
> > domains directly reset the system without the secure domain
> > knowing about it.
> >
> > With SBI SRST extension the above issues are handled
> > as follows:
> > 1) The diversity in platform specific resets is restricted to
> > M-mode runtime firmware (OpenSBI)
> > 2) All hypervisors provide SBI SRST to Guest/VM as the
> > standard reset mechanism
> > 3) Reset requests from TEE non-secure domains come to
> > M-mode runtime firmware (OpenSBI) which can then
> > notify the TEE secure domain to take necessary action.
> > Also, reset request from the TEE secure domain will
> > directly reset the system.
> >
> > Point#3 above has been already implemented in OpenSBI
> > domains.
> >
> > We have been discussing SBI SRST for almost 2 years now.
> >
> > We also had extensive discussion about this at LPC 2020.
> > https://linuxplumbersconf.org/event/7/contributions/810/attachments/631/1144/RISCV_EBBR_lpc2020.pdf
> >
>
> I understand the rationale behind this. I just have a concern about
> various reset drivers being put in the OpenSBI, which will bloat its
> size (the generic firmware) very quickly.

I was thinking of upgrading sifive_test reset driver into syscon
poweroff/reboot driver.

This will give us two generic reset drivers (GPIO and Syscon) which
pick-up most details from DT.

In future, platform specs can make recommendations around
reset mechanisms which will align with GPIO or Syscon reset.
Although, platforms not complying with platform specs might
still come-up with their own reset mechanism.

>
> We've spent lots of time standlizing the ACLINT/AIA stuff as we don't
> want each hardware vendor to invent his own interrupt system. Could we
> do something similar to the system reset controller to go a step
> further?

Agree. At least ACLINT and AIA will certainly save us efforts on
the timer and interrupt controller front.

>
> I don't have a historical reason why the Arm ecosystem has not come to
> such a path regarding system reset controllers. I am sure I missed
> something. x86 does this with a hardware that conforms with the ACPI
> programming interface so there is only one reset driver.

ARM only standardized a PSCI call for reset but they did not
standardize HW device/registers for reset. I am not sure why.

>
> Regards,
> Bin

Regards,
Anup

[PATCH 0/4] reboot SiFive Unmatched via PMIC

[Heinrich Schuchardt]

On 9/25/21 3:10 AM, Bin Meng wrote:
> On Fri, Sep 24, 2021 at 7:34 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>>
>> From: Nikita Shubin <n.shubin@yadro.com>
>>
>> This series introduce rebooting via i2c PMIC, currently on
>> SiFive Unmatched board.
>>
>> teseted via Linux with reset extension and direct ecall from
>> u-boot.
>>
>> With this it becomes possible to reboot/shutdown the board
>> if Linux or u-boot is accompanied with OpenSBI reset extension.
>>
>> "gpio-poweroff" is required to be removed from u-boot dts file, as
>> it overrides fdt_reset_da9063.
>>
>> fdt_reset_da9063 is loaded if "dlg,da9063" is present in device tree.
>>
>> Moreover it's look like we require a convenient "sleep" functionality,
>> currently i've implemented sleep via CSR_MCYCLE, assuming we don't normally
>> communicate with device while operational only on start/shutdown.
>>
>> ```
>> static inline void wait_cycles(unsigned long cycles)
>> {
>>          csr_write(CSR_MCYCLE, 0);
>>          while (cycles > csr_read_num(CSR_MCYCLE));
>> }
>> ```
>>
>> The original sequence was discovered by Alexandre Ghiti posted a patch on
>> linux-riscv mail-lists.
>>
>> https://patchwork.kernel.org/project/linux-riscv/patch/20210921053356.1705833-1-alexandre.ghiti at canonical.com/
>>
>> These magick numbers are readed as:
>> - select PAGE0
>> - set WAKE_UP bit in CONTROL_A reg
>> - mask setting POWER1, POWER domain and STANDBY, and poweroff SYSTEM domain
>>
>> Nikita Shubin (4):
>>    lib: utils/i2c: Add generic I2C configuration library
>>    lib: utils/gpio: Add simple FDT based I2C framework
>>    lib: utils/gpio: Add minimal SiFive I2C driver
>>    lib: utils/reset: Add generic da9063 reset driver
>>
> 
> It looks like we will end up having lots of reset drivers in OpenSBI
> in the future, no surprise, as this is what the spec requires, sigh.

At some point we will have to decide on a configuration system to enable 
only those drivers that are needed for the current hardware. 
Unfortunately we cannot reuse Linux Kconfig as our license is BSD-2.

Currently if a reset driver fails it simply ends up hanging in an 
endless loop.

The SiFive Unmatched will use GPIO for poweroff and I2C for reset.

We need the ability to iterate through multiple reset drivers. If 
resetting is not possible, we should return to the caller with an error 
code.

Best regards

Heinrich

> 
> Alexandre Ghiti has posted U-Boot patches to reset Unmatched via PMIC as well.
> http://patchwork.ozlabs.org/project/uboot/patch/20210924084231.3311216-5-alexandre.ghiti at canonical.com/
> 
> Regards,
> Bin
> 

[PATCH 0/4] reboot SiFive Unmatched via PMIC

[Nikita Shubin]

Hello Heinrich,

On Sat, 25 Sep 2021 10:11:29 +0200
Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote:

> At some point we will have to decide on a configuration system to
> enable only those drivers that are needed for the current hardware. 
> Unfortunately we cannot reuse Linux Kconfig as our license is BSD-2.
> 

Actually KConfig license does not apply to the code it is building and
configuring. 


> Currently if a reset driver fails it simply ends up hanging in an 
> endless loop.
> 
> The SiFive Unmatched will use GPIO for poweroff and I2C for reset.
> 
> We need the ability to iterate through multiple reset drivers. If 
> resetting is not possible, we should return to the caller with an
> error code.

How it is supposed to return to Linux kernel reset handler for example
and for what ? Could you please explain in detail ?

Yours,
Nikita Shubin

[PATCH 0/4] reboot SiFive Unmatched via PMIC

[Anup Patel]

On Sat, Sep 25, 2021 at 1:41 PM Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 9/25/21 3:10 AM, Bin Meng wrote:
> > On Fri, Sep 24, 2021 at 7:34 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
> >>
> >> From: Nikita Shubin <n.shubin@yadro.com>
> >>
> >> This series introduce rebooting via i2c PMIC, currently on
> >> SiFive Unmatched board.
> >>
> >> teseted via Linux with reset extension and direct ecall from
> >> u-boot.
> >>
> >> With this it becomes possible to reboot/shutdown the board
> >> if Linux or u-boot is accompanied with OpenSBI reset extension.
> >>
> >> "gpio-poweroff" is required to be removed from u-boot dts file, as
> >> it overrides fdt_reset_da9063.
> >>
> >> fdt_reset_da9063 is loaded if "dlg,da9063" is present in device tree.
> >>
> >> Moreover it's look like we require a convenient "sleep" functionality,
> >> currently i've implemented sleep via CSR_MCYCLE, assuming we don't normally
> >> communicate with device while operational only on start/shutdown.
> >>
> >> ```
> >> static inline void wait_cycles(unsigned long cycles)
> >> {
> >>          csr_write(CSR_MCYCLE, 0);
> >>          while (cycles > csr_read_num(CSR_MCYCLE));
> >> }
> >> ```
> >>
> >> The original sequence was discovered by Alexandre Ghiti posted a patch on
> >> linux-riscv mail-lists.
> >>
> >> https://patchwork.kernel.org/project/linux-riscv/patch/20210921053356.1705833-1-alexandre.ghiti at canonical.com/
> >>
> >> These magick numbers are readed as:
> >> - select PAGE0
> >> - set WAKE_UP bit in CONTROL_A reg
> >> - mask setting POWER1, POWER domain and STANDBY, and poweroff SYSTEM domain
> >>
> >> Nikita Shubin (4):
> >>    lib: utils/i2c: Add generic I2C configuration library
> >>    lib: utils/gpio: Add simple FDT based I2C framework
> >>    lib: utils/gpio: Add minimal SiFive I2C driver
> >>    lib: utils/reset: Add generic da9063 reset driver
> >>
> >
> > It looks like we will end up having lots of reset drivers in OpenSBI
> > in the future, no surprise, as this is what the spec requires, sigh.
>
> At some point we will have to decide on a configuration system to enable
> only those drivers that are needed for the current hardware.
> Unfortunately we cannot reuse Linux Kconfig as our license is BSD-2.

Yes, eventually we will need some kind of Kconfig-like approach where
each platform will mandatorily have a "defconfig" and few other custom
"xyz-defconfig" files. The generic platform "defconfig" will have most
drivers enabled by default but users can create custom defconfigs for
generic platform and restrict the number of drivers compiled and linked
to firmwares.

If we can't re-use Linux Kconfig then probably we might need to write
a simple config tool just for the OpenSBI project.

>
> Currently if a reset driver fails it simply ends up hanging in an
> endless loop.
>
> The SiFive Unmatched will use GPIO for poweroff and I2C for reset.
>
> We need the ability to iterate through multiple reset drivers. If
> resetting is not possible, we should return to the caller with an error
> code.

Yes, we can do this in generic SBI library sbi_system.c

Regards,
Anup

>
> Best regards
>
> Heinrich
>
> >
> > Alexandre Ghiti has posted U-Boot patches to reset Unmatched via PMIC as well.
> > http://patchwork.ozlabs.org/project/uboot/patch/20210924084231.3311216-5-alexandre.ghiti at canonical.com/
> >
> > Regards,
> > Bin
> >
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi