| Message ID | 20170517083745.24479-6-guodong.xu@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 17, 2017 at 04:37:38PM +0800, Guodong Xu wrote: > From: Zhangfei Gao <zhangfei.gao@linaro.org> > > Add I2C nodes for Hi3660-hikey960. > > On HiKey960, > I2C0, I2C7 is connected to Low Speed Expansion Connector. > I2C1 is connected to ADV7535. > I2C3 is connected to USB5734. > > Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> > Signed-off-by: Guodong Xu <guodong.xu@linaro.org> > --- > arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts | 18 ++++++++ > arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 56 +++++++++++++++++++++++ > 2 files changed, 74 insertions(+) > > diff --git a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts > index 64875a5..f685b1e 100644 > --- a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts > +++ b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts > @@ -29,6 +29,24 @@ > }; > }; > > +&i2c0 { > + status = "okay"; > +}; > + > +&i2c1 { > + status = "okay"; > + > + adv7533: adv7533@39 { > + status = "ok"; > + compatible = "adi,adv7533"; > + reg = <0x39>; > + }; > +}; > + > +&i2c7 { > + status = "okay"; > +}; labels for the LS connector? > + > &uart5 { > status = "okay"; > }; > diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi > index f55710a..f217c9d 100644 > --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi > +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi > @@ -186,6 +186,62 @@ > #reset-cells = <2>; > }; > > + i2c0: i2c@FFD71000 { lowercase hex please. > + compatible = "snps,designware-i2c"; These should have an SoC specific compatible. > + reg = <0x0 0xFFD71000 0x0 0x1000>; > + interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>; > + #address-cells = <1>; > + #size-cells = <0>; > + clock-frequency = <400000>; > + clocks = <&crg_ctrl HI3660_CLK_GATE_I2C0>; > + resets = <&iomcu_rst 0x20 3>; > + pinctrl-names = "default"; > + pinctrl-0 = <&i2c0_pmx_func &i2c0_cfg_func>; > + status = "disabled"; > + }; > + > + i2c1: i2c@FFD72000 { ditto > + compatible = "snps,designware-i2c"; > + reg = <0x0 0xFFD72000 0x0 0x1000>; > + interrupts = <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>; > + #address-cells = <1>; > + #size-cells = <0>; > + clock-frequency = <400000>; > + clocks = <&crg_ctrl HI3660_CLK_GATE_I2C1>; > + resets = <&iomcu_rst 0x20 4>; > + pinctrl-names = "default"; > + pinctrl-0 = <&i2c1_pmx_func &i2c1_cfg_func>; > + status = "disabled"; > + }; > + > + i2c3: i2c@FDF0C000 { > + compatible = "snps,designware-i2c"; > + reg = <0x0 0xFDF0C000 0x0 0x1000>; > + interrupts = <GIC_SPI 81 IRQ_TYPE_LEVEL_HIGH>; > + #address-cells = <1>; > + #size-cells = <0>; > + clock-frequency = <400000>; > + clocks = <&crg_ctrl HI3660_CLK_GATE_I2C3>; > + resets = <&crg_rst 0x78 7>; > + pinctrl-names = "default"; > + pinctrl-0 = <&i2c3_pmx_func &i2c3_cfg_func>; > + status = "disabled"; > + }; > + > + i2c7: i2c@FDF0B000 { > + compatible = "snps,designware-i2c"; > + reg = <0x0 0xFDF0B000 0x0 0x1000>; > + interrupts = <GIC_SPI 314 IRQ_TYPE_LEVEL_HIGH>; > + #address-cells = <1>; > + #size-cells = <0>; > + clock-frequency = <400000>; > + clocks = <&crg_ctrl HI3660_CLK_GATE_I2C7>; > + resets = <&crg_rst 0x60 14>; > + pinctrl-names = "default"; > + pinctrl-0 = <&i2c7_pmx_func &i2c7_cfg_func>; > + status = "disabled"; > + }; > + > uart5: serial@fdf05000 { > compatible = "arm,pl011", "arm,primecell"; > reg = <0x0 0xfdf05000 0x0 0x1000>; > -- > 2.10.2 >
Hi, Rob Thanks for the review. On 2017年05月23日 08:39, Rob Herring wrote: > On Wed, May 17, 2017 at 04:37:38PM +0800, Guodong Xu wrote: >> From: Zhangfei Gao <zhangfei.gao@linaro.org> >> >> Add I2C nodes for Hi3660-hikey960. >> >> On HiKey960, >> I2C0, I2C7 is connected to Low Speed Expansion Connector. >> I2C1 is connected to ADV7535. >> I2C3 is connected to USB5734. >> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> >> Signed-off-by: Guodong Xu <guodong.xu@linaro.org> >> --- >> arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts | 18 ++++++++ >> arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 56 +++++++++++++++++++++++ >> 2 files changed, 74 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts >> index 64875a5..f685b1e 100644 >> --- a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts >> +++ b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts >> @@ -29,6 +29,24 @@ >> }; >> }; >> >> +&i2c0 { >> + status = "okay"; >> +}; >> + >> +&i2c1 { >> + status = "okay"; >> + >> + adv7533: adv7533@39 { >> + status = "ok"; >> + compatible = "adi,adv7533"; >> + reg = <0x39>; >> + }; >> +}; >> + >> +&i2c7 { >> + status = "okay"; >> +}; > labels for the LS connector? Any examples? There is compile error if only change dts like ls-connector { &i2c7 { status = "okay"; }; }; > >> + >> &uart5 { >> status = "okay"; >> }; >> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi >> index f55710a..f217c9d 100644 >> --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi >> +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi >> @@ -186,6 +186,62 @@ >> #reset-cells = <2>; >> }; >> >> + i2c0: i2c@FFD71000 { > lowercase hex please. Yes, will change > >> + compatible = "snps,designware-i2c"; > These should have an SoC specific compatible. We directly use drivers/i2c/busses/i2c-designware-platdrv.c, do we still an soc specific compatible? Checked arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi, and other examples, compatible = "snps,designware-i2c" is used. Thanks
On 2017年05月23日 13:55, zhangfei wrote: > Hi, Rob > > > Thanks for the review. > > On 2017年05月23日 08:39, Rob Herring wrote: >> On Wed, May 17, 2017 at 04:37:38PM +0800, Guodong Xu wrote: >>> From: Zhangfei Gao <zhangfei.gao@linaro.org> >>> >>> Add I2C nodes for Hi3660-hikey960. >>> >>> On HiKey960, >>> I2C0, I2C7 is connected to Low Speed Expansion Connector. >>> I2C1 is connected to ADV7535. >>> I2C3 is connected to USB5734. >>> >>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> >>> Signed-off-by: Guodong Xu <guodong.xu@linaro.org> >>> --- >>> arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts | 18 ++++++++ >>> arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 56 >>> +++++++++++++++++++++++ >>> 2 files changed, 74 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts >>> b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts >>> index 64875a5..f685b1e 100644 >>> --- a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts >>> +++ b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts >>> @@ -29,6 +29,24 @@ >>> }; >>> }; >>> +&i2c0 { >>> + status = "okay"; >>> +}; >>> + >>> +&i2c1 { >>> + status = "okay"; >>> + >>> + adv7533: adv7533@39 { >>> + status = "ok"; >>> + compatible = "adi,adv7533"; >>> + reg = <0x39>; >>> + }; >>> +}; >>> + >>> +&i2c7 { >>> + status = "okay"; >>> +}; >> labels for the LS connector? Do you mean arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi i2c@78ba000 { /* On Low speed expansion */ label = "LS-I2C1"; status = "okay"; }; spi@78b7000 { /* On High speed expansion */ label = "HS-SPI1"; status = "okay"; }; Thanks >> >>> + compatible = "snps,designware-i2c"; >> These should have an SoC specific compatible. > We directly use drivers/i2c/busses/i2c-designware-platdrv.c, > do we still an soc specific compatible? > Checked arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi, and other examples, > > compatible = "snps,designware-i2c" is used. > > > Thanks >
On Tue, May 23, 2017 at 1:36 AM, zhangfei <zhangfei.gao@linaro.org> wrote: > > > On 2017年05月23日 13:55, zhangfei wrote: >> >> Hi, Rob >> >> >> Thanks for the review. >> >> On 2017年05月23日 08:39, Rob Herring wrote: >>> >>> On Wed, May 17, 2017 at 04:37:38PM +0800, Guodong Xu wrote: >>>> >>>> From: Zhangfei Gao <zhangfei.gao@linaro.org> >>>> >>>> Add I2C nodes for Hi3660-hikey960. >>>> >>>> On HiKey960, >>>> I2C0, I2C7 is connected to Low Speed Expansion Connector. >>>> I2C1 is connected to ADV7535. >>>> I2C3 is connected to USB5734. >>>> >>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> >>>> Signed-off-by: Guodong Xu <guodong.xu@linaro.org> >>>> --- >>>> arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts | 18 ++++++++ >>>> arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 56 >>>> +++++++++++++++++++++++ >>>> 2 files changed, 74 insertions(+) >>>> >>>> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts >>>> b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts >>>> index 64875a5..f685b1e 100644 >>>> --- a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts >>>> +++ b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts >>>> @@ -29,6 +29,24 @@ >>>> }; >>>> }; >>>> +&i2c0 { >>>> + status = "okay"; >>>> +}; >>>> + >>>> +&i2c1 { >>>> + status = "okay"; >>>> + >>>> + adv7533: adv7533@39 { >>>> + status = "ok"; >>>> + compatible = "adi,adv7533"; >>>> + reg = <0x39>; >>>> + }; >>>> +}; >>>> + >>>> +&i2c7 { >>>> + status = "okay"; >>>> +}; >>> >>> labels for the LS connector? > > > Do you mean arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi > > i2c@78ba000 { > /* On Low speed expansion */ > label = "LS-I2C1"; > status = "okay"; > }; > > spi@78b7000 { > /* On High speed expansion */ > label = "HS-SPI1"; > status = "okay"; > }; Yes. And the names need to align across boards to be useful. Rob
On Tue, May 23, 2017 at 12:55 AM, zhangfei <zhangfei.gao@linaro.org> wrote: > Hi, Rob > > > Thanks for the review. > > > On 2017年05月23日 08:39, Rob Herring wrote: >> >> On Wed, May 17, 2017 at 04:37:38PM +0800, Guodong Xu wrote: >>> >>> From: Zhangfei Gao <zhangfei.gao@linaro.org> >>> >>> Add I2C nodes for Hi3660-hikey960. >>> >>> On HiKey960, >>> I2C0, I2C7 is connected to Low Speed Expansion Connector. >>> I2C1 is connected to ADV7535. >>> I2C3 is connected to USB5734. [...] >>> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi >>> b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi >>> index f55710a..f217c9d 100644 >>> --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi >>> +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi >>> @@ -186,6 +186,62 @@ >>> #reset-cells = <2>; >>> }; >>> + i2c0: i2c@FFD71000 { >> >> lowercase hex please. > > Yes, will change >> >> >>> + compatible = "snps,designware-i2c"; >> >> These should have an SoC specific compatible. > > We directly use drivers/i2c/busses/i2c-designware-platdrv.c, > do we still an soc specific compatible? It's fine if the driver uses the snps compatible, but the dts should still have an SoC specific one. > Checked arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi, and other examples, > > compatible = "snps,designware-i2c" is used. That was a mistake in the other platforms. We shouldn't continue repeating it. Rob
Hi, Jarkko Would you mind give some suggestion? On 2017年05月23日 20:48, Rob Herring wrote: > >>>> + compatible = "snps,designware-i2c"; >>> These should have an SoC specific compatible. >> We directly use drivers/i2c/busses/i2c-designware-platdrv.c, >> do we still an soc specific compatible? > It's fine if the driver uses the snps compatible, but the dts should > still have an SoC specific one. > >> Checked arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi, and other examples, >> >> compatible = "snps,designware-i2c" is used. > That was a mistake in the other platforms. We shouldn't continue repeating it. > > Rob Rob suggest add something like "hisilicon,hi3660-dw-i2c" as well. "The problem is dw-i2c does not give any clue as to what the configuration or version of the IP is. Is that fully discoverable with version/capability registers? If not then you need a specific compatible. Generally when we have not required them, it ends up being a problem later on." While Documentation/devicetree/bindings/i2c/i2c-designware.txt compatible : should be "snps,designware-i2c" Besides, on Hikey960, [ 3.822353] dw_readl(dev, DW_IC_COMP_VERSION)=0x3132302a [ 3.827763] dw_readl(dev, DW_IC_COMP_TYPE)=0x44570140 Are these two registers enough to distinguish version etc? Thanks
On 05/24/2017 05:34 AM, zhangfei wrote: > Rob suggest add something like "hisilicon,hi3660-dw-i2c" as well. > "The problem is dw-i2c does not give any clue as to what the > configuration or version of the IP is. > Is that fully discoverable with version/capability registers? If not > then you need a specific compatible. > Generally when we have not required them, it ends up being a problem > later on." > Some features are discoverable from DW_IC_COMP_PARAM_1, DW_IC_COMP_VERSION and DW_IC_COMP_TYPE registers. Although my specification does not document what's inside of DW_IC_COMP_VERSION and DW_IC_COMP_TYPE but code is using them. > > While Documentation/devicetree/bindings/i2c/i2c-designware.txt > compatible : should be "snps,designware-i2c" > > Besides, on Hikey960, > [ 3.822353] dw_readl(dev, DW_IC_COMP_VERSION)=0x3132302a > [ 3.827763] dw_readl(dev, DW_IC_COMP_TYPE)=0x44570140 > Are these two registers enough to distinguish version etc? > I've seen DW_IC_COMP_VERSION being 0x3131352a or 0x3132312a in our platforms. DW_IC_COMP_TYPE has the same value what you are seeing.
diff --git a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts index 64875a5..f685b1e 100644 --- a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts +++ b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts @@ -29,6 +29,24 @@ }; }; +&i2c0 { + status = "okay"; +}; + +&i2c1 { + status = "okay"; + + adv7533: adv7533@39 { + status = "ok"; + compatible = "adi,adv7533"; + reg = <0x39>; + }; +}; + +&i2c7 { + status = "okay"; +}; + &uart5 { status = "okay"; }; diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi index f55710a..f217c9d 100644 --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi @@ -186,6 +186,62 @@ #reset-cells = <2>; }; + i2c0: i2c@FFD71000 { + compatible = "snps,designware-i2c"; + reg = <0x0 0xFFD71000 0x0 0x1000>; + interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>; + #address-cells = <1>; + #size-cells = <0>; + clock-frequency = <400000>; + clocks = <&crg_ctrl HI3660_CLK_GATE_I2C0>; + resets = <&iomcu_rst 0x20 3>; + pinctrl-names = "default"; + pinctrl-0 = <&i2c0_pmx_func &i2c0_cfg_func>; + status = "disabled"; + }; + + i2c1: i2c@FFD72000 { + compatible = "snps,designware-i2c"; + reg = <0x0 0xFFD72000 0x0 0x1000>; + interrupts = <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>; + #address-cells = <1>; + #size-cells = <0>; + clock-frequency = <400000>; + clocks = <&crg_ctrl HI3660_CLK_GATE_I2C1>; + resets = <&iomcu_rst 0x20 4>; + pinctrl-names = "default"; + pinctrl-0 = <&i2c1_pmx_func &i2c1_cfg_func>; + status = "disabled"; + }; + + i2c3: i2c@FDF0C000 { + compatible = "snps,designware-i2c"; + reg = <0x0 0xFDF0C000 0x0 0x1000>; + interrupts = <GIC_SPI 81 IRQ_TYPE_LEVEL_HIGH>; + #address-cells = <1>; + #size-cells = <0>; + clock-frequency = <400000>; + clocks = <&crg_ctrl HI3660_CLK_GATE_I2C3>; + resets = <&crg_rst 0x78 7>; + pinctrl-names = "default"; + pinctrl-0 = <&i2c3_pmx_func &i2c3_cfg_func>; + status = "disabled"; + }; + + i2c7: i2c@FDF0B000 { + compatible = "snps,designware-i2c"; + reg = <0x0 0xFDF0B000 0x0 0x1000>; + interrupts = <GIC_SPI 314 IRQ_TYPE_LEVEL_HIGH>; + #address-cells = <1>; + #size-cells = <0>; + clock-frequency = <400000>; + clocks = <&crg_ctrl HI3660_CLK_GATE_I2C7>; + resets = <&crg_rst 0x60 14>; + pinctrl-names = "default"; + pinctrl-0 = <&i2c7_pmx_func &i2c7_cfg_func>; + status = "disabled"; + }; + uart5: serial@fdf05000 { compatible = "arm,pl011", "arm,primecell"; reg = <0x0 0xfdf05000 0x0 0x1000>;