[PATCH v11 0/8] Turris Omnia MCU driver

[PATCH v11 0/8] Turris Omnia MCU driver

[Marek Behún]

Hello Andy, Hans, Ilpo, Arnd, Gregory, and others,

this is v11 of the series adding Turris Omnia MCU driver.

Changes since v10:
- dropped patch 7 from v10 ("Add support for digital message signing via
  debugfs"). This must be done via different kernel API (should be
  doable via keyctl), but this requires more work which I currently
  don't have, unfortunately
- in patch 3 where I introduce support for MCU connected GPIOs
  changed u32 types to unsigned long where it made sense, in order
  to be able to use __assign_bit(), __set_bit(), test_bit().
  This was suggested by Andy.
- in patch 3 deduplicated code in omnia_gpio_get_multiple()
- moved the "fixing" in patch 3 of functions introduced in patch 2
  to patch 2, this was a rebasing error in v10
- changed date to September 2024 and KernelVersion to 6.11 in
  Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu

Links to previous cover letters (v1 to v10):
  https://patchwork.kernel.org/project/linux-soc/cover/20230823161012.6986-1-kabel@kernel.org/
  https://patchwork.kernel.org/project/linux-soc/cover/20230919103815.16818-1-kabel@kernel.org/
  https://patchwork.kernel.org/project/linux-soc/cover/20231023143130.11602-1-kabel@kernel.org/
  https://patchwork.kernel.org/project/linux-soc/cover/20231026161803.16750-1-kabel@kernel.org/
  https://patchwork.kernel.org/project/linux-soc/cover/20240323164359.21642-1-kabel@kernel.org/
  https://patchwork.kernel.org/project/linux-soc/cover/20240418121116.22184-1-kabel@kernel.org/
  https://patchwork.kernel.org/project/linux-soc/cover/20240424173809.7214-1-kabel@kernel.org/
  https://patchwork.kernel.org/project/linux-soc/cover/20240430115111.3453-1-kabel@kernel.org/
  https://patchwork.kernel.org/project/linux-soc/cover/20240508103118.23345-1-kabel@kernel.org/
  https://patchwork.kernel.org/project/linux-soc/cover/20240510101819.13551-1-kabel@kernel.org/

Marek Behún (8):
  dt-bindings: firmware: add cznic,turris-omnia-mcu binding
  platform: cznic: Add preliminary support for Turris Omnia MCU
  platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
  platform: cznic: turris-omnia-mcu: Add support for poweroff and wakeup
  platform: cznic: turris-omnia-mcu: Add support for MCU watchdog
  platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG
  ARM: dts: turris-omnia: Add MCU system-controller node
  ARM: dts: turris-omnia: Add GPIO key node for front button

 .../sysfs-bus-i2c-devices-turris-omnia-mcu    |  113 ++
 .../firmware/cznic,turris-omnia-mcu.yaml      |   86 ++
 MAINTAINERS                                   |    4 +
 .../dts/marvell/armada-385-turris-omnia.dts   |   35 +-
 drivers/platform/Kconfig                      |    2 +
 drivers/platform/Makefile                     |    1 +
 drivers/platform/cznic/Kconfig                |   48 +
 drivers/platform/cznic/Makefile               |    8 +
 .../platform/cznic/turris-omnia-mcu-base.c    |  407 +++++++
 .../platform/cznic/turris-omnia-mcu-gpio.c    | 1071 +++++++++++++++++
 .../cznic/turris-omnia-mcu-sys-off-wakeup.c   |  257 ++++
 .../platform/cznic/turris-omnia-mcu-trng.c    |  103 ++
 .../cznic/turris-omnia-mcu-watchdog.c         |  128 ++
 drivers/platform/cznic/turris-omnia-mcu.h     |  194 +++
 include/linux/turris-omnia-mcu-interface.h    |  249 ++++
 15 files changed, 2705 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
 create mode 100644 Documentation/devicetree/bindings/firmware/cznic,turris-omnia-mcu.yaml
 create mode 100644 drivers/platform/cznic/Kconfig
 create mode 100644 drivers/platform/cznic/Makefile
 create mode 100644 drivers/platform/cznic/turris-omnia-mcu-base.c
 create mode 100644 drivers/platform/cznic/turris-omnia-mcu-gpio.c
 create mode 100644 drivers/platform/cznic/turris-omnia-mcu-sys-off-wakeup.c
 create mode 100644 drivers/platform/cznic/turris-omnia-mcu-trng.c
 create mode 100644 drivers/platform/cznic/turris-omnia-mcu-watchdog.c
 create mode 100644 drivers/platform/cznic/turris-omnia-mcu.h
 create mode 100644 include/linux/turris-omnia-mcu-interface.h

-- 
2.44.2

[PATCH v11 6/8] platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG

[Marek Behún]

