Hi Doug,
On 3/7/2018 10:19 PM, Doug Anderson wrote:
Hi,
On Wed, Mar 7, 2018 at 6:42 PM, Sagar Dharia <sdharia@xxxxxxxxxxxxxx> wrote:
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.
+
+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.
OK, I'm fine with leaving something like this till later if/when it
comes up. Documenting a little bit more about how these timings work
seems like it would be nice, though, at least mentioning what the
source clock is...
Yes, we will document how t-high and t-low is derived from source.
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.
Yeah, I certainly agree that the calls are needed if
pm_runtime_get_sync() and I'm not suggesting removing them. Hence the
"as far as I can tell the whole sequence is right".
...but I'm actually kinda worried if you're saying that you actually
ran into this case. Hopefully that got fixed and code no longer tries
to read from the PMIC at a bad time anymore? That code should be
fixed not to be running so late in suspend.
I have added Harry Y and Abhijeet D (developers for PMIC I2C client
team). They can comment if there is still a usecase of very late
transaction during suspend and/or very early transaction during resume.
+ /* 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.
Still feels like this belongs in the i2c core, not your driver. Maybe
you should send a patch for the core and remove it from here?
...and also, it seems like any i2c clients that don't implement the
suspend/resume callbacks and try to do i2c transactions late in the
game need to be fixed. It should be documented that this isn't a
valid thing for a driver to do and if we end up in this error case
then it's not an i2c issue but it's a bad driver somewhere.
You are right: this check is special for our HW due to usecases
mentioned above.
This check can go in i2c-core, but then it will not be necessary if
all adapters and clients that we work with are upstreamed (and all
implement system suspend/resume).
We will remove this in next version of geni i2c-adapter driver.
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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
