[PATCH] i2c: designware: Get HCNT/LCNT values from dts

[PATCH] i2c: designware: Get HCNT/LCNT values from dts

[Lawrence,Wang]

From: "Wang, Lawrence" <lawrence.wang@nokia-sbell.com>

Current code support config the HCNT/LCNT only via ACPI method. for those
platform that not support ACPI, will get the HCNT/LCNT value based on input
clock. But it is not always accuracy. for example in some platform will get
lower speed(320khz) in fast mode, and get faster speed(105khz/even more) in
standard mode.

This patch makes it possible for the non-ACPI platform to pass more optimal
HCNT/LCNT values to the core driver via dts if they are known beforehand.
If these are not set we use the calculated values.

Signed-off-by: Wang, Lawrence <lawrence.wang@nokia-sbell.com>
---
 drivers/i2c/busses/i2c-designware-common.c  | 37 +++++++++++++++++++++
 drivers/i2c/busses/i2c-designware-core.h    |  1 +
 drivers/i2c/busses/i2c-designware-platdrv.c |  3 ++
 3 files changed, 41 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index bf2a4920638a..7cdceeaa9741 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -314,6 +314,43 @@ static inline u32 i2c_dw_acpi_round_bus_speed(struct device *device) { return 0;
 
 #endif	/* CONFIG_ACPI */
 
+struct dw_scl_cfg {
+	u16 ss_hcnt;
+	u16 ss_lcnt;
+	u16 fs_hcnt;
+	u16 fs_lcnt;
+	u16 fp_hcnt;
+	u16 fp_lcnt;
+	u16 hs_hcnt;
+	u16 hs_lcnt;
+};
+
+/*
+ * The HCNT/LCNT information calculated based on the input clock are not always
+ * accurate for some given platform. In some systems get it more higher SCL speed.
+ * On such systems we better get results from dts config.
+ */
+void i2c_dw_scl_timing_configure(struct dw_i2c_dev *dev)
+{
+	int ret;
+	struct dw_scl_cfg i2c_scl_timing;
+
+	ret = device_property_read_u16_array(dev->dev, "dw-i2c-scl-timing",
+					(u16 *)&i2c_scl_timing, sizeof(i2c_scl_timing)/sizeof(u16));
+	if (ret)
+		return;
+
+	dev->ss_hcnt = i2c_scl_timing.ss_hcnt;
+	dev->ss_lcnt = i2c_scl_timing.ss_lcnt;
+	dev->fs_hcnt = i2c_scl_timing.fs_hcnt;
+	dev->fs_lcnt = i2c_scl_timing.fs_lcnt;
+	dev->fp_hcnt = i2c_scl_timing.fp_hcnt;
+	dev->fp_lcnt = i2c_scl_timing.fp_lcnt;
+	dev->hs_hcnt = i2c_scl_timing.hs_hcnt;
+	dev->hs_lcnt = i2c_scl_timing.hs_lcnt;
+}
+EXPORT_SYMBOL_GPL(i2c_dw_scl_timing_configure);
+
 void i2c_dw_adjust_bus_speed(struct dw_i2c_dev *dev)
 {
 	u32 acpi_speed = i2c_dw_acpi_round_bus_speed(dev->dev);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 60a2e750cee9..536b30ea723b 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -372,6 +372,7 @@ static inline int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev) { return 0;
 
 int i2c_dw_validate_speed(struct dw_i2c_dev *dev);
 void i2c_dw_adjust_bus_speed(struct dw_i2c_dev *dev);
+void i2c_dw_scl_timing_configure(struct dw_i2c_dev *dev);
 
 #if IS_ENABLED(CONFIG_ACPI)
 int i2c_dw_acpi_configure(struct device *device);
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 21113665ddea..b08177edf1fc 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -237,6 +237,9 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	t = &dev->timings;
 	i2c_parse_fw_timings(&pdev->dev, t, false);
 
+	// Try to get HCNT/LCNT from dts
+	i2c_dw_scl_timing_configure(dev);
+
 	i2c_dw_adjust_bus_speed(dev);
 
 	if (pdev->dev.of_node)
-- 
2.25.1

Re: [PATCH] i2c: designware: Get HCNT/LCNT values from dts

[Andy Shevchenko]

On Mon, Nov 15, 2021 at 10:35:55AM +0100, Lawrence,Wang wrote:
> From: "Wang, Lawrence" <lawrence.wang@nokia-sbell.com>
> 
> Current code support config the HCNT/LCNT only via ACPI method. for those
> platform that not support ACPI, will get the HCNT/LCNT value based on input
> clock. But it is not always accuracy. for example in some platform will get
> lower speed(320khz) in fast mode, and get faster speed(105khz/even more) in
> standard mode.
> 
> This patch makes it possible for the non-ACPI platform to pass more optimal
> HCNT/LCNT values to the core driver via dts if they are known beforehand.
> If these are not set we use the calculated values.

Besides the fact it misses DT schema update, why this is needed at all?
What's wrong with the existing DT timings?

So far seems NAK to me.

...

> +	ret = device_property_read_u16_array(dev->dev, "dw-i2c-scl-timing",
> +					(u16 *)&i2c_scl_timing, sizeof(i2c_scl_timing)/sizeof(u16));
> +	if (ret)
> +		return;

No, you have to have one property per each value AFAIU DT schema requirements,
Ask Rob Herring about it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] i2c: designware: Get HCNT/LCNT values from dts

[Andy Shevchenko]

On Mon, Nov 15, 2021 at 12:19:26PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 15, 2021 at 10:35:55AM +0100, Lawrence,Wang wrote:
> > From: "Wang, Lawrence" <lawrence.wang@nokia-sbell.com>
> > 
> > Current code support config the HCNT/LCNT only via ACPI method. for those
> > platform that not support ACPI, will get the HCNT/LCNT value based on input
> > clock. But it is not always accuracy. for example in some platform will get
> > lower speed(320khz) in fast mode, and get faster speed(105khz/even more) in
> > standard mode.
> > 
> > This patch makes it possible for the non-ACPI platform to pass more optimal
> > HCNT/LCNT values to the core driver via dts if they are known beforehand.
> > If these are not set we use the calculated values.
> 
> Besides the fact it misses DT schema update, why this is needed at all?
> What's wrong with the existing DT timings?

Just for your convenience an excerpt from
Documentation/devicetree/bindings/i2c/i2c.txt

- i2c-scl-falling-time-ns
Number of nanoseconds the SCL signal takes to fall; t(f) in the I2C
specification.

- i2c-scl-internal-delay-ns
Number of nanoseconds the IP core additionally needs to setup SCL.

- i2c-scl-rising-time-ns
Number of nanoseconds the SCL signal takes to rise; t(r) in the I2C
specification.

- i2c-sda-falling-time-ns
Number of nanoseconds the SDA signal takes to fall; t(f) in the I2C
specification.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH] i2c: designware: Get HCNT/LCNT values from dts

[Wang, Lawrence (NSB - CN/Hangzhou)]

Hello Shevchenko

Thanks for your quick reply.

Besides the fact it misses DT schema update, why this is needed at all? 
------> we need a interface to configure the HCNT/LCNT via dts (current we just have ACPI to config it).
What's wrong with the existing DT timings? 
--------> the HCNT/LCNT value calculated by code is not accuracy. This is similar changes as the ACPI interface.
	i2c_dw_acpi_params(device, "SSCN", &dev->ss_hcnt, &dev->ss_lcnt, &ss_ht);
	i2c_dw_acpi_params(device, "FPCN", &dev->fp_hcnt, &dev->fp_lcnt, &fp_ht);
	i2c_dw_acpi_params(device, "HSCN", &dev->hs_hcnt, &dev->hs_lcnt, &hs_ht);
	i2c_dw_acpi_params(device, "FMCN", &dev->fs_hcnt, &dev->fs_lcnt, &fs_ht);
										   
- i2c-scl-falling-time-ns
- i2c-scl-internal-delay-ns
- i2c-scl-rising-time-ns
- i2c-sda-falling-time-ns
Yeah, I know those properties. But those are common for i2c.
My changes is for the specific HW controller(designware) for configuring its two register HCNT/LCNT.
In this case, still we need one property per each value?


--
With Best Regards,
Lawrence, Wang

-----Original Message-----
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> 
Sent: 2021年11月15日 18:28
To: Wang, Lawrence (NSB - CN/Hangzhou) <lawrence.wang@nokia-sbell.com>
Cc: jarkko.nikula@linux.intel.com; mika.westerberg@linux.intel.com; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org; Wang@wrlinb193.emea.nsn-net.net
Subject: Re: [PATCH] i2c: designware: Get HCNT/LCNT values from dts

On Mon, Nov 15, 2021 at 12:19:26PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 15, 2021 at 10:35:55AM +0100, Lawrence,Wang wrote:
> > From: "Wang, Lawrence" <lawrence.wang@nokia-sbell.com>
> > 
> > Current code support config the HCNT/LCNT only via ACPI method. for 
> > those platform that not support ACPI, will get the HCNT/LCNT value 
> > based on input clock. But it is not always accuracy. for example in 
> > some platform will get lower speed(320khz) in fast mode, and get 
> > faster speed(105khz/even more) in standard mode.
> > 
> > This patch makes it possible for the non-ACPI platform to pass more 
> > optimal HCNT/LCNT values to the core driver via dts if they are known beforehand.
> > If these are not set we use the calculated values.
> 
> Besides the fact it misses DT schema update, why this is needed at all?
> What's wrong with the existing DT timings?

Just for your convenience an excerpt from Documentation/devicetree/bindings/i2c/i2c.txt

- i2c-scl-falling-time-ns
Number of nanoseconds the SCL signal takes to fall; t(f) in the I2C specification.

- i2c-scl-internal-delay-ns
Number of nanoseconds the IP core additionally needs to setup SCL.

- i2c-scl-rising-time-ns
Number of nanoseconds the SCL signal takes to rise; t(r) in the I2C specification.

- i2c-sda-falling-time-ns
Number of nanoseconds the SDA signal takes to fall; t(f) in the I2C specification.

--
With Best Regards,
Andy Shevchenko

Re: [PATCH] i2c: designware: Get HCNT/LCNT values from dts

[Andy Shevchenko]

On Mon, Nov 15, 2021 at 10:41:06AM +0000, Wang, Lawrence (NSB - CN/Hangzhou) wrote:

Please, do not top post!

> Besides the fact it misses DT schema update, why this is needed at all? 
> ------> we need a interface to configure the HCNT/LCNT via dts (current we just have ACPI to config it).
> What's wrong with the existing DT timings? 
> --------> the HCNT/LCNT value calculated by code is not accuracy. This is similar changes as the ACPI interface.

So, _this_ is a problem, try to fix it instead of band aiding here and there.

P.S. You forgot to include proper people in the Cc list. I recommend to
consider using my script [1] for sending patches.

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH] i2c: designware: Get HCNT/LCNT values from dts

[Wang, Lawrence (NSB - CN/Hangzhou)]

So, _this_ is a problem, try to fix it instead of band aiding here and there.
--------> Please read the commit message carefully. Current code have those parameters
for HCNT/LCNT. The only missing is no interface to config it in non-ACPI platform. That is 
what this patch do. It is not 'band aiding'

Re: [PATCH] i2c: designware: Get HCNT/LCNT values from dts

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 352 bytes --]


> +	ret = device_property_read_u16_array(dev->dev, "dw-i2c-scl-timing",
> +					(u16 *)&i2c_scl_timing, sizeof(i2c_scl_timing)/sizeof(u16));

Putting hex values directly into DT is not a proper binding. They need
to be more generic. Sadly, I don't know the DW hardware, so I can't be
of much more help. But Andy already gave some pointers, as I saw.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]