Add support for true random number generator provided by the MCU.
New Omnia boards come without the Atmel SHA204-A chip. Instead the
crypto functionality is provided by new microcontroller, which has
a TRNG peripheral.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/platform/cznic/Kconfig                |   2 +
 drivers/platform/cznic/Makefile               |   1 +
 .../platform/cznic/turris-omnia-mcu-base.c    |   6 +-
 .../platform/cznic/turris-omnia-mcu-gpio.c    |   2 +-
 .../platform/cznic/turris-omnia-mcu-trng.c    | 103 ++++++++++++++++++
 drivers/platform/cznic/turris-omnia-mcu.h     |   8 ++
 6 files changed, 120 insertions(+), 2 deletions(-)
 create mode 100644 drivers/platform/cznic/turris-omnia-mcu-trng.c

diff --git a/drivers/platform/cznic/Kconfig b/drivers/platform/cznic/Kconfig
index e262930b3faf..6edac80d5fa3 100644
--- a/drivers/platform/cznic/Kconfig
+++ b/drivers/platform/cznic/Kconfig
@@ -18,6 +18,7 @@ config TURRIS_OMNIA_MCU
 	depends on I2C
 	select GPIOLIB
 	select GPIOLIB_IRQCHIP
+	select HW_RANDOM
 	select RTC_CLASS
 	select WATCHDOG_CORE
 	help
@@ -27,6 +28,7 @@ config TURRIS_OMNIA_MCU
 	  - board poweroff into true low power mode (with voltage regulators
 	    disabled) and the ability to configure wake up from this mode (via
 	    rtcwake)
