From mboxrd@z Thu Jan 1 00:00:00 1970 From: Silvan Wicki Subject: Re: [PATCH v2] i2c: busses: i2c-bcm2835: limits cdiv to allowed values Date: Tue, 2 Jun 2015 17:45:06 +0200 Message-ID: <20150602154506.GA2341@jessie.tinag.lan> References: <1432895204-19924-1-git-send-email-linux_wi@tinag.ch> <87pp5f5mlw.fsf@eliezer.anholt.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <87pp5f5mlw.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org> Sender: linux-i2c-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Eric Anholt Cc: wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org, linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: linux-i2c@vger.kernel.org On Mon, Jun 01, 2015 at 11:07:07AM -0700, Eric Anholt wrote: > Silvan Wicki 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 > > --- > > 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.