linux-kernel - Re: [PATCH v2 1/1] i2c: qcom-geni: Support systems with 32MHz serial engine clock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <def1c338-8e41-4622-83d5-7a377d780d76@linaro.org>
Date: Thu, 26 Sep 2024 13:28:37 +0300
From: Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>
To: Manikanta Mylavarapu <quic_mmanikan@...cinc.com>, andi.shyti@...nel.org,
 linux-arm-msm@...r.kernel.org, linux-i2c@...r.kernel.org,
 linux-kernel@...r.kernel.org
Cc: quic_srichara@...cinc.com, quic_varada@...cinc.com
Subject: Re: [PATCH v2 1/1] i2c: qcom-geni: Support systems with 32MHz serial
 engine clock

Hello Manikanta.

On 9/26/24 06:43, Manikanta Mylavarapu wrote:
> In existing socs, I2C serial engine is sourced from XO (19.2MHz).
> Where as in IPQ5424, I2C serial engine is sourced from GPLL0 (32MHz).
> 
> The existing map table is based on 19.2MHz. This patch incorporate
> the clock map table to derive the SCL clock from the 32MHz source
> clock frequency.
> 
> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@...cinc.com>
> ---
> Changes in v2:
> 	- Dropped IPQ5424 from the commit title
> 	- Added else part to assign geni_i2c_clk_map_19p2mhz to itr
> 	- Dropped MHZ macro and used HZ_PER_MHZ macro
> 	- Expanded SE to serial engine
> 	- Added the reason for 32MHz clock in commit message
> 
>   drivers/i2c/busses/i2c-qcom-geni.c | 19 ++++++++++++++++---
>   1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 212336f724a6..22f2a0d83641 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -16,6 +16,7 @@
>   #include <linux/pm_runtime.h>
>   #include <linux/soc/qcom/geni-se.h>
>   #include <linux/spinlock.h>
> +#include <linux/units.h>
>   
>   #define SE_I2C_TX_TRANS_LEN		0x26c
>   #define SE_I2C_RX_TRANS_LEN		0x270
> @@ -146,18 +147,30 @@ struct geni_i2c_clk_fld {
>    * clk_freq_out = t / t_cycle
>    * source_clock = 19.2 MHz
>    */
> -static const struct geni_i2c_clk_fld geni_i2c_clk_map[] = {
> +static const struct geni_i2c_clk_fld geni_i2c_clk_map_19p2mhz[] = {
>   	{KHZ(100), 7, 10, 11, 26},
>   	{KHZ(400), 2,  5, 12, 24},
>   	{KHZ(1000), 1, 3,  9, 18},
>   };
>   
> +/* source_clock = 32 MHz */
> +static const struct geni_i2c_clk_fld geni_i2c_clk_map_32mhz[] = {
> +	{KHZ(100), 7, 14, 18, 40},
> +	{KHZ(400), 4,  3, 11, 20},
> +	{KHZ(1000), 4, 3,  6, 15},
> +};

Please double check the values.

This is what I get:
* for 100KHz: 32000000 / (40 * 7) ~ 114286, apparently 32000000 / (40 * 8) would
be a better fit, however it's unclear what would be proper t_high / t_low values,
* for 400KHz: it seems good,
* for 1000KHz: 32000000 / (15 * 4) ~ 533333, which is almost 1/2 of the wanted
bus frequency, so this one looks very wrong.

Do you have any ideas how to get better bus frequency settings?

> +
>   static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c)
>   {
>   	int i;
> -	const struct geni_i2c_clk_fld *itr = geni_i2c_clk_map;
> +	const struct geni_i2c_clk_fld *itr;
> +
> +	if (clk_get_rate(gi2c->se.clk) == 32 * HZ_PER_MHZ)
> +		itr = geni_i2c_clk_map_32mhz;
> +	else
> +		itr = geni_i2c_clk_map_19p2mhz;
>   
> -	for (i = 0; i < ARRAY_SIZE(geni_i2c_clk_map); i++, itr++) {
> +	for (i = 0; i < ARRAY_SIZE(geni_i2c_clk_map_19p2mhz); i++, itr++) {

Struct/data organization here is so weak, that here it's implicitly assumed
that sizes of two arrays are equal. It could be kept as is of course, just
pointing to it.

>   		if (itr->clk_freq_out == gi2c->clk_freq_out) {
>   			gi2c->clk_fld = itr;
>   			return 0;

--
Best wishes,
Vladimir