+	  - true random number generator (if available on the MCU)
 	  - MCU watchdog
 	  - GPIO pins
 	    - to get front button press events (the front button can be
diff --git a/drivers/platform/cznic/Makefile b/drivers/platform/cznic/Makefile
index 687f7718c0a1..eae4c6b341ff 100644
--- a/drivers/platform/cznic/Makefile
+++ b/drivers/platform/cznic/Makefile
@@ -4,4 +4,5 @@ obj-$(CONFIG_TURRIS_OMNIA_MCU)	+= turris-omnia-mcu.o
 turris-omnia-mcu-y		:= turris-omnia-mcu-base.o
 turris-omnia-mcu-y		+= turris-omnia-mcu-gpio.o
 turris-omnia-mcu-y		+= turris-omnia-mcu-sys-off-wakeup.o
+turris-omnia-mcu-y		+= turris-omnia-mcu-trng.o
 turris-omnia-mcu-y		+= turris-omnia-mcu-watchdog.o
diff --git a/drivers/platform/cznic/turris-omnia-mcu-base.c b/drivers/platform/cznic/turris-omnia-mcu-base.c
index f44996588d38..1d4153a96526 100644
--- a/drivers/platform/cznic/turris-omnia-mcu-base.c
+++ b/drivers/platform/cznic/turris-omnia-mcu-base.c
@@ -380,7 +380,11 @@ static int omnia_mcu_probe(struct i2c_client *client)
 	if (err)
 		return err;
 
-	return omnia_mcu_register_gpiochip(mcu);
+	err = omnia_mcu_register_gpiochip(mcu);
+	if (err)
+		return err;
+
+	return omnia_mcu_register_trng(mcu);
 }
 
 static const struct of_device_id of_omnia_mcu_match[] = {
diff --git a/drivers/platform/cznic/turris-omnia-mcu-gpio.c b/drivers/platform/cznic/turris-omnia-mcu-gpio.c
index bc7965e6c879..972364d3d223 100644
--- a/drivers/platform/cznic/turris-omnia-mcu-gpio.c
+++ b/drivers/platform/cznic/turris-omnia-mcu-gpio.c
@@ -174,7 +174,7 @@ static const struct omnia_gpio omnia_gpios[64] = {
 };
 
 /* mapping from interrupts to indexes of GPIOs in the omnia_gpios array */
-static const u8 omnia_int_to_gpio_idx[32] = {
+const u8 omnia_int_to_gpio_idx[32] = {
 	[__bf_shf(OMNIA_INT_CARD_DET)]			= 4,
 	[__bf_shf(OMNIA_INT_MSATA_IND)]			= 5,
 	[__bf_shf(OMNIA_INT_USB30_OVC)]			= 6,
diff --git a/drivers/platform/cznic/turris-omnia-mcu-trng.c b/drivers/platform/cznic/turris-omnia-mcu-trng.c
new file mode 100644
index 000000000000..fbde00f3fca1
--- /dev/null
+++ b/drivers/platform/cznic/turris-omnia-mcu-trng.c
@@ -0,0 +1,103 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CZ.NIC's Turris Omnia MCU TRNG driver
+ *
+ * 2024 by Marek Behún <kabel@kernel.org>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/completion.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/hw_random.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/string.h>
+#include <linux/turris-omnia-mcu-interface.h>
+#include <linux/types.h>
+
+#include "turris-omnia-mcu.h"
+
+#define OMNIA_CMD_TRNG_MAX_ENTROPY_LEN	64
+
+static irqreturn_t omnia_trng_irq_handler(int irq, void *dev_id)
+{
+	struct omnia_mcu *mcu = dev_id;
+
+	complete(&mcu->trng_completion);
+
+	return IRQ_HANDLED;
+}
+
+static int omnia_trng_read(struct hwrng *rng, void *data, size_t max, bool wait)
+{
+	struct omnia_mcu *mcu = (struct omnia_mcu *)rng->priv;
+	u8 reply[1 + OMNIA_CMD_TRNG_MAX_ENTROPY_LEN];
+	int err, bytes;
+
+	if (!wait && !completion_done(&mcu->trng_completion))
+		return 0;
+
+	do {
+		if (wait_for_completion_interruptible(&mcu->trng_completion))
+			return -ERESTARTSYS;
+
+		err = omnia_cmd_read(mcu->client,
+				     OMNIA_CMD_TRNG_COLLECT_ENTROPY,
+				     reply, sizeof(reply));
+		if (err)
+			return err;
+
+		bytes = min3(reply[0], max, OMNIA_CMD_TRNG_MAX_ENTROPY_LEN);
+	} while (wait && !bytes);
+
+	memcpy(data, &reply[1], bytes);
+
+	return bytes;
+}
+
+int omnia_mcu_register_trng(struct omnia_mcu *mcu)
+{
+	struct device *dev = &mcu->client->dev;
+	u8 irq_idx, dummy;
+	int irq, err;
+
+	if (!(mcu->features & OMNIA_FEAT_TRNG))
+		return 0;
+
+	irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_INT_TRNG)];
+	irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
+	if (irq < 0)
+		return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n");
+
+	/*
+	 * If someone else cleared the TRNG interrupt but did not read the
+	 * entropy, a new interrupt won't be generated, and entropy collection
+	 * will be stuck. Ensure an interrupt will be generated by executing
+	 * the collect entropy command (and discarding the result).
+	 */
+	err = omnia_cmd_read(mcu->client, OMNIA_CMD_TRNG_COLLECT_ENTROPY,
+			     &dummy, 1);
+	if (err)
+		return err;
+
+	init_completion(&mcu->trng_completion);
+
+	err = devm_request_threaded_irq(dev, irq, NULL, omnia_trng_irq_handler,
+					IRQF_ONESHOT, "turris-omnia-mcu-trng",
+					mcu);
+	if (err)
+		return dev_err_probe(dev, err, "Cannot request TRNG IRQ\n");
+
+	mcu->trng.name = "turris-omnia-mcu-trng";
+	mcu->trng.read = omnia_trng_read;
+	mcu->trng.priv = (unsigned long)mcu;
+
+	err = devm_hwrng_register(dev, &mcu->trng);
+	if (err)
+		return dev_err_probe(dev, err, "Cannot register TRNG\n");
+
+	return 0;
+}
diff --git a/drivers/platform/cznic/turris-omnia-mcu.h b/drivers/platform/cznic/turris-omnia-mcu.h
index 5f846909934f..ee8d58516156 100644
--- a/drivers/platform/cznic/turris-omnia-mcu.h
+++ b/drivers/platform/cznic/turris-omnia-mcu.h
@@ -9,7 +9,9 @@
 #define __TURRIS_OMNIA_MCU_H
 
 #include <linux/bitops.h>
+#include <linux/completion.h>
 #include <linux/gpio/driver.h>
+#include <linux/hw_random.h>
 #include <linux/if_ether.h>
 #include <linux/mutex.h>
 #include <linux/rtc.h>
@@ -47,6 +49,10 @@ struct omnia_mcu {
 
 	/* MCU watchdog */
 	struct watchdog_device wdt;
+
+	/* true random number generator */
+	struct hwrng trng;
+	struct completion trng_completion;
 };
 
 int omnia_cmd_write_read(const struct i2c_client *client,
@@ -176,11 +182,13 @@ static inline int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd,
 	return omnia_cmd_read(client, cmd, reply, sizeof(*reply));
 }
 
+extern const u8 omnia_int_to_gpio_idx[32];
 extern const struct attribute_group omnia_mcu_gpio_group;
 extern const struct attribute_group omnia_mcu_poweroff_group;
 
 int omnia_mcu_register_gpiochip(struct omnia_mcu *mcu);
 int omnia_mcu_register_sys_off_and_wakeup(struct omnia_mcu *mcu);
+int omnia_mcu_register_trng(struct omnia_mcu *mcu);
 int omnia_mcu_register_watchdog(struct omnia_mcu *mcu);
 
 #endif /* __TURRIS_OMNIA_MCU_H */
-- 
2.44.2

Re: [PATCH v11 6/8] platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG

[Andy Shevchenko]

On Wed, Jun 5, 2024 at 7:19 PM Marek Behún <kabel@kernel.org> wrote:
>
> Add support for true random number generator provided by the MCU.
> New Omnia boards come without the Atmel SHA204-A chip. Instead the
> crypto functionality is provided by new microcontroller, which has
> a TRNG peripheral.

+Cc: Bart for gpiochip_get_desc() usage.

...

> +#include <linux/bitfield.h>
> +#include <linux/completion.h>

+ errno.h

> +#include <linux/gpio/consumer.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/hw_random.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/minmax.h>
> +#include <linux/module.h>
> +#include <linux/string.h>

> +#include <linux/turris-omnia-mcu-interface.h>

As per other patches.

> +#include <linux/types.h>
> +
> +#include "turris-omnia-mcu.h"

...

> +       irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_INT_TRNG)];
> +       irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
> +       if (irq < 0)
> +               return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n");

