Hi Wolfram,
On Sun, Dec 22, 2024 at 12:44 PM Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
>
>
> > On the RZ/G2L and RZ/G3S there is a restriction for forcing the SDA/SCL states:
> >
> > ● Write:
> > 0: Changes the RIICnSCL/RIICnSDA pin output to a low level.
> > 1: Changes the RIICnSCL/RIICnSDA pin in a high-impedance state.
> > (High level output is achieved through an external pull-up resistor.)
> >
> > So using the generic algorithm may be platform dependent as it would
> > only work on platforms which have external pull-up resistor on SDA/SCL
> > pins. So to overcome this and make recovery possible on the platforms
> > I choose the RIIC feature to output clock cycles as required.
>
> I would be super-surprised if this is really a restriction and not a
> very precise documentation. In other words, I am quite sure that there
> is no difference between the bit forcing SCL high (via high-impedance)
> and the internal RIIC handling when it needs SCL high. Most I2C busses
> are open-drain anyhow.
>
> Or is it confirmed by hardware engineers that RIIC is able to support
> push/pull-busses but only this bit cannot?
>
Based on the feedback from the HW engineer the restriction is valid
see attached image (i2c-pullup.png). The SCL and SDA are Schmitt
input/open-drain output pins for both master and slave operations.
Because the output is open drain, an external pull-up resistor is
required.
Assuming there is an external pull-up resistor for all the platforms I
implemented the I2C bus recovery using the generic recovery algorithm
and I'm seeing issues, as the required number of clock pulses are not
being triggered (Note, the i2c clock frequency is 400000Hz where the
below tests are run).
I'm not sure if the below restriction is causing an issue:
--------------------
The SDAO and SCLO bits directly control the SDAn and SCLn signals
output from the RIIC. When writing to these bits, also write 0b to the
SOWP bit. Setting these bits results in input to the RIIC by the input
buffer. When slave mode is selected, a start condition might be
detected and the bus might be released, depending on the bit settings.
Do not rewrite these bits during a start condition, stop condition,
restart condition, or during transmission or reception. Operation
after rewriting under the specified conditions is not guaranteed. When
reading these bits, the state of signals output from the RIIC can be
read.
Ive run the below sequence and attached the images where the recovery
is failing, during recovery I see there aren't reliable clock pulses
see the attached images i2c-error-*.png
$ echo 0x68 > incomplete_address_phase;i2cget -y -f 3 0x68 8
See images i2c-error-0.png and i2c-error-1.png ---> In here we can
see the i2c error is injected and the SDA line goes low and for
recovery we can see only 6 clock pulses were forced
$ i2cget -y -f 3 0x68 8 #trigger recovery again by reading
See images i2c-error-2.png --->In here we can see only 3 clock pulses
were forced
$ i2cget -y -f 3 0x68 8 #trigger recovery again by reading
See images i2c-error-3.png ---> This is where the i2c has recovered
successfully and we can see proper 9 clock pulses
Below is the I2C recovery code using the generic algorithm:
diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 850f640d8fd4..3b9cb26ede3d 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -54,6 +54,8 @@
#define ICCR1_IICRST BIT(6)
#define ICCR1_CLO BIT(5)
#define ICCR1_SOWP BIT(4)
+#define ICCR1_SCLO BIT(3)
+#define ICCR1_SDAO BIT(2)
#define ICCR1_SCLI BIT(1)
#define ICCR1_SDAI BIT(0)
@@ -181,7 +183,7 @@ static int riic_xfer(struct i2c_adapter *adap,
struct i2c_msg msgs[], int num)
return ret;
riic->err = riic_bus_barrier(riic);
- if (riic->err)
+ if (riic->err < 0)
goto out;
reinit_completion(&riic->msg_done);
@@ -515,6 +517,51 @@ static int riic_recover_bus(struct i2c_adapter *adap)
return 0;
}
+static int riic_get_scl(struct i2c_adapter *adap)
+{
+ struct riic_dev *riic = i2c_get_adapdata(adap);
+
+ return !!(riic_readb(riic, RIIC_ICCR1) & ICCR1_SCLI);
+}
+
+static void riic_set_scl(struct i2c_adapter *adap, int val)
+{
+ struct riic_dev *riic = i2c_get_adapdata(adap);
+
+ if (val)
+ riic_clear_set_bit(riic, ICCR1_SOWP, ICCR1_SCLO, RIIC_ICCR1);
+ else
+ riic_clear_set_bit(riic, ICCR1_SOWP | ICCR1_SCLO, 0,
RIIC_ICCR1);
+
+ riic_clear_set_bit(riic, 0, ICCR1_SOWP, RIIC_ICCR1);
+}
+
+static void riic_set_sda(struct i2c_adapter *adap, int val)
+{
+ struct riic_dev *riic = i2c_get_adapdata(adap);
+
+ if (val)
+ riic_clear_set_bit(riic, ICCR1_SOWP, ICCR1_SDAO, RIIC_ICCR1);
+ else
+ riic_clear_set_bit(riic, ICCR1_SOWP | ICCR1_SDAO, 0,
RIIC_ICCR1);
+
+ riic_clear_set_bit(riic, 0, ICCR1_SOWP, RIIC_ICCR1);
+}
+
+static int riic_get_bus_free(struct i2c_adapter *adap)
+{
+ struct riic_dev *riic = i2c_get_adapdata(adap);
+
+ udelay(5);
+
+ /* Check if the bus is busy or SDA is not high */
+ if ((riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) ||
+ !(riic_readb(riic, RIIC_ICCR1) & ICCR1_SDAI))
+ return -EBUSY;
+
+ return 1;
+}
+
static const struct riic_irq_desc riic_irqs[] = {
{ .res_num = 0, .isr = riic_tend_isr, .name = "riic-tend" },
{ .res_num = 1, .isr = riic_rdrf_isr, .name = "riic-rdrf" },
@@ -524,7 +571,11 @@ static const struct riic_irq_desc riic_irqs[] = {
};
static struct i2c_bus_recovery_info riic_bri = {
- .recover_bus = riic_recover_bus,
+ .get_scl = riic_get_scl,
+ .set_scl = riic_set_scl,
+ .set_sda = riic_set_sda,
+ .get_bus_free = riic_get_bus_free,
+ .recover_bus = i2c_generic_scl_recovery,
};
static int riic_i2c_probe(struct platform_device *pdev)
Attachment:
i2c-error-0.png
Description: PNG image
Attachment:
i2c-error-3.png
Description: PNG image
Attachment:
i2c-pullup.png
Description: PNG image
Attachment:
i2c-error-1.png
Description: PNG image
Attachment:
i2c-error-2.png
Description: PNG image
