| Message ID | 1475085056-5205-8-git-send-email-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Noralf Trønnes <noralf@tronnes.org> writes: > Support a dynamic clock by reading the frequency and setting the > divisor in the transfer function instead of during probe. Is this fixing some particular case you could note in the commit message? As is, it makes me think that we should be using a notifier for when the parent clock (that's the one I assume you're talking about being dynamic) changes. But maybe you're working around a variable VPU clock being set by the firmware, because we don't have a notifier for it? I'm a bit worried because I think this is going to be pretty expensive to be doing per transfer.
Den 28.09.2016 23:24, skrev Eric Anholt: > Noralf Trønnes <noralf@tronnes.org> writes: > >> Support a dynamic clock by reading the frequency and setting the >> divisor in the transfer function instead of during probe. > Is this fixing some particular case you could note in the commit > message? As is, it makes me think that we should be using a notifier > for when the parent clock (that's the one I assume you're talking about > being dynamic) changes. But maybe you're working around a variable VPU > clock being set by the firmware, because we don't have a notifier for > it? > > I'm a bit worried because I think this is going to be pretty expensive > to be doing per transfer. Actually I thought that this was a known problem, but trying to track it down now, I can't find anything definitive. I don't have any tool to look at the bus signals, so I couldn't actually test it. Sorry about this, we can just drop this patch. Noralf.
On 28.09.2016 23:24, Eric Anholt wrote: > Noralf Trønnes <noralf@tronnes.org> writes: > >> Support a dynamic clock by reading the frequency and setting the >> divisor in the transfer function instead of during probe. > Is this fixing some particular case you could note in the commit > message? As is, it makes me think that we should be using a notifier > for when the parent clock (that's the one I assume you're talking about > being dynamic) changes. But maybe you're working around a variable VPU > clock being set by the firmware, because we don't have a notifier for > it? > > I'm a bit worried because I think this is going to be pretty expensive > to be doing per transfer. Well, the clocks are all configured without CLK_GET_RATE_NOCACHE et. al., so the value is read from cache, which is not (very) expensive (see clk_core_get_rate). This also means that any clock change of the VPU done by the firmware does not propagate to the linux kernel anyway and the unchanged cached value is returned. To make this work it would require a notification mechanism from the firmware to trigger a re-validation of all the caches. (or some sort of watchdog process). Adding a notifier to each driver (I2C, SPI) instead is - imo - a lot of unnecessary code complexity, as any currently running transfer would still be impacted, because changing the clock-divider in flight is a asking for trouble. But then changing the vpu-clock speed while a I2s/SPI/... transfer is running is also asking for trouble.... The only place where - IMO - a notifier would make sense is with the auxiliar UART driver(8250_bcm2835aux.c), as there we only read the clock rates when setting/changing the baud rate. But - again - this would require some notification by the firmware in the first place and any reception in the window of change would go wrong because of unexpected effective baud rate changes. So as far as I can tell the change to read the current clock rate in the transfer function is a reasonable approach and the clock framework should handle the communication with the firmware about such changes. (And I remember having had some discussions around this subject with Phil Elwell or popcornmix some time ago on github where it boiled down to: what is the "right" interface? - I can not find the reference right now) Reviewed-by: Martin Sperl <kernel@martin.sperl.org> Thanks, Martin
diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c index 6a3e753..2b6b682 100644 --- a/drivers/i2c/busses/i2c-bcm2835.c +++ b/drivers/i2c/busses/i2c-bcm2835.c @@ -58,6 +58,7 @@ struct bcm2835_i2c_dev { void __iomem *regs; struct clk *clk; int irq; + u32 bus_clk_rate; struct i2c_adapter adapter; struct completion completion; struct i2c_msg *curr_msg; @@ -78,6 +79,30 @@ static inline u32 bcm2835_i2c_readl(struct bcm2835_i2c_dev *i2c_dev, u32 reg) return readl(i2c_dev->regs + reg); } +static int bcm2835_i2c_set_divider(struct bcm2835_i2c_dev *i2c_dev) +{ + u32 divider; + + divider = DIV_ROUND_UP(clk_get_rate(i2c_dev->clk), + i2c_dev->bus_clk_rate); + /* + * 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_MIN) || + (divider > BCM2835_I2C_CDIV_MAX)) { + dev_err_ratelimited(i2c_dev->dev, "Invalid clock-frequency\n"); + return -EINVAL; + } + + bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DIV, divider); + + return 0; +} + static void bcm2835_fill_txfifo(struct bcm2835_i2c_dev *i2c_dev) { u32 val; @@ -215,7 +240,7 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], { struct bcm2835_i2c_dev *i2c_dev = i2c_get_adapdata(adap); unsigned long time_left; - int i; + int i, ret; for (i = 0; i < (num - 1); i++) if (msgs[i].flags & I2C_M_RD) { @@ -224,6 +249,10 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], return -EOPNOTSUPP; } + ret = bcm2835_i2c_set_divider(i2c_dev); + if (ret) + return ret; + i2c_dev->curr_msg = msgs; i2c_dev->num_msgs = num; reinit_completion(&i2c_dev->completion); @@ -273,7 +302,6 @@ static int bcm2835_i2c_probe(struct platform_device *pdev) { struct bcm2835_i2c_dev *i2c_dev; struct resource *mem, *irq; - u32 bus_clk_rate, divider; int ret; struct i2c_adapter *adap; @@ -297,27 +325,12 @@ static int bcm2835_i2c_probe(struct platform_device *pdev) } ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency", - &bus_clk_rate); + &i2c_dev->bus_clk_rate); if (ret < 0) { dev_warn(&pdev->dev, "Could not read clock-frequency property\n"); - bus_clk_rate = 100000; - } - - divider = DIV_ROUND_UP(clk_get_rate(i2c_dev->clk), bus_clk_rate); - /* - * 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_MIN) || - (divider > BCM2835_I2C_CDIV_MAX)) { - dev_err(&pdev->dev, "Invalid clock-frequency\n"); - return -ENODEV; + i2c_dev->bus_clk_rate = 100000; } - bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DIV, divider); irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); if (!irq) {
Support a dynamic clock by reading the frequency and setting the divisor in the transfer function instead of during probe. Signed-off-by: Noralf Trønnes <noralf@tronnes.org> --- drivers/i2c/busses/i2c-bcm2835.c | 51 +++++++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 19 deletions(-)