Okay, it's a bit more complicated than that. The gpiochip_get_desc()
shouldn't be used. Bart, what can you suggest to do here? Opencoding
it doesn't sound to me a (fully) correct approach in a long term.

--
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 0/8] Turris Omnia MCU driver

[Andy Shevchenko]

On Wed, Jun 5, 2024 at 7:19 PM Marek Behún <kabel@kernel.org> wrote:
>
> Hello Andy, Hans, Ilpo, Arnd, Gregory, and others,
>
> this is v11 of the series adding Turris Omnia MCU driver.

Thank you!
There are a few small issues here and there, but overall LGTM. The
only one main question is what to do with gpiochip_get_desc(). I Cc'ed
Bart for this.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 0/8] Turris Omnia MCU driver

[Marek Behún]

On Wed, 5 Jun 2024 22:05:37 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Jun 5, 2024 at 7:19 PM Marek Behún <kabel@kernel.org> wrote:
> >
> > Hello Andy, Hans, Ilpo, Arnd, Gregory, and others,
> >
> > this is v11 of the series adding Turris Omnia MCU driver.  
> 
> Thank you!
> There are a few small issues here and there, but overall LGTM. The
> only one main question is what to do with gpiochip_get_desc(). I Cc'ed
> Bart for this.

Thank you for the review, I am going to apply the changes you requested
and wait for Bart, and we'll see what to do with the
gpiochip_get_desc().

Marek

Re: [PATCH v11 6/8] platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG

[Marek Behún]

On Wed, 5 Jun 2024 22:00:20 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> > +       irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_INT_TRNG)];
> > +       irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
> > +       if (irq < 0)
> > +               return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n");  
> 
> Okay, it's a bit more complicated than that. The gpiochip_get_desc()
> shouldn't be used. Bart, what can you suggest to do here? Opencoding
> it doesn't sound to me a (fully) correct approach in a long term.

Note that I can't use gpiochip_request_own_desc(), nor any other
function that calls gpio_request_commit() (like gpiod_get()), because
that checks for gpiochip_line_is_valid(), and this returns false for
the TRNG line, cause that line is not a GPIO line, but interrupt only
line.

That is why I used
  irq = irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx);
until v7, with no reference to gpio descriptors, since this line is not
a GPIO line.

We have discussed this back in April, in the thread
  https://lore.kernel.org/soc/20240418121116.22184-8-kabel@kernel.org/
where we concluded that
  irq = gpiod_to_irq(gpiochip_get_desc(gc, irq_idx));
is better...

Marek

Re: [PATCH v11 6/8] platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG

[Marek Behún]

On Wed, 5 Jun 2024 22:00:20 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> > +#include <linux/bitfield.h>
> > +#include <linux/completion.h>  
> 
> + errno.h

I use -EIO, -EINVAL and -ENOMEM in turris-omnia-mcu-base.c,
-EINVAL, -ENOTSUPP in turris-omnia-mcu-gpio.c.

Should I include errno.h in those also? Or is this only needed for
-ERESTARTSYS?

Marek

Re: [PATCH v11 6/8] platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG

[Andy Shevchenko]

On Thu, Jun 06, 2024 at 11:11:13AM +0200, Marek Behún wrote:
> On Wed, 5 Jun 2024 22:00:20 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> > > +#include <linux/bitfield.h>
> > > +#include <linux/completion.h>
> > 
> > + errno.h
> 
> I use -EIO, -EINVAL and -ENOMEM in turris-omnia-mcu-base.c,
> -EINVAL, -ENOTSUPP in turris-omnia-mcu-gpio.c.

> Should I include errno.h in those also?

If you have err.h, then no (it includes asm/errno.h), otherwise, yes.

> Or is this only needed for -ERESTARTSYS?

Definitely yes for Linux internal error codes (>= 512).
Note, that ENOTSUPP is also Linux internal code.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 6/8] platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG

[Andy Shevchenko]

On Thu, Jun 06, 2024 at 10:53:08AM +0200, Marek Behún wrote:
> On Wed, 5 Jun 2024 22:00:20 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> > > +       irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_INT_TRNG)];
> > > +       irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
> > > +       if (irq < 0)
> > > +               return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n");  
> > 
> > Okay, it's a bit more complicated than that. The gpiochip_get_desc()
> > shouldn't be used. Bart, what can you suggest to do here? Opencoding
> > it doesn't sound to me a (fully) correct approach in a long term.
> 
> Note that I can't use gpiochip_request_own_desc(), nor any other
> function that calls gpio_request_commit() (like gpiod_get()), because
> that checks for gpiochip_line_is_valid(), and this returns false for
> the TRNG line, cause that line is not a GPIO line, but interrupt only
> line.
> 
> That is why I used
>   irq = irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx);
> until v7, with no reference to gpio descriptors, since this line is not
> a GPIO line.
> 
> We have discussed this back in April, in the thread
>   https://lore.kernel.org/soc/20240418121116.22184-8-kabel@kernel.org/
> where we concluded that
>   irq = gpiod_to_irq(gpiochip_get_desc(gc, irq_idx));
> is better...

