Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <47e7eb15-e38d-ead3-de84-b7454e2c6eb8@gmail.com>
Date:   Wed, 22 Jun 2022 22:50:51 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v4 1/2] aspeed: i2c: add clock duty cycle property
Content-Language: en-US
To:     Brendan Higgins <brendanhiggins@google.com>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Joel Stanley <joel@jms.id.au>,
        Andrew Jeffery <andrew@aj.id.au>,
        Rob Herring <robh+dt@kernel.org>,
        Rayn Chen <rayn_chen@aspeedtech.com>
Cc:     Patrick Williams <patrick@stwcx.xyz>,
        Potin Lai <potin.lai@quantatw.com>, linux-i2c@vger.kernel.org,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-aspeed@lists.ozlabs.org, linux-kernel@vger.kernel.org
References: <20220610054722.32229-1-potin.lai.pt@gmail.com>
 <20220610054722.32229-2-potin.lai.pt@gmail.com>
From:   Potin Lai <potin.lai.pt@gmail.com>
In-Reply-To: <20220610054722.32229-2-potin.lai.pt@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

Potin Lai 於 6/10/2022 1:47 PM 寫道:
> Introduce i2c-clk-high-min-percent property for setting a minimum clock
> high percentage.
>
> This driver calculate clk_high and clk_low with giving duty cycle. If it
> could not find a suit clk_high and clk_low, it apply default duty cycle
> 50%.
>
> Signed-off-by: Potin Lai <potin.lai.pt@gmail.com>
> ---
>  drivers/i2c/busses/i2c-aspeed.c | 56 ++++++++++++++++++++++++++-------
>  1 file changed, 45 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index 67e8b97c0c95..9715dc4f933f 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -19,6 +19,7 @@
>  #include <linux/irqchip/chained_irq.h>
>  #include <linux/irqdomain.h>
>  #include <linux/kernel.h>
> +#include <linux/math.h>
>  #include <linux/module.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
> @@ -27,6 +28,8 @@
>  #include <linux/reset.h>
>  #include <linux/slab.h>
>  
> +#define DEFAULT_I2C_CLK_DUTY_CYCLE			50
> +
>  /* I2C Register */
>  #define ASPEED_I2C_FUN_CTRL_REG				0x00
>  #define ASPEED_I2C_AC_TIMING_REG1			0x04
> @@ -149,9 +152,11 @@ struct aspeed_i2c_bus {
>  	spinlock_t			lock;
>  	struct completion		cmd_complete;
>  	u32				(*get_clk_reg_val)(struct device *dev,
> -							   u32 divisor);
> +							   u32 divisor,
> +							   u32 duty_cycle);
>  	unsigned long			parent_clk_frequency;
>  	u32				bus_frequency;
> +	u32				duty_cycle;
>  	/* Transaction state. */
>  	enum aspeed_i2c_master_state	master_state;
>  	struct i2c_msg			*msgs;
> @@ -798,9 +803,11 @@ static const struct i2c_algorithm aspeed_i2c_algo = {
>  
>  static u32 aspeed_i2c_get_clk_reg_val(struct device *dev,
>  				      u32 clk_high_low_mask,
> -				      u32 divisor)
> +				      u32 divisor,
> +				      u32 duty_cycle)
>  {
>  	u32 base_clk_divisor, clk_high_low_max, clk_high, clk_low, tmp;
> +	u32 tmp_base_clk_divisor;
>  
>  	/*
>  	 * SCL_high and SCL_low represent a value 1 greater than what is stored
> @@ -842,10 +849,32 @@ static u32 aspeed_i2c_get_clk_reg_val(struct device *dev,
>  			"clamping clock divider: divider requested, %u, is greater than largest possible divider, %u.\n",
>  			divisor, (1 << base_clk_divisor) * clk_high_low_max);
>  	} else {
> -		tmp = (divisor + (1 << base_clk_divisor) - 1)
> +		for (tmp_base_clk_divisor = base_clk_divisor;
> +		    tmp_base_clk_divisor <= ASPEED_I2CD_TIME_BASE_DIVISOR_MASK;
> +		    tmp_base_clk_divisor++) {
> +			/* calculate clk_high and clk_low with duty cycle */
> +			tmp = (divisor + (1 << tmp_base_clk_divisor) - 1)
> +				>> tmp_base_clk_divisor;
> +
> +			clk_high = DIV_ROUND_UP(tmp * duty_cycle, 100);
> +			clk_low = tmp - clk_high;
> +
> +			if (max(clk_high, clk_low) <= (clk_high_low_mask + 1))
> +				break;
> +		}
> +
> +		if (tmp_base_clk_divisor <= ASPEED_I2CD_TIME_BASE_DIVISOR_MASK)
> +			base_clk_divisor = tmp_base_clk_divisor;
> +		else {
> +			dev_err(dev,
> +				"could not find clk_high and clk_low with duty cycle %u%%\n, recalculate with base_clk_divisor %u and duty_cycle 50%%",
> +				duty_cycle, base_clk_divisor);
> +			duty_cycle = 50;
> +			tmp = (divisor + (1 << base_clk_divisor) - 1)
>  				>> base_clk_divisor;
> -		clk_low = tmp / 2;
> -		clk_high = tmp - clk_low;
> +			clk_high = DIV_ROUND_UP(tmp * duty_cycle, 100);
> +			clk_low = tmp - clk_high;
> +		}
>  
>  		if (clk_high)
>  			clk_high--;
> @@ -863,22 +892,22 @@ static u32 aspeed_i2c_get_clk_reg_val(struct device *dev,
>  			   & ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
>  }
>  
> -static u32 aspeed_i2c_24xx_get_clk_reg_val(struct device *dev, u32 divisor)
> +static u32 aspeed_i2c_24xx_get_clk_reg_val(struct device *dev, u32 divisor, u32 duty_cycle)
>  {
>  	/*
>  	 * clk_high and clk_low are each 3 bits wide, so each can hold a max
>  	 * value of 8 giving a clk_high_low_max of 16.
>  	 */
> -	return aspeed_i2c_get_clk_reg_val(dev, GENMASK(2, 0), divisor);
> +	return aspeed_i2c_get_clk_reg_val(dev, GENMASK(2, 0), divisor, duty_cycle);
>  }
>  
> -static u32 aspeed_i2c_25xx_get_clk_reg_val(struct device *dev, u32 divisor)
> +static u32 aspeed_i2c_25xx_get_clk_reg_val(struct device *dev, u32 divisor, u32 duty_cycle)
>  {
>  	/*
>  	 * clk_high and clk_low are each 4 bits wide, so each can hold a max
>  	 * value of 16 giving a clk_high_low_max of 32.
>  	 */
> -	return aspeed_i2c_get_clk_reg_val(dev, GENMASK(3, 0), divisor);
> +	return aspeed_i2c_get_clk_reg_val(dev, GENMASK(3, 0), divisor, duty_cycle);
>  }
>  
>  /* precondition: bus.lock has been acquired. */
> @@ -891,7 +920,7 @@ static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus)
>  	clk_reg_val &= (ASPEED_I2CD_TIME_TBUF_MASK |
>  			ASPEED_I2CD_TIME_THDSTA_MASK |
>  			ASPEED_I2CD_TIME_TACST_MASK);
> -	clk_reg_val |= bus->get_clk_reg_val(bus->dev, divisor);
> +	clk_reg_val |= bus->get_clk_reg_val(bus->dev, divisor, bus->duty_cycle);
>  	writel(clk_reg_val, bus->base + ASPEED_I2C_AC_TIMING_REG1);
>  	writel(ASPEED_NO_TIMEOUT_CTRL, bus->base + ASPEED_I2C_AC_TIMING_REG2);
>  
> @@ -1009,11 +1038,16 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
>  		bus->bus_frequency = I2C_MAX_STANDARD_MODE_FREQ;
>  	}
>  
> +	ret = of_property_read_u32(pdev->dev.of_node,
> +				   "i2c-clk-high-min-percent", &bus->duty_cycle);
> +	if (ret < 0 || !bus->duty_cycle || bus->duty_cycle > 100)
> +		bus->duty_cycle = DEFAULT_I2C_CLK_DUTY_CYCLE;
> +
>  	match = of_match_node(aspeed_i2c_bus_of_table, pdev->dev.of_node);
>  	if (!match)
>  		bus->get_clk_reg_val = aspeed_i2c_24xx_get_clk_reg_val;
>  	else
> -		bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
> +		bus->get_clk_reg_val = (u32 (*)(struct device *, u32, u32))
>  				match->data;
>  
>  	/* Initialize the I2C adapter */
Hi Maintainers,
Could someone please help me review this patch and give me some advice?
Thanks!

Potin