| Message ID | 1432895204-19924-1-git-send-email-linux_wi@tinag.ch |
|---|---|
| State | Superseded |
| Headers | show |
Silvan Wicki <linux_wi@tinag.ch> writes: > 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 > > Signed-off-by: Silvan Wicki <linux_wi@tinag.ch> > --- > drivers/i2c/busses/i2c-bcm2835.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c > index c9336a3..8195b04 100644 > --- a/drivers/i2c/busses/i2c-bcm2835.c > +++ b/drivers/i2c/busses/i2c-bcm2835.c > @@ -50,6 +50,9 @@ > #define BCM2835_I2C_S_CLKT BIT(9) > #define BCM2835_I2C_S_LEN BIT(10) /* Fake bit for SW error reporting */ > > +#define BCM2835_I2C_CDIV_MIN 0x0002 > +#define BCM2835_I2C_CDIV_MAX 0xFFFE > + > #define BCM2835_I2C_TIMEOUT (msecs_to_jiffies(1000)) > > struct bcm2835_i2c_dev { > @@ -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. > + */ > + if (divider < BCM2835_I2C_CDIV_MIN) > + divider = BCM2835_I2C_CDIV_MIN; > + /* > * 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; > bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DIV, divider); I'm not clear on the motivation of this patch. If the bus_clk_rate is something totally unreasonable (125MHz? 3.8KHz?) it seems like something is misconfigured and we should at best just fail the probe. Does this fix some particular problem you've run into?
On Mon, Jun 01, 2015 at 11:07:07AM -0700, Eric Anholt wrote: > Silvan Wicki <linux_wi@tinag.ch> writes: > > > 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 > > > > Signed-off-by: Silvan Wicki <linux_wi@tinag.ch> > > --- > > drivers/i2c/busses/i2c-bcm2835.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c > > index c9336a3..8195b04 100644 > > --- a/drivers/i2c/busses/i2c-bcm2835.c > > +++ b/drivers/i2c/busses/i2c-bcm2835.c > > @@ -50,6 +50,9 @@ > > #define BCM2835_I2C_S_CLKT BIT(9) > > #define BCM2835_I2C_S_LEN BIT(10) /* Fake bit for SW error reporting */ > > > > +#define BCM2835_I2C_CDIV_MIN 0x0002 > > +#define BCM2835_I2C_CDIV_MAX 0xFFFE > > + > > #define BCM2835_I2C_TIMEOUT (msecs_to_jiffies(1000)) > > > > struct bcm2835_i2c_dev { > > @@ -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. > > + */ > > + if (divider < BCM2835_I2C_CDIV_MIN) > > + divider = BCM2835_I2C_CDIV_MIN; > > + /* > > * 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; > > bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DIV, divider); > > I'm not clear on the motivation of this patch. If the bus_clk_rate is > something totally unreasonable (125MHz? 3.8KHz?) it seems like > something is misconfigured and we should at best just fail the probe. > Does this fix some particular problem you've run into? The motivation is to not allow wrong values to be written in CDIV. No, i was not able to fix my problem. I had to choose another clock rate. Failing the probe would be an alternative. The problem with failing is, when do we fail? I did not find values in the datasheet about what speeds are supported or not. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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'd echo Eric's question: I'm curious whether this patch solves a real-world problem, or if you found it by code inspection? -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c index c9336a3..8195b04 100644 --- a/drivers/i2c/busses/i2c-bcm2835.c +++ b/drivers/i2c/busses/i2c-bcm2835.c @@ -50,6 +50,9 @@ #define BCM2835_I2C_S_CLKT BIT(9) #define BCM2835_I2C_S_LEN BIT(10) /* Fake bit for SW error reporting */ +#define BCM2835_I2C_CDIV_MIN 0x0002 +#define BCM2835_I2C_CDIV_MAX 0xFFFE + #define BCM2835_I2C_TIMEOUT (msecs_to_jiffies(1000)) struct bcm2835_i2c_dev { @@ -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. + */ + if (divider < BCM2835_I2C_CDIV_MIN) + divider = BCM2835_I2C_CDIV_MIN; + /* * 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; bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DIV, divider); irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
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 Signed-off-by: Silvan Wicki <linux_wi@tinag.ch> --- drivers/i2c/busses/i2c-bcm2835.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)