That's fine to not use other APIs, the problem here is with reference counting
on the GPIO device. The API you could use is gpio_device_get_desc(). But you
need to have a GPIO device pointer somewhere in your driver being available.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 6/8] platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG

[Marek Behún]

On Thu, 6 Jun 2024 13:11:09 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Jun 06, 2024 at 10:53:08AM +0200, Marek Behún wrote:
> > On Wed, 5 Jun 2024 22:00:20 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >   
> > > > +       irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_INT_TRNG)];
> > > > +       irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
> > > > +       if (irq < 0)
> > > > +               return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n");    
> > > 
> > > Okay, it's a bit more complicated than that. The gpiochip_get_desc()
> > > shouldn't be used. Bart, what can you suggest to do here? Opencoding
> > > it doesn't sound to me a (fully) correct approach in a long term.  
> > 
> > Note that I can't use gpiochip_request_own_desc(), nor any other
> > function that calls gpio_request_commit() (like gpiod_get()), because
> > that checks for gpiochip_line_is_valid(), and this returns false for
> > the TRNG line, cause that line is not a GPIO line, but interrupt only
> > line.
> > 
> > That is why I used
> >   irq = irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx);
> > until v7, with no reference to gpio descriptors, since this line is not
> > a GPIO line.
> > 
> > We have discussed this back in April, in the thread
> >   https://lore.kernel.org/soc/20240418121116.22184-8-kabel@kernel.org/
> > where we concluded that
> >   irq = gpiod_to_irq(gpiochip_get_desc(gc, irq_idx));
> > is better...  
> 
> That's fine to not use other APIs, the problem here is with reference counting
> on the GPIO device. The API you could use is gpio_device_get_desc(). But you
> need to have a GPIO device pointer somewhere in your driver being available.

Rewriting to gpio_device_get_desc() is simple, since
  gpiochip_get_desc(gc, hwnum)
is equivalent to
  gpio_device_get_desc(gc->gpiodev, hwnum)

Obviously neither of these take care of reference counting. But what
reference counting are you talking about?
Is it the
  try_module_get(desc->gdev->owner)
in gpiod_request()?
Or are we talking only about the FLAG_REQUESTED flag on the descriptor
flags? (This one should not be needed since the GPIO line cannot be
requested, becuase it is not a valid GPIO line.)

Since the line is not a valid GPIO line, I thought that we don't need
to refcount in GPIO code. gpiod_to_irq() will call the gpiochip's
.to_irq() method, which will call gpiochip_to_irq(), which will call
irq_create_mapping()
  gpiod_to_irq()
    gpiochip_to_irq()
      irq_create_mapping()

Then on gpiochip removal, the gpiochip_irqchip_remove() manually
disposes all IRQ mappings with irq_dispose_mapping().

Marek

Re: [PATCH v11 6/8] platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG

[Herbert Xu]

On Wed, Jun 05, 2024 at 06:18:49PM +0200, Marek Behún wrote:
>
> +static int omnia_trng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> +{
> +	struct omnia_mcu *mcu = (struct omnia_mcu *)rng->priv;

Please don't cast rng->priv in this manner.  Please take a look at
drivers/char/hw_random/bcm2835-rng.c for how it should be done.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v11 6/8] platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG

[Marek Behún]

On Fri, 7 Jun 2024 18:30:24 +0800
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Wed, Jun 05, 2024 at 06:18:49PM +0200, Marek Behún wrote:
> >
> > +static int omnia_trng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> > +{
> > +	struct omnia_mcu *mcu = (struct omnia_mcu *)rng->priv;  
> 
> Please don't cast rng->priv in this manner.  Please take a look at
> drivers/char/hw_random/bcm2835-rng.c for how it should be done.
> 
> Thanks,

THX, prepared for next version.

Re: [PATCH v11 6/8] platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG

[Bartosz Golaszewski]

On Wed, Jun 5, 2024 at 9:00 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Jun 5, 2024 at 7:19 PM Marek Behún <kabel@kernel.org> wrote:
> >
> > Add support for true random number generator provided by the MCU.
> > New Omnia boards come without the Atmel SHA204-A chip. Instead the
> > crypto functionality is provided by new microcontroller, which has
> > a TRNG peripheral.
>
> +Cc: Bart for gpiochip_get_desc() usage.
>
> ...
>
> > +#include <linux/bitfield.h>
> > +#include <linux/completion.h>
>
> + errno.h
>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/gpio/driver.h>
> > +#include <linux/hw_random.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/minmax.h>
> > +#include <linux/module.h>
> > +#include <linux/string.h>
>
> > +#include <linux/turris-omnia-mcu-interface.h>
>
> As per other patches.
>
> > +#include <linux/types.h>
> > +
> > +#include "turris-omnia-mcu.h"
>
> ...
>
> > +       irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_INT_TRNG)];
> > +       irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
> > +       if (irq < 0)
> > +               return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n");
>
> Okay, it's a bit more complicated than that. The gpiochip_get_desc()
> shouldn't be used. Bart, what can you suggest to do here? Opencoding
> it doesn't sound to me a (fully) correct approach in a long term.
>

