From: Sagar Dharia <sdharia@codeaurora.org> To: Doug Anderson <dianders@chromium.org>, Jonathan Corbet <corbet@lwn.net>, Andy Gross <andy.gross@linaro.org>, David Brown <david.brown@linaro.org>, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Wolfram Sang <wsa@the-dreams.de>, Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>, linux-doc@vger.kernel.org, linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, linux-i2c@vger.kernel.org, linux-serial@vger.kernel.org, Jiri Slaby <jslaby@suse.com>, evgreen@chromium.org, acourbot@chromium.org, Girish Mahadevan <girishm@codeaurora.org>, swboyd@chromium.org Subject: Re: [v3, 3/4] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller Date: Wed, 7 Mar 2018 19:42:24 -0700 [thread overview] Message-ID: <03c5ed5b-5160-bdd5-82f9-4a8beab33ca8@codeaurora.org> (raw) In-Reply-To: <CAD=FV=WbeP8xpVi7cji9hsNyxEam2D5D+_21ur_MuUO8FK42OQ@mail.gmail.com> Hi Doug, Thank you for reviewing the patch. I will take a stab at a few comments below. We will address most of the other comments in next version of I2C patch. > > >> + >> +#define I2C_AUTO_SUSPEND_DELAY 250 > > Why 250 ms? That seems like an eternity. Is it really that expensive > to turn resources off and on? I would sorta just expect clocks and > stuff to get turned off right after a transaction finished unless > another one was pending right behind it... > The response from RPMh to turn on/off shared resources also take quite a few msecs. The QUP serial bus block sits quite a few shared-NOCs away from the memory and runtime-PM is used a bandwidth vote/NOC vote for these NOCs from QUP to memory. Also the RPC between apps and RPMh can sometimes take longer depending on other tasks running on apps. This 250 msec avoids thrashing of these RPCs between apps and RPMh. If you plan to keep these NOCs on forever, then your are right: runtime-PM will be only used to turn on/off local clocks and we won't even need autosuspend. that's not true on products where this driver is currently deployed. >> + >> +static const struct geni_i2c_clk_fld geni_i2c_clk_map[] = { >> + {KHz(100), 7, 10, 11, 26}, >> + {KHz(400), 2, 5, 12, 24}, >> + {KHz(1000), 1, 3, 9, 18}, > > So I guess this is all relying on an input serial clock of 19.2MHz? > Maybe document that? > > Assuming I'm understanding the math here, is it really OK for your > 100kHz and 1MHz mode to be running slightly fast? > > 19200. / 2 / 24 >>>> 400.0 > > 19200. / 7 / 26 >>>> 105.49450549450549 > > 19200. / 1 / 18 >>>> 1066.6666666666667 > > It seems like you'd want the fastest clock that you can make that's > _less than_ the spec. > > > It would also be interesting to know if it's expected that boards > might need to tweak the t_high / t_low depending on their electrical > characteristics. In the past I've had lots of requests from board > makers to tweak things because they've got a long trace, or a stronger > or weaker pull, or ... If so we might later need to add some dts > properties like "i2c-scl-rising-time-ns" and make the math more > dynamic here, unless your hardware somehow automatically adjusts for > this type of thing... >These values are derived by our HW team to comply with the t-high and t-low specs of I2C. We have confirmed on scope that the frequency of SCL is indeed less than/equal to the spec. We have not come across slaves who have needed to tweak these things. We are open to adding these properties in dts if you have any such slaves not conforming due to board-layout of other reasons. >> + mode = msg->len > 32 ? GENI_SE_DMA : GENI_SE_FIFO; > > DMA is hard and i2c transfers > 32 bytes are rare. Do we really gain > a lot by transferring i2c commands over DMA compared to a FIFO? > Enough to justify the code complexity and the set of bugs that will > show up? I'm sure it will be a controversial assertion given that the > code's already written, but personally I'd be supportive of ripping > DMA mode out to simplify the driver. I'd be curious if anyone else > agrees. To me it seems like premature optimization. Yes, we have multiple clients (e.g. touch, NFC) using I2C for data transfers bigger than 32 bytes (some transfers are 100s of bytes). The fifo size is 32, so we can definitely avoid at least 1 interrupt when DMA mode is used with data size > 32. > > >> + geni_se_select_mode(&gi2c->se, mode); >> + writel_relaxed(msg->len, gi2c->se.base + SE_I2C_RX_TRANS_LEN); >> + geni_se_setup_m_cmd(&gi2c->se, I2C_READ, m_param); >> + if (mode == GENI_SE_DMA) { >> + rx_dma = geni_se_rx_dma_prep(&gi2c->se, msg->buf, msg->len); > > Randomly I noticed a flag called "I2C_M_DMA_SAFE". Do we need to > check this flag before using msg->buf for DMA? ...or use > i2c_get_dma_safe_msg_buf()? > > ...btw: the relative lack of people doing this in the kernel is > further evidence of DMA not really being worth it for i2c busses. I cannot comment about other drivers here using or not using DMA since they may not be exercised with slaves like NFC? >> + ret = pm_runtime_get_sync(gi2c->se.dev); >> + if (ret < 0) { >> + dev_err(gi2c->se.dev, "error turning SE resources:%d\n", ret); >> + pm_runtime_put_noidle(gi2c->se.dev); >> + /* Set device in suspended since resume failed */ >> + pm_runtime_set_suspended(gi2c->se.dev); >> + return ret; > > Wow, that's a cluster of arcane calls to handle a call that probably > will never fail (it just enables clocks and sets pinctrl). Sigh. > ...but as far as I can tell the whole sequence is right. You > definitely need a "put" after a failed get and it looks like > pm_runtime_set_suspended() has a special exception where it can be > called if you got a runtime error... We didn't have this in before either. But this condition is somewhat frequent if I2C transactions are called on cusp of exiting system suspend. (e.g. PMIC slave getting a wakeup-IRQ and trying to read from PMIC through I2C to read its status as to what caused that wake-up. At that time, get_sync doesn't really enable resources (kernel 4.9) since the runtime-pm ref-count isn't decremented. We run the risk of unclocked access if these arcane calls aren't present. You can go through runtime-pm documentation chapter 6 for more details. >> + ret = devm_request_irq(&pdev->dev, gi2c->irq, geni_i2c_irq, >> + IRQF_TRIGGER_HIGH, "i2c_geni", gi2c); >> + if (ret) { >> + dev_err(&pdev->dev, "Request_irq failed:%d: err:%d\n", >> + gi2c->irq, ret); >> + return ret; >> + } >> + disable_irq(gi2c->irq); > > Can you explain the goal of the disable_irq() here. Is it actually > needed for something or does it somehow save power? From drivers I've > reviewed in the past this doesn't seem like a common thing to do, so > I'm curious what it's supposed to gain for you. I'd be inclined to > just delete the whole disable/enable of the irq from this driver. Qualcomm's power team suggests we enable/disable unused IRQs. Otherwise they can block apps from entering some low-power mode (unless the interrupt is in some list?) I will confirm again with them and let you know. >> + /* Make sure no transactions are pending */ >> + ret = i2c_trylock_bus(&gi2c->adap, I2C_LOCK_SEGMENT); >> + if (!ret) { >> + dev_err(gi2c->se.dev, "late I2C transaction request\n"); >> + return -EBUSY; >> + } > > Does this happen? How? > > Nothing about this code looks special for your hardware. If this is > really needed, why is it not part of the i2c core since there's > nothing specific about your driver here? > There have been some clients that don't implement sys-suspend/resume callbacks (so i2c adapter has no clue they are done with their transactions) and this allows us to be flexible when they call I2C transactions extremely late. > >> + if (!pm_runtime_status_suspended(device)) { >> + geni_i2c_runtime_suspend(device); >> + pm_runtime_disable(device); >> + pm_runtime_set_suspended(device); >> + pm_runtime_enable(device); >> + } > > Similar question. Why do you need this special case code? Are there > cases where we're all the way at suspend_noirq and yet we still > haven't runtime suspended? Can you please document how we get into > this state? > This is when transaction happens less-than 250 msec of the system-suspend. PM-runtime has not gotten a chance to auto-suspend us since timer hasn't expired before system-suspend is attempted. These calls make sure that we truly turn off driver resources and make runtime-PM state consistent with the HW state. We can document this. Thanks Sagar -- Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
WARNING: multiple messages have this Message-ID (diff)
From: Sagar Dharia <sdharia@codeaurora.org> To: Doug Anderson <dianders@chromium.org>, Jonathan Corbet <corbet@lwn.net>, Andy Gross <andy.gross@linaro.org>, David Brown <david.brown@linaro.org>, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Wolfram Sang <wsa@the-dreams.de>, Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>, linux-doc@vger.kernel.org, linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, linux-i2c@vger.kernel.org, linux-serial@vger.kernel.org, Jiri Slaby <jslaby@suse.com>, evgreen@chromium.org, acourbot@chromium.org, Girish Mahadevan <girishm@codeaurora.org>, swboyd@chromium.org Subject: Re: [v3, 3/4] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller Date: Wed, 7 Mar 2018 19:42:24 -0700 [thread overview] Message-ID: <03c5ed5b-5160-bdd5-82f9-4a8beab33ca8@codeaurora.org> (raw) In-Reply-To: <CAD=FV=WbeP8xpVi7cji9hsNyxEam2D5D+_21ur_MuUO8FK42OQ@mail.gmail.com> Hi Doug, Thank you for reviewing the patch. I will take a stab at a few comments below. We will address most of the other comments in next version of I2C patch. > > >> + >> +#define I2C_AUTO_SUSPEND_DELAY 250 > > Why 250 ms? That seems like an eternity. Is it really that expensive > to turn resources off and on? I would sorta just expect clocks and > stuff to get turned off right after a transaction finished unless > another one was pending right behind it... > The response from RPMh to turn on/off shared resources also take quite a few msecs. The QUP serial bus block sits quite a few shared-NOCs away from the memory and runtime-PM is used a bandwidth vote/NOC vote for these NOCs from QUP to memory. Also the RPC between apps and RPMh can sometimes take longer depending on other tasks running on apps. This 250 msec avoids thrashing of these RPCs between apps and RPMh. If you plan to keep these NOCs on forever, then your are right: runtime-PM will be only used to turn on/off local clocks and we won't even need autosuspend. that's not true on products where this driver is currently deployed. >> + >> +static const struct geni_i2c_clk_fld geni_i2c_clk_map[] = { >> + {KHz(100), 7, 10, 11, 26}, >> + {KHz(400), 2, 5, 12, 24}, >> + {KHz(1000), 1, 3, 9, 18}, > > So I guess this is all relying on an input serial clock of 19.2MHz? > Maybe document that? > > Assuming I'm understanding the math here, is it really OK for your > 100kHz and 1MHz mode to be running slightly fast? > > 19200. / 2 / 24 >>>> 400.0 > > 19200. / 7 / 26 >>>> 105.49450549450549 > > 19200. / 1 / 18 >>>> 1066.6666666666667 > > It seems like you'd want the fastest clock that you can make that's > _less than_ the spec. > > > It would also be interesting to know if it's expected that boards > might need to tweak the t_high / t_low depending on their electrical > characteristics. In the past I've had lots of requests from board > makers to tweak things because they've got a long trace, or a stronger > or weaker pull, or ... If so we might later need to add some dts > properties like "i2c-scl-rising-time-ns" and make the math more > dynamic here, unless your hardware somehow automatically adjusts for > this type of thing... >These values are derived by our HW team to comply with the t-high and t-low specs of I2C. We have confirmed on scope that the frequency of SCL is indeed less than/equal to the spec. We have not come across slaves who have needed to tweak these things. We are open to adding these properties in dts if you have any such slaves not conforming due to board-layout of other reasons. >> + mode = msg->len > 32 ? GENI_SE_DMA : GENI_SE_FIFO; > > DMA is hard and i2c transfers > 32 bytes are rare. Do we really gain > a lot by transferring i2c commands over DMA compared to a FIFO? > Enough to justify the code complexity and the set of bugs that will > show up? I'm sure it will be a controversial assertion given that the > code's already written, but personally I'd be supportive of ripping > DMA mode out to simplify the driver. I'd be curious if anyone else > agrees. To me it seems like premature optimization. Yes, we have multiple clients (e.g. touch, NFC) using I2C for data transfers bigger than 32 bytes (some transfers are 100s of bytes). The fifo size is 32, so we can definitely avoid at least 1 interrupt when DMA mode is used with data size > 32. > > >> + geni_se_select_mode(&gi2c->se, mode); >> + writel_relaxed(msg->len, gi2c->se.base + SE_I2C_RX_TRANS_LEN); >> + geni_se_setup_m_cmd(&gi2c->se, I2C_READ, m_param); >> + if (mode == GENI_SE_DMA) { >> + rx_dma = geni_se_rx_dma_prep(&gi2c->se, msg->buf, msg->len); > > Randomly I noticed a flag called "I2C_M_DMA_SAFE". Do we need to > check this flag before using msg->buf for DMA? ...or use > i2c_get_dma_safe_msg_buf()? > > ...btw: the relative lack of people doing this in the kernel is > further evidence of DMA not really being worth it for i2c busses. I cannot comment about other drivers here using or not using DMA since they may not be exercised with slaves like NFC? >> + ret = pm_runtime_get_sync(gi2c->se.dev); >> + if (ret < 0) { >> + dev_err(gi2c->se.dev, "error turning SE resources:%d\n", ret); >> + pm_runtime_put_noidle(gi2c->se.dev); >> + /* Set device in suspended since resume failed */ >> + pm_runtime_set_suspended(gi2c->se.dev); >> + return ret; > > Wow, that's a cluster of arcane calls to handle a call that probably > will never fail (it just enables clocks and sets pinctrl). Sigh. > ...but as far as I can tell the whole sequence is right. You > definitely need a "put" after a failed get and it looks like > pm_runtime_set_suspended() has a special exception where it can be > called if you got a runtime error... We didn't have this in before either. But this condition is somewhat frequent if I2C transactions are called on cusp of exiting system suspend. (e.g. PMIC slave getting a wakeup-IRQ and trying to read from PMIC through I2C to read its status as to what caused that wake-up. At that time, get_sync doesn't really enable resources (kernel 4.9) since the runtime-pm ref-count isn't decremented. We run the risk of unclocked access if these arcane calls aren't present. You can go through runtime-pm documentation chapter 6 for more details. >> + ret = devm_request_irq(&pdev->dev, gi2c->irq, geni_i2c_irq, >> + IRQF_TRIGGER_HIGH, "i2c_geni", gi2c); >> + if (ret) { >> + dev_err(&pdev->dev, "Request_irq failed:%d: err:%d\n", >> + gi2c->irq, ret); >> + return ret; >> + } >> + disable_irq(gi2c->irq); > > Can you explain the goal of the disable_irq() here. Is it actually > needed for something or does it somehow save power? From drivers I've > reviewed in the past this doesn't seem like a common thing to do, so > I'm curious what it's supposed to gain for you. I'd be inclined to > just delete the whole disable/enable of the irq from this driver. Qualcomm's power team suggests we enable/disable unused IRQs. Otherwise they can block apps from entering some low-power mode (unless the interrupt is in some list?) I will confirm again with them and let you know. >> + /* Make sure no transactions are pending */ >> + ret = i2c_trylock_bus(&gi2c->adap, I2C_LOCK_SEGMENT); >> + if (!ret) { >> + dev_err(gi2c->se.dev, "late I2C transaction request\n"); >> + return -EBUSY; >> + } > > Does this happen? How? > > Nothing about this code looks special for your hardware. If this is > really needed, why is it not part of the i2c core since there's > nothing specific about your driver here? > There have been some clients that don't implement sys-suspend/resume callbacks (so i2c adapter has no clue they are done with their transactions) and this allows us to be flexible when they call I2C transactions extremely late. > >> + if (!pm_runtime_status_suspended(device)) { >> + geni_i2c_runtime_suspend(device); >> + pm_runtime_disable(device); >> + pm_runtime_set_suspended(device); >> + pm_runtime_enable(device); >> + } > > Similar question. Why do you need this special case code? Are there > cases where we're all the way at suspend_noirq and yet we still > haven't runtime suspended? Can you please document how we get into > this state? > This is when transaction happens less-than 250 msec of the system-suspend. PM-runtime has not gotten a chance to auto-suspend us since timer hasn't expired before system-suspend is attempted. These calls make sure that we truly turn off driver resources and make runtime-PM state consistent with the HW state. We can document this. Thanks Sagar -- Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2018-03-08 2:42 UTC|newest] Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-02-28 1:38 [PATCH v3 0/4] Introduce GENI SE Controller Driver Karthikeyan Ramasubramanian 2018-02-28 1:38 ` [PATCH v3 1/4] dt-bindings: soc: qcom: Add device tree binding for GENI SE Karthikeyan Ramasubramanian 2018-03-05 23:58 ` Rob Herring 2018-03-05 23:58 ` Rob Herring 2018-03-06 0:55 ` Karthik Ramasubramanian 2018-03-06 0:55 ` Karthik Ramasubramanian 2018-03-06 13:22 ` Rob Herring 2018-03-06 13:22 ` Rob Herring 2018-03-06 17:13 ` Karthik Ramasubramanian 2018-03-06 17:13 ` Karthik Ramasubramanian 2018-02-28 1:38 ` [PATCH v3 2/4] soc: qcom: Add GENI based QUP Wrapper driver Karthikeyan Ramasubramanian 2018-03-02 20:41 ` Stephen Boyd 2018-03-02 20:41 ` Stephen Boyd 2018-03-02 20:58 ` Evan Green 2018-03-02 20:58 ` Evan Green 2018-03-03 0:58 ` Karthik Ramasubramanian 2018-03-03 0:58 ` Karthik Ramasubramanian 2018-03-06 21:56 ` Stephen Boyd 2018-03-06 21:56 ` Stephen Boyd [not found] ` <152037339742.218381.11498404122038956963-n1Xw8LXHxjTHt/MElyovVYaSKrA+ACpX0E9HWUfgJXw@public.gmane.org> 2018-03-08 6:46 ` Karthik Ramasubramanian 2018-03-08 6:46 ` Karthik Ramasubramanian [not found] ` <945b6c00-dde6-6ec7-4577-4cc0d034796b-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> 2018-03-08 13:24 ` Robin Murphy 2018-03-08 13:24 ` Robin Murphy [not found] ` <8567be1b-1431-4f1d-cb41-6a7eaa434438-5wv7dgnIgG8@public.gmane.org> 2018-03-08 14:41 ` Christoph Hellwig 2018-03-08 14:41 ` Christoph Hellwig 2018-03-08 18:18 ` Karthik Ramasubramanian 2018-03-08 18:18 ` Karthik Ramasubramanian 2018-03-09 18:18 ` Karthik Ramasubramanian 2018-03-09 18:18 ` Karthik Ramasubramanian 2018-02-28 1:38 ` [PATCH v3 3/4] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller Karthikeyan Ramasubramanian 2018-03-07 21:16 ` [v3, " Doug Anderson 2018-03-07 21:16 ` Doug Anderson 2018-03-08 2:42 ` Sagar Dharia [this message] 2018-03-08 2:42 ` Sagar Dharia 2018-03-08 5:19 ` Doug Anderson 2018-03-08 5:19 ` Doug Anderson 2018-03-08 21:12 ` Doug Anderson 2018-03-08 21:12 ` Doug Anderson 2018-03-09 1:06 ` Sagar Dharia 2018-03-09 1:06 ` Sagar Dharia 2018-03-09 5:02 ` Doug Anderson 2018-03-09 5:02 ` Doug Anderson 2018-03-09 1:27 ` Sagar Dharia 2018-03-09 1:27 ` Sagar Dharia 2018-03-09 6:43 ` Karthik Ramasubramanian 2018-03-09 6:43 ` Karthik Ramasubramanian 2018-02-28 1:38 ` [PATCH v3 4/4] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP Karthikeyan Ramasubramanian 2018-03-02 22:11 ` Stephen Boyd 2018-03-02 22:11 ` Stephen Boyd 2018-03-06 0:51 ` Karthik Ramasubramanian 2018-03-06 0:51 ` Karthik Ramasubramanian 2018-03-06 21:45 ` Stephen Boyd 2018-03-06 21:45 ` Stephen Boyd 2018-03-08 6:06 ` Karthik Ramasubramanian 2018-03-08 6:06 ` Karthik Ramasubramanian 2018-03-08 22:32 ` Stephen Boyd 2018-03-08 22:32 ` Stephen Boyd 2018-03-09 4:57 ` Karthik Ramasubramanian 2018-03-09 4:57 ` Karthik Ramasubramanian 2018-03-03 0:11 ` Evan Green 2018-03-03 0:11 ` Evan Green 2018-03-13 20:16 ` Karthik Ramasubramanian 2018-03-13 20:16 ` Karthik Ramasubramanian 2018-03-16 18:39 ` Evan Green 2018-03-16 18:39 ` Evan Green
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=03c5ed5b-5160-bdd5-82f9-4a8beab33ca8@codeaurora.org \ --to=sdharia@codeaurora.org \ --cc=acourbot@chromium.org \ --cc=andy.gross@linaro.org \ --cc=corbet@lwn.net \ --cc=david.brown@linaro.org \ --cc=devicetree@vger.kernel.org \ --cc=dianders@chromium.org \ --cc=evgreen@chromium.org \ --cc=girishm@codeaurora.org \ --cc=gregkh@linuxfoundation.org \ --cc=jslaby@suse.com \ --cc=kramasub@codeaurora.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-doc@vger.kernel.org \ --cc=linux-i2c@vger.kernel.org \ --cc=linux-serial@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=robh+dt@kernel.org \ --cc=swboyd@chromium.org \ --cc=wsa@the-dreams.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.