This series first perform enhancements in the way interrupt are handled
and cleanup in messages.
Then it adds support for the stm32mp25 which differs in that
it only has a single irq line for both event/error and has a
different handling of the FastModePlus.
Support is then enabled within the stm32mp25 related device-trees.
Changelog:
v2: - correct st,stm32-i2c.yaml. Use if then else scheme to indicate
number of interrupts / interrupt-names depending on the
compatible while keeping the description within the common part
- correct 2 maybe-uninitialized warnings
* ret in stm32f7_i2c_write_fm_plus_bits
* irq_error in stm32f7_i2c_probe, move the platform_get_irq
within the same if block as devm_request_threaded_irq
Alain Volmat (7):
i2c: stm32f7: perform most of irq job in threaded handler
i2c: stm32f7: simplify status messages in case of errors
dt-bindings: i2c: document st,stm32mp25-i2c compatible
i2c: stm32f7: add support for stm32mp25 soc
arm64: dts: st: add all 8 i2c nodes on stm32mp251
arm64: dts: st: add i2c2/i2c8 pins for stm32mp25
arm64: dts: st: add i2c2 / i2c8 properties on stm32mp257f-ev1
.../devicetree/bindings/i2c/st,stm32-i2c.yaml | 28 ++
arch/arm64/boot/dts/st/stm32mp25-pinctrl.dtsi | 36 ++
arch/arm64/boot/dts/st/stm32mp251.dtsi | 96 +++++
arch/arm64/boot/dts/st/stm32mp257f-ev1.dts | 20 ++
drivers/i2c/busses/i2c-stm32f7.c | 334 ++++++++++--------
5 files changed, 357 insertions(+), 157 deletions(-)
--
2.25.1
The irq handling is currently split between the irq handler
and the threaded irq handler. Some of the handling (such as
dma related stuffs) done within the irq handler might sleep or
take some time leading to issues if the kernel is built with
realtime constraints. In order to fix that, perform an overall
rework to perform most of the job within the threaded handler
and only keep fifo access in the non threaded handler.
Signed-off-by: Alain Volmat <[email protected]>
---
drivers/i2c/busses/i2c-stm32f7.c | 126 ++++++++++++++-----------------
1 file changed, 56 insertions(+), 70 deletions(-)
diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
index 983509936727..34dcc370e615 100644
--- a/drivers/i2c/busses/i2c-stm32f7.c
+++ b/drivers/i2c/busses/i2c-stm32f7.c
@@ -1497,17 +1497,11 @@ static irqreturn_t stm32f7_i2c_slave_isr_event(struct stm32f7_i2c_dev *i2c_dev)
static irqreturn_t stm32f7_i2c_isr_event(int irq, void *data)
{
struct stm32f7_i2c_dev *i2c_dev = data;
- struct stm32f7_i2c_msg *f7_msg = &i2c_dev->f7_msg;
- struct stm32_i2c_dma *dma = i2c_dev->dma;
- void __iomem *base = i2c_dev->base;
- u32 status, mask;
- int ret = IRQ_HANDLED;
+ u32 status;
- /* Check if the interrupt if for a slave device */
- if (!i2c_dev->master_mode) {
- ret = stm32f7_i2c_slave_isr_event(i2c_dev);
- return ret;
- }
+ /* Check if the interrupt is for a slave device */
+ if (!i2c_dev->master_mode)
+ return IRQ_WAKE_THREAD;
status = readl_relaxed(i2c_dev->base + STM32F7_I2C_ISR);
@@ -1519,6 +1513,29 @@ static irqreturn_t stm32f7_i2c_isr_event(int irq, void *data)
if (status & STM32F7_I2C_ISR_RXNE)
stm32f7_i2c_read_rx_data(i2c_dev);
+ /* Wake up the thread if other flags are raised */
+ if (status &
+ (STM32F7_I2C_ISR_NACKF | STM32F7_I2C_ISR_STOPF |
+ STM32F7_I2C_ISR_TC | STM32F7_I2C_ISR_TCR))
+ return IRQ_WAKE_THREAD;
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t stm32f7_i2c_isr_event_thread(int irq, void *data)
+{
+ struct stm32f7_i2c_dev *i2c_dev = data;
+ struct stm32f7_i2c_msg *f7_msg = &i2c_dev->f7_msg;
+ struct stm32_i2c_dma *dma = i2c_dev->dma;
+ void __iomem *base = i2c_dev->base;
+ u32 status, mask;
+ int ret;
+
+ if (!i2c_dev->master_mode)
+ return stm32f7_i2c_slave_isr_event(i2c_dev);
+
+ status = readl_relaxed(i2c_dev->base + STM32F7_I2C_ISR);
+
/* NACK received */
if (status & STM32F7_I2C_ISR_NACKF) {
dev_dbg(i2c_dev->dev, "<%s>: Receive NACK (addr %x)\n",
@@ -1531,33 +1548,28 @@ static irqreturn_t stm32f7_i2c_isr_event(int irq, void *data)
f7_msg->result = -ENXIO;
}
- /* STOP detection flag */
- if (status & STM32F7_I2C_ISR_STOPF) {
- /* Disable interrupts */
- if (stm32f7_i2c_is_slave_registered(i2c_dev))
- mask = STM32F7_I2C_XFER_IRQ_MASK;
+ if (status & STM32F7_I2C_ISR_TCR) {
+ if (f7_msg->smbus)
+ stm32f7_i2c_smbus_reload(i2c_dev);
else
- mask = STM32F7_I2C_ALL_IRQ_MASK;
- stm32f7_i2c_disable_irq(i2c_dev, mask);
-
- /* Clear STOP flag */
- writel_relaxed(STM32F7_I2C_ICR_STOPCF, base + STM32F7_I2C_ICR);
-
- if (i2c_dev->use_dma && !f7_msg->result) {
- ret = IRQ_WAKE_THREAD;
- } else {
- i2c_dev->master_mode = false;
- complete(&i2c_dev->complete);
- }
+ stm32f7_i2c_reload(i2c_dev);
}
/* Transfer complete */
if (status & STM32F7_I2C_ISR_TC) {
+ /* Wait for dma transfer completion before sending next message */
+ if (i2c_dev->use_dma && !f7_msg->result) {
+ ret = wait_for_completion_timeout(&i2c_dev->dma->dma_complete, HZ);
+ if (!ret) {
+ dev_dbg(i2c_dev->dev, "<%s>: Timed out\n", __func__);
+ stm32f7_i2c_disable_dma_req(i2c_dev);
+ dmaengine_terminate_async(dma->chan_using);
+ f7_msg->result = -ETIMEDOUT;
+ }
+ }
if (f7_msg->stop) {
mask = STM32F7_I2C_CR2_STOP;
stm32f7_i2c_set_bits(base + STM32F7_I2C_CR2, mask);
- } else if (i2c_dev->use_dma && !f7_msg->result) {
- ret = IRQ_WAKE_THREAD;
} else if (f7_msg->smbus) {
stm32f7_i2c_smbus_rep_start(i2c_dev);
} else {
@@ -1567,47 +1579,18 @@ static irqreturn_t stm32f7_i2c_isr_event(int irq, void *data)
}
}
- if (status & STM32F7_I2C_ISR_TCR) {
- if (f7_msg->smbus)
- stm32f7_i2c_smbus_reload(i2c_dev);
+ /* STOP detection flag */
+ if (status & STM32F7_I2C_ISR_STOPF) {
+ /* Disable interrupts */
+ if (stm32f7_i2c_is_slave_registered(i2c_dev))
+ mask = STM32F7_I2C_XFER_IRQ_MASK;
else
- stm32f7_i2c_reload(i2c_dev);
- }
-
- return ret;
-}
-
-static irqreturn_t stm32f7_i2c_isr_event_thread(int irq, void *data)
-{
- struct stm32f7_i2c_dev *i2c_dev = data;
- struct stm32f7_i2c_msg *f7_msg = &i2c_dev->f7_msg;
- struct stm32_i2c_dma *dma = i2c_dev->dma;
- u32 status;
- int ret;
-
- /*
- * Wait for dma transfer completion before sending next message or
- * notity the end of xfer to the client
- */
- ret = wait_for_completion_timeout(&i2c_dev->dma->dma_complete, HZ);
- if (!ret) {
- dev_dbg(i2c_dev->dev, "<%s>: Timed out\n", __func__);
- stm32f7_i2c_disable_dma_req(i2c_dev);
- dmaengine_terminate_async(dma->chan_using);
- f7_msg->result = -ETIMEDOUT;
- }
+ mask = STM32F7_I2C_ALL_IRQ_MASK;
+ stm32f7_i2c_disable_irq(i2c_dev, mask);
- status = readl_relaxed(i2c_dev->base + STM32F7_I2C_ISR);
+ /* Clear STOP flag */
+ writel_relaxed(STM32F7_I2C_ICR_STOPCF, base + STM32F7_I2C_ICR);
- if (status & STM32F7_I2C_ISR_TC) {
- if (f7_msg->smbus) {
- stm32f7_i2c_smbus_rep_start(i2c_dev);
- } else {
- i2c_dev->msg_id++;
- i2c_dev->msg++;
- stm32f7_i2c_xfer_msg(i2c_dev, i2c_dev->msg);
- }
- } else {
i2c_dev->master_mode = false;
complete(&i2c_dev->complete);
}
@@ -1615,7 +1598,7 @@ static irqreturn_t stm32f7_i2c_isr_event_thread(int irq, void *data)
return IRQ_HANDLED;
}
-static irqreturn_t stm32f7_i2c_isr_error(int irq, void *data)
+static irqreturn_t stm32f7_i2c_isr_error_thread(int irq, void *data)
{
struct stm32f7_i2c_dev *i2c_dev = data;
struct stm32f7_i2c_msg *f7_msg = &i2c_dev->f7_msg;
@@ -2205,8 +2188,11 @@ static int stm32f7_i2c_probe(struct platform_device *pdev)
return ret;
}
- ret = devm_request_irq(&pdev->dev, irq_error, stm32f7_i2c_isr_error, 0,
- pdev->name, i2c_dev);
+ ret = devm_request_threaded_irq(&pdev->dev, irq_error,
+ NULL,
+ stm32f7_i2c_isr_error_thread,
+ IRQF_ONESHOT,
+ pdev->name, i2c_dev);
if (ret) {
dev_err(&pdev->dev, "Failed to request irq error %i\n",
irq_error);
--
2.25.1
Avoid usage of __func__ when reporting an error message
since dev_err/dev_dbg are already providing enough details
to identify the source of the message.
Signed-off-by: Alain Volmat <[email protected]>
---
drivers/i2c/busses/i2c-stm32f7.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
index 34dcc370e615..2a011deec3c5 100644
--- a/drivers/i2c/busses/i2c-stm32f7.c
+++ b/drivers/i2c/busses/i2c-stm32f7.c
@@ -1602,6 +1602,7 @@ static irqreturn_t stm32f7_i2c_isr_error_thread(int irq, void *data)
{
struct stm32f7_i2c_dev *i2c_dev = data;
struct stm32f7_i2c_msg *f7_msg = &i2c_dev->f7_msg;
+ u16 addr = f7_msg->addr;
void __iomem *base = i2c_dev->base;
struct device *dev = i2c_dev->dev;
struct stm32_i2c_dma *dma = i2c_dev->dma;
@@ -1611,8 +1612,7 @@ static irqreturn_t stm32f7_i2c_isr_error_thread(int irq, void *data)
/* Bus error */
if (status & STM32F7_I2C_ISR_BERR) {
- dev_err(dev, "<%s>: Bus error accessing addr 0x%x\n",
- __func__, f7_msg->addr);
+ dev_err(dev, "Bus error accessing addr 0x%x\n", addr);
writel_relaxed(STM32F7_I2C_ICR_BERRCF, base + STM32F7_I2C_ICR);
stm32f7_i2c_release_bus(&i2c_dev->adap);
f7_msg->result = -EIO;
@@ -1620,21 +1620,19 @@ static irqreturn_t stm32f7_i2c_isr_error_thread(int irq, void *data)
/* Arbitration loss */
if (status & STM32F7_I2C_ISR_ARLO) {
- dev_dbg(dev, "<%s>: Arbitration loss accessing addr 0x%x\n",
- __func__, f7_msg->addr);
+ dev_dbg(dev, "Arbitration loss accessing addr 0x%x\n", addr);
writel_relaxed(STM32F7_I2C_ICR_ARLOCF, base + STM32F7_I2C_ICR);
f7_msg->result = -EAGAIN;
}
if (status & STM32F7_I2C_ISR_PECERR) {
- dev_err(dev, "<%s>: PEC error in reception accessing addr 0x%x\n",
- __func__, f7_msg->addr);
+ dev_err(dev, "PEC error in reception accessing addr 0x%x\n", addr);
writel_relaxed(STM32F7_I2C_ICR_PECCF, base + STM32F7_I2C_ICR);
f7_msg->result = -EINVAL;
}
if (status & STM32F7_I2C_ISR_ALERT) {
- dev_dbg(dev, "<%s>: SMBus alert received\n", __func__);
+ dev_dbg(dev, "SMBus alert received\n");
writel_relaxed(STM32F7_I2C_ICR_ALERTCF, base + STM32F7_I2C_ICR);
i2c_handle_smbus_alert(i2c_dev->alert->ara);
return IRQ_HANDLED;
--
2.25.1
Add a new compatible st,stm32mp25-i2c for the STM32MP25 series which
has only one interrupt line for both events and errors and differs in
term of handling of FastModePlus.
Signed-off-by: Alain Volmat <[email protected]>
---
.../devicetree/bindings/i2c/st,stm32-i2c.yaml | 28 +++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/Documentation/devicetree/bindings/i2c/st,stm32-i2c.yaml b/Documentation/devicetree/bindings/i2c/st,stm32-i2c.yaml
index 94b75d9f66cd..1b31b87c1800 100644
--- a/Documentation/devicetree/bindings/i2c/st,stm32-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/st,stm32-i2c.yaml
@@ -19,6 +19,7 @@ allOf:
- st,stm32f7-i2c
- st,stm32mp13-i2c
- st,stm32mp15-i2c
+ - st,stm32mp25-i2c
then:
properties:
i2c-scl-rising-time-ns:
@@ -41,6 +42,30 @@ allOf:
clock-frequency:
enum: [100000, 400000]
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - st,stm32f4-i2c
+ - st,stm32f7-i2c
+ - st,stm32mp13-i2c
+ - st,stm32mp15-i2c
+ then:
+ properties:
+ interrupts:
+ minItems: 2
+
+ interrupt-names:
+ minItems: 2
+ else:
+ properties:
+ interrupts:
+ maxItems: 1
+
+ interrupt-names:
+ maxItems: 1
+
properties:
compatible:
enum:
@@ -48,6 +73,7 @@ properties:
- st,stm32f7-i2c
- st,stm32mp13-i2c
- st,stm32mp15-i2c
+ - st,stm32mp25-i2c
reg:
maxItems: 1
@@ -56,11 +82,13 @@ properties:
items:
- description: interrupt ID for I2C event
- description: interrupt ID for I2C error
+ minItems: 1
interrupt-names:
items:
- const: event
- const: error
+ minItems: 1
resets:
maxItems: 1
--
2.25.1
The stm32mp25 has only a single interrupt line used for both
events and errors. In order to cope with that, reorganise the
error handling code so that it can be called either from the
common handler (used in case of SoC having only a single IT line)
and the error handler for others.
The CR1 register also embeds a new FMP bit, necessary when running
at Fast Mode Plus frequency. This bit should be used instead of
the SYSCFG bit used on other platforms.
Add a new compatible to distinguish between the SoCs and two
boolean within the setup structure in order to know if the
platform has a single/multiple IT lines and if the FMP bit
within CR1 is available or not.
Signed-off-by: Alain Volmat <[email protected]>
Signed-off-by: Valentin Caron <[email protected]>
---
drivers/i2c/busses/i2c-stm32f7.c | 230 ++++++++++++++++++-------------
1 file changed, 133 insertions(+), 97 deletions(-)
diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
index 2a011deec3c5..5634332900fb 100644
--- a/drivers/i2c/busses/i2c-stm32f7.c
+++ b/drivers/i2c/busses/i2c-stm32f7.c
@@ -50,6 +50,7 @@
#define STM32F7_I2C_TXDR 0x28
/* STM32F7 I2C control 1 */
+#define STM32_I2C_CR1_FMP BIT(24)
#define STM32F7_I2C_CR1_PECEN BIT(23)
#define STM32F7_I2C_CR1_ALERTEN BIT(22)
#define STM32F7_I2C_CR1_SMBHEN BIT(20)
@@ -226,6 +227,8 @@ struct stm32f7_i2c_spec {
* @rise_time: Rise time (ns)
* @fall_time: Fall time (ns)
* @fmp_clr_offset: Fast Mode Plus clear register offset from set register
+ * @single_it_line: Only a single IT line is used for both events/errors
+ * @fmp_cr1_bit: Fast Mode Plus control is done via a bit in CR1
*/
struct stm32f7_i2c_setup {
u32 speed_freq;
@@ -233,6 +236,8 @@ struct stm32f7_i2c_setup {
u32 rise_time;
u32 fall_time;
u32 fmp_clr_offset;
+ bool single_it_line;
+ bool fmp_cr1_bit;
};
/**
@@ -418,6 +423,13 @@ static const struct stm32f7_i2c_setup stm32mp13_setup = {
.fmp_clr_offset = 0x4,
};
+static const struct stm32f7_i2c_setup stm32mp25_setup = {
+ .rise_time = STM32F7_I2C_RISE_TIME_DEFAULT,
+ .fall_time = STM32F7_I2C_FALL_TIME_DEFAULT,
+ .single_it_line = true,
+ .fmp_cr1_bit = true,
+};
+
static inline void stm32f7_i2c_set_bits(void __iomem *reg, u32 mask)
{
writel_relaxed(readl_relaxed(reg) | mask, reg);
@@ -1419,15 +1431,13 @@ static bool stm32f7_i2c_is_slave_busy(struct stm32f7_i2c_dev *i2c_dev)
return i == busy;
}
-static irqreturn_t stm32f7_i2c_slave_isr_event(struct stm32f7_i2c_dev *i2c_dev)
+static irqreturn_t stm32f7_i2c_slave_isr_event(struct stm32f7_i2c_dev *i2c_dev, u32 status)
{
void __iomem *base = i2c_dev->base;
- u32 cr2, status, mask;
+ u32 cr2, mask;
u8 val;
int ret;
- status = readl_relaxed(i2c_dev->base + STM32F7_I2C_ISR);
-
/* Slave transmitter mode */
if (status & STM32F7_I2C_ISR_TXIS) {
i2c_slave_event(i2c_dev->slave_running,
@@ -1494,17 +1504,81 @@ static irqreturn_t stm32f7_i2c_slave_isr_event(struct stm32f7_i2c_dev *i2c_dev)
return IRQ_HANDLED;
}
+static irqreturn_t stm32f7_i2c_handle_isr_errs(struct stm32f7_i2c_dev *i2c_dev, u32 status)
+{
+ struct stm32f7_i2c_msg *f7_msg = &i2c_dev->f7_msg;
+ u16 addr = f7_msg->addr;
+ void __iomem *base = i2c_dev->base;
+ struct device *dev = i2c_dev->dev;
+ struct stm32_i2c_dma *dma = i2c_dev->dma;
+
+ /* Bus error */
+ if (status & STM32F7_I2C_ISR_BERR) {
+ dev_err(dev, "Bus error accessing addr 0x%x\n", addr);
+ writel_relaxed(STM32F7_I2C_ICR_BERRCF, base + STM32F7_I2C_ICR);
+ stm32f7_i2c_release_bus(&i2c_dev->adap);
+ f7_msg->result = -EIO;
+ }
+
+ /* Arbitration loss */
+ if (status & STM32F7_I2C_ISR_ARLO) {
+ dev_dbg(dev, "Arbitration loss accessing addr 0x%x\n", addr);
+ writel_relaxed(STM32F7_I2C_ICR_ARLOCF, base + STM32F7_I2C_ICR);
+ f7_msg->result = -EAGAIN;
+ }
+
+ if (status & STM32F7_I2C_ISR_PECERR) {
+ dev_err(dev, "PEC error in reception accessing addr 0x%x\n", addr);
+ writel_relaxed(STM32F7_I2C_ICR_PECCF, base + STM32F7_I2C_ICR);
+ f7_msg->result = -EINVAL;
+ }
+
+ if (status & STM32F7_I2C_ISR_ALERT) {
+ dev_dbg(dev, "SMBus alert received\n");
+ writel_relaxed(STM32F7_I2C_ICR_ALERTCF, base + STM32F7_I2C_ICR);
+ i2c_handle_smbus_alert(i2c_dev->alert->ara);
+ return IRQ_HANDLED;
+ }
+
+ if (!i2c_dev->slave_running) {
+ u32 mask;
+ /* Disable interrupts */
+ if (stm32f7_i2c_is_slave_registered(i2c_dev))
+ mask = STM32F7_I2C_XFER_IRQ_MASK;
+ else
+ mask = STM32F7_I2C_ALL_IRQ_MASK;
+ stm32f7_i2c_disable_irq(i2c_dev, mask);
+ }
+
+ /* Disable dma */
+ if (i2c_dev->use_dma) {
+ stm32f7_i2c_disable_dma_req(i2c_dev);
+ dmaengine_terminate_async(dma->chan_using);
+ }
+
+ i2c_dev->master_mode = false;
+ complete(&i2c_dev->complete);
+
+ return IRQ_HANDLED;
+}
+
+#define STM32F7_ERR_EVENTS (STM32F7_I2C_ISR_BERR | STM32F7_I2C_ISR_ARLO |\
+ STM32F7_I2C_ISR_PECERR | STM32F7_I2C_ISR_ALERT)
static irqreturn_t stm32f7_i2c_isr_event(int irq, void *data)
{
struct stm32f7_i2c_dev *i2c_dev = data;
u32 status;
- /* Check if the interrupt is for a slave device */
- if (!i2c_dev->master_mode)
- return IRQ_WAKE_THREAD;
-
status = readl_relaxed(i2c_dev->base + STM32F7_I2C_ISR);
+ /*
+ * Check if the interrupt is for a slave device or related
+ * to errors flags (in case of single it line mode)
+ */
+ if (!i2c_dev->master_mode ||
+ (i2c_dev->setup.single_it_line && (status & STM32F7_ERR_EVENTS)))
+ return IRQ_WAKE_THREAD;
+
/* Tx empty */
if (status & STM32F7_I2C_ISR_TXIS)
stm32f7_i2c_write_tx_data(i2c_dev);
@@ -1531,10 +1605,14 @@ static irqreturn_t stm32f7_i2c_isr_event_thread(int irq, void *data)
u32 status, mask;
int ret;
+ status = readl_relaxed(i2c_dev->base + STM32F7_I2C_ISR);
+
if (!i2c_dev->master_mode)
- return stm32f7_i2c_slave_isr_event(i2c_dev);
+ return stm32f7_i2c_slave_isr_event(i2c_dev, status);
- status = readl_relaxed(i2c_dev->base + STM32F7_I2C_ISR);
+ /* Handle errors in case of this handler is used for events/errors */
+ if (i2c_dev->setup.single_it_line && (status & STM32F7_ERR_EVENTS))
+ return stm32f7_i2c_handle_isr_errs(i2c_dev, status);
/* NACK received */
if (status & STM32F7_I2C_ISR_NACKF) {
@@ -1601,63 +1679,11 @@ static irqreturn_t stm32f7_i2c_isr_event_thread(int irq, void *data)
static irqreturn_t stm32f7_i2c_isr_error_thread(int irq, void *data)
{
struct stm32f7_i2c_dev *i2c_dev = data;
- struct stm32f7_i2c_msg *f7_msg = &i2c_dev->f7_msg;
- u16 addr = f7_msg->addr;
- void __iomem *base = i2c_dev->base;
- struct device *dev = i2c_dev->dev;
- struct stm32_i2c_dma *dma = i2c_dev->dma;
u32 status;
status = readl_relaxed(i2c_dev->base + STM32F7_I2C_ISR);
- /* Bus error */
- if (status & STM32F7_I2C_ISR_BERR) {
- dev_err(dev, "Bus error accessing addr 0x%x\n", addr);
- writel_relaxed(STM32F7_I2C_ICR_BERRCF, base + STM32F7_I2C_ICR);
- stm32f7_i2c_release_bus(&i2c_dev->adap);
- f7_msg->result = -EIO;
- }
-
- /* Arbitration loss */
- if (status & STM32F7_I2C_ISR_ARLO) {
- dev_dbg(dev, "Arbitration loss accessing addr 0x%x\n", addr);
- writel_relaxed(STM32F7_I2C_ICR_ARLOCF, base + STM32F7_I2C_ICR);
- f7_msg->result = -EAGAIN;
- }
-
- if (status & STM32F7_I2C_ISR_PECERR) {
- dev_err(dev, "PEC error in reception accessing addr 0x%x\n", addr);
- writel_relaxed(STM32F7_I2C_ICR_PECCF, base + STM32F7_I2C_ICR);
- f7_msg->result = -EINVAL;
- }
-
- if (status & STM32F7_I2C_ISR_ALERT) {
- dev_dbg(dev, "SMBus alert received\n");
- writel_relaxed(STM32F7_I2C_ICR_ALERTCF, base + STM32F7_I2C_ICR);
- i2c_handle_smbus_alert(i2c_dev->alert->ara);
- return IRQ_HANDLED;
- }
-
- if (!i2c_dev->slave_running) {
- u32 mask;
- /* Disable interrupts */
- if (stm32f7_i2c_is_slave_registered(i2c_dev))
- mask = STM32F7_I2C_XFER_IRQ_MASK;
- else
- mask = STM32F7_I2C_ALL_IRQ_MASK;
- stm32f7_i2c_disable_irq(i2c_dev, mask);
- }
-
- /* Disable dma */
- if (i2c_dev->use_dma) {
- stm32f7_i2c_disable_dma_req(i2c_dev);
- dmaengine_terminate_async(dma->chan_using);
- }
-
- i2c_dev->master_mode = false;
- complete(&i2c_dev->complete);
-
- return IRQ_HANDLED;
+ return stm32f7_i2c_handle_isr_errs(i2c_dev, status);
}
static int stm32f7_i2c_wait_polling(struct stm32f7_i2c_dev *i2c_dev)
@@ -1993,23 +2019,27 @@ static int stm32f7_i2c_unreg_slave(struct i2c_client *slave)
static int stm32f7_i2c_write_fm_plus_bits(struct stm32f7_i2c_dev *i2c_dev,
bool enable)
{
- int ret;
+ int ret = 0;
if (i2c_dev->bus_rate <= I2C_MAX_FAST_MODE_FREQ ||
- IS_ERR_OR_NULL(i2c_dev->regmap))
+ (!i2c_dev->setup.fmp_cr1_bit && IS_ERR_OR_NULL(i2c_dev->regmap)))
/* Optional */
return 0;
- if (i2c_dev->fmp_sreg == i2c_dev->fmp_creg)
- ret = regmap_update_bits(i2c_dev->regmap,
- i2c_dev->fmp_sreg,
- i2c_dev->fmp_mask,
- enable ? i2c_dev->fmp_mask : 0);
- else
- ret = regmap_write(i2c_dev->regmap,
- enable ? i2c_dev->fmp_sreg :
- i2c_dev->fmp_creg,
- i2c_dev->fmp_mask);
+ if (i2c_dev->setup.fmp_cr1_bit) {
+ if (enable)
+ stm32f7_i2c_set_bits(i2c_dev->base + STM32F7_I2C_CR1, STM32_I2C_CR1_FMP);
+ else
+ stm32f7_i2c_clr_bits(i2c_dev->base + STM32F7_I2C_CR1, STM32_I2C_CR1_FMP);
+ } else {
+ if (i2c_dev->fmp_sreg == i2c_dev->fmp_creg)
+ ret = regmap_update_bits(i2c_dev->regmap, i2c_dev->fmp_sreg,
+ i2c_dev->fmp_mask, enable ? i2c_dev->fmp_mask : 0);
+ else
+ ret = regmap_write(i2c_dev->regmap,
+ enable ? i2c_dev->fmp_sreg : i2c_dev->fmp_creg,
+ i2c_dev->fmp_mask);
+ }
return ret;
}
@@ -2143,6 +2173,13 @@ static int stm32f7_i2c_probe(struct platform_device *pdev)
if (!i2c_dev)
return -ENOMEM;
+ setup = of_device_get_match_data(&pdev->dev);
+ if (!setup) {
+ dev_err(&pdev->dev, "Can't get device data\n");
+ return -ENODEV;
+ }
+ i2c_dev->setup = *setup;
+
i2c_dev->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
if (IS_ERR(i2c_dev->base))
return PTR_ERR(i2c_dev->base);
@@ -2152,10 +2189,6 @@ static int stm32f7_i2c_probe(struct platform_device *pdev)
if (irq_event < 0)
return irq_event;
- irq_error = platform_get_irq(pdev, 1);
- if (irq_error < 0)
- return irq_error;
-
i2c_dev->wakeup_src = of_property_read_bool(pdev->dev.of_node,
"wakeup-source");
@@ -2186,23 +2219,22 @@ static int stm32f7_i2c_probe(struct platform_device *pdev)
return ret;
}
- ret = devm_request_threaded_irq(&pdev->dev, irq_error,
- NULL,
- stm32f7_i2c_isr_error_thread,
- IRQF_ONESHOT,
- pdev->name, i2c_dev);
- if (ret) {
- dev_err(&pdev->dev, "Failed to request irq error %i\n",
- irq_error);
- return ret;
- }
+ if (!i2c_dev->setup.single_it_line) {
+ irq_error = platform_get_irq(pdev, 1);
+ if (irq_error < 0)
+ return irq_error;
- setup = of_device_get_match_data(&pdev->dev);
- if (!setup) {
- dev_err(&pdev->dev, "Can't get device data\n");
- return -ENODEV;
+ ret = devm_request_threaded_irq(&pdev->dev, irq_error,
+ NULL,
+ stm32f7_i2c_isr_error_thread,
+ IRQF_ONESHOT,
+ pdev->name, i2c_dev);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to request irq error %i\n",
+ irq_error);
+ return ret;
+ }
}
- i2c_dev->setup = *setup;
ret = stm32f7_i2c_setup_timing(i2c_dev, &i2c_dev->setup);
if (ret)
@@ -2210,9 +2242,12 @@ static int stm32f7_i2c_probe(struct platform_device *pdev)
/* Setup Fast mode plus if necessary */
if (i2c_dev->bus_rate > I2C_MAX_FAST_MODE_FREQ) {
- ret = stm32f7_i2c_setup_fm_plus_bits(pdev, i2c_dev);
- if (ret)
- return ret;
+ if (!i2c_dev->setup.fmp_cr1_bit) {
+ ret = stm32f7_i2c_setup_fm_plus_bits(pdev, i2c_dev);
+ if (ret)
+ return ret;
+ }
+
ret = stm32f7_i2c_write_fm_plus_bits(i2c_dev, true);
if (ret)
return ret;
@@ -2491,6 +2526,7 @@ static const struct of_device_id stm32f7_i2c_match[] = {
{ .compatible = "st,stm32f7-i2c", .data = &stm32f7_setup},
{ .compatible = "st,stm32mp15-i2c", .data = &stm32mp15_setup},
{ .compatible = "st,stm32mp13-i2c", .data = &stm32mp13_setup},
+ { .compatible = "st,stm32mp25-i2c", .data = &stm32mp25_setup},
{},
};
MODULE_DEVICE_TABLE(of, stm32f7_i2c_match);
--
2.25.1
Add the 8 nodes for all i2c instances available on the stm32mp251.
Signed-off-by: Alain Volmat <[email protected]>
---
arch/arm64/boot/dts/st/stm32mp251.dtsi | 96 ++++++++++++++++++++++++++
1 file changed, 96 insertions(+)
diff --git a/arch/arm64/boot/dts/st/stm32mp251.dtsi b/arch/arm64/boot/dts/st/stm32mp251.dtsi
index dfbdb3a773e4..93bc8a8908ce 100644
--- a/arch/arm64/boot/dts/st/stm32mp251.dtsi
+++ b/arch/arm64/boot/dts/st/stm32mp251.dtsi
@@ -116,6 +116,102 @@ usart2: serial@400e0000 {
status = "disabled";
};
+ i2c1: i2c@40120000 {
+ compatible = "st,stm32mp25-i2c";
+ reg = <0x40120000 0x400>;
+ interrupt-names = "event";
+ interrupts = <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&rcc CK_KER_I2C1>;
+ resets = <&rcc I2C1_R>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
+ i2c2: i2c@40130000 {
+ compatible = "st,stm32mp25-i2c";
+ reg = <0x40130000 0x400>;
+ interrupt-names = "event";
+ interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&rcc CK_KER_I2C2>;
+ resets = <&rcc I2C2_R>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
+ i2c3: i2c@40140000 {
+ compatible = "st,stm32mp25-i2c";
+ reg = <0x40140000 0x400>;
+ interrupt-names = "event";
+ interrupts = <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&rcc CK_KER_I2C3>;
+ resets = <&rcc I2C3_R>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
+ i2c4: i2c@40150000 {
+ compatible = "st,stm32mp25-i2c";
+ reg = <0x40150000 0x400>;
+ interrupt-names = "event";
+ interrupts = <GIC_SPI 168 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&rcc CK_KER_I2C4>;
+ resets = <&rcc I2C4_R>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
+ i2c5: i2c@40160000 {
+ compatible = "st,stm32mp25-i2c";
+ reg = <0x40160000 0x400>;
+ interrupt-names = "event";
+ interrupts = <GIC_SPI 181 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&rcc CK_KER_I2C5>;
+ resets = <&rcc I2C5_R>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
+ i2c6: i2c@40170000 {
+ compatible = "st,stm32mp25-i2c";
+ reg = <0x40170000 0x400>;
+ interrupt-names = "event";
+ interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&rcc CK_KER_I2C6>;
+ resets = <&rcc I2C6_R>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
+ i2c7: i2c@40180000 {
+ compatible = "st,stm32mp25-i2c";
+ reg = <0x40180000 0x400>;
+ interrupt-names = "event";
+ interrupts = <GIC_SPI 210 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&rcc CK_KER_I2C7>;
+ resets = <&rcc I2C7_R>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
+ i2c8: i2c@46040000 {
+ compatible = "st,stm32mp25-i2c";
+ reg = <0x46040000 0x400>;
+ interrupt-names = "event";
+ interrupts = <GIC_SPI 212 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&rcc CK_KER_I2C8>;
+ resets = <&rcc I2C8_R>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
sdmmc1: mmc@48220000 {
compatible = "st,stm32mp25-sdmmc2", "arm,pl18x", "arm,primecell";
arm,primecell-periphid = <0x00353180>;
--
2.25.1
Add the i2c2 and i2c8 pins used on STM32MP257F-EV1 board.
Signed-off-by: Alain Volmat <[email protected]>
---
arch/arm64/boot/dts/st/stm32mp25-pinctrl.dtsi | 36 +++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/arch/arm64/boot/dts/st/stm32mp25-pinctrl.dtsi b/arch/arm64/boot/dts/st/stm32mp25-pinctrl.dtsi
index 66791a974f8f..4194807606ce 100644
--- a/arch/arm64/boot/dts/st/stm32mp25-pinctrl.dtsi
+++ b/arch/arm64/boot/dts/st/stm32mp25-pinctrl.dtsi
@@ -6,6 +6,23 @@
#include <dt-bindings/pinctrl/stm32-pinfunc.h>
&pinctrl {
+ i2c2_pins_a: i2c2-0 {
+ pins {
+ pinmux = <STM32_PINMUX('B', 5, AF9)>, /* I2C2_SCL */
+ <STM32_PINMUX('B', 4, AF9)>; /* I2C2_SDA */
+ bias-disable;
+ drive-open-drain;
+ slew-rate = <0>;
+ };
+ };
+
+ i2c2_sleep_pins_a: i2c2-sleep-0 {
+ pins {
+ pinmux = <STM32_PINMUX('B', 5, ANALOG)>, /* I2C2_SCL */
+ <STM32_PINMUX('B', 4, ANALOG)>; /* I2C2_SDA */
+ };
+ };
+
sdmmc1_b4_pins_a: sdmmc1-b4-0 {
pins1 {
pinmux = <STM32_PINMUX('E', 4, AF10)>, /* SDMMC1_D0 */
@@ -90,3 +107,22 @@ pins {
};
};
};
+
+&pinctrl_z {
+ i2c8_pins_a: i2c8-0 {
+ pins {
+ pinmux = <STM32_PINMUX('Z', 4, AF8)>, /* I2C8_SCL */
+ <STM32_PINMUX('Z', 3, AF8)>; /* I2C8_SDA */
+ bias-disable;
+ drive-open-drain;
+ slew-rate = <0>;
+ };
+ };
+
+ i2c8_sleep_pins_a: i2c8-sleep-0 {
+ pins {
+ pinmux = <STM32_PINMUX('Z', 4, ANALOG)>, /* I2C8_SCL */
+ <STM32_PINMUX('Z', 3, ANALOG)>; /* I2C8_SDA */
+ };
+ };
+};
--
2.25.1
Add properties for i2c2 and i2c8 available on the stm32mp257f-ev1.
i2c2 is enabled since several devices are attached to it while
i2c8 is kept disabled since only used via the gpio expansion connector.
Signed-off-by: Alain Volmat <[email protected]>
---
arch/arm64/boot/dts/st/stm32mp257f-ev1.dts | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/arch/arm64/boot/dts/st/stm32mp257f-ev1.dts b/arch/arm64/boot/dts/st/stm32mp257f-ev1.dts
index b2d3afb15758..0ea8e69bfb3d 100644
--- a/arch/arm64/boot/dts/st/stm32mp257f-ev1.dts
+++ b/arch/arm64/boot/dts/st/stm32mp257f-ev1.dts
@@ -55,6 +55,26 @@ &arm_wdt {
status = "okay";
};
+&i2c2 {
+ pinctrl-names = "default", "sleep";
+ pinctrl-0 = <&i2c2_pins_a>;
+ pinctrl-1 = <&i2c2_sleep_pins_a>;
+ i2c-scl-rising-time-ns = <100>;
+ i2c-scl-falling-time-ns = <13>;
+ clock-frequency = <400000>;
+ status = "okay";
+};
+
+&i2c8 {
+ pinctrl-names = "default", "sleep";
+ pinctrl-0 = <&i2c8_pins_a>;
+ pinctrl-1 = <&i2c8_sleep_pins_a>;
+ i2c-scl-rising-time-ns = <57>;
+ i2c-scl-falling-time-ns = <7>;
+ clock-frequency = <400000>;
+ status = "disabled";
+};
+
&sdmmc1 {
pinctrl-names = "default", "opendrain", "sleep";
pinctrl-0 = <&sdmmc1_b4_pins_a>;
--
2.25.1
Hi Alain,
On Fri, Dec 08, 2023 at 05:47:10PM +0100, Alain Volmat wrote:
> The irq handling is currently split between the irq handler
> and the threaded irq handler. Some of the handling (such as
> dma related stuffs) done within the irq handler might sleep or
> take some time leading to issues if the kernel is built with
> realtime constraints. In order to fix that, perform an overall
> rework to perform most of the job within the threaded handler
> and only keep fifo access in the non threaded handler.
>
> Signed-off-by: Alain Volmat <[email protected]>
quite a difficult review because this git diff algorithm makes it
difficult to read throuhg.
But it looks like just a copy paste from to
stm32f7_i2c_isr_event() to stm32f7_i2c_isr_event_thread() of the
STM32F7_I2C_ISR_NACKF, STM32F7_I2C_ISR_STOPF, STM32F7_I2C_ISR_TC
and STM32F7_I2C_ISR_TCR.
[...]
> +static irqreturn_t stm32f7_i2c_isr_event_thread(int irq, void *data)
> +{
> + struct stm32f7_i2c_dev *i2c_dev = data;
> + struct stm32f7_i2c_msg *f7_msg = &i2c_dev->f7_msg;
> + struct stm32_i2c_dma *dma = i2c_dev->dma;
> + void __iomem *base = i2c_dev->base;
> + u32 status, mask;
> + int ret;
> +
> + if (!i2c_dev->master_mode)
> + return stm32f7_i2c_slave_isr_event(i2c_dev);
> +
> + status = readl_relaxed(i2c_dev->base + STM32F7_I2C_ISR);
looks to me like this readl_relaxed is read too many times during
the whole irq handling.
Reviewed-by: Andi Shyti <[email protected]>
Thanks,
Andi
Hi Alain,
On Fri, Dec 08, 2023 at 05:47:11PM +0100, Alain Volmat wrote:
> Avoid usage of __func__ when reporting an error message
> since dev_err/dev_dbg are already providing enough details
> to identify the source of the message.
>
> Signed-off-by: Alain Volmat <[email protected]>
Reviewed-by: Andi Shyti <[email protected]>
Thanks,
Andi
Hi Alain,
On Fri, Dec 08, 2023 at 05:47:13PM +0100, Alain Volmat wrote:
> The stm32mp25 has only a single interrupt line used for both
> events and errors. In order to cope with that, reorganise the
> error handling code so that it can be called either from the
> common handler (used in case of SoC having only a single IT line)
> and the error handler for others.
> The CR1 register also embeds a new FMP bit, necessary when running
> at Fast Mode Plus frequency. This bit should be used instead of
> the SYSCFG bit used on other platforms.
> Add a new compatible to distinguish between the SoCs and two
> boolean within the setup structure in order to know if the
> platform has a single/multiple IT lines and if the FMP bit
> within CR1 is available or not.
>
> Signed-off-by: Alain Volmat <[email protected]>
> Signed-off-by: Valentin Caron <[email protected]>
your SoB here should come last because you are the one sending
the patch.
> ---
> drivers/i2c/busses/i2c-stm32f7.c | 230 ++++++++++++++++++-------------
> 1 file changed, 133 insertions(+), 97 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
> index 2a011deec3c5..5634332900fb 100644
> --- a/drivers/i2c/busses/i2c-stm32f7.c
> +++ b/drivers/i2c/busses/i2c-stm32f7.c
> @@ -50,6 +50,7 @@
> #define STM32F7_I2C_TXDR 0x28
>
> /* STM32F7 I2C control 1 */
> +#define STM32_I2C_CR1_FMP BIT(24)
> #define STM32F7_I2C_CR1_PECEN BIT(23)
> #define STM32F7_I2C_CR1_ALERTEN BIT(22)
> #define STM32F7_I2C_CR1_SMBHEN BIT(20)
> @@ -226,6 +227,8 @@ struct stm32f7_i2c_spec {
> * @rise_time: Rise time (ns)
> * @fall_time: Fall time (ns)
> * @fmp_clr_offset: Fast Mode Plus clear register offset from set register
> + * @single_it_line: Only a single IT line is used for both events/errors
> + * @fmp_cr1_bit: Fast Mode Plus control is done via a bit in CR1
Is the Fast Mode Plus an optional feature?
> */
> struct stm32f7_i2c_setup {
> u32 speed_freq;
> @@ -233,6 +236,8 @@ struct stm32f7_i2c_setup {
> u32 rise_time;
> u32 fall_time;
> u32 fmp_clr_offset;
> + bool single_it_line;
> + bool fmp_cr1_bit;
> };
[...]
> -static irqreturn_t stm32f7_i2c_slave_isr_event(struct stm32f7_i2c_dev *i2c_dev)
> +static irqreturn_t stm32f7_i2c_slave_isr_event(struct stm32f7_i2c_dev *i2c_dev, u32 status)
> {
> void __iomem *base = i2c_dev->base;
> - u32 cr2, status, mask;
> + u32 cr2, mask;
> u8 val;
> int ret;
>
> - status = readl_relaxed(i2c_dev->base + STM32F7_I2C_ISR);
> -
good to see this change here, relates to my comment in patch 1.
But I think this should go on a different patch.
> /* Slave transmitter mode */
> if (status & STM32F7_I2C_ISR_TXIS) {
> i2c_slave_event(i2c_dev->slave_running,
> @@ -1494,17 +1504,81 @@ static irqreturn_t stm32f7_i2c_slave_isr_event(struct stm32f7_i2c_dev *i2c_dev)
> return IRQ_HANDLED;
> }
[...]
> - setup = of_device_get_match_data(&pdev->dev);
> - if (!setup) {
> - dev_err(&pdev->dev, "Can't get device data\n");
> - return -ENODEV;
> + ret = devm_request_threaded_irq(&pdev->dev, irq_error,
> + NULL,
> + stm32f7_i2c_isr_error_thread,
> + IRQF_ONESHOT,
> + pdev->name, i2c_dev);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to request irq error %i\n",
> + irq_error);
please use dev_err_probe();
> + return ret;
> + }
out of the scope of the patch and just for curiosity: does the
driver work without being able to signal on the error interrupt
line?
Overall the patch looks good to me, though.
Andi
On Fri, Dec 08, 2023 at 05:47:12PM +0100, Alain Volmat wrote:
> Add a new compatible st,stm32mp25-i2c for the STM32MP25 series which
> has only one interrupt line for both events and errors and differs in
> term of handling of FastModePlus.
>
> Signed-off-by: Alain Volmat <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
Cheers,
Conor.
Hi Andi,
thanks for your review.
I'll shortly post a v3 to correct all your comments.
On Sat, Dec 09, 2023 at 01:07:47AM +0100, Andi Shyti wrote:
> Hi Alain,
>
> On Fri, Dec 08, 2023 at 05:47:13PM +0100, Alain Volmat wrote:
> > The stm32mp25 has only a single interrupt line used for both
> > events and errors. In order to cope with that, reorganise the
> > error handling code so that it can be called either from the
> > common handler (used in case of SoC having only a single IT line)
> > and the error handler for others.
> > The CR1 register also embeds a new FMP bit, necessary when running
> > at Fast Mode Plus frequency. This bit should be used instead of
> > the SYSCFG bit used on other platforms.
> > Add a new compatible to distinguish between the SoCs and two
> > boolean within the setup structure in order to know if the
> > platform has a single/multiple IT lines and if the FMP bit
> > within CR1 is available or not.
> >
> > Signed-off-by: Alain Volmat <[email protected]>
> > Signed-off-by: Valentin Caron <[email protected]>
>
> your SoB here should come last because you are the one sending
> the patch.
Fixed.
>
> > ---
> > drivers/i2c/busses/i2c-stm32f7.c | 230 ++++++++++++++++++-------------
> > 1 file changed, 133 insertions(+), 97 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
> > index 2a011deec3c5..5634332900fb 100644
> > --- a/drivers/i2c/busses/i2c-stm32f7.c
> > +++ b/drivers/i2c/busses/i2c-stm32f7.c
> > @@ -50,6 +50,7 @@
> > #define STM32F7_I2C_TXDR 0x28
> >
> > /* STM32F7 I2C control 1 */
> > +#define STM32_I2C_CR1_FMP BIT(24)
> > #define STM32F7_I2C_CR1_PECEN BIT(23)
> > #define STM32F7_I2C_CR1_ALERTEN BIT(22)
> > #define STM32F7_I2C_CR1_SMBHEN BIT(20)
> > @@ -226,6 +227,8 @@ struct stm32f7_i2c_spec {
> > * @rise_time: Rise time (ns)
> > * @fall_time: Fall time (ns)
> > * @fmp_clr_offset: Fast Mode Plus clear register offset from set register
> > + * @single_it_line: Only a single IT line is used for both events/errors
> > + * @fmp_cr1_bit: Fast Mode Plus control is done via a bit in CR1
>
> Is the Fast Mode Plus an optional feature?
Yes, from what I've seen, it seems an optional feature on some versions
of stm32f7.
>
> > */
> > struct stm32f7_i2c_setup {
> > u32 speed_freq;
> > @@ -233,6 +236,8 @@ struct stm32f7_i2c_setup {
> > u32 rise_time;
> > u32 fall_time;
> > u32 fmp_clr_offset;
> > + bool single_it_line;
> > + bool fmp_cr1_bit;
> > };
>
> [...]
>
> > -static irqreturn_t stm32f7_i2c_slave_isr_event(struct stm32f7_i2c_dev *i2c_dev)
> > +static irqreturn_t stm32f7_i2c_slave_isr_event(struct stm32f7_i2c_dev *i2c_dev, u32 status)
> > {
> > void __iomem *base = i2c_dev->base;
> > - u32 cr2, status, mask;
> > + u32 cr2, mask;
> > u8 val;
> > int ret;
> >
> > - status = readl_relaxed(i2c_dev->base + STM32F7_I2C_ISR);
> > -
>
> good to see this change here, relates to my comment in patch 1.
> But I think this should go on a different patch.
Agreed. I've split this small change into a dedicated commit done before
this patch.
>
> > /* Slave transmitter mode */
> > if (status & STM32F7_I2C_ISR_TXIS) {
> > i2c_slave_event(i2c_dev->slave_running,
> > @@ -1494,17 +1504,81 @@ static irqreturn_t stm32f7_i2c_slave_isr_event(struct stm32f7_i2c_dev *i2c_dev)
> > return IRQ_HANDLED;
> > }
>
> [...]
>
> > - setup = of_device_get_match_data(&pdev->dev);
> > - if (!setup) {
> > - dev_err(&pdev->dev, "Can't get device data\n");
> > - return -ENODEV;
> > + ret = devm_request_threaded_irq(&pdev->dev, irq_error,
> > + NULL,
> > + stm32f7_i2c_isr_error_thread,
> > + IRQF_ONESHOT,
> > + pdev->name, i2c_dev);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Failed to request irq error %i\n",
> > + irq_error);
>
> please use dev_err_probe();
Done as well in a dedicated commit at the very beginning of the serie.
>
> > + return ret;
> > + }
>
> out of the scope of the patch and just for curiosity: does the
> driver work without being able to signal on the error interrupt
> line?
Sorry, not sure to understand the question. Just for clarification,
on the MP25 not having a dedicated error line doesn't mean we are not
signaled for errors. It is simply they just come via the same
interrupt line, hence we need to check for both event and error
within the same handler.
On systems using two interrupts line, the error interrupt irq line
should be handled in order to tackle those errors cases.
Sorry, does this answer your question ?
>
> Overall the patch looks good to me, though.
>
> Andi