* [PATCH v3 2/2] arm64: dts: mt8173: Add I2C device node
@ 2015-06-12 12:28 ` Daniel Kurtz
0 siblings, 0 replies; 30+ messages in thread
From: Daniel Kurtz @ 2015-06-12 12:28 UTC (permalink / raw)
To: linux-arm-kernel
Hi Eddie,
On Fri, Jun 12, 2015 at 5:27 PM, Eddie Huang <eddie.huang@mediatek.com> wrote:
>
> Add MT8173 I2C device nodes, include I2C controllers and pins.
> MT8173 has six I2C controllers, from i2c0 to i2c6, exclude i2c5.
> The 6th I2C controller register base doesn't next to 5th I2C,
> and there is a hardware between 5th and 6th I2C controller. So
> SoC designer name 6th controller as "i2c6", not "i2c5".
This is slightly misleading. There are in fact 7 I2C controllers, but
"i2c5" is dedicated for use by HDMI for ddc & hdcp.
Is there a reason why the HDMI I2C port cannot be controlled by the
generic i2c driver?
Of course the hdmiddc / i2c5 node can always be added in a later
patch, so this is no reason to hold up this patch.
> Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
> ---
> arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 50 ++++++++++++++++++++
> arch/arm64/boot/dts/mediatek/mt8173.dtsi | 72 +++++++++++++++++++++++++++++
> 2 files changed, 122 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> index 43d5401..2e01988 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> +++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> @@ -33,6 +33,56 @@
> chosen { };
> };
>
> +&pio {
I don't think we needed to move these i2c pinmux descriptions from
mt8173.dtsi to the board .dts file.
AFAICT, the "i2cN_pins_a" nodes defined here are the pinctl
configuration that the corresponding enabled i2c nodes would choose.
Thus, they are generic to the MT8173 SoC, not specific to any board.
By themselves, these nodes do not actually select a pin configuration.
It is the nodes that *enable* the individual i2c nodes, and hence
activate those pin settings, which is board specific.
Hence, if your intent is to have the evb enable all i2c nodes, it
would have a set of nodes like this:
&i2c0 {
pinctrl-names = "default";
pinctrl-0 = <&i2c0_pins_a>;
status = "okay";
};
&i2c1 {
pinctrl-names = "default";
pinctrl-0 = <&i2c1_pins_a>;
status = "okay";
};
...
> + i2c0_pins_a: i2c0 at 0 {
Do these nodes need the "@0"?
>
> + pins1 {
> + pinmux = <MT8173_PIN_45_SDA0__FUNC_SDA0>,
> + <MT8173_PIN_46_SCL0__FUNC_SCL0>;
> + bias-disable;
> + };
> + };
> +
> + i2c1_pins_a: i2c1 at 0 {
> + pins1 {
> + pinmux = <MT8173_PIN_125_SDA1__FUNC_SDA1>,
> + <MT8173_PIN_126_SCL1__FUNC_SCL1>;
> + bias-disable;
> + };
> + };
> +
> + i2c2_pins_a: i2c2 at 0 {
> + pins1 {
> + pinmux = <MT8173_PIN_43_SDA2__FUNC_SDA2>,
> + <MT8173_PIN_44_SCL2__FUNC_SCL2>;
> + bias-disable;
> + };
> + };
> +
> + i2c3_pins_a: i2c3 at 0 {
> + pins1 {
> + pinmux = <MT8173_PIN_106_SDA3__FUNC_SDA3>,
> + <MT8173_PIN_107_SCL3__FUNC_SCL3>;
> + bias-disable;
> + };
> + };
> +
> + i2c4_pins_a: i2c4 at 0 {
> + pins1 {
> + pinmux = <MT8173_PIN_133_SDA4__FUNC_SDA4>,
> + <MT8173_PIN_134_SCL4__FUNC_SCL4>;
> + bias-disable;
> + };
> + };
> +
> + i2c6_pins_a: i2c6 at 0 {
> + pins1 {
> + pinmux = <MT8173_PIN_100_MSDC2_DAT0__FUNC_SDA5>,
> + <MT8173_PIN_101_MSDC2_DAT1__FUNC_SCL5>;
These are the SDA/SCL pins for i2c port 6, so they should really be
_SDA6 / _SCL6.
However... I checked, and these settings are labeled "SDA5 & SCL5" in
the datasheet.
I recommend marking them correctly as 6 here and fixing the datasheet :-).
>
> + bias-disable;
> + };
> + };
> +};
> +
> &uart0 {
> status = "okay";
> };
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> index b52ec43..6d3dbbdd 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> @@ -229,6 +229,78 @@
> clocks = <&uart_clk>;
> status = "disabled";
> };
> +
> + i2c0: i2c at 11007000 {
> + compatible = "mediatek,mt8173-i2c";
> + reg = <0 0x11007000 0 0x70>,
> + <0 0x11000100 0 0x80>;
> + interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_LOW>;
> + clock-div = <16>;
> + clocks = <&pericfg CLK_PERI_I2C0>,
> + <&pericfg CLK_PERI_AP_DMA>;
> + clock-names = "main", "dma";
The following fields must also be selected by the i2c nodes when they
are enabled:
clock-frequency = <100000>;
pinctrl-names = "default";
pinctrl-0 = <&i2c0_pins_a>;
#address-cells = <1>;
#size-cells = <0>;
So, is there any reason not to also include them here in mt8173.dtsi
so we can use them as defaults?
This would simplify the i2c nodes in the board specific .dts files.
The only field that might be board specific would be clock-frequency,
but that is trivial to override on a port-by-port basis in board files
as necessary.
Thanks,
-Dan
>
> + status = "disabled";
> + };
> +
> + i2c1: i2c at 11008000 {
> + compatible = "mediatek,mt8173-i2c";
> + reg = <0 0x11008000 0 0x70>,
> + <0 0x11000180 0 0x80>;
> + interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_LOW>;
> + clock-div = <16>;
> + clocks = <&pericfg CLK_PERI_I2C1>,
> + <&pericfg CLK_PERI_AP_DMA>;
> + clock-names = "main", "dma";
> + status = "disabled";
> + };
> +
> + i2c2: i2c at 11009000 {
> + compatible = "mediatek,mt8173-i2c";
> + reg = <0 0x11009000 0 0x70>,
> + <0 0x11000200 0 0x80>;
> + interrupts = <GIC_SPI 78 IRQ_TYPE_LEVEL_LOW>;
> + clock-div = <16>;
> + clocks = <&pericfg CLK_PERI_I2C2>,
> + <&pericfg CLK_PERI_AP_DMA>;
> + clock-names = "main", "dma";
> + status = "disabled";
> + };
> +
> + i2c3: i2c3 at 11010000 {
> + compatible = "mediatek,mt8173-i2c";
> + reg = <0 0x11010000 0 0x70>,
> + <0 0x11000280 0 0x80>;
> + interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_LOW>;
> + clock-div = <16>;
> + clocks = <&pericfg CLK_PERI_I2C3>,
> + <&pericfg CLK_PERI_AP_DMA>;
> + clock-names = "main", "dma";
> + status = "disabled";
> + };
> +
> + i2c4: i2c4 at 11011000 {
> + compatible = "mediatek,mt8173-i2c";
> + reg = <0 0x11011000 0 0x70>,
> + <0 0x11000300 0 0x80>;
> + interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_LOW>;
> + clock-div = <16>;
> + clocks = <&pericfg CLK_PERI_I2C4>,
> + <&pericfg CLK_PERI_AP_DMA>;
> + clock-names = "main", "dma";
> + status = "disabled";
> + };
> +
> + i2c6: i2c6 at 11013000 {
> + compatible = "mediatek,mt8173-i2c";
> + reg = <0 0x11013000 0 0x70>,
> + <0 0x11000080 0 0x80>;
> + interrupts = <GIC_SPI 82 IRQ_TYPE_LEVEL_LOW>;
> + clock-div = <16>;
> + clocks = <&pericfg CLK_PERI_I2C6>,
> + <&pericfg CLK_PERI_AP_DMA>;
> + clock-names = "main", "dma";
> + status = "disabled";
> + };
> };
>
> };
> --
> 1.8.1.1.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v3 2/2] arm64: dts: mt8173: Add I2C device node
@ 2015-06-12 12:28 ` Daniel Kurtz
0 siblings, 0 replies; 30+ messages in thread
From: Daniel Kurtz @ 2015-06-12 12:28 UTC (permalink / raw)
To: Eddie Huang
Cc: Matthias Brugger, Sascha Hauer, open list:OPEN FIRMWARE AND...,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Linus Walleij
Hi Eddie,
On Fri, Jun 12, 2015 at 5:27 PM, Eddie Huang <eddie.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
>
> Add MT8173 I2C device nodes, include I2C controllers and pins.
> MT8173 has six I2C controllers, from i2c0 to i2c6, exclude i2c5.
> The 6th I2C controller register base doesn't next to 5th I2C,
> and there is a hardware between 5th and 6th I2C controller. So
> SoC designer name 6th controller as "i2c6", not "i2c5".
This is slightly misleading. There are in fact 7 I2C controllers, but
"i2c5" is dedicated for use by HDMI for ddc & hdcp.
Is there a reason why the HDMI I2C port cannot be controlled by the
generic i2c driver?
Of course the hdmiddc / i2c5 node can always be added in a later
patch, so this is no reason to hold up this patch.
> Signed-off-by: Eddie Huang <eddie.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
> arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 50 ++++++++++++++++++++
> arch/arm64/boot/dts/mediatek/mt8173.dtsi | 72 +++++++++++++++++++++++++++++
> 2 files changed, 122 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> index 43d5401..2e01988 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> +++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> @@ -33,6 +33,56 @@
> chosen { };
> };
>
> +&pio {
I don't think we needed to move these i2c pinmux descriptions from
mt8173.dtsi to the board .dts file.
AFAICT, the "i2cN_pins_a" nodes defined here are the pinctl
configuration that the corresponding enabled i2c nodes would choose.
Thus, they are generic to the MT8173 SoC, not specific to any board.
By themselves, these nodes do not actually select a pin configuration.
It is the nodes that *enable* the individual i2c nodes, and hence
activate those pin settings, which is board specific.
Hence, if your intent is to have the evb enable all i2c nodes, it
would have a set of nodes like this:
&i2c0 {
pinctrl-names = "default";
pinctrl-0 = <&i2c0_pins_a>;
status = "okay";
};
&i2c1 {
pinctrl-names = "default";
pinctrl-0 = <&i2c1_pins_a>;
status = "okay";
};
...
> + i2c0_pins_a: i2c0@0 {
Do these nodes need the "@0"?
>
> + pins1 {
> + pinmux = <MT8173_PIN_45_SDA0__FUNC_SDA0>,
> + <MT8173_PIN_46_SCL0__FUNC_SCL0>;
> + bias-disable;
> + };
> + };
> +
> + i2c1_pins_a: i2c1@0 {
> + pins1 {
> + pinmux = <MT8173_PIN_125_SDA1__FUNC_SDA1>,
> + <MT8173_PIN_126_SCL1__FUNC_SCL1>;
> + bias-disable;
> + };
> + };
> +
> + i2c2_pins_a: i2c2@0 {
> + pins1 {
> + pinmux = <MT8173_PIN_43_SDA2__FUNC_SDA2>,
> + <MT8173_PIN_44_SCL2__FUNC_SCL2>;
> + bias-disable;
> + };
> + };
> +
> + i2c3_pins_a: i2c3@0 {
> + pins1 {
> + pinmux = <MT8173_PIN_106_SDA3__FUNC_SDA3>,
> + <MT8173_PIN_107_SCL3__FUNC_SCL3>;
> + bias-disable;
> + };
> + };
> +
> + i2c4_pins_a: i2c4@0 {
> + pins1 {
> + pinmux = <MT8173_PIN_133_SDA4__FUNC_SDA4>,
> + <MT8173_PIN_134_SCL4__FUNC_SCL4>;
> + bias-disable;
> + };
> + };
> +
> + i2c6_pins_a: i2c6@0 {
> + pins1 {
> + pinmux = <MT8173_PIN_100_MSDC2_DAT0__FUNC_SDA5>,
> + <MT8173_PIN_101_MSDC2_DAT1__FUNC_SCL5>;
These are the SDA/SCL pins for i2c port 6, so they should really be
_SDA6 / _SCL6.
However... I checked, and these settings are labeled "SDA5 & SCL5" in
the datasheet.
I recommend marking them correctly as 6 here and fixing the datasheet :-).
>
> + bias-disable;
> + };
> + };
> +};
> +
> &uart0 {
> status = "okay";
> };
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> index b52ec43..6d3dbbdd 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> @@ -229,6 +229,78 @@
> clocks = <&uart_clk>;
> status = "disabled";
> };
> +
> + i2c0: i2c@11007000 {
> + compatible = "mediatek,mt8173-i2c";
> + reg = <0 0x11007000 0 0x70>,
> + <0 0x11000100 0 0x80>;
> + interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_LOW>;
> + clock-div = <16>;
> + clocks = <&pericfg CLK_PERI_I2C0>,
> + <&pericfg CLK_PERI_AP_DMA>;
> + clock-names = "main", "dma";
The following fields must also be selected by the i2c nodes when they
are enabled:
clock-frequency = <100000>;
pinctrl-names = "default";
pinctrl-0 = <&i2c0_pins_a>;
#address-cells = <1>;
#size-cells = <0>;
So, is there any reason not to also include them here in mt8173.dtsi
so we can use them as defaults?
This would simplify the i2c nodes in the board specific .dts files.
The only field that might be board specific would be clock-frequency,
but that is trivial to override on a port-by-port basis in board files
as necessary.
Thanks,
-Dan
>
> + status = "disabled";
> + };
> +
> + i2c1: i2c@11008000 {
> + compatible = "mediatek,mt8173-i2c";
> + reg = <0 0x11008000 0 0x70>,
> + <0 0x11000180 0 0x80>;
> + interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_LOW>;
> + clock-div = <16>;
> + clocks = <&pericfg CLK_PERI_I2C1>,
> + <&pericfg CLK_PERI_AP_DMA>;
> + clock-names = "main", "dma";
> + status = "disabled";
> + };
> +
> + i2c2: i2c@11009000 {
> + compatible = "mediatek,mt8173-i2c";
> + reg = <0 0x11009000 0 0x70>,
> + <0 0x11000200 0 0x80>;
> + interrupts = <GIC_SPI 78 IRQ_TYPE_LEVEL_LOW>;
> + clock-div = <16>;
> + clocks = <&pericfg CLK_PERI_I2C2>,
> + <&pericfg CLK_PERI_AP_DMA>;
> + clock-names = "main", "dma";
> + status = "disabled";
> + };
> +
> + i2c3: i2c3@11010000 {
> + compatible = "mediatek,mt8173-i2c";
> + reg = <0 0x11010000 0 0x70>,
> + <0 0x11000280 0 0x80>;
> + interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_LOW>;
> + clock-div = <16>;
> + clocks = <&pericfg CLK_PERI_I2C3>,
> + <&pericfg CLK_PERI_AP_DMA>;
> + clock-names = "main", "dma";
> + status = "disabled";
> + };
> +
> + i2c4: i2c4@11011000 {
> + compatible = "mediatek,mt8173-i2c";
> + reg = <0 0x11011000 0 0x70>,
> + <0 0x11000300 0 0x80>;
> + interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_LOW>;
> + clock-div = <16>;
> + clocks = <&pericfg CLK_PERI_I2C4>,
> + <&pericfg CLK_PERI_AP_DMA>;
> + clock-names = "main", "dma";
> + status = "disabled";
> + };
> +
> + i2c6: i2c6@11013000 {
> + compatible = "mediatek,mt8173-i2c";
> + reg = <0 0x11013000 0 0x70>,
> + <0 0x11000080 0 0x80>;
> + interrupts = <GIC_SPI 82 IRQ_TYPE_LEVEL_LOW>;
> + clock-div = <16>;
> + clocks = <&pericfg CLK_PERI_I2C6>,
> + <&pericfg CLK_PERI_AP_DMA>;
> + clock-names = "main", "dma";
> + status = "disabled";
> + };
> };
>
> };
> --
> 1.8.1.1.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v3 2/2] arm64: dts: mt8173: Add I2C device node
@ 2015-06-12 17:13 ` Matthias Brugger
0 siblings, 0 replies; 30+ messages in thread
From: Matthias Brugger @ 2015-06-12 17:13 UTC (permalink / raw)
To: Daniel Kurtz
Cc: Eddie Huang, Sascha Hauer, open list:OPEN FIRMWARE AND...,
linux-arm-kernel, linux-kernel, linux-mediatek, Linus Walleij
On Friday, June 12, 2015 08:28:51 PM Daniel Kurtz wrote:
> Hi Eddie,
>
> On Fri, Jun 12, 2015 at 5:27 PM, Eddie Huang <eddie.huang@mediatek.com>
wrote:
> > Add MT8173 I2C device nodes, include I2C controllers and pins.
> > MT8173 has six I2C controllers, from i2c0 to i2c6, exclude i2c5.
> > The 6th I2C controller register base doesn't next to 5th I2C,
> > and there is a hardware between 5th and 6th I2C controller. So
> > SoC designer name 6th controller as "i2c6", not "i2c5".
>
> This is slightly misleading. There are in fact 7 I2C controllers, but
> "i2c5" is dedicated for use by HDMI for ddc & hdcp.
> Is there a reason why the HDMI I2C port cannot be controlled by the
> generic i2c driver?
>
> Of course the hdmiddc / i2c5 node can always be added in a later
> patch, so this is no reason to hold up this patch.
>
> > Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
> > ---
> >
> > arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 50 ++++++++++++++++++++
> > arch/arm64/boot/dts/mediatek/mt8173.dtsi | 72
> > +++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts index 43d5401..2e01988
> > 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > @@ -33,6 +33,56 @@
> >
> > chosen { };
> >
> > };
> >
> > +&pio {
>
> I don't think we needed to move these i2c pinmux descriptions from
> mt8173.dtsi to the board .dts file.
>
> AFAICT, the "i2cN_pins_a" nodes defined here are the pinctl
> configuration that the corresponding enabled i2c nodes would choose.
> Thus, they are generic to the MT8173 SoC, not specific to any board.
> By themselves, these nodes do not actually select a pin configuration.
>
> It is the nodes that *enable* the individual i2c nodes, and hence
> activate those pin settings, which is board specific.
>
> Hence, if your intent is to have the evb enable all i2c nodes, it
> would have a set of nodes like this:
>
>
> &i2c0 {
> pinctrl-names = "default";
> pinctrl-0 = <&i2c0_pins_a>;
> status = "okay";
> };
>
> &i2c1 {
> pinctrl-names = "default";
> pinctrl-0 = <&i2c1_pins_a>;
> status = "okay";
> };
>
> ...
>
> > + i2c0_pins_a: i2c0@0 {
>
> Do these nodes need the "@0"?
>
> > + pins1 {
> > + pinmux = <MT8173_PIN_45_SDA0__FUNC_SDA0>,
> > + <MT8173_PIN_46_SCL0__FUNC_SCL0>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c1_pins_a: i2c1@0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_125_SDA1__FUNC_SDA1>,
> > + <MT8173_PIN_126_SCL1__FUNC_SCL1>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c2_pins_a: i2c2@0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_43_SDA2__FUNC_SDA2>,
> > + <MT8173_PIN_44_SCL2__FUNC_SCL2>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c3_pins_a: i2c3@0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_106_SDA3__FUNC_SDA3>,
> > + <MT8173_PIN_107_SCL3__FUNC_SCL3>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c4_pins_a: i2c4@0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_133_SDA4__FUNC_SDA4>,
> > + <MT8173_PIN_134_SCL4__FUNC_SCL4>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c6_pins_a: i2c6@0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_100_MSDC2_DAT0__FUNC_SDA5>,
> > + <MT8173_PIN_101_MSDC2_DAT1__FUNC_SCL5>;
>
> These are the SDA/SCL pins for i2c port 6, so they should really be
> _SDA6 / _SCL6.
> However... I checked, and these settings are labeled "SDA5 & SCL5" in
> the datasheet.
> I recommend marking them correctly as 6 here and fixing the datasheet :-).
>
> > + bias-disable;
> > + };
> > + };
> > +};
> > +
> >
> > &uart0 {
> >
> > status = "okay";
> >
> > };
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > b/arch/arm64/boot/dts/mediatek/mt8173.dtsi index b52ec43..6d3dbbdd 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > @@ -229,6 +229,78 @@
> >
> > clocks = <&uart_clk>;
> > status = "disabled";
> >
> > };
> >
> > +
> > + i2c0: i2c@11007000 {
> > + compatible = "mediatek,mt8173-i2c";
> > + reg = <0 0x11007000 0 0x70>,
> > + <0 0x11000100 0 0x80>;
> > + interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_LOW>;
> > + clock-div = <16>;
> > + clocks = <&pericfg CLK_PERI_I2C0>,
> > + <&pericfg CLK_PERI_AP_DMA>;
> > + clock-names = "main", "dma";
>
> The following fields must also be selected by the i2c nodes when they
> are enabled:
>
> clock-frequency = <100000>;
> pinctrl-names = "default";
> pinctrl-0 = <&i2c0_pins_a>;
> #address-cells = <1>;
> #size-cells = <0>;
>
> So, is there any reason not to also include them here in mt8173.dtsi
> so we can use them as defaults?
> This would simplify the i2c nodes in the board specific .dts files.
> The only field that might be board specific would be clock-frequency,
> but that is trivial to override on a port-by-port basis in board files
> as necessary.
The problem is, that most of the pins have several modes (up to eight).
In the future when all the device drivers are implemented, this gets you a
really huge dtsi. AFAIK this can cause a somewhat long parsing time of the dtb
when booting the system.
Sascha, please correct me, if I'm wrong.
Thanks,
Matthias
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH v3 2/2] arm64: dts: mt8173: Add I2C device node
@ 2015-06-12 17:13 ` Matthias Brugger
0 siblings, 0 replies; 30+ messages in thread
From: Matthias Brugger @ 2015-06-12 17:13 UTC (permalink / raw)
To: linux-arm-kernel
On Friday, June 12, 2015 08:28:51 PM Daniel Kurtz wrote:
> Hi Eddie,
>
> On Fri, Jun 12, 2015 at 5:27 PM, Eddie Huang <eddie.huang@mediatek.com>
wrote:
> > Add MT8173 I2C device nodes, include I2C controllers and pins.
> > MT8173 has six I2C controllers, from i2c0 to i2c6, exclude i2c5.
> > The 6th I2C controller register base doesn't next to 5th I2C,
> > and there is a hardware between 5th and 6th I2C controller. So
> > SoC designer name 6th controller as "i2c6", not "i2c5".
>
> This is slightly misleading. There are in fact 7 I2C controllers, but
> "i2c5" is dedicated for use by HDMI for ddc & hdcp.
> Is there a reason why the HDMI I2C port cannot be controlled by the
> generic i2c driver?
>
> Of course the hdmiddc / i2c5 node can always be added in a later
> patch, so this is no reason to hold up this patch.
>
> > Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
> > ---
> >
> > arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 50 ++++++++++++++++++++
> > arch/arm64/boot/dts/mediatek/mt8173.dtsi | 72
> > +++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts index 43d5401..2e01988
> > 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > @@ -33,6 +33,56 @@
> >
> > chosen { };
> >
> > };
> >
> > +&pio {
>
> I don't think we needed to move these i2c pinmux descriptions from
> mt8173.dtsi to the board .dts file.
>
> AFAICT, the "i2cN_pins_a" nodes defined here are the pinctl
> configuration that the corresponding enabled i2c nodes would choose.
> Thus, they are generic to the MT8173 SoC, not specific to any board.
> By themselves, these nodes do not actually select a pin configuration.
>
> It is the nodes that *enable* the individual i2c nodes, and hence
> activate those pin settings, which is board specific.
>
> Hence, if your intent is to have the evb enable all i2c nodes, it
> would have a set of nodes like this:
>
>
> &i2c0 {
> pinctrl-names = "default";
> pinctrl-0 = <&i2c0_pins_a>;
> status = "okay";
> };
>
> &i2c1 {
> pinctrl-names = "default";
> pinctrl-0 = <&i2c1_pins_a>;
> status = "okay";
> };
>
> ...
>
> > + i2c0_pins_a: i2c0 at 0 {
>
> Do these nodes need the "@0"?
>
> > + pins1 {
> > + pinmux = <MT8173_PIN_45_SDA0__FUNC_SDA0>,
> > + <MT8173_PIN_46_SCL0__FUNC_SCL0>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c1_pins_a: i2c1 at 0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_125_SDA1__FUNC_SDA1>,
> > + <MT8173_PIN_126_SCL1__FUNC_SCL1>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c2_pins_a: i2c2 at 0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_43_SDA2__FUNC_SDA2>,
> > + <MT8173_PIN_44_SCL2__FUNC_SCL2>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c3_pins_a: i2c3 at 0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_106_SDA3__FUNC_SDA3>,
> > + <MT8173_PIN_107_SCL3__FUNC_SCL3>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c4_pins_a: i2c4 at 0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_133_SDA4__FUNC_SDA4>,
> > + <MT8173_PIN_134_SCL4__FUNC_SCL4>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c6_pins_a: i2c6 at 0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_100_MSDC2_DAT0__FUNC_SDA5>,
> > + <MT8173_PIN_101_MSDC2_DAT1__FUNC_SCL5>;
>
> These are the SDA/SCL pins for i2c port 6, so they should really be
> _SDA6 / _SCL6.
> However... I checked, and these settings are labeled "SDA5 & SCL5" in
> the datasheet.
> I recommend marking them correctly as 6 here and fixing the datasheet :-).
>
> > + bias-disable;
> > + };
> > + };
> > +};
> > +
> >
> > &uart0 {
> >
> > status = "okay";
> >
> > };
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > b/arch/arm64/boot/dts/mediatek/mt8173.dtsi index b52ec43..6d3dbbdd 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > @@ -229,6 +229,78 @@
> >
> > clocks = <&uart_clk>;
> > status = "disabled";
> >
> > };
> >
> > +
> > + i2c0: i2c at 11007000 {
> > + compatible = "mediatek,mt8173-i2c";
> > + reg = <0 0x11007000 0 0x70>,
> > + <0 0x11000100 0 0x80>;
> > + interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_LOW>;
> > + clock-div = <16>;
> > + clocks = <&pericfg CLK_PERI_I2C0>,
> > + <&pericfg CLK_PERI_AP_DMA>;
> > + clock-names = "main", "dma";
>
> The following fields must also be selected by the i2c nodes when they
> are enabled:
>
> clock-frequency = <100000>;
> pinctrl-names = "default";
> pinctrl-0 = <&i2c0_pins_a>;
> #address-cells = <1>;
> #size-cells = <0>;
>
> So, is there any reason not to also include them here in mt8173.dtsi
> so we can use them as defaults?
> This would simplify the i2c nodes in the board specific .dts files.
> The only field that might be board specific would be clock-frequency,
> but that is trivial to override on a port-by-port basis in board files
> as necessary.
The problem is, that most of the pins have several modes (up to eight).
In the future when all the device drivers are implemented, this gets you a
really huge dtsi. AFAIK this can cause a somewhat long parsing time of the dtb
when booting the system.
Sascha, please correct me, if I'm wrong.
Thanks,
Matthias
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v3 2/2] arm64: dts: mt8173: Add I2C device node
@ 2015-06-12 17:13 ` Matthias Brugger
0 siblings, 0 replies; 30+ messages in thread
From: Matthias Brugger @ 2015-06-12 17:13 UTC (permalink / raw)
To: Daniel Kurtz
Cc: Eddie Huang, Sascha Hauer, open list:OPEN FIRMWARE AND...,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Linus Walleij
On Friday, June 12, 2015 08:28:51 PM Daniel Kurtz wrote:
> Hi Eddie,
>
> On Fri, Jun 12, 2015 at 5:27 PM, Eddie Huang <eddie.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
wrote:
> > Add MT8173 I2C device nodes, include I2C controllers and pins.
> > MT8173 has six I2C controllers, from i2c0 to i2c6, exclude i2c5.
> > The 6th I2C controller register base doesn't next to 5th I2C,
> > and there is a hardware between 5th and 6th I2C controller. So
> > SoC designer name 6th controller as "i2c6", not "i2c5".
>
> This is slightly misleading. There are in fact 7 I2C controllers, but
> "i2c5" is dedicated for use by HDMI for ddc & hdcp.
> Is there a reason why the HDMI I2C port cannot be controlled by the
> generic i2c driver?
>
> Of course the hdmiddc / i2c5 node can always be added in a later
> patch, so this is no reason to hold up this patch.
>
> > Signed-off-by: Eddie Huang <eddie.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > ---
> >
> > arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 50 ++++++++++++++++++++
> > arch/arm64/boot/dts/mediatek/mt8173.dtsi | 72
> > +++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts index 43d5401..2e01988
> > 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > @@ -33,6 +33,56 @@
> >
> > chosen { };
> >
> > };
> >
> > +&pio {
>
> I don't think we needed to move these i2c pinmux descriptions from
> mt8173.dtsi to the board .dts file.
>
> AFAICT, the "i2cN_pins_a" nodes defined here are the pinctl
> configuration that the corresponding enabled i2c nodes would choose.
> Thus, they are generic to the MT8173 SoC, not specific to any board.
> By themselves, these nodes do not actually select a pin configuration.
>
> It is the nodes that *enable* the individual i2c nodes, and hence
> activate those pin settings, which is board specific.
>
> Hence, if your intent is to have the evb enable all i2c nodes, it
> would have a set of nodes like this:
>
>
> &i2c0 {
> pinctrl-names = "default";
> pinctrl-0 = <&i2c0_pins_a>;
> status = "okay";
> };
>
> &i2c1 {
> pinctrl-names = "default";
> pinctrl-0 = <&i2c1_pins_a>;
> status = "okay";
> };
>
> ...
>
> > + i2c0_pins_a: i2c0@0 {
>
> Do these nodes need the "@0"?
>
> > + pins1 {
> > + pinmux = <MT8173_PIN_45_SDA0__FUNC_SDA0>,
> > + <MT8173_PIN_46_SCL0__FUNC_SCL0>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c1_pins_a: i2c1@0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_125_SDA1__FUNC_SDA1>,
> > + <MT8173_PIN_126_SCL1__FUNC_SCL1>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c2_pins_a: i2c2@0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_43_SDA2__FUNC_SDA2>,
> > + <MT8173_PIN_44_SCL2__FUNC_SCL2>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c3_pins_a: i2c3@0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_106_SDA3__FUNC_SDA3>,
> > + <MT8173_PIN_107_SCL3__FUNC_SCL3>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c4_pins_a: i2c4@0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_133_SDA4__FUNC_SDA4>,
> > + <MT8173_PIN_134_SCL4__FUNC_SCL4>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c6_pins_a: i2c6@0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_100_MSDC2_DAT0__FUNC_SDA5>,
> > + <MT8173_PIN_101_MSDC2_DAT1__FUNC_SCL5>;
>
> These are the SDA/SCL pins for i2c port 6, so they should really be
> _SDA6 / _SCL6.
> However... I checked, and these settings are labeled "SDA5 & SCL5" in
> the datasheet.
> I recommend marking them correctly as 6 here and fixing the datasheet :-).
>
> > + bias-disable;
> > + };
> > + };
> > +};
> > +
> >
> > &uart0 {
> >
> > status = "okay";
> >
> > };
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > b/arch/arm64/boot/dts/mediatek/mt8173.dtsi index b52ec43..6d3dbbdd 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > @@ -229,6 +229,78 @@
> >
> > clocks = <&uart_clk>;
> > status = "disabled";
> >
> > };
> >
> > +
> > + i2c0: i2c@11007000 {
> > + compatible = "mediatek,mt8173-i2c";
> > + reg = <0 0x11007000 0 0x70>,
> > + <0 0x11000100 0 0x80>;
> > + interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_LOW>;
> > + clock-div = <16>;
> > + clocks = <&pericfg CLK_PERI_I2C0>,
> > + <&pericfg CLK_PERI_AP_DMA>;
> > + clock-names = "main", "dma";
>
> The following fields must also be selected by the i2c nodes when they
> are enabled:
>
> clock-frequency = <100000>;
> pinctrl-names = "default";
> pinctrl-0 = <&i2c0_pins_a>;
> #address-cells = <1>;
> #size-cells = <0>;
>
> So, is there any reason not to also include them here in mt8173.dtsi
> so we can use them as defaults?
> This would simplify the i2c nodes in the board specific .dts files.
> The only field that might be board specific would be clock-frequency,
> but that is trivial to override on a port-by-port basis in board files
> as necessary.
The problem is, that most of the pins have several modes (up to eight).
In the future when all the device drivers are implemented, this gets you a
really huge dtsi. AFAIK this can cause a somewhat long parsing time of the dtb
when booting the system.
Sascha, please correct me, if I'm wrong.
Thanks,
Matthias
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v3 2/2] arm64: dts: mt8173: Add I2C device node
2015-06-12 17:13 ` Matthias Brugger
(?)
@ 2015-06-30 6:26 ` Linus Walleij
-1 siblings, 0 replies; 30+ messages in thread
From: Linus Walleij @ 2015-06-30 6:26 UTC (permalink / raw)
To: Matthias Brugger
Cc: Daniel Kurtz, Eddie Huang, Sascha Hauer,
open list:OPEN FIRMWARE AND...,
linux-arm-kernel, linux-kernel, linux-mediatek
On Fri, Jun 12, 2015 at 7:13 PM, Matthias Brugger
<matthias.bgg@gmail.com> wrote:
> In the future when all the device drivers are implemented, this gets you a
> really huge dtsi. AFAIK this can cause a somewhat long parsing time of the dtb
> when booting the system.
Boot time claims need to be backed up with ftrace logs or something. One of
the arguments against device tree in the beginning was parse time and then
we made measurements and it proved moot.
However being unable to manage the dts[i] sources is a valid argument for
simplifying and splitting up stuff.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 2/2] arm64: dts: mt8173: Add I2C device node
@ 2015-06-30 6:26 ` Linus Walleij
0 siblings, 0 replies; 30+ messages in thread
From: Linus Walleij @ 2015-06-30 6:26 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jun 12, 2015 at 7:13 PM, Matthias Brugger
<matthias.bgg@gmail.com> wrote:
> In the future when all the device drivers are implemented, this gets you a
> really huge dtsi. AFAIK this can cause a somewhat long parsing time of the dtb
> when booting the system.
Boot time claims need to be backed up with ftrace logs or something. One of
the arguments against device tree in the beginning was parse time and then
we made measurements and it proved moot.
However being unable to manage the dts[i] sources is a valid argument for
simplifying and splitting up stuff.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 2/2] arm64: dts: mt8173: Add I2C device node
@ 2015-06-30 6:26 ` Linus Walleij
0 siblings, 0 replies; 30+ messages in thread
From: Linus Walleij @ 2015-06-30 6:26 UTC (permalink / raw)
To: Matthias Brugger
Cc: Daniel Kurtz, Eddie Huang, Sascha Hauer,
open list:OPEN FIRMWARE AND...,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Fri, Jun 12, 2015 at 7:13 PM, Matthias Brugger
<matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> In the future when all the device drivers are implemented, this gets you a
> really huge dtsi. AFAIK this can cause a somewhat long parsing time of the dtb
> when booting the system.
Boot time claims need to be backed up with ftrace logs or something. One of
the arguments against device tree in the beginning was parse time and then
we made measurements and it proved moot.
However being unable to manage the dts[i] sources is a valid argument for
simplifying and splitting up stuff.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 2/2] arm64: dts: mt8173: Add I2C device node
2015-06-12 12:28 ` Daniel Kurtz
(?)
@ 2015-06-15 6:12 ` Sascha Hauer
-1 siblings, 0 replies; 30+ messages in thread
From: Sascha Hauer @ 2015-06-15 6:12 UTC (permalink / raw)
To: Daniel Kurtz
Cc: Eddie Huang, Matthias Brugger, open list:OPEN FIRMWARE AND...,
linux-arm-kernel, linux-kernel, linux-mediatek, Linus Walleij
On Fri, Jun 12, 2015 at 08:28:51PM +0800, Daniel Kurtz wrote:
> Hi Eddie,
>
> On Fri, Jun 12, 2015 at 5:27 PM, Eddie Huang <eddie.huang@mediatek.com> wrote:
> >
> > Add MT8173 I2C device nodes, include I2C controllers and pins.
> > MT8173 has six I2C controllers, from i2c0 to i2c6, exclude i2c5.
> > The 6th I2C controller register base doesn't next to 5th I2C,
> > and there is a hardware between 5th and 6th I2C controller. So
> > SoC designer name 6th controller as "i2c6", not "i2c5".
>
>
> This is slightly misleading. There are in fact 7 I2C controllers, but
> "i2c5" is dedicated for use by HDMI for ddc & hdcp.
> Is there a reason why the HDMI I2C port cannot be controlled by the
> generic i2c driver?
>
> Of course the hdmiddc / i2c5 node can always be added in a later
> patch, so this is no reason to hold up this patch.
>
> > Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
> > ---
> > arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 50 ++++++++++++++++++++
> > arch/arm64/boot/dts/mediatek/mt8173.dtsi | 72 +++++++++++++++++++++++++++++
> > 2 files changed, 122 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > index 43d5401..2e01988 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > @@ -33,6 +33,56 @@
> > chosen { };
> > };
> >
> > +&pio {
>
> I don't think we needed to move these i2c pinmux descriptions from
> mt8173.dtsi to the board .dts file.
>
> AFAICT, the "i2cN_pins_a" nodes defined here are the pinctl
> configuration that the corresponding enabled i2c nodes would choose.
> Thus, they are generic to the MT8173 SoC, not specific to any board.
> By themselves, these nodes do not actually select a pin configuration.
On i.MX we started with i2cN_pins_[abcde] groups in the SoC dtsi file
aswell. At some point we realized that this does not scale anymore.
The problems were:
- Whenever a new board came along which needed some pin setting which
didn't already exist a new group was created in the SoC dtsi file using
the next free letter from the alphabet. The ordering of the group names
was rather arbitrary and often enough there were merge conflicts to
resolve when during one merge window two new boards showed up which both
added i2cN_pin_x with different content.
- When a new board needs the same pins but with different drive strength
settings a new group was required
- With SD/MMC we had groups for 4bit data, groups adding the remaining
pins for 8bit data
- With UARTs we had groups for RX/TX and additionally groups adding
RTS/CTS
- For graphics we had groups for 16bit data and 24bit data
- There is no way to remove the unused nodes from the binary dtb, so
every board contained all existing pingroups for every other board,
so the dtbs became quite big
So with only a few existing boards with very similar pinmux groups
it may work fine to put the groups into the SoC dtsi, but this way
may also explode quite fast with more and different boards.
I don't know how much variation there will be with Mediatek boards, so
I'm fine with either way. You may want to look at:
5a2a7d5 ARM: dts: imx51: make pinctrl nodes board specific
7ac0f70 ARM: dts: imx53: make pinctrl nodes board specific
817c27a ARM: dts: imx6qdl: make pinctrl nodes board specific
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH v3 2/2] arm64: dts: mt8173: Add I2C device node
@ 2015-06-15 6:12 ` Sascha Hauer
0 siblings, 0 replies; 30+ messages in thread
From: Sascha Hauer @ 2015-06-15 6:12 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jun 12, 2015 at 08:28:51PM +0800, Daniel Kurtz wrote:
> Hi Eddie,
>
> On Fri, Jun 12, 2015 at 5:27 PM, Eddie Huang <eddie.huang@mediatek.com> wrote:
> >
> > Add MT8173 I2C device nodes, include I2C controllers and pins.
> > MT8173 has six I2C controllers, from i2c0 to i2c6, exclude i2c5.
> > The 6th I2C controller register base doesn't next to 5th I2C,
> > and there is a hardware between 5th and 6th I2C controller. So
> > SoC designer name 6th controller as "i2c6", not "i2c5".
>
>
> This is slightly misleading. There are in fact 7 I2C controllers, but
> "i2c5" is dedicated for use by HDMI for ddc & hdcp.
> Is there a reason why the HDMI I2C port cannot be controlled by the
> generic i2c driver?
>
> Of course the hdmiddc / i2c5 node can always be added in a later
> patch, so this is no reason to hold up this patch.
>
> > Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
> > ---
> > arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 50 ++++++++++++++++++++
> > arch/arm64/boot/dts/mediatek/mt8173.dtsi | 72 +++++++++++++++++++++++++++++
> > 2 files changed, 122 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > index 43d5401..2e01988 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > @@ -33,6 +33,56 @@
> > chosen { };
> > };
> >
> > +&pio {
>
> I don't think we needed to move these i2c pinmux descriptions from
> mt8173.dtsi to the board .dts file.
>
> AFAICT, the "i2cN_pins_a" nodes defined here are the pinctl
> configuration that the corresponding enabled i2c nodes would choose.
> Thus, they are generic to the MT8173 SoC, not specific to any board.
> By themselves, these nodes do not actually select a pin configuration.
On i.MX we started with i2cN_pins_[abcde] groups in the SoC dtsi file
aswell. At some point we realized that this does not scale anymore.
The problems were:
- Whenever a new board came along which needed some pin setting which
didn't already exist a new group was created in the SoC dtsi file using
the next free letter from the alphabet. The ordering of the group names
was rather arbitrary and often enough there were merge conflicts to
resolve when during one merge window two new boards showed up which both
added i2cN_pin_x with different content.
- When a new board needs the same pins but with different drive strength
settings a new group was required
- With SD/MMC we had groups for 4bit data, groups adding the remaining
pins for 8bit data
- With UARTs we had groups for RX/TX and additionally groups adding
RTS/CTS
- For graphics we had groups for 16bit data and 24bit data
- There is no way to remove the unused nodes from the binary dtb, so
every board contained all existing pingroups for every other board,
so the dtbs became quite big
So with only a few existing boards with very similar pinmux groups
it may work fine to put the groups into the SoC dtsi, but this way
may also explode quite fast with more and different boards.
I don't know how much variation there will be with Mediatek boards, so
I'm fine with either way. You may want to look at:
5a2a7d5 ARM: dts: imx51: make pinctrl nodes board specific
7ac0f70 ARM: dts: imx53: make pinctrl nodes board specific
817c27a ARM: dts: imx6qdl: make pinctrl nodes board specific
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v3 2/2] arm64: dts: mt8173: Add I2C device node
@ 2015-06-15 6:12 ` Sascha Hauer
0 siblings, 0 replies; 30+ messages in thread
From: Sascha Hauer @ 2015-06-15 6:12 UTC (permalink / raw)
To: Daniel Kurtz
Cc: Eddie Huang, Matthias Brugger, open list:OPEN FIRMWARE AND...,
linux-arm-kernel, linux-kernel, linux-mediatek, Linus Walleij
On Fri, Jun 12, 2015 at 08:28:51PM +0800, Daniel Kurtz wrote:
> Hi Eddie,
>
> On Fri, Jun 12, 2015 at 5:27 PM, Eddie Huang <eddie.huang@mediatek.com> wrote:
> >
> > Add MT8173 I2C device nodes, include I2C controllers and pins.
> > MT8173 has six I2C controllers, from i2c0 to i2c6, exclude i2c5.
> > The 6th I2C controller register base doesn't next to 5th I2C,
> > and there is a hardware between 5th and 6th I2C controller. So
> > SoC designer name 6th controller as "i2c6", not "i2c5".
>
>
> This is slightly misleading. There are in fact 7 I2C controllers, but
> "i2c5" is dedicated for use by HDMI for ddc & hdcp.
> Is there a reason why the HDMI I2C port cannot be controlled by the
> generic i2c driver?
>
> Of course the hdmiddc / i2c5 node can always be added in a later
> patch, so this is no reason to hold up this patch.
>
> > Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
> > ---
> > arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 50 ++++++++++++++++++++
> > arch/arm64/boot/dts/mediatek/mt8173.dtsi | 72 +++++++++++++++++++++++++++++
> > 2 files changed, 122 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > index 43d5401..2e01988 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > @@ -33,6 +33,56 @@
> > chosen { };
> > };
> >
> > +&pio {
>
> I don't think we needed to move these i2c pinmux descriptions from
> mt8173.dtsi to the board .dts file.
>
> AFAICT, the "i2cN_pins_a" nodes defined here are the pinctl
> configuration that the corresponding enabled i2c nodes would choose.
> Thus, they are generic to the MT8173 SoC, not specific to any board.
> By themselves, these nodes do not actually select a pin configuration.
On i.MX we started with i2cN_pins_[abcde] groups in the SoC dtsi file
aswell. At some point we realized that this does not scale anymore.
The problems were:
- Whenever a new board came along which needed some pin setting which
didn't already exist a new group was created in the SoC dtsi file using
the next free letter from the alphabet. The ordering of the group names
was rather arbitrary and often enough there were merge conflicts to
resolve when during one merge window two new boards showed up which both
added i2cN_pin_x with different content.
- When a new board needs the same pins but with different drive strength
settings a new group was required
- With SD/MMC we had groups for 4bit data, groups adding the remaining
pins for 8bit data
- With UARTs we had groups for RX/TX and additionally groups adding
RTS/CTS
- For graphics we had groups for 16bit data and 24bit data
- There is no way to remove the unused nodes from the binary dtb, so
every board contained all existing pingroups for every other board,
so the dtbs became quite big
So with only a few existing boards with very similar pinmux groups
it may work fine to put the groups into the SoC dtsi, but this way
may also explode quite fast with more and different boards.
I don't know how much variation there will be with Mediatek boards, so
I'm fine with either way. You may want to look at:
5a2a7d5 ARM: dts: imx51: make pinctrl nodes board specific
7ac0f70 ARM: dts: imx53: make pinctrl nodes board specific
817c27a ARM: dts: imx6qdl: make pinctrl nodes board specific
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v3 2/2] arm64: dts: mt8173: Add I2C device node
@ 2015-06-15 7:46 ` Eddie Huang
0 siblings, 0 replies; 30+ messages in thread
From: Eddie Huang @ 2015-06-15 7:46 UTC (permalink / raw)
To: Sascha Hauer
Cc: Daniel Kurtz, Matthias Brugger, open list:OPEN FIRMWARE AND...,
linux-arm-kernel, linux-kernel, linux-mediatek, Linus Walleij
Hi Sascha,
On Mon, 2015-06-15 at 08:12 +0200, Sascha Hauer wrote:
> On Fri, Jun 12, 2015 at 08:28:51PM +0800, Daniel Kurtz wrote:
> > On Fri, Jun 12, 2015 at 5:27 PM, Eddie Huang <eddie.huang@mediatek.com> wrote:
> > >
> > > Add MT8173 I2C device nodes, include I2C controllers and pins.
> > > MT8173 has six I2C controllers, from i2c0 to i2c6, exclude i2c5.
> > > The 6th I2C controller register base doesn't next to 5th I2C,
> > > and there is a hardware between 5th and 6th I2C controller. So
> > > SoC designer name 6th controller as "i2c6", not "i2c5".
> >
> >
> > This is slightly misleading. There are in fact 7 I2C controllers, but
> > "i2c5" is dedicated for use by HDMI for ddc & hdcp.
> > Is there a reason why the HDMI I2C port cannot be controlled by the
> > generic i2c driver?
> >
> > Of course the hdmiddc / i2c5 node can always be added in a later
> > patch, so this is no reason to hold up this patch.
> >
> > > Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
> > > ---
> > > arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 50 ++++++++++++++++++++
> > > arch/arm64/boot/dts/mediatek/mt8173.dtsi | 72 +++++++++++++++++++++++++++++
> > > 2 files changed, 122 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > > index 43d5401..2e01988 100644
> > > --- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > > +++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > > @@ -33,6 +33,56 @@
> > > chosen { };
> > > };
> > >
> > > +&pio {
> >
> > I don't think we needed to move these i2c pinmux descriptions from
> > mt8173.dtsi to the board .dts file.
> >
> > AFAICT, the "i2cN_pins_a" nodes defined here are the pinctl
> > configuration that the corresponding enabled i2c nodes would choose.
> > Thus, they are generic to the MT8173 SoC, not specific to any board.
> > By themselves, these nodes do not actually select a pin configuration.
>
> On i.MX we started with i2cN_pins_[abcde] groups in the SoC dtsi file
> aswell. At some point we realized that this does not scale anymore.
> The problems were:
>
> - Whenever a new board came along which needed some pin setting which
> didn't already exist a new group was created in the SoC dtsi file using
> the next free letter from the alphabet. The ordering of the group names
> was rather arbitrary and often enough there were merge conflicts to
> resolve when during one merge window two new boards showed up which both
> added i2cN_pin_x with different content.
> - When a new board needs the same pins but with different drive strength
> settings a new group was required
> - With SD/MMC we had groups for 4bit data, groups adding the remaining
> pins for 8bit data
> - With UARTs we had groups for RX/TX and additionally groups adding
> RTS/CTS
> - For graphics we had groups for 16bit data and 24bit data
> - There is no way to remove the unused nodes from the binary dtb, so
> every board contained all existing pingroups for every other board,
> so the dtbs became quite big
>
> So with only a few existing boards with very similar pinmux groups
> it may work fine to put the groups into the SoC dtsi, but this way
> may also explode quite fast with more and different boards.
> I don't know how much variation there will be with Mediatek boards, so
> I'm fine with either way. You may want to look at:
>
> 5a2a7d5 ARM: dts: imx51: make pinctrl nodes board specific
> 7ac0f70 ARM: dts: imx53: make pinctrl nodes board specific
> 817c27a ARM: dts: imx6qdl: make pinctrl nodes board specific
>
We tend to put basic and fixed pins in SoC dtsi, platform-variant pins
to board dts.Since these pins already move once, I hope we reach a
consensus that I put i2c pins back to SoC dtsi.
Eddie
Thanks
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH v3 2/2] arm64: dts: mt8173: Add I2C device node
@ 2015-06-15 7:46 ` Eddie Huang
0 siblings, 0 replies; 30+ messages in thread
From: Eddie Huang @ 2015-06-15 7:46 UTC (permalink / raw)
To: linux-arm-kernel
Hi Sascha,
On Mon, 2015-06-15 at 08:12 +0200, Sascha Hauer wrote:
> On Fri, Jun 12, 2015 at 08:28:51PM +0800, Daniel Kurtz wrote:
> > On Fri, Jun 12, 2015 at 5:27 PM, Eddie Huang <eddie.huang@mediatek.com> wrote:
> > >
> > > Add MT8173 I2C device nodes, include I2C controllers and pins.
> > > MT8173 has six I2C controllers, from i2c0 to i2c6, exclude i2c5.
> > > The 6th I2C controller register base doesn't next to 5th I2C,
> > > and there is a hardware between 5th and 6th I2C controller. So
> > > SoC designer name 6th controller as "i2c6", not "i2c5".
> >
> >
> > This is slightly misleading. There are in fact 7 I2C controllers, but
> > "i2c5" is dedicated for use by HDMI for ddc & hdcp.
> > Is there a reason why the HDMI I2C port cannot be controlled by the
> > generic i2c driver?
> >
> > Of course the hdmiddc / i2c5 node can always be added in a later
> > patch, so this is no reason to hold up this patch.
> >
> > > Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
> > > ---
> > > arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 50 ++++++++++++++++++++
> > > arch/arm64/boot/dts/mediatek/mt8173.dtsi | 72 +++++++++++++++++++++++++++++
> > > 2 files changed, 122 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > > index 43d5401..2e01988 100644
> > > --- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > > +++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > > @@ -33,6 +33,56 @@
> > > chosen { };
> > > };
> > >
> > > +&pio {
> >
> > I don't think we needed to move these i2c pinmux descriptions from
> > mt8173.dtsi to the board .dts file.
> >
> > AFAICT, the "i2cN_pins_a" nodes defined here are the pinctl
> > configuration that the corresponding enabled i2c nodes would choose.
> > Thus, they are generic to the MT8173 SoC, not specific to any board.
> > By themselves, these nodes do not actually select a pin configuration.
>
> On i.MX we started with i2cN_pins_[abcde] groups in the SoC dtsi file
> aswell. At some point we realized that this does not scale anymore.
> The problems were:
>
> - Whenever a new board came along which needed some pin setting which
> didn't already exist a new group was created in the SoC dtsi file using
> the next free letter from the alphabet. The ordering of the group names
> was rather arbitrary and often enough there were merge conflicts to
> resolve when during one merge window two new boards showed up which both
> added i2cN_pin_x with different content.
> - When a new board needs the same pins but with different drive strength
> settings a new group was required
> - With SD/MMC we had groups for 4bit data, groups adding the remaining
> pins for 8bit data
> - With UARTs we had groups for RX/TX and additionally groups adding
> RTS/CTS
> - For graphics we had groups for 16bit data and 24bit data
> - There is no way to remove the unused nodes from the binary dtb, so
> every board contained all existing pingroups for every other board,
> so the dtbs became quite big
>
> So with only a few existing boards with very similar pinmux groups
> it may work fine to put the groups into the SoC dtsi, but this way
> may also explode quite fast with more and different boards.
> I don't know how much variation there will be with Mediatek boards, so
> I'm fine with either way. You may want to look at:
>
> 5a2a7d5 ARM: dts: imx51: make pinctrl nodes board specific
> 7ac0f70 ARM: dts: imx53: make pinctrl nodes board specific
> 817c27a ARM: dts: imx6qdl: make pinctrl nodes board specific
>
We tend to put basic and fixed pins in SoC dtsi, platform-variant pins
to board dts.Since these pins already move once, I hope we reach a
consensus that I put i2c pins back to SoC dtsi.
Eddie
Thanks
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v3 2/2] arm64: dts: mt8173: Add I2C device node
@ 2015-06-15 7:46 ` Eddie Huang
0 siblings, 0 replies; 30+ messages in thread
From: Eddie Huang @ 2015-06-15 7:46 UTC (permalink / raw)
To: Sascha Hauer
Cc: Daniel Kurtz, Matthias Brugger, open list:OPEN FIRMWARE AND...,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Linus Walleij
Hi Sascha,
On Mon, 2015-06-15 at 08:12 +0200, Sascha Hauer wrote:
> On Fri, Jun 12, 2015 at 08:28:51PM +0800, Daniel Kurtz wrote:
> > On Fri, Jun 12, 2015 at 5:27 PM, Eddie Huang <eddie.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> > >
> > > Add MT8173 I2C device nodes, include I2C controllers and pins.
> > > MT8173 has six I2C controllers, from i2c0 to i2c6, exclude i2c5.
> > > The 6th I2C controller register base doesn't next to 5th I2C,
> > > and there is a hardware between 5th and 6th I2C controller. So
> > > SoC designer name 6th controller as "i2c6", not "i2c5".
> >
> >
> > This is slightly misleading. There are in fact 7 I2C controllers, but
> > "i2c5" is dedicated for use by HDMI for ddc & hdcp.
> > Is there a reason why the HDMI I2C port cannot be controlled by the
> > generic i2c driver?
> >
> > Of course the hdmiddc / i2c5 node can always be added in a later
> > patch, so this is no reason to hold up this patch.
> >
> > > Signed-off-by: Eddie Huang <eddie.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > > ---
> > > arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 50 ++++++++++++++++++++
> > > arch/arm64/boot/dts/mediatek/mt8173.dtsi | 72 +++++++++++++++++++++++++++++
> > > 2 files changed, 122 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > > index 43d5401..2e01988 100644
> > > --- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > > +++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > > @@ -33,6 +33,56 @@
> > > chosen { };
> > > };
> > >
> > > +&pio {
> >
> > I don't think we needed to move these i2c pinmux descriptions from
> > mt8173.dtsi to the board .dts file.
> >
> > AFAICT, the "i2cN_pins_a" nodes defined here are the pinctl
> > configuration that the corresponding enabled i2c nodes would choose.
> > Thus, they are generic to the MT8173 SoC, not specific to any board.
> > By themselves, these nodes do not actually select a pin configuration.
>
> On i.MX we started with i2cN_pins_[abcde] groups in the SoC dtsi file
> aswell. At some point we realized that this does not scale anymore.
> The problems were:
>
> - Whenever a new board came along which needed some pin setting which
> didn't already exist a new group was created in the SoC dtsi file using
> the next free letter from the alphabet. The ordering of the group names
> was rather arbitrary and often enough there were merge conflicts to
> resolve when during one merge window two new boards showed up which both
> added i2cN_pin_x with different content.
> - When a new board needs the same pins but with different drive strength
> settings a new group was required
> - With SD/MMC we had groups for 4bit data, groups adding the remaining
> pins for 8bit data
> - With UARTs we had groups for RX/TX and additionally groups adding
> RTS/CTS
> - For graphics we had groups for 16bit data and 24bit data
> - There is no way to remove the unused nodes from the binary dtb, so
> every board contained all existing pingroups for every other board,
> so the dtbs became quite big
>
> So with only a few existing boards with very similar pinmux groups
> it may work fine to put the groups into the SoC dtsi, but this way
> may also explode quite fast with more and different boards.
> I don't know how much variation there will be with Mediatek boards, so
> I'm fine with either way. You may want to look at:
>
> 5a2a7d5 ARM: dts: imx51: make pinctrl nodes board specific
> 7ac0f70 ARM: dts: imx53: make pinctrl nodes board specific
> 817c27a ARM: dts: imx6qdl: make pinctrl nodes board specific
>
We tend to put basic and fixed pins in SoC dtsi, platform-variant pins
to board dts.Since these pins already move once, I hope we reach a
consensus that I put i2c pins back to SoC dtsi.
Eddie
Thanks
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 2/2] arm64: dts: mt8173: Add I2C device node
@ 2015-06-15 7:35 ` Eddie Huang
0 siblings, 0 replies; 30+ messages in thread
From: Eddie Huang @ 2015-06-15 7:35 UTC (permalink / raw)
To: Daniel Kurtz
Cc: Matthias Brugger, Sascha Hauer, open list:OPEN FIRMWARE AND...,
linux-arm-kernel, linux-kernel, linux-mediatek, Linus Walleij
Hi Dan,
On Fri, 2015-06-12 at 20:28 +0800, Daniel Kurtz wrote:
> On Fri, Jun 12, 2015 at 5:27 PM, Eddie Huang <eddie.huang@mediatek.com> wrote:
> >
> > Add MT8173 I2C device nodes, include I2C controllers and pins.
> > MT8173 has six I2C controllers, from i2c0 to i2c6, exclude i2c5.
> > The 6th I2C controller register base doesn't next to 5th I2C,
> > and there is a hardware between 5th and 6th I2C controller. So
> > SoC designer name 6th controller as "i2c6", not "i2c5".
>
>
> This is slightly misleading. There are in fact 7 I2C controllers, but
> "i2c5" is dedicated for use by HDMI for ddc & hdcp.
> Is there a reason why the HDMI I2C port cannot be controlled by the
> generic i2c driver?
>
We add some extra HW function to HDMI I2C port, we have special driver
to control this HW, not generic I2C driver. This is why I don't count
this hardware to generic I2C controllers.
> Of course the hdmiddc / i2c5 node can always be added in a later
> patch, so this is no reason to hold up this patch.
>
> > Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
> > ---
> > arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 50 ++++++++++++++++++++
> > arch/arm64/boot/dts/mediatek/mt8173.dtsi | 72 +++++++++++++++++++++++++++++
> > 2 files changed, 122 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > index 43d5401..2e01988 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > @@ -33,6 +33,56 @@
> > chosen { };
> > };
> >
> > +&pio {
>
> I don't think we needed to move these i2c pinmux descriptions from
> mt8173.dtsi to the board .dts file.
>
> AFAICT, the "i2cN_pins_a" nodes defined here are the pinctl
> configuration that the corresponding enabled i2c nodes would choose.
> Thus, they are generic to the MT8173 SoC, not specific to any board.
> By themselves, these nodes do not actually select a pin configuration.
>
> It is the nodes that *enable* the individual i2c nodes, and hence
> activate those pin settings, which is board specific.
>
> Hence, if your intent is to have the evb enable all i2c nodes, it
> would have a set of nodes like this:
>
>
> &i2c0 {
> pinctrl-names = "default";
> pinctrl-0 = <&i2c0_pins_a>;
> status = "okay";
> };
>
> &i2c1 {
> pinctrl-names = "default";
> pinctrl-0 = <&i2c1_pins_a>;
> status = "okay";
> };
>
> ...
>
I personally think put I2C pins in SoC dtsi is ok because it is basic
and fixed. Almost every platform need these pins, so not necessary to
care about dtb size. And these pins are fixed. We put pins that may
change by platforms to board dts, like mmc.
>
> > + i2c0_pins_a: i2c0@0 {
>
> Do these nodes need the "@0"?
>
Will remove.
> >
> > + pins1 {
> > + pinmux = <MT8173_PIN_45_SDA0__FUNC_SDA0>,
> > + <MT8173_PIN_46_SCL0__FUNC_SCL0>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c1_pins_a: i2c1@0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_125_SDA1__FUNC_SDA1>,
> > + <MT8173_PIN_126_SCL1__FUNC_SCL1>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c2_pins_a: i2c2@0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_43_SDA2__FUNC_SDA2>,
> > + <MT8173_PIN_44_SCL2__FUNC_SCL2>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c3_pins_a: i2c3@0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_106_SDA3__FUNC_SDA3>,
> > + <MT8173_PIN_107_SCL3__FUNC_SCL3>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c4_pins_a: i2c4@0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_133_SDA4__FUNC_SDA4>,
> > + <MT8173_PIN_134_SCL4__FUNC_SCL4>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c6_pins_a: i2c6@0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_100_MSDC2_DAT0__FUNC_SDA5>,
> > + <MT8173_PIN_101_MSDC2_DAT1__FUNC_SCL5>;
>
> These are the SDA/SCL pins for i2c port 6, so they should really be
> _SDA6 / _SCL6.
> However... I checked, and these settings are labeled "SDA5 & SCL5" in
> the datasheet.
> I recommend marking them correctly as 6 here and fixing the datasheet :-).
>
We make mistake at the beginning not give these pins suitable name, if
we change now, it may affect already shipped products. So I tend to keep
it.
> >
> > + bias-disable;
> > + };
> > + };
> > +};
> > +
> > &uart0 {
> > status = "okay";
> > };
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > index b52ec43..6d3dbbdd 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > @@ -229,6 +229,78 @@
> > clocks = <&uart_clk>;
> > status = "disabled";
> > };
> > +
> > + i2c0: i2c@11007000 {
> > + compatible = "mediatek,mt8173-i2c";
> > + reg = <0 0x11007000 0 0x70>,
> > + <0 0x11000100 0 0x80>;
> > + interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_LOW>;
> > + clock-div = <16>;
> > + clocks = <&pericfg CLK_PERI_I2C0>,
> > + <&pericfg CLK_PERI_AP_DMA>;
> > + clock-names = "main", "dma";
>
> The following fields must also be selected by the i2c nodes when they
> are enabled:
>
> clock-frequency = <100000>;
> pinctrl-names = "default";
> pinctrl-0 = <&i2c0_pins_a>;
> #address-cells = <1>;
> #size-cells = <0>;
>
> So, is there any reason not to also include them here in mt8173.dtsi
> so we can use them as defaults?
> This would simplify the i2c nodes in the board specific .dts files.
> The only field that might be board specific would be clock-frequency,
> but that is trivial to override on a port-by-port basis in board files
> as necessary.
>
Since I tend to put I2C pins in SoC dtsi, I think I will put all you
mention above in SoC dtsi.
Eddie
Thanks
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH v3 2/2] arm64: dts: mt8173: Add I2C device node
@ 2015-06-15 7:35 ` Eddie Huang
0 siblings, 0 replies; 30+ messages in thread
From: Eddie Huang @ 2015-06-15 7:35 UTC (permalink / raw)
To: linux-arm-kernel
Hi Dan,
On Fri, 2015-06-12 at 20:28 +0800, Daniel Kurtz wrote:
> On Fri, Jun 12, 2015 at 5:27 PM, Eddie Huang <eddie.huang@mediatek.com> wrote:
> >
> > Add MT8173 I2C device nodes, include I2C controllers and pins.
> > MT8173 has six I2C controllers, from i2c0 to i2c6, exclude i2c5.
> > The 6th I2C controller register base doesn't next to 5th I2C,
> > and there is a hardware between 5th and 6th I2C controller. So
> > SoC designer name 6th controller as "i2c6", not "i2c5".
>
>
> This is slightly misleading. There are in fact 7 I2C controllers, but
> "i2c5" is dedicated for use by HDMI for ddc & hdcp.
> Is there a reason why the HDMI I2C port cannot be controlled by the
> generic i2c driver?
>
We add some extra HW function to HDMI I2C port, we have special driver
to control this HW, not generic I2C driver. This is why I don't count
this hardware to generic I2C controllers.
> Of course the hdmiddc / i2c5 node can always be added in a later
> patch, so this is no reason to hold up this patch.
>
> > Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
> > ---
> > arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 50 ++++++++++++++++++++
> > arch/arm64/boot/dts/mediatek/mt8173.dtsi | 72 +++++++++++++++++++++++++++++
> > 2 files changed, 122 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > index 43d5401..2e01988 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > @@ -33,6 +33,56 @@
> > chosen { };
> > };
> >
> > +&pio {
>
> I don't think we needed to move these i2c pinmux descriptions from
> mt8173.dtsi to the board .dts file.
>
> AFAICT, the "i2cN_pins_a" nodes defined here are the pinctl
> configuration that the corresponding enabled i2c nodes would choose.
> Thus, they are generic to the MT8173 SoC, not specific to any board.
> By themselves, these nodes do not actually select a pin configuration.
>
> It is the nodes that *enable* the individual i2c nodes, and hence
> activate those pin settings, which is board specific.
>
> Hence, if your intent is to have the evb enable all i2c nodes, it
> would have a set of nodes like this:
>
>
> &i2c0 {
> pinctrl-names = "default";
> pinctrl-0 = <&i2c0_pins_a>;
> status = "okay";
> };
>
> &i2c1 {
> pinctrl-names = "default";
> pinctrl-0 = <&i2c1_pins_a>;
> status = "okay";
> };
>
> ...
>
I personally think put I2C pins in SoC dtsi is ok because it is basic
and fixed. Almost every platform need these pins, so not necessary to
care about dtb size. And these pins are fixed. We put pins that may
change by platforms to board dts, like mmc.
>
> > + i2c0_pins_a: i2c0 at 0 {
>
> Do these nodes need the "@0"?
>
Will remove.
> >
> > + pins1 {
> > + pinmux = <MT8173_PIN_45_SDA0__FUNC_SDA0>,
> > + <MT8173_PIN_46_SCL0__FUNC_SCL0>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c1_pins_a: i2c1 at 0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_125_SDA1__FUNC_SDA1>,
> > + <MT8173_PIN_126_SCL1__FUNC_SCL1>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c2_pins_a: i2c2 at 0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_43_SDA2__FUNC_SDA2>,
> > + <MT8173_PIN_44_SCL2__FUNC_SCL2>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c3_pins_a: i2c3 at 0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_106_SDA3__FUNC_SDA3>,
> > + <MT8173_PIN_107_SCL3__FUNC_SCL3>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c4_pins_a: i2c4 at 0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_133_SDA4__FUNC_SDA4>,
> > + <MT8173_PIN_134_SCL4__FUNC_SCL4>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c6_pins_a: i2c6 at 0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_100_MSDC2_DAT0__FUNC_SDA5>,
> > + <MT8173_PIN_101_MSDC2_DAT1__FUNC_SCL5>;
>
> These are the SDA/SCL pins for i2c port 6, so they should really be
> _SDA6 / _SCL6.
> However... I checked, and these settings are labeled "SDA5 & SCL5" in
> the datasheet.
> I recommend marking them correctly as 6 here and fixing the datasheet :-).
>
We make mistake at the beginning not give these pins suitable name, if
we change now, it may affect already shipped products. So I tend to keep
it.
> >
> > + bias-disable;
> > + };
> > + };
> > +};
> > +
> > &uart0 {
> > status = "okay";
> > };
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > index b52ec43..6d3dbbdd 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > @@ -229,6 +229,78 @@
> > clocks = <&uart_clk>;
> > status = "disabled";
> > };
> > +
> > + i2c0: i2c at 11007000 {
> > + compatible = "mediatek,mt8173-i2c";
> > + reg = <0 0x11007000 0 0x70>,
> > + <0 0x11000100 0 0x80>;
> > + interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_LOW>;
> > + clock-div = <16>;
> > + clocks = <&pericfg CLK_PERI_I2C0>,
> > + <&pericfg CLK_PERI_AP_DMA>;
> > + clock-names = "main", "dma";
>
> The following fields must also be selected by the i2c nodes when they
> are enabled:
>
> clock-frequency = <100000>;
> pinctrl-names = "default";
> pinctrl-0 = <&i2c0_pins_a>;
> #address-cells = <1>;
> #size-cells = <0>;
>
> So, is there any reason not to also include them here in mt8173.dtsi
> so we can use them as defaults?
> This would simplify the i2c nodes in the board specific .dts files.
> The only field that might be board specific would be clock-frequency,
> but that is trivial to override on a port-by-port basis in board files
> as necessary.
>
Since I tend to put I2C pins in SoC dtsi, I think I will put all you
mention above in SoC dtsi.
Eddie
Thanks
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v3 2/2] arm64: dts: mt8173: Add I2C device node
@ 2015-06-15 7:35 ` Eddie Huang
0 siblings, 0 replies; 30+ messages in thread
From: Eddie Huang @ 2015-06-15 7:35 UTC (permalink / raw)
To: Daniel Kurtz
Cc: Matthias Brugger, Sascha Hauer, open list:OPEN FIRMWARE AND...,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Linus Walleij
Hi Dan,
On Fri, 2015-06-12 at 20:28 +0800, Daniel Kurtz wrote:
> On Fri, Jun 12, 2015 at 5:27 PM, Eddie Huang <eddie.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> >
> > Add MT8173 I2C device nodes, include I2C controllers and pins.
> > MT8173 has six I2C controllers, from i2c0 to i2c6, exclude i2c5.
> > The 6th I2C controller register base doesn't next to 5th I2C,
> > and there is a hardware between 5th and 6th I2C controller. So
> > SoC designer name 6th controller as "i2c6", not "i2c5".
>
>
> This is slightly misleading. There are in fact 7 I2C controllers, but
> "i2c5" is dedicated for use by HDMI for ddc & hdcp.
> Is there a reason why the HDMI I2C port cannot be controlled by the
> generic i2c driver?
>
We add some extra HW function to HDMI I2C port, we have special driver
to control this HW, not generic I2C driver. This is why I don't count
this hardware to generic I2C controllers.
> Of course the hdmiddc / i2c5 node can always be added in a later
> patch, so this is no reason to hold up this patch.
>
> > Signed-off-by: Eddie Huang <eddie.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > ---
> > arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 50 ++++++++++++++++++++
> > arch/arm64/boot/dts/mediatek/mt8173.dtsi | 72 +++++++++++++++++++++++++++++
> > 2 files changed, 122 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > index 43d5401..2e01988 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > @@ -33,6 +33,56 @@
> > chosen { };
> > };
> >
> > +&pio {
>
> I don't think we needed to move these i2c pinmux descriptions from
> mt8173.dtsi to the board .dts file.
>
> AFAICT, the "i2cN_pins_a" nodes defined here are the pinctl
> configuration that the corresponding enabled i2c nodes would choose.
> Thus, they are generic to the MT8173 SoC, not specific to any board.
> By themselves, these nodes do not actually select a pin configuration.
>
> It is the nodes that *enable* the individual i2c nodes, and hence
> activate those pin settings, which is board specific.
>
> Hence, if your intent is to have the evb enable all i2c nodes, it
> would have a set of nodes like this:
>
>
> &i2c0 {
> pinctrl-names = "default";
> pinctrl-0 = <&i2c0_pins_a>;
> status = "okay";
> };
>
> &i2c1 {
> pinctrl-names = "default";
> pinctrl-0 = <&i2c1_pins_a>;
> status = "okay";
> };
>
> ...
>
I personally think put I2C pins in SoC dtsi is ok because it is basic
and fixed. Almost every platform need these pins, so not necessary to
care about dtb size. And these pins are fixed. We put pins that may
change by platforms to board dts, like mmc.
>
> > + i2c0_pins_a: i2c0@0 {
>
> Do these nodes need the "@0"?
>
Will remove.
> >
> > + pins1 {
> > + pinmux = <MT8173_PIN_45_SDA0__FUNC_SDA0>,
> > + <MT8173_PIN_46_SCL0__FUNC_SCL0>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c1_pins_a: i2c1@0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_125_SDA1__FUNC_SDA1>,
> > + <MT8173_PIN_126_SCL1__FUNC_SCL1>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c2_pins_a: i2c2@0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_43_SDA2__FUNC_SDA2>,
> > + <MT8173_PIN_44_SCL2__FUNC_SCL2>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c3_pins_a: i2c3@0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_106_SDA3__FUNC_SDA3>,
> > + <MT8173_PIN_107_SCL3__FUNC_SCL3>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c4_pins_a: i2c4@0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_133_SDA4__FUNC_SDA4>,
> > + <MT8173_PIN_134_SCL4__FUNC_SCL4>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + i2c6_pins_a: i2c6@0 {
> > + pins1 {
> > + pinmux = <MT8173_PIN_100_MSDC2_DAT0__FUNC_SDA5>,
> > + <MT8173_PIN_101_MSDC2_DAT1__FUNC_SCL5>;
>
> These are the SDA/SCL pins for i2c port 6, so they should really be
> _SDA6 / _SCL6.
> However... I checked, and these settings are labeled "SDA5 & SCL5" in
> the datasheet.
> I recommend marking them correctly as 6 here and fixing the datasheet :-).
>
We make mistake at the beginning not give these pins suitable name, if
we change now, it may affect already shipped products. So I tend to keep
it.
> >
> > + bias-disable;
> > + };
> > + };
> > +};
> > +
> > &uart0 {
> > status = "okay";
> > };
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > index b52ec43..6d3dbbdd 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > @@ -229,6 +229,78 @@
> > clocks = <&uart_clk>;
> > status = "disabled";
> > };
> > +
> > + i2c0: i2c@11007000 {
> > + compatible = "mediatek,mt8173-i2c";
> > + reg = <0 0x11007000 0 0x70>,
> > + <0 0x11000100 0 0x80>;
> > + interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_LOW>;
> > + clock-div = <16>;
> > + clocks = <&pericfg CLK_PERI_I2C0>,
> > + <&pericfg CLK_PERI_AP_DMA>;
> > + clock-names = "main", "dma";
>
> The following fields must also be selected by the i2c nodes when they
> are enabled:
>
> clock-frequency = <100000>;
> pinctrl-names = "default";
> pinctrl-0 = <&i2c0_pins_a>;
> #address-cells = <1>;
> #size-cells = <0>;
>
> So, is there any reason not to also include them here in mt8173.dtsi
> so we can use them as defaults?
> This would simplify the i2c nodes in the board specific .dts files.
> The only field that might be board specific would be clock-frequency,
> but that is trivial to override on a port-by-port basis in board files
> as necessary.
>
Since I tend to put I2C pins in SoC dtsi, I think I will put all you
mention above in SoC dtsi.
Eddie
Thanks
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 30+ messages in thread