From: Sasha Levin <sashal@kernel.org>
To: patches@lists.linux.dev, stable@vger.kernel.org
Cc: Tali Perry <tali.perry1@gmail.com>,
Mohammed Elbadry <mohammed.0.elbadry@gmail.com>,
Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>,
Andi Shyti <andi.shyti@kernel.org>,
Sasha Levin <sashal@kernel.org>,
avifishman70@gmail.com, tmaimon77@gmail.com,
openbmc@lists.ozlabs.org, linux-i2c@vger.kernel.org
Subject: [PATCH AUTOSEL 5.10 07/27] i2c: npcm: Add clock toggle recovery
Date: Tue, 3 Jun 2025 21:06:00 -0400 [thread overview]
Message-ID: <20250604010620.6819-7-sashal@kernel.org> (raw)
In-Reply-To: <20250604010620.6819-1-sashal@kernel.org>
From: Tali Perry <tali.perry1@gmail.com>
[ Upstream commit 38010591a0fc3203f1cee45b01ab358b72dd9ab2 ]
During init of the bus, the module checks that the bus is idle.
If one of the lines are stuck try to recover them first before failing.
Sometimes SDA and SCL are low if improper reset occurs (e.g., reboot).
Signed-off-by: Tali Perry <tali.perry1@gmail.com>
Signed-off-by: Mohammed Elbadry <mohammed.0.elbadry@gmail.com>
Reviewed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
Link: https://lore.kernel.org/r/20250328193252.1570811-1-mohammed.0.elbadry@gmail.com
Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
**YES** This commit should be backported to stable kernel trees. Here's
my detailed analysis: ## **Primary Reasons for Backporting:** ### **1.
Fixes a Real Hardware Bug** The commit addresses a legitimate hardware
initialization issue where the NPCM I2C controller can fail to
initialize if SDA or SCL lines are stuck low after system reset. This is
not a theoretical problem but a real-world scenario that occurs when: -
External I2C devices (sensors, EEPROMs) hold SDA low after improper
reset - Power cycling leaves slave devices in an inconsistent state -
BMC systems reboot while I2C transactions are in progress ### **2.
Small, Contained Fix** The code change is minimal and well-contained: -
**Before**: Hard failure with `dev_err()` and `return -ENXIO` when lines
are stuck - **After**: Attempts recovery first, only fails if recovery
doesn't work - Uses existing `npcm_i2c_recovery_tgclk()` function that's
already proven and in use for runtime recovery ### **3. Prevents System
Boot Failures** Without this fix, systems can fail to boot completely
when I2C controllers can't initialize due to stuck bus lines. The commit
message specifically mentions "Sometimes SDA and SCL are low if improper
reset occurs (e.g., reboot)" - this is a boot-critical issue. ### **4.
Conservative Error Handling** The fix uses defensive programming: -
First attempts recovery using hardware-specific TGCLK mechanism - Only
fails initialization if recovery is unsuccessful - Downgrades the
initial error from `dev_err` to `dev_warn` with recovery attempt -
Maintains the same failure path if recovery doesn't work ### **5.
Alignment with Similar Successful Backports** Looking at the reference
commits, this follows the pattern of similar commit #4 (npcm timeout
calculation fix) which was marked "YES" for backporting. Both: - Fix
NPCM I2C driver issues - Address real hardware problems - Make small,
targeted changes - Don't introduce new features ### **6. Hardware-
Specific, Low Risk** The change only affects the NPCM I2C controller
initialization path and uses existing recovery mechanisms. The risk of
regression is minimal since: - It only adds a recovery attempt before an
existing failure case - Uses proven recovery logic already in the driver
- Specific to Nuvoton BMC hardware ## **Code Analysis:** The key change
replaces immediate failure: ```c // OLD: Immediate failure
dev_err(bus->dev, "I2C%d init fail: lines are low\n", bus->num); return
-ENXIO; ``` With recovery attempt: ```c // NEW: Try recovery first
dev_warn(bus->dev, " I2C%d SDA=%d SCL=%d, attempting to recover\n",
...); if (npcm_i2c_recovery_tgclk(&bus->adap)) { dev_err(bus->dev,
"I2C%d init fail: SDA=%d SCL=%d\n", ...); return -ENXIO; } ``` This is a
textbook example of a good stable backport candidate: it fixes a real
bug that prevents system functionality, uses minimal changes, and has
low regression risk.
drivers/i2c/busses/i2c-npcm7xx.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index d97694ac29ca9..3f30c3cff7201 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -1950,10 +1950,14 @@ static int npcm_i2c_init_module(struct npcm_i2c *bus, enum i2c_mode mode,
/* check HW is OK: SDA and SCL should be high at this point. */
if ((npcm_i2c_get_SDA(&bus->adap) == 0) || (npcm_i2c_get_SCL(&bus->adap) == 0)) {
- dev_err(bus->dev, "I2C%d init fail: lines are low\n", bus->num);
- dev_err(bus->dev, "SDA=%d SCL=%d\n", npcm_i2c_get_SDA(&bus->adap),
- npcm_i2c_get_SCL(&bus->adap));
- return -ENXIO;
+ dev_warn(bus->dev, " I2C%d SDA=%d SCL=%d, attempting to recover\n", bus->num,
+ npcm_i2c_get_SDA(&bus->adap), npcm_i2c_get_SCL(&bus->adap));
+ if (npcm_i2c_recovery_tgclk(&bus->adap)) {
+ dev_err(bus->dev, "I2C%d init fail: SDA=%d SCL=%d\n",
+ bus->num, npcm_i2c_get_SDA(&bus->adap),
+ npcm_i2c_get_SCL(&bus->adap));
+ return -ENXIO;
+ }
}
npcm_i2c_int_enable(bus, true);
--
2.39.5
next prev parent reply other threads:[~2025-06-04 1:06 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-06-04 1:05 [PATCH AUTOSEL 5.10 01/27] net: macb: Check return value of dma_set_mask_and_coherent() Sasha Levin
2025-06-04 1:05 ` [PATCH AUTOSEL 5.10 02/27] tipc: use kfree_sensitive() for aead cleanup Sasha Levin
2025-06-04 1:05 ` [PATCH AUTOSEL 5.10 03/27] i2c: designware: Invoke runtime suspend on quick slave re-registration Sasha Levin
2025-06-04 1:05 ` [PATCH AUTOSEL 5.10 04/27] emulex/benet: correct command version selection in be_cmd_get_stats() Sasha Levin
2025-06-04 1:05 ` [PATCH AUTOSEL 5.10 05/27] wifi: mt76: mt76x2: Add support for LiteOn WN4516R,WN4519R Sasha Levin
2025-06-04 1:05 ` [PATCH AUTOSEL 5.10 06/27] sctp: Do not wake readers in __sctp_write_space() Sasha Levin
2025-06-04 1:06 ` Sasha Levin [this message]
2025-06-04 1:06 ` [PATCH AUTOSEL 5.10 08/27] net: dlink: add synchronization for stats update Sasha Levin
2025-06-04 1:06 ` [PATCH AUTOSEL 5.10 09/27] tcp: always seek for minimal rtt in tcp_rcv_rtt_update() Sasha Levin
2025-06-04 1:06 ` [PATCH AUTOSEL 5.10 10/27] tcp: fix initial tp->rcvq_space.space value for passive TS enabled flows Sasha Levin
2025-06-04 1:06 ` [PATCH AUTOSEL 5.10 11/27] ipv4/route: Use this_cpu_inc() for stats on PREEMPT_RT Sasha Levin
2025-06-04 1:06 ` [PATCH AUTOSEL 5.10 12/27] openvswitch: Stricter validation for the userspace action Sasha Levin
2025-06-04 1:06 ` [PATCH AUTOSEL 5.10 13/27] net: atlantic: generate software timestamp just before the doorbell Sasha Levin
2025-06-04 1:06 ` [PATCH AUTOSEL 5.10 14/27] pinctrl: armada-37xx: propagate error from armada_37xx_pmx_set_by_name() Sasha Levin
2025-06-04 1:06 ` [PATCH AUTOSEL 5.10 15/27] pinctrl: armada-37xx: propagate error from armada_37xx_gpio_get_direction() Sasha Levin
2025-06-04 1:06 ` [PATCH AUTOSEL 5.10 16/27] pinctrl: armada-37xx: propagate error from armada_37xx_pmx_gpio_set_direction() Sasha Levin
2025-06-04 1:06 ` [PATCH AUTOSEL 5.10 17/27] pinctrl: armada-37xx: propagate error from armada_37xx_gpio_get() Sasha Levin
2025-06-04 1:06 ` [PATCH AUTOSEL 5.10 18/27] net: mlx4: add SOF_TIMESTAMPING_TX_SOFTWARE flag when getting ts info Sasha Levin
2025-06-04 1:06 ` [PATCH AUTOSEL 5.10 19/27] wifi: mac80211: do not offer a mesh path if forwarding is disabled Sasha Levin
2025-06-04 1:06 ` [PATCH AUTOSEL 5.10 20/27] clk: rockchip: rk3036: mark ddrphy as critical Sasha Levin
2025-06-04 1:06 ` [PATCH AUTOSEL 5.10 21/27] scsi: lpfc: Fix lpfc_check_sli_ndlp() handling for GEN_REQUEST64 commands Sasha Levin
2025-06-04 1:06 ` [PATCH AUTOSEL 5.10 22/27] iommu/amd: Ensure GA log notifier callbacks finish running before module unload Sasha Levin
2025-06-04 1:06 ` [PATCH AUTOSEL 5.10 23/27] vxlan: Do not treat dst cache initialization errors as fatal Sasha Levin
2025-06-04 1:06 ` [PATCH AUTOSEL 5.10 24/27] software node: Correct a OOB check in software_node_get_reference_args() Sasha Levin
2025-06-04 1:06 ` [PATCH AUTOSEL 5.10 25/27] scsi: lpfc: Use memcpy() for BIOS version Sasha Levin
2025-06-04 1:06 ` [PATCH AUTOSEL 5.10 26/27] sock: Correct error checking condition for (assign|release)_proto_idx() Sasha Levin
2025-06-04 1:06 ` [PATCH AUTOSEL 5.10 27/27] i40e: fix MMIO write access to an invalid page in i40e_clear_hw Sasha Levin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250604010620.6819-7-sashal@kernel.org \
--to=sashal@kernel.org \
--cc=andi.shyti@kernel.org \
--cc=avifishman70@gmail.com \
--cc=linux-i2c@vger.kernel.org \
--cc=mohammed.0.elbadry@gmail.com \
--cc=openbmc@lists.ozlabs.org \
--cc=patches@lists.linux.dev \
--cc=quic_msavaliy@quicinc.com \
--cc=stable@vger.kernel.org \
--cc=tali.perry1@gmail.com \
--cc=tmaimon77@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).