Apply the different formula and register setting for activating FM+ on
Gen4 devtypes.
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
drivers/i2c/busses/i2c-rcar.c | 125 ++++++++++++++++++++++++----------
1 file changed, 89 insertions(+), 36 deletions(-)
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 76aa16bf17b2..a48849b393a3 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -41,6 +41,10 @@
#define ICSAR 0x1C /* slave address */
#define ICMAR 0x20 /* master address */
#define ICRXTX 0x24 /* data port */
+#define ICCCR2 0x28 /* Clock control 2 */
+#define ICMPR 0x2C /* SCL mask control */
+#define ICHPR 0x30 /* SCL HIGH control */
+#define ICLPR 0x34 /* SCL LOW control */
#define ICFBSCR 0x38 /* first bit setup cycle (Gen3) */
#define ICDMAER 0x3c /* DMA enable (Gen3) */
@@ -84,6 +88,12 @@
#define RMDMAE BIT(1) /* DMA Master Received Enable */
#define TMDMAE BIT(0) /* DMA Master Transmitted Enable */
+/* ICCCR2 */
+#define FMPE BIT(7) /* Fast Mode Plus Enable */
+#define CDFD BIT(2) /* CDF Disable */
+#define HLSE BIT(1) /* HIGH/LOW Separate Control Enable */
+#define SME BIT(0) /* SCL Mask Enable */
+
/* ICFBSCR */
#define TCYC17 0x0f /* 17*Tcyc delay 1st bit between SDA and SCL */
@@ -104,11 +114,12 @@
#define ID_NACK BIT(4)
#define ID_EPROTO BIT(5)
/* persistent flags */
+#define ID_P_FMPLUS BIT(27)
#define ID_P_NOT_ATOMIC BIT(28)
#define ID_P_HOST_NOTIFY BIT(29)
#define ID_P_NO_RXDMA BIT(30) /* HW forbids RXDMA sometimes */
#define ID_P_PM_BLOCKED BIT(31)
-#define ID_P_MASK GENMASK(31, 28)
+#define ID_P_MASK GENMASK(31, 27)
enum rcar_i2c_type {
I2C_RCAR_GEN1,
@@ -128,7 +139,7 @@ struct rcar_i2c_priv {
wait_queue_head_t wait;
int pos;
- u32 icccr;
+ u32 clock_val;
u8 recovery_icmcr; /* protected by adapter lock */
enum rcar_i2c_type devtype;
struct i2c_client *slave;
@@ -217,7 +228,17 @@ static void rcar_i2c_init(struct rcar_i2c_priv *priv)
rcar_i2c_write(priv, ICMCR, MDBS);
rcar_i2c_write(priv, ICMSR, 0);
/* start clock */
- rcar_i2c_write(priv, ICCCR, priv->icccr);
+ if (priv->flags & ID_P_FMPLUS) {
+ rcar_i2c_write(priv, ICCCR, 0);
+ rcar_i2c_write(priv, ICMPR, priv->clock_val);
+ rcar_i2c_write(priv, ICHPR, 3 * priv->clock_val);
+ rcar_i2c_write(priv, ICLPR, 3 * priv->clock_val);
+ rcar_i2c_write(priv, ICCCR2, FMPE | CDFD | HLSE | SME);
+ } else {
+ rcar_i2c_write(priv, ICCCR, priv->clock_val);
+ if (priv->devtype >= I2C_RCAR_GEN3)
+ rcar_i2c_write(priv, ICCCR2, 0);
+ }
if (priv->devtype >= I2C_RCAR_GEN3)
rcar_i2c_write(priv, ICFBSCR, TCYC17);
@@ -242,7 +263,7 @@ static int rcar_i2c_bus_barrier(struct rcar_i2c_priv *priv)
static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
{
- u32 scgd, cdf, round, ick, sum, scl, cdf_width;
+ u32 scgd, cdf = 0, round, ick, sum, scl, cdf_width, smd;
unsigned long rate;
struct device *dev = rcar_i2c_priv_to_dev(priv);
struct i2c_timings t = {
@@ -252,19 +273,26 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
.scl_int_delay_ns = 50,
};
- cdf_width = (priv->devtype == I2C_RCAR_GEN1) ? 2 : 3;
-
/* Fall back to previously used values if not supplied */
i2c_parse_fw_timings(dev, &t, false);
+ if (t.bus_freq_hz > I2C_MAX_FAST_MODE_FREQ &&
+ priv->devtype >= I2C_RCAR_GEN4)
+ priv->flags |= ID_P_FMPLUS;
+ else
+ priv->flags &= ~ID_P_FMPLUS;
+
/*
* calculate SCL clock
* see
- * ICCCR
+ * ICCCR (and ICCCR2 for FastMode+)
*
* ick = clkp / (1 + CDF)
* SCL = ick / (20 + SCGD * 8 + F[(ticf + tr + intd) * ick])
*
+ * for FastMode+:
+ * SCL = clkp / (8 + SMD * 2 + SCLD + SCHD +F[(ticf + tr + intd) * clkp])
+ *
* ick : I2C internal clock < 20 MHz
* ticf : I2C SCL falling time
* tr : I2C SCL rising time
@@ -273,10 +301,14 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
* F[] : integer up-valuation
*/
rate = clk_get_rate(priv->clk);
- cdf = rate / 20000000;
- if (cdf >= 1U << cdf_width) {
- dev_err(dev, "Input clock %lu too high\n", rate);
- return -EIO;
+
+ if (!(priv->flags & ID_P_FMPLUS)) {
+ cdf = rate / 20000000;
+ cdf_width = (priv->devtype == I2C_RCAR_GEN1) ? 2 : 3;
+ if (cdf >= 1U << cdf_width) {
+ dev_err(dev, "Input clock %lu too high\n", rate);
+ return -EIO;
+ }
}
ick = rate / (cdf + 1);
@@ -292,34 +324,55 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
round = (ick + 500000) / 1000000 * sum;
round = (round + 500) / 1000;
- /*
- * SCL = ick / (20 + SCGD * 8 + F[(ticf + tr + intd) * ick])
- *
- * Calculation result (= SCL) should be less than
- * bus_speed for hardware safety
- *
- * We could use something along the lines of
- * div = ick / (bus_speed + 1) + 1;
- * scgd = (div - 20 - round + 7) / 8;
- * scl = ick / (20 + (scgd * 8) + round);
- * (not fully verified) but that would get pretty involved
- */
- for (scgd = 0; scgd < 0x40; scgd++) {
- scl = ick / (20 + (scgd * 8) + round);
- if (scl <= t.bus_freq_hz)
- break;
- }
+ if (priv->flags & ID_P_FMPLUS) {
+ /*
+ * SMD should be smaller than SCLD and SCHD, we arbitrarily set
+ * the ratio 1:3. SCHD:SCLD ratio is 1:1, thus:
+ * SCL = clkp / (8 + SMD * 2 + SCLD + SCHD + F[(ticf + tr + intd) * clkp])
+ * SCL = clkp / (8 + SMD * 2 + SMD * 3 + SMD * 3 + F[...])
+ * SCL = clkp / (8 + SMD * 8 + F[...])
+ */
+ smd = DIV_ROUND_UP(ick / t.bus_freq_hz - 8 - round, 8);
+ scl = ick / (8 + 8 * smd + round);
- if (scgd == 0x40) {
- dev_err(dev, "it is impossible to calculate best SCL\n");
- return -EIO;
- }
+ if (smd > 0xff) {
+ dev_err(dev, "it is impossible to calculate best SCL\n");
+ return -EINVAL;
+ }
- dev_dbg(dev, "clk %d/%d(%lu), round %u, CDF:0x%x, SCGD: 0x%x\n",
- scl, t.bus_freq_hz, rate, round, cdf, scgd);
+ dev_dbg(dev, "clk %d/%d(%lu), round %u, SMD:0x%x, SCHD: 0x%x\n",
+ scl, t.bus_freq_hz, rate, round, smd, 3 * smd);
- /* keep icccr value */
- priv->icccr = scgd << cdf_width | cdf;
+ priv->clock_val = smd;
+ } else {
+ /*
+ * SCL = ick / (20 + SCGD * 8 + F[(ticf + tr + intd) * ick])
+ *
+ * Calculation result (= SCL) should be less than
+ * bus_speed for hardware safety
+ *
+ * We could use something along the lines of
+ * div = ick / (bus_speed + 1) + 1;
+ * scgd = (div - 20 - round + 7) / 8;
+ * scl = ick / (20 + (scgd * 8) + round);
+ * (not fully verified) but that would get pretty involved
+ */
+ for (scgd = 0; scgd < 0x40; scgd++) {
+ scl = ick / (20 + (scgd * 8) + round);
+ if (scl <= t.bus_freq_hz)
+ break;
+ }
+
+ if (scgd == 0x40) {
+ dev_err(dev, "it is impossible to calculate best SCL\n");
+ return -EINVAL;
+ }
+
+ dev_dbg(dev, "clk %d/%d(%lu), round %u, CDF:0x%x, SCGD: 0x%x\n",
+ scl, t.bus_freq_hz, rate, round, cdf, scgd);
+
+ priv->clock_val = scgd << cdf_width | cdf;
+ }
return 0;
}
--
2.35.1Hi Wolfram,
On Tue, Sep 5, 2023 at 6:01 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Apply the different formula and register setting for activating FM+ on
> Gen4 devtypes.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Thanks for your patch!
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -128,7 +139,7 @@ struct rcar_i2c_priv {
> wait_queue_head_t wait;
>
> int pos;
> - u32 icccr;
> + u32 clock_val;
Perhaps use a union to store either icccr or smd?
> u8 recovery_icmcr; /* protected by adapter lock */
> enum rcar_i2c_type devtype;
> struct i2c_client *slave;
> @@ -217,7 +228,17 @@ static void rcar_i2c_init(struct rcar_i2c_priv *priv)
> rcar_i2c_write(priv, ICMCR, MDBS);
> rcar_i2c_write(priv, ICMSR, 0);
> /* start clock */
> - rcar_i2c_write(priv, ICCCR, priv->icccr);
> + if (priv->flags & ID_P_FMPLUS) {
> + rcar_i2c_write(priv, ICCCR, 0);
> + rcar_i2c_write(priv, ICMPR, priv->clock_val);
> + rcar_i2c_write(priv, ICHPR, 3 * priv->clock_val);
> + rcar_i2c_write(priv, ICLPR, 3 * priv->clock_val);
> + rcar_i2c_write(priv, ICCCR2, FMPE | CDFD | HLSE | SME);
ICCCR2 note 1: "ICCCR2 should be written to prior to writing ICCCR."
> + } else {
> + rcar_i2c_write(priv, ICCCR, priv->clock_val);
> + if (priv->devtype >= I2C_RCAR_GEN3)
> + rcar_i2c_write(priv, ICCCR2, 0);
Likewise.
> + }
>
> if (priv->devtype >= I2C_RCAR_GEN3)
> rcar_i2c_write(priv, ICFBSCR, TCYC17);
> @@ -242,7 +263,7 @@ static int rcar_i2c_bus_barrier(struct rcar_i2c_priv *priv)
>
> static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
> {
> - u32 scgd, cdf, round, ick, sum, scl, cdf_width;
> + u32 scgd, cdf = 0, round, ick, sum, scl, cdf_width, smd;
> unsigned long rate;
> struct device *dev = rcar_i2c_priv_to_dev(priv);
> struct i2c_timings t = {
> @@ -252,19 +273,26 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
> .scl_int_delay_ns = 50,
> };
>
> - cdf_width = (priv->devtype == I2C_RCAR_GEN1) ? 2 : 3;
> -
> /* Fall back to previously used values if not supplied */
> i2c_parse_fw_timings(dev, &t, false);
>
> + if (t.bus_freq_hz > I2C_MAX_FAST_MODE_FREQ &&
> + priv->devtype >= I2C_RCAR_GEN4)
> + priv->flags |= ID_P_FMPLUS;
> + else
> + priv->flags &= ~ID_P_FMPLUS;
> +
> /*
> * calculate SCL clock
> * see
> - * ICCCR
> + * ICCCR (and ICCCR2 for FastMode+)
> *
> * ick = clkp / (1 + CDF)
> * SCL = ick / (20 + SCGD * 8 + F[(ticf + tr + intd) * ick])
> *
> + * for FastMode+:
> + * SCL = clkp / (8 + SMD * 2 + SCLD + SCHD +F[(ticf + tr + intd) * clkp])
> + *
> * ick : I2C internal clock < 20 MHz
> * ticf : I2C SCL falling time
> * tr : I2C SCL rising time
> @@ -273,10 +301,14 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
> * F[] : integer up-valuation
> */
> rate = clk_get_rate(priv->clk);
> - cdf = rate / 20000000;
> - if (cdf >= 1U << cdf_width) {
> - dev_err(dev, "Input clock %lu too high\n", rate);
> - return -EIO;
> +
> + if (!(priv->flags & ID_P_FMPLUS)) {
> + cdf = rate / 20000000;
> + cdf_width = (priv->devtype == I2C_RCAR_GEN1) ? 2 : 3;
> + if (cdf >= 1U << cdf_width) {
> + dev_err(dev, "Input clock %lu too high\n", rate);
> + return -EIO;
> + }
> }
> ick = rate / (cdf + 1);
In case of FM+, cdf will be zero, and ick == rate?
> @@ -292,34 +324,55 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
> round = (ick + 500000) / 1000000 * sum;
ick == rate if FM+
> round = (round + 500) / 1000;
DIV_ROUND_UP()
>
> - /*
> - * SCL = ick / (20 + SCGD * 8 + F[(ticf + tr + intd) * ick])
> - *
> - * Calculation result (= SCL) should be less than
> - * bus_speed for hardware safety
> - *
> - * We could use something along the lines of
> - * div = ick / (bus_speed + 1) + 1;
> - * scgd = (div - 20 - round + 7) / 8;
> - * scl = ick / (20 + (scgd * 8) + round);
> - * (not fully verified) but that would get pretty involved
> - */
> - for (scgd = 0; scgd < 0x40; scgd++) {
> - scl = ick / (20 + (scgd * 8) + round);
> - if (scl <= t.bus_freq_hz)
> - break;
> - }
> + if (priv->flags & ID_P_FMPLUS) {
IIUIC, on R-ar Gen3 and later you can use ICCCR2 without FM+, for
improved accuracy, too?
> + /*
> + * SMD should be smaller than SCLD and SCHD, we arbitrarily set
> + * the ratio 1:3. SCHD:SCLD ratio is 1:1, thus:
> + * SCL = clkp / (8 + SMD * 2 + SCLD + SCHD + F[(ticf + tr + intd) * clkp])
> + * SCL = clkp / (8 + SMD * 2 + SMD * 3 + SMD * 3 + F[...])
> + * SCL = clkp / (8 + SMD * 8 + F[...])
> + */
> + smd = DIV_ROUND_UP(ick / t.bus_freq_hz - 8 - round, 8);
Perhaps use rate instead of ick?
DIV_ROUND_UP(ick, 8 * (t.bus_freq_hz - 8 - round));
> + scl = ick / (8 + 8 * smd + round);
DIV_ROUND_UP()?
>
> - if (scgd == 0x40) {
> - dev_err(dev, "it is impossible to calculate best SCL\n");
> - return -EIO;
> - }
> + if (smd > 0xff) {
> + dev_err(dev, "it is impossible to calculate best SCL\n");
> + return -EINVAL;
Perhaps some "goto error", to share with the error handling for non-FM+?
> + }
>
> - dev_dbg(dev, "clk %d/%d(%lu), round %u, CDF:0x%x, SCGD: 0x%x\n",
> - scl, t.bus_freq_hz, rate, round, cdf, scgd);
> + dev_dbg(dev, "clk %d/%d(%lu), round %u, SMD:0x%x, SCHD: 0x%x\n",
%u/%u
Perhaps it makes more sense to print SMD and SCHD in decimal?
This also applies to the existing code (CDF/SCGD) you moved into
the else branch.
> + scl, t.bus_freq_hz, rate, round, smd, 3 * smd);
>
> - /* keep icccr value */
> - priv->icccr = scgd << cdf_width | cdf;
> + priv->clock_val = smd;
> + } else {
> + /*
> + * SCL = ick / (20 + SCGD * 8 + F[(ticf + tr + intd) * ick])
> + *
> + * Calculation result (= SCL) should be less than
> + * bus_speed for hardware safety
> + *
> + * We could use something along the lines of
> + * div = ick / (bus_speed + 1) + 1;
> + * scgd = (div - 20 - round + 7) / 8;
> + * scl = ick / (20 + (scgd * 8) + round);
> + * (not fully verified) but that would get pretty involved
> + */
> + for (scgd = 0; scgd < 0x40; scgd++) {
> + scl = ick / (20 + (scgd * 8) + round);
> + if (scl <= t.bus_freq_hz)
> + break;
> + }
> +
> + if (scgd == 0x40) {
> + dev_err(dev, "it is impossible to calculate best SCL\n");
> + return -EINVAL;
This was -EIO before.
> + }
> +
> + dev_dbg(dev, "clk %d/%d(%lu), round %u, CDF:0x%x, SCGD: 0x%x\n",
> + scl, t.bus_freq_hz, rate, round, cdf, scgd);
> +
> + priv->clock_val = scgd << cdf_width | cdf;
> + }
>
> return 0;
> }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Geert,
> > - u32 icccr;
> > + u32 clock_val;
>
> Perhaps use a union to store either icccr or smd?
Yup, can do.
>
> > u8 recovery_icmcr; /* protected by adapter lock */
> > enum rcar_i2c_type devtype;
> > struct i2c_client *slave;
> > @@ -217,7 +228,17 @@ static void rcar_i2c_init(struct rcar_i2c_priv *priv)
> > rcar_i2c_write(priv, ICMCR, MDBS);
> > rcar_i2c_write(priv, ICMSR, 0);
> > /* start clock */
> > - rcar_i2c_write(priv, ICCCR, priv->icccr);
> > + if (priv->flags & ID_P_FMPLUS) {
> > + rcar_i2c_write(priv, ICCCR, 0);
> > + rcar_i2c_write(priv, ICMPR, priv->clock_val);
> > + rcar_i2c_write(priv, ICHPR, 3 * priv->clock_val);
> > + rcar_i2c_write(priv, ICLPR, 3 * priv->clock_val);
> > + rcar_i2c_write(priv, ICCCR2, FMPE | CDFD | HLSE | SME);
>
> ICCCR2 note 1: "ICCCR2 should be written to prior to writing ICCCR."
Eeks, I remembered it the other way around :/
> > ick = rate / (cdf + 1);
>
> In case of FM+, cdf will be zero, and ick == rate?
Yes.
>
> > @@ -292,34 +324,55 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
> > round = (ick + 500000) / 1000000 * sum;
>
> ick == rate if FM+
Yes, does this induce a change here?
> > round = (round + 500) / 1000;
>
> DIV_ROUND_UP()
DIV_ROUND_CLOSEST() I'd say, but I have a seperate patch for that.
> > + if (priv->flags & ID_P_FMPLUS) {
>
> IIUIC, on R-ar Gen3 and later you can use ICCCR2 without FM+, for
> improved accuracy, too?
Yeah, we could do that. It indeed improves accuracy:
old new
100kHz: 97680/100000 99950/100000
400kHz: 373482/400000 399201/400000
Caring about regressions here is a bit over the top, or?
> > + /*
> > + * SMD should be smaller than SCLD and SCHD, we arbitrarily set
> > + * the ratio 1:3. SCHD:SCLD ratio is 1:1, thus:
> > + * SCL = clkp / (8 + SMD * 2 + SCLD + SCHD + F[(ticf + tr + intd) * clkp])
> > + * SCL = clkp / (8 + SMD * 2 + SMD * 3 + SMD * 3 + F[...])
> > + * SCL = clkp / (8 + SMD * 8 + F[...])
> > + */
> > + smd = DIV_ROUND_UP(ick / t.bus_freq_hz - 8 - round, 8);
>
> Perhaps use rate instead of ick?
That's probably cleaner.
> DIV_ROUND_UP(ick, 8 * (t.bus_freq_hz - 8 - round));
This looks like you assumed "ick / (t.bus_freq_hz - 8 - round)" but it
is "(ick / t.bus_freq_hz) - 8 - round"?
> > + scl = ick / (8 + 8 * smd + round);
>
> DIV_ROUND_UP()?
Okay.
> > + if (smd > 0xff) {
> > + dev_err(dev, "it is impossible to calculate best SCL\n");
> > + return -EINVAL;
>
> Perhaps some "goto error", to share with the error handling for non-FM+?
I will check with the refactored code.
> > - dev_dbg(dev, "clk %d/%d(%lu), round %u, CDF:0x%x, SCGD: 0x%x\n",
> > - scl, t.bus_freq_hz, rate, round, cdf, scgd);
> > + dev_dbg(dev, "clk %d/%d(%lu), round %u, SMD:0x%x, SCHD: 0x%x\n",
>
> %u/%u
>
> Perhaps it makes more sense to print SMD and SCHD in decimal?
>
> This also applies to the existing code (CDF/SCGD) you moved into
> the else branch.
Can do. I don't care it is debug output.
> > + if (scgd == 0x40) {
> > + dev_err(dev, "it is impossible to calculate best SCL\n");
> > + return -EINVAL;
>
> This was -EIO before.
I'll squash this into a seperate cleanup patch I have.
Thanks,
Wolfram
Hi Wolfram,
On Wed, Sep 6, 2023 at 2:11 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > > ick = rate / (cdf + 1);
> >
> > In case of FM+, cdf will be zero, and ick == rate?
>
> Yes.
>
> > > @@ -292,34 +324,55 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
> > > round = (ick + 500000) / 1000000 * sum;
> >
> > ick == rate if FM+
>
> Yes, does this induce a change here?
No, just pointing it out, and wondering if this is intended...
>
> > > round = (round + 500) / 1000;
> >
> > DIV_ROUND_UP()
>
> DIV_ROUND_CLOSEST() I'd say, but I have a seperate patch for that.
Oops (it's too hot here for more coffee...)
> > > + if (priv->flags & ID_P_FMPLUS) {
> >
> > IIUIC, on R-ar Gen3 and later you can use ICCCR2 without FM+, for
> > improved accuracy, too?
>
> Yeah, we could do that. It indeed improves accuracy:
>
> old new
> 100kHz: 97680/100000 99950/100000
> 400kHz: 373482/400000 399201/400000
>
> Caring about regressions here is a bit over the top, or?
Probably OK.
> > > + /*
> > > + * SMD should be smaller than SCLD and SCHD, we arbitrarily set
> > > + * the ratio 1:3. SCHD:SCLD ratio is 1:1, thus:
> > > + * SCL = clkp / (8 + SMD * 2 + SCLD + SCHD + F[(ticf + tr + intd) * clkp])
> > > + * SCL = clkp / (8 + SMD * 2 + SMD * 3 + SMD * 3 + F[...])
> > > + * SCL = clkp / (8 + SMD * 8 + F[...])
> > > + */
> > > + smd = DIV_ROUND_UP(ick / t.bus_freq_hz - 8 - round, 8);
> >
> > Perhaps use rate instead of ick?
>
> That's probably cleaner.
>
> > DIV_ROUND_UP(ick, 8 * (t.bus_freq_hz - 8 - round));
>
> This looks like you assumed "ick / (t.bus_freq_hz - 8 - round)" but it
> is "(ick / t.bus_freq_hz) - 8 - round"?
Oops (again)
OK do you need rounding for the division of ick and t.bus_freq_hz,
or is the adjustment bij "- (round + 8)" already taking care of that?
I guess I just don't understand the intended formula here...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
> OK do you need rounding for the division of ick and t.bus_freq_hz, > or is the adjustment bij "- (round + 8)" already taking care of that? Unless I overlooked something, it is the latter. The new formula produces the same values for 100 and 400kHz as the old one. > I guess I just don't understand the intended formula here... Ok, needs more documentation then.
Hi Wolfram,
[...]
> @@ -217,7 +228,17 @@ static void rcar_i2c_init(struct rcar_i2c_priv *priv)
> rcar_i2c_write(priv, ICMCR, MDBS);
> rcar_i2c_write(priv, ICMSR, 0);
> /* start clock */
> - rcar_i2c_write(priv, ICCCR, priv->icccr);
> + if (priv->flags & ID_P_FMPLUS) {
> + rcar_i2c_write(priv, ICCCR, 0);
> + rcar_i2c_write(priv, ICMPR, priv->clock_val);
> + rcar_i2c_write(priv, ICHPR, 3 * priv->clock_val);
> + rcar_i2c_write(priv, ICLPR, 3 * priv->clock_val);
> + rcar_i2c_write(priv, ICCCR2, FMPE | CDFD | HLSE | SME);
> + } else {
> + rcar_i2c_write(priv, ICCCR, priv->clock_val);
> + if (priv->devtype >= I2C_RCAR_GEN3)
> + rcar_i2c_write(priv, ICCCR2, 0);
is this last bit part of the FM+ enabling or is it part of the
GEN4 support?
> + }
[...]
> + if (priv->flags & ID_P_FMPLUS) {
> + /*
> + * SMD should be smaller than SCLD and SCHD, we arbitrarily set
> + * the ratio 1:3. SCHD:SCLD ratio is 1:1, thus:
> + * SCL = clkp / (8 + SMD * 2 + SCLD + SCHD + F[(ticf + tr + intd) * clkp])
> + * SCL = clkp / (8 + SMD * 2 + SMD * 3 + SMD * 3 + F[...])
> + * SCL = clkp / (8 + SMD * 8 + F[...])
> + */
> + smd = DIV_ROUND_UP(ick / t.bus_freq_hz - 8 - round, 8);
> + scl = ick / (8 + 8 * smd + round);
>
> - if (scgd == 0x40) {
> - dev_err(dev, "it is impossible to calculate best SCL\n");
> - return -EIO;
> - }
> + if (smd > 0xff) {
> + dev_err(dev, "it is impossible to calculate best SCL\n");
> + return -EINVAL;
> + }
>
> - dev_dbg(dev, "clk %d/%d(%lu), round %u, CDF:0x%x, SCGD: 0x%x\n",
> - scl, t.bus_freq_hz, rate, round, cdf, scgd);
> + dev_dbg(dev, "clk %d/%d(%lu), round %u, SMD:0x%x, SCHD: 0x%x\n",
> + scl, t.bus_freq_hz, rate, round, smd, 3 * smd);
I trust the formula application is right here... I can't say much :)
I don't see anything odd here.
>
> - /* keep icccr value */
> - priv->icccr = scgd << cdf_width | cdf;
> + priv->clock_val = smd;
> + } else {
> + /*
> + * SCL = ick / (20 + SCGD * 8 + F[(ticf + tr + intd) * ick])
> + *
> + * Calculation result (= SCL) should be less than
> + * bus_speed for hardware safety
> + *
> + * We could use something along the lines of
> + * div = ick / (bus_speed + 1) + 1;
> + * scgd = (div - 20 - round + 7) / 8;
> + * scl = ick / (20 + (scgd * 8) + round);
> + * (not fully verified) but that would get pretty involved
> + */
> + for (scgd = 0; scgd < 0x40; scgd++) {
> + scl = ick / (20 + (scgd * 8) + round);
> + if (scl <= t.bus_freq_hz)
> + break;
> + }
> +
> + if (scgd == 0x40) {
would be nice to give a meaning to this 0x40 constant... either
having it in a define or a comment, at least.
Andi
> + dev_err(dev, "it is impossible to calculate best SCL\n");
> + return -EINVAL;
> + }
Hi Andi,
> > @@ -217,7 +228,17 @@ static void rcar_i2c_init(struct rcar_i2c_priv *priv)
> > rcar_i2c_write(priv, ICMCR, MDBS);
> > rcar_i2c_write(priv, ICMSR, 0);
> > /* start clock */
> > - rcar_i2c_write(priv, ICCCR, priv->icccr);
> > + if (priv->flags & ID_P_FMPLUS) {
> > + rcar_i2c_write(priv, ICCCR, 0);
> > + rcar_i2c_write(priv, ICMPR, priv->clock_val);
> > + rcar_i2c_write(priv, ICHPR, 3 * priv->clock_val);
> > + rcar_i2c_write(priv, ICLPR, 3 * priv->clock_val);
> > + rcar_i2c_write(priv, ICCCR2, FMPE | CDFD | HLSE | SME);
> > + } else {
> > + rcar_i2c_write(priv, ICCCR, priv->clock_val);
> > + if (priv->devtype >= I2C_RCAR_GEN3)
> > + rcar_i2c_write(priv, ICCCR2, 0);
>
> is this last bit part of the FM+ enabling or is it part of the
> GEN4 support?
It is "disabling FM+" for lower speeds. Since we never used ICCCR2
before FM+, we need to make sure it is cleared properly.
> > + for (scgd = 0; scgd < 0x40; scgd++) {
> > + scl = ick / (20 + (scgd * 8) + round);
> > + if (scl <= t.bus_freq_hz)
> > + break;
> > + }
> > +
> > + if (scgd == 0x40) {
>
> would be nice to give a meaning to this 0x40 constant... either
> having it in a define or a comment, at least.
This code existed before and was just moved into an if-body. It will be
updated in another series following this one.
Thanks for the review,
Wolfram
Hi Wolfram,
On Thu, Sep 7, 2023 at 7:39 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > > @@ -217,7 +228,17 @@ static void rcar_i2c_init(struct rcar_i2c_priv *priv)
> > > rcar_i2c_write(priv, ICMCR, MDBS);
> > > rcar_i2c_write(priv, ICMSR, 0);
> > > /* start clock */
> > > - rcar_i2c_write(priv, ICCCR, priv->icccr);
> > > + if (priv->flags & ID_P_FMPLUS) {
> > > + rcar_i2c_write(priv, ICCCR, 0);
> > > + rcar_i2c_write(priv, ICMPR, priv->clock_val);
> > > + rcar_i2c_write(priv, ICHPR, 3 * priv->clock_val);
> > > + rcar_i2c_write(priv, ICLPR, 3 * priv->clock_val);
> > > + rcar_i2c_write(priv, ICCCR2, FMPE | CDFD | HLSE | SME);
> > > + } else {
> > > + rcar_i2c_write(priv, ICCCR, priv->clock_val);
> > > + if (priv->devtype >= I2C_RCAR_GEN3)
> > > + rcar_i2c_write(priv, ICCCR2, 0);
> >
> > is this last bit part of the FM+ enabling or is it part of the
> > GEN4 support?
>
> It is "disabling FM+" for lower speeds. Since we never used ICCCR2
> before FM+, we need to make sure it is cleared properly.
This may become clearer if you first introduce support for ICCCR2
on R-Car Gen3 and later, to improve i2c rate accuracy, and add
support for FM+ in a separate patch?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
> This may become clearer if you first introduce support for ICCCR2 > on R-Car Gen3 and later, to improve i2c rate accuracy, and add > support for FM+ in a separate patch? That's my plan for the next revision of this series.
Hi Wolfram,
> > > @@ -217,7 +228,17 @@ static void rcar_i2c_init(struct rcar_i2c_priv *priv)
> > > rcar_i2c_write(priv, ICMCR, MDBS);
> > > rcar_i2c_write(priv, ICMSR, 0);
> > > /* start clock */
> > > - rcar_i2c_write(priv, ICCCR, priv->icccr);
> > > + if (priv->flags & ID_P_FMPLUS) {
> > > + rcar_i2c_write(priv, ICCCR, 0);
> > > + rcar_i2c_write(priv, ICMPR, priv->clock_val);
> > > + rcar_i2c_write(priv, ICHPR, 3 * priv->clock_val);
> > > + rcar_i2c_write(priv, ICLPR, 3 * priv->clock_val);
> > > + rcar_i2c_write(priv, ICCCR2, FMPE | CDFD | HLSE | SME);
> > > + } else {
> > > + rcar_i2c_write(priv, ICCCR, priv->clock_val);
> > > + if (priv->devtype >= I2C_RCAR_GEN3)
> > > + rcar_i2c_write(priv, ICCCR2, 0);
> >
> > is this last bit part of the FM+ enabling or is it part of the
> > GEN4 support?
>
> It is "disabling FM+" for lower speeds. Since we never used ICCCR2
> before FM+, we need to make sure it is cleared properly.
OK... I'm missing some hardware details here :)
> > > + for (scgd = 0; scgd < 0x40; scgd++) {
> > > + scl = ick / (20 + (scgd * 8) + round);
> > > + if (scl <= t.bus_freq_hz)
> > > + break;
> > > + }
> > > +
> > > + if (scgd == 0x40) {
> >
> > would be nice to give a meaning to this 0x40 constant... either
> > having it in a define or a comment, at least.
>
> This code existed before and was just moved into an if-body. It will be
> updated in another series following this one.
OK, thanks!
Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
Andi
© 2016 - 2026 Red Hat, Inc.