On Thu, Jun 04, 2015 at 09:10:21PM -0600, Stephen Warren wrote: > On 05/29/2015 04:26 AM, Silvan Wicki wrote: > > Adds: make sure bits 16-31 of DIV register are always 0 > > Adds: assume minimal divider of 2 if divider resulted in 0 > > (bcm2835 sets divider to 32768 if cdiv is set to 0) > > > > See page 33/34 of BCM2835-ARM-Peripherals.pdf for the DIV register. > > https://www.raspberrypi.org/wp-content/uploads/2012/02/BCM2835-ARM-Peripherals.pdf > > > diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c > > > @@ -252,12 +255,22 @@ static int bcm2835_i2c_probe(struct platform_device *pdev) > > > > divider = DIV_ROUND_UP(clk_get_rate(i2c_dev->clk), bus_clk_rate); > > /* > > + * Divider results in 0 by extremely high bus_clk_rate values > > + * such as bus_clk_rate >= 4044967297 and core_clock = 250MHz. > > + * In such a case assume the minimal possible divider since > > + * bcm2835 chip sets divisor internally to 32768 if cdiv is 0. > > + */ > > I would rephrase that as: > > /* > * A divider of0 results in extremely high bus_clk_rate values > * such as bus_clk_rate >= 4044967297 with core_clock = 250MHz. > * In such a case assume the minimal possible divider, since > * bcm2835 chip sets divisor internally to 32768 if cdiv is 0. > */ > > > + if (divider < BCM2835_I2C_CDIV_MIN) > > + divider = BCM2835_I2C_CDIV_MIN; > > This seems reasonable; if the calculated divider is too small, then it > means we can't run the clock that fas. Running it more slowly loses some > performance, but should work fine. I might suggest a dev_info() or > dev_dbg() here. > > > + /* > > * Per the datasheet, the register is always interpreted as an even > > * number, by rounding down. In other words, the LSB is ignored. So, > > * if the LSB is set, increment the divider to avoid any issue. > > */ > > if (divider & 1) > > divider++; > > + if (divider > BCM2835_I2C_CDIV_MAX) > > + divider = BCM2835_I2C_CDIV_MAX; > > I think this should be an error. If the calculated divider is too large, > it means we want to run the clock slower than we can. If we run the > clock faster than requested, the clock rate might exceed the attached > I2C devices' capabilities (otherwise, why would we want such a slow > clock?). As such, I'd suggest a dev_err() here, and to fail the probe() > by returning an error. > i made an updated patch, v3. > I'd echo Eric's question: I'm curious whether this patch solves a > real-world problem, or if you found it by code inspection? i used i2c on the raspberry pi with an atmega328p. when i was testing with diffrent i2c speeds it dit not always work as expected. so it was kind of both, a problem i encountered in real-world that lead to code inspection. so i analyzed the driver and found some issues. one thing is this patch with the DIV register. one thing is that in bcm2835_i2c_isr the S register is read and then directly written back without ensuring that the reserved bits (10-31) are cleared. the datasheet mentions: write as 0, read don't care. it is not guaranteed that the reserved bits will always be 0. but we need to write a 0. so the proper way i think is to clear the bits with a bitmask. i think there are more registers which we may need to clear the reserved bits. bitmask for S register would be 0x03FF. one thing is that the DEL register is never adjusted. so if cdiv/2 gets higher than FEDL/REDL the BSC may malfunction. the datasheet mentions: the delay values should always be set to less than cdiv/2. and it would be nice to have the ability to manipulate the CLKT->TOUT value. to change the clock stretching timeout. i'm new kernel development and find it best to do only one patch at a time. cheers -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
