linux-kernel - Re: [PATCH v3 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] [day] [month] [year] [list]

Message-ID: <72we2tesj5whmfgo3yc4mdta6lasu4v4ll74pmvysaxnwaf4tv@zqanlhy3jgsv>
Date: Mon, 30 Sep 2024 22:07:36 -0500
From: Bjorn Andersson <andersson@...nel.org>
To: Manikanta Mylavarapu <quic_mmanikan@...cinc.com>
Cc: andi.shyti@...nel.org, linux-arm-msm@...r.kernel.org, 
	linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org, quic_srichara@...cinc.com, 
	quic_varada@...cinc.com
Subject: Re: [PATCH v3 1/1] i2c: qcom-geni: Support systems with 32MHz serial
 engine clock

On Mon, Sep 30, 2024 at 08:17:09PM GMT, 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 incorporates
> the clock map table to derive the SCL clock from the 32MHz source
> clock frequency.
> 
> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@...cinc.com>

Reviewed-by: Bjorn Andersson <andersson@...nel.org>

> ---
> Changes in v3:
> 	- Updated geni_i2c_clk_map_32mhz array values
> 	- Added sentinel value to both 19.2MHz, 32MHz clk map arrays
> 	- Updated loop termination condition based on sentinel value
> 
>  drivers/i2c/busses/i2c-qcom-geni.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 212336f724a6..579c01686823 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,22 +147,36 @@ 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},
> +	{},

For future reference, the reason to leave a trailing ',' is so that one
can add another line in the array without touching the previous last
entry. This will of course never happen when that is a sentinel.

Unless Andi insist, I don't think it's worth resubmitting the patch for
this, but now you know.

Regards,
Bjorn

> +};
> +
> +/* source_clock = 32 MHz */
> +static const struct geni_i2c_clk_fld geni_i2c_clk_map_32mhz[] = {
> +	{KHZ(100), 8, 14, 18, 40},
> +	{KHZ(400), 4,  3, 11, 20},
> +	{KHZ(1000), 2, 3,  6, 15},
> +	{},
>  };
>  
>  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++) {
> +	while (itr->clk_freq_out != 0) {
>  		if (itr->clk_freq_out == gi2c->clk_freq_out) {
>  			gi2c->clk_fld = itr;
>  			return 0;
>  		}
> +		itr++;
>  	}
>  	return -EINVAL;
>  }
> -- 
> 2.34.1
> 
>