Andy's worried about reference counting of the GPIO device. Maybe you
should just ref the GPIO device in irq_request_resources() and unref
it in irq_release_resources()? Then you could use gpiochip_get_desc()
just fine.

Bart

Re: [PATCH v11 6/8] platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG

[Marek Behún]

On Mon, 17 Jun 2024 10:38:31 +0200
Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> On Wed, Jun 5, 2024 at 9:00 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Wed, Jun 5, 2024 at 7:19 PM Marek Behún <kabel@kernel.org> wrote:  
> > >
> > > Add support for true random number generator provided by the MCU.
> > > New Omnia boards come without the Atmel SHA204-A chip. Instead the
> > > crypto functionality is provided by new microcontroller, which has
> > > a TRNG peripheral.  
> >
> > +Cc: Bart for gpiochip_get_desc() usage.
> >
> > ...
> >  
> > > +#include <linux/bitfield.h>
> > > +#include <linux/completion.h>  
> >
> > + errno.h
> >  
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/gpio/driver.h>
> > > +#include <linux/hw_random.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/minmax.h>
> > > +#include <linux/module.h>
> > > +#include <linux/string.h>  
> >  
> > > +#include <linux/turris-omnia-mcu-interface.h>  
> >
> > As per other patches.
> >  
> > > +#include <linux/types.h>
> > > +
> > > +#include "turris-omnia-mcu.h"  
> >
> > ...
> >  
> > > +       irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_INT_TRNG)];
> > > +       irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
> > > +       if (irq < 0)
> > > +               return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n");  
> >
> > Okay, it's a bit more complicated than that. The gpiochip_get_desc()
> > shouldn't be used. Bart, what can you suggest to do here? Opencoding
> > it doesn't sound to me a (fully) correct approach in a long term.
> >  
> 
> Andy's worried about reference counting of the GPIO device. Maybe you
> should just ref the GPIO device in irq_request_resources() and unref
> it in irq_release_resources()? Then you could use gpiochip_get_desc()
> just fine.

But this is already being done.

The irqchip uses GPIOCHIP_IRQ_RESOURCE_HELPERS macro in its definition:

  static const struct irq_chip omnia_mcu_irq_chip = {
    ...
    GPIOCHIP_IRQ_RESOURCE_HELPERS,
  };

This means that gpiochip_irq_reqres() and gpiochip_irq_relres() are
used as irq_request_resources() and irq_release_resources().

The gpiochip_reqres_irq() code increases the module refcount and even
locks the line as IRQ:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.10-rc4#n3732

Marek

Re: [PATCH v11 6/8] platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG

[Bartosz Golaszewski]

On Mon, Jun 17, 2024 at 10:56 AM Marek Behún <kabel@kernel.org> wrote:
>
> On Mon, 17 Jun 2024 10:38:31 +0200
> Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > On Wed, Jun 5, 2024 at 9:00 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > >
> > > On Wed, Jun 5, 2024 at 7:19 PM Marek Behún <kabel@kernel.org> wrote:
> > > >
> > > > Add support for true random number generator provided by the MCU.
> > > > New Omnia boards come without the Atmel SHA204-A chip. Instead the
> > > > crypto functionality is provided by new microcontroller, which has
> > > > a TRNG peripheral.
> > >
> > > +Cc: Bart for gpiochip_get_desc() usage.
> > >
> > > ...
> > >
> > > > +#include <linux/bitfield.h>
> > > > +#include <linux/completion.h>
> > >
> > > + errno.h
> > >
> > > > +#include <linux/gpio/consumer.h>
> > > > +#include <linux/gpio/driver.h>
> > > > +#include <linux/hw_random.h>
> > > > +#include <linux/i2c.h>
> > > > +#include <linux/interrupt.h>
> > > > +#include <linux/minmax.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/string.h>
> > >
> > > > +#include <linux/turris-omnia-mcu-interface.h>
> > >
> > > As per other patches.
> > >
> > > > +#include <linux/types.h>
> > > > +
> > > > +#include "turris-omnia-mcu.h"
> > >
> > > ...
> > >
> > > > +       irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_INT_TRNG)];
> > > > +       irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
> > > > +       if (irq < 0)
> > > > +               return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n");
> > >
> > > Okay, it's a bit more complicated than that. The gpiochip_get_desc()
> > > shouldn't be used. Bart, what can you suggest to do here? Opencoding
> > > it doesn't sound to me a (fully) correct approach in a long term.
> > >
> >
> > Andy's worried about reference counting of the GPIO device. Maybe you
> > should just ref the GPIO device in irq_request_resources() and unref
> > it in irq_release_resources()? Then you could use gpiochip_get_desc()
> > just fine.
>
> But this is already being done.
>
> The irqchip uses GPIOCHIP_IRQ_RESOURCE_HELPERS macro in its definition:
>
>   static const struct irq_chip omnia_mcu_irq_chip = {
>     ...
>     GPIOCHIP_IRQ_RESOURCE_HELPERS,
>   };
>
> This means that gpiochip_irq_reqres() and gpiochip_irq_relres() are
> used as irq_request_resources() and irq_release_resources().
>
> The gpiochip_reqres_irq() code increases the module refcount and even
> locks the line as IRQ:
>
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.10-rc4#n3732
>
> Marek

