| Message ID | 20231128-i2c-qup-dvfs-v1-3-59a0e3039111@kernkonzept.com |
|---|---|
| State | Accepted |
| Delegated to: | Andi Shyti |
| Headers | show |
| Series | i2c: qup: Allow scaling power domains and interconnect | expand |
Hi Stephan, On Tue, Nov 28, 2023 at 10:48:37AM +0100, Stephan Gerhold wrote: > When the I2C QUP controller is used together with a DMA engine it needs > to vote for the interconnect path to the DRAM. Otherwise it may be > unable to access the memory quickly enough. > > The requested peak bandwidth is dependent on the I2C core clock. > > To avoid sending votes too often the bandwidth is always requested when > a DMA transfer starts, but dropped only on runtime suspend. Runtime > suspend should only happen if no transfer is active. After resumption we > can defer the next vote until the first DMA transfer actually happens. > > The implementation is largely identical to the one introduced for > spi-qup in commit ecdaa9473019 ("spi: qup: Vote for interconnect > bandwidth to DRAM") since both drivers represent the same hardware > block. > > Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com> the patch looks good to me. > --- > The bandwidth calculation is taken over from Qualcomm's > downstream/vendor driver [1]. Due to lack of documentation about the > interconnect setup/behavior I cannot say exactly if this is right. > Unfortunately, this is not implemented very consistently downstream... > > [1]: https://git.codelinaro.org/clo/la/kernel/msm-3.10/-/commit/67174e2624ea64814231e7e1e4af83fd882302c6 Krzysztof, any chance you can help here? Thanks, Andi
Hi Stephen, sorry for the very late reply here. Just one question. ... > downstream/vendor driver [1]. Due to lack of documentation about the > interconnect setup/behavior I cannot say exactly if this is right. > Unfortunately, this is not implemented very consistently downstream... Can we have someone from Qualcomm or Linaro taking a peak here? > [1]: https://git.codelinaro.org/clo/la/kernel/msm-3.10/-/commit/67174e2624ea64814231e7e1e4af83fd882302c6 ... > @@ -1745,6 +1775,11 @@ static int qup_i2c_probe(struct platform_device *pdev) > goto fail_dma; > } > qup->is_dma = true; > + > + qup->icc_path = devm_of_icc_get(&pdev->dev, NULL); > + if (IS_ERR(qup->icc_path)) > + return dev_err_probe(&pdev->dev, PTR_ERR(qup->icc_path), > + "failed to get interconnect path\n"); Can we live without it if it fails? Thanks, Andi
Sorry for replying to my own mail, bu I needed to fix Stephan and Konrad's emails. On Wed, Feb 19, 2025 at 12:02:11AM +0100, Andi Shyti wrote: > Hi Stephen, > > sorry for the very late reply here. Just one question. > > ... > > > downstream/vendor driver [1]. Due to lack of documentation about the > > interconnect setup/behavior I cannot say exactly if this is right. > > Unfortunately, this is not implemented very consistently downstream... > > Can we have someone from Qualcomm or Linaro taking a peak here? > > > [1]: https://git.codelinaro.org/clo/la/kernel/msm-3.10/-/commit/67174e2624ea64814231e7e1e4af83fd882302c6 > > ... > > > @@ -1745,6 +1775,11 @@ static int qup_i2c_probe(struct platform_device *pdev) > > goto fail_dma; > > } > > qup->is_dma = true; > > + > > + qup->icc_path = devm_of_icc_get(&pdev->dev, NULL); > > + if (IS_ERR(qup->icc_path)) > > + return dev_err_probe(&pdev->dev, PTR_ERR(qup->icc_path), > > + "failed to get interconnect path\n"); > > Can we live without it if it fails? > > Thanks, > Andi
On 19/02/2025 00:02, Andi Shyti wrote: > Hi Stephen, > > sorry for the very late reply here. Just one question. > > ... > >> downstream/vendor driver [1]. Due to lack of documentation about the >> interconnect setup/behavior I cannot say exactly if this is right. >> Unfortunately, this is not implemented very consistently downstream... > > Can we have someone from Qualcomm or Linaro taking a peak here? You replied to some old email, not in my inbox anymore, but your quote lacks standard quote-template, like: On 19/02/2025 00:02, Andi Shyti wrote: so I really don't know when was it sent. For sure more than a month ago, maybe more? This has to be resent if you want anything done here. Best regards, Krzysztof
Hi Andi, On Wed, Feb 19, 2025 at 12:02:06AM +0100, Andi Shyti wrote: > > sorry for the very late reply here. Just one question. > Thanks for bringing the patch back up after such a long time. I've been meaning to resend it, but never found the time to do so... :-) > > > downstream/vendor driver [1]. Due to lack of documentation about the > > interconnect setup/behavior I cannot say exactly if this is right. > > Unfortunately, this is not implemented very consistently downstream... > > Can we have someone from Qualcomm or Linaro taking a peak here? > I suppose I count as someone from Linaro nowadays. However, since this driver is only used on really old platforms nowadays, I'm not sure where to look or who to ask... At the end, the whole bus scaling/interconnect is always somewhat "imprecise". There is no clear "correct" or "wrong", since the ideal bandwidth depends heavily on the actual use case that we are not aware of in the driver. There is also overhead when voting for bandwidth, since that can take a couple of milliseconds. The most important part is that we vote for any bandwidth at all, since otherwise the bus path could potentially be completely off and it would get stuck. My patch implements one of the approaches that was used in the downstream/vendor drivers and matches what we already have upstream in the corresponding spi-qup driver. I think it's "good enough". If someone ever wants to fine tune this based on actual measurements they can just submit an incremental patch. Right now this series is blocking adding the necessary properties in the device tree and that's not good. Surprisingly this series still applies cleanly on top of linux-next. The dt-bindings have review tags and there was plenty of time for someone else to chime in for the driver. So maybe you can just pick them up? :D > > [1]: https://git.codelinaro.org/clo/la/kernel/msm-3.10/-/commit/67174e2624ea64814231e7e1e4af83fd882302c6 > > ... > > > @@ -1745,6 +1775,11 @@ static int qup_i2c_probe(struct platform_device *pdev) > > goto fail_dma; > > } > > qup->is_dma = true; > > + > > + qup->icc_path = devm_of_icc_get(&pdev->dev, NULL); > > + if (IS_ERR(qup->icc_path)) > > + return dev_err_probe(&pdev->dev, PTR_ERR(qup->icc_path), > > + "failed to get interconnect path\n"); > > Can we live without it if it fails? > of_icc_get() returns NULL if the interconnect API is disabled, or if "interconnects" is not defined in the device tree, so this is already handled. If "interconnects" is enabled and defined, I think we shouldn't ignore errors. Therefore, this should work as intended. Let me know if I should resend the patch or if you can apply it directly. Thanks, Stephan
Hi Stephan, On Wed, Feb 19, 2025 at 11:40:16AM +0100, Stephan Gerhold wrote: > Hi Andi, > > On Wed, Feb 19, 2025 at 12:02:06AM +0100, Andi Shyti wrote: > > > > sorry for the very late reply here. Just one question. > > > > Thanks for bringing the patch back up after such a long time. I've been > meaning to resend it, but never found the time to do so... :-) We have a long list of forgotten patches that belong to the far past. I'm trying to revive them. > > > downstream/vendor driver [1]. Due to lack of documentation about the > > > interconnect setup/behavior I cannot say exactly if this is right. > > > Unfortunately, this is not implemented very consistently downstream... > > > > Can we have someone from Qualcomm or Linaro taking a peak here? > > > > I suppose I count as someone from Linaro nowadays. However, since this > driver is only used on really old platforms nowadays, I'm not sure where > to look or who to ask... > > At the end, the whole bus scaling/interconnect is always somewhat > "imprecise". There is no clear "correct" or "wrong", since the ideal > bandwidth depends heavily on the actual use case that we are not aware > of in the driver. There is also overhead when voting for bandwidth, > since that can take a couple of milliseconds. > > The most important part is that we vote for any bandwidth at all, since > otherwise the bus path could potentially be completely off and it would > get stuck. My patch implements one of the approaches that was used in > the downstream/vendor drivers and matches what we already have upstream > in the corresponding spi-qup driver. I think it's "good enough". If > someone ever wants to fine tune this based on actual measurements they > can just submit an incremental patch. Right now this series is blocking > adding the necessary properties in the device tree and that's not good. > > Surprisingly this series still applies cleanly on top of linux-next. The > dt-bindings have review tags and there was plenty of time for someone > else to chime in for the driver. So maybe you can just pick them up? :D Yes, I already tested them. > > > [1]: https://git.codelinaro.org/clo/la/kernel/msm-3.10/-/commit/67174e2624ea64814231e7e1e4af83fd882302c6 > > > > ... > > > > > @@ -1745,6 +1775,11 @@ static int qup_i2c_probe(struct platform_device *pdev) > > > goto fail_dma; > > > } > > > qup->is_dma = true; > > > + > > > + qup->icc_path = devm_of_icc_get(&pdev->dev, NULL); > > > + if (IS_ERR(qup->icc_path)) > > > + return dev_err_probe(&pdev->dev, PTR_ERR(qup->icc_path), > > > + "failed to get interconnect path\n"); > > > > Can we live without it if it fails? > > > > of_icc_get() returns NULL if the interconnect API is disabled, or if > "interconnects" is not defined in the device tree, so this is already > handled. If "interconnects" is enabled and defined, I think we shouldn't > ignore errors. Therefore, this should work as intended. yes, because qup_i2c_vote_bw() checks inside for NULL values. My idea was that: if (IS_ERR(...)) { dev_warn(...) qup->icc_path = NULL; } and let things work. Anyway, if you want to keep it this way, fine with me, I don't have a strong opinion, rather than a preference to keep going. Thanks, Andi > Let me know if I should resend the patch or if you can apply it > directly. > > Thanks, > Stephan
Hi Krzysztof, On Wed, Feb 19, 2025 at 08:00:25AM +0100, Krzysztof Kozlowski wrote: > On 19/02/2025 00:02, Andi Shyti wrote: > > sorry for the very late reply here. Just one question. > > > > ... > > > >> downstream/vendor driver [1]. Due to lack of documentation about the > >> interconnect setup/behavior I cannot say exactly if this is right. > >> Unfortunately, this is not implemented very consistently downstream... > > > > Can we have someone from Qualcomm or Linaro taking a peak here? > > You replied to some old email, not in my inbox anymore, feeling nostalgic :-) > but your quote > lacks standard quote-template, like: > > On 19/02/2025 00:02, Andi Shyti wrote: I'm strictly following RFC-1855, but you're right I removed a bit too much to lose time reference. > so I really don't know when was it sent. For sure more than a month ago, > maybe more? This has to be resent if you want anything done here. It was sent on "Tue, 28 Nov 2023 10:48:34 +0100", definitely more than a month ago, I'm also surprised to have it in my inbox. But it still applies cleanly. Perhaps a resend can invite people for more reviews. I don't mind. Thanks, Andi
On Wed, Feb 19, 2025 at 08:30:35PM +0100, Andi Shyti wrote: > On Wed, Feb 19, 2025 at 11:40:16AM +0100, Stephan Gerhold wrote: > > On Wed, Feb 19, 2025 at 12:02:06AM +0100, Andi Shyti wrote: > > > > > > sorry for the very late reply here. Just one question. > > > > > > > Thanks for bringing the patch back up after such a long time. I've been > > meaning to resend it, but never found the time to do so... :-) > > We have a long list of forgotten patches that belong to the far > past. I'm trying to revive them. > Thanks, this is much appreciated! > [...] > > > > @@ -1745,6 +1775,11 @@ static int qup_i2c_probe(struct platform_device *pdev) > > > > goto fail_dma; > > > > } > > > > qup->is_dma = true; > > > > + > > > > + qup->icc_path = devm_of_icc_get(&pdev->dev, NULL); > > > > + if (IS_ERR(qup->icc_path)) > > > > + return dev_err_probe(&pdev->dev, PTR_ERR(qup->icc_path), > > > > + "failed to get interconnect path\n"); > > > > > > Can we live without it if it fails? > > > > > > > of_icc_get() returns NULL if the interconnect API is disabled, or if > > "interconnects" is not defined in the device tree, so this is already > > handled. If "interconnects" is enabled and defined, I think we shouldn't > > ignore errors. Therefore, this should work as intended. > > yes, because qup_i2c_vote_bw() checks inside for NULL values. > > My idea was that: > > if (IS_ERR(...)) { > dev_warn(...) > qup->icc_path = NULL; > } > > and let things work. Anyway, if you want to keep it this way, > fine with me, I don't have a strong opinion, rather than a > preference to keep going. I would prefer to keep it the way it is. It's okay to omit the "interconnects" in the DT (either for old device trees, or because you don't define the "dmas" either). But if they are defined, we should not be ignoring errors. -EPROBE_DEFER definitely needs to be handled, but even for -EINVAL or similar it would be better to make it obvious in my opinion. None of the existing users should be affected, since no one defines "interconnects" at the moment. Thanks, Stephan
On 19.02.2025 11:40 AM, Stephan Gerhold wrote: > Hi Andi, > > On Wed, Feb 19, 2025 at 12:02:06AM +0100, Andi Shyti wrote: >> >> sorry for the very late reply here. Just one question. >> > > Thanks for bringing the patch back up after such a long time. I've been > meaning to resend it, but never found the time to do so... :-) > >> >>> downstream/vendor driver [1]. Due to lack of documentation about the >>> interconnect setup/behavior I cannot say exactly if this is right. >>> Unfortunately, this is not implemented very consistently downstream... >> >> Can we have someone from Qualcomm or Linaro taking a peak here? >> > > I suppose I count as someone from Linaro nowadays. However, since this > driver is only used on really old platforms nowadays, I'm not sure where > to look or who to ask... > > At the end, the whole bus scaling/interconnect is always somewhat > "imprecise". There is no clear "correct" or "wrong", since the ideal > bandwidth depends heavily on the actual use case that we are not aware > of in the driver. There is also overhead when voting for bandwidth, > since that can take a couple of milliseconds. > > The most important part is that we vote for any bandwidth at all, since > otherwise the bus path could potentially be completely off and it would > get stuck. My patch implements one of the approaches that was used in > the downstream/vendor drivers and matches what we already have upstream > in the corresponding spi-qup driver. I think it's "good enough". If > someone ever wants to fine tune this based on actual measurements they > can just submit an incremental patch. Right now this series is blocking > adding the necessary properties in the device tree and that's not good. Yeah, the throughput of an I2C controller isn't even very likely to affect the total bus frequency requirement, although it's a strict requirement that the requested bw is nonzero (otherwise the bus may be clock-gated) Konrad
diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c index 598102d16677..ee92a315f074 100644 --- a/drivers/i2c/busses/i2c-qup.c +++ b/drivers/i2c/busses/i2c-qup.c @@ -14,6 +14,7 @@ #include <linux/dma-mapping.h> #include <linux/err.h> #include <linux/i2c.h> +#include <linux/interconnect.h> #include <linux/interrupt.h> #include <linux/io.h> #include <linux/module.h> @@ -150,6 +151,8 @@ /* TAG length for DATA READ in RX FIFO */ #define READ_RX_TAGS_LEN 2 +#define QUP_BUS_WIDTH 8 + static unsigned int scl_freq; module_param_named(scl_freq, scl_freq, uint, 0444); MODULE_PARM_DESC(scl_freq, "SCL frequency override"); @@ -227,6 +230,7 @@ struct qup_i2c_dev { int irq; struct clk *clk; struct clk *pclk; + struct icc_path *icc_path; struct i2c_adapter adap; int clk_ctl; @@ -255,6 +259,10 @@ struct qup_i2c_dev { /* To configure when bus is in run state */ u32 config_run; + /* bandwidth votes */ + u32 src_clk_freq; + u32 cur_bw_clk_freq; + /* dma parameters */ bool is_dma; /* To check if the current transfer is using DMA */ @@ -453,6 +461,23 @@ static int qup_i2c_bus_active(struct qup_i2c_dev *qup, int len) return ret; } +static int qup_i2c_vote_bw(struct qup_i2c_dev *qup, u32 clk_freq) +{ + u32 needed_peak_bw; + int ret; + + if (qup->cur_bw_clk_freq == clk_freq) + return 0; + + needed_peak_bw = Bps_to_icc(clk_freq * QUP_BUS_WIDTH); + ret = icc_set_bw(qup->icc_path, 0, needed_peak_bw); + if (ret) + return ret; + + qup->cur_bw_clk_freq = clk_freq; + return 0; +} + static void qup_i2c_write_tx_fifo_v1(struct qup_i2c_dev *qup) { struct qup_i2c_block *blk = &qup->blk; @@ -840,6 +865,10 @@ static int qup_i2c_bam_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int ret = 0; int idx = 0; + ret = qup_i2c_vote_bw(qup, qup->src_clk_freq); + if (ret) + return ret; + enable_irq(qup->irq); ret = qup_i2c_req_dma(qup); @@ -1645,6 +1674,7 @@ static void qup_i2c_disable_clocks(struct qup_i2c_dev *qup) config = readl(qup->base + QUP_CONFIG); config |= QUP_CLOCK_AUTO_GATE; writel(config, qup->base + QUP_CONFIG); + qup_i2c_vote_bw(qup, 0); clk_disable_unprepare(qup->pclk); } @@ -1745,6 +1775,11 @@ static int qup_i2c_probe(struct platform_device *pdev) goto fail_dma; } qup->is_dma = true; + + qup->icc_path = devm_of_icc_get(&pdev->dev, NULL); + if (IS_ERR(qup->icc_path)) + return dev_err_probe(&pdev->dev, PTR_ERR(qup->icc_path), + "failed to get interconnect path\n"); } nodma: @@ -1793,6 +1828,7 @@ static int qup_i2c_probe(struct platform_device *pdev) qup_i2c_enable_clocks(qup); src_clk_freq = clk_get_rate(qup->clk); } + qup->src_clk_freq = src_clk_freq; /* * Bootloaders might leave a pending interrupt on certain QUP's,
When the I2C QUP controller is used together with a DMA engine it needs to vote for the interconnect path to the DRAM. Otherwise it may be unable to access the memory quickly enough. The requested peak bandwidth is dependent on the I2C core clock. To avoid sending votes too often the bandwidth is always requested when a DMA transfer starts, but dropped only on runtime suspend. Runtime suspend should only happen if no transfer is active. After resumption we can defer the next vote until the first DMA transfer actually happens. The implementation is largely identical to the one introduced for spi-qup in commit ecdaa9473019 ("spi: qup: Vote for interconnect bandwidth to DRAM") since both drivers represent the same hardware block. Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com> --- The bandwidth calculation is taken over from Qualcomm's downstream/vendor driver [1]. Due to lack of documentation about the interconnect setup/behavior I cannot say exactly if this is right. Unfortunately, this is not implemented very consistently downstream... [1]: https://git.codelinaro.org/clo/la/kernel/msm-3.10/-/commit/67174e2624ea64814231e7e1e4af83fd882302c6 --- drivers/i2c/busses/i2c-qup.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)