| Message ID | f030f08f72dbcf83887013d000c1e2a1a9ffac81.1705324589.git.geert+renesas@glider.be |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCH/RFC] i2c: sh_mobile: Switch R-Mobile A1/APE6 and SH-Mobile AG5 to new frequency calculation | expand |
Hi Geert, On Mon, Jan 15, 2024 at 02:18:12PM +0100, Geert Uytterhoeven wrote: > Switch the R-Mobile A1, R-Mobile APE6, and SH-Mobile AG5 SoCs to the new > frequency calculation formula, to (a) avoid running the I2C bus too fast, > and (b) bring the low/high ratio closer to the recommended ratio 5/4. > > Measurement results on R-Mobile APE6 and SH-Mobile AG5 (fck=104 MHz, > clks_per_count=2): > 100 kHz: 106 kHz LH=1.12 before, 99.6 kHz L/H=1.22 after > 400 kHz: 384 kHz LH=1.67 before, 392 kHz L/H=1.27 after > > Measurement results on R-Mobile A1 (fck=49.5 MHz, clks_per_count=1): > 100 kHz: 106 kHz L/H=1.09 before, 99.6 kHz L/H=1.20 after > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> Looks good to me, and it seems to comply with the datasheets? (I haven't checked) > Notes: > - fast_clock_dt_config is now unused, so I guess we should drop it, > and rename v2_freq_calc_dt_config to fast_clock_dt_config? Yes, we can (and should do that). > - The formulas in the documentation for R-Mobile A1 do not include the > "-1" and "-5", presumably this is for rounding? Sounds reasonable. The formulas evolved over time :) > - The formulas in the documentation for SH-Mobile AG5 include > the "-1" and "-5" in the example for 400 kHz, but not in the example > for 100 kHz. *shrug* > - The SH-Mobile AG5 documentation suggests to use an L/H ratio of 5/3 > instead of 5/4 for 400 kHz, which means the old formula is better > for 400 kHz? Both ratios are within the I2C specifications for minimum SCL lo/hi time. 5/3 is a little further away from the minimum requirements, so maybe a tad more robust. But both should work(tm). > - Remaining users of the old formula are sh7343, sh7366, and sh772[234]. > At least for sh772[234], the "Transfer Rate" formula in the > documentation is the same as for R-Mobile A1. Hence probably we > should switch the default_dt_config and the default fallback to the > "v2" formula, too? I think we could. But touching old systems without testing and a need is more risky than leaving them alone. > - I am still a bit puzzled, as the "v2" formula introduced in commit > 4ecfb9d3b229fff5 ("i2c: sh_mobile: add new frequency calculation for > later SoC") is basically what the driver did before commit > 23a612916a51cc37 ("i2c: i2c-sh_mobile: optimize ICCH/ICCL values > according to I2C bus speed")? Interesting. I can't recall that I analyzed the back then existing formula when I implemented v2. I recall that I used R-Car Gen2 datasheets as the reference. I probably thought those are the "new" algorithms. I likely didn't have the old datasheets back then. Still, my memory is not clear about this. Obvious to say, this I2C core is not used anymore in newer R-Car SoCs. So, I won't dive too deep into it... Happy hacking, Wolfram
diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c index 5adbe62cf6212865..9e2e12d482d8e640 100644 --- a/drivers/i2c/busses/i2c-sh_mobile.c +++ b/drivers/i2c/busses/i2c-sh_mobile.c @@ -773,7 +773,7 @@ static int sh_mobile_i2c_r8a7740_workaround(struct sh_mobile_i2c_data *pd) iic_wr(pd, ICCR, ICCR_TRS); udelay(10); - return sh_mobile_i2c_init(pd); + return sh_mobile_i2c_v2_init(pd); } static const struct sh_mobile_dt_config default_dt_config = { @@ -797,7 +797,7 @@ static const struct sh_mobile_dt_config r8a7740_dt_config = { }; static const struct of_device_id sh_mobile_i2c_dt_ids[] = { - { .compatible = "renesas,iic-r8a73a4", .data = &fast_clock_dt_config }, + { .compatible = "renesas,iic-r8a73a4", .data = &v2_freq_calc_dt_config }, { .compatible = "renesas,iic-r8a7740", .data = &r8a7740_dt_config }, { .compatible = "renesas,iic-r8a774c0", .data = &v2_freq_calc_dt_config }, { .compatible = "renesas,iic-r8a7790", .data = &v2_freq_calc_dt_config }, @@ -807,7 +807,7 @@ static const struct of_device_id sh_mobile_i2c_dt_ids[] = { { .compatible = "renesas,iic-r8a7794", .data = &v2_freq_calc_dt_config }, { .compatible = "renesas,iic-r8a7795", .data = &v2_freq_calc_dt_config }, { .compatible = "renesas,iic-r8a77990", .data = &v2_freq_calc_dt_config }, - { .compatible = "renesas,iic-sh73a0", .data = &fast_clock_dt_config }, + { .compatible = "renesas,iic-sh73a0", .data = &v2_freq_calc_dt_config }, { .compatible = "renesas,rcar-gen2-iic", .data = &v2_freq_calc_dt_config }, { .compatible = "renesas,rcar-gen3-iic", .data = &v2_freq_calc_dt_config }, { .compatible = "renesas,rmobile-iic", .data = &default_dt_config },
Switch the R-Mobile A1, R-Mobile APE6, and SH-Mobile AG5 SoCs to the new frequency calculation formula, to (a) avoid running the I2C bus too fast, and (b) bring the low/high ratio closer to the recommended ratio 5/4. Measurement results on R-Mobile APE6 and SH-Mobile AG5 (fck=104 MHz, clks_per_count=2): 100 kHz: 106 kHz LH=1.12 before, 99.6 kHz L/H=1.22 after 400 kHz: 384 kHz LH=1.67 before, 392 kHz L/H=1.27 after Measurement results on R-Mobile A1 (fck=49.5 MHz, clks_per_count=1): 100 kHz: 106 kHz L/H=1.09 before, 99.6 kHz L/H=1.20 after Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- Measurements after applying "[PATCH 1/3] ARM: dts: renesas: r8a73a4: Fix external clocks and clock rate"[1]. Before, the clock rates were even 4% higher. Notes: - fast_clock_dt_config is now unused, so I guess we should drop it, and rename v2_freq_calc_dt_config to fast_clock_dt_config? - The formulas in the documentation for R-Mobile A1 do not include the "-1" and "-5", presumably this is for rounding? - The formulas in the documentation for SH-Mobile AG5 include the "-1" and "-5" in the example for 400 kHz, but not in the example for 100 kHz. - The SH-Mobile AG5 documentation suggests to use an L/H ratio of 5/3 instead of 5/4 for 400 kHz, which means the old formula is better for 400 kHz? - Remaining users of the old formula are sh7343, sh7366, and sh772[234]. At least for sh772[234], the "Transfer Rate" formula in the documentation is the same as for R-Mobile A1. Hence probably we should switch the default_dt_config and the default fallback to the "v2" formula, too? - I am still a bit puzzled, as the "v2" formula introduced in commit 4ecfb9d3b229fff5 ("i2c: sh_mobile: add new frequency calculation for later SoC") is basically what the driver did before commit 23a612916a51cc37 ("i2c: i2c-sh_mobile: optimize ICCH/ICCL values according to I2C bus speed")? [1] https://lore.kernel.org/r/1692bc8cd465d62168cbf110522ad62a7af3f606.1705315614.git.geert+renesas@glider.be --- drivers/i2c/busses/i2c-sh_mobile.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)