diff mbox series

[v6,07/11] platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG

Message ID 20240418121116.22184-8-kabel@kernel.org (mailing list archive)
State Superseded
Delegated to: Arnd Bergmann
Headers show
Series Turris Omnia MCU driver | expand

Commit Message

Marek Behún April 18, 2024, 12:11 p.m. UTC 
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    | 89 +++++++++++++++++++
 drivers/platform/cznic/turris-omnia-mcu.h     |  8 ++
 6 files changed, 106 insertions(+), 2 deletions(-)
 create mode 100644 drivers/platform/cznic/turris-omnia-mcu-trng.c

Comments

Andy Shevchenko April 23, 2024, 3:58 p.m. UTC | #1 
On Thu, Apr 18, 2024 at 02:11:12PM +0200, Marek Behún 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.

...

> +int omnia_mcu_register_trng(struct omnia_mcu *mcu)
> +{
> +	struct device *dev = &mcu->client->dev;
> +	int irq, err;
> +	u8 irq_idx;
> +
> +	if (!(mcu->features & FEAT_TRNG))
> +		return 0;

> +	irq_idx = omnia_int_to_gpio_idx[__bf_shf(INT_TRNG)];
> +	irq = devm_irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx);
> +	if (irq < 0)
> +		return dev_err_probe(dev, irq, "Cannot map TRNG IRQ\n");

This looks like some workaround against existing gpiod_to_irq(). Why do you
need this?

> +	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;
> +}
Marek Behún April 23, 2024, 4:32 p.m. UTC | #2 
On Tue, 23 Apr 2024 18:58:19 +0300
Andy Shevchenko <andy@kernel.org> wrote:

> On Thu, Apr 18, 2024 at 02:11:12PM +0200, Marek Behún 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.  
> 
> ...
> 
> > +int omnia_mcu_register_trng(struct omnia_mcu *mcu)
> > +{
> > +	struct device *dev = &mcu->client->dev;
> > +	int irq, err;
> > +	u8 irq_idx;
> > +
> > +	if (!(mcu->features & FEAT_TRNG))
> > +		return 0;  
> 
> > +	irq_idx = omnia_int_to_gpio_idx[__bf_shf(INT_TRNG)];
> > +	irq = devm_irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx);
> > +	if (irq < 0)
> > +		return dev_err_probe(dev, irq, "Cannot map TRNG IRQ\n");  
> 
> This looks like some workaround against existing gpiod_to_irq(). Why do you
> need this?

Hmmm, I thought that would not work because that line is only valid
as an IRQ, not as a GPIO (this is enforced via the valid_mask member of
gpio_chip and gpio_irq_chip).

But looking at the code of gpiolib, if I do
  irq = gpiod_to_irq(gpiochip_get_desc(gc, irq_idx));
the valid_mask is not enforced anywhere.

Is this semantically right to do even in spite of the fact that the
line is not a valid GPIO line?

Marek
Andy Shevchenko April 23, 2024, 4:43 p.m. UTC | #3 
On Tue, Apr 23, 2024 at 7:32 PM Marek Behún <kabel@kernel.org> wrote:
> On Tue, 23 Apr 2024 18:58:19 +0300
> Andy Shevchenko <andy@kernel.org> wrote:
> > On Thu, Apr 18, 2024 at 02:11:12PM +0200, Marek Behún wrote:

...

> > > +   irq_idx = omnia_int_to_gpio_idx[__bf_shf(INT_TRNG)];
> > > +   irq = devm_irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx);
> > > +   if (irq < 0)
> > > +           return dev_err_probe(dev, irq, "Cannot map TRNG IRQ\n");
> >
> > This looks like some workaround against existing gpiod_to_irq(). Why do you
> > need this?
>
> Hmmm, I thought that would not work because that line is only valid
> as an IRQ, not as a GPIO (this is enforced via the valid_mask member of
> gpio_chip and gpio_irq_chip).
>
> But looking at the code of gpiolib, if I do
>   irq = gpiod_to_irq(gpiochip_get_desc(gc, irq_idx));
> the valid_mask is not enforced anywhere.

