* [PATCH v2 1/5] lib: utils/reset: add priority to gpio reset
2021-10-15 13:19 [PATCH v2 0/5] I2C framework, reboot Unmatched via PMIC Nikita Shubin
@ 2021-10-15 13:19 ` Nikita Shubin
2021-10-15 13:19 ` [PATCH v2 2/5] lib: utils/i2c: Add generic I2C configuration library Nikita Shubin
` (6 subsequent siblings)
7 siblings, 0 replies; 38+ messages in thread
From: Nikita Shubin @ 2021-10-15 13:19 UTC (permalink / raw)
To: opensbi
From: Nikita Shubin <n.shubin@yadro.com>
Make gpio_system_reset_check return priority instead of just true/false.
Make default 128 priority for reset/shutdown and ability to specify
priority in device tree.
Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
---
lib/utils/reset/fdt_reset_gpio.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/lib/utils/reset/fdt_reset_gpio.c b/lib/utils/reset/fdt_reset_gpio.c
index 4da1450..44b67d8 100644
--- a/lib/utils/reset/fdt_reset_gpio.c
+++ b/lib/utils/reset/fdt_reset_gpio.c
@@ -23,16 +23,19 @@ struct gpio_reset {
struct gpio_pin pin;
u32 active_delay;
u32 inactive_delay;
+ u8 priority;
};
static struct gpio_reset poweroff = {
.active_delay = 100,
- .inactive_delay = 100
+ .inactive_delay = 100,
+ .priority = 128
};
static struct gpio_reset restart = {
.active_delay = 100,
- .inactive_delay = 100
+ .inactive_delay = 100,
+ .priority = 128
};
static struct gpio_reset *gpio_get_reset_settings(u32 type)
@@ -59,7 +62,12 @@ static struct gpio_reset *gpio_get_reset_settings(u32 type)
static int gpio_system_reset_check(u32 type, u32 reason)
{
- return !!gpio_get_reset_settings(type);
+ struct gpio_reset *reset = gpio_get_reset_settings(type);
+
+ if (reset)
+ return reset->priority;
+
+ return 0;
}
static void gpio_system_reset(u32 type, u32 reason)
@@ -115,6 +123,10 @@ static int gpio_reset_init(void *fdt, int nodeoff,
if (len > 0)
reset->inactive_delay = fdt32_to_cpu(*val);
+ val = fdt_getprop(fdt, nodeoff, "priority", &len);
+ if (len > 0)
+ reset->priority = fdt32_to_cpu(*val);
+
sbi_system_reset_add_device(&gpio_reset);
return 0;
--
2.31.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v2 2/5] lib: utils/i2c: Add generic I2C configuration library
2021-10-15 13:19 [PATCH v2 0/5] I2C framework, reboot Unmatched via PMIC Nikita Shubin
2021-10-15 13:19 ` [PATCH v2 1/5] lib: utils/reset: add priority to gpio reset Nikita Shubin
@ 2021-10-15 13:19 ` Nikita Shubin
2021-10-19 12:04 ` Alexandre Ghiti
2021-10-15 13:19 ` [PATCH v2 3/5] lib: utils/i2c: Add simple FDT based I2C framework Nikita Shubin
` (5 subsequent siblings)
7 siblings, 1 reply; 38+ messages in thread
From: Nikita Shubin @ 2021-10-15 13:19 UTC (permalink / raw)
To: opensbi
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>
---
v1 -> v2:
- switch to lists instead of array
- rename send to write
- use buffers instead of single byte
- add single byte read/write
---
include/sbi_utils/i2c/i2c.h | 99 +++++++++++++++++++++++++++++++++++++
lib/utils/i2c/i2c.c | 85 +++++++++++++++++++++++++++++++
lib/utils/i2c/objects.mk | 10 ++++
3 files changed, 194 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..76cdb66
--- /dev/null
+++ b/include/sbi_utils/i2c/i2c.h
@@ -0,0 +1,99 @@
+/*
+ * 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>
+#include <sbi/sbi_list.h>
+
+/** Representation of a I2C adapter */
+struct i2c_adapter {
+ /** Pointer to I2C driver owning this I2C adapter */
+ void *driver;
+
+ /** Unique 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 buffer to given address, register
+ *
+ * @return 0 on success and negative error code on failure
+ */
+ int (*smbus_write)(struct i2c_adapter *ia, uint8_t addr, uint8_t reg,
+ uint8_t *buffer, int len);
+
+ /**
+ * Read buffer from given address, register
+ *
+ * @return 0 on success and negative error code on failure
+ */
+ int (*smbus_read)(struct i2c_adapter *ia, uint8_t addr, uint8_t reg,
+ uint8_t *buffer, int len);
+
+ /** List */
+ struct sbi_dlist node;
+};
+
+static inline struct i2c_adapter *to_i2c_adapter(struct sbi_dlist *node)
+{
+ return container_of(node, struct i2c_adapter, node);
+}
+
+/** 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);
+
+/** Write to device on I2C adapter bus */
+int i2c_adapter_smbus_write(struct i2c_adapter *ia, uint8_t addr,
+ uint8_t reg, uint8_t *buffer, int len);
+
+/** Read from device on I2C adapter bus */
+int i2c_adapter_smbus_read(struct i2c_adapter *ia, uint8_t addr,
+ uint8_t reg, uint8_t *buffer, int len);
+
+/** Write single byte from device on I2C adapter bus */
+static inline int i2c_adapter_smbus_reg_write(struct i2c_adapter *ia,
+ uint8_t addr, uint8_t reg, uint8_t val)
+{
+ return i2c_adapter_smbus_write(ia, addr, reg, &val, 1);
+}
+
+/** Read single byte from device on I2C adapter bus */
+static inline int i2c_adapter_smbus_reg_read(struct i2c_adapter *ia,
+ uint8_t addr, uint8_t reg, uint8_t *val)
+{
+ uint8_t buf;
+ int ret = i2c_adapter_smbus_read(ia, addr, reg, &buf, 1);
+
+ if (ret)
+ return ret;
+
+ *val = buf;
+ return 0;
+}
+
+#endif
diff --git a/lib/utils/i2c/i2c.c b/lib/utils/i2c/i2c.c
new file mode 100644
index 0000000..d23ac91
--- /dev/null
+++ b/lib/utils/i2c/i2c.c
@@ -0,0 +1,85 @@
+/*
+ * 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>
+
+static SBI_LIST_HEAD(i2c_adapters_list);
+
+struct i2c_adapter *i2c_adapter_find(int id)
+{
+ struct sbi_dlist *pos;
+
+ sbi_list_for_each(pos, &(i2c_adapters_list)) {
+ struct i2c_adapter *adap = to_i2c_adapter(pos);
+
+ if (adap->id == id)
+ return adap;
+ }
+
+ return NULL;
+}
+
+int i2c_adapter_add(struct i2c_adapter *ia)
+{
+ if (!ia)
+ return SBI_EINVAL;
+
+ if (i2c_adapter_find(ia->id))
+ return SBI_EALREADY;
+
+ sbi_list_add(&(ia->node), &(i2c_adapters_list));
+
+ return 0;
+}
+
+void i2c_adapter_remove(struct i2c_adapter *ia)
+{
+ if (!ia)
+ return;
+
+ sbi_list_del(&(ia->node));
+}
+
+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_smbus_write(struct i2c_adapter *ia, uint8_t addr,
+ uint8_t reg, uint8_t *buffer, int len)
+{
+ if (!ia)
+ return SBI_EINVAL;
+ if (!ia->smbus_write)
+ return SBI_ENOSYS;
+
+ return ia->smbus_write(ia, addr, reg, buffer, len);
+}
+
+
+int i2c_adapter_smbus_read(struct i2c_adapter *ia, uint8_t addr,
+ uint8_t reg, uint8_t *buffer, int len)
+{
+ if (!ia)
+ return SBI_EINVAL;
+ if (!ia->smbus_read)
+ return SBI_ENOSYS;
+
+ return ia->smbus_read(ia, addr, reg, buffer, len);
+}
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
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v2 2/5] lib: utils/i2c: Add generic I2C configuration library
2021-10-15 13:19 ` [PATCH v2 2/5] lib: utils/i2c: Add generic I2C configuration library Nikita Shubin
@ 2021-10-19 12:04 ` Alexandre Ghiti
2021-10-20 6:41 ` Nikita Shubin
0 siblings, 1 reply; 38+ messages in thread
From: Alexandre Ghiti @ 2021-10-19 12:04 UTC (permalink / raw)
To: opensbi
On Fri, Oct 15, 2021 at 3:20 PM Nikita Shubin
<nikita.shubin@maquefel.me> 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>
> ---
> v1 -> v2:
> - switch to lists instead of array
> - rename send to write
> - use buffers instead of single byte
> - add single byte read/write
> ---
> include/sbi_utils/i2c/i2c.h | 99 +++++++++++++++++++++++++++++++++++++
> lib/utils/i2c/i2c.c | 85 +++++++++++++++++++++++++++++++
> lib/utils/i2c/objects.mk | 10 ++++
> 3 files changed, 194 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..76cdb66
> --- /dev/null
> +++ b/include/sbi_utils/i2c/i2c.h
> @@ -0,0 +1,99 @@
> +/*
> + * 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>
> +#include <sbi/sbi_list.h>
> +
> +/** Representation of a I2C adapter */
> +struct i2c_adapter {
> + /** Pointer to I2C driver owning this I2C adapter */
> + void *driver;
> +
> + /** Unique 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 buffer to given address, register
> + *
> + * @return 0 on success and negative error code on failure
> + */
> + int (*smbus_write)(struct i2c_adapter *ia, uint8_t addr, uint8_t reg,
> + uint8_t *buffer, int len);
Why did you rename those functions with smbus prefix? And I would use
size_t for the len argument.
> +
> + /**
> + * Read buffer from given address, register
> + *
> + * @return 0 on success and negative error code on failure
> + */
> + int (*smbus_read)(struct i2c_adapter *ia, uint8_t addr, uint8_t reg,
> + uint8_t *buffer, int len);
> +
> + /** List */
> + struct sbi_dlist node;
> +};
> +
> +static inline struct i2c_adapter *to_i2c_adapter(struct sbi_dlist *node)
> +{
> + return container_of(node, struct i2c_adapter, node);
> +}
> +
> +/** 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);
> +
> +/** Write to device on I2C adapter bus */
> +int i2c_adapter_smbus_write(struct i2c_adapter *ia, uint8_t addr,
> + uint8_t reg, uint8_t *buffer, int len);
> +
> +/** Read from device on I2C adapter bus */
> +int i2c_adapter_smbus_read(struct i2c_adapter *ia, uint8_t addr,
> + uint8_t reg, uint8_t *buffer, int len);
> +
> +/** Write single byte from device on I2C adapter bus */
> +static inline int i2c_adapter_smbus_reg_write(struct i2c_adapter *ia,
> + uint8_t addr, uint8_t reg, uint8_t val)
> +{
> + return i2c_adapter_smbus_write(ia, addr, reg, &val, 1);
> +}
> +
> +/** Read single byte from device on I2C adapter bus */
> +static inline int i2c_adapter_smbus_reg_read(struct i2c_adapter *ia,
> + uint8_t addr, uint8_t reg, uint8_t *val)
> +{
> + uint8_t buf;
> + int ret = i2c_adapter_smbus_read(ia, addr, reg, &buf, 1);
> +
> + if (ret)
> + return ret;
> +
> + *val = buf;
> + return 0;
> +}
Nice to add the helpers, though I'm not sure "reg_read" is the right
suffix as it may not be quite explicit: what about simply read_byte?
> +
> +#endif
> diff --git a/lib/utils/i2c/i2c.c b/lib/utils/i2c/i2c.c
> new file mode 100644
> index 0000000..d23ac91
> --- /dev/null
> +++ b/lib/utils/i2c/i2c.c
> @@ -0,0 +1,85 @@
> +/*
> + * 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>
> +
> +static SBI_LIST_HEAD(i2c_adapters_list);
> +
> +struct i2c_adapter *i2c_adapter_find(int id)
> +{
> + struct sbi_dlist *pos;
> +
> + sbi_list_for_each(pos, &(i2c_adapters_list)) {
> + struct i2c_adapter *adap = to_i2c_adapter(pos);
> +
> + if (adap->id == id)
> + return adap;
> + }
> +
> + return NULL;
> +}
> +
> +int i2c_adapter_add(struct i2c_adapter *ia)
> +{
> + if (!ia)
> + return SBI_EINVAL;
> +
> + if (i2c_adapter_find(ia->id))
> + return SBI_EALREADY;
> +
> + sbi_list_add(&(ia->node), &(i2c_adapters_list));
> +
> + return 0;
> +}
> +
> +void i2c_adapter_remove(struct i2c_adapter *ia)
> +{
> + if (!ia)
> + return;
> +
> + sbi_list_del(&(ia->node));
> +}
> +
> +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_smbus_write(struct i2c_adapter *ia, uint8_t addr,
> + uint8_t reg, uint8_t *buffer, int len)
> +{
> + if (!ia)
> + return SBI_EINVAL;
> + if (!ia->smbus_write)
> + return SBI_ENOSYS;
> +
> + return ia->smbus_write(ia, addr, reg, buffer, len);
> +}
> +
> +
> +int i2c_adapter_smbus_read(struct i2c_adapter *ia, uint8_t addr,
> + uint8_t reg, uint8_t *buffer, int len)
> +{
> + if (!ia)
> + return SBI_EINVAL;
> + if (!ia->smbus_read)
> + return SBI_ENOSYS;
> +
> + return ia->smbus_read(ia, addr, reg, buffer, len);
> +}
Again, I don't find those helpers really useful, IMO the adapter
should be hidden from the device, see my previous review. But you can
convince me we don't need it, I'm open to hearing what you think :)
Alex
> 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
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH v2 2/5] lib: utils/i2c: Add generic I2C configuration library
2021-10-19 12:04 ` Alexandre Ghiti
@ 2021-10-20 6:41 ` Nikita Shubin
2021-10-20 8:19 ` Alexandre Ghiti
0 siblings, 1 reply; 38+ messages in thread
From: Nikita Shubin @ 2021-10-20 6:41 UTC (permalink / raw)
To: opensbi
Hello Alexandre!
On Tue, 19 Oct 2021 14:04:28 +0200
Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote:
> On Fri, Oct 15, 2021 at 3:20 PM Nikita Shubin
> <nikita.shubin@maquefel.me> 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>
> > ---
> > v1 -> v2:
> > - switch to lists instead of array
> > - rename send to write
> > - use buffers instead of single byte
> > - add single byte read/write
> > ---
> > include/sbi_utils/i2c/i2c.h | 99
> > +++++++++++++++++++++++++++++++++++++ lib/utils/i2c/i2c.c |
> > 85 +++++++++++++++++++++++++++++++ lib/utils/i2c/objects.mk | 10
> > ++++ 3 files changed, 194 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..76cdb66
> > --- /dev/null
> > +++ b/include/sbi_utils/i2c/i2c.h
> > @@ -0,0 +1,99 @@
> > +/*
> > + * 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>
> > +#include <sbi/sbi_list.h>
> > +
> > +/** Representation of a I2C adapter */
> > +struct i2c_adapter {
> > + /** Pointer to I2C driver owning this I2C adapter */
> > + void *driver;
> > +
> > + /** Unique 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 buffer to given address, register
> > + *
> > + * @return 0 on success and negative error code on failure
> > + */
> > + int (*smbus_write)(struct i2c_adapter *ia, uint8_t addr,
> > uint8_t reg,
> > + uint8_t *buffer, int len);
>
> Why did you rename those functions with smbus prefix? And I would use
> size_t for the len argument.
Cause, i think, actually specifying a register to read/write as a part
of transaction is an smbus thing, as i stated in cover letter, raw
read/write i2c don't use repeated start.
And may be size_t indeed.
>
> > +
> > + /**
> > + * Read buffer from given address, register
> > + *
> > + * @return 0 on success and negative error code on failure
> > + */
> > + int (*smbus_read)(struct i2c_adapter *ia, uint8_t addr,
> > uint8_t reg,
> > + uint8_t *buffer, int len);
> > +
> > + /** List */
> > + struct sbi_dlist node;
> > +};
> > +
> > +static inline struct i2c_adapter *to_i2c_adapter(struct sbi_dlist
> > *node) +{
> > + return container_of(node, struct i2c_adapter, node);
> > +}
> > +
> > +/** 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);
> > +
> > +/** Write to device on I2C adapter bus */
> > +int i2c_adapter_smbus_write(struct i2c_adapter *ia, uint8_t addr,
> > + uint8_t reg, uint8_t *buffer, int len);
> > +
> > +/** Read from device on I2C adapter bus */
> > +int i2c_adapter_smbus_read(struct i2c_adapter *ia, uint8_t addr,
> > + uint8_t reg, uint8_t *buffer, int len);
> > +
> > +/** Write single byte from device on I2C adapter bus */
> > +static inline int i2c_adapter_smbus_reg_write(struct i2c_adapter
> > *ia,
> > + uint8_t addr, uint8_t reg, uint8_t
> > val) +{
> > + return i2c_adapter_smbus_write(ia, addr, reg, &val, 1);
> > +}
> > +
> > +/** Read single byte from device on I2C adapter bus */
> > +static inline int i2c_adapter_smbus_reg_read(struct i2c_adapter
> > *ia,
> > + uint8_t addr, uint8_t reg, uint8_t
> > *val) +{
> > + uint8_t buf;
> > + int ret = i2c_adapter_smbus_read(ia, addr, reg, &buf, 1);
> > +
> > + if (ret)
> > + return ret;
> > +
> > + *val = buf;
> > + return 0;
> > +}
>
> Nice to add the helpers, though I'm not sure "reg_read" is the right
> suffix as it may not be quite explicit: what about simply read_byte?
I think byte_read/byte_write should be "raw" and omit the
uint8_t reg and have naming like:
i2c_adapter_byte_read
i2c_adapter_byte_write
Without smbus prefix, but that's a bit different story.
>
> > +
> > +#endif
> > diff --git a/lib/utils/i2c/i2c.c b/lib/utils/i2c/i2c.c
> > new file mode 100644
> > index 0000000..d23ac91
> > --- /dev/null
> > +++ b/lib/utils/i2c/i2c.c
> > @@ -0,0 +1,85 @@
> > +/*
> > + * 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>
> > +
> > +static SBI_LIST_HEAD(i2c_adapters_list);
> > +
> > +struct i2c_adapter *i2c_adapter_find(int id)
> > +{
> > + struct sbi_dlist *pos;
> > +
> > + sbi_list_for_each(pos, &(i2c_adapters_list)) {
> > + struct i2c_adapter *adap = to_i2c_adapter(pos);
> > +
> > + if (adap->id == id)
> > + return adap;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +int i2c_adapter_add(struct i2c_adapter *ia)
> > +{
> > + if (!ia)
> > + return SBI_EINVAL;
> > +
> > + if (i2c_adapter_find(ia->id))
> > + return SBI_EALREADY;
> > +
> > + sbi_list_add(&(ia->node), &(i2c_adapters_list));
> > +
> > + return 0;
> > +}
> > +
> > +void i2c_adapter_remove(struct i2c_adapter *ia)
> > +{
> > + if (!ia)
> > + return;
> > +
> > + sbi_list_del(&(ia->node));
> > +}
> > +
> > +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_smbus_write(struct i2c_adapter *ia, uint8_t addr,
> > + uint8_t reg, uint8_t *buffer, int len)
> > +{
> > + if (!ia)
> > + return SBI_EINVAL;
> > + if (!ia->smbus_write)
> > + return SBI_ENOSYS;
> > +
> > + return ia->smbus_write(ia, addr, reg, buffer, len);
> > +}
> > +
> > +
> > +int i2c_adapter_smbus_read(struct i2c_adapter *ia, uint8_t addr,
> > + uint8_t reg, uint8_t *buffer, int len)
> > +{
> > + if (!ia)
> > + return SBI_EINVAL;
> > + if (!ia->smbus_read)
> > + return SBI_ENOSYS;
> > +
> > + return ia->smbus_read(ia, addr, reg, buffer, len);
> > +}
>
> Again, I don't find those helpers really useful, IMO the adapter
> should be hidden from the device, see my previous review. But you can
> convince me we don't need it, I'm open to hearing what you think :)
let's move this to 0 patch disucssion.
>
> Alex
>
>
> > 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
> >
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH v2 2/5] lib: utils/i2c: Add generic I2C configuration library
2021-10-20 6:41 ` Nikita Shubin
@ 2021-10-20 8:19 ` Alexandre Ghiti
0 siblings, 0 replies; 38+ messages in thread
From: Alexandre Ghiti @ 2021-10-20 8:19 UTC (permalink / raw)
To: opensbi
On Wed, Oct 20, 2021 at 8:41 AM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> Hello Alexandre!
>
> On Tue, 19 Oct 2021 14:04:28 +0200
> Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote:
>
> > On Fri, Oct 15, 2021 at 3:20 PM Nikita Shubin
> > <nikita.shubin@maquefel.me> 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>
> > > ---
> > > v1 -> v2:
> > > - switch to lists instead of array
> > > - rename send to write
> > > - use buffers instead of single byte
> > > - add single byte read/write
> > > ---
> > > include/sbi_utils/i2c/i2c.h | 99
> > > +++++++++++++++++++++++++++++++++++++ lib/utils/i2c/i2c.c |
> > > 85 +++++++++++++++++++++++++++++++ lib/utils/i2c/objects.mk | 10
> > > ++++ 3 files changed, 194 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..76cdb66
> > > --- /dev/null
> > > +++ b/include/sbi_utils/i2c/i2c.h
> > > @@ -0,0 +1,99 @@
> > > +/*
> > > + * 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>
> > > +#include <sbi/sbi_list.h>
> > > +
> > > +/** Representation of a I2C adapter */
> > > +struct i2c_adapter {
> > > + /** Pointer to I2C driver owning this I2C adapter */
> > > + void *driver;
> > > +
> > > + /** Unique 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 buffer to given address, register
> > > + *
> > > + * @return 0 on success and negative error code on failure
> > > + */
> > > + int (*smbus_write)(struct i2c_adapter *ia, uint8_t addr,
> > > uint8_t reg,
> > > + uint8_t *buffer, int len);
> >
> > Why did you rename those functions with smbus prefix? And I would use
> > size_t for the len argument.
>
> Cause, i think, actually specifying a register to read/write as a part
> of transaction is an smbus thing, as i stated in cover letter, raw
> read/write i2c don't use repeated start.
Ok!
>
> And may be size_t indeed.
>
> >
> > > +
> > > + /**
> > > + * Read buffer from given address, register
> > > + *
> > > + * @return 0 on success and negative error code on failure
> > > + */
> > > + int (*smbus_read)(struct i2c_adapter *ia, uint8_t addr,
> > > uint8_t reg,
> > > + uint8_t *buffer, int len);
> > > +
> > > + /** List */
> > > + struct sbi_dlist node;
> > > +};
> > > +
> > > +static inline struct i2c_adapter *to_i2c_adapter(struct sbi_dlist
> > > *node) +{
> > > + return container_of(node, struct i2c_adapter, node);
> > > +}
> > > +
> > > +/** 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);
> > > +
> > > +/** Write to device on I2C adapter bus */
> > > +int i2c_adapter_smbus_write(struct i2c_adapter *ia, uint8_t addr,
> > > + uint8_t reg, uint8_t *buffer, int len);
> > > +
> > > +/** Read from device on I2C adapter bus */
> > > +int i2c_adapter_smbus_read(struct i2c_adapter *ia, uint8_t addr,
> > > + uint8_t reg, uint8_t *buffer, int len);
> > > +
> > > +/** Write single byte from device on I2C adapter bus */
> > > +static inline int i2c_adapter_smbus_reg_write(struct i2c_adapter
> > > *ia,
> > > + uint8_t addr, uint8_t reg, uint8_t
> > > val) +{
> > > + return i2c_adapter_smbus_write(ia, addr, reg, &val, 1);
> > > +}
> > > +
> > > +/** Read single byte from device on I2C adapter bus */
> > > +static inline int i2c_adapter_smbus_reg_read(struct i2c_adapter
> > > *ia,
> > > + uint8_t addr, uint8_t reg, uint8_t
> > > *val) +{
> > > + uint8_t buf;
> > > + int ret = i2c_adapter_smbus_read(ia, addr, reg, &buf, 1);
> > > +
> > > + if (ret)
> > > + return ret;
> > > +
> > > + *val = buf;
> > > + return 0;
> > > +}
> >
> > Nice to add the helpers, though I'm not sure "reg_read" is the right
> > suffix as it may not be quite explicit: what about simply read_byte?
>
> I think byte_read/byte_write should be "raw" and omit the
> uint8_t reg and have naming like:
>
> i2c_adapter_byte_read
> i2c_adapter_byte_write
>
> Without smbus prefix, but that's a bit different story.
As I'm far from being an i2c/smbus expert, are we sure the value of a
register always holds in an uint8_t?
>
>
> >
> > > +
> > > +#endif
> > > diff --git a/lib/utils/i2c/i2c.c b/lib/utils/i2c/i2c.c
> > > new file mode 100644
> > > index 0000000..d23ac91
> > > --- /dev/null
> > > +++ b/lib/utils/i2c/i2c.c
> > > @@ -0,0 +1,85 @@
> > > +/*
> > > + * 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>
> > > +
> > > +static SBI_LIST_HEAD(i2c_adapters_list);
> > > +
> > > +struct i2c_adapter *i2c_adapter_find(int id)
> > > +{
> > > + struct sbi_dlist *pos;
> > > +
> > > + sbi_list_for_each(pos, &(i2c_adapters_list)) {
> > > + struct i2c_adapter *adap = to_i2c_adapter(pos);
> > > +
> > > + if (adap->id == id)
> > > + return adap;
> > > + }
> > > +
> > > + return NULL;
> > > +}
> > > +
> > > +int i2c_adapter_add(struct i2c_adapter *ia)
> > > +{
> > > + if (!ia)
> > > + return SBI_EINVAL;
> > > +
> > > + if (i2c_adapter_find(ia->id))
> > > + return SBI_EALREADY;
> > > +
> > > + sbi_list_add(&(ia->node), &(i2c_adapters_list));
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +void i2c_adapter_remove(struct i2c_adapter *ia)
> > > +{
> > > + if (!ia)
> > > + return;
> > > +
> > > + sbi_list_del(&(ia->node));
> > > +}
> > > +
> > > +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_smbus_write(struct i2c_adapter *ia, uint8_t addr,
> > > + uint8_t reg, uint8_t *buffer, int len)
> > > +{
> > > + if (!ia)
> > > + return SBI_EINVAL;
> > > + if (!ia->smbus_write)
> > > + return SBI_ENOSYS;
> > > +
> > > + return ia->smbus_write(ia, addr, reg, buffer, len);
> > > +}
> > > +
> > > +
> > > +int i2c_adapter_smbus_read(struct i2c_adapter *ia, uint8_t addr,
> > > + uint8_t reg, uint8_t *buffer, int len)
> > > +{
> > > + if (!ia)
> > > + return SBI_EINVAL;
> > > + if (!ia->smbus_read)
> > > + return SBI_ENOSYS;
> > > +
> > > + return ia->smbus_read(ia, addr, reg, buffer, len);
> > > +}
> >
> > Again, I don't find those helpers really useful, IMO the adapter
> > should be hidden from the device, see my previous review. But you can
> > convince me we don't need it, I'm open to hearing what you think :)
>
> let's move this to 0 patch disucssion.
>
> >
> > Alex
> >
> >
> > > 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
> > >
> > >
> > > --
> > > opensbi mailing list
> > > opensbi at lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/opensbi
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 3/5] lib: utils/i2c: Add simple FDT based I2C framework
2021-10-15 13:19 [PATCH v2 0/5] I2C framework, reboot Unmatched via PMIC Nikita Shubin
2021-10-15 13:19 ` [PATCH v2 1/5] lib: utils/reset: add priority to gpio reset Nikita Shubin
2021-10-15 13:19 ` [PATCH v2 2/5] lib: utils/i2c: Add generic I2C configuration library Nikita Shubin
@ 2021-10-15 13:19 ` Nikita Shubin
2021-10-19 12:05 ` Alexandre Ghiti
2021-10-15 13:19 ` [PATCH v2 4/5] lib: utils/i2c: Add minimal SiFive I2C driver Nikita Shubin
` (4 subsequent siblings)
7 siblings, 1 reply; 38+ messages in thread
From: Nikita Shubin @ 2021-10-15 13:19 UTC (permalink / raw)
To: opensbi
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 | 108 ++++++++++++++++++++++++++++++++
lib/utils/i2c/objects.mk | 1 +
3 files changed, 135 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..fce6d73
--- /dev/null
+++ b/lib/utils/i2c/fdt_i2c.c
@@ -0,0 +1,108 @@
+/*
+ * 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
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v2 3/5] lib: utils/i2c: Add simple FDT based I2C framework
2021-10-15 13:19 ` [PATCH v2 3/5] lib: utils/i2c: Add simple FDT based I2C framework Nikita Shubin
@ 2021-10-19 12:05 ` Alexandre Ghiti
2021-10-20 6:49 ` Nikita Shubin
0 siblings, 1 reply; 38+ messages in thread
From: Alexandre Ghiti @ 2021-10-19 12:05 UTC (permalink / raw)
To: opensbi
On Fri, Oct 15, 2021 at 3:20 PM Nikita Shubin <nikita.shubin@maquefel.me> 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 | 108 ++++++++++++++++++++++++++++++++
> lib/utils/i2c/objects.mk | 1 +
> 3 files changed, 135 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..fce6d73
> --- /dev/null
> +++ b/lib/utils/i2c/fdt_i2c.c
> @@ -0,0 +1,108 @@
> +/*
> + * 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;
> +}
I don't see the point of this function since it simply returns
adapter->driver right?
> +
> +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
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH v2 3/5] lib: utils/i2c: Add simple FDT based I2C framework
2021-10-19 12:05 ` Alexandre Ghiti
@ 2021-10-20 6:49 ` Nikita Shubin
0 siblings, 0 replies; 38+ messages in thread
From: Nikita Shubin @ 2021-10-20 6:49 UTC (permalink / raw)
To: opensbi
Hello Alexandre!
On Tue, 19 Oct 2021 14:05:58 +0200
Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote:
> On Fri, Oct 15, 2021 at 3:20 PM Nikita Shubin
> <nikita.shubin@maquefel.me> 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 | 108
> > ++++++++++++++++++++++++++++++++ lib/utils/i2c/objects.mk |
> > 1 + 3 files changed, 135 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..fce6d73
> > --- /dev/null
> > +++ b/lib/utils/i2c/fdt_i2c.c
> > @@ -0,0 +1,108 @@
> > +/*
> > + * 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;
> > +}
>
> I don't see the point of this function since it simply returns
> adapter->driver right?
It looks like you are right. This can be ommited as well as
fdt_i2c_adapter_driver check in fdt_i2c_adapter_get.
>
> > +
> > +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
> >
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 4/5] lib: utils/i2c: Add minimal SiFive I2C driver
2021-10-15 13:19 [PATCH v2 0/5] I2C framework, reboot Unmatched via PMIC Nikita Shubin
` (2 preceding siblings ...)
2021-10-15 13:19 ` [PATCH v2 3/5] lib: utils/i2c: Add simple FDT based I2C framework Nikita Shubin
@ 2021-10-15 13:19 ` Nikita Shubin
2021-10-19 12:34 ` Alexandre Ghiti
2021-10-15 13:19 ` [PATCH v2 5/5] lib: utils/reset: Add generic da9063 reset driver Nikita Shubin
` (3 subsequent siblings)
7 siblings, 1 reply; 38+ messages in thread
From: Nikita Shubin @ 2021-10-15 13:19 UTC (permalink / raw)
To: opensbi
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>
---
v1 -> v2:
- setreg/getreg add sifive_i2c_ prefix
- use sbi_timer_mdelay for delay
- renamed send to write
- reworked read/write to support buffers
- renamed read/write to smbus_read/smbus_write
---
lib/utils/i2c/fdt_i2c.c | 3 +
lib/utils/i2c/fdt_i2c_sifive.c | 271 +++++++++++++++++++++++++++++++++
lib/utils/i2c/objects.mk | 1 +
3 files changed, 275 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 fce6d73..bbf9df6 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
diff --git a/lib/utils/i2c/fdt_i2c_sifive.c b/lib/utils/i2c/fdt_i2c_sifive.c
new file mode 100644
index 0000000..aeb67ee
--- /dev/null
+++ b/lib/utils/i2c/fdt_i2c_sifive.c
@@ -0,0 +1,271 @@
+/*
+ * 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/sbi_timer.h>
+#include <sbi_utils/fdt/fdt_helper.h>
+#include <sbi_utils/i2c/fdt_i2c.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 sifive_i2c_setreg(struct sifive_i2c_adapter *adap,
+ int reg, u8 value)
+{
+ writel(value, (volatile void *)adap->addr + reg);
+}
+
+static inline u8 sifive_i2c_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 = sifive_i2c_getreg(adap, SIFIVE_I2C_SR);
+
+ if (val & SIFIVE_I2C_STATUS_RXACK)
+ return SBI_EIO;
+
+ return 0;
+}
+
+static int sifive_i2c_adapter_poll(struct sifive_i2c_adapter *adap, uint32_t mask)
+{
+ int max_retry = 5;
+ uint8_t val;
+
+ do {
+ val = sifive_i2c_getreg(adap, SIFIVE_I2C_SR);
+ sbi_timer_mdelay(100);
+ } 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;
+
+ sifive_i2c_setreg(adap, SIFIVE_I2C_TXR, val);
+ val = SIFIVE_I2C_CMD_STA | SIFIVE_I2C_CMD_WR | SIFIVE_I2C_CMD_IACK;
+ sifive_i2c_setreg(adap, SIFIVE_I2C_CR, val);
+
+ return sifive_i2c_adapter_poll_tip(adap);
+}
+
+static int sifive_i2c_adapter_smbus_write(struct i2c_adapter *ia,
+ uint8_t addr, uint8_t reg,
+ uint8_t *buffer, int len)
+{
+ struct sifive_i2c_adapter *adap =
+ container_of(ia, struct sifive_i2c_adapter, adapter);
+ 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 */
+ sifive_i2c_setreg(adap, SIFIVE_I2C_TXR, reg);
+ sifive_i2c_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 */
+ while (len) {
+ sifive_i2c_setreg(adap, SIFIVE_I2C_TXR, *buffer);
+ sifive_i2c_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;
+
+ buffer++;
+ len--;
+ }
+
+ sifive_i2c_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;
+
+ sifive_i2c_setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_IACK);
+
+ return 0;
+}
+
+static int sifive_i2c_adapter_smbus_read(struct i2c_adapter *ia,
+ uint8_t addr, uint8_t reg,
+ uint8_t *buffer, int len)
+{
+ struct sifive_i2c_adapter *adap =
+ container_of(ia, struct sifive_i2c_adapter, adapter);
+ int rc;
+
+ 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;
+
+ sifive_i2c_setreg(adap, SIFIVE_I2C_TXR, reg);
+ sifive_i2c_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 */
+ rc = sifive_i2c_adapter_start(adap, addr, SIFIVE_I2C_READ_BIT);
+ if (rc)
+ return rc;
+
+ rc = sifive_i2c_adapter_rxack(adap);
+ if (rc)
+ return rc;
+
+ while (len) {
+ if (len == 1)
+ sifive_i2c_setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_ACK | SIFIVE_I2C_CMD_RD | SIFIVE_I2C_CMD_IACK);
+ else
+ sifive_i2c_setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_RD | SIFIVE_I2C_CMD_IACK);
+
+ rc = sifive_i2c_adapter_poll_tip(adap);
+ if (rc)
+ return rc;
+
+ *buffer = sifive_i2c_getreg(adap, SIFIVE_I2C_RXR);
+ buffer++;
+ len--;
+ }
+
+ sifive_i2c_setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_STO | SIFIVE_I2C_CMD_IACK);
+ rc = sifive_i2c_adapter_poll_busy(adap);
+ if (rc)
+ return rc;
+
+ sifive_i2c_setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_IACK);
+
+ 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 */
+ sifive_i2c_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.smbus_write = sifive_i2c_adapter_smbus_write;
+ adapter->adapter.smbus_read = sifive_i2c_adapter_smbus_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
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v2 4/5] lib: utils/i2c: Add minimal SiFive I2C driver
2021-10-15 13:19 ` [PATCH v2 4/5] lib: utils/i2c: Add minimal SiFive I2C driver Nikita Shubin
@ 2021-10-19 12:34 ` Alexandre Ghiti
2021-10-20 6:51 ` Nikita Shubin
0 siblings, 1 reply; 38+ messages in thread
From: Alexandre Ghiti @ 2021-10-19 12:34 UTC (permalink / raw)
To: opensbi
On Fri, Oct 15, 2021 at 3:20 PM Nikita Shubin <nikita.shubin@maquefel.me> 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>
> ---
> v1 -> v2:
> - setreg/getreg add sifive_i2c_ prefix
> - use sbi_timer_mdelay for delay
> - renamed send to write
> - reworked read/write to support buffers
> - renamed read/write to smbus_read/smbus_write
> ---
> lib/utils/i2c/fdt_i2c.c | 3 +
> lib/utils/i2c/fdt_i2c_sifive.c | 271 +++++++++++++++++++++++++++++++++
> lib/utils/i2c/objects.mk | 1 +
> 3 files changed, 275 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 fce6d73..bbf9df6 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
> diff --git a/lib/utils/i2c/fdt_i2c_sifive.c b/lib/utils/i2c/fdt_i2c_sifive.c
> new file mode 100644
> index 0000000..aeb67ee
> --- /dev/null
> +++ b/lib/utils/i2c/fdt_i2c_sifive.c
> @@ -0,0 +1,271 @@
> +/*
> + * 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/sbi_timer.h>
> +#include <sbi_utils/fdt/fdt_helper.h>
> +#include <sbi_utils/i2c/fdt_i2c.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 sifive_i2c_setreg(struct sifive_i2c_adapter *adap,
> + int reg, u8 value)
> +{
> + writel(value, (volatile void *)adap->addr + reg);
> +}
> +
> +static inline u8 sifive_i2c_getreg(struct sifive_i2c_adapter *adap, int reg)
> +{
> + return readl((volatile void *)adap->addr + reg);
> +}
> +
reg is defined as a uint8_t in calling functions and here as an int.
> +static int sifive_i2c_adapter_rxack(struct sifive_i2c_adapter *adap)
> +{
> + uint8_t val = sifive_i2c_getreg(adap, SIFIVE_I2C_SR);
> +
> + if (val & SIFIVE_I2C_STATUS_RXACK)
> + return SBI_EIO;
> +
> + return 0;
> +}
> +
> +static int sifive_i2c_adapter_poll(struct sifive_i2c_adapter *adap, uint32_t mask)
> +{
> + int max_retry = 5;
> + uint8_t val;
> +
> + do {
> + val = sifive_i2c_getreg(adap, SIFIVE_I2C_SR);
> + sbi_timer_mdelay(100);
> + } 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;
> +
> + sifive_i2c_setreg(adap, SIFIVE_I2C_TXR, val);
> + val = SIFIVE_I2C_CMD_STA | SIFIVE_I2C_CMD_WR | SIFIVE_I2C_CMD_IACK;
> + sifive_i2c_setreg(adap, SIFIVE_I2C_CR, val);
> +
> + return sifive_i2c_adapter_poll_tip(adap);
> +}
> +
> +static int sifive_i2c_adapter_smbus_write(struct i2c_adapter *ia,
> + uint8_t addr, uint8_t reg,
> + uint8_t *buffer, int len)
> +{
> + struct sifive_i2c_adapter *adap =
> + container_of(ia, struct sifive_i2c_adapter, adapter);
> + 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 */
> + sifive_i2c_setreg(adap, SIFIVE_I2C_TXR, reg);
> + sifive_i2c_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 */
> + while (len) {
> + sifive_i2c_setreg(adap, SIFIVE_I2C_TXR, *buffer);
> + sifive_i2c_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;
> +
> + buffer++;
> + len--;
> + }
> +
> + sifive_i2c_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;
> +
> + sifive_i2c_setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_IACK);
> +
> + return 0;
> +}
> +
> +static int sifive_i2c_adapter_smbus_read(struct i2c_adapter *ia,
> + uint8_t addr, uint8_t reg,
> + uint8_t *buffer, int len)
> +{
> + struct sifive_i2c_adapter *adap =
> + container_of(ia, struct sifive_i2c_adapter, adapter);
> + int rc;
> +
> + 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;
> +
> + sifive_i2c_setreg(adap, SIFIVE_I2C_TXR, reg);
> + sifive_i2c_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 */
> + rc = sifive_i2c_adapter_start(adap, addr, SIFIVE_I2C_READ_BIT);
> + if (rc)
> + return rc;
> +
> + rc = sifive_i2c_adapter_rxack(adap);
> + if (rc)
> + return rc;
> +
> + while (len) {
> + if (len == 1)
> + sifive_i2c_setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_ACK | SIFIVE_I2C_CMD_RD | SIFIVE_I2C_CMD_IACK);
> + else
> + sifive_i2c_setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_RD | SIFIVE_I2C_CMD_IACK);
> +
> + rc = sifive_i2c_adapter_poll_tip(adap);
> + if (rc)
> + return rc;
> +
> + *buffer = sifive_i2c_getreg(adap, SIFIVE_I2C_RXR);
> + buffer++;
> + len--;
> + }
> +
> + sifive_i2c_setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_STO | SIFIVE_I2C_CMD_IACK);
> + rc = sifive_i2c_adapter_poll_busy(adap);
> + if (rc)
> + return rc;
> +
> + sifive_i2c_setreg(adap, SIFIVE_I2C_CR, SIFIVE_I2C_CMD_IACK);
> +
> + 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 */
> + sifive_i2c_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.smbus_write = sifive_i2c_adapter_smbus_write;
> + adapter->adapter.smbus_read = sifive_i2c_adapter_smbus_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
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH v2 4/5] lib: utils/i2c: Add minimal SiFive I2C driver
2021-10-19 12:34 ` Alexandre Ghiti
@ 2021-10-20 6:51 ` Nikita Shubin
0 siblings, 0 replies; 38+ messages in thread
From: Nikita Shubin @ 2021-10-20 6:51 UTC (permalink / raw)
To: opensbi
Hello Alexandre!
On Tue, 19 Oct 2021 14:34:30 +0200
Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote:
> > +static inline void sifive_i2c_setreg(struct sifive_i2c_adapter
> > *adap,
> > + int reg, u8 value)
> > +{
> > + writel(value, (volatile void *)adap->addr + reg);
> > +}
> > +
> > +static inline u8 sifive_i2c_getreg(struct sifive_i2c_adapter
> > *adap, int reg) +{
> > + return readl((volatile void *)adap->addr + reg);
> > +}
> > +
>
> reg is defined as a uint8_t in calling functions and here as an int.
Agreed.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 5/5] lib: utils/reset: Add generic da9063 reset driver
2021-10-15 13:19 [PATCH v2 0/5] I2C framework, reboot Unmatched via PMIC Nikita Shubin
` (3 preceding siblings ...)
2021-10-15 13:19 ` [PATCH v2 4/5] lib: utils/i2c: Add minimal SiFive I2C driver Nikita Shubin
@ 2021-10-15 13:19 ` Nikita Shubin
2021-10-19 12:39 ` Alexandre Ghiti
2021-10-26 8:04 ` Heinrich Schuchardt
2021-10-15 13:44 ` [PATCH v2 0/5] I2C framework, reboot Unmatched via PMIC Jessica Clarke
` (2 subsequent siblings)
7 siblings, 2 replies; 38+ messages in thread
From: Nikita Shubin @ 2021-10-15 13:19 UTC (permalink / raw)
To: opensbi
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 SHUTDOWN 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>
---
v1 -> v2:
- changed to sub dts node da9063-reset instead of relying on dlg,da9063
- added priority dts node
- return priority system_reset_check either readed from dts or default 1
---
lib/utils/reset/fdt_reset.c | 2 +
lib/utils/reset/fdt_reset_da9063.c | 214 +++++++++++++++++++++++++++++
lib/utils/reset/objects.mk | 1 +
3 files changed, 217 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..e3c9ced
--- /dev/null
+++ b/lib/utils/reset/fdt_reset_da9063.c
@@ -0,0 +1,214 @@
+/*
+ * 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;
+ u8 priority;
+} da9063 = {
+ .priority = 1
+};
+
+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 da9063.priority;
+ }
+
+ return 0;
+}
+
+static inline int da9063_sanity_check(struct i2c_adapter *adap, uint32_t reg)
+{
+ uint8_t val;
+ int rc = i2c_adapter_smbus_reg_write(adap, reg, DA9063_REG_PAGE_CON, 0x02);
+
+ if (rc)
+ return rc;
+
+ /* check set page*/
+ rc = i2c_adapter_smbus_reg_read(adap, reg, 0x0, &val);
+ if (rc)
+ return rc;
+
+ if (val != 0x02)
+ return SBI_ENODEV;
+
+ /* read and check device id */
+ rc = i2c_adapter_smbus_reg_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_smbus_reg_write(adap, da9063.reg,
+ DA9063_REG_PAGE_CON, 0x00);
+
+ if (rc)
+ return rc;
+
+ return i2c_adapter_smbus_reg_write(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_smbus_reg_write(adap, da9063.reg,
+ DA9063_REG_PAGE_CON, 0x00);
+
+ if (rc)
+ return rc;
+
+ rc = i2c_adapter_smbus_reg_write(adap, da9063.reg,
+ DA9063_REG_CONTROL_F, DA9063_CONTROL_F_WAKEUP);
+ if (rc)
+ return rc;
+
+ return i2c_adapter_smbus_reg_write(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_dev, i2c_bus, len;
+ const fdt32_t *val;
+ struct i2c_adapter *adapter;
+ uint64_t addr;
+
+ /* find dlg,da9063 parent node */
+ i2c_dev = fdt_parent_offset(fdt, nodeoff);
+ if (i2c_dev < 0)
+ return i2c_dev;
+
+ rc = fdt_get_node_addr_size(fdt, i2c_dev, 0, &addr, NULL);
+ if (rc)
+ return rc;
+
+ da9063.reg = addr;
+ val = fdt_getprop(fdt, nodeoff, "priority", &len);
+ if (len > 0)
+ da9063.priority = fdt32_to_cpu(*val);
+
+ /* find i2c bus parent node */
+ i2c_bus = fdt_parent_offset(fdt, i2c_dev);
+ 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;
+
+ sbi_system_reset_add_device(&da9063_reset_i2c);
+
+ return 0;
+}
+
+static const struct fdt_match da9063_reset_match[] = {
+ { .compatible = "da9063-reset", .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
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v2 5/5] lib: utils/reset: Add generic da9063 reset driver
2021-10-15 13:19 ` [PATCH v2 5/5] lib: utils/reset: Add generic da9063 reset driver Nikita Shubin
@ 2021-10-19 12:39 ` Alexandre Ghiti
2021-10-20 7:00 ` Nikita Shubin
2021-10-26 8:04 ` Heinrich Schuchardt
1 sibling, 1 reply; 38+ messages in thread
From: Alexandre Ghiti @ 2021-10-19 12:39 UTC (permalink / raw)
To: opensbi
On Fri, Oct 15, 2021 at 3:20 PM Nikita Shubin <nikita.shubin@maquefel.me> 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 SHUTDOWN 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>
> ---
> v1 -> v2:
> - changed to sub dts node da9063-reset instead of relying on dlg,da9063
> - added priority dts node
> - return priority system_reset_check either readed from dts or default 1
> ---
> lib/utils/reset/fdt_reset.c | 2 +
> lib/utils/reset/fdt_reset_da9063.c | 214 +++++++++++++++++++++++++++++
> lib/utils/reset/objects.mk | 1 +
> 3 files changed, 217 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..e3c9ced
> --- /dev/null
> +++ b/lib/utils/reset/fdt_reset_da9063.c
> @@ -0,0 +1,214 @@
> +/*
> + * 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;
> + u8 priority;
> +} da9063 = {
> + .priority = 1
> +};
> +
> +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 da9063.priority;
> + }
> +
> + return 0;
> +}
> +
> +static inline int da9063_sanity_check(struct i2c_adapter *adap, uint32_t reg)
> +{
> + uint8_t val;
> + int rc = i2c_adapter_smbus_reg_write(adap, reg, DA9063_REG_PAGE_CON, 0x02);
> +
> + if (rc)
> + return rc;
> +
> + /* check set page*/
> + rc = i2c_adapter_smbus_reg_read(adap, reg, 0x0, &val);
> + if (rc)
> + return rc;
> +
> + if (val != 0x02)
> + return SBI_ENODEV;
> +
> + /* read and check device id */
> + rc = i2c_adapter_smbus_reg_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_smbus_reg_write(adap, da9063.reg,
> + DA9063_REG_PAGE_CON, 0x00);
> +
> + if (rc)
> + return rc;
> +
> + return i2c_adapter_smbus_reg_write(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_smbus_reg_write(adap, da9063.reg,
> + DA9063_REG_PAGE_CON, 0x00);
> +
> + if (rc)
> + return rc;
> +
> + rc = i2c_adapter_smbus_reg_write(adap, da9063.reg,
> + DA9063_REG_CONTROL_F, DA9063_CONTROL_F_WAKEUP);
> + if (rc)
> + return rc;
> +
> + return i2c_adapter_smbus_reg_write(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_dev, i2c_bus, len;
> + const fdt32_t *val;
> + struct i2c_adapter *adapter;
> + uint64_t addr;
> +
> + /* find dlg,da9063 parent node */
> + i2c_dev = fdt_parent_offset(fdt, nodeoff);
> + if (i2c_dev < 0)
> + return i2c_dev;
> +
> + rc = fdt_get_node_addr_size(fdt, i2c_dev, 0, &addr, NULL);
> + if (rc)
> + return rc;
> +
> + da9063.reg = addr;
> + val = fdt_getprop(fdt, nodeoff, "priority", &len);
> + if (len > 0)
> + da9063.priority = fdt32_to_cpu(*val);
> +
> + /* find i2c bus parent node */
> + i2c_bus = fdt_parent_offset(fdt, i2c_dev);
> + 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;
> +
> + sbi_system_reset_add_device(&da9063_reset_i2c);
> +
> + return 0;
> +}
> +
> +static const struct fdt_match da9063_reset_match[] = {
> + { .compatible = "da9063-reset", .data = (void *)TRUE },
> + { },
> +};
Agree with Jessica here, as per Adam, the reset as we implement it
here should remain a board specific thing, so we should not rely on
this device tree node. I was hoping to make something that would work
in general for da9063 users, but no luck.
And finally I think we should listen to Adam and go for a full reset
using the RTC, instead of the partial reset we implemented here. We
sacrifice the RTC but anyway, it was not even exposed in the device
tree, we should add a comment there so that people don't get surprised
if they enable it. I will post the right sequence adapted to your
sources soon.
Thanks,
Alex
> +
> +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
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH v2 5/5] lib: utils/reset: Add generic da9063 reset driver
2021-10-19 12:39 ` Alexandre Ghiti
@ 2021-10-20 7:00 ` Nikita Shubin
2021-10-20 8:33 ` Alexandre Ghiti
0 siblings, 1 reply; 38+ messages in thread
From: Nikita Shubin @ 2021-10-20 7:00 UTC (permalink / raw)
To: opensbi
Hello Alexandre!
On Tue, 19 Oct 2021 14:39:16 +0200
Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote:
> > +
> > +static const struct fdt_match da9063_reset_match[] = {
> > + { .compatible = "da9063-reset", .data = (void *)TRUE },
> > + { },
> > +};
>
> Agree with Jessica here, as per Adam, the reset as we implement it
> here should remain a board specific thing, so we should not rely on
> this device tree node. I was hoping to make something that would work
> in general for da9063 users, but no luck.
>
> And finally I think we should listen to Adam and go for a full reset
> using the RTC, instead of the partial reset we implemented here. We
> sacrifice the RTC but anyway, it was not even exposed in the device
> tree, we should add a comment there so that people don't get surprised
> if they enable it. I will post the right sequence adapted to your
> sources soon.
Well the RTC is much more valuable than reset actually and used for
many tests.
Indeed da9063 reset can be moved totally to "platform" code, but do you
think that leaving users with hard-coded reset cutting them off from RTC
and no other possibility, other than fixing OpenSBI code in-place is
really a good option ?
With DTS approuch it can be as simple as removing the da9063-reset from
DTS file.
>
> Thanks,
>
> Alex
>
>
> > +
> > +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
> >
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH v2 5/5] lib: utils/reset: Add generic da9063 reset driver
2021-10-20 7:00 ` Nikita Shubin
@ 2021-10-20 8:33 ` Alexandre Ghiti
2021-10-20 9:00 ` Nikita Shubin
0 siblings, 1 reply; 38+ messages in thread
From: Alexandre Ghiti @ 2021-10-20 8:33 UTC (permalink / raw)
To: opensbi
On Wed, Oct 20, 2021 at 9:00 AM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> Hello Alexandre!
>
> On Tue, 19 Oct 2021 14:39:16 +0200
> Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote:
>
> > > +
> > > +static const struct fdt_match da9063_reset_match[] = {
> > > + { .compatible = "da9063-reset", .data = (void *)TRUE },
> > > + { },
> > > +};
> >
> > Agree with Jessica here, as per Adam, the reset as we implement it
> > here should remain a board specific thing, so we should not rely on
> > this device tree node. I was hoping to make something that would work
> > in general for da9063 users, but no luck.
> >
> > And finally I think we should listen to Adam and go for a full reset
> > using the RTC, instead of the partial reset we implemented here. We
> > sacrifice the RTC but anyway, it was not even exposed in the device
> > tree, we should add a comment there so that people don't get surprised
> > if they enable it. I will post the right sequence adapted to your
> > sources soon.
>
> Well the RTC is much more valuable than reset actually and used for
> many tests.
>
> Indeed da9063 reset can be moved totally to "platform" code, but do you
> think that leaving users with hard-coded reset cutting them off from RTC
> and no other possibility, other than fixing OpenSBI code in-place is
> really a good option ?
>
> With DTS approuch it can be as simple as removing the da9063-reset from
> DTS file.
>
Arf, I change my mind every two days regarding this...I'm just trying
to have a reliable reset, and to quote Adam below, the reset as
implemented here is only partial:
"With the sequence you provided this is only a partial reset whereby all of the
output rails are sequenced down then up again and restored to OTP voltages.
However the remainder of the chip settings aren't reset as this isn't a true
reset of the device going through full reload from OTP, so for example settings
of regulator mode GPIO states, or IRQ mask bits would persist on the restart,
which could have implications on system operation."
I imagine the partial reset could have side-effects on the RTC
configuration too. So we might end up with a partial reset and a
broken RTC...
and again from Adam:
"Personally, if it was possible I think the RTC approach would be best as it's a
full reset and to me is far safer with regards to potential side effects"
> >
> > Thanks,
> >
> > Alex
> >
> >
> > > +
> > > +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
> > >
> > >
> > > --
> > > opensbi mailing list
> > > opensbi at lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/opensbi
>
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH v2 5/5] lib: utils/reset: Add generic da9063 reset driver
2021-10-20 8:33 ` Alexandre Ghiti
@ 2021-10-20 9:00 ` Nikita Shubin
2021-10-21 4:40 ` Alexandre Ghiti
0 siblings, 1 reply; 38+ messages in thread
From: Nikita Shubin @ 2021-10-20 9:00 UTC (permalink / raw)
To: opensbi
On Wed, 20 Oct 2021 10:33:26 +0200
Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote:
> On Wed, Oct 20, 2021 at 9:00 AM Nikita Shubin
> <nikita.shubin@maquefel.me> wrote:
> >
> > Hello Alexandre!
> >
> > On Tue, 19 Oct 2021 14:39:16 +0200
> > Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote:
> >
> > > > +
> > > > +static const struct fdt_match da9063_reset_match[] = {
> > > > + { .compatible = "da9063-reset", .data = (void *)TRUE },
> > > > + { },
> > > > +};
> > >
> > > Agree with Jessica here, as per Adam, the reset as we implement it
> > > here should remain a board specific thing, so we should not rely
> > > on this device tree node. I was hoping to make something that
> > > would work in general for da9063 users, but no luck.
> > >
> > > And finally I think we should listen to Adam and go for a full
> > > reset using the RTC, instead of the partial reset we implemented
> > > here. We sacrifice the RTC but anyway, it was not even exposed in
> > > the device tree, we should add a comment there so that people
> > > don't get surprised if they enable it. I will post the right
> > > sequence adapted to your sources soon.
> >
> > Well the RTC is much more valuable than reset actually and used for
> > many tests.
> >
> > Indeed da9063 reset can be moved totally to "platform" code, but do
> > you think that leaving users with hard-coded reset cutting them off
> > from RTC and no other possibility, other than fixing OpenSBI code
> > in-place is really a good option ?
> >
> > With DTS approuch it can be as simple as removing the da9063-reset
> > from DTS file.
> >
>
> Arf, I change my mind every two days regarding this...I'm just trying
> to have a reliable reset, and to quote Adam below, the reset as
> implemented here is only partial:
>
> "With the sequence you provided this is only a partial reset whereby
> all of the output rails are sequenced down then up again and restored
> to OTP voltages. However the remainder of the chip settings aren't
> reset as this isn't a true reset of the device going through full
> reload from OTP, so for example settings of regulator mode GPIO
> states, or IRQ mask bits would persist on the restart, which could
> have implications on system operation."
Actually i don't think battling over a board which isn't supposed to be
more than a development board and won't be massed used makes any sense.
It's more like QoL for those who are currently porting software or
experimenting with RISC-V.
Before this we used openocd reset, which can be greatly automated with
expect.
>
> I imagine the partial reset could have side-effects on the RTC
> configuration too. So we might end up with a partial reset and a
> broken RTC...
>
Well we should really investigate this.
As well as i don't quite understand this "sacrifice RTC" thing.
> and again from Adam:
>
> "Personally, if it was possible I think the RTC approach would be
> best as it's a full reset and to me is far safer with regards to
> potential side effects"
>
>
> > >
> > > Thanks,
> > >
> > > Alex
> > >
> > >
> > > > +
> > > > +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
> > > >
> > > >
> > > > --
> > > > opensbi mailing list
> > > > opensbi at lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/opensbi
> >
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH v2 5/5] lib: utils/reset: Add generic da9063 reset driver
2021-10-20 9:00 ` Nikita Shubin
@ 2021-10-21 4:40 ` Alexandre Ghiti
0 siblings, 0 replies; 38+ messages in thread
From: Alexandre Ghiti @ 2021-10-21 4:40 UTC (permalink / raw)
To: opensbi
On Wed, Oct 20, 2021 at 11:00 AM Nikita Shubin
<nikita.shubin@maquefel.me> wrote:
>
> On Wed, 20 Oct 2021 10:33:26 +0200
> Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote:
>
> > On Wed, Oct 20, 2021 at 9:00 AM Nikita Shubin
> > <nikita.shubin@maquefel.me> wrote:
> > >
> > > Hello Alexandre!
> > >
> > > On Tue, 19 Oct 2021 14:39:16 +0200
> > > Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote:
> > >
> > > > > +
> > > > > +static const struct fdt_match da9063_reset_match[] = {
> > > > > + { .compatible = "da9063-reset", .data = (void *)TRUE },
> > > > > + { },
> > > > > +};
> > > >
> > > > Agree with Jessica here, as per Adam, the reset as we implement it
> > > > here should remain a board specific thing, so we should not rely
> > > > on this device tree node. I was hoping to make something that
> > > > would work in general for da9063 users, but no luck.
> > > >
> > > > And finally I think we should listen to Adam and go for a full
> > > > reset using the RTC, instead of the partial reset we implemented
> > > > here. We sacrifice the RTC but anyway, it was not even exposed in
> > > > the device tree, we should add a comment there so that people
> > > > don't get surprised if they enable it. I will post the right
> > > > sequence adapted to your sources soon.
> > >
> > > Well the RTC is much more valuable than reset actually and used for
> > > many tests.
> > >
> > > Indeed da9063 reset can be moved totally to "platform" code, but do
> > > you think that leaving users with hard-coded reset cutting them off
> > > from RTC and no other possibility, other than fixing OpenSBI code
> > > in-place is really a good option ?
> > >
> > > With DTS approuch it can be as simple as removing the da9063-reset
> > > from DTS file.
> > >
> >
> > Arf, I change my mind every two days regarding this...I'm just trying
> > to have a reliable reset, and to quote Adam below, the reset as
> > implemented here is only partial:
> >
> > "With the sequence you provided this is only a partial reset whereby
> > all of the output rails are sequenced down then up again and restored
> > to OTP voltages. However the remainder of the chip settings aren't
> > reset as this isn't a true reset of the device going through full
> > reload from OTP, so for example settings of regulator mode GPIO
> > states, or IRQ mask bits would persist on the restart, which could
> > have implications on system operation."
>
> Actually i don't think battling over a board which isn't supposed to be
> more than a development board and won't be massed used makes any sense.
>
> It's more like QoL for those who are currently porting software or
> experimenting with RISC-V.
>
> Before this we used openocd reset, which can be greatly automated with
> expect.
>
> >
> > I imagine the partial reset could have side-effects on the RTC
> > configuration too. So we might end up with a partial reset and a
> > broken RTC...
> >
>
> Well we should really investigate this.
>
> As well as i don't quite understand this "sacrifice RTC" thing.
To use the RTC for the reset, the cleanest solution is to set a
one-shot alarm which could have been already programmed by the user.
There is also the periodic tick which may or not clobber
user-specified registers and is way simpler to implement.
I gave you all the input I had from Dialog, the choice is up to you
now, we spent enough time on this already.
Thanks,
Alex
>
> > and again from Adam:
> >
> > "Personally, if it was possible I think the RTC approach would be
> > best as it's a full reset and to me is far safer with regards to
> > potential side effects"
> >
> >
> > > >
> > > > Thanks,
> > > >
> > > > Alex
> > > >
> > > >
> > > > > +
> > > > > +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
> > > > >
> > > > >
> > > > > --
> > > > > opensbi mailing list
> > > > > opensbi at lists.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/opensbi
> > >
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 5/5] lib: utils/reset: Add generic da9063 reset driver
2021-10-15 13:19 ` [PATCH v2 5/5] lib: utils/reset: Add generic da9063 reset driver Nikita Shubin
2021-10-19 12:39 ` Alexandre Ghiti
@ 2021-10-26 8:04 ` Heinrich Schuchardt
2021-10-26 9:40 ` Heinrich Schuchardt
1 sibling, 1 reply; 38+ messages in thread
From: Heinrich Schuchardt @ 2021-10-26 8:04 UTC (permalink / raw)
To: opensbi
On 10/15/21 15:19, 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 SHUTDOWN 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>
> ---
> v1 -> v2:
> - changed to sub dts node da9063-reset instead of relying on dlg,da9063
> - added priority dts node
> - return priority system_reset_check either readed from dts or default 1
> ---
> lib/utils/reset/fdt_reset.c | 2 +
> lib/utils/reset/fdt_reset_da9063.c | 214 +++++++++++++++++++++++++++++
> lib/utils/reset/objects.mk | 1 +
> 3 files changed, 217 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..e3c9ced
> --- /dev/null
> +++ b/lib/utils/reset/fdt_reset_da9063.c
> @@ -0,0 +1,214 @@
> +/*
> + * 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;
> + u8 priority;
> +} da9063 = {
> + .priority = 1
This value should be 0 until da9063_reset_init() succees. See comments
below.
> +};
> +
> +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 da9063.priority;
> + }
> +
> + return 0;
> +}
> +
> +static inline int da9063_sanity_check(struct i2c_adapter *adap, uint32_t reg)
> +{
> + uint8_t val;
> + int rc = i2c_adapter_smbus_reg_write(adap, reg, DA9063_REG_PAGE_CON, 0x02);
> +
> + if (rc)
> + return rc;
> +
> + /* check set page*/
> + rc = i2c_adapter_smbus_reg_read(adap, reg, 0x0, &val);
> + if (rc)
> + return rc;
> +
> + if (val != 0x02)
> + return SBI_ENODEV;
> +
> + /* read and check device id */
> + rc = i2c_adapter_smbus_reg_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_smbus_reg_write(adap, da9063.reg,
> + DA9063_REG_PAGE_CON, 0x00);
> +
> + if (rc)
> + return rc;
> +
> + return i2c_adapter_smbus_reg_write(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_smbus_reg_write(adap, da9063.reg,
> + DA9063_REG_PAGE_CON, 0x00);
> +
> + if (rc)
> + return rc;
> +
> + rc = i2c_adapter_smbus_reg_write(adap, da9063.reg,
> + DA9063_REG_CONTROL_F, DA9063_CONTROL_F_WAKEUP);
> + if (rc)
> + return rc;
> +
> + return i2c_adapter_smbus_reg_write(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_dev, i2c_bus, len;
> + const fdt32_t *val;
> + struct i2c_adapter *adapter;
> + uint64_t addr;
> +
> + /* find dlg,da9063 parent node */
> + i2c_dev = fdt_parent_offset(fdt, nodeoff);
> + if (i2c_dev < 0)
> + return i2c_dev;
> +
> + rc = fdt_get_node_addr_size(fdt, i2c_dev, 0, &addr, NULL);
> + if (rc)
> + return rc;
If you return anything but -ENODEV or 0 to fdt_reset_init(),
fdt_reset_init will return a non-zero return value and the board will
hang without any console message.
I see the following alternative solutions:
1) Move the initialization of the console before all other devices and
provide error messages.
2) Return -ENODEV if the problem does not require hanging.
3) Let fdt_reset_init() just continue in case of any initialization error.
1) makes sense anyway. But hanging is a bad idea.
2) and 3) require that system_reset_check return 0 if the initialization
failed.
Best regards
Heinrich
> +
> + da9063.reg = addr;
> + val = fdt_getprop(fdt, nodeoff, "priority", &len);
> + if (len > 0)
> + da9063.priority = fdt32_to_cpu(*val);
> +
> + /* find i2c bus parent node */
> + i2c_bus = fdt_parent_offset(fdt, i2c_dev);
> + 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;
> +
> + sbi_system_reset_add_device(&da9063_reset_i2c);
> +
> + return 0;
> +}
> +
> +static const struct fdt_match da9063_reset_match[] = {
> + { .compatible = "da9063-reset", .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
>
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH v2 5/5] lib: utils/reset: Add generic da9063 reset driver
2021-10-26 8:04 ` Heinrich Schuchardt
@ 2021-10-26 9:40 ` Heinrich Schuchardt
2021-10-26 14:17 ` Nikita Shubin
2021-11-03 7:59 ` Nikita Shubin
0 siblings, 2 replies; 38+ messages in thread
From: Heinrich Schuchardt @ 2021-10-26 9:40 UTC (permalink / raw)
To: opensbi
On 10/26/21 10:04, Heinrich Schuchardt wrote:
> On 10/15/21 15:19, 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 SHUTDOWN 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>
Alexandre and I had the same finding for the driver:
Reset works fine after a hard reset (via the reset button) when coming
from Linux.
After a poweroff reset in U-Boot does not work.
Our expectation is that OpenSBI ensures that the SBI system reset
extension works upon entry into the next firmware stage without
requiring any further initialization by that software.
When a reset is requested you should make not presumption about the
state of the system. Simply fully initialize the da9063 state to
guarantee a successful reset.
Best regards
Heinrich
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 5/5] lib: utils/reset: Add generic da9063 reset driver
2021-10-26 9:40 ` Heinrich Schuchardt
@ 2021-10-26 14:17 ` Nikita Shubin
2021-11-03 7:59 ` Nikita Shubin
1 sibling, 0 replies; 38+ messages in thread
From: Nikita Shubin @ 2021-10-26 14:17 UTC (permalink / raw)
To: opensbi
Hello Heinrich.
On Tue, 26 Oct 2021 11:40:13 +0200
Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote:
> On 10/26/21 10:04, Heinrich Schuchardt wrote:
> > On 10/15/21 15:19, 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 SHUTDOWN 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>
>
> Alexandre and I had the same finding for the driver:
>
> Reset works fine after a hard reset (via the reset button) when
> coming from Linux.
>
> After a poweroff reset in U-Boot does not work.
Thank you for testing!
>
> Our expectation is that OpenSBI ensures that the SBI system reset
> extension works upon entry into the next firmware stage without
> requiring any further initialization by that software.
>
> When a reset is requested you should make not presumption about the
> state of the system. Simply fully initialize the da9063 state to
> guarantee a successful reset.
I think you are right and we shouldn't rely on someone already
initialized it for us.
Indeed i should investigate and expiriment some more.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 5/5] lib: utils/reset: Add generic da9063 reset driver
2021-10-26 9:40 ` Heinrich Schuchardt
2021-10-26 14:17 ` Nikita Shubin
@ 2021-11-03 7:59 ` Nikita Shubin
2021-11-03 12:07 ` Heinrich Schuchardt
1 sibling, 1 reply; 38+ messages in thread
From: Nikita Shubin @ 2021-11-03 7:59 UTC (permalink / raw)
To: opensbi
Hello All!
On Tue, 26 Oct 2021 11:40:13 +0200
Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote:
> On 10/26/21 10:04, Heinrich Schuchardt wrote:
> > On 10/15/21 15:19, 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 SHUTDOWN 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>
>
> Alexandre and I had the same finding for the driver:
>
> Reset works fine after a hard reset (via the reset button) when
> coming from Linux.
>
> After a poweroff reset in U-Boot does not work.
>
> Our expectation is that OpenSBI ensures that the SBI system reset
> extension works upon entry into the next firmware stage without
> requiring any further initialization by that software.
>
> When a reset is requested you should make not presumption about the
> state of the system. Simply fully initialize the da9063 state to
> guarantee a successful reset.
Heinrich and Alexandre, i have done a small investigation:
The RTC works fine on Unmatched, just had a small issue with PLIC driver
firing IRQ only once, here is a separate patch from Guo Ren:
https://patchwork.kernel.org/project/linux-riscv/patch/20211101131736.3800114-1-guoren at kernel.org/
And a small patch to enable da9063 as a wakeup source:
https://github.com/YADRO-KNS/linux/commit/37c2a5b2031a42af99dd7125f8c93ab71236d3fa
Indeed after power-cycle with RESET button or POWER button, I2C is
unavailable (not initialised by U-Boot):
First boot reset after powercycle with powerdown/reset button:
=> reset
resetting ...
da9063_system_reset: chip is not da9063 PMIC
After rebooting with Linux reboot:
=> reset
resetting ...
All okay and that's means that it's not a full reset, I2C was
initialised by linux, and not resetted.
However if we arm the clock and go shutdown:
sh -c "echo `date '+%s' -d '+ 1 minutes'` >
/sys/class/rtc/rtc0/wakealarm" shutdown
shutdown
We get:
=> reset
resetting ...
da9063_system_reset: chip is not da9063 PMIC
So "reset" with RTC is indeed a full reset...
All above also means that "sacrificing" RTC means sacrificing user
setted wakeup ALARM's.
May be it's worth splitting reset into WARM_REBOOT, COLD_REBOOT with
WARM reboot is going through method proposed by Alexandre and COLD
reboot via RTC, and explicitly specify that COLD reboot WILL overwrite
ALARM setting ?
What do you think about this ?
>
> Best regards
>
> Heinrich
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 5/5] lib: utils/reset: Add generic da9063 reset driver
2021-11-03 7:59 ` Nikita Shubin
@ 2021-11-03 12:07 ` Heinrich Schuchardt
2021-11-03 12:13 ` Nikita Shubin
0 siblings, 1 reply; 38+ messages in thread
From: Heinrich Schuchardt @ 2021-11-03 12:07 UTC (permalink / raw)
To: opensbi
On 11/3/21 08:59, Nikita Shubin wrote:
> Hello All!
>
> On Tue, 26 Oct 2021 11:40:13 +0200
> Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote:
>
>> On 10/26/21 10:04, Heinrich Schuchardt wrote:
>>> On 10/15/21 15:19, 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 SHUTDOWN 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>
>>
>> Alexandre and I had the same finding for the driver:
>>
>> Reset works fine after a hard reset (via the reset button) when
>> coming from Linux.
>>
>> After a poweroff reset in U-Boot does not work.
>>
>> Our expectation is that OpenSBI ensures that the SBI system reset
>> extension works upon entry into the next firmware stage without
>> requiring any further initialization by that software.
>>
>> When a reset is requested you should make not presumption about the
>> state of the system. Simply fully initialize the da9063 state to
>> guarantee a successful reset.
>
> Heinrich and Alexandre, i have done a small investigation:
>
> The RTC works fine on Unmatched, just had a small issue with PLIC driver
> firing IRQ only once, here is a separate patch from Guo Ren:
>
> https://patchwork.kernel.org/project/linux-riscv/patch/20211101131736.3800114-1-guoren at kernel.org/
>
> And a small patch to enable da9063 as a wakeup source:
>
> https://github.com/YADRO-KNS/linux/commit/37c2a5b2031a42af99dd7125f8c93ab71236d3fa
>
> Indeed after power-cycle with RESET button or POWER button, I2C is
> unavailable (not initialised by U-Boot):
>
> First boot reset after powercycle with powerdown/reset button:
> => reset
> resetting ...
> da9063_system_reset: chip is not da9063 PMIC
>
> After rebooting with Linux reboot:
> => reset
> resetting ...
>
> All okay and that's means that it's not a full reset, I2C was
> initialised by linux, and not resetted.
Doesn't this imply that the I2C initialization code should be added to
OpenSBI?
Best regards
Heinrich
>
> However if we arm the clock and go shutdown:
>
> sh -c "echo `date '+%s' -d '+ 1 minutes'` >
> /sys/class/rtc/rtc0/wakealarm" shutdown
> shutdown
>
> We get:
> => reset
> resetting ...
> da9063_system_reset: chip is not da9063 PMIC
>
> So "reset" with RTC is indeed a full reset...
>
> All above also means that "sacrificing" RTC means sacrificing user
> setted wakeup ALARM's.
>
> May be it's worth splitting reset into WARM_REBOOT, COLD_REBOOT with
> WARM reboot is going through method proposed by Alexandre and COLD
> reboot via RTC, and explicitly specify that COLD reboot WILL overwrite
> ALARM setting ?
>
> What do you think about this ?
>
>>
>> Best regards
>>
>> Heinrich
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 5/5] lib: utils/reset: Add generic da9063 reset driver
2021-11-03 12:07 ` Heinrich Schuchardt
@ 2021-11-03 12:13 ` Nikita Shubin
2021-11-04 13:06 ` Anup Patel
0 siblings, 1 reply; 38+ messages in thread
From: Nikita Shubin @ 2021-11-03 12:13 UTC (permalink / raw)
To: opensbi
On Wed, 3 Nov 2021 13:07:29 +0100
Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote:
> > After rebooting with Linux reboot:
> > => reset
> > resetting ...
> >
> > All okay and that's means that it's not a full reset, I2C was
> > initialised by linux, and not resetted.
>
> Doesn't this imply that the I2C initialization code should be added
> to OpenSBI?
>
Without any doubt.
> Best regards
>
> Heinrich
>
> >
> > However if we arm the clock and go shutdown:
> >
> > sh -c "echo `date '+%s' -d '+ 1 minutes'` >
> > /sys/class/rtc/rtc0/wakealarm" shutdown
> > shutdown
> >
> > We get:
> > => reset
> > resetting ...
> > da9063_system_reset: chip is not da9063 PMIC
> >
> > So "reset" with RTC is indeed a full reset...
> >
> > All above also means that "sacrificing" RTC means sacrificing user
> > setted wakeup ALARM's.
> >
> > May be it's worth splitting reset into WARM_REBOOT, COLD_REBOOT with
> > WARM reboot is going through method proposed by Alexandre and COLD
> > reboot via RTC, and explicitly specify that COLD reboot WILL
> > overwrite ALARM setting ?
> >
> > What do you think about this ?
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 5/5] lib: utils/reset: Add generic da9063 reset driver
2021-11-03 12:13 ` Nikita Shubin
@ 2021-11-04 13:06 ` Anup Patel
0 siblings, 0 replies; 38+ messages in thread
From: Anup Patel @ 2021-11-04 13:06 UTC (permalink / raw)
To: opensbi
On Wed, Nov 3, 2021 at 5:43 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> On Wed, 3 Nov 2021 13:07:29 +0100
> Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote:
>
> > > After rebooting with Linux reboot:
> > > => reset
> > > resetting ...
> > >
> > > All okay and that's means that it's not a full reset, I2C was
> > > initialised by linux, and not resetted.
> >
> > Doesn't this imply that the I2C initialization code should be added
> > to OpenSBI?
> >
>
> Without any doubt.
I am okay having a simple I2C initialization.
This series will be the last thing for OpenSBI v1.0 release
so please update this series soon.
Regards,
Anup
>
> > Best regards
> >
> > Heinrich
> >
> > >
> > > However if we arm the clock and go shutdown:
> > >
> > > sh -c "echo `date '+%s' -d '+ 1 minutes'` >
> > > /sys/class/rtc/rtc0/wakealarm" shutdown
> > > shutdown
> > >
> > > We get:
> > > => reset
> > > resetting ...
> > > da9063_system_reset: chip is not da9063 PMIC
> > >
> > > So "reset" with RTC is indeed a full reset...
> > >
> > > All above also means that "sacrificing" RTC means sacrificing user
> > > setted wakeup ALARM's.
> > >
> > > May be it's worth splitting reset into WARM_REBOOT, COLD_REBOOT with
> > > WARM reboot is going through method proposed by Alexandre and COLD
> > > reboot via RTC, and explicitly specify that COLD reboot WILL
> > > overwrite ALARM setting ?
> > >
> > > What do you think about this ?
> > >
> > >>
> > >> Best regards
> > >>
> > >> Heinrich
> > >
> >
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 0/5] I2C framework, reboot Unmatched via PMIC
2021-10-15 13:19 [PATCH v2 0/5] I2C framework, reboot Unmatched via PMIC Nikita Shubin
` (4 preceding siblings ...)
2021-10-15 13:19 ` [PATCH v2 5/5] lib: utils/reset: Add generic da9063 reset driver Nikita Shubin
@ 2021-10-15 13:44 ` Jessica Clarke
[not found] ` <1372921634305615@mail.yandex.ru>
2021-10-19 11:57 ` Alexandre Ghiti
2021-10-20 4:59 ` Alexandre Ghiti
7 siblings, 1 reply; 38+ messages in thread
From: Jessica Clarke @ 2021-10-15 13:44 UTC (permalink / raw)
To: opensbi
On 15 Oct 2021, at 14:19, Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> From: Nikita Shubin <n.shubin@yadro.com>
>
> This series introduce rebooting via i2c da9063 PMIC, currently on
> SiFive Unmatched board.
>
> "gpio-poweroff" remains with default priority of 128 - default priority,
> da9063 is 1 by default the least priority.
>
> da9063-reset {
> compatible = "da9063-reset";
> priority = <1>;
> };
I don?t see why priority is needed; the DA9063 is known to always be a
last-resort way to reboot. Just hard-code it as the lowest priority. I
also don?t see why we need a device tree node, just make it a platform
quirk, because this isn?t (and shouldn?t be) a standardised thing?
Definitely don?t want it being exposed to anything other than OpenSBI,
which is what ends up happening if it lives in the device tree...
Jess
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH v2 0/5] I2C framework, reboot Unmatched via PMIC
2021-10-15 13:19 [PATCH v2 0/5] I2C framework, reboot Unmatched via PMIC Nikita Shubin
` (5 preceding siblings ...)
2021-10-15 13:44 ` [PATCH v2 0/5] I2C framework, reboot Unmatched via PMIC Jessica Clarke
@ 2021-10-19 11:57 ` Alexandre Ghiti
2021-10-20 7:26 ` Nikita Shubin
2021-10-20 4:59 ` Alexandre Ghiti
7 siblings, 1 reply; 38+ messages in thread
From: Alexandre Ghiti @ 2021-10-19 11:57 UTC (permalink / raw)
To: opensbi
On Fri, Oct 15, 2021 at 3:20 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> From: Nikita Shubin <n.shubin@yadro.com>
>
> This series introduce rebooting via i2c da9063 PMIC, currently on
> SiFive Unmatched board.
>
> "gpio-poweroff" remains with default priority of 128 - default priority,
> da9063 is 1 by default the least priority.
>
> da9063-reset {
> compatible = "da9063-reset";
> priority = <1>;
> };
>
> Is required to be added as a child node of PMIC.
>
> tested via Linux and U-Boot with reset extension:
>
> OpenSBI:
> Platform Reboot Device : da9063-reset
> Platform Shutdown Device : gpio-reset
>
> v1 -> v2:
> Added:
> lib: sbi: add priority for reset handler
>
> Renamed read/write to smbus_write/smbus_read, as actually this
> is not a "raw" read/write but a one with a register address provided,
> later a "raw" version will be need but currently i don't have anything
> to test it.
>
> To Xiang W:
> I have analized your proposal of switching to sbi_list,
> and switched i2c adapters array to list. Unfortunately
> to switch drivers we require "init" functions.
>
This also has the merit of getting rid of this limit of 16 i2c
adapters, that's nice. Maybe you could use that in the gpio library
too?
> to Alexander Ghiti:
>
> > No need for the struct i2c_adapter argument as it is globally accessible.
>
> It looks like a more clean and reusable way to me, let's here more opinions
>
> > Why should the da9063 device care about the adapter configuration here?
> > I think It should just rely on an already configured/initialized i2c
> > controller.
>
> With clocks init added to sifive_i2c configure it can become usable even if nobody
> cared about controller initialization.
I don't understand what you mean here. IMO, the device driver should
only take care of the communication with the device, not the
controller. That can be done in the init function of the controller
driver, which in addition would simplify the callbacks and the device
driver code.
>
> > 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.
>
> And how do you think it will look like ? In out case it's not a separate driver/client
> entity but all togather for simplicity.
In fdt_i2c_adapter_get, instead of passing a pointer to a struct
i2c_adapter, we could pass a i2c_device structure which would get
filled automatically with its adapter chip. But that implies
introducing this i2c_device like I mentioned in the previous review :)
In addition, I took a look at the gpio library and they did it this
way too.
Again, I don't see why the driver developer should care about its
adapter since everything could be done behind the scenes for him.
>
> Nikita Shubin (5):
> lib: utils/reset: add priority to gpio reset
> lib: utils/i2c: Add generic I2C configuration library
> lib: utils/i2c: Add simple FDT based I2C framework
> lib: utils/i2c: 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 | 99 +++++++++++
> lib/utils/i2c/fdt_i2c.c | 111 ++++++++++++
> lib/utils/i2c/fdt_i2c_sifive.c | 271 +++++++++++++++++++++++++++++
> lib/utils/i2c/i2c.c | 85 +++++++++
> lib/utils/i2c/objects.mk | 12 ++
> lib/utils/reset/fdt_reset.c | 2 +
> lib/utils/reset/fdt_reset_da9063.c | 214 +++++++++++++++++++++++
> lib/utils/reset/fdt_reset_gpio.c | 18 +-
> lib/utils/reset/objects.mk | 1 +
> 10 files changed, 836 insertions(+), 3 deletions(-)
> 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
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH v2 0/5] I2C framework, reboot Unmatched via PMIC
2021-10-19 11:57 ` Alexandre Ghiti
@ 2021-10-20 7:26 ` Nikita Shubin
2021-10-20 8:11 ` Alexandre Ghiti
0 siblings, 1 reply; 38+ messages in thread
From: Nikita Shubin @ 2021-10-20 7:26 UTC (permalink / raw)
To: opensbi
On Tue, 19 Oct 2021 13:57:35 +0200
Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote:
> On Fri, Oct 15, 2021 at 3:20 PM Nikita Shubin
> <nikita.shubin@maquefel.me> wrote:
> >
> > From: Nikita Shubin <n.shubin@yadro.com>
> >
> > This series introduce rebooting via i2c da9063 PMIC, currently on
> > SiFive Unmatched board.
> >
> > "gpio-poweroff" remains with default priority of 128 - default
> > priority, da9063 is 1 by default the least priority.
> >
> > da9063-reset {
> > compatible = "da9063-reset";
> > priority = <1>;
> > };
> >
> > Is required to be added as a child node of PMIC.
> >
> > tested via Linux and U-Boot with reset extension:
> >
> > OpenSBI:
> > Platform Reboot Device : da9063-reset
> > Platform Shutdown Device : gpio-reset
> >
> > v1 -> v2:
> > Added:
> > lib: sbi: add priority for reset handler
> >
> > Renamed read/write to smbus_write/smbus_read, as actually this
> > is not a "raw" read/write but a one with a register address
> > provided, later a "raw" version will be need but currently i don't
> > have anything to test it.
> >
> > To Xiang W:
> > I have analized your proposal of switching to sbi_list,
> > and switched i2c adapters array to list. Unfortunately
> > to switch drivers we require "init" functions.
> >
>
> This also has the merit of getting rid of this limit of 16 i2c
> adapters, that's nice. Maybe you could use that in the gpio library
> too?
Indeed, but it's not a part of this patch series.
>
> > to Alexander Ghiti:
> >
> > > No need for the struct i2c_adapter argument as it is globally
> > > accessible.
> >
> > It looks like a more clean and reusable way to me, let's here more
> > opinions
> > > Why should the da9063 device care about the adapter configuration
> > > here? I think It should just rely on an already
> > > configured/initialized i2c controller.
> >
> > With clocks init added to sifive_i2c configure it can become usable
> > even if nobody cared about controller initialization.
>
> I don't understand what you mean here. IMO, the device driver should
> only take care of the communication with the device, not the
> controller. That can be done in the init function of the controller
> driver, which in addition would simplify the callbacks and the device
> driver code.
Well to make use of I2C you should enable it togather with clocks, see
u-boot/arch/riscv/dts/fu740-c000.dtsi for example:
i2c0: i2c at 10030000 {
...
clocks = <&prci PRCI_CLK_PCLK>;
...
It part of driver responsibility to enable clocks it requires, cause if
not is required nobody will enable them.
>
> >
> > > 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.
> >
> > And how do you think it will look like ? In out case it's not a
> > separate driver/client entity but all togather for simplicity.
>
> In fdt_i2c_adapter_get, instead of passing a pointer to a struct
> i2c_adapter, we could pass a i2c_device structure which would get
> filled automatically with its adapter chip. But that implies
> introducing this i2c_device like I mentioned in the previous review :)
> In addition, I took a look at the gpio library and they did it this
> way too.
>
> Again, I don't see why the driver developer should care about its
> adapter since everything could be done behind the scenes for him.
Are you suggesting to separate the da9063_reset into client/driver
parts ? Don't you think that it will add unnecessary complicity if our
case ?
Linux i2c_client has a reference to the adapter it's sitting on:
https://elixir.bootlin.com/linux/v5.15-rc6/source/include/linux/i2c.h#L336
u-boot is relying on some "default" i2c adapter/bus ideed, but if we
look on:
https://elixir.bootlin.com/u-boot/latest/source/include/i2c.h#L718
We see it's rather complex and hard to understand.
>
> >
> > Nikita Shubin (5):
> > lib: utils/reset: add priority to gpio reset
> > lib: utils/i2c: Add generic I2C configuration library
> > lib: utils/i2c: Add simple FDT based I2C framework
> > lib: utils/i2c: 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 | 99 +++++++++++
> > lib/utils/i2c/fdt_i2c.c | 111 ++++++++++++
> > lib/utils/i2c/fdt_i2c_sifive.c | 271
> > +++++++++++++++++++++++++++++ lib/utils/i2c/i2c.c |
> > 85 +++++++++ lib/utils/i2c/objects.mk | 12 ++
> > lib/utils/reset/fdt_reset.c | 2 +
> > lib/utils/reset/fdt_reset_da9063.c | 214 +++++++++++++++++++++++
> > lib/utils/reset/fdt_reset_gpio.c | 18 +-
> > lib/utils/reset/objects.mk | 1 +
> > 10 files changed, 836 insertions(+), 3 deletions(-)
> > 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
> >
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH v2 0/5] I2C framework, reboot Unmatched via PMIC
2021-10-20 7:26 ` Nikita Shubin
@ 2021-10-20 8:11 ` Alexandre Ghiti
2021-10-20 8:42 ` Nikita Shubin
0 siblings, 1 reply; 38+ messages in thread
From: Alexandre Ghiti @ 2021-10-20 8:11 UTC (permalink / raw)
To: opensbi
On Wed, Oct 20, 2021 at 9:26 AM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> On Tue, 19 Oct 2021 13:57:35 +0200
> Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote:
>
> > On Fri, Oct 15, 2021 at 3:20 PM Nikita Shubin
> > <nikita.shubin@maquefel.me> wrote:
> > >
> > > From: Nikita Shubin <n.shubin@yadro.com>
> > >
> > > This series introduce rebooting via i2c da9063 PMIC, currently on
> > > SiFive Unmatched board.
> > >
> > > "gpio-poweroff" remains with default priority of 128 - default
> > > priority, da9063 is 1 by default the least priority.
> > >
> > > da9063-reset {
> > > compatible = "da9063-reset";
> > > priority = <1>;
> > > };
> > >
> > > Is required to be added as a child node of PMIC.
> > >
> > > tested via Linux and U-Boot with reset extension:
> > >
> > > OpenSBI:
> > > Platform Reboot Device : da9063-reset
> > > Platform Shutdown Device : gpio-reset
> > >
> > > v1 -> v2:
> > > Added:
> > > lib: sbi: add priority for reset handler
> > >
> > > Renamed read/write to smbus_write/smbus_read, as actually this
> > > is not a "raw" read/write but a one with a register address
> > > provided, later a "raw" version will be need but currently i don't
> > > have anything to test it.
> > >
> > > To Xiang W:
> > > I have analized your proposal of switching to sbi_list,
> > > and switched i2c adapters array to list. Unfortunately
> > > to switch drivers we require "init" functions.
> > >
> >
> > This also has the merit of getting rid of this limit of 16 i2c
> > adapters, that's nice. Maybe you could use that in the gpio library
> > too?
>
> Indeed, but it's not a part of this patch series.
>
> >
> > > to Alexander Ghiti:
> > >
> > > > No need for the struct i2c_adapter argument as it is globally
> > > > accessible.
> > >
> > > It looks like a more clean and reusable way to me, let's here more
> > > opinions
> > > > Why should the da9063 device care about the adapter configuration
> > > > here? I think It should just rely on an already
> > > > configured/initialized i2c controller.
> > >
> > > With clocks init added to sifive_i2c configure it can become usable
> > > even if nobody cared about controller initialization.
> >
> > I don't understand what you mean here. IMO, the device driver should
> > only take care of the communication with the device, not the
> > controller. That can be done in the init function of the controller
> > driver, which in addition would simplify the callbacks and the device
> > driver code.
>
> Well to make use of I2C you should enable it togather with clocks, see
> u-boot/arch/riscv/dts/fu740-c000.dtsi for example:
>
> i2c0: i2c at 10030000 {
> ...
> clocks = <&prci PRCI_CLK_PCLK>;
> ...
>
> It part of driver responsibility to enable clocks it requires, cause if
> not is required nobody will enable them.
I understand that but why couldn't this configuration be done in the
adapter driver instead of the da9063 driver? At the initialization of
the adapter driver, or at each beginning of a write sequence...etc.
>
> >
> > >
> > > > 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.
> > >
> > > And how do you think it will look like ? In out case it's not a
> > > separate driver/client entity but all togather for simplicity.
> >
> > In fdt_i2c_adapter_get, instead of passing a pointer to a struct
> > i2c_adapter, we could pass a i2c_device structure which would get
> > filled automatically with its adapter chip. But that implies
> > introducing this i2c_device like I mentioned in the previous review :)
> > In addition, I took a look at the gpio library and they did it this
> > way too.
> >
> > Again, I don't see why the driver developer should care about its
> > adapter since everything could be done behind the scenes for him.
>
> Are you suggesting to separate the da9063_reset into client/driver
> parts ? Don't you think that it will add unnecessary complicity if our
> case ?
To make sure we understand each other, find below a diff of what it
would look like the way I imagine it. It just makes the notion of
adapter disappear from the i2c device driver, that's all. This is what
is done in openSBI with gpio_pin structure, in Linux here
https://www.kernel.org/doc/html/latest/i2c/writing-clients.html#smbus-communication.
diff --git a/include/sbi_utils/i2c/i2c.h b/include/sbi_utils/i2c/i2c.h
index 76cdb66..0f1671e 100644
--- a/include/sbi_utils/i2c/i2c.h
+++ b/include/sbi_utils/i2c/i2c.h
@@ -50,6 +50,10 @@ struct i2c_adapter {
struct sbi_dlist node;
};
+struct i2c_device {
+ struct i2c_adapter *adap;
+};
+
static inline struct i2c_adapter *to_i2c_adapter(struct sbi_dlist *node)
{
return container_of(node, struct i2c_adapter, node);
diff --git a/lib/utils/i2c/i2c.c b/lib/utils/i2c/i2c.c
index d23ac91..9c64d09 100644
--- a/lib/utils/i2c/i2c.c
+++ b/lib/utils/i2c/i2c.c
@@ -61,25 +61,25 @@ int i2c_adapter_configure(struct i2c_adapter *ia)
return ia->configure(ia);
}
-int i2c_adapter_smbus_write(struct i2c_adapter *ia, uint8_t addr,
+int i2c_smbus_write(struct i2c_device *dev, uint8_t addr,
uint8_t reg, uint8_t *buffer, int len)
{
- if (!ia)
+ if (!dev)
return SBI_EINVAL;
- if (!ia->smbus_write)
+ if (!dev->adap || !dev->adap->smbus_write)
return SBI_ENOSYS;
- return ia->smbus_write(ia, addr, reg, buffer, len);
+ return dev->adap->smbus_write(dev, addr, reg, buffer, len);
}
-int i2c_adapter_smbus_read(struct i2c_adapter *ia, uint8_t addr,
+int i2c_smbus_read(struct i2c_device *dev, uint8_t addr,
uint8_t reg, uint8_t *buffer, int len)
{
- if (!ia)
+ if (!dev)
return SBI_EINVAL;
- if (!ia->smbus_read)
+ if (!dev->adap || !dev->adap->smbus_read)
return SBI_ENOSYS;
- return ia->smbus_read(ia, addr, reg, buffer, len);
+ return dev->adap->smbus_read(dev, addr, reg, buffer, len);
}
diff --git a/lib/utils/reset/fdt_reset_da9063.c
b/lib/utils/reset/fdt_reset_da9063.c
index e3c9ced..a14d941 100644
--- a/lib/utils/reset/fdt_reset_da9063.c
+++ b/lib/utils/reset/fdt_reset_da9063.c
@@ -45,7 +45,7 @@
#define PMIC_CHIP_ID_DA9063 0x61
static struct {
- struct i2c_adapter *adapter;
+ struct i2c_device *dev;
uint32_t reg;
u8 priority;
} da9063 = {
@@ -103,20 +103,20 @@ static inline int da9063_shutdown(struct
i2c_adapter *adap, uint32_t reg)
DA9063_REG_CONTROL_F,
DA9063_CONTROL_F_SHUTDOWN);
}
-static inline int da9063_reset(struct i2c_adapter *adap, uint32_t reg)
+static inline int da9063_reset(struct i2c_device *dev, uint32_t reg)
{
- int rc = i2c_adapter_smbus_reg_write(adap, da9063.reg,
+ int rc = i2c_smbus_reg_write(dev, da9063.reg,
DA9063_REG_PAGE_CON, 0x00);
if (rc)
return rc;
- rc = i2c_adapter_smbus_reg_write(adap, da9063.reg,
+ rc = i2c_smbus_reg_write(dev, da9063.reg,
DA9063_REG_CONTROL_F, DA9063_CONTROL_F_WAKEUP);
if (rc)
return rc;
- return i2c_adapter_smbus_reg_write(adap, da9063.reg,
+ return i2c_smbus_reg_write(dev, da9063.reg,
DA9063_REG_CONTROL_A,
DA9063_CONTROL_A_M_POWER1_EN |
DA9063_CONTROL_A_M_POWER_EN |
@@ -169,7 +169,6 @@ static int da9063_reset_init(void *fdt, int nodeoff,
{
int rc, i2c_dev, i2c_bus, len;
const fdt32_t *val;
- struct i2c_adapter *adapter;
uint64_t addr;
/* find dlg,da9063 parent node */
@@ -192,12 +191,10 @@ static int da9063_reset_init(void *fdt, int nodeoff,
return i2c_bus;
/* i2c adapter get */
- rc = fdt_i2c_adapter_get(fdt, i2c_bus, &adapter);
+ rc = fdt_i2c_adapter_get(fdt, i2c_bus, &da9063.dev);
if (rc)
return rc;
- da9063.adapter = adapter;
-
sbi_system_reset_add_device(&da9063_reset_i2c);
return 0;
>
> Linux i2c_client has a reference to the adapter it's sitting on:
> https://elixir.bootlin.com/linux/v5.15-rc6/source/include/linux/i2c.h#L336
>
> u-boot is relying on some "default" i2c adapter/bus ideed, but if we
> look on:
>
> https://elixir.bootlin.com/u-boot/latest/source/include/i2c.h#L718
>
> We see it's rather complex and hard to understand.
>
>
>
> >
> > >
> > > Nikita Shubin (5):
> > > lib: utils/reset: add priority to gpio reset
> > > lib: utils/i2c: Add generic I2C configuration library
> > > lib: utils/i2c: Add simple FDT based I2C framework
> > > lib: utils/i2c: 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 | 99 +++++++++++
> > > lib/utils/i2c/fdt_i2c.c | 111 ++++++++++++
> > > lib/utils/i2c/fdt_i2c_sifive.c | 271
> > > +++++++++++++++++++++++++++++ lib/utils/i2c/i2c.c |
> > > 85 +++++++++ lib/utils/i2c/objects.mk | 12 ++
> > > lib/utils/reset/fdt_reset.c | 2 +
> > > lib/utils/reset/fdt_reset_da9063.c | 214 +++++++++++++++++++++++
> > > lib/utils/reset/fdt_reset_gpio.c | 18 +-
> > > lib/utils/reset/objects.mk | 1 +
> > > 10 files changed, 836 insertions(+), 3 deletions(-)
> > > 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
> > >
> > >
> > > --
> > > opensbi mailing list
> > > opensbi at lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/opensbi
>
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v2 0/5] I2C framework, reboot Unmatched via PMIC
2021-10-20 8:11 ` Alexandre Ghiti
@ 2021-10-20 8:42 ` Nikita Shubin
2021-10-21 4:34 ` Alexandre Ghiti
0 siblings, 1 reply; 38+ messages in thread
From: Nikita Shubin @ 2021-10-20 8:42 UTC (permalink / raw)
To: opensbi
On Wed, 20 Oct 2021 10:11:56 +0200
Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote:
> >
> > It part of driver responsibility to enable clocks it requires,
> > cause if not is required nobody will enable them.
>
> I understand that but why couldn't this configuration be done in the
> adapter driver instead of the da9063 driver? At the initialization of
> the adapter driver, or at each beginning of a write sequence...etc.
>
I think we should stick to "don't touch until you really need", that's
why we shouldn't do it in driver init, we can enable clocks on unused
bus which will lead to more power consumption, and other's won't be
aware that it needs to be gated off (and even so it's need to gated on
once again).
Also it's really good to disable interrupts when you don't need them.
Doing it every read/write seems quite wrong to me, that's why i decided
to add a separate method for such tasks.
> >
> > >
> > > >
> > > > > 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.
> > > >
> > > > And how do you think it will look like ? In out case it's not a
> > > > separate driver/client entity but all togather for simplicity.
> > >
> > > In fdt_i2c_adapter_get, instead of passing a pointer to a struct
> > > i2c_adapter, we could pass a i2c_device structure which would get
> > > filled automatically with its adapter chip. But that implies
> > > introducing this i2c_device like I mentioned in the previous
> > > review :) In addition, I took a look at the gpio library and they
> > > did it this way too.
> > >
> > > Again, I don't see why the driver developer should care about its
> > > adapter since everything could be done behind the scenes for him.
> > >
> >
> > Are you suggesting to separate the da9063_reset into client/driver
> > parts ? Don't you think that it will add unnecessary complicity if
> > our case ?
>
> To make sure we understand each other, find below a diff of what it
> would look like the way I imagine it. It just makes the notion of
> adapter disappear from the i2c device driver, that's all. This is what
> is done in openSBI with gpio_pin structure, in Linux here
> https://www.kernel.org/doc/html/latest/i2c/writing-clients.html#smbus-communication.
>
> diff --git a/include/sbi_utils/i2c/i2c.h b/include/sbi_utils/i2c/i2c.h
> index 76cdb66..0f1671e 100644
> --- a/include/sbi_utils/i2c/i2c.h
> +++ b/include/sbi_utils/i2c/i2c.h
> @@ -50,6 +50,10 @@ struct i2c_adapter {
> struct sbi_dlist node;
> };
>
> +struct i2c_device {
> + struct i2c_adapter *adap;
> +};
> +
> static inline struct i2c_adapter *to_i2c_adapter(struct sbi_dlist
> *node) {
> return container_of(node, struct i2c_adapter, node);
> diff --git a/lib/utils/i2c/i2c.c b/lib/utils/i2c/i2c.c
> index d23ac91..9c64d09 100644
> --- a/lib/utils/i2c/i2c.c
> +++ b/lib/utils/i2c/i2c.c
> @@ -61,25 +61,25 @@ int i2c_adapter_configure(struct i2c_adapter *ia)
> return ia->configure(ia);
> }
>
> -int i2c_adapter_smbus_write(struct i2c_adapter *ia, uint8_t addr,
> +int i2c_smbus_write(struct i2c_device *dev, uint8_t addr,
> uint8_t reg, uint8_t *buffer, int len)
> {
> - if (!ia)
> + if (!dev)
> return SBI_EINVAL;
> - if (!ia->smbus_write)
> + if (!dev->adap || !dev->adap->smbus_write)
> return SBI_ENOSYS;
>
> - return ia->smbus_write(ia, addr, reg, buffer, len);
> + return dev->adap->smbus_write(dev, addr, reg, buffer, len);
> }
>
>
> -int i2c_adapter_smbus_read(struct i2c_adapter *ia, uint8_t addr,
> +int i2c_smbus_read(struct i2c_device *dev, uint8_t addr,
> uint8_t reg, uint8_t *buffer, int len)
> {
> - if (!ia)
> + if (!dev)
> return SBI_EINVAL;
> - if (!ia->smbus_read)
> + if (!dev->adap || !dev->adap->smbus_read)
> return SBI_ENOSYS;
>
> - return ia->smbus_read(ia, addr, reg, buffer, len);
> + return dev->adap->smbus_read(dev, addr, reg, buffer, len);
> }
> diff --git a/lib/utils/reset/fdt_reset_da9063.c
> b/lib/utils/reset/fdt_reset_da9063.c
> index e3c9ced..a14d941 100644
> --- a/lib/utils/reset/fdt_reset_da9063.c
> +++ b/lib/utils/reset/fdt_reset_da9063.c
> @@ -45,7 +45,7 @@
> #define PMIC_CHIP_ID_DA9063 0x61
>
> static struct {
> - struct i2c_adapter *adapter;
> + struct i2c_device *dev;
> uint32_t reg;
> u8 priority;
> } da9063 = {
> @@ -103,20 +103,20 @@ static inline int da9063_shutdown(struct
> i2c_adapter *adap, uint32_t reg)
> DA9063_REG_CONTROL_F,
> DA9063_CONTROL_F_SHUTDOWN);
> }
>
> -static inline int da9063_reset(struct i2c_adapter *adap, uint32_t
> reg) +static inline int da9063_reset(struct i2c_device *dev, uint32_t
> reg) {
> - int rc = i2c_adapter_smbus_reg_write(adap, da9063.reg,
> + int rc = i2c_smbus_reg_write(dev, da9063.reg,
> DA9063_REG_PAGE_CON, 0x00);
>
> if (rc)
> return rc;
>
> - rc = i2c_adapter_smbus_reg_write(adap, da9063.reg,
> + rc = i2c_smbus_reg_write(dev, da9063.reg,
> DA9063_REG_CONTROL_F,
> DA9063_CONTROL_F_WAKEUP); if (rc)
> return rc;
>
> - return i2c_adapter_smbus_reg_write(adap, da9063.reg,
> + return i2c_smbus_reg_write(dev, da9063.reg,
> DA9063_REG_CONTROL_A,
> DA9063_CONTROL_A_M_POWER1_EN |
> DA9063_CONTROL_A_M_POWER_EN |
> @@ -169,7 +169,6 @@ static int da9063_reset_init(void *fdt, int
> nodeoff, {
> int rc, i2c_dev, i2c_bus, len;
> const fdt32_t *val;
> - struct i2c_adapter *adapter;
> uint64_t addr;
>
> /* find dlg,da9063 parent node */
> @@ -192,12 +191,10 @@ static int da9063_reset_init(void *fdt, int
> nodeoff, return i2c_bus;
>
> /* i2c adapter get */
> - rc = fdt_i2c_adapter_get(fdt, i2c_bus, &adapter);
> + rc = fdt_i2c_adapter_get(fdt, i2c_bus, &da9063.dev);
> if (rc)
> return rc;
>
> - da9063.adapter = adapter;
> -
> sbi_system_reset_add_device(&da9063_reset_i2c);
>
> return 0;
>
So you indeed suggesting to add one level of abstraction.
I don't see any strong benefit from this.
Who will take care of device creation/initialisation in this case - the
adapter ?
>
> >
> > Linux i2c_client has a reference to the adapter it's sitting on:
> > https://elixir.bootlin.com/linux/v5.15-rc6/source/include/linux/i2c.h#L336
> >
> > u-boot is relying on some "default" i2c adapter/bus ideed, but if we
> > look on:
> >
> > https://elixir.bootlin.com/u-boot/latest/source/include/i2c.h#L718
> >
> > We see it's rather complex and hard to understand.
> >
> >
> >
> > >
> > > >
> > > > Nikita Shubin (5):
> > > > lib: utils/reset: add priority to gpio reset
> > > > lib: utils/i2c: Add generic I2C configuration library
> > > > lib: utils/i2c: Add simple FDT based I2C framework
> > > > lib: utils/i2c: 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 | 99 +++++++++++
> > > > lib/utils/i2c/fdt_i2c.c | 111 ++++++++++++
> > > > lib/utils/i2c/fdt_i2c_sifive.c | 271
> > > > +++++++++++++++++++++++++++++ lib/utils/i2c/i2c.c
> > > > | 85 +++++++++ lib/utils/i2c/objects.mk | 12 ++
> > > > lib/utils/reset/fdt_reset.c | 2 +
> > > > lib/utils/reset/fdt_reset_da9063.c | 214
> > > > +++++++++++++++++++++++ lib/utils/reset/fdt_reset_gpio.c |
> > > > 18 +- lib/utils/reset/objects.mk | 1 +
> > > > 10 files changed, 836 insertions(+), 3 deletions(-)
> > > > 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
> > > >
> > > >
> > > > --
> > > > opensbi mailing list
> > > > opensbi at lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/opensbi
> >
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH v2 0/5] I2C framework, reboot Unmatched via PMIC
2021-10-20 8:42 ` Nikita Shubin
@ 2021-10-21 4:34 ` Alexandre Ghiti
0 siblings, 0 replies; 38+ messages in thread
From: Alexandre Ghiti @ 2021-10-21 4:34 UTC (permalink / raw)
To: opensbi
On Wed, Oct 20, 2021 at 10:42 AM Nikita Shubin
<nikita.shubin@maquefel.me> wrote:
>
> On Wed, 20 Oct 2021 10:11:56 +0200
> Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote:
>
> > >
> > > It part of driver responsibility to enable clocks it requires,
> > > cause if not is required nobody will enable them.
> >
> > I understand that but why couldn't this configuration be done in the
> > adapter driver instead of the da9063 driver? At the initialization of
> > the adapter driver, or at each beginning of a write sequence...etc.
> >
>
> I think we should stick to "don't touch until you really need", that's
> why we shouldn't do it in driver init, we can enable clocks on unused
> bus which will lead to more power consumption, and other's won't be
> aware that it needs to be gated off (and even so it's need to gated on
> once again).
>
> Also it's really good to disable interrupts when you don't need them.
>
> Doing it every read/write seems quite wrong to me, that's why i decided
> to add a separate method for such tasks.
>
> > >
> > > >
> > > > >
> > > > > > 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.
> > > > >
> > > > > And how do you think it will look like ? In out case it's not a
> > > > > separate driver/client entity but all togather for simplicity.
> > > >
> > > > In fdt_i2c_adapter_get, instead of passing a pointer to a struct
> > > > i2c_adapter, we could pass a i2c_device structure which would get
> > > > filled automatically with its adapter chip. But that implies
> > > > introducing this i2c_device like I mentioned in the previous
> > > > review :) In addition, I took a look at the gpio library and they
> > > > did it this way too.
> > > >
> > > > Again, I don't see why the driver developer should care about its
> > > > adapter since everything could be done behind the scenes for him.
> > > >
> > >
> > > Are you suggesting to separate the da9063_reset into client/driver
> > > parts ? Don't you think that it will add unnecessary complicity if
> > > our case ?
> >
> > To make sure we understand each other, find below a diff of what it
> > would look like the way I imagine it. It just makes the notion of
> > adapter disappear from the i2c device driver, that's all. This is what
> > is done in openSBI with gpio_pin structure, in Linux here
> > https://www.kernel.org/doc/html/latest/i2c/writing-clients.html#smbus-communication.
> >
> > diff --git a/include/sbi_utils/i2c/i2c.h b/include/sbi_utils/i2c/i2c.h
> > index 76cdb66..0f1671e 100644
> > --- a/include/sbi_utils/i2c/i2c.h
> > +++ b/include/sbi_utils/i2c/i2c.h
> > @@ -50,6 +50,10 @@ struct i2c_adapter {
> > struct sbi_dlist node;
> > };
> >
> > +struct i2c_device {
> > + struct i2c_adapter *adap;
> > +};
> > +
> > static inline struct i2c_adapter *to_i2c_adapter(struct sbi_dlist
> > *node) {
> > return container_of(node, struct i2c_adapter, node);
> > diff --git a/lib/utils/i2c/i2c.c b/lib/utils/i2c/i2c.c
> > index d23ac91..9c64d09 100644
> > --- a/lib/utils/i2c/i2c.c
> > +++ b/lib/utils/i2c/i2c.c
> > @@ -61,25 +61,25 @@ int i2c_adapter_configure(struct i2c_adapter *ia)
> > return ia->configure(ia);
> > }
> >
> > -int i2c_adapter_smbus_write(struct i2c_adapter *ia, uint8_t addr,
> > +int i2c_smbus_write(struct i2c_device *dev, uint8_t addr,
> > uint8_t reg, uint8_t *buffer, int len)
> > {
> > - if (!ia)
> > + if (!dev)
> > return SBI_EINVAL;
> > - if (!ia->smbus_write)
> > + if (!dev->adap || !dev->adap->smbus_write)
> > return SBI_ENOSYS;
> >
> > - return ia->smbus_write(ia, addr, reg, buffer, len);
> > + return dev->adap->smbus_write(dev, addr, reg, buffer, len);
> > }
> >
> >
> > -int i2c_adapter_smbus_read(struct i2c_adapter *ia, uint8_t addr,
> > +int i2c_smbus_read(struct i2c_device *dev, uint8_t addr,
> > uint8_t reg, uint8_t *buffer, int len)
> > {
> > - if (!ia)
> > + if (!dev)
> > return SBI_EINVAL;
> > - if (!ia->smbus_read)
> > + if (!dev->adap || !dev->adap->smbus_read)
> > return SBI_ENOSYS;
> >
> > - return ia->smbus_read(ia, addr, reg, buffer, len);
> > + return dev->adap->smbus_read(dev, addr, reg, buffer, len);
> > }
> > diff --git a/lib/utils/reset/fdt_reset_da9063.c
> > b/lib/utils/reset/fdt_reset_da9063.c
> > index e3c9ced..a14d941 100644
> > --- a/lib/utils/reset/fdt_reset_da9063.c
> > +++ b/lib/utils/reset/fdt_reset_da9063.c
> > @@ -45,7 +45,7 @@
> > #define PMIC_CHIP_ID_DA9063 0x61
> >
> > static struct {
> > - struct i2c_adapter *adapter;
> > + struct i2c_device *dev;
> > uint32_t reg;
> > u8 priority;
> > } da9063 = {
> > @@ -103,20 +103,20 @@ static inline int da9063_shutdown(struct
> > i2c_adapter *adap, uint32_t reg)
> > DA9063_REG_CONTROL_F,
> > DA9063_CONTROL_F_SHUTDOWN);
> > }
> >
> > -static inline int da9063_reset(struct i2c_adapter *adap, uint32_t
> > reg) +static inline int da9063_reset(struct i2c_device *dev, uint32_t
> > reg) {
> > - int rc = i2c_adapter_smbus_reg_write(adap, da9063.reg,
> > + int rc = i2c_smbus_reg_write(dev, da9063.reg,
> > DA9063_REG_PAGE_CON, 0x00);
> >
> > if (rc)
> > return rc;
> >
> > - rc = i2c_adapter_smbus_reg_write(adap, da9063.reg,
> > + rc = i2c_smbus_reg_write(dev, da9063.reg,
> > DA9063_REG_CONTROL_F,
> > DA9063_CONTROL_F_WAKEUP); if (rc)
> > return rc;
> >
> > - return i2c_adapter_smbus_reg_write(adap, da9063.reg,
> > + return i2c_smbus_reg_write(dev, da9063.reg,
> > DA9063_REG_CONTROL_A,
> > DA9063_CONTROL_A_M_POWER1_EN |
> > DA9063_CONTROL_A_M_POWER_EN |
> > @@ -169,7 +169,6 @@ static int da9063_reset_init(void *fdt, int
> > nodeoff, {
> > int rc, i2c_dev, i2c_bus, len;
> > const fdt32_t *val;
> > - struct i2c_adapter *adapter;
> > uint64_t addr;
> >
> > /* find dlg,da9063 parent node */
> > @@ -192,12 +191,10 @@ static int da9063_reset_init(void *fdt, int
> > nodeoff, return i2c_bus;
> >
> > /* i2c adapter get */
> > - rc = fdt_i2c_adapter_get(fdt, i2c_bus, &adapter);
> > + rc = fdt_i2c_adapter_get(fdt, i2c_bus, &da9063.dev);
> > if (rc)
> > return rc;
> >
> > - da9063.adapter = adapter;
> > -
> > sbi_system_reset_add_device(&da9063_reset_i2c);
> >
> > return 0;
> >
>
> So you indeed suggesting to add one level of abstraction.
>
> I don't see any strong benefit from this.
Ok.
>
> Who will take care of device creation/initialisation in this case - the
> adapter ?
>
> >
> > >
> > > Linux i2c_client has a reference to the adapter it's sitting on:
> > > https://elixir.bootlin.com/linux/v5.15-rc6/source/include/linux/i2c.h#L336
> > >
> > > u-boot is relying on some "default" i2c adapter/bus ideed, but if we
> > > look on:
> > >
> > > https://elixir.bootlin.com/u-boot/latest/source/include/i2c.h#L718
> > >
> > > We see it's rather complex and hard to understand.
> > >
> > >
> > >
> > > >
> > > > >
> > > > > Nikita Shubin (5):
> > > > > lib: utils/reset: add priority to gpio reset
> > > > > lib: utils/i2c: Add generic I2C configuration library
> > > > > lib: utils/i2c: Add simple FDT based I2C framework
> > > > > lib: utils/i2c: 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 | 99 +++++++++++
> > > > > lib/utils/i2c/fdt_i2c.c | 111 ++++++++++++
> > > > > lib/utils/i2c/fdt_i2c_sifive.c | 271
> > > > > +++++++++++++++++++++++++++++ lib/utils/i2c/i2c.c
> > > > > | 85 +++++++++ lib/utils/i2c/objects.mk | 12 ++
> > > > > lib/utils/reset/fdt_reset.c | 2 +
> > > > > lib/utils/reset/fdt_reset_da9063.c | 214
> > > > > +++++++++++++++++++++++ lib/utils/reset/fdt_reset_gpio.c |
> > > > > 18 +- lib/utils/reset/objects.mk | 1 +
> > > > > 10 files changed, 836 insertions(+), 3 deletions(-)
> > > > > 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
> > > > >
> > > > >
> > > > > --
> > > > > opensbi mailing list
> > > > > opensbi at lists.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/opensbi
> > >
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 0/5] I2C framework, reboot Unmatched via PMIC
2021-10-15 13:19 [PATCH v2 0/5] I2C framework, reboot Unmatched via PMIC Nikita Shubin
` (6 preceding siblings ...)
2021-10-19 11:57 ` Alexandre Ghiti
@ 2021-10-20 4:59 ` Alexandre Ghiti
2021-10-20 6:17 ` Nikita Shubin
7 siblings, 1 reply; 38+ messages in thread
From: Alexandre Ghiti @ 2021-10-20 4:59 UTC (permalink / raw)
To: opensbi
On Fri, Oct 15, 2021 at 3:20 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> From: Nikita Shubin <n.shubin@yadro.com>
>
> This series introduce rebooting via i2c da9063 PMIC, currently on
> SiFive Unmatched board.
>
> "gpio-poweroff" remains with default priority of 128 - default priority,
> da9063 is 1 by default the least priority.
>
> da9063-reset {
> compatible = "da9063-reset";
> priority = <1>;
> };
>
> Is required to be added as a child node of PMIC.
>
> tested via Linux and U-Boot with reset extension:
>
> OpenSBI:
> Platform Reboot Device : da9063-reset
> Platform Shutdown Device : gpio-reset
>
> v1 -> v2:
> Added:
> lib: sbi: add priority for reset handler
>
> Renamed read/write to smbus_write/smbus_read, as actually this
> is not a "raw" read/write but a one with a register address provided,
> later a "raw" version will be need but currently i don't have anything
> to test it.
>
> To Xiang W:
> I have analized your proposal of switching to sbi_list,
> and switched i2c adapters array to list. Unfortunately
> to switch drivers we require "init" functions.
>
> to Alexander Ghiti:
>
> > No need for the struct i2c_adapter argument as it is globally accessible.
>
> It looks like a more clean and reusable way to me, let's here more opinions
>
> > Why should the da9063 device care about the adapter configuration here?
> > I think It should just rely on an already configured/initialized i2c
> > controller.
>
> With clocks init added to sifive_i2c configure it can become usable even if nobody
> cared about controller initialization.
>
> > 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.
>
> And how do you think it will look like ? In out case it's not a separate driver/client
> entity but all togather for simplicity.
>
> Nikita Shubin (5):
> lib: utils/reset: add priority to gpio reset
> lib: utils/i2c: Add generic I2C configuration library
> lib: utils/i2c: Add simple FDT based I2C framework
> lib: utils/i2c: 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 | 99 +++++++++++
> lib/utils/i2c/fdt_i2c.c | 111 ++++++++++++
> lib/utils/i2c/fdt_i2c_sifive.c | 271 +++++++++++++++++++++++++++++
> lib/utils/i2c/i2c.c | 85 +++++++++
> lib/utils/i2c/objects.mk | 12 ++
> lib/utils/reset/fdt_reset.c | 2 +
> lib/utils/reset/fdt_reset_da9063.c | 214 +++++++++++++++++++++++
> lib/utils/reset/fdt_reset_gpio.c | 18 +-
> lib/utils/reset/objects.mk | 1 +
> 10 files changed, 836 insertions(+), 3 deletions(-)
> 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
>
I'm testing your patchset and although it seems to work from Linux, it
fails when used from u-boot at the first smbus_write in
da9063_sanity_check. Maybe it relies on some initialization done by
the Linux driver?
Thanks,
Alex
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH v2 0/5] I2C framework, reboot Unmatched via PMIC
2021-10-20 4:59 ` Alexandre Ghiti
@ 2021-10-20 6:17 ` Nikita Shubin
2021-10-26 6:34 ` Alexandre Ghiti
0 siblings, 1 reply; 38+ messages in thread
From: Nikita Shubin @ 2021-10-20 6:17 UTC (permalink / raw)
To: opensbi
Hello Alexandre!
On Wed, 20 Oct 2021 06:59:25 +0200
Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote:
> On Fri, Oct 15, 2021 at 3:20 PM Nikita Shubin
> <nikita.shubin@maquefel.me> wrote:
> >
> > From: Nikita Shubin <n.shubin@yadro.com>
> >
> > This series introduce rebooting via i2c da9063 PMIC, currently on
> > SiFive Unmatched board.
> >
> > "gpio-poweroff" remains with default priority of 128 - default
> > priority, da9063 is 1 by default the least priority.
> >
> > da9063-reset {
> > compatible = "da9063-reset";
> > priority = <1>;
> > };
> >
> > Is required to be added as a child node of PMIC.
> >
> > tested via Linux and U-Boot with reset extension:
> >
> > OpenSBI:
> > Platform Reboot Device : da9063-reset
> > Platform Shutdown Device : gpio-reset
> >
> > v1 -> v2:
> > Added:
> > lib: sbi: add priority for reset handler
> >
> > Renamed read/write to smbus_write/smbus_read, as actually this
> > is not a "raw" read/write but a one with a register address
> > provided, later a "raw" version will be need but currently i don't
> > have anything to test it.
> >
> > To Xiang W:
> > I have analized your proposal of switching to sbi_list,
> > and switched i2c adapters array to list. Unfortunately
> > to switch drivers we require "init" functions.
> >
> > to Alexander Ghiti:
> >
> > > No need for the struct i2c_adapter argument as it is globally
> > > accessible.
> >
> > It looks like a more clean and reusable way to me, let's here more
> > opinions
> > > Why should the da9063 device care about the adapter configuration
> > > here? I think It should just rely on an already
> > > configured/initialized i2c controller.
> >
> > With clocks init added to sifive_i2c configure it can become usable
> > even if nobody cared about controller initialization.
> >
> > > 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.
> >
> > And how do you think it will look like ? In out case it's not a
> > separate driver/client entity but all togather for simplicity.
> >
> > Nikita Shubin (5):
> > lib: utils/reset: add priority to gpio reset
> > lib: utils/i2c: Add generic I2C configuration library
> > lib: utils/i2c: Add simple FDT based I2C framework
> > lib: utils/i2c: 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 | 99 +++++++++++
> > lib/utils/i2c/fdt_i2c.c | 111 ++++++++++++
> > lib/utils/i2c/fdt_i2c_sifive.c | 271
> > +++++++++++++++++++++++++++++ lib/utils/i2c/i2c.c |
> > 85 +++++++++ lib/utils/i2c/objects.mk | 12 ++
> > lib/utils/reset/fdt_reset.c | 2 +
> > lib/utils/reset/fdt_reset_da9063.c | 214 +++++++++++++++++++++++
> > lib/utils/reset/fdt_reset_gpio.c | 18 +-
> > lib/utils/reset/objects.mk | 1 +
> > 10 files changed, 836 insertions(+), 3 deletions(-)
> > 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
> >
>
> I'm testing your patchset and although it seems to work from Linux, it
> fails when used from u-boot at the first smbus_write in
> da9063_sanity_check. Maybe it relies on some initialization done by
> the Linux driver?
>
That's sound strange becouse i explicitly tested with u-boot SBI
extension, well in fact i tested mostly via u-boot because it's quicker
than booting Linux.
Do you have CONFIG_SYS_I2C_OCORES enabled ?
I tested on u-boot v2021.07 with Sifive Unmatched patches and SBI reset
extension applied. May it is time for configure ;) ?
Sharing my u-boot branch:
https://github.com/maquefel/u-boot/tree/yadro/riscv/unmatched-v2021.07
> Thanks,
>
> Alex
>
>
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH v2 0/5] I2C framework, reboot Unmatched via PMIC
2021-10-20 6:17 ` Nikita Shubin
@ 2021-10-26 6:34 ` Alexandre Ghiti
2021-10-26 14:30 ` Nikita Shubin
0 siblings, 1 reply; 38+ messages in thread
From: Alexandre Ghiti @ 2021-10-26 6:34 UTC (permalink / raw)
To: opensbi
On Wed, Oct 20, 2021 at 8:17 AM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> Hello Alexandre!
>
> On Wed, 20 Oct 2021 06:59:25 +0200
> Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote:
>
> > On Fri, Oct 15, 2021 at 3:20 PM Nikita Shubin
> > <nikita.shubin@maquefel.me> wrote:
> > >
> > > From: Nikita Shubin <n.shubin@yadro.com>
> > >
> > > This series introduce rebooting via i2c da9063 PMIC, currently on
> > > SiFive Unmatched board.
> > >
> > > "gpio-poweroff" remains with default priority of 128 - default
> > > priority, da9063 is 1 by default the least priority.
> > >
> > > da9063-reset {
> > > compatible = "da9063-reset";
> > > priority = <1>;
> > > };
> > >
> > > Is required to be added as a child node of PMIC.
> > >
> > > tested via Linux and U-Boot with reset extension:
> > >
> > > OpenSBI:
> > > Platform Reboot Device : da9063-reset
> > > Platform Shutdown Device : gpio-reset
> > >
> > > v1 -> v2:
> > > Added:
> > > lib: sbi: add priority for reset handler
> > >
> > > Renamed read/write to smbus_write/smbus_read, as actually this
> > > is not a "raw" read/write but a one with a register address
> > > provided, later a "raw" version will be need but currently i don't
> > > have anything to test it.
> > >
> > > To Xiang W:
> > > I have analized your proposal of switching to sbi_list,
> > > and switched i2c adapters array to list. Unfortunately
> > > to switch drivers we require "init" functions.
> > >
> > > to Alexander Ghiti:
> > >
> > > > No need for the struct i2c_adapter argument as it is globally
> > > > accessible.
> > >
> > > It looks like a more clean and reusable way to me, let's here more
> > > opinions
> > > > Why should the da9063 device care about the adapter configuration
> > > > here? I think It should just rely on an already
> > > > configured/initialized i2c controller.
> > >
> > > With clocks init added to sifive_i2c configure it can become usable
> > > even if nobody cared about controller initialization.
> > >
> > > > 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.
> > >
> > > And how do you think it will look like ? In out case it's not a
> > > separate driver/client entity but all togather for simplicity.
> > >
> > > Nikita Shubin (5):
> > > lib: utils/reset: add priority to gpio reset
> > > lib: utils/i2c: Add generic I2C configuration library
> > > lib: utils/i2c: Add simple FDT based I2C framework
> > > lib: utils/i2c: 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 | 99 +++++++++++
> > > lib/utils/i2c/fdt_i2c.c | 111 ++++++++++++
> > > lib/utils/i2c/fdt_i2c_sifive.c | 271
> > > +++++++++++++++++++++++++++++ lib/utils/i2c/i2c.c |
> > > 85 +++++++++ lib/utils/i2c/objects.mk | 12 ++
> > > lib/utils/reset/fdt_reset.c | 2 +
> > > lib/utils/reset/fdt_reset_da9063.c | 214 +++++++++++++++++++++++
> > > lib/utils/reset/fdt_reset_gpio.c | 18 +-
> > > lib/utils/reset/objects.mk | 1 +
> > > 10 files changed, 836 insertions(+), 3 deletions(-)
> > > 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
> > >
> >
> > I'm testing your patchset and although it seems to work from Linux, it
> > fails when used from u-boot at the first smbus_write in
> > da9063_sanity_check. Maybe it relies on some initialization done by
> > the Linux driver?
> >
>
> That's sound strange becouse i explicitly tested with u-boot SBI
> extension, well in fact i tested mostly via u-boot because it's quicker
> than booting Linux.
>
> Do you have CONFIG_SYS_I2C_OCORES enabled ?
>
> I tested on u-boot v2021.07 with Sifive Unmatched patches and SBI reset
> extension applied. May it is time for configure ;) ?
>
> Sharing my u-boot branch:
> https://github.com/maquefel/u-boot/tree/yadro/riscv/unmatched-v2021.07
That's weird, it still does not work for me from u-boot with the following:
- openSBI: https://github.com/AlexGhiti/opensbi/tree/int/alex/nikita_pmic_reset_v2
- u-boot: https://github.com/maquefel/u-boot/tree/yadro/riscv/unmatched-v2021.07
And this fails this way:
=> reset
resetting ...
da9063_system_reset: chip is not da9063 PMIC
Alex
>
>
> > Thanks,
> >
> > Alex
> >
> >
> > >
> > > --
> > > opensbi mailing list
> > > opensbi at lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/opensbi
>
^ permalink raw reply [flat|nested] 38+ messages in thread