Date:   Thu, 4 Nov 2021 17:10:04 +0900
From:   "Jaewon Kim" <jaewon02.kim@...sung.com>
To:     "'Krzysztof Kozlowski'" <krzysztof.kozlowski@...onical.com>,
        "'Wolfram Sang'" <wsa@...nel.org>,
        "'Rob Herring'" <robh+dt@...nel.org>
Cc:     "'Chanho Park'" <chanho61.park@...sung.com>,
        <linux-i2c@...r.kernel.org>, <linux-samsung-soc@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/2] i2c: exynos5: support USI(Universal Serial
 Interface)

Hello Krzysztof

> On 01/11/2021 12:38, Jaewon Kim wrote:
> > Serial IPs(UART, I2C, SPI) are integrated into New IP-Core called
> > USI(Universal Serial Interface).
> >
> > As it is integrated into USI, there are additinal HW changes.
> > Registers to control USI and sysreg to set serial IPs have been added.
> > Also, some timing registres have been changed.
> >
> > Signed-off-by: Jaewon Kim <jaewon02.kim@...sung.com>
> > ---
> >  drivers/i2c/busses/i2c-exynos5.c | 120
> > ++++++++++++++++++++++++++++---
> >  1 file changed, 110 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-exynos5.c
> > b/drivers/i2c/busses/i2c-exynos5.c
> > index 97d4f3ac0abd..f2dc7848f840 100644
> > --- a/drivers/i2c/busses/i2c-exynos5.c
> > +++ b/drivers/i2c/busses/i2c-exynos5.c
> > @@ -22,6 +22,8 @@
> >  #include <linux/of_device.h>
> >  #include <linux/of_irq.h>
> >  #include <linux/spinlock.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/regmap.h>
> >
> >  /*
> >   * HSI2C controller from Samsung supports 2 modes of operation @@
> > -166,9 +168,21 @@
> >
> >  #define EXYNOS5_I2C_TIMEOUT (msecs_to_jiffies(100))
> >
> > +/* USI(Universal Serial Interface) Register map */
> > +#define USI_CON					0xc4
> > +#define USI_OPTION				0xc8
> > +
> > +/* USI(Universal Serial Interface) Register bits */
> > +#define USI_CON_RESET				(1 << 0)
> 
> BIT()
> 

Okay, I will change it to BIT()

> > +
> > +/* SYSREG Register bit */
> > +#define SYSREG_USI_SW_CONF_MASK			(0x7 << 0)
> > +#define SYSREG_I2C_SW_CONF			(1u << 2)
> 
> BIT()
> 

Okay, I will change it to BIT()

> > +
> >  enum i2c_type_exynos {
> >  	I2C_TYPE_EXYNOS5,
> >  	I2C_TYPE_EXYNOS7,
> > +	I2C_TYPE_USI,
> >  };
> >
> >  struct exynos5_i2c {
> > @@ -199,6 +213,10 @@ struct exynos5_i2c {
> >
> >  	/* Version of HS-I2C Hardware */
> >  	const struct exynos_hsi2c_variant *variant;
> > +
> > +	/* USI sysreg info */
> > +	struct regmap		*usi_sysreg;
> > +	unsigned int		usi_offset;
> >  };
> >
> >  /**
> > @@ -230,6 +248,11 @@ static const struct exynos_hsi2c_variant exynos7_hsi2c_data = {
> >  	.hw		= I2C_TYPE_EXYNOS7,
> >  };
> >
> > +static const struct exynos_hsi2c_variant exynos_usi_hsi2c_data = {
> > +	.fifo_depth	= 64,
> > +	.hw		= I2C_TYPE_USI,
> > +};
> > +
> >  static const struct of_device_id exynos5_i2c_match[] = {
> >  	{
> >  		.compatible = "samsung,exynos5-hsi2c", @@ -243,6 +266,9 @@ static
> > const struct of_device_id exynos5_i2c_match[] = {
> >  	}, {
> >  		.compatible = "samsung,exynos7-hsi2c",
> >  		.data = &exynos7_hsi2c_data
> > +	}, {
> > +		.compatible = "samsung,exynos-usi-hsi2c",
> > +		.data = &exynos_usi_hsi2c_data
> >  	}, {},
> >  };
> >  MODULE_DEVICE_TABLE(of, exynos5_i2c_match); @@ -281,6 +307,31 @@
> > static int exynos5_i2c_set_timing(struct exynos5_i2c *i2c, bool hs_timings)
> >  		i2c->op_clock;
> >  	int div, clk_cycle, temp;
> >
> > +	/* In case of HSI2C controllers in USI
> > +	 * timing control formula changed.
> > +	 *
> > +	 * FSCL = IPCLK / ((CLK_DIV + 1) * 16)
> > +	 * T_SCL_LOW = IPCLK * (CLK_DIV + 1) * (N + M)
> > +	 *  [N : number of 0's in the TSCL_H_HS]
> > +	 *  [M : number of 0's in the TSCL_L_HS]
> > +	 * T_SCL_HIGH = IPCLK * (CLK_DIV + 1) * (N + M)
> > +	 *  [N : number of 1's in the TSCL_H_HS]
> > +	 *  [M : number of 1's in the TSCL_L_HS]
> > +	 *
> > +	 *  result of (N + M) is always 8.
> > +	 *  In genaral case, we don`t need to control timing_s1, timing_s2.
> 
> s/genaral/general/
> (please run spellcheck)
> s/don`t/don't/
> 

Sorry, I will run spellcheck in next.

> > +	 */
> > +	if (i2c->variant->hw == I2C_TYPE_USI) {
> > +		div = ((clkin / (16 * i2c->op_clock)) - 1);
> > +		i2c_timing_s3 = div << 16;
> > +		if (hs_timings)
> > +			writel(i2c_timing_s3, i2c->regs + HSI2C_TIMING_HS3);
> > +		else
> > +			writel(i2c_timing_s3, i2c->regs + HSI2C_TIMING_FS3);
> > +
> > +		return 0;
> > +	}
> > +
> >  	/*
> >  	 * In case of HSI2C controller in Exynos5 series
> >  	 * FPCLK / FI2C =
> > @@ -355,6 +406,16 @@ static int exynos5_hsi2c_clock_setup(struct exynos5_i2c *i2c)
> >  	return exynos5_i2c_set_timing(i2c, true);  }
> >
> > +static void exynos_usi_reset(struct exynos5_i2c *i2c)
> 
> The name of function suggests you are performing a reset but the code looks like it is only clearing
> the reset flag. How about calling the function exynos_usi_clear_reset()?
> 

Accroding to below review, I will add reset and clear code.

> > +{
> > +	u32 val;
> > +
> > +	/* Clear the software reset of USI block (it's set at startup) */
> > +	val = readl(i2c->regs + USI_CON);
> > +	val &= ~USI_CON_RESET;
> > +	writel(val, i2c->regs + USI_CON);
> > +}
> > +
> >  /*
> >   * exynos5_i2c_init: configures the controller for I2C functionality
> >   * Programs I2C controller for Master mode operation @@ -385,6 +446,9
> > @@ static void exynos5_i2c_reset(struct exynos5_i2c *i2c)  {
> >  	u32 i2c_ctl;
> >
> > +	if (i2c->variant->hw == I2C_TYPE_USI)
> > +		exynos_usi_reset(i2c);
> > +
> >  	/* Set and clear the bit for reset */
> >  	i2c_ctl = readl(i2c->regs + HSI2C_CTL);
> >  	i2c_ctl |= HSI2C_SW_RST;
> > @@ -422,7 +486,8 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
> >  	writel(int_status, i2c->regs + HSI2C_INT_STATUS);
> >
> >  	/* handle interrupt related to the transfer status */
> > -	if (i2c->variant->hw == I2C_TYPE_EXYNOS7) {
> > +	if (i2c->variant->hw == I2C_TYPE_EXYNOS7 ||
> > +			i2c->variant->hw == I2C_TYPE_USI) {
> >  		if (int_status & HSI2C_INT_TRANS_DONE) {
> >  			i2c->trans_done = 1;
> >  			i2c->state = 0;
> > @@ -569,13 +634,13 @@ static void exynos5_i2c_bus_check(struct
> > exynos5_i2c *i2c)  {
> >  	unsigned long timeout;
> >
> > -	if (i2c->variant->hw != I2C_TYPE_EXYNOS7)
> > +	if (i2c->variant->hw == I2C_TYPE_EXYNOS5)
> >  		return;
> >
> >  	/*
> > -	 * HSI2C_MASTER_ST_LOSE state in EXYNOS7 variant before transaction
> > -	 * indicates that bus is stuck (SDA is low). In such case bus recovery
> > -	 * can be performed.
> > +	 * HSI2C_MASTER_ST_LOSE state in EXYNOS7 or EXYNOS_USI variant before
> > +	 * transaction indicates that bus is stuck (SDA is low).
> > +	 * In such case bus recovery can be performed.
> >  	 */
> >  	timeout = jiffies + msecs_to_jiffies(100);
> >  	for (;;) {
> > @@ -611,10 +676,10 @@ static void exynos5_i2c_message_start(struct exynos5_i2c *i2c, int stop)
> >  	unsigned long flags;
> >  	unsigned short trig_lvl;
> >
> > -	if (i2c->variant->hw == I2C_TYPE_EXYNOS7)
> > -		int_en |= HSI2C_INT_I2C_TRANS;
> > -	else
> > +	if (i2c->variant->hw == I2C_TYPE_EXYNOS5)
> >  		int_en |= HSI2C_INT_I2C;
> > +	else
> > +		int_en |= HSI2C_INT_I2C_TRANS;
> >
> >  	i2c_ctl = readl(i2c->regs + HSI2C_CTL);
> >  	i2c_ctl &= ~(HSI2C_TXCHON | HSI2C_RXCHON); @@ -738,6 +803,37 @@
> > static const struct i2c_algorithm exynos5_i2c_algorithm = {
> >  	.functionality		= exynos5_i2c_func,
> >  };
> >
> > +static int exynos_usi_init(struct exynos5_i2c *i2c) {
> > +	struct device *dev = i2c->dev;
> > +	int ret;
> > +
> > +	if (i2c->variant->hw != I2C_TYPE_USI)
> > +		return 0;
> > +
> > +	/* USI regmap control */
> 
> Drop the comment, it's obvious. What is missing here, is a comment explaining what are you
> initializing exactly in the USI. Please add it.
> 

Okay, I will add detailed information.

> > +	i2c->usi_sysreg = syscon_regmap_lookup_by_phandle(
> > +			dev->of_node, "samsung,usi-sysreg");
> 
> Align the lines to opening parenthesis.
> 
> > +	if (IS_ERR(i2c->usi_sysreg)) {
> > +		dev_err(dev, "Cannot find usi-sysreg\n");
> > +		return PTR_ERR(i2c->usi_sysreg);
> > +	}
> > +
> > +	ret = of_property_read_u32_index(dev->of_node,
> > +				"samsung,usi-sysreg", 1, &i2c->usi_offset);
> 
> Align the lines to opening parenthesis.
> 

Okay.

> Offset is not described in the bindings.
> 

Okay, I will add offset description in bindings docs.

> > +	if (ret) {
> > +		dev_err(dev, "usi-sysreg offset is not specified\n");
> > +		return ret;
> > +	}
> > +
> > +	regmap_update_bits(i2c->usi_sysreg, i2c->usi_offset,
> > +			SYSREG_USI_SW_CONF_MASK, SYSREG_I2C_SW_CONF);
> > +
> > +	exynos_usi_reset(i2c);
> 
> You are clearing the reset flag, but not setting it back on probe failure. What happens if the probe
> fails after this clear()? E.g.
> because of deferred probe? The next probe try will start on a not-reset controller. Will it work?
> 

The user manual guides USI_RESET to be done after changing the system register.
For clarity, we will change not only to clear reset, but to clear after reset.

> > +
> > +	return 0;
> > +}
> > +
> >  static int exynos5_i2c_probe(struct platform_device *pdev)  {
> >  	struct device_node *np = pdev->dev.of_node; @@ -777,6 +873,12 @@
> > static int exynos5_i2c_probe(struct platform_device *pdev)
> >  	i2c->adap.algo_data = i2c;
> >  	i2c->adap.dev.parent = &pdev->dev;
> >
> > +	i2c->variant = of_device_get_match_data(&pdev->dev);
> > +
> > +	ret = exynos_usi_init(i2c);
> > +	if (ret)
> > +		return ret;
> > +
> >  	/* Clear pending interrupts from u-boot or misc causes */
> >  	exynos5_i2c_clr_pend_irq(i2c);
> >
> > @@ -794,8 +896,6 @@ static int exynos5_i2c_probe(struct platform_device *pdev)
> >  		goto err_clk;
> >  	}
> >
> > -	i2c->variant = of_device_get_match_data(&pdev->dev);
> > -
> >  	ret = exynos5_hsi2c_clock_setup(i2c);
> >  	if (ret)
> >  		goto err_clk;
> >
> 
> 
> Best regards,
> Krzysztof


Thanks
Jaewon Kim

Powered by blists - more mailing lists