Hi Doug
On 3/20/2018 9:47 PM, Doug Anderson wrote:
> Hi,
>
> On Tue, Mar 20, 2018 at 3:16 PM, Sagar Dharia <sdharia@xxxxxxxxxxxxxx> wrote:
>>>>>> + pinconf {
>>>>>> + pins = "gpio55", "gpio56";
>>>>>> + drive-strength = <2>;
>>>>>> + bias-disable;
>>>>>> + };
>>>>>> + };
>>>>>> +
>>>>>> + qup-i2c10-sleep {
>>>>>> + pinconf {
>>>>>> + pins = "gpio55", "gpio56";
>>>>>> + bias-pull-up;
>>>>>
>>>>> Are you sure that you want pullups enabled for sleep here? There are
>>>>> external pulls on this line (as there are on many i2c busses) so doing
>>>>> this will double-enable pulls. It probably won't hurt, but I'm
>>>>> curious if there's some sort of reason here.
>>>>>
>>>> 1. We need the lines to remain high to avoid slaves sensing a false
>>>> start-condition (this can happen if the SDA goes down before SCL).
>>>> 2. Disclaimer: I'm not a HW expert, but we were told that
>>>> tri-state/bias-disabled lines can draw more current. I will find out
>>>> more about that.
>>>
>>> Agreed that they need to remain high, but you've got very strong
>>> pullups external to the SoC. Those will keep it high. You don't need
>>> the internal ones too.
>>>
>>> As extra evidence that the external pullups _must_ be present on your
>>> board: you specify bias-disable in the active state. That can only
>>> work if there are external pullups (or if there were some special
>>> extra secret internal pullups that were part of geni). i2c is an
>>> open-drain bus and thus there must be pullups on the bus in order to
>>> communicate.
>>>
>>
>> You are right, I followed up about the pull-up recommendation and that
>> was for a GPIO where there was no external pull-up (GPIO was not used
>> for I2C). It's safe to assume I2C will always have external pullup.
>
> It is even more safe to say that I2C will always have an external
> pullup on the SDM845-MTP. Remember that the pullup config is in the
> board device tree file, not the SoC one. So even if someone out there
> decides that the internal pull is somehow good enough for their own
> board and they don't stuff external ones, then it will be up to them
> to turn the pull up on in their own board file.
>
>
>> We
>> will change sleep-config of I2C GPIOs to no-pull.
>
> Even better IMHO: don't specify the bias in the sleep config. I don't
> believe it's possible for the sleep config to take effect without the
> default config since the default config applies at probe time. ...so
> you'll always get the default config applied at probe time and you
> don't need to touch the bias at sleep time.
Good point, we will remove the bias from the sleep config for i2c GPIOs.
Thanks
Sagar
>
>
>>>>>> + i2c10: i2c@a88000 {
>>>>>
>>>>> Seems like it might be nice to add all the i2c busses into the main
>>>>> sdm845.dtsi file. Sure, most won't be enabled, but it seems like it
>>>>> would avoid churn later.
>>>>>
>>>>> ...if you're sure you want to add only one i2c controller, subject of
>>>>> this patch should indicate that.
>>>>>
>>>>
>>>> Yes, we typically have a "platform(sdm845 here)-qupv3.dtsi" defining
>>>> most of the serial-bus instances (i2c, spi, and uart with
>>>> status=disabled) that we include from the common header. The boards
>>>> enable instances they need.
>>>> Will that be okay?
>>>
>>> Unless you really feel the need to put these in a separate file I'd
>>> just put them straight in sdm845.dtsi. Yeah, it'll get big, but
>>> that's OK by me. I _think_ this matches what Bjorn was suggesting on
>>> previous device tree patches, but CCing him just in case. I'm
>>> personally OK with whatever Bjorn and other folks with more Qualcomm
>>> history would like.
>>>
>>> ...but yeah, I'm asking for them all to be listed with status="disabled".
>>>
>>
>> Sure, we will change the subject of this patch to indicate that we are
>> adding 1 controller as of now. Later we will add all I2C controllers to
>> dtsi as another patch since that will need pinctrl settings for GPIOs
>> used by those instances and the wrappers devices needed by them.
>
> Yeah, it's fine to just change the subject of this patch. It would be
> nice to add all the other controllers in sooner rather than later, but
> it doesn't have to be today.
>
>
> -Doug
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
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-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
