Hi,
This series adds two tangent features to the Nomadik I2C controller:
- Add a new compatible to support Mobileye EyeQ5 which uses the same IP
block as Nomadik.
It has two quirks to be handled:
- The memory bus only supports 32-bit accesses. Avoid readb() and
writeb() calls that might generate byte load/store instructions.
- We must write a value into a shared register region (OLB)
depending on the I2C bus speed.
- Allow xfer timeouts below one jiffy by using a waitqueue and hrtimers
instead of a completion.
The situation to be addressed is:
- Many devices on the same I2C bus.
- One xfer to each device is sent at regular interval.
- One device gets stuck and does not answer.
- With long timeouts, following devices won't get their message. A
shorter timeout ensures we can still talk to the following
devices.
This clashes a bit with the current i2c_adapter timeout field that
stores a jiffies amount. We therefore avoid it and store the value
in a private struct field, as a µs amount. If the timeout is less
than a jiffy duration, we switch from standard jiffies timeout to
hrtimers.
There is one patch targeting a hwmon dt-bindings file:
Documentation/devicetree/bindings/hwmon/lm75.yaml. The rest is touching
the I2C bus driver, its bindings and platform devicetrees.
About dependencies:
- The series is based upon v6.8-rc6.
- For testing on EyeQ5 hardware and devicetree patches, we need the
base platform series from Grégory [0] and its dependency [1]. Both
in mips-next [2].
- Devicetree commits require the EyeQ5 syscon series [3] that provides
the reset controller node.
- The LM75 dt-bindings patch depends on the common schema
hwmon-common.yaml series from Krzysztof [4]. Found in hwmon-next [5].
Have a nice day,
Théo Lebrun
[0]: https://lore.kernel.org/lkml/[email protected]/
[1]: https://lore.kernel.org/linux-mips/[email protected]/
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/mips/linux.git/log/
[3]: https://lore.kernel.org/lkml/[email protected]/
[4]: https://lore.kernel.org/lkml/[email protected]/
[5]: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git/log/?h=hwmon-next
To: Linus Walleij <[email protected]>
To: Andi Shyti <[email protected]>
To: Rob Herring <[email protected]>
To: Krzysztof Kozlowski <[email protected]>
To: Conor Dooley <[email protected]>
To: Thomas Bogendoerfer <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: Gregory Clement <[email protected]>
Cc: Vladimir Kondratiev <[email protected]>
Cc: Thomas Petazzoni <[email protected]>
Cc: Tawfik Bayouk <[email protected]>
Signed-off-by: Théo Lebrun <[email protected]>
Changes in v2:
- dt-bindings: i2c: st,nomadic-i2c:
- Drop timeout-usecs property, rely on generic i2c-transfer-timeout-us.
- Use phandle to syscon with cell args; remove mobileye,id prop; move
mobileye,olb from if-statement to top-level.
- dt-bindings: hwmon: lm75:
- Inherit from hwmon-common.yaml rather than declare generic label property.
- i2c: nomadik: (ie driver code)
- Parse i2c-transfer-timeout-us rather than custom timeout-usecs property.
- Introduce readb/writeb helpers with fallback to readl/writel.
- Avoid readb() on Mobileye.
- Use mobileye,olb cell args to get controller index rather than mobileye,id.
- Take 5 Reviewed-by Linus Walleij.
- MIPS: mobileye: (ie devicetrees)
- Use mobileye,olb with cell args rather than mobileye,id.
- Squash reset commit.
- Add i2c-transfer-timeout-us value of 10ms to all controllers.
- Rename LM75 instance from tmp112@48 to temperature-sensor@48.
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Théo Lebrun (11):
dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example
dt-bindings: hwmon: lm75: use common hwmon schema
i2c: nomadik: rename private struct pointers from dev to priv
i2c: nomadik: simplify IRQ masking logic
i2c: nomadik: use bitops helpers
i2c: nomadik: support short xfer timeouts using waitqueue & hrtimer
i2c: nomadik: replace jiffies by ktime for FIFO flushing timeout
i2c: nomadik: fetch i2c-transfer-timeout-us property from devicetree
i2c: nomadik: support Mobileye EyeQ5 I2C controller
MIPS: mobileye: eyeq5: add 5 I2C controller nodes
MIPS: mobileye: eyeq5: add evaluation board I2C temp sensor
Documentation/devicetree/bindings/hwmon/lm75.yaml | 3 +-
.../devicetree/bindings/i2c/st,nomadik-i2c.yaml | 48 +-
arch/mips/boot/dts/mobileye/eyeq5-epm5.dts | 8 +
arch/mips/boot/dts/mobileye/eyeq5.dtsi | 75 +++
drivers/i2c/busses/i2c-nomadik.c | 720 ++++++++++++---------
5 files changed, 541 insertions(+), 313 deletions(-)
---
base-commit: a6cc37d1a531e1c99e7989001a0529b443397900
change-id: 20231023-mbly-i2c-7c2fbbb1299f
Best regards,
--
Théo Lebrun <[email protected]>
Constant register bit fields are declared using hardcoded hex values;
replace them by calls to BIT() and GENMASK(). Replace custom GEN_MASK()
macro by the generic FIELD_PREP(). Replace manual bit manipulations by
the generic FIELD_GET() macro.
Reviewed-by: Linus Walleij <[email protected]>
Signed-off-by: Théo Lebrun <[email protected]>
---
drivers/i2c/busses/i2c-nomadik.c | 150 ++++++++++++++++++++-------------------
1 file changed, 77 insertions(+), 73 deletions(-)
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 80bdf7e42613..aa68ab402b10 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -9,6 +9,7 @@
* Author: Srinidhi Kasagar <[email protected]>
* Author: Sachin Verma <[email protected]>
*/
+#include <linux/bitfield.h>
#include <linux/init.h>
#include <linux/module.h>
#include <linux/amba/bus.h>
@@ -42,54 +43,59 @@
#define I2C_ICR (0x038)
/* Control registers */
-#define I2C_CR_PE (0x1 << 0) /* Peripheral Enable */
-#define I2C_CR_OM (0x3 << 1) /* Operating mode */
-#define I2C_CR_SAM (0x1 << 3) /* Slave addressing mode */
-#define I2C_CR_SM (0x3 << 4) /* Speed mode */
-#define I2C_CR_SGCM (0x1 << 6) /* Slave general call mode */
-#define I2C_CR_FTX (0x1 << 7) /* Flush Transmit */
-#define I2C_CR_FRX (0x1 << 8) /* Flush Receive */
-#define I2C_CR_DMA_TX_EN (0x1 << 9) /* DMA Tx enable */
-#define I2C_CR_DMA_RX_EN (0x1 << 10) /* DMA Rx Enable */
-#define I2C_CR_DMA_SLE (0x1 << 11) /* DMA sync. logic enable */
-#define I2C_CR_LM (0x1 << 12) /* Loopback mode */
-#define I2C_CR_FON (0x3 << 13) /* Filtering on */
-#define I2C_CR_FS (0x3 << 15) /* Force stop enable */
+#define I2C_CR_PE BIT(0) /* Peripheral Enable */
+#define I2C_CR_OM GENMASK(2, 1) /* Operating mode */
+#define I2C_CR_SAM BIT(3) /* Slave addressing mode */
+#define I2C_CR_SM GENMASK(5, 4) /* Speed mode */
+#define I2C_CR_SGCM BIT(6) /* Slave general call mode */
+#define I2C_CR_FTX BIT(7) /* Flush Transmit */
+#define I2C_CR_FRX BIT(8) /* Flush Receive */
+#define I2C_CR_DMA_TX_EN BIT(9) /* DMA Tx enable */
+#define I2C_CR_DMA_RX_EN BIT(10) /* DMA Rx Enable */
+#define I2C_CR_DMA_SLE BIT(11) /* DMA sync. logic enable */
+#define I2C_CR_LM BIT(12) /* Loopback mode */
+#define I2C_CR_FON GENMASK(14, 13) /* Filtering on */
+#define I2C_CR_FS GENMASK(16, 15) /* Force stop enable */
+
+/* Slave control register (SCR) */
+#define I2C_SCR_SLSU GENMASK(31, 16) /* Slave data setup time */
/* Master controller (MCR) register */
-#define I2C_MCR_OP (0x1 << 0) /* Operation */
-#define I2C_MCR_A7 (0x7f << 1) /* 7-bit address */
-#define I2C_MCR_EA10 (0x7 << 8) /* 10-bit Extended address */
-#define I2C_MCR_SB (0x1 << 11) /* Extended address */
-#define I2C_MCR_AM (0x3 << 12) /* Address type */
-#define I2C_MCR_STOP (0x1 << 14) /* Stop condition */
-#define I2C_MCR_LENGTH (0x7ff << 15) /* Transaction length */
+#define I2C_MCR_OP BIT(0) /* Operation */
+#define I2C_MCR_A7 GENMASK(7, 1) /* 7-bit address */
+#define I2C_MCR_EA10 GENMASK(10, 8) /* 10-bit Extended address */
+#define I2C_MCR_SB BIT(11) /* Extended address */
+#define I2C_MCR_AM GENMASK(13, 12) /* Address type */
+#define I2C_MCR_STOP BIT(14) /* Stop condition */
+#define I2C_MCR_LENGTH GENMASK(25, 15) /* Transaction length */
/* Status register (SR) */
-#define I2C_SR_OP (0x3 << 0) /* Operation */
-#define I2C_SR_STATUS (0x3 << 2) /* controller status */
-#define I2C_SR_CAUSE (0x7 << 4) /* Abort cause */
-#define I2C_SR_TYPE (0x3 << 7) /* Receive type */
-#define I2C_SR_LENGTH (0x7ff << 9) /* Transfer length */
+#define I2C_SR_OP GENMASK(1, 0) /* Operation */
+#define I2C_SR_STATUS GENMASK(3, 2) /* controller status */
+#define I2C_SR_CAUSE GENMASK(6, 4) /* Abort cause */
+#define I2C_SR_TYPE GENMASK(8, 7) /* Receive type */
+#define I2C_SR_LENGTH GENMASK(19, 9) /* Transfer length */
+
+/* Baud-rate counter register (BRCR) */
+#define I2C_BRCR_BRCNT1 GENMASK(31, 16) /* Baud-rate counter 1 */
+#define I2C_BRCR_BRCNT2 GENMASK(15, 0) /* Baud-rate counter 2 */
/* Interrupt mask set/clear (IMSCR) bits */
-#define I2C_IT_TXFE (0x1 << 0)
-#define I2C_IT_TXFNE (0x1 << 1)
-#define I2C_IT_TXFF (0x1 << 2)
-#define I2C_IT_TXFOVR (0x1 << 3)
-#define I2C_IT_RXFE (0x1 << 4)
-#define I2C_IT_RXFNF (0x1 << 5)
-#define I2C_IT_RXFF (0x1 << 6)
-#define I2C_IT_RFSR (0x1 << 16)
-#define I2C_IT_RFSE (0x1 << 17)
-#define I2C_IT_WTSR (0x1 << 18)
-#define I2C_IT_MTD (0x1 << 19)
-#define I2C_IT_STD (0x1 << 20)
-#define I2C_IT_MAL (0x1 << 24)
-#define I2C_IT_BERR (0x1 << 25)
-#define I2C_IT_MTDWS (0x1 << 28)
-
-#define GEN_MASK(val, mask, sb) (((val) << (sb)) & (mask))
+#define I2C_IT_TXFE BIT(0)
+#define I2C_IT_TXFNE BIT(1)
+#define I2C_IT_TXFF BIT(2)
+#define I2C_IT_TXFOVR BIT(3)
+#define I2C_IT_RXFE BIT(4)
+#define I2C_IT_RXFNF BIT(5)
+#define I2C_IT_RXFF BIT(6)
+#define I2C_IT_RFSR BIT(16)
+#define I2C_IT_RFSE BIT(17)
+#define I2C_IT_WTSR BIT(18)
+#define I2C_IT_MTD BIT(19)
+#define I2C_IT_STD BIT(20)
+#define I2C_IT_MAL BIT(24)
+#define I2C_IT_BERR BIT(25)
+#define I2C_IT_MTDWS BIT(28)
/* some bits in ICR are reserved */
#define I2C_CLEAR_ALL_INTS 0x131f007f
@@ -284,7 +290,7 @@ static int init_hw(struct nmk_i2c_dev *priv)
}
/* enable peripheral, master mode operation */
-#define DEFAULT_I2C_REG_CR ((1 << 1) | I2C_CR_PE)
+#define DEFAULT_I2C_REG_CR (FIELD_PREP(I2C_CR_OM, 0b01) | I2C_CR_PE)
/**
* load_i2c_mcr_reg() - load the MCR register
@@ -296,41 +302,42 @@ static u32 load_i2c_mcr_reg(struct nmk_i2c_dev *priv, u16 flags)
u32 mcr = 0;
unsigned short slave_adr_3msb_bits;
- mcr |= GEN_MASK(priv->cli.slave_adr, I2C_MCR_A7, 1);
+ mcr |= FIELD_PREP(I2C_MCR_A7, priv->cli.slave_adr);
if (unlikely(flags & I2C_M_TEN)) {
/* 10-bit address transaction */
- mcr |= GEN_MASK(2, I2C_MCR_AM, 12);
+ mcr |= FIELD_PREP(I2C_MCR_AM, 2);
/*
* Get the top 3 bits.
* EA10 represents extended address in MCR. This includes
* the extension (MSB bits) of the 7 bit address loaded
* in A7
*/
- slave_adr_3msb_bits = (priv->cli.slave_adr >> 7) & 0x7;
+ slave_adr_3msb_bits = FIELD_GET(GENMASK(9, 7),
+ priv->cli.slave_adr);
- mcr |= GEN_MASK(slave_adr_3msb_bits, I2C_MCR_EA10, 8);
+ mcr |= FIELD_PREP(I2C_MCR_EA10, slave_adr_3msb_bits);
} else {
/* 7-bit address transaction */
- mcr |= GEN_MASK(1, I2C_MCR_AM, 12);
+ mcr |= FIELD_PREP(I2C_MCR_AM, 1);
}
/* start byte procedure not applied */
- mcr |= GEN_MASK(0, I2C_MCR_SB, 11);
+ mcr |= FIELD_PREP(I2C_MCR_SB, 0);
/* check the operation, master read/write? */
if (priv->cli.operation == I2C_WRITE)
- mcr |= GEN_MASK(I2C_WRITE, I2C_MCR_OP, 0);
+ mcr |= FIELD_PREP(I2C_MCR_OP, I2C_WRITE);
else
- mcr |= GEN_MASK(I2C_READ, I2C_MCR_OP, 0);
+ mcr |= FIELD_PREP(I2C_MCR_OP, I2C_READ);
/* stop or repeated start? */
if (priv->stop)
- mcr |= GEN_MASK(1, I2C_MCR_STOP, 14);
+ mcr |= FIELD_PREP(I2C_MCR_STOP, 1);
else
- mcr &= ~(GEN_MASK(1, I2C_MCR_STOP, 14));
+ mcr &= ~FIELD_PREP(I2C_MCR_STOP, 1);
- mcr |= GEN_MASK(priv->cli.count, I2C_MCR_LENGTH, 15);
+ mcr |= FIELD_PREP(I2C_MCR_LENGTH, priv->cli.count);
return mcr;
}
@@ -383,7 +390,7 @@ static void setup_i2c_controller(struct nmk_i2c_dev *priv)
slsu += 1;
dev_dbg(&priv->adev->dev, "calculated SLSU = %04x\n", slsu);
- writel(slsu << 16, priv->virtbase + I2C_SCR);
+ writel(FIELD_PREP(I2C_SCR_SLSU, slsu), priv->virtbase + I2C_SCR);
/*
* The spec says, in case of std. mode the divider is
@@ -399,8 +406,8 @@ static void setup_i2c_controller(struct nmk_i2c_dev *priv)
* plus operation. Currently we do not supprt high speed mode
* so set brcr1 to 0.
*/
- brcr1 = 0 << 16;
- brcr2 = (i2c_clk / (priv->clk_freq * div)) & 0xffff;
+ brcr1 = FIELD_PREP(I2C_BRCR_BRCNT1, 0);
+ brcr2 = FIELD_PREP(I2C_BRCR_BRCNT2, i2c_clk / (priv->clk_freq * div));
/* set the baud rate counter register */
writel((brcr1 | brcr2), priv->virtbase + I2C_BRCR);
@@ -414,12 +421,13 @@ static void setup_i2c_controller(struct nmk_i2c_dev *priv)
if (priv->sm > I2C_FREQ_MODE_FAST) {
dev_err(&priv->adev->dev,
"do not support this mode defaulting to std. mode\n");
- brcr2 = i2c_clk / (I2C_MAX_STANDARD_MODE_FREQ * 2) & 0xffff;
+ brcr2 = FIELD_PREP(I2C_BRCR_BRCNT2,
+ i2c_clk / (I2C_MAX_STANDARD_MODE_FREQ * 2));
writel((brcr1 | brcr2), priv->virtbase + I2C_BRCR);
- writel(I2C_FREQ_MODE_STANDARD << 4,
- priv->virtbase + I2C_CR);
+ writel(FIELD_PREP(I2C_CR_SM, I2C_FREQ_MODE_STANDARD),
+ priv->virtbase + I2C_CR);
}
- writel(priv->sm << 4, priv->virtbase + I2C_CR);
+ writel(FIELD_PREP(I2C_CR_SM, priv->sm), priv->virtbase + I2C_CR);
/* set the Tx and Rx FIFO threshold */
writel(priv->tft, priv->virtbase + I2C_TFTR);
@@ -583,13 +591,8 @@ static int nmk_i2c_xfer_one(struct nmk_i2c_dev *priv, u16 flags)
u32 cause;
i2c_sr = readl(priv->virtbase + I2C_SR);
- /*
- * Check if the controller I2C operation status
- * is set to ABORT(11b).
- */
- if (((i2c_sr >> 2) & 0x3) == 0x3) {
- /* get the abort cause */
- cause = (i2c_sr >> 4) & 0x7;
+ if (FIELD_GET(I2C_SR_STATUS, i2c_sr) == I2C_ABORT) {
+ cause = FIELD_GET(I2C_SR_CAUSE, i2c_sr);
dev_err(&priv->adev->dev, "%s\n",
cause >= ARRAY_SIZE(abort_causes) ?
"unknown reason" :
@@ -730,7 +733,7 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
misr = readl(priv->virtbase + I2C_MISR);
src = __ffs(misr);
- switch ((1 << src)) {
+ switch (BIT(src)) {
/* Transmit FIFO nearly empty interrupt */
case I2C_IT_TXFNE:
@@ -824,15 +827,16 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
* during the transaction.
*/
case I2C_IT_BERR:
+ {
+ u32 sr = readl(priv->virtbase + I2C_SR);
priv->result = -EIO;
- /* get the status */
- if (((readl(priv->virtbase + I2C_SR) >> 2) & 0x3) == I2C_ABORT)
+ if (FIELD_GET(I2C_SR_STATUS, sr) == I2C_ABORT)
init_hw(priv);
i2c_set_bit(priv->virtbase + I2C_ICR, I2C_IT_BERR);
complete(&priv->xfer_complete);
-
- break;
+ }
+ break;
/*
* Tx FIFO overrun interrupt.
--
2.44.0
Replace the completion by a waitqueue for synchronization from IRQ
handler to task. For short timeouts, use hrtimers, else use timers.
Usecase: avoid blocking the I2C bus for too long when an issue occurs.
The threshold picked is one jiffy: if timeout is below that, use
hrtimers. This threshold is NOT configurable.
Implement behavior but do NOT change fetching of timeout. This means the
timeout is unchanged (200ms) and the hrtimer case will never trigger.
A waitqueue is used because it supports both desired timeout approaches.
See wait_event_timeout() and wait_event_hrtimeout(). An atomic boolean
serves as synchronization condition.
Reviewed-by: Linus Walleij <[email protected]>
Signed-off-by: Théo Lebrun <[email protected]>
---
drivers/i2c/busses/i2c-nomadik.c | 70 +++++++++++++++++++++++++++-------------
1 file changed, 48 insertions(+), 22 deletions(-)
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index aa68ab402b10..e68b8e0d7919 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -162,10 +162,11 @@ struct i2c_nmk_client {
* @clk_freq: clock frequency for the operation mode
* @tft: Tx FIFO Threshold in bytes
* @rft: Rx FIFO Threshold in bytes
- * @timeout: Slave response timeout (ms)
+ * @timeout_usecs: Slave response timeout
* @sm: speed mode
* @stop: stop condition.
- * @xfer_complete: acknowledge completion for a I2C message.
+ * @xfer_wq: xfer done wait queue.
+ * @xfer_done: xfer done boolean.
* @result: controller propogated result.
*/
struct nmk_i2c_dev {
@@ -179,10 +180,11 @@ struct nmk_i2c_dev {
u32 clk_freq;
unsigned char tft;
unsigned char rft;
- int timeout;
+ int timeout_usecs;
enum i2c_freq_mode sm;
int stop;
- struct completion xfer_complete;
+ struct wait_queue_head xfer_wq;
+ bool xfer_done;
int result;
};
@@ -434,6 +436,22 @@ static void setup_i2c_controller(struct nmk_i2c_dev *priv)
writel(priv->rft, priv->virtbase + I2C_RFTR);
}
+static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv)
+{
+ if (priv->timeout_usecs < jiffies_to_usecs(1)) {
+ unsigned long timeout_usecs = priv->timeout_usecs;
+ ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC);
+
+ wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done, timeout);
+ } else {
+ unsigned long timeout = usecs_to_jiffies(priv->timeout_usecs);
+
+ wait_event_timeout(priv->xfer_wq, priv->xfer_done, timeout);
+ }
+
+ return priv->xfer_done;
+}
+
/**
* read_i2c() - Read from I2C client device
* @priv: private data of I2C Driver
@@ -445,9 +463,9 @@ static void setup_i2c_controller(struct nmk_i2c_dev *priv)
*/
static int read_i2c(struct nmk_i2c_dev *priv, u16 flags)
{
- int status = 0;
u32 mcr, irq_mask;
- unsigned long timeout;
+ int status = 0;
+ bool xfer_done;
mcr = load_i2c_mcr_reg(priv, flags);
writel(mcr, priv->virtbase + I2C_MCR);
@@ -459,7 +477,8 @@ static int read_i2c(struct nmk_i2c_dev *priv, u16 flags)
/* enable the controller */
i2c_set_bit(priv->virtbase + I2C_CR, I2C_CR_PE);
- init_completion(&priv->xfer_complete);
+ init_waitqueue_head(&priv->xfer_wq);
+ priv->xfer_done = false;
/* enable interrupts by setting the mask */
irq_mask = (I2C_IT_RXFNF | I2C_IT_RXFF |
@@ -475,10 +494,9 @@ static int read_i2c(struct nmk_i2c_dev *priv, u16 flags)
writel(readl(priv->virtbase + I2C_IMSCR) | irq_mask,
priv->virtbase + I2C_IMSCR);
- timeout = wait_for_completion_timeout(
- &priv->xfer_complete, priv->adap.timeout);
+ xfer_done = nmk_i2c_wait_xfer_done(priv);
- if (timeout == 0) {
+ if (!xfer_done) {
/* Controller timed out */
dev_err(&priv->adev->dev, "read from slave 0x%x timed out\n",
priv->cli.slave_adr);
@@ -513,9 +531,9 @@ static void fill_tx_fifo(struct nmk_i2c_dev *priv, int no_bytes)
*/
static int write_i2c(struct nmk_i2c_dev *priv, u16 flags)
{
- u32 status = 0;
u32 mcr, irq_mask;
- unsigned long timeout;
+ u32 status = 0;
+ bool xfer_done;
mcr = load_i2c_mcr_reg(priv, flags);
@@ -528,7 +546,8 @@ static int write_i2c(struct nmk_i2c_dev *priv, u16 flags)
/* enable the controller */
i2c_set_bit(priv->virtbase + I2C_CR, I2C_CR_PE);
- init_completion(&priv->xfer_complete);
+ init_waitqueue_head(&priv->xfer_wq);
+ priv->xfer_done = false;
/* enable interrupts by settings the masks */
irq_mask = (I2C_IT_TXFOVR | I2C_IT_MAL | I2C_IT_BERR);
@@ -554,10 +573,9 @@ static int write_i2c(struct nmk_i2c_dev *priv, u16 flags)
writel(readl(priv->virtbase + I2C_IMSCR) | irq_mask,
priv->virtbase + I2C_IMSCR);
- timeout = wait_for_completion_timeout(
- &priv->xfer_complete, priv->adap.timeout);
+ xfer_done = nmk_i2c_wait_xfer_done(priv);
- if (timeout == 0) {
+ if (!xfer_done) {
/* Controller timed out */
dev_err(&priv->adev->dev, "write to slave 0x%x timed out\n",
priv->cli.slave_adr);
@@ -807,7 +825,9 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
priv->cli.count);
init_hw(priv);
}
- complete(&priv->xfer_complete);
+ priv->xfer_done = true;
+ wake_up(&priv->xfer_wq);
+
break;
@@ -817,7 +837,9 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
init_hw(priv);
i2c_set_bit(priv->virtbase + I2C_ICR, I2C_IT_MAL);
- complete(&priv->xfer_complete);
+ priv->xfer_done = true;
+ wake_up(&priv->xfer_wq);
+
break;
@@ -834,7 +856,9 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
init_hw(priv);
i2c_set_bit(priv->virtbase + I2C_ICR, I2C_IT_BERR);
- complete(&priv->xfer_complete);
+ priv->xfer_done = true;
+ wake_up(&priv->xfer_wq);
+
}
break;
@@ -848,7 +872,9 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
init_hw(priv);
dev_err(dev, "Tx Fifo Over run\n");
- complete(&priv->xfer_complete);
+ priv->xfer_done = true;
+ wake_up(&priv->xfer_wq);
+
break;
@@ -949,7 +975,7 @@ static void nmk_i2c_of_probe(struct device_node *np,
priv->sm = I2C_FREQ_MODE_FAST;
priv->tft = 1; /* Tx FIFO threshold */
priv->rft = 8; /* Rx FIFO threshold */
- priv->timeout = 200; /* Slave response timeout(ms) */
+ priv->timeout_usecs = 200 * USEC_PER_MSEC; /* Slave response timeout */
}
static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
@@ -1009,7 +1035,7 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
adap->owner = THIS_MODULE;
adap->class = I2C_CLASS_DEPRECATED;
adap->algo = &nmk_i2c_algo;
- adap->timeout = msecs_to_jiffies(priv->timeout);
+ adap->timeout = usecs_to_jiffies(priv->timeout_usecs);
snprintf(adap->name, sizeof(adap->name),
"Nomadik I2C at %pR", &adev->res);
--
2.44.0
Add compatible for the integration of the same DB8500 IP block into the
Mobileye EyeQ5 platform. Two quirks are present:
- The memory bus only supports 32-bit accesses. Avoid writeb() and
readb() by introducing helper functions that fallback to writel()
and readl().
- A register must be configured for the I2C speed mode; it is located
in a shared register region called OLB. We access that memory region
using a syscon & regmap that gets passed as a phandle (mobileye,olb).
A two-bit enum per controller is written into the register; that
requires us to know the global index of the I2C controller (cell arg
to the mobileye,olb phandle).
We add #include <linux/mfd/syscon.h> and <linux/regmap.h> and sort
headers.
Signed-off-by: Théo Lebrun <[email protected]>
---
drivers/i2c/busses/i2c-nomadik.c | 95 +++++++++++++++++++++++++++++++++++-----
1 file changed, 84 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 2d3247979e45..e9a77377add4 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -6,22 +6,30 @@
* I2C master mode controller driver, used in Nomadik 8815
* and Ux500 platforms.
*
+ * The Mobileye EyeQ5 platform is also supported; it uses
+ * the same Ux500/DB8500 IP block with two quirks:
+ * - The memory bus only supports 32-bit accesses.
+ * - A register must be configured for the I2C speed mode;
+ * it is located in a shared register region called OLB.
+ *
* Author: Srinidhi Kasagar <[email protected]>
* Author: Sachin Verma <[email protected]>
*/
+#include <linux/amba/bus.h>
#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
#include <linux/init.h>
-#include <linux/module.h>
-#include <linux/amba/bus.h>
-#include <linux/slab.h>
#include <linux/interrupt.h>
-#include <linux/i2c.h>
-#include <linux/err.h>
-#include <linux/clk.h>
#include <linux/io.h>
-#include <linux/pm_runtime.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
#include <linux/of.h>
#include <linux/pinctrl/consumer.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
#define DRIVER_NAME "nmk-i2c"
@@ -110,6 +118,10 @@ enum i2c_freq_mode {
I2C_FREQ_MODE_FAST_PLUS, /* up to 1 Mb/s */
};
+/* Mobileye EyeQ5 offset into a shared register region (called OLB) */
+#define NMK_I2C_EYEQ5_OLB_IOCR2 0x0B8
+#define NMK_I2C_EYEQ5_OLB_IOCR2_SHIFT(id) (4 + 2 * (id))
+
/**
* struct i2c_vendor_data - per-vendor variations
* @has_mtdws: variant has the MTDWS bit
@@ -168,6 +180,7 @@ struct i2c_nmk_client {
* @xfer_wq: xfer done wait queue.
* @xfer_done: xfer done boolean.
* @result: controller propogated result.
+ * @has_32b_bus: controller is on a bus that only supports 32-bit accesses.
*/
struct nmk_i2c_dev {
struct i2c_vendor_data *vendor;
@@ -186,6 +199,7 @@ struct nmk_i2c_dev {
struct wait_queue_head xfer_wq;
bool xfer_done;
int result;
+ bool has_32b_bus;
};
/* controller's abort causes */
@@ -209,6 +223,24 @@ static inline void i2c_clr_bit(void __iomem *reg, u32 mask)
writel(readl(reg) & ~mask, reg);
}
+static inline u8 nmk_i2c_readb(const struct nmk_i2c_dev *priv,
+ unsigned long reg)
+{
+ if (priv->has_32b_bus)
+ return readl(priv->virtbase + reg);
+ else
+ return readb(priv->virtbase + reg);
+}
+
+static inline void nmk_i2c_writeb(const struct nmk_i2c_dev *priv, u32 val,
+ unsigned long reg)
+{
+ if (priv->has_32b_bus)
+ writel(val, priv->virtbase + reg);
+ else
+ writeb(val, priv->virtbase + reg);
+}
+
/**
* flush_i2c_fifo() - This function flushes the I2C FIFO
* @priv: private data of I2C Driver
@@ -514,7 +546,7 @@ static void fill_tx_fifo(struct nmk_i2c_dev *priv, int no_bytes)
(priv->cli.count != 0);
count--) {
/* write to the Tx FIFO */
- writeb(*priv->cli.buffer, priv->virtbase + I2C_TFR);
+ nmk_i2c_writeb(priv, *priv->cli.buffer, I2C_TFR);
priv->cli.buffer++;
priv->cli.count--;
priv->cli.xfer_bytes++;
@@ -783,7 +815,7 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
case I2C_IT_RXFNF:
for (count = rft; count > 0; count--) {
/* Read the Rx FIFO */
- *priv->cli.buffer = readb(priv->virtbase + I2C_RFR);
+ *priv->cli.buffer = nmk_i2c_readb(priv, I2C_RFR);
priv->cli.buffer++;
}
priv->cli.count -= rft;
@@ -793,7 +825,7 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
/* Rx FIFO full */
case I2C_IT_RXFF:
for (count = MAX_I2C_FIFO_THRESHOLD; count > 0; count--) {
- *priv->cli.buffer = readb(priv->virtbase + I2C_RFR);
+ *priv->cli.buffer = nmk_i2c_readb(priv, I2C_RFR);
priv->cli.buffer++;
}
priv->cli.count -= MAX_I2C_FIFO_THRESHOLD;
@@ -809,7 +841,7 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
if (priv->cli.count == 0)
break;
*priv->cli.buffer =
- readb(priv->virtbase + I2C_RFR);
+ nmk_i2c_readb(priv, I2C_RFR);
priv->cli.buffer++;
priv->cli.count--;
priv->cli.xfer_bytes++;
@@ -985,6 +1017,38 @@ static void nmk_i2c_of_probe(struct device_node *np,
priv->timeout_usecs = 200 * USEC_PER_MSEC;
}
+static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
+{
+ struct device *dev = &priv->adev->dev;
+ struct device_node *np = dev->of_node;
+ unsigned int shift, speed_mode;
+ struct regmap *olb;
+ unsigned int id;
+
+ priv->has_32b_bus = true;
+
+ olb = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &id);
+ if (IS_ERR_OR_NULL(olb)) {
+ if (!olb)
+ olb = ERR_PTR(-ENOENT);
+ return dev_err_probe(dev, PTR_ERR(olb),
+ "failed OLB lookup: %lu\n", PTR_ERR(olb));
+ }
+
+ if (priv->clk_freq <= 400000)
+ speed_mode = 0b00;
+ else if (priv->clk_freq <= 1000000)
+ speed_mode = 0b01;
+ else
+ speed_mode = 0b10;
+
+ shift = NMK_I2C_EYEQ5_OLB_IOCR2_SHIFT(id);
+ regmap_update_bits(olb, NMK_I2C_EYEQ5_OLB_IOCR2,
+ 0b11 << shift, speed_mode << shift);
+
+ return 0;
+}
+
static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
{
int ret = 0;
@@ -1001,8 +1065,17 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
priv->vendor = vendor;
priv->adev = adev;
+ priv->has_32b_bus = false;
nmk_i2c_of_probe(np, priv);
+ if (of_device_is_compatible(np, "mobileye,eyeq5-i2c")) {
+ ret = nmk_i2c_eyeq5_probe(priv);
+ if (ret) {
+ dev_info(dev, "%s: %d: %d\n", __func__, __LINE__, ret);
+ return ret;
+ }
+ }
+
if (priv->tft > max_fifo_threshold) {
dev_warn(dev, "requested TX FIFO threshold %u, adjusted down to %u\n",
priv->tft, max_fifo_threshold);
--
2.44.0
Add the SoC I2C controller nodes to the platform devicetree. Use a
default bus frequency of 400kHz. They are AMBA devices that are matched
on PeriphID.
Set transfer timeout to 10ms instead of Linux's default of 200ms.
Signed-off-by: Théo Lebrun <[email protected]>
---
arch/mips/boot/dts/mobileye/eyeq5.dtsi | 75 ++++++++++++++++++++++++++++++++++
1 file changed, 75 insertions(+)
diff --git a/arch/mips/boot/dts/mobileye/eyeq5.dtsi b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
index 8d4f65ec912d..540d55503f3b 100644
--- a/arch/mips/boot/dts/mobileye/eyeq5.dtsi
+++ b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
@@ -70,6 +70,81 @@ soc: soc {
ranges;
compatible = "simple-bus";
+ i2c0: i2c@300000 {
+ compatible = "mobileye,eyeq5-i2c", "arm,primecell";
+ reg = <0 0x300000 0x0 0x1000>;
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_SHARED 1 IRQ_TYPE_LEVEL_HIGH>;
+ clock-frequency = <400000>; /* Fast mode */
+ #address-cells = <1>;
+ #size-cells = <0>;
+ clocks = <&i2c_ser_clk>, <&i2c_clk>;
+ clock-names = "i2cclk", "apb_pclk";
+ resets = <&reset 0 13>;
+ i2c-transfer-timeout-us = <10000>;
+ mobileye,olb = <&olb 0>;
+ };
+
+ i2c1: i2c@400000 {
+ compatible = "mobileye,eyeq5-i2c", "arm,primecell";
+ reg = <0 0x400000 0x0 0x1000>;
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_SHARED 2 IRQ_TYPE_LEVEL_HIGH>;
+ clock-frequency = <400000>; /* Fast mode */
+ #address-cells = <1>;
+ #size-cells = <0>;
+ clocks = <&i2c_ser_clk>, <&i2c_clk>;
+ clock-names = "i2cclk", "apb_pclk";
+ resets = <&reset 0 14>;
+ i2c-transfer-timeout-us = <10000>;
+ mobileye,olb = <&olb 1>;
+ };
+
+ i2c2: i2c@500000 {
+ compatible = "mobileye,eyeq5-i2c", "arm,primecell";
+ reg = <0 0x500000 0x0 0x1000>;
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_SHARED 3 IRQ_TYPE_LEVEL_HIGH>;
+ clock-frequency = <400000>; /* Fast mode */
+ #address-cells = <1>;
+ #size-cells = <0>;
+ clocks = <&i2c_ser_clk>, <&i2c_clk>;
+ clock-names = "i2cclk", "apb_pclk";
+ resets = <&reset 0 15>;
+ i2c-transfer-timeout-us = <10000>;
+ mobileye,olb = <&olb 2>;
+ };
+
+ i2c3: i2c@600000 {
+ compatible = "mobileye,eyeq5-i2c", "arm,primecell";
+ reg = <0 0x600000 0x0 0x1000>;
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_SHARED 4 IRQ_TYPE_LEVEL_HIGH>;
+ clock-frequency = <400000>; /* Fast mode */
+ #address-cells = <1>;
+ #size-cells = <0>;
+ clocks = <&i2c_ser_clk>, <&i2c_clk>;
+ clock-names = "i2cclk", "apb_pclk";
+ resets = <&reset 0 16>;
+ i2c-transfer-timeout-us = <10000>;
+ mobileye,olb = <&olb 3>;
+ };
+
+ i2c4: i2c@700000 {
+ compatible = "mobileye,eyeq5-i2c", "arm,primecell";
+ reg = <0 0x700000 0x0 0x1000>;
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_SHARED 5 IRQ_TYPE_LEVEL_HIGH>;
+ clock-frequency = <400000>; /* Fast mode */
+ #address-cells = <1>;
+ #size-cells = <0>;
+ clocks = <&i2c_ser_clk>, <&i2c_clk>;
+ clock-names = "i2cclk", "apb_pclk";
+ resets = <&reset 0 17>;
+ i2c-transfer-timeout-us = <10000>;
+ mobileye,olb = <&olb 4>;
+ };
+
uart0: serial@800000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0 0x800000 0x0 0x1000>;
--
2.44.0
On Thu, Feb 29, 2024 at 7:10 PM Théo Lebrun <[email protected]> wrote:
> Add compatible for the integration of the same DB8500 IP block into the
> Mobileye EyeQ5 platform. Two quirks are present:
>
> - The memory bus only supports 32-bit accesses. Avoid writeb() and
> readb() by introducing helper functions that fallback to writel()
> and readl().
>
> - A register must be configured for the I2C speed mode; it is located
> in a shared register region called OLB. We access that memory region
> using a syscon & regmap that gets passed as a phandle (mobileye,olb).
>
> A two-bit enum per controller is written into the register; that
> requires us to know the global index of the I2C controller (cell arg
> to the mobileye,olb phandle).
>
> We add #include <linux/mfd/syscon.h> and <linux/regmap.h> and sort
> headers.
>
> Signed-off-by: Théo Lebrun <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Thu, Feb 29, 2024 at 7:11 PM Théo Lebrun <[email protected]> wrote:
> Add the SoC I2C controller nodes to the platform devicetree. Use a
> default bus frequency of 400kHz. They are AMBA devices that are matched
> on PeriphID.
>
> Set transfer timeout to 10ms instead of Linux's default of 200ms.
>
> Signed-off-by: Théo Lebrun <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
Hi Theo,
..
> @@ -284,7 +290,7 @@ static int init_hw(struct nmk_i2c_dev *priv)
> }
>
> /* enable peripheral, master mode operation */
> -#define DEFAULT_I2C_REG_CR ((1 << 1) | I2C_CR_PE)
> +#define DEFAULT_I2C_REG_CR (FIELD_PREP(I2C_CR_OM, 0b01) | I2C_CR_PE)
0b01?
> /**
> * load_i2c_mcr_reg() - load the MCR register
> @@ -296,41 +302,42 @@ static u32 load_i2c_mcr_reg(struct nmk_i2c_dev *priv, u16 flags)
> u32 mcr = 0;
> unsigned short slave_adr_3msb_bits;
>
> - mcr |= GEN_MASK(priv->cli.slave_adr, I2C_MCR_A7, 1);
> + mcr |= FIELD_PREP(I2C_MCR_A7, priv->cli.slave_adr);
>
> if (unlikely(flags & I2C_M_TEN)) {
> /* 10-bit address transaction */
> - mcr |= GEN_MASK(2, I2C_MCR_AM, 12);
> + mcr |= FIELD_PREP(I2C_MCR_AM, 2);
> /*
> * Get the top 3 bits.
> * EA10 represents extended address in MCR. This includes
> * the extension (MSB bits) of the 7 bit address loaded
> * in A7
> */
> - slave_adr_3msb_bits = (priv->cli.slave_adr >> 7) & 0x7;
> + slave_adr_3msb_bits = FIELD_GET(GENMASK(9, 7),
> + priv->cli.slave_adr);
This looks like the only one not having a define. Shall we give a
definition to GENMASK(9, 7)?
>
> - mcr |= GEN_MASK(slave_adr_3msb_bits, I2C_MCR_EA10, 8);
> + mcr |= FIELD_PREP(I2C_MCR_EA10, slave_adr_3msb_bits);
..
> @@ -824,15 +827,16 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
> * during the transaction.
> */
> case I2C_IT_BERR:
> + {
> + u32 sr = readl(priv->virtbase + I2C_SR);
please give a blank line after the variable definition.
Besides, I'd prefer the assignment, when it is a bit more
complex, in a different line, as well.
Rest looks OK, didn't see anything off.
Andi
On Thu, Feb 29, 2024 at 07:10:54PM +0100, Théo Lebrun wrote:
> Replace the completion by a waitqueue for synchronization from IRQ
> handler to task. For short timeouts, use hrtimers, else use timers.
> Usecase: avoid blocking the I2C bus for too long when an issue occurs.
>
> The threshold picked is one jiffy: if timeout is below that, use
> hrtimers. This threshold is NOT configurable.
>
> Implement behavior but do NOT change fetching of timeout. This means the
> timeout is unchanged (200ms) and the hrtimer case will never trigger.
>
> A waitqueue is used because it supports both desired timeout approaches.
> See wait_event_timeout() and wait_event_hrtimeout(). An atomic boolean
> serves as synchronization condition.
>
> Reviewed-by: Linus Walleij <[email protected]>
> Signed-off-by: Théo Lebrun <[email protected]>
Largely:
Reviewed-by: Wolfram Sang <[email protected]>
Nit:
> - int timeout;
> + int timeout_usecs;
I think 'unsigned' makes a lot of sense here. Maybe u32 even?
> + if (of_device_is_compatible(np, "mobileye,eyeq5-i2c")) {
> + ret = nmk_i2c_eyeq5_probe(priv);
> + if (ret) {
> + dev_info(dev, "%s: %d: %d\n", __func__, __LINE__, ret);
This is debug code, or? Please remove it. Especially since
nmk_i2c_eyeq5_probe() prints something out on error.
Hello,
On Sat Mar 2, 2024 at 2:31 AM CET, Andi Shyti wrote:
[...]
> > @@ -284,7 +290,7 @@ static int init_hw(struct nmk_i2c_dev *priv)
> > }
> >
> > /* enable peripheral, master mode operation */
> > -#define DEFAULT_I2C_REG_CR ((1 << 1) | I2C_CR_PE)
> > +#define DEFAULT_I2C_REG_CR (FIELD_PREP(I2C_CR_OM, 0b01) | I2C_CR_PE)
>
> 0b01?
OM is a two-bit field. We want to put 0b01 in that. We do not declare
constants for its 4 potential values. Values are:
- 0b00 slave mode
- 0b01 master mode
- 0b10 master/slave mode
- 0b11 reserved
To me the comment above DEFAULT_I2C_REG_CR is enough to show that we go
into master mode. We could declare all values as constants but only
0b01 would get used.
>
> > /**
> > * load_i2c_mcr_reg() - load the MCR register
> > @@ -296,41 +302,42 @@ static u32 load_i2c_mcr_reg(struct nmk_i2c_dev *priv, u16 flags)
> > u32 mcr = 0;
> > unsigned short slave_adr_3msb_bits;
> >
> > - mcr |= GEN_MASK(priv->cli.slave_adr, I2C_MCR_A7, 1);
> > + mcr |= FIELD_PREP(I2C_MCR_A7, priv->cli.slave_adr);
> >
> > if (unlikely(flags & I2C_M_TEN)) {
> > /* 10-bit address transaction */
> > - mcr |= GEN_MASK(2, I2C_MCR_AM, 12);
> > + mcr |= FIELD_PREP(I2C_MCR_AM, 2);
> > /*
> > * Get the top 3 bits.
> > * EA10 represents extended address in MCR. This includes
> > * the extension (MSB bits) of the 7 bit address loaded
> > * in A7
> > */
> > - slave_adr_3msb_bits = (priv->cli.slave_adr >> 7) & 0x7;
> > + slave_adr_3msb_bits = FIELD_GET(GENMASK(9, 7),
> > + priv->cli.slave_adr);
>
> This looks like the only one not having a define. Shall we give a
> definition to GENMASK(9, 7)?
Indeed. What should it be named? It could be generic; this is about
getting the upper 3 bits from an extended (10-bit) I2C address?
> > - mcr |= GEN_MASK(slave_adr_3msb_bits, I2C_MCR_EA10, 8);
> > + mcr |= FIELD_PREP(I2C_MCR_EA10, slave_adr_3msb_bits);
>
> ...
>
> > @@ -824,15 +827,16 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
> > * during the transaction.
> > */
> > case I2C_IT_BERR:
> > + {
> > + u32 sr = readl(priv->virtbase + I2C_SR);
>
> please give a blank line after the variable definition.
>
> Besides, I'd prefer the assignment, when it is a bit more
> complex, in a different line, as well.
Will do.
> Rest looks OK, didn't see anything off.
Thanks for the review Andi!
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hello,
On Mon Mar 4, 2024 at 10:18 AM CET, Wolfram Sang wrote:
> On Thu, Feb 29, 2024 at 07:10:54PM +0100, Théo Lebrun wrote:
> > Replace the completion by a waitqueue for synchronization from IRQ
> > handler to task. For short timeouts, use hrtimers, else use timers.
> > Usecase: avoid blocking the I2C bus for too long when an issue occurs.
> >
> > The threshold picked is one jiffy: if timeout is below that, use
> > hrtimers. This threshold is NOT configurable.
> >
> > Implement behavior but do NOT change fetching of timeout. This means the
> > timeout is unchanged (200ms) and the hrtimer case will never trigger.
> >
> > A waitqueue is used because it supports both desired timeout approaches.
> > See wait_event_timeout() and wait_event_hrtimeout(). An atomic boolean
> > serves as synchronization condition.
> >
> > Reviewed-by: Linus Walleij <[email protected]>
> > Signed-off-by: Théo Lebrun <[email protected]>
>
> Largely:
>
> Reviewed-by: Wolfram Sang <[email protected]>
Thanks for the reviews Wolfram.
> Nit:
>
> > - int timeout;
> > + int timeout_usecs;
>
> I think 'unsigned' makes a lot of sense here. Maybe u32 even?
Yes unsigned would make sense. unsigned int or u32, I wouldn't know
which to pick.
Regards,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hello,
On Mon Mar 4, 2024 at 10:27 AM CET, Wolfram Sang wrote:
>
> > + if (of_device_is_compatible(np, "mobileye,eyeq5-i2c")) {
> > + ret = nmk_i2c_eyeq5_probe(priv);
> > + if (ret) {
> > + dev_info(dev, "%s: %d: %d\n", __func__, __LINE__, ret);
>
> This is debug code, or? Please remove it. Especially since
> nmk_i2c_eyeq5_probe() prints something out on error.
It is. Nice catch, sorry about that.
Regards,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
> > > - int timeout;
> > > + int timeout_usecs;
> >
> > I think 'unsigned' makes a lot of sense here. Maybe u32 even?
>
> Yes unsigned would make sense. unsigned int or u32, I wouldn't know
> which to pick.
It gets (later) fed by of_property_read_u32(), so I tend to suggest u32.
Sounds like least conversions then but please double check.
Hi Theo,
..
> +static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv)
> +{
> + if (priv->timeout_usecs < jiffies_to_usecs(1)) {
> + unsigned long timeout_usecs = priv->timeout_usecs;
> + ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC);
> +
> + wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done, timeout);
> + } else {
> + unsigned long timeout = usecs_to_jiffies(priv->timeout_usecs);
> +
> + wait_event_timeout(priv->xfer_wq, priv->xfer_done, timeout);
> + }
> +
> + return priv->xfer_done;
You could eventually write this as
static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv)
{
if (priv->timeout_usecs < jiffies_to_usecs(1)) {
...
return !wait_event_hrtimeout(...);
}
...
return wait_event_timeout(...);
}
It looks a bit cleaner to me... your choice.
Rest looks good.
Reviewed-by: Andi Shyti <[email protected]>
Andi
Hi Theo,
..
> +#include <linux/amba/bus.h>
> #include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> #include <linux/init.h>
> -#include <linux/module.h>
> -#include <linux/amba/bus.h>
> -#include <linux/slab.h>
> #include <linux/interrupt.h>
> -#include <linux/i2c.h>
> -#include <linux/err.h>
> -#include <linux/clk.h>
> #include <linux/io.h>
> -#include <linux/pm_runtime.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> #include <linux/of.h>
> #include <linux/pinctrl/consumer.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
Please reorder the headers in a different patch.
> #define DRIVER_NAME "nmk-i2c"
>
..
> +static inline u8 nmk_i2c_readb(const struct nmk_i2c_dev *priv,
> + unsigned long reg)
> +{
> + if (priv->has_32b_bus)
> + return readl(priv->virtbase + reg);
> + else
> + return readb(priv->virtbase + reg);
nit: no need for the else (your choice though, if you want to
have ti coherent with the write counterpart).
> +}
> +
> +static inline void nmk_i2c_writeb(const struct nmk_i2c_dev *priv, u32 val,
> + unsigned long reg)
> +{
> + if (priv->has_32b_bus)
> + writel(val, priv->virtbase + reg);
> + else
> + writeb(val, priv->virtbase + reg);
> +}
..
> +static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
> +{
> + struct device *dev = &priv->adev->dev;
> + struct device_node *np = dev->of_node;
> + unsigned int shift, speed_mode;
> + struct regmap *olb;
> + unsigned int id;
> +
> + priv->has_32b_bus = true;
> +
> + olb = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &id);
> + if (IS_ERR_OR_NULL(olb)) {
> + if (!olb)
> + olb = ERR_PTR(-ENOENT);
> + return dev_err_probe(dev, PTR_ERR(olb),
> + "failed OLB lookup: %lu\n", PTR_ERR(olb));
just return PTR_ERR(olb) and use dev_err_probe() in the upper
layer probe.
> + }
> +
> + if (priv->clk_freq <= 400000)
> + speed_mode = 0b00;
> + else if (priv->clk_freq <= 1000000)
> + speed_mode = 0b01;
> + else
> + speed_mode = 0b10;
would be nice to have these as defines.
> +
> + shift = NMK_I2C_EYEQ5_OLB_IOCR2_SHIFT(id);
> + regmap_update_bits(olb, NMK_I2C_EYEQ5_OLB_IOCR2,
> + 0b11 << shift, speed_mode << shift);
please define these values and for hexadecimals use 0x...
> + return 0;
> +}
> +
> static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
> {
> int ret = 0;
> @@ -1001,8 +1065,17 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
>
> priv->vendor = vendor;
> priv->adev = adev;
> + priv->has_32b_bus = false;
> nmk_i2c_of_probe(np, priv);
>
> + if (of_device_is_compatible(np, "mobileye,eyeq5-i2c")) {
> + ret = nmk_i2c_eyeq5_probe(priv);
> + if (ret) {
> + dev_info(dev, "%s: %d: %d\n", __func__, __LINE__, ret);
> + return ret;
> + }
as said above, use dev_err_probe here.
Andi
> + }
> +
> if (priv->tft > max_fifo_threshold) {
> dev_warn(dev, "requested TX FIFO threshold %u, adjusted down to %u\n",
> priv->tft, max_fifo_threshold);
>
> --
> 2.44.0
>
Hello,
On Mon Mar 4, 2024 at 2:54 PM CET, Andi Shyti wrote:
> Hi Theo,
>
> ...
>
> > +static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv)
> > +{
> > + if (priv->timeout_usecs < jiffies_to_usecs(1)) {
> > + unsigned long timeout_usecs = priv->timeout_usecs;
> > + ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC);
> > +
> > + wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done, timeout);
> > + } else {
> > + unsigned long timeout = usecs_to_jiffies(priv->timeout_usecs);
> > +
> > + wait_event_timeout(priv->xfer_wq, priv->xfer_done, timeout);
> > + }
> > +
> > + return priv->xfer_done;
>
> You could eventually write this as
>
> static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv)
> {
> if (priv->timeout_usecs < jiffies_to_usecs(1)) {
> ...
>
> return !wait_event_hrtimeout(...);
> }
>
> ...
> return wait_event_timeout(...);
> }
>
> It looks a bit cleaner to me... your choice.
The full block would become:
static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv)
{
if (priv->timeout_usecs < jiffies_to_usecs(1)) {
unsigned long timeout_usecs = priv->timeout_usecs;
ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC);
return !wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done,
timeout);
}
return wait_event_timeout(priv->xfer_wq, priv->xfer_done,
usecs_to_jiffies(priv->timeout_usecs));
}
Three things:
- Deindenting the jiffy timeout case means no variable declaration
after the if-block. This is fine from my point-of-view.
- It means we depend on the half-mess that are return values from
wait_event_*timeout() macros. I wanted to avoid that because it
looks like an error when you read the above code and see one is
negated while the other is not.
- Also, I'm not confident in casting either return value to bool; what
happens if either macro returns an error? This is a theoretical case
that shouldn't happen, but behavior might change at some point or
bugs could occur. We know priv->xfer_done will give us the right
answer.
My preference still goes to the original version, but I'm happy we are
having a discussion about this code block.
> Reviewed-by: Andi Shyti <[email protected]>
Thanks for your review Andi!
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hi Theo,
On Mon, Mar 04, 2024 at 03:32:38PM +0100, Th?o Lebrun wrote:
> On Mon Mar 4, 2024 at 2:54 PM CET, Andi Shyti wrote:
> > > +static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv)
> > > +{
> > > + if (priv->timeout_usecs < jiffies_to_usecs(1)) {
> > > + unsigned long timeout_usecs = priv->timeout_usecs;
> > > + ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC);
> > > +
> > > + wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done, timeout);
> > > + } else {
> > > + unsigned long timeout = usecs_to_jiffies(priv->timeout_usecs);
> > > +
> > > + wait_event_timeout(priv->xfer_wq, priv->xfer_done, timeout);
> > > + }
> > > +
> > > + return priv->xfer_done;
> >
> > You could eventually write this as
> >
> > static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv)
> > {
> > if (priv->timeout_usecs < jiffies_to_usecs(1)) {
> > ...
> >
> > return !wait_event_hrtimeout(...);
> > }
> >
> > ...
> > return wait_event_timeout(...);
> > }
> >
> > It looks a bit cleaner to me... your choice.
>
> The full block would become:
>
> static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv)
> {
> if (priv->timeout_usecs < jiffies_to_usecs(1)) {
> unsigned long timeout_usecs = priv->timeout_usecs;
> ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC);
>
> return !wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done,
> timeout);
> }
>
> return wait_event_timeout(priv->xfer_wq, priv->xfer_done,
> usecs_to_jiffies(priv->timeout_usecs));
> }
>
> Three things:
>
> - Deindenting the jiffy timeout case means no variable declaration
> after the if-block. This is fine from my point-of-view.
>
> - It means we depend on the half-mess that are return values from
> wait_event_*timeout() macros. I wanted to avoid that because it
> looks like an error when you read the above code and see one is
> negated while the other is not.
>
> - Also, I'm not confident in casting either return value to bool; what
> happens if either macro returns an error? This is a theoretical case
> that shouldn't happen, but behavior might change at some point or
> bugs could occur. We know priv->xfer_done will give us the right
> answer.
>
> My preference still goes to the original version, but I'm happy we are
> having a discussion about this code block.
sure... it's not a binding comment.
Andi
Hello,
On Mon Mar 4, 2024 at 3:08 PM CET, Andi Shyti wrote:
> Hi Theo,
>
> ...
>
> > +#include <linux/amba/bus.h>
> > #include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > #include <linux/init.h>
> > -#include <linux/module.h>
> > -#include <linux/amba/bus.h>
> > -#include <linux/slab.h>
> > #include <linux/interrupt.h>
> > -#include <linux/i2c.h>
> > -#include <linux/err.h>
> > -#include <linux/clk.h>
> > #include <linux/io.h>
> > -#include <linux/pm_runtime.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/module.h>
> > #include <linux/of.h>
> > #include <linux/pinctrl/consumer.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
>
> Please reorder the headers in a different patch.
Will do.
>
> > #define DRIVER_NAME "nmk-i2c"
> >
>
> ...
>
> > +static inline u8 nmk_i2c_readb(const struct nmk_i2c_dev *priv,
> > + unsigned long reg)
> > +{
> > + if (priv->has_32b_bus)
> > + return readl(priv->virtbase + reg);
> > + else
> > + return readb(priv->virtbase + reg);
>
> nit: no need for the else (your choice though, if you want to
> have ti coherent with the write counterpart).
Indeed, the useless else block can be removed. Will do.
> > +}
> > +
> > +static inline void nmk_i2c_writeb(const struct nmk_i2c_dev *priv, u32 val,
> > + unsigned long reg)
> > +{
> > + if (priv->has_32b_bus)
> > + writel(val, priv->virtbase + reg);
> > + else
> > + writeb(val, priv->virtbase + reg);
> > +}
>
> ...
>
> > +static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
> > +{
> > + struct device *dev = &priv->adev->dev;
> > + struct device_node *np = dev->of_node;
> > + unsigned int shift, speed_mode;
> > + struct regmap *olb;
> > + unsigned int id;
> > +
> > + priv->has_32b_bus = true;
> > +
> > + olb = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &id);
> > + if (IS_ERR_OR_NULL(olb)) {
> > + if (!olb)
> > + olb = ERR_PTR(-ENOENT);
> > + return dev_err_probe(dev, PTR_ERR(olb),
> > + "failed OLB lookup: %lu\n", PTR_ERR(olb));
>
> just return PTR_ERR(olb) and use dev_err_probe() in the upper
> layer probe.
Good catch. In previous revisions nmk_i2c_eyeq5_probe() had multiple
error cases so it had to be the one doing the logging. Now that there
is a single possible error parent can do it. It should simplify code.
>
> > + }
> > +
> > + if (priv->clk_freq <= 400000)
> > + speed_mode = 0b00;
> > + else if (priv->clk_freq <= 1000000)
> > + speed_mode = 0b01;
> > + else
> > + speed_mode = 0b10;
>
> would be nice to have these as defines.
Agreed. Will be named based on I2C mode names, eg standard, fast, high
speed, fast plus.
>
> > +
> > + shift = NMK_I2C_EYEQ5_OLB_IOCR2_SHIFT(id);
> > + regmap_update_bits(olb, NMK_I2C_EYEQ5_OLB_IOCR2,
> > + 0b11 << shift, speed_mode << shift);
>
> please define these values and for hexadecimals use 0x...
0b11 is a two-bit mask. What do you mean by "these values"? Something
like:
#define NMK_I2C_EYEQ5_SPEED_MODE_FAST 0b00
#define NMK_I2C_EYEQ5_SPEED_MODE_FAST_PLUS 0b01
#define NMK_I2C_EYEQ5_SPEED_MODE_HIGH_SPEED 0b10
static const u8 nmk_i2c_eyeq5_masks[] = {
[0] = 0x0030,
[1] = 0x00C0,
[2] = 0x0300,
[3] = 0x0C00,
[4] = 0x3000,
};
static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
{
// ...
unsigned int id, mask, speed_mode;
priv->has_32b_bus = true;
olb = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &id);
// TODO: olb error checking
// TODO: check id is valid
if (priv->clk_freq <= 400000)
speed_mode = NMK_I2C_EYEQ5_SPEED_MODE_FAST;
else if (priv->clk_freq <= 1000000)
speed_mode = NMK_I2C_EYEQ5_SPEED_MODE_FAST_PLUS;
else
speed_mode = NMK_I2C_EYEQ5_SPEED_MODE_HIGH_SPEED;
mask = nmk_i2c_eyeq5_masks[id];
regmap_update_bits(olb, NMK_I2C_EYEQ5_OLB_IOCR2,
mask, speed_mode << __fls(mask));
return 0;
}
Else I do not see what you are suggesting by "please define these
values".
Have a nice day,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hi Theo,
> Th?o Lebrun (11):
> dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example
> dt-bindings: hwmon: lm75: use common hwmon schema
> i2c: nomadik: rename private struct pointers from dev to priv
> i2c: nomadik: simplify IRQ masking logic
> i2c: nomadik: use bitops helpers
> i2c: nomadik: support short xfer timeouts using waitqueue & hrtimer
> i2c: nomadik: replace jiffies by ktime for FIFO flushing timeout
> i2c: nomadik: fetch i2c-transfer-timeout-us property from devicetree
> i2c: nomadik: support Mobileye EyeQ5 I2C controller
> MIPS: mobileye: eyeq5: add 5 I2C controller nodes
> MIPS: mobileye: eyeq5: add evaluation board I2C temp sensor
what's your plan for this series? If you extract into a separate
series the refactoring patches that are not dependent on the
bindings I could queue them up for the merge window.
Andi
Hello Andi,
On Wed Mar 6, 2024 at 2:49 AM CET, Andi Shyti wrote:
> > Théo Lebrun (11):
> > dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example
> > dt-bindings: hwmon: lm75: use common hwmon schema
> > i2c: nomadik: rename private struct pointers from dev to priv
> > i2c: nomadik: simplify IRQ masking logic
> > i2c: nomadik: use bitops helpers
> > i2c: nomadik: support short xfer timeouts using waitqueue & hrtimer
> > i2c: nomadik: replace jiffies by ktime for FIFO flushing timeout
> > i2c: nomadik: fetch i2c-transfer-timeout-us property from devicetree
> > i2c: nomadik: support Mobileye EyeQ5 I2C controller
> > MIPS: mobileye: eyeq5: add 5 I2C controller nodes
> > MIPS: mobileye: eyeq5: add evaluation board I2C temp sensor
>
> what's your plan for this series? If you extract into a separate
> series the refactoring patches that are not dependent on the
> bindings I could queue them up for the merge window.
V3 is ready and will be sent today. I think we can get trailers from
dt-bindings maintainers as the discussion has been caried out on this
revision.
Am I being too optimistic of seeing this series queued before the merge
window?
Thanks Andi,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com