atmel sha: i2c bus speed under multiplexer (WAS: devicetree for Turris Omnia is missing i2c frequency for atsha204a)

atmel sha: i2c bus speed under multiplexer (WAS: devicetree for Turris Omnia is missing i2c frequency for atsha204a)

[Marek Behún]

On Sat, 11 May 2024 20:09:30 +0000
m95d@psihoexpert.ro wrote:

> Hello.
> 
> Booting kernel v6.6 I get these errors in dmesg:
> 
> atmel-sha204a 6-0064: failed to read clock-frequency property
> atmel-sha204a: probe of 6-0064 failed with error -22
> 
> I'm attaching a patch to fix it.
> It adds 1MHz clock frequency to the i2c devicetree node where atsha204a is connected. This is the max. supported frequency according to the atmel sha204a specs sheet.
> Tested. Works.
> 
> Thank you.
> 
> diff --git a/arch/arm/boot/dts/marvell/armada-385-turris-omnia.dts b/arch/arm/boot/dts/marvell/armada-385-turris-omnia.dts
> index 7b755bb4e4e7..64caabc81585 100644
> --- a/arch/arm/boot/dts/marvell/armada-385-turris-omnia.dts
> +++ b/arch/arm/boot/dts/marvell/armada-385-turris-omnia.dts
> @@ -378,6 +378,7 @@ i2c@5 {
>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  			reg = <5>;
> +			clock-frequency = <1000000>;
>  
>  			/* ATSHA204A-MAHDA-T crypto module */
>  			crypto@64 {
> 
> Signed-off-by: Marius Dinu <m95d+git@psihoexpert.ro>

Hi Marius,

adding I2C maintainers and Atmel crypto driver maintainers to this
thread.

Although this fixes your problem of failed driver probe, this change
is in fact wrong on several points.

First, according to documentation the A38x I2C controller should be
used either with 100 kHz standard I2C rate or 400 kHz fast rate. 1 MHz
is too much.

Second, the ATSHA is connected via the PCA9547 I2C multiplexer.
According to documentation for this multiplexer, the maximum clock
frequency that should be used is also 400 kHz.

Third, this change adds the clock-frequency to one channel of the
multiplexer (the one on which the ATSHA is connected). Looking at the
I2C code, that clock-frequency is not used for multiplexer channels, so
the actual I2C frequency is not set to 1 MHz, and instead remains at
100 kHz.

Fourth, the ATSHA driver uses the parsed clock-frequency to compute
wake token size. Since the given clock-frequency is 1 MHz, but the
I2C bus in reality operates at 100 kHz, the computed wake token size is
incorrect.

In my opinion, a correct fix should instead come into the atmel SHA
crypto driver, drivers/crypto/atmel-i2c.c: the bus clock rate is first
tried to be determined from ACPI:
  bus_clk_rate = i2c_acpi_find_bus_speed(&client->adapter->dev);
If that fails, the second option is to read the clock-frequency
property of the parent device (the I2C bus), which on Turris Omnia is
an I2C multiplexer channel. This is where it is wrong. Instead, there
should either:
- be a similar function to the i2c_acpi_find_bus_speed(), but for OF
- or there should I2C core function to report bus speed,
  i2c_find_bus_speed(), that would also work over multiplexers.

Marek