Which one? GPIO has two: one per GPIO realm and one for IRQ domain.

> Is this semantically right to do even in spite of the fact that the
> line is not a valid GPIO line?

Yes. It's orthogonal to that.
Marek Behún April 23, 2024, 4:57 p.m. UTC | #4 
On Tue, 23 Apr 2024 19:43:41 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Apr 23, 2024 at 7:32 PM Marek Behún <kabel@kernel.org> wrote:
> > On Tue, 23 Apr 2024 18:58:19 +0300
> > Andy Shevchenko <andy@kernel.org> wrote:  
> > > On Thu, Apr 18, 2024 at 02:11:12PM +0200, Marek Behún wrote:  
> 
> ...
> 
> > > > +   irq_idx = omnia_int_to_gpio_idx[__bf_shf(INT_TRNG)];
> > > > +   irq = devm_irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx);
> > > > +   if (irq < 0)
> > > > +           return dev_err_probe(dev, irq, "Cannot map TRNG IRQ\n");  
> > >
> > > This looks like some workaround against existing gpiod_to_irq(). Why do you
> > > need this?  
> >
> > Hmmm, I thought that would not work because that line is only valid
> > as an IRQ, not as a GPIO (this is enforced via the valid_mask member of
> > gpio_chip and gpio_irq_chip).
> >
> > But looking at the code of gpiolib, if I do
> >   irq = gpiod_to_irq(gpiochip_get_desc(gc, irq_idx));
> > the valid_mask is not enforced anywhere.  
> 
> Which one? GPIO has two: one per GPIO realm and one for IRQ domain.

The GPIO line validity is not enforced. The IRQ line validity is
enforced in the gpiochip_to_irq() method.

> > Is this semantically right to do even in spite of the fact that the
> > line is not a valid GPIO line?  
> 
> Yes. It's orthogonal to that.
>
Andy Shevchenko April 23, 2024, 5:25 p.m. UTC | #5 
On Tue, Apr 23, 2024 at 06:57:04PM +0200, Marek Behún wrote:
> On Tue, 23 Apr 2024 19:43:41 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Tue, Apr 23, 2024 at 7:32 PM Marek Behún <kabel@kernel.org> wrote:
> > > On Tue, 23 Apr 2024 18:58:19 +0300
> > > Andy Shevchenko <andy@kernel.org> wrote:  
> > > > On Thu, Apr 18, 2024 at 02:11:12PM +0200, Marek Behún wrote:  

...

> > > > > +   irq_idx = omnia_int_to_gpio_idx[__bf_shf(INT_TRNG)];
> > > > > +   irq = devm_irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx);
> > > > > +   if (irq < 0)
> > > > > +           return dev_err_probe(dev, irq, "Cannot map TRNG IRQ\n");  
> > > >
> > > > This looks like some workaround against existing gpiod_to_irq(). Why do you
> > > > need this?  
> > >
> > > Hmmm, I thought that would not work because that line is only valid
> > > as an IRQ, not as a GPIO (this is enforced via the valid_mask member of
> > > gpio_chip and gpio_irq_chip).
> > >
> > > But looking at the code of gpiolib, if I do
> > >   irq = gpiod_to_irq(gpiochip_get_desc(gc, irq_idx));
> > > the valid_mask is not enforced anywhere.  
> > 
> > Which one? GPIO has two: one per GPIO realm and one for IRQ domain.
> 
> The GPIO line validity is not enforced. The IRQ line validity is
> enforced in the gpiochip_to_irq() method.

Okay, but does it work for you as expected then?

If not, we should fix GPIO library to have gpiod_to_irq() to work as expected.

