| Message ID | 1521071931-9294-7-git-send-email-kramasub@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi, On Wed, Mar 14, 2018 at 4:58 PM, Karthikeyan Ramasubramanian <kramasub@codeaurora.org> wrote: > Add I2C master controller support for a built-in test I2C slave. > > Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> > --- > arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 19 +++++++++++++++++++ > arch/arm64/boot/dts/qcom/sdm845.dtsi | 29 +++++++++++++++++++++++++++++ > 2 files changed, 48 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts > index ea3efc5..69445f1 100644 > --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts > +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts > @@ -27,6 +27,10 @@ > serial@a84000 { > status = "okay"; > }; > + > + i2c@a88000 { > + status = "okay"; Any idea what clock frequency is appropriate for MTP? You've got some pretty beefy 2.2K external pulls I think, so presumably 400 KHz is what you're aiming for? It would be nice to specify one so you don't end up the the spam in the log "Bus frequency not specified ..." > + }; > }; > > pinctrl@3400000 { > @@ -50,5 +54,20 @@ > bias-pull-down; > }; > }; > + > + qup-i2c10-default { nit: probably in the pinctrl section we want to sort alphabetically? We could try to sort by "lowest numbered pin", but IMHO alphabetical makes the most sense. > + 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. > + }; > + }; > }; > }; > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi > index 59334d9..9ef056f 100644 > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi > @@ -209,6 +209,21 @@ > pins = "gpio4", "gpio5"; > }; > }; > + > + qup_i2c10_default: qup-i2c10-default { > + pinmux { > + function = "qup10"; > + pins = "gpio55", "gpio56"; > + }; > + }; > + > + qup_i2c10_sleep: qup-i2c10-sleep { > + pinmux { > + function = "gpio"; > + pins = "gpio55", "gpio56"; > + }; > + }; > + > }; > > timer@17c90000 { > @@ -309,6 +324,20 @@ > interrupts = <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>; > status = "disabled"; > }; > + > + 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. > + compatible = "qcom,geni-i2c"; > + reg = <0xa88000 0x4000>; > + clock-names = "se"; > + clocks = <&gcc GCC_QUPV3_WRAP1_S2_CLK>; > + pinctrl-names = "default", "sleep"; > + pinctrl-0 = <&qup_i2c10_default>; > + pinctrl-1 = <&qup_i2c10_sleep>; > + interrupts = <GIC_SPI 355 IRQ_TYPE_LEVEL_HIGH>; > + #address-cells = <1>; > + #size-cells = <0>; > + status = "disabled"; > + }; > }; > }; > }; > -- > 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Doug, Thanks for reviewing the patch. On 3/16/2018 5:54 PM, Doug Anderson wrote: > Hi, > > On Wed, Mar 14, 2018 at 4:58 PM, Karthikeyan Ramasubramanian > <kramasub@codeaurora.org> wrote: >> Add I2C master controller support for a built-in test I2C slave. >> >> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> >> --- >> arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 19 +++++++++++++++++++ >> arch/arm64/boot/dts/qcom/sdm845.dtsi | 29 +++++++++++++++++++++++++++++ >> 2 files changed, 48 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts >> index ea3efc5..69445f1 100644 >> --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts >> +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts >> @@ -27,6 +27,10 @@ >> serial@a84000 { >> status = "okay"; >> }; >> + >> + i2c@a88000 { >> + status = "okay"; > > Any idea what clock frequency is appropriate for MTP? You've got some > pretty beefy 2.2K external pulls I think, so presumably 400 KHz is > what you're aiming for? It would be nice to specify one so you don't > end up the the spam in the log "Bus frequency not specified ..." > > Yes, we plan to use 400Khz. We will specify it here. >> + }; >> }; >> >> pinctrl@3400000 { >> @@ -50,5 +54,20 @@ >> bias-pull-down; >> }; >> }; >> + >> + qup-i2c10-default { > > nit: probably in the pinctrl section we want to sort alphabetically? > We could try to sort by "lowest numbered pin", but IMHO alphabetical > makes the most sense. > Sure. > >> + 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. > >> + }; >> + }; >> }; >> }; >> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi >> index 59334d9..9ef056f 100644 >> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi >> @@ -209,6 +209,21 @@ >> pins = "gpio4", "gpio5"; >> }; >> }; >> + >> + qup_i2c10_default: qup-i2c10-default { >> + pinmux { >> + function = "qup10"; >> + pins = "gpio55", "gpio56"; >> + }; >> + }; >> + >> + qup_i2c10_sleep: qup-i2c10-sleep { >> + pinmux { >> + function = "gpio"; >> + pins = "gpio55", "gpio56"; >> + }; >> + }; >> + >> }; >> >> timer@17c90000 { >> @@ -309,6 +324,20 @@ >> interrupts = <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>; >> status = "disabled"; >> }; >> + >> + 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? Thanks Sagar > >> + compatible = "qcom,geni-i2c"; >> + reg = <0xa88000 0x4000>; >> + clock-names = "se"; >> + clocks = <&gcc GCC_QUPV3_WRAP1_S2_CLK>; >> + pinctrl-names = "default", "sleep"; >> + pinctrl-0 = <&qup_i2c10_default>; >> + pinctrl-1 = <&qup_i2c10_sleep>; >> + interrupts = <GIC_SPI 355 IRQ_TYPE_LEVEL_HIGH>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + status = "disabled"; >> + }; >> }; >> }; >> }; >> -- >> 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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Hi, On Mon, Mar 19, 2018 at 3:15 PM, Sagar Dharia <sdharia@codeaurora.org> 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. >>> + }; >>> + }; >>> }; >>> }; >>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi >>> index 59334d9..9ef056f 100644 >>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi >>> @@ -209,6 +209,21 @@ >>> pins = "gpio4", "gpio5"; >>> }; >>> }; >>> + >>> + qup_i2c10_default: qup-i2c10-default { >>> + pinmux { >>> + function = "qup10"; >>> + pins = "gpio55", "gpio56"; >>> + }; >>> + }; >>> + >>> + qup_i2c10_sleep: qup-i2c10-sleep { >>> + pinmux { >>> + function = "gpio"; >>> + pins = "gpio55", "gpio56"; >>> + }; >>> + }; >>> + >>> }; >>> >>> timer@17c90000 { >>> @@ -309,6 +324,20 @@ >>> interrupts = <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>; >>> status = "disabled"; >>> }; >>> + >>> + 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". -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Doug Anderson (2018-03-19 16:56:27) > On Mon, Mar 19, 2018 at 3:15 PM, Sagar Dharia <sdharia@codeaurora.org> wrote: > > > > 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. Upstream puts all SoC nodes in the SoC file. The split file by functional area method is to avoid merge conflicts in the SoC file and that doesn't make sense outside of the internal workflow. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 3/19/2018 5:56 PM, Doug Anderson wrote: > Hi, > > On Mon, Mar 19, 2018 at 3:15 PM, Sagar Dharia <sdharia@codeaurora.org> 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. We will change sleep-config of I2C GPIOs to no-pull. > >>>> + }; >>>> + }; >>>> }; >>>> }; >>>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi >>>> index 59334d9..9ef056f 100644 >>>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi >>>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi >>>> @@ -209,6 +209,21 @@ >>>> pins = "gpio4", "gpio5"; >>>> }; >>>> }; >>>> + >>>> + qup_i2c10_default: qup-i2c10-default { >>>> + pinmux { >>>> + function = "qup10"; >>>> + pins = "gpio55", "gpio56"; >>>> + }; >>>> + }; >>>> + >>>> + qup_i2c10_sleep: qup-i2c10-sleep { >>>> + pinmux { >>>> + function = "gpio"; >>>> + pins = "gpio55", "gpio56"; >>>> + }; >>>> + }; >>>> + >>>> }; >>>> >>>> timer@17c90000 { >>>> @@ -309,6 +324,20 @@ >>>> interrupts = <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>; >>>> status = "disabled"; >>>> }; >>>> + >>>> + 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. Thanks Sagar > > -Doug > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Hi, On Tue, Mar 20, 2018 at 3:16 PM, Sagar Dharia <sdharia@codeaurora.org> 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. >>>>> + 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@codeaurora.org> 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts index ea3efc5..69445f1 100644 --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts @@ -27,6 +27,10 @@ serial@a84000 { status = "okay"; }; + + i2c@a88000 { + status = "okay"; + }; }; pinctrl@3400000 { @@ -50,5 +54,20 @@ bias-pull-down; }; }; + + qup-i2c10-default { + pinconf { + pins = "gpio55", "gpio56"; + drive-strength = <2>; + bias-disable; + }; + }; + + qup-i2c10-sleep { + pinconf { + pins = "gpio55", "gpio56"; + bias-pull-up; + }; + }; }; }; diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi index 59334d9..9ef056f 100644 --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi @@ -209,6 +209,21 @@ pins = "gpio4", "gpio5"; }; }; + + qup_i2c10_default: qup-i2c10-default { + pinmux { + function = "qup10"; + pins = "gpio55", "gpio56"; + }; + }; + + qup_i2c10_sleep: qup-i2c10-sleep { + pinmux { + function = "gpio"; + pins = "gpio55", "gpio56"; + }; + }; + }; timer@17c90000 { @@ -309,6 +324,20 @@ interrupts = <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>; status = "disabled"; }; + + i2c10: i2c@a88000 { + compatible = "qcom,geni-i2c"; + reg = <0xa88000 0x4000>; + clock-names = "se"; + clocks = <&gcc GCC_QUPV3_WRAP1_S2_CLK>; + pinctrl-names = "default", "sleep"; + pinctrl-0 = <&qup_i2c10_default>; + pinctrl-1 = <&qup_i2c10_sleep>; + interrupts = <GIC_SPI 355 IRQ_TYPE_LEVEL_HIGH>; + #address-cells = <1>; + #size-cells = <0>; + status = "disabled"; + }; }; }; };
Add I2C master controller support for a built-in test I2C slave. Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> --- arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 19 +++++++++++++++++++ arch/arm64/boot/dts/qcom/sdm845.dtsi | 29 +++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+)