Re: [PATCH 64/64] i2c: reword i2c_algorithm in drivers according to newest specification
On 3/22/24 3:25 PM, Wolfram Sang wrote:
Match the wording in i2c_algorithm in I2C drivers wrt. the newest I2C
v7, SMBus 3.2, I3C specifications and replace "master/slave" with more
appropriate terms. For some drivers, this means no more conversions are
needed. For the others more work needs to be done but this will be
performed incrementally along with API changes/improvements. All these
changes here are simple search/replace results.
Signed-off-by: Wolfram Sang
---
drivers/i2c/busses/i2c-amd-mp2-plat.c | 2 +-
drivers/i2c/busses/i2c-at91-master.c | 2 +-
drivers/i2c/busses/i2c-at91-slave.c| 8
drivers/i2c/busses/i2c-axxia.c | 10 +-
drivers/i2c/busses/i2c-cros-ec-tunnel.c| 2 +-
drivers/i2c/busses/i2c-designware-master.c | 2 +-
drivers/i2c/busses/i2c-designware-slave.c | 8
drivers/i2c/busses/i2c-diolan-u2c.c| 2 +-
drivers/i2c/busses/i2c-exynos5.c | 4 ++--
drivers/i2c/busses/i2c-gxp.c | 12 ++--
drivers/i2c/busses/i2c-hisi.c | 4 ++--
drivers/i2c/busses/i2c-img-scb.c | 2 +-
drivers/i2c/busses/i2c-imx.c | 12 ++--
drivers/i2c/busses/i2c-jz4780.c| 2 +-
drivers/i2c/busses/i2c-kempld.c| 2 +-
drivers/i2c/busses/i2c-meson.c | 4 ++--
drivers/i2c/busses/i2c-mlxbf.c | 8
drivers/i2c/busses/i2c-mt65xx.c| 2 +-
drivers/i2c/busses/i2c-mxs.c | 2 +-
drivers/i2c/busses/i2c-nomadik.c | 2 +-
drivers/i2c/busses/i2c-npcm7xx.c | 12 ++--
drivers/i2c/busses/i2c-nvidia-gpu.c| 4 ++--
drivers/i2c/busses/i2c-ocores.c| 8
drivers/i2c/busses/i2c-octeon-platdrv.c| 2 +-
drivers/i2c/busses/i2c-omap.c | 4 ++--
drivers/i2c/busses/i2c-opal.c | 4 ++--
drivers/i2c/busses/i2c-pasemi-core.c | 2 +-
drivers/i2c/busses/i2c-pnx.c | 2 +-
drivers/i2c/busses/i2c-pxa.c | 12 ++--
drivers/i2c/busses/i2c-qcom-cci.c | 2 +-
drivers/i2c/busses/i2c-qcom-geni.c | 2 +-
drivers/i2c/busses/i2c-robotfuzz-osif.c| 2 +-
drivers/i2c/busses/i2c-rzv2m.c | 8
drivers/i2c/busses/i2c-s3c2410.c | 4 ++--
drivers/i2c/busses/i2c-stm32f7.c | 14 +++---
drivers/i2c/busses/i2c-tegra-bpmp.c| 4 ++--
drivers/i2c/busses/i2c-tegra.c | 4 ++--
drivers/i2c/busses/i2c-thunderx-pcidrv.c | 2 +-
drivers/i2c/busses/i2c-virtio.c| 2 +-
drivers/i2c/busses/i2c-wmt.c | 2 +-
drivers/i2c/busses/i2c-xiic.c | 2 +-
41 files changed, 95 insertions(+), 95 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-master.c
b/drivers/i2c/busses/i2c-designware-master.c
index c7e56002809a..14c61b31f877 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -832,7 +832,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
}
static const struct i2c_algorithm i2c_dw_algo = {
- .master_xfer = i2c_dw_xfer,
+ .xfer = i2c_dw_xfer,
.functionality = i2c_dw_func,
};
diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c
index 2e079cf20bb5..b47ad6b16814 100644
--- a/drivers/i2c/busses/i2c-designware-slave.c
+++ b/drivers/i2c/busses/i2c-designware-slave.c
@@ -58,7 +58,7 @@ static int i2c_dw_init_slave(struct dw_i2c_dev *dev)
return 0;
}
-static int i2c_dw_reg_slave(struct i2c_client *slave)
+static int i2c_dw_reg_target(struct i2c_client *slave)
{
struct dw_i2c_dev *dev = i2c_get_adapdata(slave->adapter);
@@ -83,7 +83,7 @@ static int i2c_dw_reg_slave(struct i2c_client *slave)
return 0;
}
-static int i2c_dw_unreg_slave(struct i2c_client *slave)
+static int i2c_dw_unreg_target(struct i2c_client *slave)
{
struct dw_i2c_dev *dev = i2c_get_adapdata(slave->adapter);
@@ -214,8 +214,8 @@ static irqreturn_t i2c_dw_isr_slave(int this_irq, void *dev_id)
static const struct i2c_algorithm i2c_dw_algo = {
.functionality = i2c_dw_func,
- .reg_slave = i2c_dw_reg_slave,
- .unreg_slave = i2c_dw_unreg_slave,
+ .reg_target = i2c_dw_reg_target,
+ .unreg_target = i2c_dw_unreg_target,
};
Acked-by: Jarkko Nikula
Re: [PATCH v6 3/5] i2c: add support for HiSilicon I2C controller
Hi
On 3/31/21 4:36 PM, Yicong Yang wrote:
+ ret = device_property_read_u64(dev, "clk_rate", >clk_rate_khz);
+ if (ret) {
+ dev_err(dev, "failed to get clock frequency, ret = %d\n", ret);
+ return ret;
+ }
+
+ ctlr->clk_rate_khz = DIV_ROUND_UP_ULL(ctlr->clk_rate_khz, HZ_PER_KHZ);
+
I'd use a temp variable here for reading the "clk_rate" property in
Hertz and calculating the derived kHz value from it. As a bonus allow to
use u32 for clk_rate_khz instead of u64. u32 will still provide plenty
of headroom :-)
Reason for temp variable is for me it's confusing to see statement like
"rate_khz = rate_khz / 1000".
Jarkko
Re: [PATCH v1 2/2] i2c: designware: Get rid of legacy platform data
On 4/6/21 1:49 PM, Wolfram Sang wrote: On Wed, Mar 31, 2021 at 06:48:51PM +0300, Andy Shevchenko wrote: Platform data is a legacy interface to supply device properties to the driver. In this case we don't have anymore in-kernel users for it. Just remove it for good. Signed-off-by: Andy Shevchenko Acked-by: Wolfram Sang Acked-by: Jarkko Nikula
Re: [PATCH v1 1/1] i2c: designware: Switch over to i2c_freq_mode_string()
On 3/30/21 4:46 PM, Andy Shevchenko wrote: Use generic i2c_freq_mode_string() helper to print chosen bus speed. Signed-off-by: Andy Shevchenko --- Depends on the "Add support for HiSilicon I2C controller" series. Message-Id - 1617109549-4013-1-git-send-email-yangyic...@hisilicon.com Yicong, feel free to attach to your new version of it. drivers/i2c/busses/i2c-designware-master.c | 20 1 file changed, 4 insertions(+), 16 deletions(-) Please note kernel test robot reported some issues with this patch before included in this series. Acked-by: Jarkko Nikula
Re: [PATCH v5 2/5] i2c: core: add api to provide frequency mode strings
Hi
On 3/30/21 5:19 PM, Yicong Yang wrote:
Some I2C drivers like Designware and HiSilicon will print the
bus frequency mode information, so add a public one that everyone
can make use of.
Reviewed-by: Andy Shevchenko
Signed-off-by: Yicong Yang
---
include/linux/i2c.h | 20
1 file changed, 20 insertions(+)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 10bd0b0..6837e64 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -47,6 +47,26 @@ typedef int (*i2c_slave_cb_t)(struct i2c_client *client,
#define I2C_MAX_HIGH_SPEED_MODE_FREQ 340
#define I2C_MAX_ULTRA_FAST_MODE_FREQ 500
+static inline const char *i2c_freq_mode_string(u32 bus_freq_hz)
+{
+ switch (bus_freq_hz) {
+ case I2C_MAX_STANDARD_MODE_FREQ:
+ return "Standard Mode(100KHz)";
+ case I2C_MAX_FAST_MODE_FREQ:
+ return "Fast Mode(400KHz)";
+ case I2C_MAX_FAST_MODE_PLUS_FREQ:
+ return "Fast Mode Plus(1.0MHz)";
+ case I2C_MAX_TURBO_MODE_FREQ:
+ return "Turbo Mode(1.4MHz)";
+ case I2C_MAX_HIGH_SPEED_MODE_FREQ:
+ return "High Speed Mode(3.4MHz)";
+ case I2C_MAX_ULTRA_FAST_MODE_FREQ:
+ return "Ultra Fast Mode(5.0MHz)";
+ default:
+ return "Unknown Mode";
+ }
A few minor nits here:
- KHz -> kHz
- Space between text and opening parenthesis: "Mode(" -> "Mode ("
- Space between number and unit: (1.0MHz) -> (1.0 MHz)
With those changes you may add my
Reviewed-by: Jarkko Nikula
Tested-by: Jarkko Nikula
Re: [PATCH v2] i2c: designware: Add driver support for AMD NAVI GPU
Hi
On 3/22/21 6:59 PM, Sanket Goswami wrote:
The Latest AMD NAVI GPU card has an integrated Type-C controller and
Designware I2C with PCI Interface. The Type-C controller can be
accessed over I2C. The client driver is part of the USB Type-C UCSI
driver.
Also, there exists a couple of notable IP limitations that are dealt as
workarounds:
- I2C transaction work on a polling mode as IP does not generate
interrupt.
- I2C read command sent twice to address the IP issues.
Reviewed-by: Shyam Sundar S K
Co-developed-by: Nehal Bakulchandra Shah
Signed-off-by: Nehal Bakulchandra Shah
Signed-off-by: Sanket Goswami
---
Changes in v2:
- Utilized existing functionality of i2c_dw_xfer_init to configure I2C
bus.
- Removed i2c_dw_populate_client and rewrrient navi_amd_register_client
to deduplicate from existing drivers.
- Addressed review comments from Andy.
drivers/i2c/busses/i2c-designware-common.c | 3 +
drivers/i2c/busses/i2c-designware-core.h | 3 +
drivers/i2c/busses/i2c-designware-master.c | 136 +
drivers/i2c/busses/i2c-designware-pcidrv.c | 57 +
4 files changed, 199 insertions(+)
diff --git a/drivers/i2c/busses/i2c-designware-common.c
b/drivers/i2c/busses/i2c-designware-common.c
index 3c19aada4b30..50883a70b482 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -150,6 +150,9 @@ int i2c_dw_init_regmap(struct dw_i2c_dev *dev)
reg = readl(dev->base + DW_IC_COMP_TYPE);
i2c_dw_release_lock(dev);
+ if (dev->flags & AMD_NON_INTR_MODE)
+ map_cfg.max_register = AMD_UCSI_INTR_REG;
+
if (reg == swab32(DW_IC_COMP_TYPE_VALUE)) {
map_cfg.reg_read = dw_reg_read_swab;
map_cfg.reg_write = dw_reg_write_swab;
diff --git a/drivers/i2c/busses/i2c-designware-core.h
b/drivers/i2c/busses/i2c-designware-core.h
index 5392b82f68a4..f29923c452ac 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -293,9 +293,12 @@ struct dw_i2c_dev {
#define ACCESS_INTR_MASK BIT(0)
#define ACCESS_NO_IRQ_SUSPEND BIT(1)
+#define AMD_NON_INTR_MODE BIT(2)
#define MODEL_MSCC_OCELOT BIT(8)
#define MODEL_BAIKAL_BT1 BIT(9)
#define MODEL_MASKGENMASK(11, 8)
+#define AMD_UCSI_INTR_EN 0xd
+#define AMD_UCSI_INTR_REG 0x474
int i2c_dw_init_regmap(struct dw_i2c_dev *dev);
u32 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset);
diff --git a/drivers/i2c/busses/i2c-designware-master.c
b/drivers/i2c/busses/i2c-designware-master.c
index dd27b9dbe931..a76e1f992850 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -23,6 +23,10 @@
#include "i2c-designware-core.h"
+#define AMD_TIMEOUT_MIN_MSEC 1
+#define AMD_TIMEOUT_MAX_MSEC 11000
+#define AMD_MASTERCFG_MASK GENMASK(15, 0)
+
static void i2c_dw_configure_fifo_master(struct dw_i2c_dev *dev)
{
/* Configure Tx/Rx FIFO threshold levels */
@@ -208,6 +212,13 @@ static int i2c_dw_init_master(struct dw_i2c_dev *dev)
if (dev->sda_hold_time)
regmap_write(dev->map, DW_IC_SDA_HOLD, dev->sda_hold_time);
+ /*
+* In order to enable the interrupt for UCSI i.e. AMD NAVI GPU card,
+* it is mandatory to set the right value in specific register
+* (offset:0x474) as per the hardware IP specification.
+*/
+ if (dev->flags & AMD_NON_INTR_MODE)
+ regmap_write(dev->map, AMD_UCSI_INTR_REG, AMD_UCSI_INTR_EN);
This confuses me - this patch is about adding support for DesignWare IP
that does not generate interrupts but here code is enabling an
interrupt. I guess it's for UCSI but should above code then go to a
driver handling that IP?
+static int i2c_dw_check_stopbit(struct dw_i2c_dev *i2cd)
...
+static int i2c_dw_status(struct dw_i2c_dev *i2cd)
...
+static int amd_i2c_dw_master_xfer(struct i2c_adapter *adap, struct i2c_msg
*msgs, int num_msgs)
...
+{
+ struct dw_i2c_dev *i2cd = i2c_get_adapdata(adap);
For uniformity I'd use struct dw_i2c_dev *dev name instead of *i2cd
since that what driver uses currently in other places.
@@ -461,6 +587,13 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
pm_runtime_get_sync(dev->dev);
+ /*
+* Initiate I2C message transfer when AMD NAVI GPU card is enabled,
+* As it is polling based transfer mechanism, which does not support
+* interrupt based functionalities of existing DesignWare driver.
+*/
+ if (dev->flags & AMD_NON_INTR_MODE)
+ return amd_i2c_dw_master_xfer(adap, msgs, num);
Does runtime PM go out of sync here?
Jarkko
Re: [PATCH v5 3/4] i2c: designware: Use pcim_alloc_irq_vectors() to allocate IRQ vectors
On 2/26/21 5:50 PM, Dejin Zheng wrote: The pcim_alloc_irq_vectors() function, an explicit device-managed version of pci_alloc_irq_vectors(). If pcim_enable_device() has been called before, then pci_alloc_irq_vectors() is actually a device-managed function. It is used here as a device-managed function, So replace it with pcim_alloc_irq_vectors(). At the same time, Remove the pci_free_irq_vectors() function to simplify the error handling path. the freeing resources will take automatically when device is gone. Acked-by: Andy Shevchenko Signed-off-by: Dejin Zheng --- Acked-by: Jarkko Nikula
Re: [PATCH] spi: pca2xx-pci: Fix an issue about missing call to 'pci_free_irq_vectors()'
On 2/15/21 11:23 AM, Jan Kiszka wrote:
On 14.02.21 15:57, Dejin Zheng wrote:
Call to 'pci_free_irq_vectors()' are missing both in the error handling
path of the probe function, and in the remove function. So add them.
Fixes: 64e02cb0bdfc7c ("spi: pca2xx-pci: Allow MSI")
Signed-off-by: Dejin Zheng
---
drivers/spi/spi-pxa2xx-pci.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-pci.c
index 14fc41ed2361..1ec840e78ff4 100644
--- a/drivers/spi/spi-pxa2xx-pci.c
+++ b/drivers/spi/spi-pxa2xx-pci.c
@@ -254,8 +254,10 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev,
snprintf(buf, sizeof(buf), "pxa2xx-spi.%d", ssp->port_id);
ssp->clk = clk_register_fixed_rate(>dev, buf , NULL, 0,
c->max_clk_rate);
-if (IS_ERR(ssp->clk))
- return PTR_ERR(ssp->clk);
+ if (IS_ERR(ssp->clk)) {
+ ret = PTR_ERR(ssp->clk);
+ goto err_irq;
+ }
memset(, 0, sizeof(pi));
pi.fwnode = dev->dev.fwnode;
@@ -268,12 +270,16 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev,
pdev = platform_device_register_full();
if (IS_ERR(pdev)) {
clk_unregister(ssp->clk);
- return PTR_ERR(pdev);
+ ret = PTR_ERR(pdev);
+ goto err_irq;
}
pci_set_drvdata(dev, pdev);
return 0;
+err_irq:
+ pci_free_irq_vectors(dev);
+ return ret;
}
static void pxa2xx_spi_pci_remove(struct pci_dev *dev)
@@ -283,6 +289,7 @@ static void pxa2xx_spi_pci_remove(struct pci_dev *dev)
spi_pdata = dev_get_platdata(>dev);
+ pci_free_irq_vectors(dev);
platform_device_unregister(pdev);
clk_unregister(spi_pdata->ssp.clk);
}
Reviewed-by: Jan Kiszka
Please fix pca2xx-pci -> pxa2xx-pci in the subject line. With that
change you may add:
Reviewed-by: Jarkko Nikula
Re: [PATCH i2c-next] i2c: designware: Consolidate pci_free_irq_vectors to a single place
On 2/14/21 8:45 AM, Dejin Zheng wrote: Consolidate pci_free_irq_vectors to a single place using "goto free_irq" for simplify the code. Signed-off-by: Dejin Zheng --- drivers/i2c/busses/i2c-designware-pcidrv.c | 22 ++ 1 file changed, 10 insertions(+), 12 deletions(-) Acked-by: Jarkko Nikula
Re: [PATCH 1/2] MAINTAINERS: Update email address for TI ASoC and twl4030 codec drivers
On 15.12.2020 15.05, Peter Ujfalusi wrote: > My employment with TI is coming to an end, it is my intention to look after > the drivers I have worked with over the years. > > Signed-off-by: Peter Ujfalusi > Signed-off-by: Peter Ujfalusi > --- > MAINTAINERS | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index f6e7162241eb..a091f183b27f 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -12862,7 +12862,7 @@ F:include/misc/ocxl* > F: include/uapi/misc/ocxl.h > > OMAP AUDIO SUPPORT > -M: Peter Ujfalusi > +M: Peter Ujfalusi > M: Jarkko Nikula > L: alsa-de...@alsa-project.org (moderated for non-subscribers) > L: linux-o...@vger.kernel.org > @@ -17537,7 +17537,7 @@ F:arch/xtensa/ > F: drivers/irqchip/irq-xtensa-* > Acked-by: Jarkko Nikula
Re: [PATCH v1] i2c: nvidia-gpu: drop empty stub for runtime pm
On 11/11/20 12:54 PM, Vaibhav Gupta wrote: On Tue, Nov 10, 2020 at 02:33:43PM +0200, Jarkko Nikula wrote: +#define gpu_i2c_suspend NULL Perhaps we can put NULL directly into UNIVERSAL_DEV_PM_OPS() for the suspend callback? Yes. I have noticed that the approach for this is random. Many drivers pass NULL directly to the dev_pm_ops type variable, and rest of them use a macro. I used it for symmetry. I mean there is 'gpu_i2c_resume', so although a macro, I have put a 'gpu_i2c_suspend'. Although it won't make any significant change, but if required, I can send another patch where NULL is passed into UNIVERSAL_DEV_PM_OPS() instead. No need to resend from my side, it was just a remark and I gave already the reviewed-by tag. Jarkko
Re: [PATCH v1] i2c: nvidia-gpu: drop empty stub for runtime pm
On 11/7/20 11:04 AM, Vaibhav Gupta wrote:
On Sat, Nov 07, 2020 at 01:51:51PM +0530, Vaibhav Gupta wrote:
After the commit c5eb1190074c ("PCI / PM: Allow runtime PM without callback
functions") we no more need empty stubs for runtime-pm to work.
The driver has no device specific task(s) for .suspend() . The stub was
placed just for runtime-pm, which can be dropped now.
Reported-by: Bjorn Helgaas
Signed-off-by: Vaibhav Gupta
---
drivers/i2c/busses/i2c-nvidia-gpu.c | 10 +-
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c
b/drivers/i2c/busses/i2c-nvidia-gpu.c
index f9a69b109e5c..6b20601ffb13 100644
--- a/drivers/i2c/busses/i2c-nvidia-gpu.c
+++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
@@ -353,15 +353,7 @@ static void gpu_i2c_remove(struct pci_dev *pdev)
pci_free_irq_vectors(pdev);
}
-/*
- * We need gpu_i2c_suspend() even if it is stub, for runtime pm to work
- * correctly. Without it, lspci shows runtime pm status as "D0" for the card.
- * Documentation/power/pci.rst also insists for driver to provide this.
- */
-static __maybe_unused int gpu_i2c_suspend(struct device *dev)
-{
- return 0;
-}
+#define gpu_i2c_suspend NULL
Perhaps we can put NULL directly into UNIVERSAL_DEV_PM_OPS() for the
suspend callback?
static __maybe_unused int gpu_i2c_resume(struct device *dev)
{
--
2.28.0
The patch is only compile-tested.
It should work also system suspend point of view. This patch affects
also it since callbacks are set with the UNIVERSAL_DEV_PM_OPS(). Means
the same callback is used for runtime PM and system suspend.
I quickly debugged this with an another driver and PCI stack does put
the device into D3 state in system suspend from pci_prepare_to_sleep()
if the suspend callback is not set and device was on prior it.
Reviewed-by: Jarkko Nikula
Re: [PATCH 2/2] i2c: designware: slave should do WRITE_REQUESTED before WRITE_RECEIVED
On 11/4/20 12:17 PM, michael...@vatics.com wrote: Hi Wolfram, dev->status can be used to record the current state, especially Designware I2C controller has no interrupts to identify a write-request. This patch Just double-checking: the designware HW does not raise an interrupt when its own address + RW bit has been received? Not exactly. There're an interrupt state name "RD_REQ" but no one named like "WR_REQ". For read-request, the slave will get a RD_REQ interrupt. For write-request, the slave won't be interrupted until data arrived to trigger interrupt "RX_FULL". I tried to use GPIO to simulate an I2C master. I only sent its own address + W bit without any data and then I got only a STOP_DET interrupt. If I sent its own address + W bit + one byte data and then I got one RX_FULL and a STOP_DET. It seems the controller doesn't interrupt when RW bit is W, but R does. What do you think, Jarkko? Yes, the datasheet has a flowchart for slave mode and it shows for a write only RX_FULL interrupt followed by read from IC_DATA_CMD to retrieve received byte. Which I believe won't occur if there is no incoming data byte and only STOP_DET happens as you have observed. The flowchart however doesn't include the STOP_DET flow. Jarkko
Re: [PATCH 2/2] i2c: designware: slave should do WRITE_REQUESTED before WRITE_RECEIVED
On 10/30/20 10:04 AM, Michael Wu wrote: Sometimes we would get the following flow when doing an i2cset: 0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4 I2C_SLAVE_WRITE_RECEIVED 0x1 STATUS SLAVE_ACTIVITY=0x0 : RAW_INTR_STAT=0x714 : INTR_STAT=0x204 I2C_SLAVE_WRITE_REQUESTED I2C_SLAVE_WRITE_RECEIVED Documentation/i2c/slave-interface.rst says that I2C_SLAVE_WRITE_REQUESTED, which is mandatory, should be sent while the data did not arrive yet. It means in a write-request I2C_SLAVE_WRITE_REQUESTED should be reported before any I2C_SLAVE_WRITE_RECEIVED. By the way, I2C_SLAVE_STOP didn't be reported in the above case because DW_IC_INTR_STAT was not 0x200. dev->status can be used to record the current state, especially Designware I2C controller has no interrupts to identify a write-request. This patch makes not only I2C_SLAVE_WRITE_REQUESTED been reported first when IC_INTR_RX_FULL is rising and dev->status isn't STATUS_WRITE_IN_PROGRESS but also I2C_SLAVE_STOP been reported when a STOP condition is received. Signed-off-by: Michael Wu --- drivers/i2c/busses/i2c-designware-slave.c | 45 +-- 1 file changed, 18 insertions(+), 27 deletions(-) Acked-by: Jarkko Nikula
Re: [PATCH v3] i2c: designware: call i2c_dw_read_clear_intrbits_slave() once
On 10/23/20 8:40 AM, Michael Wu wrote: If some bits were cleared by i2c_dw_read_clear_intrbits_slave() in i2c_dw_isr_slave() and not handled immediately, those cleared bits would not be shown again by later i2c_dw_read_clear_intrbits_slave(). They therefore were forgotten to be handled. i2c_dw_read_clear_intrbits_slave() should be called once in an ISR and take its returned state for all later handlings. Signed-off-by: Michael Wu --- Change in v3: - revert deleted braces of 'else' branch in v2 Change in v2: - revert moving I2C_SLAVE_WRITE_REQUESTED reporting in v1 drivers/i2c/busses/i2c-designware-slave.c | 7 +-- 1 file changed, 1 insertion(+), 6 deletions(-) Acked-by: Jarkko Nikula
Re: [PATCH v2] i2c: designware: call i2c_dw_read_clear_intrbits_slave() once
Hi
On 10/22/20 8:46 AM, Michael Wu wrote:
@@ -217,10 +214,8 @@ static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
if (!i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_RECEIVED,
))
dev_vdbg(dev->dev, "Byte %X acked!", val);
- } else {
+ } else
i2c_slave_event(dev->slave, I2C_SLAVE_STOP, );
- stat = i2c_dw_read_clear_intrbits_slave(dev);
- }
Minor nit. Please don't remove braces here since the upper part of if
statement has them.
From Documentation/process/coding-style.rst:
"
This does not apply if only one branch of a conditional statement is a
single
statement; in the latter case use braces in both branches:
.. code-block:: c
if (condition) {
do_this();
do_that();
} else {
otherwise();
}
"
Otherwise looks good to me.
Jarkko
Re: [PATCH] i2c: designware: call i2c_dw_read_clear_intrbits_slave() once
Hi On 10/20/20 11:33 AM, Michael Wu wrote: i2c_dw_read_clear_intrbits_slave() was called per each interrupt handle. It caused some interrupt bits which haven't been handled yet were cleared, the corresponding handlers would do nothing due to interrupt bits been discarded. For example, $ i2cset -f -y 2 0x42 0x00 0x41; dmesg -c [0][clear_intrbits]0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4 [1][irq_handler ]0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4 WRITE_RECEIVED [0][clear_intrbits]0x1 STATUS SLAVE_ACTIVITY=0x0 : RAW_INTR_STAT=0x714 : INTR_STAT=0x204 [1][irq_handler ]0x1 STATUS SLAVE_ACTIVITY=0x0 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4 WRITE_RECEIVED t1: ISR with the 1st IC_INTR_RX_FULL. t2: Clear listed IC_INTR bits by i2c_dw_read_clear_intrbits_slave(). t3: Enter i2c_dw_irq_handler_slave() and then do i2c_slave_event(WRITE_RECEIVED) because if (stat & DW_IC_INTR_RX_FULL). t4: ISR with both IC_INTR_STOP_DET and the 2nd IC_INTR_RX_FULL. t5: Clear listed IC_INTR bits by i2c_dw_read_clear_intrbits_slave(). The current IC_INTR_STOP_DET is cleared by this i2c_dw_read_clear_intrbits_slave(). t6: Enter i2c_dw_irq_handler_slave() and then do i2c_slave_event(WRITE_RECEIVED) because if (stat & DW_IC_INTR_RX_FULL). t7: i2c_slave_event(STOP) never be done because IC_INTR_STOP_DET was cleared in t5. The root cause is that i2c_dw_read_clear_intrbits_slave() was called many times. Calling i2c_dw_read_clear_intrbits_slave() once in one ISR and take the returned stat for later handling is the solution. Signed-off-by: Michael Wu --- drivers/i2c/busses/i2c-designware-slave.c | 16 +--- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c index 44974b53a626..02e7c5171827 100644 --- a/drivers/i2c/busses/i2c-designware-slave.c +++ b/drivers/i2c/busses/i2c-designware-slave.c @@ -159,7 +159,6 @@ static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev) u32 raw_stat, stat, enabled, tmp; u8 val = 0, slave_activity; - regmap_read(dev->map, DW_IC_INTR_STAT, ); regmap_read(dev->map, DW_IC_ENABLE, ); regmap_read(dev->map, DW_IC_RAW_INTR_STAT, _stat); regmap_read(dev->map, DW_IC_STATUS, ); @@ -168,13 +167,11 @@ static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev) if (!enabled || !(raw_stat & ~DW_IC_INTR_ACTIVITY) || !dev->slave) return 0; + stat = i2c_dw_read_clear_intrbits_slave(dev); dev_dbg(dev->dev, "%#x STATUS SLAVE_ACTIVITY=%#x : RAW_INTR_STAT=%#x : INTR_STAT=%#x\n", enabled, slave_activity, raw_stat, stat); - if ((stat & DW_IC_INTR_RX_FULL) && (stat & DW_IC_INTR_STOP_DET)) - i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_REQUESTED, ); - ... + + if ((stat & DW_IC_INTR_RX_FULL) && (stat & DW_IC_INTR_STOP_DET)) + i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_REQUESTED, ); Was this move a leftover that got committed by accident? I think it's better to have this logic change in another patch. Or was it even questionable to move the I2C_SLAVE_WRITE_REQUESTED reporting after all other? Jarkko
Re: [PATCH] i2c: designware: fix slave omitted IC_INTR_STOP_DET
Hi On 10/14/20 8:25 AM, Michael Wu wrote: When an I2C slave works, sometimes both IC_INTR_RX_FULL and IC_INTR_STOP_DET are rising during an IRQ handle, especially when system is busy or too late to handle interrupts. If IC_INTR_RX_FULL is rising and the system doesn't handle immediately, IC_INTR_STOP_DET may be rising and the system has to handle these two events. For this there may be two problems: e 1. IC_INTR_STOP_DET is rising after i2c_dw_read_clear_intrbits_slave() done: It seems invalidated because WRITE_REQUESTED is done after the 1st WRITE_RECEIVED. $ i2cset -f -y 2 0x42 0x00 0x41; dmesg -c [0][clear_intrbits]0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4 [1][irq_handler ]0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4 WRITE_RECEIVED [0][clear_intrbits]0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4 [1][irq_handler ]0x1 STATUS SLAVE_ACTIVITY=0x0 : RAW_INTR_STAT=0x714 : INTR_STAT=0x204 WRITE_REQUESTED WRITE_RECEIVED [0][clear_intrbits]0x1 STATUS SLAVE_ACTIVITY=0x0 : RAW_INTR_STAT=0x710 : INTR_STAT=0x200 [1][irq_handler ]0x1 STATUS SLAVE_ACTIVITY=0x0 : RAW_INTR_STAT=0x510 : INTR_STAT=0x0 STOP [2][clear_intrbits]0x1 STATUS SLAVE_ACTIVITY=0x0 : RAW_INTR_STAT=0x510 : INTR_STAT=0x0 t1: ISR with the 1st IC_INTR_RX_FULL. t2: Clear listed IC_INTR bits by i2c_dw_read_clear_intrbits_slave(). t3: Enter i2c_dw_irq_handler_slave() and then do i2c_slave_event(WRITE_RECEIVED) because if (stat & DW_IC_INTR_RX_FULL). t4: ISR with the 2nd IC_INTR_RX_FULL. t5: Clear listed IC_INTR bits by i2c_dw_read_clear_intrbits_slave(), while IC_INTR_STOP_DET has not risen yet. t6: Enter i2c_dw_irq_handler_slave() and then IC_INTR_STOP_DET is rising. i2c_slave_event(WRITE_REQUESTED) will be done first because if ((stat & DW_IC_INTR_RX_FULL) && (stat & DW_IC_INTR_STOP_DET)) and then doing i2c_slave_event(WRITE_RECEIVED). t7: do i2c_slave_event(STOP) due to IC_INTR_STOP_DET not be cleared yet. 2. Both IC_INTR_STOP_DET and IC_INTR_RX_FULL are rising before i2c_dw_read_clear_intrbits_slave(): STOP cannot wait because IC_INTR_STOP_DET is cleared by i2c_dw_read_clear_intrbits_slave(). $ i2cset -f -y 2 0x42 0x00 0x41; dmesg -c [0][clear_intrbits]0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4 [1][irq_handler ]0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4 WRITE_RECEIVED [0][clear_intrbits]0x1 STATUS SLAVE_ACTIVITY=0x0 : RAW_INTR_STAT=0x714 : INTR_STAT=0x204 [1][irq_handler ]0x1 STATUS SLAVE_ACTIVITY=0x0 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4 WRITE_RECEIVED t1: ISR with the 1st IC_INTR_RX_FULL. t2: Clear listed IC_INTR bits by i2c_dw_read_clear_intrbits_slave(). t3: Enter i2c_dw_irq_handler_slave() and then do i2c_slave_event(WRITE_RECEIVED) because if (stat & DW_IC_INTR_RX_FULL). t4: ISR with both IC_INTR_STOP_DET and the 2nd IC_INTR_RX_FULL. t5: Clear listed IC_INTR bits by i2c_dw_read_clear_intrbits_slave(). The current IC_INTR_STOP_DET is cleared by this i2c_dw_read_clear_intrbits_slave(). t6: Enter i2c_dw_irq_handler_slave() and then do i2c_slave_event(WRITE_RECEIVED) because if (stat & DW_IC_INTR_RX_FULL). t7: i2c_slave_event(STOP) never be done because IC_INTR_STOP_DET was cleared in t5. In order to resolve these problems, i2c_dw_read_clear_intrbits_slave() should be called only one time in ISR and take the returned stat to handle those occurred events. Signed-off-by: Michael Wu --- drivers/i2c/busses/i2c-designware-slave.c | 79 --- 1 file changed, 40 insertions(+), 39 deletions(-) Thanks for the patch. I was thinking this too after your report but haven't found actually time to look at implementing it. But what I was thinking it is probably good to have two patches. First patch that changes only i2c_dw_read_clear_intrbits_slave() semantics so that it's called only once like here and second patch that does other logic changes. Makes easier to catch possible regressions I think. Jarkko
Re: [PATCH v2 0/4] i2c-hid: Save power by reducing i2c xfers with block reads
On 9/17/20 5:02 PM, Andy Shevchenko wrote: On Thu, Sep 17, 2020 at 8:26 AM Sultan Alsawaf wrote: From: Sultan Alsawaf This is a fixed resubmission of "[PATCH 0/2] i2c-hid: Save power by reducing i2c xfers with block reads". That original patchset did not have enough fixes for the designware i2c adapter's I2C_M_RECV_LEN feature, which is documented extensively in the original email thread. Here is the original cover letter, which still applies: "I noticed on my Dell Precision 15 5540 with an i9-9880H that simply putting my finger on the touchpad would increase my system's power consumption by 4W, which is quite considerable. Resting my finger on the touchpad would generate roughly 4000 i2c irqs per second, or roughly 20 i2c irqs per touchpad irq. Upon closer inspection, I noticed that the i2c-hid driver would always transfer the maximum report size over i2c (which is 60 bytes for my touchpad), but all of my touchpad's normal touch events are only 32 bytes long according to the length byte contained in the buffer sequence. Therefore, I was able to save about 2W of power by passing the I2C_M_RECV_LEN flag in i2c-hid, which says to look for the payload length in the first byte of the transfer buffer and adjust the i2c transaction accordingly. The only problem though is that my i2c controller's driver allows bytes other than the first one to be used to retrieve the payload length, which is incorrect according to the SMBus spec, and would break my i2c-hid change since not *all* of the reports from my touchpad are conforming SMBus block reads. This patchset fixes the I2C_M_RECV_LEN behavior in the designware i2c driver and modifies i2c-hid to use I2C_M_RECV_LEN to save quite a bit of power. Even if the peripheral controlled by i2c-hid doesn't support block reads, the i2c controller drivers should cope with this and proceed with the i2c transfer using the original requested length." Reviewed-by: Andy Shevchenko for I²C DesignWare patches. Also for i2c-designware Acked-by: Jarkko Nikula
Re: [PATCH v2 2/4] i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads
On 9/17/20 8:22 AM, Sultan Alsawaf wrote:
From: Sultan Alsawaf
The point of adding a byte to len in i2c_dw_recv_len() is to make sure
that tx_buf_len is nonzero, so that i2c_dw_xfer_msg() can let the i2c
controller know that the i2c transaction can end. Otherwise, the i2c
controller will think that the transaction can never end for block
reads, which results in the stop-detection bit never being set and thus
the transaction timing out.
Adding a byte to len is not a reliable way to do this though; sometimes
it lets tx_buf_len become zero, which results in the scenario described
above. Therefore, just directly ensure tx_buf_len cannot be zero to fix
the issue.
Fixes: c3ae106050b9 ("i2c: designware: Implement support for SMBus block read and
write")
Signed-off-by: Sultan Alsawaf
---
drivers/i2c/busses/i2c-designware-master.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Were other patches in series dropped somewhere? I received only this.
Jarkko
Re: [PATCH v3] i2c: Squash of SMBus block read patchset to save power
On 9/15/20 8:48 PM, Sultan Alsawaf wrote:
On Tue, Sep 15, 2020 at 02:55:48PM +0300, Jarkko Nikula wrote:
I tested this on top of fc4f28bb3daf ("Merge tag 'for-5.9-rc5-tag' of
git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux") and seems to be
working fine. What was the key change compared to previous version that was
regressing for me?
This change fixed your issue (and my issue with 5.8):
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -395,8 +395,9 @@ i2c_dw_recv_len(struct dw_i2c_dev *dev, u8 len)
* Adjust the buffer length and mask the flag
* after receiving the first byte.
*/
- len += (flags & I2C_CLIENT_PEC) ? 2 : 1;
- dev->tx_buf_len = len - min_t(u8, len, dev->rx_outstanding);
+ if (flags & I2C_CLIENT_PEC)
+ len++;
+ dev->tx_buf_len = len - min_t(u8, len - 1, dev->rx_outstanding);
msgs[dev->msg_read_idx].len = len;
msgs[dev->msg_read_idx].flags &= ~I2C_M_RECV_LEN;
I've attributed this change with the following commit message:
"i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads
The point of adding a byte to len in i2c_dw_recv_len() is to make sure
that tx_buf_len is nonzero, so that i2c_dw_xfer_msg() can let the i2c
controller know that the i2c transaction can end. Otherwise, the i2c
controller will think that the transaction can never end for block
reads, which results in the stop-detection bit never being set and thus
the transaction timing out.
Adding a byte to len is not a reliable way to do this though; sometimes
it lets tx_buf_len become zero, which results in the scenario described
above. Therefore, just directly ensure tx_buf_len cannot be zero to fix
the issue."
Ok, nice that you found it.
Does the patch series look good to submit?
Yes, go ahead.
Jarkko
Re: [PATCH v1] x86/defconfigs: Unbreak 32-bit defconfig builds
Hi
On 9/8/20 3:13 PM, Ingo Molnar wrote:
* Andy Shevchenko wrote:
After the commit 1d0e12fd3a84 ("x86/defconfigs: Refresh defconfig files")
32-bit builds using defconfig become broken because on x86_64 build host
with no ARCH provided the default behaviour is to assume 64-bit independently
on the configuration file name. The crucial part is CONFIG_64BIT option
that used to be explicit. Let restore the latter option in order to unbreak
32-bit builds.
So exactly which build method broke due to this? The typical way to do a
defconfig build is:
make ARCH=i386 defconfig
which still works fine AFAICS.
Maybe wrong way to do it, just plain "make i386_defconfig". I'm aware of
ARCH=, but never needed to specify it for x86 targets.
Jarkko
Re: [PATCH 1/2] i2c: i801: Fix runtime PM
On 8/31/20 6:15 PM, Vaibhav Gupta wrote:
On Fri, Aug 28, 2020 at 11:26:40AM -0500, Bjorn Helgaas wrote:
[+cc Vaibhav]
On Wed, Jun 27, 2018 at 04:23:40PM -0500, Bjorn Helgaas wrote:
[+cc Rafael, linux-pm, linux-kernel]
On Wed, Jun 27, 2018 at 10:15:50PM +0200, Jean Delvare wrote:
Hi Jarkko,
On Tue, 26 Jun 2018 17:39:12 +0300, Jarkko Nikula wrote:
Commit 9c8088c7988 ("i2c: i801: Don't restore config registers on
runtime PM") nullified the runtime PM suspend/resume callback pointers
while keeping the runtime PM enabled. This causes that device stays in
D0 power state and sysfs /sys/bus/pci/devices/.../power/runtime_status
shows "error" when runtime PM framework attempts to autosuspend the
device.
This is due PCI bus runtime PM which checks for driver runtime PM
callbacks and returns with -ENOSYS if they are not set. Fix this by
having a shared dummy runtime PM callback that returns with success.
Fixes: a9c8088c7988 ("i2c: i801: Don't restore config registers on runtime PM")
I don't want to sound like I'm trying to decline all responsibility for
a regression I caused, but frankly, if just using SIMPLE_DEV_PM_OPS()
breaks runtime PM, then it's the PM model which is broken, not the
i2c-i801 driver.
I will boldly claim that the PCI bus runtime code is simply wrong in
returning -ENOSYS in the absence of runtime PM callbacks, and it should
be changed to return 0 instead. Or whoever receives that -ENOSYS should
not treat it as an error - whatever makes more sense.
Having to add dummy functions in every PCI driver that doesn't need to
do anything special for runtime PM sounds plain stupid. It should be
pretty obvious that a whole lot of drivers are going to use
SIMPLE_DEV_PM_OPS() because it exists and seems to do what they want,
and all of them will be bugged because the PCI core is doing something
silly and unexpected.
So please let's fix it at the PCI subsystem core level. Adding Bjorn
and the linux-pci list to Cc.
Thanks Jean. What you describe does sound broken. I think the PM
guys (cc'd) will have a better idea of how to deal with this.
Did we ever get anywhere with this? It seems like the thread petered
out.
This does seems worrying. I remember, few days earlier you pointed out a driver
i2c-nvidia-gpuc.c. In the code, gpu_i2c_suspend() is an empty-body function. And
comment mentioned that empty stub is necessary for runtime_pm to work.
And this driver also uses UNIVERSAL_DEV_PM_OPS.
This was fixed by c5eb1190074c ("PCI / PM: Allow runtime PM without
callback functions"). So no need for empty runtime PM callbacks anymore.
--
Jarkko
Re: [PATCH v2] HID: i2c-hid: Use block reads when possible to save power
Hi On 7/1/20 6:00 PM, Sultan Alsawaf wrote: On Wed, Jul 01, 2020 at 11:04:01AM +0300, Jarkko Nikula wrote: On 6/29/20 8:43 PM, Sultan Alsawaf wrote: Hmm, for some reason in 5.8 I get the same problem, but 5.7 is fine. Could you try this on 5.7 and see if it works? In the meantime I'll bisect 5.8 to see why it's causing problems for me... I see the same issue on top of v5.7: Try reverting my "i2c: designware: Only check the first byte for SMBus block read length" patch and apply the following change instead: This combination (the diff and this HID patch) works on top of v5.7. I tried also these other combinations: v5.7 - HID patch + this diff -> ok - HID patch -> not ok - HID + acked i2c-dw patch -> acked i2c-dw patch doesn't apply v5.8-rc3 - acked i2c-dw patch -> ok - HID patch -> nok - HID patch + acked i2c-dw patch -> nok - HID patch + this diff -> diff doesn't apply Hopefully gives some glue. I'll be out of office for a few weeks and unfortunately cannot test patches meanwhile. Jarkko
Re: [PATCH v6] i2c: designware: platdrv: Set class based on DMI
On 7/2/20 1:33 PM, Ricardo Ribalda wrote:
Current AMD's zen-based APUs use this core for some of its i2c-buses.
With this patch we re-enable autodetection of hwmon-alike devices, so
lm-sensors will be able to work automatically.
It does not affect the boot-time of embedded devices, as the class is
set based on the DMI information.
DMI is probed only on Qtechnology QT5222 Industrial Camera Platform.
DocLink: https://qtec.com/camera-technology-camera-platforms/
Fixes: 3eddad96c439 ("i2c: designware: reverts "i2c: designware: Add support for AMD I2C
controller"")
Signed-off-by: Ricardo Ribalda
Reviewed-by: Andy Shevchenko
---
v6: Removed extra line, sorry for the invalid v5
drivers/i2c/busses/i2c-designware-platdrv.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
Acked-by: Jarkko Nikula
Re: [PATCH v2] HID: i2c-hid: Use block reads when possible to save power
On 6/29/20 8:43 PM, Sultan Alsawaf wrote: Hmm, for some reason in 5.8 I get the same problem, but 5.7 is fine. Could you try this on 5.7 and see if it works? In the meantime I'll bisect 5.8 to see why it's causing problems for me... I see the same issue on top of v5.7: [9.330514] i2c_hid i2c-ELAN221D:00: Fetching the HID descriptor [9.334761] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=01 00 [9.335716] i2c_hid i2c-ELAN221D:00: HID Descriptor: 1e 00 00 01 31 02 02 00 03 00 43 00 04 00 ff 00 05 00 06 00 f3 04 1d 22 10 56 00 00 00 00 [9.353408] i2c_hid i2c-ELAN221D:00: entering i2c_hid_parse [9.353416] i2c_hid i2c-ELAN221D:00: i2c_hid_hwreset [9.353502] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power [9.353520] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 08 [9.362304] i2c_hid i2c-ELAN221D:00: resetting... [9.370585] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 01 [9.389175] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: waiting... [ 10.416458] i2c_designware i2c_designware.3: controller timed out [ 10.476853] i2c_designware i2c_designware.3: timeout in disabling adapter [ 11.983806] [] i2c_dw_isr [i2c_designware_core] [ 14.544499] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: finished. [ 14.552123] i2c_hid i2c-ELAN221D:00: failed to reset device. [ 14.559263] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power [ 14.565822] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 01 08 [ 14.600256] i2c_designware i2c_designware.3: timeout waiting for bus ready [ 14.608800] i2c_hid i2c-ELAN221D:00: failed to change power setting. [ 15.632103] i2c_hid i2c-ELAN221D:00: i2c_hid_hwreset [ 15.638460] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power [ 15.646422] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 08 ... -- Jarkko
Re: [PATCH v2] i2c: designware: platdrv: Set class based on dmi
On 6/24/20 2:25 PM, Ricardo Ribalda wrote:
Current AMD's zen-based APUs use this core for some of its i2c-buses.
With this patch we re-enable autodetection of hwmon-alike devices, so
lm-sensors will be able to work automatically.
It does not affect the boot-time of embedded devices, as the class is
set based on the dmi information.
Signed-off-by: Ricardo Ribalda
---
v2:
Changes by Andy Shevchenko :
- CodeStyle
Changes by kernel test robot
- Include dmi header to fix build error on arc
- check if dmi_get_system_info returned NULL
drivers/i2c/busses/i2c-designware-platdrv.c | 16 +++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
b/drivers/i2c/busses/i2c-designware-platdrv.c
index c2efc252..5892fdba9c25 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -173,6 +174,19 @@ static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev)
pm_runtime_put_noidle(dev->dev);
}
+static bool dw_i2c_hwmon_bus(void)
+{
+ const char *product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
+
+ if (!product_name)
+ return false;
+
+ if (strstr(product_name, "QT5222"))
+ return true;
+
+ return false;
+}
+
I'm not too familiar with the DMI stuff but is the more usual way to
have struct dmi_system_id table and match it with dmi_check_system()?
Perhaps scales better than individual dmi_get_system_info() and string
comparison calls.
Andy and I were pondering offline is there any info in ACPI table that
tells which bus have these sensors or can it be hardcoded with the DMI
match so that there is no need probe all buses for these sensors on that
product. But that can be another optimization patch I guess.
--
Jarkko
Re: [PATCH v2] i2c: designware: platdrv: Set class based on dmi
On 6/24/20 4:22 PM, Andy Shevchenko wrote:
On Wed, Jun 24, 2020 at 01:25:30PM +0200, Ricardo Ribalda wrote:
Current AMD's zen-based APUs use this core for some of its i2c-buses.
With this patch we re-enable autodetection of hwmon-alike devices, so
lm-sensors will be able to work automatically.
It does not affect the boot-time of embedded devices, as the class is
set based on the dmi information.
I think it misses Fixes tag. And...
I don't think we have regression here. Commit 70fba8302ade ("i2c:
i2c-designware-platdrv: Drop class based scanning to improve bootup
time") was done before any of those AMD ACPI IDs were added.
--
Jarkko
Re: [PATCH] i2c: designware: Fix functionality in !CONFIG_ACPI case
On 6/23/20 10:59 AM, Jarkko Nikula wrote:
Hi
On 6/23/20 5:51 AM, John Stultz wrote:
On the HiKey board, where CONFIG_ACPI is not set, we started
to see a graphics regression where the adv7511 HDMI bridge driver
wasn't probing. This was due to the i2c bus failing to start up.
I bisected the problem down to commit f9288fcc5c615 ("i2c:
designware: Move ACPI parts into common module") and after
looking at it a bit, I realized that change moved some
initialization into i2c_dw_acpi_adjust_bus_speed(). However,
i2c_dw_acpi_adjust_bus_speed() is only functional if CONFIG_ACPI
is set.
This patch pulls i2c_dw_acpi_adjust_bus_speed() out of the
ifdef CONFIG_ACPI conditional, and gets the board working again.
Andy: what you think should the i2c_dw_acpi_adjust_bus_speed() fixed to
return adjusted speed or zero if not found (also for !CONFIG_ACPI) and
move above lines back to probe? It looks more clear to me that way and
should fix the regression I think.
Ok, I sent a patch what I was thinking. Care to test John does it fix
the regression you are seeing?
--
Jarkko
Re: [PATCH] i2c: designware: Fix functionality in !CONFIG_ACPI case
Hi
On 6/23/20 5:51 AM, John Stultz wrote:
On the HiKey board, where CONFIG_ACPI is not set, we started
to see a graphics regression where the adv7511 HDMI bridge driver
wasn't probing. This was due to the i2c bus failing to start up.
I bisected the problem down to commit f9288fcc5c615 ("i2c:
designware: Move ACPI parts into common module") and after
looking at it a bit, I realized that change moved some
initialization into i2c_dw_acpi_adjust_bus_speed(). However,
i2c_dw_acpi_adjust_bus_speed() is only functional if CONFIG_ACPI
is set.
This patch pulls i2c_dw_acpi_adjust_bus_speed() out of the
ifdef CONFIG_ACPI conditional, and gets the board working again.
Cc: Andy Shevchenko
Cc: Jarkko Nikula
Cc: Wolfram Sang
Cc: linux-...@vger.kernel.org
Fixes: f9288fcc5c615 ("i2c: designware: Move ACPI parts into common module")
Signed-off-by: John Stultz
---
drivers/i2c/busses/i2c-designware-common.c | 4 ++--
drivers/i2c/busses/i2c-designware-core.h | 3 +--
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-common.c
b/drivers/i2c/busses/i2c-designware-common.c
index e3a8640db7da..33de185e15f2 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -286,6 +286,8 @@ int i2c_dw_acpi_configure(struct device *device)
}
EXPORT_SYMBOL_GPL(i2c_dw_acpi_configure);
+#endif /* CONFIG_ACPI */
+
I think the regression is in these lines below that were moved from
probe to i2c_dw_acpi_adjust_bus_speed() and cause that neither
"cloock-frequency" device property or I2C_MAX_FAST_MODE_FREQ affect the
bus speed when CONFIG_ACPI is not set.
+ /*
+* Find bus speed from the "clock-frequency" device property, ACPI
+* or by using fast mode if neither is set.
+*/
+ if (acpi_speed && t->bus_freq_hz)
+ t->bus_freq_hz = min(t->bus_freq_hz, acpi_speed);
+ else if (acpi_speed || t->bus_freq_hz)
+ t->bus_freq_hz = max(t->bus_freq_hz, acpi_speed);
+ else
+ t->bus_freq_hz = I2C_MAX_FAST_MODE_FREQ;
Andy: what you think should the i2c_dw_acpi_adjust_bus_speed() fixed to
return adjusted speed or zero if not found (also for !CONFIG_ACPI) and
move above lines back to probe? It looks more clear to me that way and
should fix the regression I think.
--
Jarkko
Re: [PATCH v2] HID: i2c-hid: Use block reads when possible to save power
On 6/16/20 6:49 PM, Sultan Alsawaf wrote:
From: Sultan Alsawaf
We have no way of knowing how large an incoming payload is going to be,
so the only strategy available up until now has been to always retrieve
the maximum possible report length over i2c, which can be quite
inefficient. For devices that send reports in block read format, the i2c
controller driver can read the payload length on the fly and terminate
the i2c transaction early, resulting in considerable power savings.
On a Dell Precision 15 5540 with an i9-9880H, resting my finger on the
touchpad causes psys power readings to go up by about 4W and hover there
until I remove my finger. With this patch, my psys readings go from 4.7W
down to 3.1W, yielding about 1.6W in savings. This is because my
touchpad's max report length is 60 bytes, but all of the regular reports
it sends for touch events are only 32 bytes, so the i2c transfer is
roughly halved for the common case.
Signed-off-by: Sultan Alsawaf
---
Jarkko, could you try this?
drivers/hid/i2c-hid/i2c-hid-core.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c
b/drivers/hid/i2c-hid/i2c-hid-core.c
index 294c84e136d7..739dccfc57e1 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -472,11 +472,14 @@ static void i2c_hid_get_input(struct i2c_hid *ihid)
int ret;
u32 ret_size;
int size = le16_to_cpu(ihid->hdesc.wMaxInputLength);
+ u16 flags;
if (size > ihid->bufsize)
size = ihid->bufsize;
- ret = i2c_master_recv(ihid->client, ihid->inbuf, size);
+ /* Try to do a block read if the size fits in one byte */
+ flags = size > 255 ? I2C_M_RD : I2C_M_RD | I2C_M_RECV_LEN;
+ ret = i2c_transfer_buffer_flags(ihid->client, ihid->inbuf, size, flags);
if (ret != size) {
if (ret < 0)
return;
This still causes a regression for me.
[9.457656] i2c_hid i2c-ELAN221D:00: Fetching the HID descriptor
[9.457663] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=01 00
[9.458591] i2c_hid i2c-ELAN221D:00: HID Descriptor: 1e 00 00 01 31
02 02 00 03 00 43 00 04 00 ff 00 05 00 06 00 f3 04 1d 22 10 56 00 00 00 00
[9.459519] i2c_hid i2c-ELAN221D:00: entering i2c_hid_parse
[9.459526] i2c_hid i2c-ELAN221D:00: i2c_hid_hwreset
[9.459576] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[9.459591] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 08
[9.464070] i2c_hid i2c-ELAN221D:00: resetting...
[9.464078] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 01
[9.464346] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: waiting...
[ 10.497169] i2c_designware i2c_designware.3: controller timed out
[ 10.533940] i2c_designware i2c_designware.3: timeout in disabling adapter
[ 14.528677] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: finished.
[ 14.528695] i2c_hid i2c-ELAN221D:00: failed to reset device.
[ 14.536125] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[ 14.536141] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 01 08
[ 14.556335] i2c_designware i2c_designware.3: timeout waiting for bus
ready
[ 14.565086] i2c_hid i2c-ELAN221D:00: failed to change power setting.
[ 15.584374] i2c_hid i2c-ELAN221D:00: i2c_hid_hwreset
[ 15.584395] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[ 15.584410] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 08
[ 15.605683] i2c_designware i2c_designware.3: timeout waiting for bus
ready
[ 15.614304] i2c_hid i2c-ELAN221D:00: failed to change power setting.
...
--
Jarkko
Re: [PATCH v2] i2c: designware: Only check the first byte for SMBus block read length
On 6/16/20 6:43 PM, Sultan Alsawaf wrote:
From: Sultan Alsawaf
SMBus block reads can be broken because the read function will just skip
over bytes it doesn't like until reaching a byte that conforms to the
length restrictions for block reads. This is problematic when it isn't
known if the incoming payload is indeed a conforming block read.
According to the SMBus specification, block reads will only send the
payload length in the first byte, so we can fix this by only considering
the first byte in a sequence for block read length purposes.
Fixes: c3ae106050b9 ("i2c: designware: Implement support for SMBus block read and
write")
Signed-off-by: Sultan Alsawaf
---
drivers/i2c/busses/i2c-designware-master.c | 16 +++-
1 file changed, 7 insertions(+), 9 deletions(-)
Acked-by: Jarkko Nikula
Re: [PATCH 2/2] HID: i2c-hid: Use block reads when possible to save power
On 6/15/20 12:02 AM, Sultan Alsawaf wrote: From: Sultan Alsawaf We have no way of knowing how large an incoming payload is going to be, so the only strategy available up until now has been to always retrieve the maximum possible report length over i2c, which can be quite inefficient. For devices that send reports in block read format, the i2c controller driver can read the payload length on the fly and terminate the i2c transaction early, resulting in considerable power savings. On a Dell Precision 15 5540 with an i9-9880H, resting my finger on the touchpad causes psys power readings to go up by about 4W and hover there until I remove my finger. With this patch, my psys readings go from 4.7W down to 3.1W, yielding about 1.6W in savings. This is because my touchpad's max report length is 60 bytes, but all of the regular reports it sends for touch events are only 32 bytes, so the i2c transfer is roughly halved for the common case. Signed-off-by: Sultan Alsawaf --- drivers/hid/i2c-hid/i2c-hid-core.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 294c84e136d7..4b507de48d70 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -476,7 +476,8 @@ static void i2c_hid_get_input(struct i2c_hid *ihid) if (size > ihid->bufsize) size = ihid->bufsize; - ret = i2c_master_recv(ihid->client, ihid->inbuf, size); + ret = i2c_transfer_buffer_flags(ihid->client, ihid->inbuf, size, + I2C_M_RD | I2C_M_RECV_LEN); This causes i2c-hid compatible touchscreen to stop working for me. Ok (with patch 1/2) [9.346134] i2c_hid i2c-ELAN221D:00: Fetching the HID descriptor [9.346141] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=01 00 [9.362082] i2c_hid i2c-ELAN221D:00: HID Descriptor: 1e 00 00 01 31 02 02 00 03 00 43 00 04 00 ff 00 05 00 06 00 f3 04 1d 22 10 56 00 00 00 00 [9.385897] i2c_hid i2c-ELAN221D:00: entering i2c_hid_parse [9.386547] i2c_hid i2c-ELAN221D:00: i2c_hid_hwreset [9.386598] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power [9.386616] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 08 [9.391595] i2c_hid i2c-ELAN221D:00: resetting... [9.408864] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 01 [9.410162] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: waiting... [9.418223] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: finished. [9.418231] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power [9.418236] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 08 [9.418531] i2c_hid i2c-ELAN221D:00: asking HID report descriptor [9.418537] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=02 00 [9.440093] i2c_hid i2c-ELAN221D:00: Report Descriptor: 05 0d 09 04 a1 01 85 01 09 22 a1 02 09 42 15 00 25 01 75 01 95 01 81 02 75 01 81 03 75 06 09 51 25 3f 81 02 26 ff 00 75 08 09 48 81 02 09 49 81 02 95 01 05 01 a4 26 c0 0c 75 10 55 0f 65 11 09 Not ok (with patches 1-2/2) [9.428690] i2c_hid i2c-ELAN221D:00: Fetching the HID descriptor [9.428698] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=01 00 [9.430017] i2c_hid i2c-ELAN221D:00: HID Descriptor: 1e 00 00 01 31 02 02 00 03 00 43 00 04 00 ff 00 05 00 06 00 f3 04 1d 22 10 56 00 00 00 00 [9.430836] i2c_hid i2c-ELAN221D:00: entering i2c_hid_parse [9.431205] i2c_hid i2c-ELAN221D:00: i2c_hid_hwreset [9.431294] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power [9.431314] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 08 [9.435937] i2c_hid i2c-ELAN221D:00: resetting... [9.435944] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 01 [9.436150] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: waiting... [ 10.461304] i2c_designware i2c_designware.3: controller timed out [ 10.498312] i2c_designware i2c_designware.3: timeout in disabling adapter [ 14.525115] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: finished. [ 14.525130] i2c_hid i2c-ELAN221D:00: failed to reset device. [ 14.532507] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power [ 14.532520] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 01 08 [ 14.553027] i2c_designware i2c_designware.3: timeout waiting for bus ready ... I don't know what causes the breakage but according to HID Over I2C Protocol Specification the descriptor length is 16 bits. Maybe the code misses the last byte and/or the data is off by one byte by taking the 2nd length byte as 1st data byte? -- Jarkko
Re: [PATCH 1/2] i2c: designware: Only check the first byte for SMBus block read length
On 6/15/20 12:02 AM, Sultan Alsawaf wrote:
From: Sultan Alsawaf
SMBus block reads can be broken because the read function will just skip
over bytes it doesn't like until reaching a byte that conforms to the
length restrictions for block reads. This is problematic when it isn't
known if the incoming payload is indeed a conforming block read.
According to the SMBus specification, block reads will only send the
payload length in the first byte, so we can fix this by only considering
the first byte in a sequence for block read length purposes.
Fixes: c3ae106050b9 ("i2c: designware: Implement support for SMBus block read and
write")
Signed-off-by: Sultan Alsawaf
---
drivers/i2c/busses/i2c-designware-master.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-master.c
b/drivers/i2c/busses/i2c-designware-master.c
index d6425ad6e6a3..16d38b8fc19a 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -398,7 +398,6 @@ i2c_dw_recv_len(struct dw_i2c_dev *dev, u8 len)
len += (flags & I2C_CLIENT_PEC) ? 2 : 1;
dev->tx_buf_len = len - min_t(u8, len, dev->rx_outstanding);
msgs[dev->msg_read_idx].len = len;
- msgs[dev->msg_read_idx].flags &= ~I2C_M_RECV_LEN;
return len;
}
Please update the comment about masking the flag a few lines above this
change.
@@ -430,10 +429,11 @@ i2c_dw_read(struct dw_i2c_dev *dev)
u32 flags = msgs[dev->msg_read_idx].flags;
regmap_read(dev->map, DW_IC_DATA_CMD, );
- /* Ensure length byte is a valid value */
- if (flags & I2C_M_RECV_LEN &&
- tmp <= I2C_SMBUS_BLOCK_MAX && tmp > 0) {
- len = i2c_dw_recv_len(dev, tmp);
+ if (flags & I2C_M_RECV_LEN) {
+ /* Ensure length byte is a valid value */
+ if (tmp <= I2C_SMBUS_BLOCK_MAX && tmp > 0)
+ len = i2c_dw_recv_len(dev, tmp);
+ msgs[dev->msg_read_idx].flags &=
~I2C_M_RECV_LEN;
}
*buf++ = tmp;
dev->rx_outstanding--;
With above comment change this looks good to me.
--
Jarkko
Re: [PATCH v6 07/11] i2c: designware: Discard Cherry Trail model flag
On 5/28/20 1:06 PM, Andy Shevchenko wrote:
On Thu, May 28, 2020 at 12:33:17PM +0300, Serge Semin wrote:
A PM workaround activated by the flag MODEL_CHERRYTRAIL has been removed
since commit 9cbeeca05049 ("i2c: designware: Remove Cherry Trail PMIC I2C
bus pm_disabled workaround"), but the flag most likely by mistake has been
left in the Dw I2C drivers. Let's remove it. Since MODEL_MSCC_OCELOT is
the only model-flag left, redefine it to be 0x100 so setting a very first
bit in the MODEL_MASK bits range.
Reviewed-by: Andy Shevchenko
Conditionally, in case Wolfram and Jarkko are fine with shuffling model
defines, which I consider an unneeded churn.
Signed-off-by: Serge Semin
Acked-by: Jarkko Nikula
I completely agree with Andy and more over, I didn't ack this version.
This was the version I acked:
https://www.spinics.net/lists/linux-i2c/msg45492.html
So in general please drop the received tags in case you decide to modify
patch since reviewers may not agree anymore with it.
That said, I'm fine with that minor code and commit log change. And
don't want to have yet another patchset version because it. Four
patchset versions during 2 days is a bit too much... Usual
recommendation is to wait for 1 week before posting a new version
especially if patchset is under active discussion.
Acked-by: Jarkko Nikula
Re: [PATCH v2 08/12] i2c: designware: Introduce platform drivers glue layer interface
Hi On 5/21/20 5:37 AM, Serge Semin wrote: On Wed, May 20, 2020 at 03:46:11PM +0300, Jarkko Nikula wrote: Hi On 5/10/20 12:50 PM, Serge Semin wrote: Seeing the DW I2C platform driver is getting overcomplicated with a lot of vendor-specific configs let's introduce a glue-layer interface so new platforms which equipped with Synopsys Designware APB I2C IP-core would be able to handle their peculiarities in the dedicated objects. Comment to this patch and patches 9/12 and 12/12: Currently i2c-designware-platdrv.c is about 500 lines of code so I don't think it's too overcomplicated. But I feel we have already too many Kconfig options and source modules for i2c-designware and obviously would like to push back a little from adding more. I don't think i2c-designware-platdrv.c becomes yet too complicated if Baikal related code is added there, perhaps under #ifdef CONFIG_OF like MSCC Ocelot code is currently. Well, it's up to you to decide, what solution is more suitable for you to maintain. My idea of detaching the MSCC and Baikal-T1 code to the dedicated source files was to eventually move the whole i2c-designware-* set of files into a dedicated directory drivers/i2c/buses/dw as it's done for some others Synopsys DesignWare controllers: drivers/pci/controller/dwc/, drivers/usb/dwc2, drivers/usb/dwc3, drivers/net/ethernet/synopsys/ . If you think, that it's too early for Dw I2C code to live in a dedicated directory, fine with me. I can merge the MSCC and Baikal-T1 code back into the i2c-designware-platdrv.c . So what's your final word in this matter? I think sub directory decision under each subsystem is more subsystem rather than vendor/driver specific. Good point anyway. For this patchset I'd like more if changes are done to i2c-designware-platdrv.c since it's not too complicated yet :-) If it starts to look too messy in the future then it's time split I think. Jarkko
Re: [PATCH v2 07/12] i2c: designware: Move Baytrail sem config to the platform if-clause
On 5/21/20 5:22 AM, Serge Semin wrote: On Wed, May 20, 2020 at 03:16:14PM +0300, Jarkko Nikula wrote: On 5/10/20 12:50 PM, Serge Semin wrote: Currently Intel Baytrail I2C semaphore is a feature of the DW APB I2C platform driver. It's a bit confusing to see it's config in the menu at some separated place with no reference to the platform code. Lets move the config definition under the if-I2C_DESIGNWARE_PLATFORM clause. By doing so the config menu will display the feature right below the DW I2C platform driver item and will indent it to the right so signifying its belonging. Signed-off-by: Serge Semin Cc: Alexey Malahov Cc: Thomas Bogendoerfer Cc: Paul Burton Cc: Ralf Baechle Cc: Andy Shevchenko Cc: Mika Westerberg Cc: Wolfram Sang Cc: Rob Herring Cc: Frank Rowand Cc: linux-m...@vger.kernel.org Cc: devicet...@vger.kernel.org --- drivers/i2c/busses/Kconfig | 30 +- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 368aa64e9266..ed6927c4c540 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -530,8 +530,8 @@ config I2C_DESIGNWARE_CORE config I2C_DESIGNWARE_PLATFORM tristate "Synopsys DesignWare Platform" - select I2C_DESIGNWARE_CORE depends on (ACPI && COMMON_CLK) || !ACPI + select I2C_DESIGNWARE_CORE help If you say yes to this option, support will be included for the Synopsys DesignWare I2C adapter. @@ -539,6 +539,22 @@ config I2C_DESIGNWARE_PLATFORM This driver can also be built as a module. If so, the module will be called i2c-designware-platform. +if I2C_DESIGNWARE_PLATFORM + +config I2C_DESIGNWARE_BAYTRAIL + bool "Intel Baytrail I2C semaphore support" + depends on ACPI + depends on (I2C_DESIGNWARE_PLATFORM=m && IOSF_MBI) || \ + (I2C_DESIGNWARE_PLATFORM=y && IOSF_MBI=y) + help + This driver enables managed host access to the PMIC I2C bus on select + Intel BayTrail platforms using the X-Powers AXP288 PMIC. It allows + the host to request uninterrupted access to the PMIC's I2C bus from + the platform firmware controlling it. You should say Y if running on + a BayTrail system using the AXP288. + +endif # I2C_DESIGNWARE_PLATFORM + Is the added "if I2C_DESIGNWARE_PLATFORM" needed here? Should the "depends on" be enough? The idea was to add if-endif clause here for features possibly added sometime in future. But using normal "depends on I2C_DESIGNWARE_PLATFORM" shall make the config depicted as an indented sub-config as well. Would you like me to remove the if-clause and use the depends on operator instead? Yes, please remove it from this patch. Keeps this patch simpler and if some future feature needs it then that patch(set) is the right place to add it. Jarkko
Re: [PATCH v2 08/12] i2c: designware: Introduce platform drivers glue layer interface
Hi On 5/10/20 12:50 PM, Serge Semin wrote: Seeing the DW I2C platform driver is getting overcomplicated with a lot of vendor-specific configs let's introduce a glue-layer interface so new platforms which equipped with Synopsys Designware APB I2C IP-core would be able to handle their peculiarities in the dedicated objects. Comment to this patch and patches 9/12 and 12/12: Currently i2c-designware-platdrv.c is about 500 lines of code so I don't think it's too overcomplicated. But I feel we have already too many Kconfig options and source modules for i2c-designware and obviously would like to push back a little from adding more. I don't think i2c-designware-platdrv.c becomes yet too complicated if Baikal related code is added there, perhaps under #ifdef CONFIG_OF like MSCC Ocelot code is currently. -- Jarkko
Re: [PATCH v2 10/12] i2c: designware: Discard Cherry Trail model flag
On 5/10/20 12:50 PM, Serge Semin wrote:
A PM workaround activated by the flag MODEL_CHERRYTRAIL has been removed
since commit 9cbeeca05049 ("i2c: designware: Remove Cherry Trail PMIC I2C
bus pm_disabled workaround"), but the flag most likely by mistake has been
left in the Dw I2C drivers. Lets remove it.
By doing so we get rid from the last DW APB I2C IP-core model flag, so we
can remove the MODEL_MASK macro too.
Signed-off-by: Serge Semin
Cc: Alexey Malahov
Cc: Thomas Bogendoerfer
Cc: Paul Burton
Cc: Ralf Baechle
Cc: Rob Herring
Cc: Frank Rowand
Cc: linux-m...@vger.kernel.org
Cc: devicet...@vger.kernel.org
---
drivers/i2c/busses/i2c-designware-core.h| 3 ---
drivers/i2c/busses/i2c-designware-pcidrv.c | 1 -
drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
3 files changed, 1 insertion(+), 5 deletions(-)
Acked-by: Jarkko Nikula
Re: [PATCH v2 04/12] i2c: designware: Convert driver to using regmap API
On 5/10/20 12:50 PM, Serge Semin wrote: Seeing the DW I2C driver is using flags-based accessors with two conditional clauses it would be better to replace them with the regmap API IO methods and to initialize the regmap object with read/write callbacks specific to the controller registers map implementation. This will be also handy for the drivers with non-standard registers mapping (like an embedded into the Baikal-T1 System Controller DW I2C block, which glue-driver is a part of this series). As before the driver tries to detect the mapping setup at probe stage and creates a regmap object accordingly, which will be used by the rest of the code to correctly access the controller registers. In two places it was appropriate to convert the hand-written read-modify-write and read-poll-loop design patterns to the corresponding regmap API ready-to-use methods. Note the regmap IO methods return value is checked only at the probe stage. The rest of the code won't do this because basically we have MMIO-based regmap so non of the read/write methods can fail (this also won't be needed for the Baikal-T1-specific I2C controller). Suggested-by: Andy Shevchenko Signed-off-by: Serge Semin Cc: Alexey Malahov Cc: Thomas Bogendoerfer Cc: Paul Burton Cc: Ralf Baechle Cc: Wolfram Sang Cc: Rob Herring Cc: Frank Rowand Cc: devicet...@vger.kernel.org Cc: linux-m...@vger.kernel.org --- drivers/i2c/busses/Kconfig | 1 + drivers/i2c/busses/i2c-designware-common.c | 171 +++-- drivers/i2c/busses/i2c-designware-core.h | 18 +-- drivers/i2c/busses/i2c-designware-master.c | 125 --- drivers/i2c/busses/i2c-designware-slave.c | 77 +- 5 files changed, 239 insertions(+), 153 deletions(-) Looking at patches 4/12-12/12 I think it would be good to move fixes and less invasive patches before this. Like i2c: designware: slave: Set DW I2C core module dependency i2c: designware: Use `-y` to build multi-object modules i2c: designware: Move Baytrail sem config to the platform if-clause That said, you may add: Tested-by: Jarkko Nikula Acked-by: Jarkko Nikula
Re: [PATCH v2 05/12] i2c: designware: Use `-y` to build multi-object modules
On 5/10/20 12:50 PM, Serge Semin wrote:
Since commit 4f8272802739 ("Documentation: update kbuild loadable modules
goals & examples") `-objs` is fitted for building host programs, lets
change DW I2C core, platform and PCI driver kbuild directives to using
`-y`, which more straightforward for device drivers. By doing so we can
discard the ifeq construction in favor to the more natural and less bulky
`-$(CONFIG_X) += x.o`
Signed-off-by: Serge Semin
Cc: Alexey Malahov
Cc: Thomas Bogendoerfer
Cc: Paul Burton
Cc: Ralf Baechle
Cc: Andy Shevchenko
Cc: Mika Westerberg
Cc: Wolfram Sang
Cc: Rob Herring
Cc: Frank Rowand
Cc: linux-m...@vger.kernel.org
Cc: devicet...@vger.kernel.org
---
drivers/i2c/busses/Makefile | 17 -
1 file changed, 8 insertions(+), 9 deletions(-)
Acked-by: Jarkko Nikula
Re: [PATCH v2 07/12] i2c: designware: Move Baytrail sem config to the platform if-clause
On 5/10/20 12:50 PM, Serge Semin wrote: Currently Intel Baytrail I2C semaphore is a feature of the DW APB I2C platform driver. It's a bit confusing to see it's config in the menu at some separated place with no reference to the platform code. Lets move the config definition under the if-I2C_DESIGNWARE_PLATFORM clause. By doing so the config menu will display the feature right below the DW I2C platform driver item and will indent it to the right so signifying its belonging. Signed-off-by: Serge Semin Cc: Alexey Malahov Cc: Thomas Bogendoerfer Cc: Paul Burton Cc: Ralf Baechle Cc: Andy Shevchenko Cc: Mika Westerberg Cc: Wolfram Sang Cc: Rob Herring Cc: Frank Rowand Cc: linux-m...@vger.kernel.org Cc: devicet...@vger.kernel.org --- drivers/i2c/busses/Kconfig | 30 +- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 368aa64e9266..ed6927c4c540 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -530,8 +530,8 @@ config I2C_DESIGNWARE_CORE config I2C_DESIGNWARE_PLATFORM tristate "Synopsys DesignWare Platform" - select I2C_DESIGNWARE_CORE depends on (ACPI && COMMON_CLK) || !ACPI + select I2C_DESIGNWARE_CORE help If you say yes to this option, support will be included for the Synopsys DesignWare I2C adapter. @@ -539,6 +539,22 @@ config I2C_DESIGNWARE_PLATFORM This driver can also be built as a module. If so, the module will be called i2c-designware-platform. +if I2C_DESIGNWARE_PLATFORM + +config I2C_DESIGNWARE_BAYTRAIL + bool "Intel Baytrail I2C semaphore support" + depends on ACPI + depends on (I2C_DESIGNWARE_PLATFORM=m && IOSF_MBI) || \ + (I2C_DESIGNWARE_PLATFORM=y && IOSF_MBI=y) + help + This driver enables managed host access to the PMIC I2C bus on select + Intel BayTrail platforms using the X-Powers AXP288 PMIC. It allows + the host to request uninterrupted access to the PMIC's I2C bus from + the platform firmware controlling it. You should say Y if running on + a BayTrail system using the AXP288. + +endif # I2C_DESIGNWARE_PLATFORM + Is the added "if I2C_DESIGNWARE_PLATFORM" needed here? Should the "depends on" be enough? Jarkko
Re: [PATCH v2 06/12] i2c: designware: slave: Set DW I2C core module dependency
On 5/10/20 12:50 PM, Serge Semin wrote: DW APB I2C slave code in fact depends on the DW I2C driver core, but not on the platform code. Yes, the I2C slave interface is currently supported by the platform version of the IP core, but it doesn't make it dependent on it. So make sure the DW APB I2C slave config is only available if the I2C_DESIGNWARE_CORE config is enabled. Signed-off-by: Serge Semin Cc: Alexey Malahov Cc: Thomas Bogendoerfer Cc: Paul Burton Cc: Ralf Baechle Cc: Andy Shevchenko Cc: Mika Westerberg Cc: Wolfram Sang Cc: Rob Herring Cc: Frank Rowand Cc: linux-m...@vger.kernel.org Cc: devicet...@vger.kernel.org --- drivers/i2c/busses/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Acked-by: Jarkko Nikula
Re: [PATCH] i2c: fix missing pm_runtime_put_sync in i2c_device_probe
On 4/30/20 7:35 PM, Wolfram Sang wrote:
On Thu, Apr 30, 2020 at 05:43:21PM +0200, Alain Volmat wrote:
In case of the I2C client exposes the flag I2C_CLIENT_HOST_NOTIFY,
pm_runtime_get_sync is called in order to always keep active the
adapter. However later on, pm_runtime_put_sync is never called
within the function in case of an error. This commit add this
error handling.
Fixes: 72bfcee11cf8 ("i2c: Prevent runtime suspend of adapter when Host Notify is
required")
Right, I was blind to see this.
Reviewed-by: Jarkko Nikula
[PATCH] mfd: intel-lpss: Add default I2C device properties for Gemini Lake
It turned out Intel Gemini Lake doesn't use the same I2C timing
parameters as Broxton.
I got confirmation from the Windows team that Gemini Lake systems should
use updated timing parameters that differ from those used in Broxton
based systems.
Fixes: f80e78aa11ad ("mfd: intel-lpss: Add Intel Gemini Lake PCI IDs")
Tested-by: Chris Chiu
Signed-off-by: Jarkko Nikula
---
This is not immediate stable material since there is no regression
related to this. Those machines that need updated parameters have
obviously never worked and I don't want this to cause regression either
so better to let this get some test coverage first.
---
drivers/mfd/intel-lpss-pci.c | 28
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/drivers/mfd/intel-lpss-pci.c b/drivers/mfd/intel-lpss-pci.c
index ade6e1ce5a98..269cb851a596 100644
--- a/drivers/mfd/intel-lpss-pci.c
+++ b/drivers/mfd/intel-lpss-pci.c
@@ -120,6 +120,18 @@ static const struct intel_lpss_platform_info apl_i2c_info
= {
.properties = apl_i2c_properties,
};
+static struct property_entry glk_i2c_properties[] = {
+ PROPERTY_ENTRY_U32("i2c-sda-hold-time-ns", 313),
+ PROPERTY_ENTRY_U32("i2c-sda-falling-time-ns", 171),
+ PROPERTY_ENTRY_U32("i2c-scl-falling-time-ns", 290),
+ { },
+};
+
+static const struct intel_lpss_platform_info glk_i2c_info = {
+ .clk_rate = 13300,
+ .properties = glk_i2c_properties,
+};
+
static const struct intel_lpss_platform_info cnl_i2c_info = {
.clk_rate = 21600,
.properties = spt_i2c_properties,
@@ -172,14 +184,14 @@ static const struct pci_device_id intel_lpss_pci_ids[] = {
{ PCI_VDEVICE(INTEL, 0x1ac6), (kernel_ulong_t)_info },
{ PCI_VDEVICE(INTEL, 0x1aee), (kernel_ulong_t)_uart_info },
/* GLK */
- { PCI_VDEVICE(INTEL, 0x31ac), (kernel_ulong_t)_i2c_info },
- { PCI_VDEVICE(INTEL, 0x31ae), (kernel_ulong_t)_i2c_info },
- { PCI_VDEVICE(INTEL, 0x31b0), (kernel_ulong_t)_i2c_info },
- { PCI_VDEVICE(INTEL, 0x31b2), (kernel_ulong_t)_i2c_info },
- { PCI_VDEVICE(INTEL, 0x31b4), (kernel_ulong_t)_i2c_info },
- { PCI_VDEVICE(INTEL, 0x31b6), (kernel_ulong_t)_i2c_info },
- { PCI_VDEVICE(INTEL, 0x31b8), (kernel_ulong_t)_i2c_info },
- { PCI_VDEVICE(INTEL, 0x31ba), (kernel_ulong_t)_i2c_info },
+ { PCI_VDEVICE(INTEL, 0x31ac), (kernel_ulong_t)_i2c_info },
+ { PCI_VDEVICE(INTEL, 0x31ae), (kernel_ulong_t)_i2c_info },
+ { PCI_VDEVICE(INTEL, 0x31b0), (kernel_ulong_t)_i2c_info },
+ { PCI_VDEVICE(INTEL, 0x31b2), (kernel_ulong_t)_i2c_info },
+ { PCI_VDEVICE(INTEL, 0x31b4), (kernel_ulong_t)_i2c_info },
+ { PCI_VDEVICE(INTEL, 0x31b6), (kernel_ulong_t)_i2c_info },
+ { PCI_VDEVICE(INTEL, 0x31b8), (kernel_ulong_t)_i2c_info },
+ { PCI_VDEVICE(INTEL, 0x31ba), (kernel_ulong_t)_i2c_info },
{ PCI_VDEVICE(INTEL, 0x31bc), (kernel_ulong_t)_uart_info },
{ PCI_VDEVICE(INTEL, 0x31be), (kernel_ulong_t)_uart_info },
{ PCI_VDEVICE(INTEL, 0x31c0), (kernel_ulong_t)_uart_info },
--
2.23.0.rc1
Re: Tweak I2C SDA hold time on GemniLake to make touchpad work
On 9/4/19 7:38 AM, Chris Chiu wrote:
On Tue, Sep 3, 2019 at 8:03 PM Jarkko Nikula
wrote:
Hi Chris
On 9/3/19 11:18 AM, Mika Westerberg wrote:
+Jarkko
On Tue, Sep 03, 2019 at 04:10:27PM +0800, Chris Chiu wrote:
Hi,
We're working on the acer Gemnilake laptop TravelMate B118-M for
touchpad not working issue. The touchpad fails to bring up and the
i2c-hid ouput the message as follows
[8.317293] i2c_hid i2c-ELAN0502:00: hid_descr_cmd failed
We tried on latest linux kernel 5.3.0-rc6 and it reports the same.
We then look into I2C signal level measurement to find out why.
The following is the signal output from LA for the SCL/SDA.
https://imgur.com/sKcpvdo
The SCL frequency is ~400kHz from the SCL period, but the SDA
transition is quite weird. Per the I2C spec, the data on the SDA line
must be stable during the high period of the clock. The HIGH or LOW
state of the data line can only change when the clock signal on the
SCL line is LOW. The SDA period span across 2 SCL high, I think
that's the reason why the I2C read the wrong data and fail to initialize.
Thus, we treak the SDA hold time by the following modification.
--- a/drivers/mfd/intel-lpss-pci.c
+++ b/drivers/mfd/intel-lpss-pci.c
@@ -97,7 +97,8 @@ static const struct intel_lpss_platform_info bxt_uart_info = {
};
static struct property_entry bxt_i2c_properties[] = {
- PROPERTY_ENTRY_U32("i2c-sda-hold-time-ns", 42),
+ PROPERTY_ENTRY_U32("i2c-sda-hold-time-ns", 230),
PROPERTY_ENTRY_U32("i2c-sda-falling-time-ns", 171),
PROPERTY_ENTRY_U32("i2c-scl-falling-time-ns", 208),
{ },
The reason why I choose sda hold time is by the Table 10 of
https://www.nxp.com/docs/en/user-guide/UM10204.pdf, the device
must provide a hold time at lease 300ns and and 42 here is relatively
too small. The signal measurement result for the same pin on Windows
is as follows.
https://imgur.com/BtKUIZB
Comparing to the same result running Linux
https://imgur.com/N4fPTYN
After applying the sda hold time tweak patch above, the touchpad can
be correctly initialized and work. The LA signal is shown as down below.
https://imgur.com/B3PmnIp
Could you try does attached patch work for you?
It's from last year for another related issue but there platform was
actually Apollo Lake instead of Gemini Lake but anyway it was found out
that Windows uses different timing parameters than Linux on Gemini Lake.
I didn't take patch forward back then due known Gemini Lake machines
were working with the Broxton I2C timing parameters but now it's time if
attached patch fixes the issue on your machine.
Patch is from top of v5.3-rc7 but should probably apply also to older
kernels.
--
Jarkko
Thanks, Jarkko, the patche works on my acer laptops.
Thanks. I'll send the patch out with Cc'ing you. I took the freedom to
add your Tested-by tag if you don't mind :-)
--
Jarkko
Re: Tweak I2C SDA hold time on GemniLake to make touchpad work
Hi Chris
On 9/3/19 11:18 AM, Mika Westerberg wrote:
+Jarkko
On Tue, Sep 03, 2019 at 04:10:27PM +0800, Chris Chiu wrote:
Hi,
We're working on the acer Gemnilake laptop TravelMate B118-M for
touchpad not working issue. The touchpad fails to bring up and the
i2c-hid ouput the message as follows
[8.317293] i2c_hid i2c-ELAN0502:00: hid_descr_cmd failed
We tried on latest linux kernel 5.3.0-rc6 and it reports the same.
We then look into I2C signal level measurement to find out why.
The following is the signal output from LA for the SCL/SDA.
https://imgur.com/sKcpvdo
The SCL frequency is ~400kHz from the SCL period, but the SDA
transition is quite weird. Per the I2C spec, the data on the SDA line
must be stable during the high period of the clock. The HIGH or LOW
state of the data line can only change when the clock signal on the
SCL line is LOW. The SDA period span across 2 SCL high, I think
that's the reason why the I2C read the wrong data and fail to initialize.
Thus, we treak the SDA hold time by the following modification.
--- a/drivers/mfd/intel-lpss-pci.c
+++ b/drivers/mfd/intel-lpss-pci.c
@@ -97,7 +97,8 @@ static const struct intel_lpss_platform_info bxt_uart_info = {
};
static struct property_entry bxt_i2c_properties[] = {
- PROPERTY_ENTRY_U32("i2c-sda-hold-time-ns", 42),
+ PROPERTY_ENTRY_U32("i2c-sda-hold-time-ns", 230),
PROPERTY_ENTRY_U32("i2c-sda-falling-time-ns", 171),
PROPERTY_ENTRY_U32("i2c-scl-falling-time-ns", 208),
{ },
The reason why I choose sda hold time is by the Table 10 of
https://www.nxp.com/docs/en/user-guide/UM10204.pdf, the device
must provide a hold time at lease 300ns and and 42 here is relatively
too small. The signal measurement result for the same pin on Windows
is as follows.
https://imgur.com/BtKUIZB
Comparing to the same result running Linux
https://imgur.com/N4fPTYN
After applying the sda hold time tweak patch above, the touchpad can
be correctly initialized and work. The LA signal is shown as down below.
https://imgur.com/B3PmnIp
Could you try does attached patch work for you?
It's from last year for another related issue but there platform was
actually Apollo Lake instead of Gemini Lake but anyway it was found out
that Windows uses different timing parameters than Linux on Gemini Lake.
I didn't take patch forward back then due known Gemini Lake machines
were working with the Broxton I2C timing parameters but now it's time if
attached patch fixes the issue on your machine.
Patch is from top of v5.3-rc7 but should probably apply also to older
kernels.
--
Jarkko
>From c1b1aa445838a67617bda5e8de47a0f25bea16df Mon Sep 17 00:00:00 2001
From: Jarkko Nikula
Date: Wed, 14 Nov 2018 13:37:16 +0200
Subject: [PATCH] mfd: intel-lpss: Add default I2C device properties for Gemini
Lake
It turned out Intel Gemini Lake doesn't use the same I2C timing
parameters as Broxton.
I got confirmation from the Windows team that Gemini Lake systems should
use updated timing parameters that differ from those used in Broxton
based systems.
Fixes: f80e78aa11ad ("mfd: intel-lpss: Add Intel Gemini Lake PCI IDs")
Signed-off-by: Jarkko Nikula
---
This is not immediate stable material since there is no regression
related to this. Those machines that need updated parameters have
obviously never worked and I don't want this to cause regression either
so better to let this get some test coverage first.
---
drivers/mfd/intel-lpss-pci.c | 28
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/drivers/mfd/intel-lpss-pci.c b/drivers/mfd/intel-lpss-pci.c
index ade6e1ce5a98..269cb851a596 100644
--- a/drivers/mfd/intel-lpss-pci.c
+++ b/drivers/mfd/intel-lpss-pci.c
@@ -120,6 +120,18 @@ static const struct intel_lpss_platform_info apl_i2c_info = {
.properties = apl_i2c_properties,
};
+static struct property_entry glk_i2c_properties[] = {
+ PROPERTY_ENTRY_U32("i2c-sda-hold-time-ns", 313),
+ PROPERTY_ENTRY_U32("i2c-sda-falling-time-ns", 171),
+ PROPERTY_ENTRY_U32("i2c-scl-falling-time-ns", 290),
+ { },
+};
+
+static const struct intel_lpss_platform_info glk_i2c_info = {
+ .clk_rate = 13300,
+ .properties = glk_i2c_properties,
+};
+
static const struct intel_lpss_platform_info cnl_i2c_info = {
.clk_rate = 21600,
.properties = spt_i2c_properties,
@@ -172,14 +184,14 @@ static const struct pci_device_id intel_lpss_pci_ids[] = {
{ PCI_VDEVICE(INTEL, 0x1ac6), (kernel_ulong_t)_info },
{ PCI_VDEVICE(INTEL, 0x1aee), (kernel_ulong_t)_uart_info },
/* GLK */
- { PCI_VDEVICE(INTEL, 0x31ac), (kernel_ulong_t)_i2c_info },
- { PCI_VDEVICE(INTEL, 0x31ae), (kernel_ulong_t)_i2c_info },
- { PCI_VDEVICE(INTEL, 0x31b0), (kernel_ulong_t)_i2c_info },
- { PCI_VDEVICE(INTEL, 0x31b2), (kernel_ulong_t)_i2c_info },
- { PCI_VDEVICE(INTEL, 0x31b4), (kernel_ulong_t)_i2c_info },
- { PCI_VDEVICE(INTEL, 0x31b6), (ke
Re: [PATCH] i2c: designware: Fix unused variable warning in i2c_dw_init_recovery_info
Hi
On 8/6/19 10:50 AM, Gustavo A. R. Silva wrote:
Fix the following warning:
drivers/i2c/busses/i2c-designware-master.c: In function
‘i2c_dw_init_recovery_info’:
drivers/i2c/busses/i2c-designware-master.c:658:6: warning: unused variable ‘r’
[-Wunused-variable]
int r;
^
Fixes: 33eb09a02e8d ("i2c: designware: make use of devm_gpiod_get_optional")
Signed-off-by: Gustavo A. R. Silva
---
This was fixed yesterday, not applied yet though:
https://www.spinics.net/lists/linux-i2c/msg41651.html
--
Jarkko
Re: [PATCH] ASoC: ti: Mark expected switch fall-throughs
On Mon, Jul 29, 2019 at 05:15:34PM -0500, Gustavo A. R. Silva wrote: > Mark switch cases where we are expecting to fall through. > > This patch fixes the following warning (Building: arm): > > sound/soc/ti/n810.c: In function ‘n810_ext_control’: > sound/soc/ti/n810.c:48:10: warning: this statement may fall through > [-Wimplicit-fallthrough=] >line1l = 1; >~~~^~~ > sound/soc/ti/n810.c:49:2: note: here > case N810_JACK_HP: > ^~~~ > > sound/soc/ti/rx51.c: In function ‘rx51_ext_control’: > sound/soc/ti/rx51.c:57:6: warning: this statement may fall through > [-Wimplicit-fallthrough=] >hs = 1; >~~~^~~ > sound/soc/ti/rx51.c:58:2: note: here > case RX51_JACK_HP: > ^~~~ > > Signed-off-by: Gustavo A. R. Silva > --- > sound/soc/ti/n810.c | 1 + > sound/soc/ti/rx51.c | 1 + > 2 files changed, 2 insertions(+) > Acked-by: Jarkko Nikula
Re: [PATCH] i2c: busses: Use dev_get_drvdata where possible
On 7/23/19 9:34 PM, Jean Delvare wrote: On Tue, 23 Jul 2019 19:11:10 +0800, Chuhong Yuan wrote: Instead of using to_pci_dev + pci_get_drvdata, use dev_get_drvdata to make code simpler. Signed-off-by: Chuhong Yuan --- drivers/i2c/busses/i2c-designware-pcidrv.c | 6 ++ drivers/i2c/busses/i2c-i801.c | 3 +-- 2 files changed, 3 insertions(+), 6 deletions(-) (...) Looks good to me, thanks. Reviewed-by: Jean Delvare For the i2c-designware-pcidrv.c Acked-by: Jarkko Nikula
Re: [PATCH v2 08/27] i2c: busses: remove memset after dmam_alloc_coherent
Hi
On 6/28/19 5:47 AM, Fuqian Huang wrote:
In commit af7ddd8a627c
("Merge tag 'dma-mapping-4.21' of
git://git.infradead.org/users/hch/dma-mapping"),
dmam_alloc_coherent has already zeroed the memory.
So memset is not needed.
Signed-off-by: Fuqian Huang
---
drivers/i2c/busses/i2c-ismt.c | 2 --
1 file changed, 2 deletions(-)
It would be better to refer actual commit or commits that implement that
zeroing rather than merge point in commit log if possible.
At quick look commit 518a2f1925c3 ("dma-mapping: zero memory returned
from dma_alloc_*") is the one but I'm not really an expert here to say
is that alone enough.
--
Jarkko
Re: [PATCH] i2c: designware: Add disable runtime pm quirk
On 6/26/19 5:32 AM, AceLan Kao wrote:
Adding I2C_HID_QUIRK_NO_RUNTIME_PM quirk doesn't help on this issue.
Actually, Goodix touchpad already has that PM quirk in the list for other issue.
{ I2C_VENDOR_ID_GOODIX, I2C_DEVICE_ID_GOODIX_01F0,
I2C_HID_QUIRK_NO_RUNTIME_PM },
I also modify the code as you suggested, but no luck.
Yeah, I realized it won't help as the i2c-hid device is anyway powered
on constantly when the device is open by the pm_runtime_get_sync() call
in i2c_hid_open().
It's not Goodix takes time to wakeup, it's designware I2C controller.
Designware doesn't do anything wrong here, it's Goodix set the interrupt timeout
that leads to the issue, so we have to prevent designware from runtime
suspended.
But only on that bus where Goodix is connected and open by user space.
What I mean something like below:
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c
b/drivers/hid/i2c-hid/i2c-hid-core.c
index 90164fed08d3..bbeaa39ddc23 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -795,6 +795,9 @@ static int i2c_hid_open(struct hid_device *hid)
struct i2c_hid *ihid = i2c_get_clientdata(client);
int ret = 0;
+ /* some quirk test here */
+ pm_runtime_get_sync(>adapter->dev);
+
ret = pm_runtime_get_sync(>dev);
if (ret < 0)
return ret;
@@ -812,6 +815,9 @@ static void i2c_hid_close(struct hid_device *hid)
/* Save some power */
pm_runtime_put(>dev);
+
+ /* some quirk test here */
+ pm_runtime_put(>adapter->dev);
}
static int i2c_hid_power(struct hid_device *hid, int lvl)
--
Jarkko
Re: [PATCH] i2c: designware: Add disable runtime pm quirk
On 6/25/19 11:30 AM, AceLan Kao wrote:
Dell machines come with goodix touchpad IC suffer from the double click
issue if the Designware I2C adapter enters runtime suspend.
It's because the goodix re-assert the interrupt if host doesn't read the
data within 100ms and designware takes a longer time to wake up from
runtime suspend. In the case, it got a second interrupt during
resuming, so it thinks it's a double click.
There is no simple way to fix this, it's a firmware issue and goodix
agrees to fix this in their firmware on next release, but this issue
still affects the machines that don't come with an updated firmware. So,
add a quirk to mark those machines and avoid the designware from
entering runtime suspend.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=202683
Signed-off-by: AceLan Kao
---
drivers/i2c/busses/i2c-designware-master.c | 30 --
1 file changed, 28 insertions(+), 2 deletions(-)
I think better place to have this fixed is in
drivers/hid/i2c-hid/i2c-hid-core.c by forcing the adapter device active
when communicating with such touchpad.
In that way only bus where touchpad is connected stays active, not all
and makes sure issue is handled also if that touchpad is ever connected
to any other I2C adapter than Designware.
I did something similar in the commit 72bfcee11cf8 ("i2c: Prevent
runtime suspend of adapter when Host Notify is required"). Not exactly
same issue but similar idea.
By looking at i2c-hid-core.c I saw a few i2c-hid devices have
I2C_HID_QUIRK_NO_RUNTIME_PM. Could you test how does this Goodix behave
if only i2c-hid device runtime PM is prevented not I2C adapter?
A very quick test would be to comment out those lines below:
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c
b/drivers/hid/i2c-hid/i2c-hid-core.c
index 90164fed08d3..bd3e6570c45e 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -1156,8 +1156,8 @@ static int i2c_hid_probe(struct i2c_client *client,
goto err_mem_free;
}
- if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
- pm_runtime_put(>dev);
+// if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
+// pm_runtime_put(>dev);
return 0;
@@ -1183,8 +1183,8 @@ static int i2c_hid_remove(struct i2c_client *client)
struct i2c_hid *ihid = i2c_get_clientdata(client);
struct hid_device *hid;
- if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
- pm_runtime_get_sync(>dev);
+// if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
+// pm_runtime_get_sync(>dev);
pm_runtime_disable(>dev);
pm_runtime_set_suspended(>dev);
pm_runtime_put_noidle(>dev);
--
Jarkko
Re: [PATCH 2/2] eeprom: ee1004: Deal with nack on page selection
On 5/6/19 4:16 PM, Jean Delvare wrote: Some EE1004 implementations will not properly ack page selection commands. They still set the page correctly, so there is no actual error. Deal with this case gracefully by checking the currently selected page after we receive a nack. If the page is set right then we can continue. Signed-off-by: Jean Delvare Tested-by: Jarkko Nikula Cc: Arnd Bergmann Cc: Greg Kroah-Hartman --- drivers/misc/eeprom/ee1004.c | 12 +++- 1 file changed, 11 insertions(+), 1 deletion(-) Does Dreamcat4 deserve reported and tested by tags here? I guess anonymous address is fine with those tags? (I re-tested these two patches on top of v5.1 and they make decode-dimms working on a machine with 2-4 * Crucial DD4 dimms) -- Jarkko
Re: [PATCH v2] mfd: intel-lpss: Add Intel Comet Lake PCI IDs
On 4/30/19 7:56 PM, Andy Shevchenko wrote: Intel Comet Lake has the same LPSS than Intel Cannon Lake. Add the new IDs to the list of supported devices. Signed-off-by: Andy Shevchenko --- - update i2c info drivers/mfd/intel-lpss-pci.c | 13 + 1 file changed, 13 insertions(+) Reviewed-by: Jarkko Nikula
Re: [PATCH v1] mfd: intel-lpss: Add Intel Comet Lake PCI IDs
On 4/16/19 10:55 AM, Andy Shevchenko wrote:
On Tue, Apr 16, 2019 at 6:10 AM Evan Green wrote:
On Tue, Apr 9, 2019 at 11:11 PM Andy Shevchenko
wrote:
Intel Comet Lake has the same LPSS than Intel Cannon Lake.
Add the new IDs to the list of supported devices.
static const struct pci_device_id intel_lpss_pci_ids[] = {
+ /* CML */
+ { PCI_VDEVICE(INTEL, 0x02a8), (kernel_ulong_t)_uart_info },
+ { PCI_VDEVICE(INTEL, 0x02a9), (kernel_ulong_t)_uart_info },
+ { PCI_VDEVICE(INTEL, 0x02aa), (kernel_ulong_t)_info },
+ { PCI_VDEVICE(INTEL, 0x02ab), (kernel_ulong_t)_info },
+ { PCI_VDEVICE(INTEL, 0x02c5), (kernel_ulong_t)_i2c_info },
+ { PCI_VDEVICE(INTEL, 0x02c6), (kernel_ulong_t)_i2c_info },
How come it's not cnl_i2c_info?
This is a good question, that's why Jarkko asked Lee to hold on until
we have confirmation about i2c timings.
I got confirmation I2C uses the same input clock than Cannon Lake and
matches with my measurements.
Andy: please do s/bxt_i2c_info/cnl_i2c_info/ in your patch.
--
Jarkko
Re: [Bug 203297] Synaptics touchpad TM-3127 functionality broken by PCI runtime power management patch on 4.20.2
On 4/29/19 10:45 AM, Benjamin Tissoires wrote:
I would like to ask help from input subsystem experts what kind of SMBus
power state dependency Synaptics RMI4 SMBus devices have since it cease
to work if SMBus controllers idles between transfers and how this is
best to fix?
Hmm, I am not sure there is such an existing architecture you could
use in a simple patch.
rmi-driver.c does indeed create an input device we could use to toggle
on/off the PM state, but those callbacks are not wired to the
transport driver (rmi_smbus.c), so it would required a little bit of
extra work. And then, there are other RMI4 functions (firmware
upgrade) that would not be happy if PM is in suspend while there is no
open input node.
I see.
I got another thought about this. I noticed these input drivers need
SMBus Host Notify, maybe that explain the PM dependency? If that's the
only dependency then we could prevent the controller suspend if there is
a client needing host notify mechanism. IMHO that's less hack than the
patch to rmi_smbus.c.
Keijo: care to test does this idea would fix the issue (without the
previous patch)? I also attached the diff.
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 38af18645133..d54eafad7727 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -327,6 +327,8 @@ static int i2c_device_probe(struct device *dev)
if (client->flags & I2C_CLIENT_HOST_NOTIFY) {
dev_dbg(dev, "Using Host Notify IRQ\n");
+ /* Adapter should not suspend for Host Notify */
+ pm_runtime_get_sync(>adapter->dev);
irq = i2c_smbus_host_notify_to_irq(client);
} else if (dev->of_node) {
irq = of_irq_get_byname(dev->of_node, "irq");
@@ -431,6 +433,8 @@ static int i2c_device_remove(struct device *dev)
device_init_wakeup(>dev, false);
client->irq = client->init_irq;
+ if (client->flags & I2C_CLIENT_HOST_NOTIFY)
+ pm_runtime_put(>adapter->dev);
return status;
}
So I think this "hack" (with Mika's comments addressed) should go in
until someone starts propagating the PM states correctly.
I guess you mean the Rafael's pm_runtime_get_sync() comment?
--
Jarkko
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 38af18645133..d54eafad7727 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -327,6 +327,8 @@ static int i2c_device_probe(struct device *dev)
if (client->flags & I2C_CLIENT_HOST_NOTIFY) {
dev_dbg(dev, "Using Host Notify IRQ\n");
+ /* Adapter should not suspend for Host Notify */
+ pm_runtime_get_sync(>adapter->dev);
irq = i2c_smbus_host_notify_to_irq(client);
} else if (dev->of_node) {
irq = of_irq_get_byname(dev->of_node, "irq");
@@ -431,6 +433,8 @@ static int i2c_device_remove(struct device *dev)
device_init_wakeup(>dev, false);
client->irq = client->init_irq;
+ if (client->flags & I2C_CLIENT_HOST_NOTIFY)
+ pm_runtime_put(>adapter->dev);
return status;
}
Re: [Bug 203297] Synaptics touchpad TM-3127 functionality broken by PCI runtime power management patch on 4.20.2
On 4/22/19 4:08 PM, Bjorn Helgaas wrote:
https://bugzilla.kernel.org/show_bug.cgi?id=203297
Regression, suspected but as yet unconfirmed cause:
c5eb1190074c ("PCI / PM: Allow runtime PM without callback functions")
backported to 4.20 stable as 39e1be324c2f.
With help of Keijo it was confirmed above patch broke the Synaptics
touchpad. Not bisected but touchpad works again by forcing the i2c-i801
SMBus controller always on:
"echo on >/sys/bus/pci/devices/\:00\:1f.3/power/control"
Above patch is a generalized fix that fixed the runtime PM regression on
i2c-i801 and re-allow the controller go to runtime suspend when idle. So
most probably Synaptics touchpad was broken by i2c-i801 runtime PM also
before but got unnoticed. Which is easy since on many platforms SMBus
controller doesn't necessarily have the PCI PM capabilities.
I would like to ask help from input subsystem experts what kind of SMBus
power state dependency Synaptics RMI4 SMBus devices have since it cease
to work if SMBus controllers idles between transfers and how this is
best to fix?
Instead of revert I think we'd need to have some method to force SMBus
controller on whenever the touchpad is active, like when there is a
userspace listening.
I'm not expert in this area so as quick proof of concept I had a
following hack which forces the I2C/SMBus adapter, and eventually the
parent PCI device of it on when the RMI4 SMBus device is probed and let
the SMBus controller to idle when removed.
According to Keijo it fixes the issue but I like to hear input experts
for better place to put these.
diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
index b6ccf39c6a7b..2b11d69be313 100644
--- a/drivers/input/rmi4/rmi_smbus.c
+++ b/drivers/input/rmi4/rmi_smbus.c
@@ -16,6 +16,7 @@
#include
#include
#include
+#include
#include
#include
#include "rmi_driver.h"
@@ -332,6 +333,9 @@ static int rmi_smb_probe(struct i2c_client *client,
dev_info(>dev, "registering SMbus-connected sensor\n");
+ /* Force SMBus adapter on while RMI4 device is connected */
+ pm_runtime_get(>adapter->dev);
+
error = rmi_register_transport_device(_smb->xport);
if (error) {
dev_err(>dev, "failed to register sensor: %d\n", error);
@@ -346,6 +350,7 @@ static int rmi_smb_remove(struct i2c_client *client)
struct rmi_smb_xport *rmi_smb = i2c_get_clientdata(client);
rmi_unregister_transport_device(_smb->xport);
+ pm_runtime_put(>adapter->dev);
return 0;
}
--
Jarkko
Re: [PATCH] spi: pxa2xx: Add support for Intel Comet Lake
On 4/16/19 11:00 AM, Andy Shevchenko wrote: On Tue, Apr 16, 2019 at 6:28 AM Evan Green wrote: Add PCI IDs for SPI on Comet Lake. I didn't check IDs, perhaps Jarkko can do it. From other prospectives looks good. Acked-by: Andy Shevchenko Yes, those IDs and compatibility with Cannon Lake match with my understanding. Reviewed-by: Jarkko Nikula
Re: [PATCH v2] spi: pxa2xx: fix SCR (divisor) calculation
On 4/12/19 10:32 AM, Flavio Suligoi wrote: Calculate the divisor for the SCR (Serial Clock Rate), avoiding that the SSP transmission rate can be greater than the device rate. When the division between the SSP clock and the device rate generates a reminder, we have to increment by one the divisor. In this way the resulting SSP clock will never be greater than the device SPI max frequency. For example, with: - ssp_clk = 50 MHz - dev freq = 15 MHz without this patch the SSP clock will be greater than 15 MHz: - 25 MHz for PXA25x_SSP and CE4100_SSP - 16,56 MHz for the others Instead, with this patch, we have in both case an SSP clock of 12.5MHz, so the max rate of the SPI device clock is respected. Signed-off-by: Flavio Suligoi Reviewed-by: Jarkko Nikula --- v2: - simplify the code using "DIV_ROUND_UP" v1: - first version drivers/spi/spi-pxa2xx.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c index f7068cc..a35fdcb 100644 --- a/drivers/spi/spi-pxa2xx.c +++ b/drivers/spi/spi-pxa2xx.c @@ -884,10 +884,14 @@ static unsigned int ssp_get_clk_div(struct driver_data *drv_data, int rate) rate = min_t(int, ssp_clk, rate); + /* +* Calculate the divisor for the SCR (Serial Clock Rate), avoiding +* that the SSP transmission rate can be greater than the device rate +*/ if (ssp->type == PXA25x_SSP || ssp->type == CE4100_SSP) - return (ssp_clk / (2 * rate) - 1) & 0xff; + return (DIV_ROUND_UP(ssp_clk, 2 * rate) - 1) & 0xff; else - return (ssp_clk / rate - 1) & 0xfff; + return (DIV_ROUND_UP(ssp_clk, rate) - 1) & 0xfff; } Reviewed-by: Jarkko Nikula
Re: [PATCH 1/2] spi: pxa2xx: fix SCR (divisor) calculation
On 4/10/19 3:51 PM, Flavio Suligoi wrote: Calculate the divisor for the SCR (Serial Clock Rate), avoiding that the SSP transmission rate can be greater than the device rate. When the division between the SSP clock and the device rate generates a reminder, we have to increment by one the divisor. In this way the resulting SSP clock will never be greater than the device SPI max frequency. For example, with: - ssp_clk = 50 MHz - dev freq = 15 MHz without this patch the SSP clock will be greater than 15 MHz: - 25 MHz for PXA25x_SSP and CE4100_SSP - 16,56 MHz for the others Instead, with this patch, we have in both case an SSP clock of 12.5MHz, so the max rate of the SPI device clock is respected. Signed-off-by: Flavio Suligoi --- drivers/spi/spi-pxa2xx.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c index f7068cc..c9560a1 100644 --- a/drivers/spi/spi-pxa2xx.c +++ b/drivers/spi/spi-pxa2xx.c @@ -884,10 +884,15 @@ static unsigned int ssp_get_clk_div(struct driver_data *drv_data, int rate) rate = min_t(int, ssp_clk, rate); + /* +* Calculate the divisor for the SCR (Serial Clock Rate), avoiding +* that the SSP transmission rate can be greater than the device rate +*/ if (ssp->type == PXA25x_SSP || ssp->type == CE4100_SSP) - return (ssp_clk / (2 * rate) - 1) & 0xff; + return (ssp_clk / (2 * rate) - 1 + + (ssp_clk % (2 * rate) ? 1 : 0)) & 0xff; else - return (ssp_clk / rate - 1) & 0xfff; + return (ssp_clk / rate - 1 + (ssp_clk % rate ? 1 : 0)) & 0xfff; } I think DIV_ROUND_UP() - 1 would also fix this? I realized we have also another issue here with the low rates. Calculated divider will underflow due masking with 0xff or 0xfff when the rate is low enough. Would you want to fix that by setting the ctlr->min_speed_hz so that spi core can validate the rate? I'm asking since it goes well together with your fix. Maybe one patch setting the min_speed_hz and getting masking off from here and then another fixing the rate calculation. -- Jarkko
Re: [PATCH v2 1/1] spi: pxa2xx: add driver enabling message
On 4/10/19 12:47 PM, Flavio Suligoi wrote:
Add an info message for the PXA2xx device driver start-up,
with the indication of:
- mode (slave device or master controller)
- transfer mode (DMA or GPIO)
Signed-off-by: Flavio Suligoi
---
v1: - first version
v2: - remove warning message "no DMA channels available, using PIO"
- add "slave device"/"master controller" indication message
drivers/spi/spi-pxa2xx.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index f7068cc..b9e04e2 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1696,7 +1696,6 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
if (platform_info->enable_dma) {
status = pxa2xx_spi_dma_setup(drv_data);
if (status) {
- dev_warn(dev, "no DMA channels available, using PIO\n");
platform_info->enable_dma = false;
} else {
controller->can_dma = pxa2xx_spi_can_dma;
@@ -1818,6 +1817,10 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
pm_runtime_set_active(>dev);
pm_runtime_enable(>dev);
+ dev_info(dev, "PXA2xx SPI %s (%s mode)\n",
+ platform_info->is_slave ? "slave device" : "master controller",
+ platform_info->enable_dma ? "DMA" : "PIO");
+
"slave device" is ambiguous. Please use "controller" with both, i.e.
"PXA2xx SPI %s controller (%s mode)\n", ... ? "slave" : "master", ...
With that changed you may add:
Reviewed-by: Jarkko Nikula
Re: [PATCH 1/1] spi: pxa2xx: add driver enabling message
On 4/10/19 11:13 AM, Flavio Suligoi wrote:
[9.506895] pxa2xx-spi pxa2xx-spi.13: no DMA channels available,
using PIO
[9.516770] pxa2xx-spi pxa2xx-spi.13: registered master spi2
[9.518527] pxa2xx-spi pxa2xx-spi.13: PXA2xx SPI master controller
(PIO mode)
I have added this message because, using an x86 machine, the message:
"pxa2xx-spi pxa2xx-spi.13: registered master spi2"
doesn't appear in the kernel messages!
Yeah, it needs CONFIG_SPI_DEBUG.
Actually this info message doesn't necessarily tell will the driver end
up using DMA for transfers. See pxa2xx_spi_can_dma() and
pxa2xx_spi_transfer_one().
How about replacing "no DMA channels available, using PIO" and have
instead single info message telling is the DMA available or does the
driver use PIO only?
Ok, it's a good idea, to avoid to many similar messages.
So I can simply remove the:
"no DMA channels available, using PIO"
and leave only the new:
" pxa2xx-spi pxa2xx-spi.13: PXA2xx SPI master controller (PIO mode)"
Yes, something like that.
Now I remember, please also take into account driver is dual role since
commit ec93cb6f827b ("spi: pxa2xx: Add slave mode support").
--
Jarkko
Re: [PATCH v1] mfd: intel-lpss: Add Intel Comet Lake PCI IDs
On 4/9/19 6:11 PM, Andy Shevchenko wrote:
Intel Comet Lake has the same LPSS than Intel Cannon Lake.
Add the new IDs to the list of supported devices.
Signed-off-by: Andy Shevchenko
---
drivers/mfd/intel-lpss-pci.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/mfd/intel-lpss-pci.c b/drivers/mfd/intel-lpss-pci.c
index a67f67c90ec4..50a907f93da9 100644
--- a/drivers/mfd/intel-lpss-pci.c
+++ b/drivers/mfd/intel-lpss-pci.c
@@ -129,6 +129,19 @@ static const struct intel_lpss_platform_info cnl_i2c_info
= {
};
static const struct pci_device_id intel_lpss_pci_ids[] = {
+ /* CML */
+ { PCI_VDEVICE(INTEL, 0x02a8), (kernel_ulong_t)_uart_info },
+ { PCI_VDEVICE(INTEL, 0x02a9), (kernel_ulong_t)_uart_info },
+ { PCI_VDEVICE(INTEL, 0x02aa), (kernel_ulong_t)_info },
+ { PCI_VDEVICE(INTEL, 0x02ab), (kernel_ulong_t)_info },
+ { PCI_VDEVICE(INTEL, 0x02c5), (kernel_ulong_t)_i2c_info },
+ { PCI_VDEVICE(INTEL, 0x02c6), (kernel_ulong_t)_i2c_info },
+ { PCI_VDEVICE(INTEL, 0x02c7), (kernel_ulong_t)_uart_info },
+ { PCI_VDEVICE(INTEL, 0x02e8), (kernel_ulong_t)_i2c_info },
+ { PCI_VDEVICE(INTEL, 0x02e9), (kernel_ulong_t)_i2c_info },
+ { PCI_VDEVICE(INTEL, 0x02ea), (kernel_ulong_t)_i2c_info },
+ { PCI_VDEVICE(INTEL, 0x02eb), (kernel_ulong_t)_i2c_info },
+ { PCI_VDEVICE(INTEL, 0x02fb), (kernel_ulong_t)_info },
/* BXT A-Step */
{ PCI_VDEVICE(INTEL, 0x0aac), (kernel_ulong_t)_i2c_info },
{ PCI_VDEVICE(INTEL, 0x0aae), (kernel_ulong_t)_i2c_info },
Let's hold a bit. I'd like to double check does the I2C controller use
Skylake or Broxton derived input clocks.
--
Jarkko
Re: [PATCH 1/1] spi: pxa2xx: add driver enabling message
On 4/9/19 5:11 PM, Flavio Suligoi wrote: Hi Jarkko, Hi On 4/8/19 6:22 PM, Flavio Suligoi wrote: Add an info message for the PXA2xx device driver start-up, with the indication of the transfer mode used (DMA or GPIO). This info is useful to individuate the timing when the module starts. Signed-off-by: Flavio Suligoi --- drivers/spi/spi-pxa2xx.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c index f7068cc..d449501 100644 --- a/drivers/spi/spi-pxa2xx.c +++ b/drivers/spi/spi-pxa2xx.c @@ -1826,6 +1826,9 @@ static int pxa2xx_spi_probe(struct platform_device *pdev) goto out_error_clock_enabled; } + dev_info(dev, "PXA2xx SPI master controller (%s mode)\n", +platform_info->enable_dma ? "DMA" : "PIO"); + return status; Would this look better if moved before devm_spi_register_controller() call? Ok, so in case of SPI registering failure, we have two messages, as: pxa2xx-spi 80860F0E:00: PXA2xx SPI master controller (DMA mode) pxa2xx-spi 80860F0E:00: problem registering spi controller Do you think that it is more explicative? I think yes and it should not cause confusion since the error message is the last one. I was thinking the successful registration case when DMA is not available. First there is a warning, followed by a debug message from SPI core (if CONFIG_SPI_DEBUG) and then info message. [9.506895] pxa2xx-spi pxa2xx-spi.13: no DMA channels available, using PIO [9.516770] pxa2xx-spi pxa2xx-spi.13: registered master spi2 [9.518527] pxa2xx-spi pxa2xx-spi.13: PXA2xx SPI master controller (PIO mode) Actually this info message doesn't necessarily tell will the driver end up using DMA for transfers. See pxa2xx_spi_can_dma() and pxa2xx_spi_transfer_one(). How about replacing "no DMA channels available, using PIO" and have instead single info message telling is the DMA available or does the driver use PIO only? -- Jarkko
Re: [PATCH 1/1] spi: pxa2xx: add driver enabling message
Hi On 4/8/19 6:22 PM, Flavio Suligoi wrote: Add an info message for the PXA2xx device driver start-up, with the indication of the transfer mode used (DMA or GPIO). This info is useful to individuate the timing when the module starts. Signed-off-by: Flavio Suligoi --- drivers/spi/spi-pxa2xx.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c index f7068cc..d449501 100644 --- a/drivers/spi/spi-pxa2xx.c +++ b/drivers/spi/spi-pxa2xx.c @@ -1826,6 +1826,9 @@ static int pxa2xx_spi_probe(struct platform_device *pdev) goto out_error_clock_enabled; } + dev_info(dev, "PXA2xx SPI master controller (%s mode)\n", +platform_info->enable_dma ? "DMA" : "PIO"); + return status; Would this look better if moved before devm_spi_register_controller() call? -- Jarkko
Re: [PATCH] ACPI / LPSS: Don't skip late system PM ops for hibernate on BYT/CHT
On 4/3/19 8:43 AM, Kai-Heng Feng wrote:
i2c-designware-platdrv fails to work after the system restored from
hibernation:
[ 272.775692] i2c_designware 80860F41:00: Unknown Synopsys component type:
0x
Commit 48402cee6889 ("ACPI / LPSS: Resume BYT/CHT I2C controllers from
resume_noirq") makes acpi_lpss_{suspend_late,resume_early}() bail early
on BYT/CHT as resume_from_noirq is set. This means dw_i2c_plat_resume()
doesn't gets called by acpi_lpss_resume_early(), and this causes the
issue.
Introduce acpi_lpss_{poweroff_late,restore_early}() to make sure
driver's own poweroff_late and restore_early get called.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=202139
Fixes: 48402cee6889 ("ACPI / LPSS: Resume BYT/CHT I2C controllers from
resume_noirq")
Signed-off-by: Kai-Heng Feng
---
drivers/acpi/acpi_lpss.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
On a BSW/CHT machine I have I see the warnings below at commit
48402cee6889 after resuming from hibernation but I2C bus itself is
working so obviously I tested only S3 when I gave the tested by.
This patch is fixing that so here's my tested by to this patch:
Tested-by: Jarkko Nikula
But let's wait confirmation from Hans since he has machines and test
cases where issues from shared I2C bus have shown up.
[ 261.472892] [ cut here ]
[ 261.473361] 808622C1:03 already disabled
[ 261.473688] WARNING: CPU: 0 PID: 149 at drivers/clk/clk.c:828
clk_core_disable+0xca/0x210
[ 261.473688] Modules linked in: hid_multitouch hid_generic i2c_hid
iTCO_wdt atkbd libps2 intel_rapl coretemp aesni_intel aes_x86_64
crypto_simd glue_helper intel_cstate i915 i2c_i801 intel_gtt
i2c_algo_bit lpc_ich drm_kms_helper syscopyarea sysfillrect sysimgblt
fb_sys_fops drm battery tpm_tis i8042 serio i2c_designware_platform
video i2c_designware_core tpm_tis_core tpm pinctrl_cherryview button
rng_core ac i2c_dev i2c_core loop
[ 261.473688] CPU: 0 PID: 149 Comm: kworker/0:2 Not tainted 4.19.0-rc3+ #8
[ 261.473688] Workqueue: pm pm_runtime_work
[ 261.473688] RIP: 0010:clk_core_disable+0xca/0x210
[ 261.473688] Code: ff ff ff 48 8b 33 48 c7 c7 c3 09 e5 81 e8 98 02 c5
ff 0f 0b 5b 41 5c 41 5d 5d c3 48 8b 33 48 c7 c7 ae 09 e5 81 e8 80 02 c5
ff <0f> 0b e9 60 ff ff ff 65 8b 05 f8 84 c0 7e 89 c0 48 0f a3 05 ae 56
[ 261.473688] RSP: :c95ebc58 EFLAGS: 00010082
[ 261.473688] RAX: RBX: 880179d02100 RCX:
0006
[ 261.473688] RDX: 0007 RSI: RDI:
88017ba156d0
[ 261.473688] RBP: c95ebc70 R08: R09:
0001
[ 261.473688] R10: 0004 R11: R12:
880179d02100
[ 261.473688] R13: 880179d31948 R14: 813b6030 R15:
0008
[ 261.473688] FS: () GS:88017ba0()
knlGS:
[ 261.473688] CS: 0010 DS: ES: CR0: 80050033
[ 261.473688] CR2: 570a34bc CR3: 0001758df000 CR4:
001006f0
[ 261.473688] Call Trace:
[ 261.473688] clk_core_disable_lock+0x1a/0x30
[ 261.473688] clk_disable+0x1a/0x20
[ 261.473688] i2c_dw_prepare_clk+0x25/0x60 [i2c_designware_core]
[ 261.473688] ? i2c_dw_disable+0xd/0x40 [i2c_designware_core]
[ 261.473688] dw_i2c_plat_suspend+0x2e/0x40 [i2c_designware_platform]
[ 261.473688] pm_generic_runtime_suspend+0x2c/0x30
[ 261.473688] acpi_lpss_runtime_suspend+0xd/0x30
[ 261.473688] __rpm_callback+0x75/0x1c0
[ 261.473688] ? acpi_lpss_suspend+0x1b0/0x1b0
[ 261.473688] rpm_callback+0x1f/0x80
[ 261.473688] ? acpi_lpss_suspend+0x1b0/0x1b0
[ 261.473688] rpm_suspend+0x143/0x6b0
[ 261.473688] rpm_idle+0x106/0x3c0
[ 261.473688] pm_runtime_work+0x7b/0xc0
[ 261.473688] process_one_work+0x206/0x5a0
[ 261.473688] worker_thread+0x3f/0x3a0
[ 261.473688] kthread+0x126/0x140
[ 261.473688] ? process_one_work+0x5a0/0x5a0
[ 261.473688] ? kthread_park+0x80/0x80
[ 261.473688] ret_from_fork+0x3a/0x50
[ 261.473688] irq event stamp: 1506
[ 261.473688] hardirqs last enabled at (1505): []
_raw_spin_unlock_irq+0x27/0x40
[ 261.473688] hardirqs last disabled at (1506): []
clk_enable_lock+0xd/0xa0
[ 261.473688] softirqs last enabled at (1310): []
__do_softirq+0x285/0x464
[ 261.473688] softirqs last disabled at (1303): []
irq_exit+0x9d/0xd0
[ 261.473688] ---[ end trace b9e91dac2ca18298 ]---
[ 261.509646] done.
[ 261.513366] PM: hibernation exit
[ 261.530737] [ cut here ]
[ 261.531249] 808622C1:03 already unprepared
[ 261.531629] WARNING: CPU: 0 PID: 149 at drivers/clk/clk.c:687
clk_core_unprepare+0x1aa/0x2e0
[ 261.532253] Modules linked in: hid_multitouch hid_generic i2c_hid
iTCO_wdt atkbd libps2 intel_rapl coretemp aesni_intel aes_x86_64
crypto_simd glue_helper intel_cstate i915 i2c_i801 intel_gtt
i2c_algo_bit lpc_ich drm_kms_helper syscopyarea sysfillrect sysimgblt
fb_sys_fops drm b
Re: [PATCH] [RFC] spi: pxa2xx: Do cs if restart the SSP during pxa2xx_spi_transfer_one()
Hi Jin
On 3/8/19 9:28 AM, Xiao, Jin wrote:
Yes, the spi core has de-asserted the CS before the
pxa2xx_spi_unprepare_transfer(). The problem on my side is that the new
transfer will restart the SSP in pxa2xx_spi_transfer_one(). The spi core
has asserted the CS again before restart the SSP. In my test the CS
assert before the restart SSP can't ensure the spi transfer working
correctly.
I wanted to ask have you found any other fix or reason than the patches
you have sent?
I've been trying to debug why the commit d5898e19c0d7 ("spi: pxa2xx: Use
core message processing loop") is causing the regression you are seeing.
I've been testing this by sending two or more messages each consisting
of two 4-byte transfers through the spidev. SPI mode 3 and clock is 1
MHz. I see only slight timing difference between a commit before and at
d5898e19c0d7 when the chip select is active.
8ae55af38817: CS act to CLK running ~40 us. CLK idle to CS deact ~26 us
d5898e19c0d7: CS act to CLK running ~34 us. CLK idle to CS deact ~18 us
There is more difference/variability during the times when the CS is not
active between messages. Which is understandable since there happens
message passing from userspace, queing, etc.
At 8ae55af38817 total time over 2 messages varies from ~600 us to 1200
~us and at d5898e19c0d7 from ~500 us to 1100 us. Then at v4.19 it goes a
bit slover and varies between ~700 to 1600 us. Most probably due some
other change than in SPI subsystem as there is not commit either in SPI
core or spi-pxa2xx explaining it. Or just some random config change in
my kernel configuration between 4.17 and 4.19.
What I don't understand why additional CS toggling makes it work for you
in case SSP was stopped in pxa2xx_spi_unprepare_transfer() and restarted
again in pxa2xx_spi_transfer_one(). This SSP stopping and restarting is
not visible in the SPI signals since clock is already stopped when FIFO
gets empty.
I guess the reason why you are seeing pxa2xx_spi_unprepare_transfer()
only called sometimes is that in those cases where it is not called a
new SPI message got queued while processing the current one and in other
cases queue became empty before new message was queued.
In both cases there shouldn't be difference in CS handling. Which makes
me scratching my head why additional CS toggling is fixing the issue in
case there is SSP restart. And which was due more time elapsed between
messages and queue went empty.
--
Jarkko
Re: [PATCH 1/2] i2c: i2c-designware-platdrv: Allow a dynamic adap. nr without an ACPI fwnode
Hi
On 3/11/19 1:22 PM, Hans de Goede wrote:
Before this commit the i2c-designware-platdrv assumes that if the pdev
has an apci-companion it should use a dynamic adapter-nr and otherwise
it will use pdev->id as adapter-nr.
On some devices e.g. the Apollo Lake using Acer TravelMate Spin B118,
some of the LPSS i2c-adapters are enumerated through PCI and do not have
an ACPI fwnode. These devices are handled as mfd devices so they end up
using the i2c-designware-platdrv driver.
This results in the i2c-adapter being registered with the mfd generated
pdev->id as adapter-nr, which conflicts with existing adapters, triggering
a WARN(id < 0, "couldn't get idr") in i2c-core-base.c and causing the
adapter registration to fail.
I went thinking would we get a regression if we switch the
i2c-designware-platdrv to dynamic numbering unconditionally?
Only drivers/mfd/intel-lpss.c and drivers/mfd/intel_quark_i2c_gpio.c
register platform device "i2c_designware" and otherwise in the driver
itself for known ACPI IDs and device tree bindings.
Things should be fine for ACPI cases if slave devices are also described
in ACPI tables. As far as I've understood with device tree matching
adapter number is irrelevant in slave device registration?
Andy: could you tell by commit 918fe70cf475 ("mfd: intel_quark_i2c_gpio:
support devices behind i2c bus") are those devices described in ACPI or
in some i2c_board_infos with referring to fixed adapter number either in
or out of kernel tree code?
Then drivers/platform/chrome/chromeos_laptop.c is the only code
searching for adapter named as "Synopsys DesignWare I2C adapter" without
assuming any fixed adapter numbering.
What's unclear to me can there be device tree cases where i2c-designware
probing comes with pdev->id not starting from zero or in different
order? I.e. would it make difference do we use pdev->id or dynamic
adapter numbering?
--
Jarkko
Re: [PATCH] [RFC] spi: pxa2xx: Do cs if restart the SSP during pxa2xx_spi_transfer_one()
Hi
Is this also related to the regression with d5898e19c0d7 ("spi: pxa2xx:
Use core message processing loop") you have found or another issue?
Comments below.
On 3/7/19 9:24 AM, xiao jin wrote:
The spi-pxa2xx can't read and write data correctly on our board.
The pxa_ssp_type is LPSS_BXT_SSP in our case.
With more debug we find that it's related to restart the SPP
during pxa2xx_spi_transfer_one().
In the normal case the spi_transfer_one_message() calls spi-pxa2xx
cs_assert before transferring one message. After completing the
transfer it calls spi-pxa2xx cs_deassert. The spi-pxa2xx works
well.
But in some other case pxa2xx_spi_unprepare_transfer() is called
that clears SSCR0_SSE bit before the next transfer. In the next
transfer the spi-pxa2xx driver will restart the SSP as the SSE
bit is cleared. The cs_assert before the SSP restart can't ensure
spi-pxa2xx work well.
The patch is to do cs again if spi-pxa2xx restar the SSP during
pxa2xx_spi_transfer_one()
Hmm.. please correct me if I'm wrong but pxa2xx_spi_unprepare_transfer()
is called always when there is no more messages pending and the spi core
should have deasserted the CS already?
More below.
Signed-off-by: xiao jin
Signed-off-by: he, bo
---
drivers/spi/spi-pxa2xx.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 14f4ea59caff..1a2ea46858d9 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -928,6 +928,7 @@ static int pxa2xx_spi_transfer_one(struct spi_controller
*master,
u32 cr1;
int err;
int dma_mapped;
+ bool need_cs_change = false;
/* Check if we can DMA this transfer */
if (transfer->len > MAX_DMA_LEN && chip->enable_dma) {
@@ -1056,6 +1057,11 @@ static int pxa2xx_spi_transfer_one(struct spi_controller
*master,
if ((pxa2xx_spi_read(drv_data, SSCR0) != cr0)
|| (pxa2xx_spi_read(drv_data, SSCR1) & change_mask)
!= (cr1 & change_mask)) {
+ /* It needs to deassert the chip selection
+* firstly before restart the SPP */
+ need_cs_change = true;
+ cs_deassert(spi);
+
I think code comes here at the beginning of each transfer so will be hit
multiple times before pxa2xx_spi_unprepare_transfer() if SPI message
consists of multiple transfers.
This makes me wondering if the device driver setting up the "struct
spi_transfer" is maybe missing the cs_change flag set for transfers
before last one in case HW needs CS toggling between transfers? For
instance what following drivers are doing with the cs_change flag:
drivers/char/tpm/tpm_tis_spi.c: tpm_tis_spi_transfer()
drivers/input/touchscreen/ad7877.c: ad7877_read(), ad7877_read_adc()
--
Jarkko
Re: [PATCH] spi-pxa2xx.c: modify the chip selection timing when spi transfer
Hi
On 3/6/19 5:05 AM, xiao jin wrote:
From: "he, bo"
We find spi can't work on board. More debug shows it's related
to the following patch that changed the chip selection assert and
deassert timing.
^^ timing caught my attention. More below.
@@ -610,6 +596,7 @@ static void int_transfer_complete(struct driver_data
*drv_data)
if (!pxa25x_ssp_comp(drv_data))
pxa2xx_spi_write(drv_data, SSTO, 0);
+ cs_deassert(drv_data);
spi_finalize_current_transfer(drv_data->master);
This
@@ -1070,6 +1057,7 @@ static int pxa2xx_spi_transfer_one(struct spi_controller
*master,
pxa2xx_spi_write(drv_data, SSTO, chip->timeout);
}
+ cs_assert(drv_data);
and this is not correct with core message loop. It will cause the chip
select is toggled with each transfer in PIO mode. If there is no
cs_change flag set then there shouldn't be CS toggling between the
transfers if SPI message consists of multiple transfers.
More over this patch also will regress with DMA mode since there won't
be CS deassert at all.
Timing reminded me I've seen two cases where there was a timing related
glitch in CS output:
d0283eb2dbc1 ("spi: pxa2xx: Add output control for multiple Intel LPSS
chip selects")
7a8d44bc89e5 ("spi: pxa2xx: Fix too early chipselect deassert")
Do you have a possibility to measure with an oscilloscope what goes
wrong with the CS after d5898e19c0d7 ("spi: pxa2xx: Use core message
processing loop")?
Can you share your setup if I can reproduce it here? E.g. SPI clock
frequency, single or multiple CS, frequency of occurrence, etc
--
Jarkko
Re: [PATCH v5 2/2] i2c: designware: Add support for an interface clock
On 2/28/19 3:52 PM, Gareth Williams wrote:
From: Phil Edworthy
The Synopsys I2C Controller has an interface clock, but most SoCs hide
this away. However, on some SoCs you need to explicitly enable the
interface clock in order to access the registers. Therefore, add
support for an optional interface clock.
Signed-off-by: Phil Edworthy
Signed-off-by: Gareth Williams
Acked-by: Wolfram Sang
---
v5:
- Updated comments to reference "interface clock" instead of
"peripheral clock".
- Corrected spelling in commit message, changing "explicity" to
"explicitly".
v4:
- Updated comments to reference "peripheral clock"
instead of "bus clock".
- Added Wolfram's Acked-by
v3:
- busclk renamed to pclk.
- Added comment with dw_i2c_dev struct definition describing pclk.
- Added enable rollback of first clock if second fails to enable.
v2:
- Use new devm_clk_get_optional() function as it simplifies handling when
the optional clock is not present.
---
drivers/i2c/busses/i2c-designware-common.c | 18 --
drivers/i2c/busses/i2c-designware-core.h| 2 ++
drivers/i2c/busses/i2c-designware-platdrv.c | 5 +
3 files changed, 23 insertions(+), 2 deletions(-)
Build & boot tested on linux-next that has the required commit
60b8f0ddf1a9 ("clk: Add (devm_)clk_get_optional() functions").
Acked-by: Jarkko Nikula
Tested-by: Jarkko Nikula
Re: linux-next: manual merge of the mfd tree with Linus' tree
On 2/8/19 4:37 AM, Stephen Rothwell wrote:
Hi all,
Today's linux-next merge of the mfd tree got a conflict in:
drivers/mfd/Kconfig
between commit:
9baddb61dfec ("mfd: Fix unmet dependency warning for MFD_TPS68470")
from Linus' tree and commit:
09fdc9857712 ("mfd: Kconfig: Fix I2C_DESIGNWARE_PLATFORM dependencies")
from the mfd tree.
I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging. You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.
Thanks, that's is the right fix for the conflict.
--
Jarkko
[PATCH] mfd: Fix I2C_DESIGNWARE_PLATFORM dependencies
INTEL_SOC_PMIC, INTEL_SOC_PMIC_CHTWC and MFD_TPS68470 select the
I2C_DESIGNWARE_PLATFORM without its dependencies making it possible to see
warning and build error like below:
WARNING: unmet direct dependencies detected for I2C_DESIGNWARE_PLATFORM
Depends on [n]: I2C [=y] && HAS_IOMEM [=y] && (ACPI [=y] && COMMON_CLK [=n]
|| !ACPI [=y])
Selected by [y]:
- MFD_TPS68470 [=y] && HAS_IOMEM [=y] && ACPI [=y] && I2C [=y]=y
/usr/bin/ld: drivers/i2c/busses/i2c-designware-platdrv.o: in function
`dw_i2c_plat_resume':
i2c-designware-platdrv.c:(.text+0x62): undefined reference to
`i2c_dw_prepare_clk'
/usr/bin/ld: drivers/i2c/busses/i2c-designware-platdrv.o: in function
`dw_i2c_plat_suspend':
i2c-designware-platdrv.c:(.text+0x9a): undefined reference to
`i2c_dw_prepare_clk'
/usr/bin/ld: drivers/i2c/busses/i2c-designware-platdrv.o: in function
`dw_i2c_plat_probe':
i2c-designware-platdrv.c:(.text+0x41c): undefined reference to
`i2c_dw_prepare_clk'
/usr/bin/ld: i2c-designware-platdrv.c:(.text+0x438): undefined reference to
`i2c_dw_read_comp_param'
/usr/bin/ld: i2c-designware-platdrv.c:(.text+0x545): undefined reference to
`i2c_dw_probe'
/usr/bin/ld: i2c-designware-platdrv.c:(.text+0x727): undefined reference to
`i2c_dw_probe_slave'
Fix this by making above options to depend on I2C_DESIGNWARE_PLATFORM
being built-in. I2C_DESIGNWARE_PLATFORM is a visible symbol with
dependencies so in general the select should be avoided.
Reported-by: Randy Dunlap
Fixes: acebcff9eda8 ("mfd: intel_soc_pmic: Select designware i2c-bus driver")
Fixes: de85d79f4aab ("mfd: Add Cherry Trail Whiskey Cove PMIC driver")
Fixes: 9bbf6a15ce19 ("mfd: Add support for TPS68470 device")
Cc: Stable # v4.14+
Signed-off-by: Jarkko Nikula
---
drivers/mfd/Kconfig | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index f461460a2aeb..6a84a8ecd4a9 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -519,10 +519,10 @@ config INTEL_SOC_PMIC
bool "Support for Crystal Cove PMIC"
depends on ACPI && HAS_IOMEM && I2C=y && GPIOLIB && COMMON_CLK
depends on X86 || COMPILE_TEST
+ depends on I2C_DESIGNWARE_PLATFORM=y
select MFD_CORE
select REGMAP_I2C
select REGMAP_IRQ
- select I2C_DESIGNWARE_PLATFORM
help
Select this option to enable support for Crystal Cove PMIC
on some Intel SoC systems. The PMIC provides ADC, GPIO,
@@ -548,10 +548,10 @@ config INTEL_SOC_PMIC_CHTWC
bool "Support for Intel Cherry Trail Whiskey Cove PMIC"
depends on ACPI && HAS_IOMEM && I2C=y && COMMON_CLK
depends on X86 || COMPILE_TEST
+ depends on I2C_DESIGNWARE_PLATFORM=y
select MFD_CORE
select REGMAP_I2C
select REGMAP_IRQ
- select I2C_DESIGNWARE_PLATFORM
help
Select this option to enable support for the Intel Cherry Trail
Whiskey Cove PMIC found on some Intel Cherry Trail systems.
@@ -1420,9 +1420,9 @@ config MFD_TPS65217
config MFD_TPS68470
bool "TI TPS68470 Power Management / LED chips"
depends on ACPI && I2C=y
+ depends on I2C_DESIGNWARE_PLATFORM=y
select MFD_CORE
select REGMAP_I2C
- select I2C_DESIGNWARE_PLATFORM
help
If you say yes here you get support for the TPS68470 series of
Power Management / LED chips.
--
2.20.1
Re: linux-next: Tree for Jan 18 (i2c-designware-platdrv.c)
Hi On 1/18/19 6:27 PM, Randy Dunlap wrote: On 1/17/19 8:24 PM, Stephen Rothwell wrote: Hi all, Changes since 20190117: on i386 or x86_64: ld: drivers/i2c/busses/i2c-designware-platdrv.o: in function `dw_i2c_plat_resume': i2c-designware-platdrv.c:(.text+0x4b): undefined reference to `i2c_dw_prepare_clk' ld: drivers/i2c/busses/i2c-designware-platdrv.o: in function `dw_i2c_plat_suspend': i2c-designware-platdrv.c:(.text+0x95): undefined reference to `i2c_dw_prepare_clk' ld: drivers/i2c/busses/i2c-designware-platdrv.o: in function `dw_i2c_plat_probe': i2c-designware-platdrv.c:(.text+0x63f): undefined reference to `i2c_dw_prepare_clk' ld: i2c-designware-platdrv.c:(.text+0x686): undefined reference to `i2c_dw_read_comp_param' ld: i2c-designware-platdrv.c:(.text+0x7b7): undefined reference to `i2c_dw_probe_slave' ld: i2c-designware-platdrv.c:(.text+0x7c1): undefined reference to `i2c_dw_probe' probably related to: WARNING: unmet direct dependencies detected for I2C_DESIGNWARE_PLATFORM Depends on [n]: I2C [=y] && HAS_IOMEM [=y] && (ACPI [=y] && COMMON_CLK [=n] || !ACPI [=y]) Selected by [y]: - MFD_TPS68470 [=y] && HAS_IOMEM [=y] && ACPI [=y] && I2C [=y]=y Thanks, this is due CONFIG_MFD_TPS68470 which selects the I2C_DESIGNWARE_PLATFORM but not the dependencies. There are two others in drivers/mfd/Kconfig. I'll send a patch changing them to depend on I2C_DESIGNWARE_PLATFORM=y. -- Jarkko
Re: [PATCH] PCI: fix using __initdata memory after free in disable_acs_redir parameter
Hi
On 1/15/19 7:32 PM, Logan Gunthorpe wrote:
The disable_acs_redir parameter stores a pointer to the string passed to
pci_setup(). However, the string passed to PCI setup is actually a
temporary copy allocated in static __initdata memory. After init, once
the memory is freed, it is no longer valid to reference this pointer.
This bug was noticed in v5.0-rc1 after a change in commit c5eb1190074c
("PCI / PM: Allow runtime PM without callback functions") caused
pci_disable_acs_redir() to be called during shutdown which manifested
as an unable to handle kernel paging request at:
RIP: 0010:pci_enable_acs+0x3f/0x1e0
Call Trace:
pci_restore_state.part.44+0x159/0x3c0
pci_restore_standard_config+0x33/0x40
pci_pm_runtime_resume+0x2b/0xd0
? pci_restore_standard_config+0x40/0x40
__rpm_callback+0xbc/0x1b0
rpm_callback+0x1f/0x70
? pci_restore_standard_config+0x40/0x40
rpm_resume+0x4f9/0x710
? pci_conf1_read+0xb6/0xf0
? pci_conf1_write+0xb2/0xe0
__pm_runtime_resume+0x47/0x70
pci_device_shutdown+0x1e/0x60
So this doesn't happen if you revert c5eb1190074c?
I guess this is due dev->state_saved being true set by
pci_pm_runtime_suspend() -> pci_save_state() after my patch and now
pci_pm_runtime_resume() -> pci_restore_standard_config() ->
pci_restore_state() reach the pci_enable_acs(). I think this is possible
to trigger also before my patch if device has the runtime PM callback
defined?
It was also likely possible to trigger this bug when hotplugging PCI
devices.
To fix this, instead of storing a pointer, we use kstrdup to copy the
disable_acs_redir_param to its own buffer which will never be freed.
I wasn't able to trigger this but I saw
"PCI: Can't parse disable_acs_redir parameter: " followed by a few lines
of junk during boot when I defined pci=disable_acs_redir=:00:xy.z
which disappear after your patch.
Tested-by: Jarkko Nikula
Reviewed-by: Jarkko Nikula
Re: [RFC 0/9] ASoC/ARM: Merge the davinci and omap audio directories
On 11/8/18 10:37 AM, Peter Ujfalusi wrote:
> The series also includes patches to updated the arch/arm/ files (defconfigs,
> source files using audio Kconfig symbols).
>
> I believe with v1 all of this can be merged via ASoC tree as the chances of
> conflicts are low for the touched arch/arm files. But Sekhar and Tony can
> correct me if I'm wrong.
>
> I think I have catched all (I hope) issues regarding to Kconfig thanks to the
> various autobuild machines out there kindly testing my linux-next-wip branch.
>
> I would appreciate any testing comments before I send the v1!
>
I applied on top of
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git commit
abd97b7c53ac ("Merge branch 'asoc-4.21' into asoc-next").
Patches 2 and 6 didn't apply and 7-8 I skipped. For my functional
testing none of those were required.
For the sound/soc/{omap => ti} conversion you may add:
Tested-by: Jarkko Nikula
Acked-by: Jarkko Nikula
Re: [RFC 0/9] ASoC/ARM: Merge the davinci and omap audio directories
On 11/8/18 10:37 AM, Peter Ujfalusi wrote:
> The series also includes patches to updated the arch/arm/ files (defconfigs,
> source files using audio Kconfig symbols).
>
> I believe with v1 all of this can be merged via ASoC tree as the chances of
> conflicts are low for the touched arch/arm files. But Sekhar and Tony can
> correct me if I'm wrong.
>
> I think I have catched all (I hope) issues regarding to Kconfig thanks to the
> various autobuild machines out there kindly testing my linux-next-wip branch.
>
> I would appreciate any testing comments before I send the v1!
>
I applied on top of
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git commit
abd97b7c53ac ("Merge branch 'asoc-4.21' into asoc-next").
Patches 2 and 6 didn't apply and 7-8 I skipped. For my functional
testing none of those were required.
For the sound/soc/{omap => ti} conversion you may add:
Tested-by: Jarkko Nikula
Acked-by: Jarkko Nikula
Re: [PATCH 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code
On 10/18/2018 10:36 AM, Andy Shevchenko wrote: On Thu, Oct 18, 2018 at 10:33 AM Rafael J. Wysocki wrote: On Sun, Sep 23, 2018 at 4:45 PM Hans de Goede wrote: On some BYT/CHT systems the SoC's P-Unit shares the I2C bus with the kernel. The P-Unit has a semaphore for the PMIC bus which we can take to block it from accessing the shared bus while the kernel wants to access it. Currently we have the I2C-controller driver acquiring and releasing the semaphore around each I2C transfer. There are 2 problems with this: 1) PMIC accesses often come in the form of a read-modify-write on one of the PMIC registers, we currently release the P-Unit's PMIC bus semaphore between the read and the write. If the P-Unit modifies the register during this window?, then we end up overwriting the P-Unit's changes. I believe that this is mostly an academic problem, but I'm not sure. 2) To safely access the shared I2C bus, we need to do 3 things: a) Notify the GPU driver that we are starting a window in which it may not access the P-Unit, since the P-Unit seems to ignore the semaphore for explicit power-level requests made by the GPU driver b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering C6/C7 while we hold the semaphore hangs the SoC c) Finally take the P-Unit's PMIC bus semaphore All 3 these steps together are somewhat expensive, so ideally if we have a bunch of i2c transfers grouped together we only do this once for the entire group. Taking the read-modify-write on a PMIC register as example then ideally we would only do all 3 steps once at the beginning and undo all 3 steps once at the end. For this we need to be able to take the semaphore from within e.g. the PMIC opregion driver, yet we do not want to remove the taking of the semaphore from the I2C-controller driver, as that is still necessary to protect many other code-paths leading to accessing the shared I2C bus. This means that we first have the PMIC driver acquire the semaphore and then have the I2C controller driver trying to acquire it again. To make this possible this commit does the following: 1) Move the semaphore code from being private to the I2C controller driver into the generic iosf_mbi code, which already has other code to deal with the shared bus so that it can be accessed outside of the I2C bus driver. 2) Rework the code so that it can be called multiple times nested, while still blocking I2C accesses while e.g. the GPU driver has indicated the P-Unit needs the bus through a iosf_mbi_punit_acquire() call. Signed-off-by: Hans de Goede If there are no objections or concerns regarding this patch, I'm inclined to take the entire series including it. Please, go ahead, it looks good to me. Thanks! Just in case note: please remember to take patches and tags from v3 of the series, not from this v1 thread. -- Jarkko
Re: [PATCH 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code
On 10/18/2018 10:36 AM, Andy Shevchenko wrote: On Thu, Oct 18, 2018 at 10:33 AM Rafael J. Wysocki wrote: On Sun, Sep 23, 2018 at 4:45 PM Hans de Goede wrote: On some BYT/CHT systems the SoC's P-Unit shares the I2C bus with the kernel. The P-Unit has a semaphore for the PMIC bus which we can take to block it from accessing the shared bus while the kernel wants to access it. Currently we have the I2C-controller driver acquiring and releasing the semaphore around each I2C transfer. There are 2 problems with this: 1) PMIC accesses often come in the form of a read-modify-write on one of the PMIC registers, we currently release the P-Unit's PMIC bus semaphore between the read and the write. If the P-Unit modifies the register during this window?, then we end up overwriting the P-Unit's changes. I believe that this is mostly an academic problem, but I'm not sure. 2) To safely access the shared I2C bus, we need to do 3 things: a) Notify the GPU driver that we are starting a window in which it may not access the P-Unit, since the P-Unit seems to ignore the semaphore for explicit power-level requests made by the GPU driver b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering C6/C7 while we hold the semaphore hangs the SoC c) Finally take the P-Unit's PMIC bus semaphore All 3 these steps together are somewhat expensive, so ideally if we have a bunch of i2c transfers grouped together we only do this once for the entire group. Taking the read-modify-write on a PMIC register as example then ideally we would only do all 3 steps once at the beginning and undo all 3 steps once at the end. For this we need to be able to take the semaphore from within e.g. the PMIC opregion driver, yet we do not want to remove the taking of the semaphore from the I2C-controller driver, as that is still necessary to protect many other code-paths leading to accessing the shared I2C bus. This means that we first have the PMIC driver acquire the semaphore and then have the I2C controller driver trying to acquire it again. To make this possible this commit does the following: 1) Move the semaphore code from being private to the I2C controller driver into the generic iosf_mbi code, which already has other code to deal with the shared bus so that it can be accessed outside of the I2C bus driver. 2) Rework the code so that it can be called multiple times nested, while still blocking I2C accesses while e.g. the GPU driver has indicated the P-Unit needs the bus through a iosf_mbi_punit_acquire() call. Signed-off-by: Hans de Goede If there are no objections or concerns regarding this patch, I'm inclined to take the entire series including it. Please, go ahead, it looks good to me. Thanks! Just in case note: please remember to take patches and tags from v3 of the series, not from this v1 thread. -- Jarkko
Re: [PATCH v3 3/3] i2c: designware: Cleanup bus lock handling
On 10/11/2018 05:29 PM, Hans de Goede wrote: Now that most of the special Bay- / Cherry-Trail bus lock handling has been moved to the iosf_mbi code we can simplify the remaining code a bit. Signed-off-by: Hans de Goede --- drivers/i2c/busses/i2c-designware-baytrail.c | 18 ++ drivers/i2c/busses/i2c-designware-common.c | 4 ++-- drivers/i2c/busses/i2c-designware-core.h | 9 ++--- drivers/i2c/busses/i2c-designware-platdrv.c | 2 -- 4 files changed, 6 insertions(+), 27 deletions(-) Acked-by: Jarkko Nikula Tested-by: Jarkko Nikula
Re: [PATCH v3 3/3] i2c: designware: Cleanup bus lock handling
On 10/11/2018 05:29 PM, Hans de Goede wrote: Now that most of the special Bay- / Cherry-Trail bus lock handling has been moved to the iosf_mbi code we can simplify the remaining code a bit. Signed-off-by: Hans de Goede --- drivers/i2c/busses/i2c-designware-baytrail.c | 18 ++ drivers/i2c/busses/i2c-designware-common.c | 4 ++-- drivers/i2c/busses/i2c-designware-core.h | 9 ++--- drivers/i2c/busses/i2c-designware-platdrv.c | 2 -- 4 files changed, 6 insertions(+), 27 deletions(-) Acked-by: Jarkko Nikula Tested-by: Jarkko Nikula
Re: [PATCH v3 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code
On 10/14/2018 04:17 PM, Wolfram Sang wrote: On Thu, Oct 11, 2018 at 04:29:09PM +0200, Hans de Goede wrote: On some BYT/CHT systems the SoC's P-Unit shares the I2C bus with the kernel. The P-Unit has a semaphore for the PMIC bus which we can take to block it from accessing the shared bus while the kernel wants to access it. Currently we have the I2C-controller driver acquiring and releasing the semaphore around each I2C transfer. There are 2 problems with this: 1) PMIC accesses often come in the form of a read-modify-write on one of the PMIC registers, we currently release the P-Unit's PMIC bus semaphore between the read and the write. If the P-Unit modifies the register during this window?, then we end up overwriting the P-Unit's changes. I believe that this is mostly an academic problem, but I'm not sure. 2) To safely access the shared I2C bus, we need to do 3 things: a) Notify the GPU driver that we are starting a window in which it may not access the P-Unit, since the P-Unit seems to ignore the semaphore for explicit power-level requests made by the GPU driver b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering C6/C7 while we hold the semaphore hangs the SoC c) Finally take the P-Unit's PMIC bus semaphore All 3 these steps together are somewhat expensive, so ideally if we have a bunch of i2c transfers grouped together we only do this once for the entire group. Taking the read-modify-write on a PMIC register as example then ideally we would only do all 3 steps once at the beginning and undo all 3 steps once at the end. For this we need to be able to take the semaphore from within e.g. the PMIC opregion driver, yet we do not want to remove the taking of the semaphore from the I2C-controller driver, as that is still necessary to protect many other code-paths leading to accessing the shared I2C bus. This means that we first have the PMIC driver acquire the semaphore and then have the I2C controller driver trying to acquire it again. To make this possible this commit does the following: 1) Move the semaphore code from being private to the I2C controller driver into the generic iosf_mbi code, which already has other code to deal with the shared bus so that it can be accessed outside of the I2C bus driver. 2) Rework the code so that it can be called multiple times nested, while still blocking I2C accesses while e.g. the GPU driver has indicated the P-Unit needs the bus through a iosf_mbi_punit_acquire() call. Signed-off-by: Hans de Goede For the record: once the designware maintainers are okay with this change, I am also okay with it going via the x86 platform tree. Acked-by: Jarkko Nikula Tested-by: Jarkko Nikula
Re: [PATCH v3 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code
On 10/14/2018 04:17 PM, Wolfram Sang wrote: On Thu, Oct 11, 2018 at 04:29:09PM +0200, Hans de Goede wrote: On some BYT/CHT systems the SoC's P-Unit shares the I2C bus with the kernel. The P-Unit has a semaphore for the PMIC bus which we can take to block it from accessing the shared bus while the kernel wants to access it. Currently we have the I2C-controller driver acquiring and releasing the semaphore around each I2C transfer. There are 2 problems with this: 1) PMIC accesses often come in the form of a read-modify-write on one of the PMIC registers, we currently release the P-Unit's PMIC bus semaphore between the read and the write. If the P-Unit modifies the register during this window?, then we end up overwriting the P-Unit's changes. I believe that this is mostly an academic problem, but I'm not sure. 2) To safely access the shared I2C bus, we need to do 3 things: a) Notify the GPU driver that we are starting a window in which it may not access the P-Unit, since the P-Unit seems to ignore the semaphore for explicit power-level requests made by the GPU driver b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering C6/C7 while we hold the semaphore hangs the SoC c) Finally take the P-Unit's PMIC bus semaphore All 3 these steps together are somewhat expensive, so ideally if we have a bunch of i2c transfers grouped together we only do this once for the entire group. Taking the read-modify-write on a PMIC register as example then ideally we would only do all 3 steps once at the beginning and undo all 3 steps once at the end. For this we need to be able to take the semaphore from within e.g. the PMIC opregion driver, yet we do not want to remove the taking of the semaphore from the I2C-controller driver, as that is still necessary to protect many other code-paths leading to accessing the shared I2C bus. This means that we first have the PMIC driver acquire the semaphore and then have the I2C controller driver trying to acquire it again. To make this possible this commit does the following: 1) Move the semaphore code from being private to the I2C controller driver into the generic iosf_mbi code, which already has other code to deal with the shared bus so that it can be accessed outside of the I2C bus driver. 2) Rework the code so that it can be called multiple times nested, while still blocking I2C accesses while e.g. the GPU driver has indicated the P-Unit needs the bus through a iosf_mbi_punit_acquire() call. Signed-off-by: Hans de Goede For the record: once the designware maintainers are okay with this change, I am also okay with it going via the x86 platform tree. Acked-by: Jarkko Nikula Tested-by: Jarkko Nikula
Re: [PATCH 3/3] ARM: OMAP2PLUS: Remove references to AM3517-EVM since it's in DT
On 09/23/18 18:37, Adam Ford wrote: > Since the audio codecs used on the AM3517-EVM are now available > in device tree, and the audio driver wasn't really being used > anyway, this patch removes the remaining AM3517 legacy board > options and sound soc drivers. > > Signed-off-by: Adam Ford > For sound/soc/omap/ changes: Acked-by: Jarkko Nikula
Re: [PATCH 3/3] ARM: OMAP2PLUS: Remove references to AM3517-EVM since it's in DT
On 09/23/18 18:37, Adam Ford wrote: > Since the audio codecs used on the AM3517-EVM are now available > in device tree, and the audio driver wasn't really being used > anyway, this patch removes the remaining AM3517 legacy board > options and sound soc drivers. > > Signed-off-by: Adam Ford > For sound/soc/omap/ changes: Acked-by: Jarkko Nikula
Re: [PATCH v3 3/3] i2c: designware: select gpio/default pin when prepare/unprepare recovery
On 09/17/2018 06:34 AM, Jisheng Zhang wrote: + DT maintainers On Mon, 17 Sep 2018 11:29:49 +0800 Jisheng Zhang wrote: On some platforms, the sda/scl pins are muxed with gpio functions, so they could be used for recovery. Select the gpio/default pin function when prepare/unprepare recovery. Signed-off-by: Jisheng Zhang --- drivers/i2c/busses/i2c-designware-core.h | 3 +++ drivers/i2c/busses/i2c-designware-master.c | 22 ++ 2 files changed, 25 insertions(+) Acked-by: Jarkko Nikula
Re: [PATCH v3 3/3] i2c: designware: select gpio/default pin when prepare/unprepare recovery
On 09/17/2018 06:34 AM, Jisheng Zhang wrote: + DT maintainers On Mon, 17 Sep 2018 11:29:49 +0800 Jisheng Zhang wrote: On some platforms, the sda/scl pins are muxed with gpio functions, so they could be used for recovery. Select the gpio/default pin function when prepare/unprepare recovery. Signed-off-by: Jisheng Zhang --- drivers/i2c/busses/i2c-designware-core.h | 3 +++ drivers/i2c/busses/i2c-designware-master.c | 22 ++ 2 files changed, 25 insertions(+) Acked-by: Jarkko Nikula
Re: [PATCH v2] i2c: designware: select gpio/default pin when prepare/unprepare recovery
On 09/13/2018 11:21 AM, Jisheng Zhang wrote:
On some platforms, the sda/scl pins are muxed with gpio functions, so
they could be used for recovery. Select the gpio/default pin function
when prepare/unprepare recovery.
Signed-off-by: Jisheng Zhang
---
since v1:
- use IS_ERR_OR_NULL
drivers/i2c/busses/i2c-designware-core.h | 3 +++
drivers/i2c/busses/i2c-designware-master.c | 22 ++
2 files changed, 25 insertions(+)
...
+ pinctrl = devm_pinctrl_get(dev->dev);
+ if (PTR_ERR(pinctrl) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+ if (!IS_ERR_OR_NULL(pinctrl)) {
+ dev->pinctrl = pinctrl;
+ s = pinctrl_lookup_state(pinctrl, PINCTRL_STATE_DEFAULT);
+ if (!IS_ERR_OR_NULL(s))
+ dev->pins_default = s;
+ s = pinctrl_lookup_state(pinctrl, "gpio");
+ if (!IS_ERR_OR_NULL(s))
+ dev->pins_gpio = s;
+ }
+
Should these be documented in
Documentation/devicetree/bindings/i2c/i2c-designware.txt?
--
Jarkko
Re: [PATCH v2] i2c: designware: select gpio/default pin when prepare/unprepare recovery
On 09/13/2018 11:21 AM, Jisheng Zhang wrote:
On some platforms, the sda/scl pins are muxed with gpio functions, so
they could be used for recovery. Select the gpio/default pin function
when prepare/unprepare recovery.
Signed-off-by: Jisheng Zhang
---
since v1:
- use IS_ERR_OR_NULL
drivers/i2c/busses/i2c-designware-core.h | 3 +++
drivers/i2c/busses/i2c-designware-master.c | 22 ++
2 files changed, 25 insertions(+)
...
+ pinctrl = devm_pinctrl_get(dev->dev);
+ if (PTR_ERR(pinctrl) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+ if (!IS_ERR_OR_NULL(pinctrl)) {
+ dev->pinctrl = pinctrl;
+ s = pinctrl_lookup_state(pinctrl, PINCTRL_STATE_DEFAULT);
+ if (!IS_ERR_OR_NULL(s))
+ dev->pins_default = s;
+ s = pinctrl_lookup_state(pinctrl, "gpio");
+ if (!IS_ERR_OR_NULL(s))
+ dev->pins_gpio = s;
+ }
+
Should these be documented in
Documentation/devicetree/bindings/i2c/i2c-designware.txt?
--
Jarkko
Re: [PATCH v4 0/7] Add support for MSCC Ocelot i2c
On 08/16/2018 11:45 AM, Alexandre Belloni wrote: Hello, Because the designware IP was not able to handle the SDA hold time before version 1.11a, MSCC has its own implementation. Add support for it and then add i2c on ocelot boards. I would expect patches 1 to 5 to go through the i2c tree and 6-7 through the mips tree once patch 4 has been reviewed by the DT maintainers. For the patches 1-3 and 5/7 that touch i2c-designware: Tested-by: Jarkko Nikula Acked-by: Jarkko Nikula
Re: [PATCH v4 0/7] Add support for MSCC Ocelot i2c
On 08/16/2018 11:45 AM, Alexandre Belloni wrote: Hello, Because the designware IP was not able to handle the SDA hold time before version 1.11a, MSCC has its own implementation. Add support for it and then add i2c on ocelot boards. I would expect patches 1 to 5 to go through the i2c tree and 6-7 through the mips tree once patch 4 has been reviewed by the DT maintainers. For the patches 1-3 and 5/7 that touch i2c-designware: Tested-by: Jarkko Nikula Acked-by: Jarkko Nikula
Re: [PATCH v3 0/6] Add support for MSCC Ocelot i2c
On 08/06/2018 09:54 PM, Alexandre Belloni wrote: Hello, Because the designware IP was not able to handle the SDA hold time before version 1.11a, MSCC has its own implementation. Add support for it and then add i2c on ocelot boards. I would expect patches 1 to 4 to go through the i2c tree and 5-6 through the mips tree once patch 4 has been reviewed by the DT maintainers. For the patches 1-4/6 that touch i2c-designware: Tested-by: Jarkko Nikula Acked-by: Jarkko Nikula I tested acpi_match_device() conversion to device_get_match_data() didn't affect MODEL_CHERRYTRAIL case and quick test that i2c communication continue working.
Re: [PATCH v3 0/6] Add support for MSCC Ocelot i2c
On 08/06/2018 09:54 PM, Alexandre Belloni wrote: Hello, Because the designware IP was not able to handle the SDA hold time before version 1.11a, MSCC has its own implementation. Add support for it and then add i2c on ocelot boards. I would expect patches 1 to 4 to go through the i2c tree and 5-6 through the mips tree once patch 4 has been reviewed by the DT maintainers. For the patches 1-4/6 that touch i2c-designware: Tested-by: Jarkko Nikula Acked-by: Jarkko Nikula I tested acpi_match_device() conversion to device_get_match_data() didn't affect MODEL_CHERRYTRAIL case and quick test that i2c communication continue working.
Re: [PATCH] i2c: aspeed: Improve driver to support multi-master use cases stably
Hi On 06/26/2018 07:58 PM, Jae Hyun Yoo wrote: BMC firmware should support some multi-master use cases such as multi-node, IPMB, BMC-ME link and so on but the current ASPEED I2C driver is a bit unstable for the multi-master use case. So this patch improves ASPEED I2C driver to support the multi-master use case stably. Changes: * Added XFER_MODE status register checking logic into aspeed_i2c_master_xfer to improve the current bus busy checking logic. * Changed the order of enum aspeed_i2c_master_state and enum aspeed_i2c_slave_state defines to make their initial values set to ASPEED_I2C_MASTER_INACTIVE and ASPEED_I2C_SLAVE_STOP respectively. In case of multi-master use with previous code, if a slave data comes ahead of the first master xfer, master_state starts from an invalid state. This change fixed the issue. * Adjusted spin_lock scope to make it wrap the whole irq handler using a single lock and unlock pair covers both master and slave handlers. * Added irq_status variable as a member of the struct aspeed_i2c_bus to collect handled interrupt bits throughout the master and the slave irq handlers. * Added control logic to put an order on calling the master and the slave irq handlers based on their current states. This does many unrelated looking changes in one patch making it more vulnerable for potential multiple regressions. For instance busy checking goes from single read to loop with 250 ms timeout in this patch while changing also spin lock logic and interrupt handling. Now if there is some regression it might be difficult to find what change in this patch is causing it and more over things goes more complicated if some other kind of regressions are found pointing into the same commit. I suggest splitting this into multiple smaller patches. For instance having first simple conversions patches that are unlikely to cause a regression like one patch adding '\n' to error print, another moving irq_status variable into struct aspeed_i2c_bus and so on followed by patches that change logic like busy checking, spin lock change and then patch or more for multi-master support. -- Jarkko
Re: [PATCH] i2c: aspeed: Improve driver to support multi-master use cases stably
Hi On 06/26/2018 07:58 PM, Jae Hyun Yoo wrote: BMC firmware should support some multi-master use cases such as multi-node, IPMB, BMC-ME link and so on but the current ASPEED I2C driver is a bit unstable for the multi-master use case. So this patch improves ASPEED I2C driver to support the multi-master use case stably. Changes: * Added XFER_MODE status register checking logic into aspeed_i2c_master_xfer to improve the current bus busy checking logic. * Changed the order of enum aspeed_i2c_master_state and enum aspeed_i2c_slave_state defines to make their initial values set to ASPEED_I2C_MASTER_INACTIVE and ASPEED_I2C_SLAVE_STOP respectively. In case of multi-master use with previous code, if a slave data comes ahead of the first master xfer, master_state starts from an invalid state. This change fixed the issue. * Adjusted spin_lock scope to make it wrap the whole irq handler using a single lock and unlock pair covers both master and slave handlers. * Added irq_status variable as a member of the struct aspeed_i2c_bus to collect handled interrupt bits throughout the master and the slave irq handlers. * Added control logic to put an order on calling the master and the slave irq handlers based on their current states. This does many unrelated looking changes in one patch making it more vulnerable for potential multiple regressions. For instance busy checking goes from single read to loop with 250 ms timeout in this patch while changing also spin lock logic and interrupt handling. Now if there is some regression it might be difficult to find what change in this patch is causing it and more over things goes more complicated if some other kind of regressions are found pointing into the same commit. I suggest splitting this into multiple smaller patches. For instance having first simple conversions patches that are unlikely to cause a regression like one patch adding '\n' to error print, another moving irq_status variable into struct aspeed_i2c_bus and so on followed by patches that change logic like busy checking, spin lock change and then patch or more for multi-master support. -- Jarkko
Re: [BUG] i2c-hid: ELAN Touchpad does not work on ASUS X580GD
On 05/18/2018 10:48 AM, Hans de Goede wrote: Could it be the i2c input clock definition in drivers/mfd/intel-lpss-pci.c is also wrong for Apollo Lake (N3450) ? There are lots of people having various issues with i2c attached touchpads on Apollo Lake devices, this bug: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1728244 Is sort of a collection bug for these. Various models laptops, lots of reporters. Note not sure thie is an i2c-designware issue, but it would be good to double check the input clock on Apollo Lake. Does i2c_designware_core.dyndbg=+p and i2c-hid.debug=1 command line arguments give any useful debug information from those machines? -- Jarkko
Re: [BUG] i2c-hid: ELAN Touchpad does not work on ASUS X580GD
On 05/18/2018 10:48 AM, Hans de Goede wrote: Could it be the i2c input clock definition in drivers/mfd/intel-lpss-pci.c is also wrong for Apollo Lake (N3450) ? There are lots of people having various issues with i2c attached touchpads on Apollo Lake devices, this bug: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1728244 Is sort of a collection bug for these. Various models laptops, lots of reporters. Note not sure thie is an i2c-designware issue, but it would be good to double check the input clock on Apollo Lake. Does i2c_designware_core.dyndbg=+p and i2c-hid.debug=1 command line arguments give any useful debug information from those machines? -- Jarkko
Re: [BUG] i2c-hid: ELAN Touchpad does not work on ASUS X580GD
On 05/18/2018 01:20 PM, Andy Shevchenko wrote:
On Fri, 2018-05-18 at 11:37 +0300, Andy Shevchenko wrote:
On Fri, 2018-05-18 at 09:48 +0200, Hans de Goede wrote:
Could it be the i2c input clock definition in drivers/mfd/intel-
lpss-
pci.c
is also wrong for Apollo Lake (N3450) ? There are lots of people
having
various issues with i2c attached touchpads on Apollo Lake devices,
this bug:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1728244
Is sort of a collection bug for these. Various models laptops, lots
of
reporters. Note not sure thie is an i2c-designware issue, but it
would
be good to double check the input clock on Apollo Lake.
I've checked the datasheet and the datasheet mentions 133MHz as
"serial input clk" in the lpio_bxt_regs Registers Summary, which is
also part of the LPSS, no clk is mentioned in the "Summary of
DW_apb_i2c_mem_map_DW_apb_i2c_addr_block1 Registers".
The internal datasheet we have access to mentioned in this case for
Broxton and Cannonlake together. So, your assumption might be quite
close to the truth and the issue is inherited from Broxton.
Nope. The specification I have mention the I2C input clock in Broxton is
fixed 133 MHz but in Cannon Lake it is derived through non-SW visible
divider.
Hans, can your reporters try the following patch? Depending on the
result I may send it out ASAP.
--- a/drivers/mfd/intel-lpss-pci.c
+++ b/drivers/mfd/intel-lpss-pci.c
@@ -120,7 +120,7 @@ static struct property_entry apl_i2c_properties[] =
{
};
static const struct intel_lpss_platform_info apl_i2c_info = {
- .clk_rate = 13300,
+ .clk_rate = 21600,
.properties = apl_i2c_properties,
};
Nack. The Apollo Lake HW here shows expected I2C bus clock on
oscilloscope so it is indeed clocked at 133 MHz.
--
Jarkko
Re: [BUG] i2c-hid: ELAN Touchpad does not work on ASUS X580GD
On 05/18/2018 01:20 PM, Andy Shevchenko wrote:
On Fri, 2018-05-18 at 11:37 +0300, Andy Shevchenko wrote:
On Fri, 2018-05-18 at 09:48 +0200, Hans de Goede wrote:
Could it be the i2c input clock definition in drivers/mfd/intel-
lpss-
pci.c
is also wrong for Apollo Lake (N3450) ? There are lots of people
having
various issues with i2c attached touchpads on Apollo Lake devices,
this bug:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1728244
Is sort of a collection bug for these. Various models laptops, lots
of
reporters. Note not sure thie is an i2c-designware issue, but it
would
be good to double check the input clock on Apollo Lake.
I've checked the datasheet and the datasheet mentions 133MHz as
"serial input clk" in the lpio_bxt_regs Registers Summary, which is
also part of the LPSS, no clk is mentioned in the "Summary of
DW_apb_i2c_mem_map_DW_apb_i2c_addr_block1 Registers".
The internal datasheet we have access to mentioned in this case for
Broxton and Cannonlake together. So, your assumption might be quite
close to the truth and the issue is inherited from Broxton.
Nope. The specification I have mention the I2C input clock in Broxton is
fixed 133 MHz but in Cannon Lake it is derived through non-SW visible
divider.
Hans, can your reporters try the following patch? Depending on the
result I may send it out ASAP.
--- a/drivers/mfd/intel-lpss-pci.c
+++ b/drivers/mfd/intel-lpss-pci.c
@@ -120,7 +120,7 @@ static struct property_entry apl_i2c_properties[] =
{
};
static const struct intel_lpss_platform_info apl_i2c_info = {
- .clk_rate = 13300,
+ .clk_rate = 21600,
.properties = apl_i2c_properties,
};
Nack. The Apollo Lake HW here shows expected I2C bus clock on
oscilloscope so it is indeed clocked at 133 MHz.
--
Jarkko