> > > Is this semantically right to do even in spite of the fact that the
> > > line is not a valid GPIO line?  
> > 
> > Yes. It's orthogonal to that.
Marek Behún April 24, 2024, 4:55 p.m. UTC | #6 
On Tue, 23 Apr 2024 20:25:14 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Apr 23, 2024 at 06:57:04PM +0200, Marek Behún wrote:
> > On Tue, 23 Apr 2024 19:43:41 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > On Tue, Apr 23, 2024 at 7:32 PM Marek Behún <kabel@kernel.org> wrote:  
> > > > On Tue, 23 Apr 2024 18:58:19 +0300
> > > > Andy Shevchenko <andy@kernel.org> wrote:    
> > > > > On Thu, Apr 18, 2024 at 02:11:12PM +0200, Marek Behún wrote:    
> 
> ...
> 
> > > > > > +   irq_idx = omnia_int_to_gpio_idx[__bf_shf(INT_TRNG)];
> > > > > > +   irq = devm_irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx);
> > > > > > +   if (irq < 0)
> > > > > > +           return dev_err_probe(dev, irq, "Cannot map TRNG IRQ\n");    
> > > > >
> > > > > This looks like some workaround against existing gpiod_to_irq(). Why do you
> > > > > need this?    
> > > >
> > > > Hmmm, I thought that would not work because that line is only valid
> > > > as an IRQ, not as a GPIO (this is enforced via the valid_mask member of
> > > > gpio_chip and gpio_irq_chip).
> > > >
> > > > But looking at the code of gpiolib, if I do
> > > >   irq = gpiod_to_irq(gpiochip_get_desc(gc, irq_idx));
> > > > the valid_mask is not enforced anywhere.    
> > > 
> > > Which one? GPIO has two: one per GPIO realm and one for IRQ domain.  
> > 
> > The GPIO line validity is not enforced. The IRQ line validity is
> > enforced in the gpiochip_to_irq() method.  
> 
> Okay, but does it work for you as expected then?
> 
> If not, we should fix GPIO library to have gpiod_to_irq() to work as expected.

Yes, it does. I am going to send a new version in a few minutes.

Marek
diff mbox series

Patch

diff --git a/drivers/platform/cznic/Kconfig b/drivers/platform/cznic/Kconfig
index e2649cdecc38..750d5f47dba8 100644
--- a/drivers/platform/cznic/Kconfig
+++ b/drivers/platform/cznic/Kconfig
@@ -19,6 +19,7 @@  config TURRIS_OMNIA_MCU
 	depends on I2C
 	select GPIOLIB
 	select GPIOLIB_IRQCHIP
+	select HW_RANDOM
 	select RTC_CLASS
 	select WATCHDOG_CORE
 	help
@@ -28,6 +29,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 5f88119d825c..7fe4a3df93a6 100644
--- a/drivers/platform/cznic/turris-omnia-mcu-base.c
+++ b/drivers/platform/cznic/turris-omnia-mcu-base.c
@@ -369,7 +369,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 b3b203f0d2b9..625018ca82cc 100644
--- a/drivers/platform/cznic/turris-omnia-mcu-gpio.c
+++ b/drivers/platform/cznic/turris-omnia-mcu-gpio.c
@@ -162,7 +162,7 @@  static const struct omnia_gpio {
 };
 
 /* 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(INT_CARD_DET)]		= 4,
 	[__bf_shf(INT_MSATA_IND)]		= 5,
 	[__bf_shf(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..24fa8cb5d522
--- /dev/null
+++ b/drivers/platform/cznic/turris-omnia-mcu-trng.c
@@ -0,0 +1,89 @@ 
+// 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/devm-helpers.h>
+#include <linux/i2c.h>
+#include <linux/irqdomain.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 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 + 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 -EINTR;
+
+		err = omnia_cmd_read(mcu->client, CMD_TRNG_COLLECT_ENTROPY,
+				     reply, sizeof(reply));
+		if (err)
+			return err;
+
+		bytes = min3(reply[0], max, 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;
+	int irq, err;
+	u8 irq_idx;
+
+	if (!(mcu->features & FEAT_TRNG))
+		return 0;
+
+	irq_idx = omnia_int_to_gpio_idx[__bf_shf(INT_TRNG)];
+	irq = devm_irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx);
+	if (irq < 0)
+		return dev_err_probe(dev, irq, "Cannot map TRNG IRQ\n");
+
+	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 1838cb3d636e..e0cf10f8c32e 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>
@@ -45,6 +47,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,
@@ -147,11 +153,13 @@  static inline int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd)
 	return err ?: 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 */