| Message ID | 20190508071227.18609-1-nh6z@nh6z.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] i2c: bcm2835: Model Divider in CCF | expand |
I’m just following up on this since I haven’t heard anything since I submitted the v2 patch a week ago. Wolfram, does this look like a sane approach? Stefan, were my changes in v2 acceptable? I’m happy to go back to the drawing board to change things if there’s a better answer to the problem. Thanks for everyone’s patience with me trying to work through the proper process. -- Annaliese McDermond nh6z@nh6z.net > On May 8, 2019, at 12:12 AM, Annaliese McDermond <nh6z@nh6z.net> wrote: > > Model the I2C bus clock divider as a part of the Core Clock Framework. > Primarily this removes the clk_get_rate() call from each transfer. > This call causes problems for slave drivers that themselves have > internal clock components that are controlled by an I2C interface. > When the slave's internal clock component is prepared, the prepare > lock is obtained, and it makes calls to the I2C subsystem to > command the hardware to activate the clock. In order to perform > the I2C transfer, this driver sets the divider, which requires > it to get the parent clock rate, which it does with clk_get_rate(). > Unfortunately, this function will try to take the clock prepare > lock, which is already held by the slave's internal clock calls > creating a deadlock. > > Modeling the divider in the CCF natively removes this dependency > and the divider value is only set upon changing the bus clock > frequency or changes in the parent clock that cascade down to this > divisor. This obviates the need to set the divider with every > transfer and avoids the deadlock described above. It also should > provide better clock debugging and save a few cycles on each > transfer due to not having to recalcuate the divider value. > > Signed-off-by: Annaliese McDermond <nh6z@nh6z.net> > --- > Changes in v2: > - Remove empty whitespace between declarations > - Preserve comment regarding register rounding > - Consolodate declarations and initial assignments > - Return proper error pointer from bcm2835_i2c_register_div() > - Properly handle return value from bcm2835_i2c_register_div() > in bcm2835_i2c_probe() > - Refactor divider calulation code into separate function > - Use clk_set_rate_exclusive() to ensure the clock is not > modified by other consumers > - Deregister clock in bcm2835_i2c_remove() to properly clean up > after ourselves > > drivers/i2c/busses/i2c-bcm2835.c | 144 ++++++++++++++++++++++++------- > 1 file changed, 113 insertions(+), 31 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c > index d2fbb4bb4a43..e66e569b8f09 100644 > --- a/drivers/i2c/busses/i2c-bcm2835.c > +++ b/drivers/i2c/busses/i2c-bcm2835.c > @@ -4,6 +4,8 @@ > */ > > #include <linux/clk.h> > +#include <linux/clkdev.h> > +#include <linux/clk-provider.h> > #include <linux/completion.h> > #include <linux/err.h> > #include <linux/i2c.h> > @@ -51,9 +53,7 @@ > struct bcm2835_i2c_dev { > struct device *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; > @@ -74,12 +74,17 @@ 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) > +#define to_clk_bcm2835_i2c(_hw) container_of(_hw, struct clk_bcm2835_i2c, hw) > +struct clk_bcm2835_i2c { > + struct clk_hw hw; > + struct bcm2835_i2c_dev *i2c_dev; > +}; > + > +static int clk_bcm2835_i2c_calc_divider(unsigned long rate, > + unsigned long parent_rate) > { > - u32 divider, redl, fedl; > + u32 divider = DIV_ROUND_UP(parent_rate, rate); > > - 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, > @@ -88,12 +93,23 @@ static int bcm2835_i2c_set_divider(struct bcm2835_i2c_dev *i2c_dev) > 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"); > + (divider > BCM2835_I2C_CDIV_MAX)) > return -EINVAL; > - } > > - bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DIV, divider); > + return divider; > +} > + > +static int clk_bcm2835_i2c_set_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long parent_rate) > +{ > + struct clk_bcm2835_i2c *div = to_clk_bcm2835_i2c(hw); > + u32 redl, fedl; > + u32 divider = clk_bcm2835_i2c_calc_divider(rate, parent_rate); > + > + if (divider == -EINVAL) > + return -EINVAL; > + > + bcm2835_i2c_writel(div->i2c_dev, BCM2835_I2C_DIV, divider); > > /* > * Number of core clocks to wait after falling edge before > @@ -108,12 +124,60 @@ static int bcm2835_i2c_set_divider(struct bcm2835_i2c_dev *i2c_dev) > */ > redl = max(divider / 4, 1u); > > - bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DEL, > + bcm2835_i2c_writel(div->i2c_dev, BCM2835_I2C_DEL, > (fedl << BCM2835_I2C_FEDL_SHIFT) | > (redl << BCM2835_I2C_REDL_SHIFT)); > return 0; > } > > +static long clk_bcm2835_i2c_round_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long *parent_rate) > +{ > + u32 divider = clk_bcm2835_i2c_calc_divider(rate, *parent_rate); > + > + return DIV_ROUND_UP(*parent_rate, divider); > +} > + > +static unsigned long clk_bcm2835_i2c_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + struct clk_bcm2835_i2c *div = to_clk_bcm2835_i2c(hw); > + u32 divider = bcm2835_i2c_readl(div->i2c_dev, BCM2835_I2C_DIV); > + > + return DIV_ROUND_UP(parent_rate, divider); > +} > + > +static const struct clk_ops clk_bcm2835_i2c_ops = { > + .set_rate = clk_bcm2835_i2c_set_rate, > + .round_rate = clk_bcm2835_i2c_round_rate, > + .recalc_rate = clk_bcm2835_i2c_recalc_rate, > +}; > + > +static struct clk *bcm2835_i2c_register_div(struct device *dev, > + const char *mclk_name, > + struct bcm2835_i2c_dev *i2c_dev) > +{ > + struct clk_init_data init; > + struct clk_bcm2835_i2c *priv; > + const char *devname = dev_name(dev); > + > + init.ops = &clk_bcm2835_i2c_ops; > + init.name = "bcm2835-i2c"; > + init.parent_names = (const char* []) { mclk_name }; > + init.num_parents = 1; > + init.flags = 0; > + > + priv = devm_kzalloc(dev, sizeof(struct clk_bcm2835_i2c), GFP_KERNEL); > + if (priv == NULL) > + return ERR_PTR(-ENOMEM); > + > + priv->hw.init = &init; > + priv->i2c_dev = i2c_dev; > + > + clk_hw_register_clkdev(&priv->hw, init.name, devname); > + return devm_clk_register(dev, &priv->hw); > +} > + > static void bcm2835_fill_txfifo(struct bcm2835_i2c_dev *i2c_dev) > { > u32 val; > @@ -271,7 +335,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, ret; > + int i; > > for (i = 0; i < (num - 1); i++) > if (msgs[i].flags & I2C_M_RD) { > @@ -280,10 +344,6 @@ 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); > @@ -338,6 +398,9 @@ static int bcm2835_i2c_probe(struct platform_device *pdev) > struct resource *mem, *irq; > int ret; > struct i2c_adapter *adap; > + const char *mclk_name; > + struct clk *bus_clk; > + u32 bus_clk_rate; > > i2c_dev = devm_kzalloc(&pdev->dev, sizeof(*i2c_dev), GFP_KERNEL); > if (!i2c_dev) > @@ -351,21 +414,6 @@ static int bcm2835_i2c_probe(struct platform_device *pdev) > if (IS_ERR(i2c_dev->regs)) > return PTR_ERR(i2c_dev->regs); > > - i2c_dev->clk = devm_clk_get(&pdev->dev, NULL); > - if (IS_ERR(i2c_dev->clk)) { > - if (PTR_ERR(i2c_dev->clk) != -EPROBE_DEFER) > - dev_err(&pdev->dev, "Could not get clock\n"); > - return PTR_ERR(i2c_dev->clk); > - } > - > - ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency", > - &i2c_dev->bus_clk_rate); > - if (ret < 0) { > - dev_warn(&pdev->dev, > - "Could not read clock-frequency property\n"); > - i2c_dev->bus_clk_rate = 100000; > - } > - > irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > if (!irq) { > dev_err(&pdev->dev, "No IRQ resource\n"); > @@ -380,6 +428,35 @@ static int bcm2835_i2c_probe(struct platform_device *pdev) > return -ENODEV; > } > > + mclk_name = of_clk_get_parent_name(pdev->dev.of_node, 0); > + > + bus_clk = bcm2835_i2c_register_div(&pdev->dev, mclk_name, i2c_dev); > + > + if (IS_ERR(bus_clk)) { > + dev_err(&pdev->dev, "Could not register clock\n"); > + return PTR_ERR(bus_clk); > + } > + > + ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency", > + &bus_clk_rate); > + if (ret < 0) { > + dev_warn(&pdev->dev, > + "Could not read clock-frequency property\n"); > + bus_clk_rate = 100000; > + } > + > + ret = clk_set_rate_exclusive(bus_clk, bus_clk_rate); > + if (ret < 0) { > + dev_err(&pdev->dev, "Could not set clock frequency\n"); > + return ret; > + } > + > + ret = clk_prepare_enable(bus_clk); > + if (ret) { > + dev_err(&pdev->dev, "Couldn't prepare clock"); > + return ret; > + } > + > adap = &i2c_dev->adapter; > i2c_set_adapdata(adap, i2c_dev); > adap->owner = THIS_MODULE; > @@ -402,6 +479,11 @@ static int bcm2835_i2c_probe(struct platform_device *pdev) > static int bcm2835_i2c_remove(struct platform_device *pdev) > { > struct bcm2835_i2c_dev *i2c_dev = platform_get_drvdata(pdev); > + struct clk *bus_clk = devm_clk_get(i2c_dev->dev, "bcm2835-i2c"); > + > + clk_rate_exclusive_put(bus_clk); > + clk_disable_unprepare(bus_clk); > + devm_clk_unregister(i2c_dev->dev, bus_clk); > > free_irq(i2c_dev->irq, i2c_dev); > i2c_del_adapter(&i2c_dev->adapter); > -- > 2.19.1 >
Hi Annaliese, On Wed, May 15, 2019 at 10:37:03PM -0700, Annaliese McDermond wrote: > I’m just following up on this since I haven’t heard anything since I submitted the > v2 patch a week ago. Wolfram, does this look like a sane approach? Stefan, > were my changes in v2 acceptable? There is a bit of overhead involved, but conceptually it looks like an elegant solution to me. However, I am not an expert of CCF. Grepping through kernel sources, I don't see many clocks defined outside drivers/clk. So, I'd appreciate if we could get some ack/feedback from one of the CCF experts/maintainers. > I’m happy to go back to the drawing board to change things if there’s a better > answer to the problem. > > Thanks for everyone’s patience with me trying to work through the proper > process. Thanks for working on this one! Kind regards, Wolfram
Hi Stephen, hi Michael, On 08.05.19 09:12, Annaliese McDermond wrote: > Model the I2C bus clock divider as a part of the Core Clock Framework. > Primarily this removes the clk_get_rate() call from each transfer. > This call causes problems for slave drivers that themselves have > internal clock components that are controlled by an I2C interface. > When the slave's internal clock component is prepared, the prepare > lock is obtained, and it makes calls to the I2C subsystem to > command the hardware to activate the clock. In order to perform > the I2C transfer, this driver sets the divider, which requires > it to get the parent clock rate, which it does with clk_get_rate(). > Unfortunately, this function will try to take the clock prepare > lock, which is already held by the slave's internal clock calls > creating a deadlock. > > Modeling the divider in the CCF natively removes this dependency > and the divider value is only set upon changing the bus clock > frequency or changes in the parent clock that cascade down to this > divisor. This obviates the need to set the divider with every > transfer and avoids the deadlock described above. It also should > provide better clock debugging and save a few cycles on each > transfer due to not having to recalcuate the divider value. could you please take a look at this approach? > > Signed-off-by: Annaliese McDermond <nh6z@nh6z.net> > --- > Changes in v2: > - Remove empty whitespace between declarations > - Preserve comment regarding register rounding > - Consolodate declarations and initial assignments > - Return proper error pointer from bcm2835_i2c_register_div() > - Properly handle return value from bcm2835_i2c_register_div() > in bcm2835_i2c_probe() > - Refactor divider calulation code into separate function > - Use clk_set_rate_exclusive() to ensure the clock is not > modified by other consumers > - Deregister clock in bcm2835_i2c_remove() to properly clean up > after ourselves > > drivers/i2c/busses/i2c-bcm2835.c | 144 ++++++++++++++++++++++++------- > 1 file changed, 113 insertions(+), 31 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c > index d2fbb4bb4a43..e66e569b8f09 100644 > --- a/drivers/i2c/busses/i2c-bcm2835.c > +++ b/drivers/i2c/busses/i2c-bcm2835.c > @@ -4,6 +4,8 @@ > */ > > #include <linux/clk.h> > +#include <linux/clkdev.h> > +#include <linux/clk-provider.h> > #include <linux/completion.h> > #include <linux/err.h> > #include <linux/i2c.h> > @@ -51,9 +53,7 @@ > struct bcm2835_i2c_dev { > struct device *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; > @@ -74,12 +74,17 @@ 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) > +#define to_clk_bcm2835_i2c(_hw) container_of(_hw, struct clk_bcm2835_i2c, hw) > +struct clk_bcm2835_i2c { > + struct clk_hw hw; > + struct bcm2835_i2c_dev *i2c_dev; > +}; > + > +static int clk_bcm2835_i2c_calc_divider(unsigned long rate, > + unsigned long parent_rate) > { > - u32 divider, redl, fedl; > + u32 divider = DIV_ROUND_UP(parent_rate, rate); > > - 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, > @@ -88,12 +93,23 @@ static int bcm2835_i2c_set_divider(struct bcm2835_i2c_dev *i2c_dev) > 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"); > + (divider > BCM2835_I2C_CDIV_MAX)) > return -EINVAL; > - } > > - bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DIV, divider); > + return divider; > +} > + > +static int clk_bcm2835_i2c_set_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long parent_rate) > +{ > + struct clk_bcm2835_i2c *div = to_clk_bcm2835_i2c(hw); > + u32 redl, fedl; > + u32 divider = clk_bcm2835_i2c_calc_divider(rate, parent_rate); > + > + if (divider == -EINVAL) > + return -EINVAL; > + > + bcm2835_i2c_writel(div->i2c_dev, BCM2835_I2C_DIV, divider); > > /* > * Number of core clocks to wait after falling edge before > @@ -108,12 +124,60 @@ static int bcm2835_i2c_set_divider(struct bcm2835_i2c_dev *i2c_dev) > */ > redl = max(divider / 4, 1u); > > - bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DEL, > + bcm2835_i2c_writel(div->i2c_dev, BCM2835_I2C_DEL, > (fedl << BCM2835_I2C_FEDL_SHIFT) | > (redl << BCM2835_I2C_REDL_SHIFT)); > return 0; > } > > +static long clk_bcm2835_i2c_round_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long *parent_rate) > +{ > + u32 divider = clk_bcm2835_i2c_calc_divider(rate, *parent_rate); > + > + return DIV_ROUND_UP(*parent_rate, divider); > +} > + > +static unsigned long clk_bcm2835_i2c_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + struct clk_bcm2835_i2c *div = to_clk_bcm2835_i2c(hw); > + u32 divider = bcm2835_i2c_readl(div->i2c_dev, BCM2835_I2C_DIV); > + > + return DIV_ROUND_UP(parent_rate, divider); > +} > + > +static const struct clk_ops clk_bcm2835_i2c_ops = { > + .set_rate = clk_bcm2835_i2c_set_rate, > + .round_rate = clk_bcm2835_i2c_round_rate, > + .recalc_rate = clk_bcm2835_i2c_recalc_rate, > +}; > + > +static struct clk *bcm2835_i2c_register_div(struct device *dev, > + const char *mclk_name, > + struct bcm2835_i2c_dev *i2c_dev) > +{ > + struct clk_init_data init; > + struct clk_bcm2835_i2c *priv; > + const char *devname = dev_name(dev); > + > + init.ops = &clk_bcm2835_i2c_ops; > + init.name = "bcm2835-i2c"; > + init.parent_names = (const char* []) { mclk_name }; > + init.num_parents = 1; > + init.flags = 0; > + > + priv = devm_kzalloc(dev, sizeof(struct clk_bcm2835_i2c), GFP_KERNEL); > + if (priv == NULL) > + return ERR_PTR(-ENOMEM); > + > + priv->hw.init = &init; > + priv->i2c_dev = i2c_dev; > + > + clk_hw_register_clkdev(&priv->hw, init.name, devname); > + return devm_clk_register(dev, &priv->hw); > +} > + > static void bcm2835_fill_txfifo(struct bcm2835_i2c_dev *i2c_dev) > { > u32 val; > @@ -271,7 +335,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, ret; > + int i; > > for (i = 0; i < (num - 1); i++) > if (msgs[i].flags & I2C_M_RD) { > @@ -280,10 +344,6 @@ 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); > @@ -338,6 +398,9 @@ static int bcm2835_i2c_probe(struct platform_device *pdev) > struct resource *mem, *irq; > int ret; > struct i2c_adapter *adap; > + const char *mclk_name; > + struct clk *bus_clk; > + u32 bus_clk_rate; > > i2c_dev = devm_kzalloc(&pdev->dev, sizeof(*i2c_dev), GFP_KERNEL); > if (!i2c_dev) > @@ -351,21 +414,6 @@ static int bcm2835_i2c_probe(struct platform_device *pdev) > if (IS_ERR(i2c_dev->regs)) > return PTR_ERR(i2c_dev->regs); > > - i2c_dev->clk = devm_clk_get(&pdev->dev, NULL); > - if (IS_ERR(i2c_dev->clk)) { > - if (PTR_ERR(i2c_dev->clk) != -EPROBE_DEFER) > - dev_err(&pdev->dev, "Could not get clock\n"); > - return PTR_ERR(i2c_dev->clk); > - } > - > - ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency", > - &i2c_dev->bus_clk_rate); > - if (ret < 0) { > - dev_warn(&pdev->dev, > - "Could not read clock-frequency property\n"); > - i2c_dev->bus_clk_rate = 100000; > - } > - > irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > if (!irq) { > dev_err(&pdev->dev, "No IRQ resource\n"); > @@ -380,6 +428,35 @@ static int bcm2835_i2c_probe(struct platform_device *pdev) > return -ENODEV; > } > > + mclk_name = of_clk_get_parent_name(pdev->dev.of_node, 0); > + > + bus_clk = bcm2835_i2c_register_div(&pdev->dev, mclk_name, i2c_dev); > + > + if (IS_ERR(bus_clk)) { > + dev_err(&pdev->dev, "Could not register clock\n"); > + return PTR_ERR(bus_clk); > + } > + > + ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency", > + &bus_clk_rate); > + if (ret < 0) { > + dev_warn(&pdev->dev, > + "Could not read clock-frequency property\n"); > + bus_clk_rate = 100000; > + } > + > + ret = clk_set_rate_exclusive(bus_clk, bus_clk_rate); > + if (ret < 0) { > + dev_err(&pdev->dev, "Could not set clock frequency\n"); > + return ret; > + } > + > + ret = clk_prepare_enable(bus_clk); > + if (ret) { > + dev_err(&pdev->dev, "Couldn't prepare clock"); > + return ret; > + } > + > adap = &i2c_dev->adapter; > i2c_set_adapdata(adap, i2c_dev); > adap->owner = THIS_MODULE; > @@ -402,6 +479,11 @@ static int bcm2835_i2c_probe(struct platform_device *pdev) > static int bcm2835_i2c_remove(struct platform_device *pdev) > { > struct bcm2835_i2c_dev *i2c_dev = platform_get_drvdata(pdev); > + struct clk *bus_clk = devm_clk_get(i2c_dev->dev, "bcm2835-i2c"); > + > + clk_rate_exclusive_put(bus_clk); > + clk_disable_unprepare(bus_clk); > + devm_clk_unregister(i2c_dev->dev, bus_clk); > > free_irq(i2c_dev->irq, i2c_dev); > i2c_del_adapter(&i2c_dev->adapter);
Wolfram -- Thank you very much for your response. > On May 16, 2019, at 12:58 AM, Wolfram Sang <wsa@the-dreams.de> wrote: > > Hi Annaliese, > > On Wed, May 15, 2019 at 10:37:03PM -0700, Annaliese McDermond wrote: >> I’m just following up on this since I haven’t heard anything since I submitted the >> v2 patch a week ago. Wolfram, does this look like a sane approach? Stefan, >> were my changes in v2 acceptable? > > There is a bit of overhead involved, but conceptually it looks like an > elegant solution to me. However, I am not an expert of CCF. Grepping > through kernel sources, I don't see many clocks defined outside > drivers/clk. So, I'd appreciate if we could get some ack/feedback from > one of the CCF experts/maintainers. I was also similarly nervous about a clock provider being outside of drivers/clk, especially since one of the instances of that I wrote. When writing this, there was a certain logic to putting this inside of clk-bcm2835.c instead. Eric may like this approach better because there will probably be more code reuse of some of the dividers he uses in that driver. My concern was surrounding the cross pollenation of the i2c and clock drivers to a certain extent. You have to be able to set the FEDL_SHIFT and REDL_SHIFT registers when you change the clock rate. You can’t do it in the transfer function like was done before because it requires a call to clk_get_rate() which can deadlock in the conditions I encountered. You’d have to move those calls to the clk-bcm2835 driver instead. My nervousness surrounding moving the code into the clock driver rather than the i2c driver is that I don’t know how dependent they are on one another. Do we anticipate that there may be a use for the i2c-bcm2835 driver without the clk-bcm2835 driver? Asking the question very well may be premature optimization for a condition that may never exist. I’m thinking maybe Eric has some more insight into this? The other minor issue is that it’s going to require a change to the bcm2835 DT to change the clock pointer to the divider. If it fits better in the clk-bcm2835 driver, I’m happy to submit a patch that puts it there. Just let me know and I can work on it. -- Annaliese McDermond nh6z@nh6z.net
Annaliese, > Thank you very much for your response. You are welcome. > I was also similarly nervous about a clock provider being outside of > drivers/clk, especially since one of the instances of that I wrote. > > When writing this, there was a certain logic to putting this inside of > clk-bcm2835.c instead. Eric may like this approach better because there > will probably be more code reuse of some of the dividers he uses in that > driver. Regardless which solution is favoured, I am going to apply this patch in a minute: http://patchwork.ozlabs.org/patch/1097688/ It enables this driver for ARCH_BRCMSTB. So, the solution should work for this as well. (I don't know any of these platforms well) Regards, Wolfram
> On May 27, 2019, at 12:15 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > > Regardless which solution is favoured, I am going to apply this patch in > a minute: > > http://patchwork.ozlabs.org/patch/1097688/ > > It enables this driver for ARCH_BRCMSTB. So, the solution should work > for this as well. (I don't know any of these platforms well) I did some looking the other day, and I had forgotten that the RPi has 3 of the i2c-bcm2835 devices each with their own divider that sits in their register space. This makes me think the correct solution would be for the divider to be controlled in the driver as was in my original patch. Otherwise we’d have to make three different dividers in the bcm2835-clk driver, and the i2c driver would no longer work for other platforms. I haven’t heard any comments from any of the clock folks. I was perusing the code and I see there’s a SPI driver that registers its own clock and some various ethernet and MMC drivers that do the same thing. > Regards, > > Wolfram -- Annaliese McDermond nh6z@nh6z.net
On Mon, May 27, 2019 at 01:00:00PM -0700, Annaliese McDermond wrote: > > > On May 27, 2019, at 12:15 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > > > > Regardless which solution is favoured, I am going to apply this patch in > > a minute: > > > > http://patchwork.ozlabs.org/patch/1097688/ > > > > It enables this driver for ARCH_BRCMSTB. So, the solution should work > > for this as well. (I don't know any of these platforms well) > > I did some looking the other day, and I had forgotten that the RPi has 3 > of the i2c-bcm2835 devices each with their own divider that sits in their > register space. This makes me think the correct solution would be for > the divider to be controlled in the driver as was in my original patch. > Otherwise we’d have to make three different dividers in the bcm2835-clk > driver, and the i2c driver would no longer work for other platforms. I am getting more and more convinced of the original patch even without CCF acks. Others?
Hi Annaliese, thank you for mention that we have multiple I2C interfaces. On 08.05.19 09:12, Annaliese McDermond wrote: > + > +static struct clk *bcm2835_i2c_register_div(struct device *dev, > + const char *mclk_name, > + struct bcm2835_i2c_dev *i2c_dev) > +{ > + struct clk_init_data init; > + struct clk_bcm2835_i2c *priv; > + const char *devname = dev_name(dev); > + > + init.ops = &clk_bcm2835_i2c_ops; > + init.name = "bcm2835-i2c"; Does this work intentionally in case i2c-0 and i2c-1 are used at the same time? Please also check the output of /sys/kernel/debug/clk/clk_summary Regards Stefan
Annaliese McDermond <nh6z@nh6z.net> writes: >> On May 27, 2019, at 12:15 PM, Wolfram Sang <wsa@the-dreams.de> wrote: >> >> Regardless which solution is favoured, I am going to apply this patch in >> a minute: >> >> http://patchwork.ozlabs.org/patch/1097688/ >> >> It enables this driver for ARCH_BRCMSTB. So, the solution should work >> for this as well. (I don't know any of these platforms well) > > I did some looking the other day, and I had forgotten that the RPi has 3 > of the i2c-bcm2835 devices each with their own divider that sits in their > register space. This makes me think the correct solution would be for > the divider to be controlled in the driver as was in my original patch. > Otherwise we’d have to make three different dividers in the bcm2835-clk > driver, and the i2c driver would no longer work for other platforms. Control of the divider should definitely be in the i2c driver, not in clk-bcm2835.
> On May 28, 2019, at 12:52 AM, Stefan Wahren <stefan.wahren@i2se.com> wrote: > > Hi Annaliese, > > thank you for mention that we have multiple I2C interfaces. > > On 08.05.19 09:12, Annaliese McDermond wrote: >> + >> +static struct clk *bcm2835_i2c_register_div(struct device *dev, >> + const char *mclk_name, >> + struct bcm2835_i2c_dev *i2c_dev) >> +{ >> + struct clk_init_data init; >> + struct clk_bcm2835_i2c *priv; >> + const char *devname = dev_name(dev); >> + >> + init.ops = &clk_bcm2835_i2c_ops; >> + init.name = "bcm2835-i2c"; > > Does this work intentionally in case i2c-0 and i2c-1 are used at the > same time? It should work fine. The clocks are all registered separately and passed the pointer to the struct device for the interface. This keeps it accessing the correct registers and such. > Please also check the output of /sys/kernel/debug/clk/clk_summary They’ll come up with the same name in the current code in the debug output. I agree this is mildly confusing and I’ll spin another version of the patch to give them unique clock names in clk_summary. > Regards > Stefan > -- Annaliese McDermonod nh6z@nh6z.net
> On May 28, 2019, at 8:01 PM, Annaliese McDermond <nh6z@nh6z.net> wrote: > > > >> On May 28, 2019, at 12:52 AM, Stefan Wahren <stefan.wahren@i2se.com> wrote: >> Please also check the output of /sys/kernel/debug/clk/clk_summary > > They’ll come up with the same name in the current code in the debug > output. I agree this is mildly confusing and I’ll spin another version > of the patch to give them unique clock names in clk_summary. In v3 of the patch the output of /sys/kernel/debug/clk/clk_summary looks like the following with two i2c devices enabled on the RPi. Note 3f804000.i2c_div and 3f205000.i2c_div. enable prepare protect duty clock count count count rate accuracy phase cycle --------------------------------------------------------------------------------------------- otg 0 0 0 480000000 0 0 50000 osc 7 7 4 19200000 0 0 50000 gp2 1 1 1 32768 0 0 50000 tsens 1 1 1 1920000 0 0 50000 vec 0 0 0 19200000 0 0 50000 otp 0 0 0 4800000 0 0 50000 timer 0 0 0 1000002 0 0 50000 pllh 4 4 0 855000000 0 0 50000 pllh_pix_prediv 1 1 0 855000000 0 0 50000 pllh_pix 0 0 0 85500000 0 0 50000 pllh_aux 1 1 0 3339844 0 0 50000 pllh_rcal_prediv 1 1 0 3339844 0 0 50000 pllh_rcal 0 0 0 333984 0 0 50000 plld 3 3 1 2000000024 0 0 50000 plld_dsi1 0 0 0 7812501 0 0 50000 plld_dsi0 0 0 0 7812501 0 0 50000 plld_per 4 4 3 500000006 0 0 50000 pcm 1 1 1 1535999 0 0 50000 gp0 1 1 1 24999389 0 0 50000 pll 1 1 0 98303848 0 0 50000 codec_clkin 2 2 0 98303848 0 0 50000 nadc 1 1 0 12287981 0 0 50000 madc 1 1 0 6143991 0 0 50000 ndac 1 1 0 12287981 0 0 50000 mdac 2 2 0 6143991 0 0 50000 bdiv 1 1 0 1535998 0 0 50000 gp1 1 1 1 24000094 0 0 50000 hsm 0 0 0 163682866 0 0 50000 uart 0 0 0 47999625 0 0 50000 plld_core 2 2 0 500000006 0 0 50000 sdram 0 0 0 166666668 0 0 50000 pllc 3 3 1 2400000000 0 0 50000 pllc_per 1 1 0 1200000000 0 0 50000 emmc 0 0 0 200000000 0 0 50000 pllc_core2 0 0 0 9375000 0 0 50000 pllc_core1 0 0 0 9375000 0 0 50000 pllc_core0 2 2 1 1200000000 0 0 50000 vpu 3 3 2 400000000 0 0 50000 3f804000.i2c_div 1 1 1 100000 0 0 50000 3f205000.i2c_div 1 1 1 100000 0 0 50000 aux_spi2 0 0 0 400000000 0 0 50000 aux_spi1 0 0 0 400000000 0 0 50000 aux_uart 0 0 0 400000000 0 0 50000 peri_image 0 0 0 400000000 0 0 50000 pllb 2 2 0 2800000012 0 0 50000 pllb_arm 1 1 0 1400000006 0 0 50000 plla 2 2 0 2400000000 0 0 50000 plla_ccp2 0 0 0 9375000 0 0 50000 plla_dsi0 0 0 0 9375000 0 0 50000 plla_per 0 0 0 9375000 0 0 50000 plla_core 1 1 0 1200000000 0 0 50000 h264 0 0 0 300000000 0 0 50000 isp 0 0 0 300000000 0 0 50000 v3d 0 0 0 300000000 0 0 50000 sc16is752_clk 1 1 0 1843200 0 0 50000 dsi1p 0 0 0 0 0 0 50000 dsi0p 0 0 0 0 0 0 50000 dsi1e 0 0 0 0 0 0 50000 dsi0e 0 0 0 0 0 0 50000 cam1 0 0 0 0 0 0 50000 cam0 0 0 0 0 0 0 50000 dpi 0 0 0 0 0 0 50000 tec 0 0 0 0 0 0 50000 smi 0 0 0 0 0 0 50000 slim 0 0 0 0 0 0 50000 dft 0 0 0 0 0 0 50000 aveo 0 0 0 0 0 0 50000 pwm 0 0 0 0 0 0 50000 -- Annaliese McDermond nh6z@nh6z.net
diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c index d2fbb4bb4a43..e66e569b8f09 100644 --- a/drivers/i2c/busses/i2c-bcm2835.c +++ b/drivers/i2c/busses/i2c-bcm2835.c @@ -4,6 +4,8 @@ */ #include <linux/clk.h> +#include <linux/clkdev.h> +#include <linux/clk-provider.h> #include <linux/completion.h> #include <linux/err.h> #include <linux/i2c.h> @@ -51,9 +53,7 @@ struct bcm2835_i2c_dev { struct device *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; @@ -74,12 +74,17 @@ 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) +#define to_clk_bcm2835_i2c(_hw) container_of(_hw, struct clk_bcm2835_i2c, hw) +struct clk_bcm2835_i2c { + struct clk_hw hw; + struct bcm2835_i2c_dev *i2c_dev; +}; + +static int clk_bcm2835_i2c_calc_divider(unsigned long rate, + unsigned long parent_rate) { - u32 divider, redl, fedl; + u32 divider = DIV_ROUND_UP(parent_rate, rate); - 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, @@ -88,12 +93,23 @@ static int bcm2835_i2c_set_divider(struct bcm2835_i2c_dev *i2c_dev) 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"); + (divider > BCM2835_I2C_CDIV_MAX)) return -EINVAL; - } - bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DIV, divider); + return divider; +} + +static int clk_bcm2835_i2c_set_rate(struct clk_hw *hw, unsigned long rate, + unsigned long parent_rate) +{ + struct clk_bcm2835_i2c *div = to_clk_bcm2835_i2c(hw); + u32 redl, fedl; + u32 divider = clk_bcm2835_i2c_calc_divider(rate, parent_rate); + + if (divider == -EINVAL) + return -EINVAL; + + bcm2835_i2c_writel(div->i2c_dev, BCM2835_I2C_DIV, divider); /* * Number of core clocks to wait after falling edge before @@ -108,12 +124,60 @@ static int bcm2835_i2c_set_divider(struct bcm2835_i2c_dev *i2c_dev) */ redl = max(divider / 4, 1u); - bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DEL, + bcm2835_i2c_writel(div->i2c_dev, BCM2835_I2C_DEL, (fedl << BCM2835_I2C_FEDL_SHIFT) | (redl << BCM2835_I2C_REDL_SHIFT)); return 0; } +static long clk_bcm2835_i2c_round_rate(struct clk_hw *hw, unsigned long rate, + unsigned long *parent_rate) +{ + u32 divider = clk_bcm2835_i2c_calc_divider(rate, *parent_rate); + + return DIV_ROUND_UP(*parent_rate, divider); +} + +static unsigned long clk_bcm2835_i2c_recalc_rate(struct clk_hw *hw, + unsigned long parent_rate) +{ + struct clk_bcm2835_i2c *div = to_clk_bcm2835_i2c(hw); + u32 divider = bcm2835_i2c_readl(div->i2c_dev, BCM2835_I2C_DIV); + + return DIV_ROUND_UP(parent_rate, divider); +} + +static const struct clk_ops clk_bcm2835_i2c_ops = { + .set_rate = clk_bcm2835_i2c_set_rate, + .round_rate = clk_bcm2835_i2c_round_rate, + .recalc_rate = clk_bcm2835_i2c_recalc_rate, +}; + +static struct clk *bcm2835_i2c_register_div(struct device *dev, + const char *mclk_name, + struct bcm2835_i2c_dev *i2c_dev) +{ + struct clk_init_data init; + struct clk_bcm2835_i2c *priv; + const char *devname = dev_name(dev); + + init.ops = &clk_bcm2835_i2c_ops; + init.name = "bcm2835-i2c"; + init.parent_names = (const char* []) { mclk_name }; + init.num_parents = 1; + init.flags = 0; + + priv = devm_kzalloc(dev, sizeof(struct clk_bcm2835_i2c), GFP_KERNEL); + if (priv == NULL) + return ERR_PTR(-ENOMEM); + + priv->hw.init = &init; + priv->i2c_dev = i2c_dev; + + clk_hw_register_clkdev(&priv->hw, init.name, devname); + return devm_clk_register(dev, &priv->hw); +} + static void bcm2835_fill_txfifo(struct bcm2835_i2c_dev *i2c_dev) { u32 val; @@ -271,7 +335,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, ret; + int i; for (i = 0; i < (num - 1); i++) if (msgs[i].flags & I2C_M_RD) { @@ -280,10 +344,6 @@ 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); @@ -338,6 +398,9 @@ static int bcm2835_i2c_probe(struct platform_device *pdev) struct resource *mem, *irq; int ret; struct i2c_adapter *adap; + const char *mclk_name; + struct clk *bus_clk; + u32 bus_clk_rate; i2c_dev = devm_kzalloc(&pdev->dev, sizeof(*i2c_dev), GFP_KERNEL); if (!i2c_dev) @@ -351,21 +414,6 @@ static int bcm2835_i2c_probe(struct platform_device *pdev) if (IS_ERR(i2c_dev->regs)) return PTR_ERR(i2c_dev->regs); - i2c_dev->clk = devm_clk_get(&pdev->dev, NULL); - if (IS_ERR(i2c_dev->clk)) { - if (PTR_ERR(i2c_dev->clk) != -EPROBE_DEFER) - dev_err(&pdev->dev, "Could not get clock\n"); - return PTR_ERR(i2c_dev->clk); - } - - ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency", - &i2c_dev->bus_clk_rate); - if (ret < 0) { - dev_warn(&pdev->dev, - "Could not read clock-frequency property\n"); - i2c_dev->bus_clk_rate = 100000; - } - irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); if (!irq) { dev_err(&pdev->dev, "No IRQ resource\n"); @@ -380,6 +428,35 @@ static int bcm2835_i2c_probe(struct platform_device *pdev) return -ENODEV; } + mclk_name = of_clk_get_parent_name(pdev->dev.of_node, 0); + + bus_clk = bcm2835_i2c_register_div(&pdev->dev, mclk_name, i2c_dev); + + if (IS_ERR(bus_clk)) { + dev_err(&pdev->dev, "Could not register clock\n"); + return PTR_ERR(bus_clk); + } + + ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency", + &bus_clk_rate); + if (ret < 0) { + dev_warn(&pdev->dev, + "Could not read clock-frequency property\n"); + bus_clk_rate = 100000; + } + + ret = clk_set_rate_exclusive(bus_clk, bus_clk_rate); + if (ret < 0) { + dev_err(&pdev->dev, "Could not set clock frequency\n"); + return ret; + } + + ret = clk_prepare_enable(bus_clk); + if (ret) { + dev_err(&pdev->dev, "Couldn't prepare clock"); + return ret; + } + adap = &i2c_dev->adapter; i2c_set_adapdata(adap, i2c_dev); adap->owner = THIS_MODULE; @@ -402,6 +479,11 @@ static int bcm2835_i2c_probe(struct platform_device *pdev) static int bcm2835_i2c_remove(struct platform_device *pdev) { struct bcm2835_i2c_dev *i2c_dev = platform_get_drvdata(pdev); + struct clk *bus_clk = devm_clk_get(i2c_dev->dev, "bcm2835-i2c"); + + clk_rate_exclusive_put(bus_clk); + clk_disable_unprepare(bus_clk); + devm_clk_unregister(i2c_dev->dev, bus_clk); free_irq(i2c_dev->irq, i2c_dev); i2c_del_adapter(&i2c_dev->adapter);
Model the I2C bus clock divider as a part of the Core Clock Framework. Primarily this removes the clk_get_rate() call from each transfer. This call causes problems for slave drivers that themselves have internal clock components that are controlled by an I2C interface. When the slave's internal clock component is prepared, the prepare lock is obtained, and it makes calls to the I2C subsystem to command the hardware to activate the clock. In order to perform the I2C transfer, this driver sets the divider, which requires it to get the parent clock rate, which it does with clk_get_rate(). Unfortunately, this function will try to take the clock prepare lock, which is already held by the slave's internal clock calls creating a deadlock. Modeling the divider in the CCF natively removes this dependency and the divider value is only set upon changing the bus clock frequency or changes in the parent clock that cascade down to this divisor. This obviates the need to set the divider with every transfer and avoids the deadlock described above. It also should provide better clock debugging and save a few cycles on each transfer due to not having to recalcuate the divider value. Signed-off-by: Annaliese McDermond <nh6z@nh6z.net> --- Changes in v2: - Remove empty whitespace between declarations - Preserve comment regarding register rounding - Consolodate declarations and initial assignments - Return proper error pointer from bcm2835_i2c_register_div() - Properly handle return value from bcm2835_i2c_register_div() in bcm2835_i2c_probe() - Refactor divider calulation code into separate function - Use clk_set_rate_exclusive() to ensure the clock is not modified by other consumers - Deregister clock in bcm2835_i2c_remove() to properly clean up after ourselves drivers/i2c/busses/i2c-bcm2835.c | 144 ++++++++++++++++++++++++------- 1 file changed, 113 insertions(+), 31 deletions(-)