Andy: what is the issue here then exactly?

Bart

Re: [PATCH v11 6/8] platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG

[Andy Shevchenko]

On Mon, Jun 17, 2024 at 11:07 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Mon, Jun 17, 2024 at 10:56 AM Marek Behún <kabel@kernel.org> wrote:
> > On Mon, 17 Jun 2024 10:38:31 +0200
> > Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > On Wed, Jun 5, 2024 at 9:00 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Wed, Jun 5, 2024 at 7:19 PM Marek Behún <kabel@kernel.org> wrote:
> > > > >
> > > > > Add support for true random number generator provided by the MCU.
> > > > > New Omnia boards come without the Atmel SHA204-A chip. Instead the
> > > > > crypto functionality is provided by new microcontroller, which has
> > > > > a TRNG peripheral.
> > > >
> > > > +Cc: Bart for gpiochip_get_desc() usage.

...

> > > > > +       irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_INT_TRNG)];
> > > > > +       irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
> > > > > +       if (irq < 0)
> > > > > +               return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n");
> > > >
> > > > Okay, it's a bit more complicated than that. The gpiochip_get_desc()
> > > > shouldn't be used. Bart, what can you suggest to do here? Opencoding
> > > > it doesn't sound to me a (fully) correct approach in a long term.
> > >
> > > Andy's worried about reference counting of the GPIO device. Maybe you
> > > should just ref the GPIO device in irq_request_resources() and unref
> > > it in irq_release_resources()? Then you could use gpiochip_get_desc()
> > > just fine.
> >
> > But this is already being done.
> >
> > The irqchip uses GPIOCHIP_IRQ_RESOURCE_HELPERS macro in its definition:
> >
> >   static const struct irq_chip omnia_mcu_irq_chip = {
> >     ...
> >     GPIOCHIP_IRQ_RESOURCE_HELPERS,
> >   };
> >
> > This means that gpiochip_irq_reqres() and gpiochip_irq_relres() are
> > used as irq_request_resources() and irq_release_resources().
> >
> > The gpiochip_reqres_irq() code increases the module refcount and even
> > locks the line as IRQ:
> >
> >   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.10-rc4#n3732
>
> Andy: what is the issue here then exactly?

The function in use is marked as DEPRECATED. If you are fine with its
usage in this case, I have no issues with it.
If you want it to be replaced with the respective
gpio_device_get_desc(), it's fine, but then the question is how
properly get a pointer to GPIO device object.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 6/8] platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG

[Marek Behún]

On Mon, 17 Jun 2024 12:42:41 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Jun 17, 2024 at 11:07 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > On Mon, Jun 17, 2024 at 10:56 AM Marek Behún <kabel@kernel.org> wrote:  
> > > On Mon, 17 Jun 2024 10:38:31 +0200
> > > Bartosz Golaszewski <brgl@bgdev.pl> wrote:  
> > > > On Wed, Jun 5, 2024 at 9:00 PM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:  
> > > > > On Wed, Jun 5, 2024 at 7:19 PM Marek Behún <kabel@kernel.org> wrote:  
> > > > > >
> > > > > > Add support for true random number generator provided by the MCU.
> > > > > > New Omnia boards come without the Atmel SHA204-A chip. Instead the
> > > > > > crypto functionality is provided by new microcontroller, which has
> > > > > > a TRNG peripheral.  
> > > > >
> > > > > +Cc: Bart for gpiochip_get_desc() usage.  
> 
> ...
> 
> > > > > > +       irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_INT_TRNG)];
> > > > > > +       irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
> > > > > > +       if (irq < 0)
> > > > > > +               return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n");  
> > > > >
> > > > > Okay, it's a bit more complicated than that. The gpiochip_get_desc()
> > > > > shouldn't be used. Bart, what can you suggest to do here? Opencoding
> > > > > it doesn't sound to me a (fully) correct approach in a long term.  
> > > >
> > > > Andy's worried about reference counting of the GPIO device. Maybe you
> > > > should just ref the GPIO device in irq_request_resources() and unref
> > > > it in irq_release_resources()? Then you could use gpiochip_get_desc()
> > > > just fine.  
> > >
> > > But this is already being done.
> > >
> > > The irqchip uses GPIOCHIP_IRQ_RESOURCE_HELPERS macro in its definition:
> > >
> > >   static const struct irq_chip omnia_mcu_irq_chip = {
> > >     ...
> > >     GPIOCHIP_IRQ_RESOURCE_HELPERS,
> > >   };
> > >
> > > This means that gpiochip_irq_reqres() and gpiochip_irq_relres() are
> > > used as irq_request_resources() and irq_release_resources().
> > >
> > > The gpiochip_reqres_irq() code increases the module refcount and even
> > > locks the line as IRQ:
> > >
> > >   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.10-rc4#n3732  
> >
> > Andy: what is the issue here then exactly?  
> 
> The function in use is marked as DEPRECATED. If you are fine with its
> usage in this case, I have no issues with it.
> If you want it to be replaced with the respective
> gpio_device_get_desc(), it's fine, but then the question is how
> properly get a pointer to GPIO device object.
> 

Aha, I did not notice that the function is deprecated.

What about

  irq = gpiod_to_irq(gpio_device_get_desc(mcu->gc.gpiodev, irq_idx));

?

Note: I would prefer
  irq_create_mapping(mcu->gc.irq.domain, irq_idx)
since the irq_idx line is not a valid GPIO line and at this part of
the driver the fact that the IRQs are provided through a gpiochip are
semantically irrelevant (we are interested in "an IRQ", not "an IRQ from
a GPIO").

Marek

Re: [PATCH v11 6/8] platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG

[Bartosz Golaszewski]

On Mon, Jun 17, 2024 at 1:34 PM Marek Behún <kabel@kernel.org> wrote:
>
> On Mon, 17 Jun 2024 12:42:41 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> > On Mon, Jun 17, 2024 at 11:07 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > On Mon, Jun 17, 2024 at 10:56 AM Marek Behún <kabel@kernel.org> wrote:
> > > > On Mon, 17 Jun 2024 10:38:31 +0200
> > > > Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > > On Wed, Jun 5, 2024 at 9:00 PM Andy Shevchenko
> > > > > <andy.shevchenko@gmail.com> wrote:
> > > > > > On Wed, Jun 5, 2024 at 7:19 PM Marek Behún <kabel@kernel.org> wrote:
> > > > > > >
> > > > > > > Add support for true random number generator provided by the MCU.
> > > > > > > New Omnia boards come without the Atmel SHA204-A chip. Instead the
> > > > > > > crypto functionality is provided by new microcontroller, which has
> > > > > > > a TRNG peripheral.
> > > > > >
> > > > > > +Cc: Bart for gpiochip_get_desc() usage.
> >
> > ...
> >
> > > > > > > +       irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_INT_TRNG)];
> > > > > > > +       irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
> > > > > > > +       if (irq < 0)
> > > > > > > +               return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n");
> > > > > >
> > > > > > Okay, it's a bit more complicated than that. The gpiochip_get_desc()
> > > > > > shouldn't be used. Bart, what can you suggest to do here? Opencoding
> > > > > > it doesn't sound to me a (fully) correct approach in a long term.
> > > > >
> > > > > Andy's worried about reference counting of the GPIO device. Maybe you
> > > > > should just ref the GPIO device in irq_request_resources() and unref
> > > > > it in irq_release_resources()? Then you could use gpiochip_get_desc()
> > > > > just fine.
> > > >
> > > > But this is already being done.
> > > >
> > > > The irqchip uses GPIOCHIP_IRQ_RESOURCE_HELPERS macro in its definition:
> > > >
> > > >   static const struct irq_chip omnia_mcu_irq_chip = {
> > > >     ...
> > > >     GPIOCHIP_IRQ_RESOURCE_HELPERS,
> > > >   };
> > > >
> > > > This means that gpiochip_irq_reqres() and gpiochip_irq_relres() are
> > > > used as irq_request_resources() and irq_release_resources().
> > > >
> > > > The gpiochip_reqres_irq() code increases the module refcount and even
> > > > locks the line as IRQ:
> > > >
> > > >   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.10-rc4#n3732
> > >
> > > Andy: what is the issue here then exactly?
> >
> > The function in use is marked as DEPRECATED. If you are fine with its
> > usage in this case, I have no issues with it.
> > If you want it to be replaced with the respective
> > gpio_device_get_desc(), it's fine, but then the question is how
> > properly get a pointer to GPIO device object.
> >
>
> Aha, I did not notice that the function is deprecated.
>
> What about
>
>   irq = gpiod_to_irq(gpio_device_get_desc(mcu->gc.gpiodev, irq_idx));
>
> ?
>
> Note: I would prefer
>   irq_create_mapping(mcu->gc.irq.domain, irq_idx)
> since the irq_idx line is not a valid GPIO line and at this part of
> the driver the fact that the IRQs are provided through a gpiochip are
> semantically irrelevant (we are interested in "an IRQ", not "an IRQ from
> a GPIO").
>
> Marek

The reason to deprecate it was the fact that it's dangerous to use
from outside of the GPIO provider code. I actually plan to soon make
this function private to gpiolib, there's just one pinctrl driver left
to convert.

So maybe it's better to not use it here. Please keep in mind that
gpio_device_get_desc() doesn't increase the reference count of
gpio_device so you need to make sure it stays alive. But it seems to
be the case here as you're within the driver code still.

Bart