Re: [PATCH v4 6/6] arm64: dts: sdm845: Add I2C controller support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux