Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1558068 > unrolled thread

[PATCH v1 2/2] arm: dts: mt2701: add nor flash node

Started byGuochun Mao <guochun.mao@mediatek.com>
First post2017-01-13 08:30 +0100
Last post2017-01-16 17:10 +0100
Articles 14 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Guochun Mao <guochun.mao@mediatek.com> - 2017-01-13 08:30 +0100
    Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Marek Vasut <marek.vasut@gmail.com> - 2017-01-13 14:30 +0100
    Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-01-13 15:20 +0100
      Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Matthias Brugger <matthias.bgg@gmail.com> - 2017-01-13 16:20 +0100
        Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-01-13 16:30 +0100
        Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Marek Vasut <marek.vasut@gmail.com> - 2017-01-13 17:20 +0100
          Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-01-13 17:30 +0100
            Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Marek Vasut <marek.vasut@gmail.com> - 2017-01-13 17:50 +0100
              Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-01-13 18:00 +0100
                Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Marek Vasut <marek.vasut@gmail.com> - 2017-01-13 18:40 +0100
                  Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-01-14 09:40 +0100
                    Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Marek Vasut <marek.vasut@gmail.com> - 2017-01-15 01:30 +0100
                      Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-01-16 09:50 +0100
                        Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Marek Vasut <marek.vasut@gmail.com> - 2017-01-16 17:10 +0100

#1558068 — [PATCH v1 2/2] arm: dts: mt2701: add nor flash node

FromGuochun Mao <guochun.mao@mediatek.com>
Date2017-01-13 08:30 +0100
Subject[PATCH v1 2/2] arm: dts: mt2701: add nor flash node
Message-ID<sZ4tc-6qS-5@gated-at.bofh.it>
Add Mediatek nor flash node.

Signed-off-by: Guochun Mao <guochun.mao@mediatek.com>
---
 arch/arm/boot/dts/mt2701-evb.dts |   25 +++++++++++++++++++++++++
 arch/arm/boot/dts/mt2701.dtsi    |   12 ++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/arch/arm/boot/dts/mt2701-evb.dts b/arch/arm/boot/dts/mt2701-evb.dts
index 082ca88..85e5ae8 100644
--- a/arch/arm/boot/dts/mt2701-evb.dts
+++ b/arch/arm/boot/dts/mt2701-evb.dts
@@ -24,6 +24,31 @@
 	};
 };
 
+&nor_flash {
+	pinctrl-names = "default";
+	pinctrl-0 = <&nor_pins_default>;
+	status = "okay";
+	flash@0 {
+		compatible = "jedec,spi-nor";
+		reg = <0>;
+	};
+};
+
+&pio {
+	nor_pins_default: nor {
+		pins1 {
+			pinmux = <MT2701_PIN_240_EXT_XCS__FUNC_EXT_XCS>,
+				 <MT2701_PIN_241_EXT_SCK__FUNC_EXT_SCK>,
+				 <MT2701_PIN_239_EXT_SDIO0__FUNC_EXT_SDIO0>,
+				 <MT2701_PIN_238_EXT_SDIO1__FUNC_EXT_SDIO1>,
+				 <MT2701_PIN_237_EXT_SDIO2__FUNC_EXT_SDIO2>,
+				 <MT2701_PIN_236_EXT_SDIO3__FUNC_EXT_SDIO3>;
+			drive-strength = <MTK_DRIVE_4mA>;
+			bias-pull-up;
+		};
+	};
+};
+
 &uart0 {
 	status = "okay";
 };
diff --git a/arch/arm/boot/dts/mt2701.dtsi b/arch/arm/boot/dts/mt2701.dtsi
index bdf8954..1eefce4 100644
--- a/arch/arm/boot/dts/mt2701.dtsi
+++ b/arch/arm/boot/dts/mt2701.dtsi
@@ -227,6 +227,18 @@
 		status = "disabled";
 	};
 
+	nor_flash: spi@11014000 {
+		compatible = "mediatek,mt2701-nor",
+			     "mediatek,mt8173-nor";
+		reg = <0 0x11014000 0 0xe0>;
+		clocks = <&pericfg CLK_PERI_FLASH>,
+			 <&topckgen CLK_TOP_FLASH_SEL>;
+		clock-names = "spi", "sf";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		status = "disabled";
+	};
+
 	mmsys: syscon@14000000 {
 		compatible = "mediatek,mt2701-mmsys", "syscon";
 		reg = <0 0x14000000 0 0x1000>;
-- 
1.7.9.5

[toc] | [next] | [standalone]

#1558404

FromMarek Vasut <marek.vasut@gmail.com>
Date2017-01-13 14:30 +0100
Message-ID<sZa5z-1nw-11@gated-at.bofh.it>
In reply to#1558068
On 01/13/2017 08:13 AM, Guochun Mao wrote:
> Add Mediatek nor flash node.
> 
> Signed-off-by: Guochun Mao <guochun.mao@mediatek.com>
> ---
>  arch/arm/boot/dts/mt2701-evb.dts |   25 +++++++++++++++++++++++++
>  arch/arm/boot/dts/mt2701.dtsi    |   12 ++++++++++++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/mt2701-evb.dts b/arch/arm/boot/dts/mt2701-evb.dts
> index 082ca88..85e5ae8 100644
> --- a/arch/arm/boot/dts/mt2701-evb.dts
> +++ b/arch/arm/boot/dts/mt2701-evb.dts
> @@ -24,6 +24,31 @@
>  	};
>  };
>  
> +&nor_flash {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&nor_pins_default>;
> +	status = "okay";
> +	flash@0 {
> +		compatible = "jedec,spi-nor";
> +		reg = <0>;
> +	};
> +};
> +
> +&pio {
> +	nor_pins_default: nor {
> +		pins1 {
> +			pinmux = <MT2701_PIN_240_EXT_XCS__FUNC_EXT_XCS>,
> +				 <MT2701_PIN_241_EXT_SCK__FUNC_EXT_SCK>,
> +				 <MT2701_PIN_239_EXT_SDIO0__FUNC_EXT_SDIO0>,
> +				 <MT2701_PIN_238_EXT_SDIO1__FUNC_EXT_SDIO1>,
> +				 <MT2701_PIN_237_EXT_SDIO2__FUNC_EXT_SDIO2>,
> +				 <MT2701_PIN_236_EXT_SDIO3__FUNC_EXT_SDIO3>;
> +			drive-strength = <MTK_DRIVE_4mA>;
> +			bias-pull-up;
> +		};
> +	};
> +};
> +
>  &uart0 {
>  	status = "okay";
>  };
> diff --git a/arch/arm/boot/dts/mt2701.dtsi b/arch/arm/boot/dts/mt2701.dtsi
> index bdf8954..1eefce4 100644
> --- a/arch/arm/boot/dts/mt2701.dtsi
> +++ b/arch/arm/boot/dts/mt2701.dtsi
> @@ -227,6 +227,18 @@
>  		status = "disabled";
>  	};
>  
> +	nor_flash: spi@11014000 {
> +		compatible = "mediatek,mt2701-nor",
> +			     "mediatek,mt8173-nor";



Reviewed-by: Marek Vasut <marek.vasut@gmail.com>

-- 
Best regards,
Marek Vasut

[toc] | [prev] | [next] | [standalone]

#1558426

FromBoris Brezillon <boris.brezillon@free-electrons.com>
Date2017-01-13 15:20 +0100
Message-ID<sZaRX-1TQ-13@gated-at.bofh.it>
In reply to#1558068
On Fri, 13 Jan 2017 15:13:29 +0800
Guochun Mao <guochun.mao@mediatek.com> wrote:

> Add Mediatek nor flash node.
> 
> Signed-off-by: Guochun Mao <guochun.mao@mediatek.com>
> ---
>  arch/arm/boot/dts/mt2701-evb.dts |   25 +++++++++++++++++++++++++
>  arch/arm/boot/dts/mt2701.dtsi    |   12 ++++++++++++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/mt2701-evb.dts b/arch/arm/boot/dts/mt2701-evb.dts
> index 082ca88..85e5ae8 100644
> --- a/arch/arm/boot/dts/mt2701-evb.dts
> +++ b/arch/arm/boot/dts/mt2701-evb.dts
> @@ -24,6 +24,31 @@
>  	};
>  };
>  
> +&nor_flash {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&nor_pins_default>;
> +	status = "okay";
> +	flash@0 {
> +		compatible = "jedec,spi-nor";
> +		reg = <0>;
> +	};
> +};
> +
> +&pio {
> +	nor_pins_default: nor {
> +		pins1 {
> +			pinmux = <MT2701_PIN_240_EXT_XCS__FUNC_EXT_XCS>,
> +				 <MT2701_PIN_241_EXT_SCK__FUNC_EXT_SCK>,
> +				 <MT2701_PIN_239_EXT_SDIO0__FUNC_EXT_SDIO0>,
> +				 <MT2701_PIN_238_EXT_SDIO1__FUNC_EXT_SDIO1>,
> +				 <MT2701_PIN_237_EXT_SDIO2__FUNC_EXT_SDIO2>,
> +				 <MT2701_PIN_236_EXT_SDIO3__FUNC_EXT_SDIO3>;
> +			drive-strength = <MTK_DRIVE_4mA>;
> +			bias-pull-up;
> +		};
> +	};
> +};
> +
>  &uart0 {
>  	status = "okay";
>  };
> diff --git a/arch/arm/boot/dts/mt2701.dtsi b/arch/arm/boot/dts/mt2701.dtsi
> index bdf8954..1eefce4 100644
> --- a/arch/arm/boot/dts/mt2701.dtsi
> +++ b/arch/arm/boot/dts/mt2701.dtsi
> @@ -227,6 +227,18 @@
>  		status = "disabled";
>  	};
>  
> +	nor_flash: spi@11014000 {
> +		compatible = "mediatek,mt2701-nor",
> +			     "mediatek,mt8173-nor";

Why define both here? Is "mediatek,mt8173-nor" really providing a
subset of the features supported by "mediatek,mt2701-nor"?

> +		reg = <0 0x11014000 0 0xe0>;
> +		clocks = <&pericfg CLK_PERI_FLASH>,
> +			 <&topckgen CLK_TOP_FLASH_SEL>;
> +		clock-names = "spi", "sf";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		status = "disabled";
> +	};
> +
>  	mmsys: syscon@14000000 {
>  		compatible = "mediatek,mt2701-mmsys", "syscon";
>  		reg = <0 0x14000000 0 0x1000>;

[toc] | [prev] | [next] | [standalone]

#1558467

FromMatthias Brugger <matthias.bgg@gmail.com>
Date2017-01-13 16:20 +0100
Message-ID<sZbO2-2sy-23@gated-at.bofh.it>
In reply to#1558426

On 13/01/17 15:17, Boris Brezillon wrote:
> On Fri, 13 Jan 2017 15:13:29 +0800
> Guochun Mao <guochun.mao@mediatek.com> wrote:
>
>> Add Mediatek nor flash node.
>>
>> Signed-off-by: Guochun Mao <guochun.mao@mediatek.com>
>> ---
>>  arch/arm/boot/dts/mt2701-evb.dts |   25 +++++++++++++++++++++++++
>>  arch/arm/boot/dts/mt2701.dtsi    |   12 ++++++++++++
>>  2 files changed, 37 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/mt2701-evb.dts b/arch/arm/boot/dts/mt2701-evb.dts
>> index 082ca88..85e5ae8 100644
>> --- a/arch/arm/boot/dts/mt2701-evb.dts
>> +++ b/arch/arm/boot/dts/mt2701-evb.dts
>> @@ -24,6 +24,31 @@
>>  	};
>>  };
>>
>> +&nor_flash {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&nor_pins_default>;
>> +	status = "okay";
>> +	flash@0 {
>> +		compatible = "jedec,spi-nor";
>> +		reg = <0>;
>> +	};
>> +};
>> +
>> +&pio {
>> +	nor_pins_default: nor {
>> +		pins1 {
>> +			pinmux = <MT2701_PIN_240_EXT_XCS__FUNC_EXT_XCS>,
>> +				 <MT2701_PIN_241_EXT_SCK__FUNC_EXT_SCK>,
>> +				 <MT2701_PIN_239_EXT_SDIO0__FUNC_EXT_SDIO0>,
>> +				 <MT2701_PIN_238_EXT_SDIO1__FUNC_EXT_SDIO1>,
>> +				 <MT2701_PIN_237_EXT_SDIO2__FUNC_EXT_SDIO2>,
>> +				 <MT2701_PIN_236_EXT_SDIO3__FUNC_EXT_SDIO3>;
>> +			drive-strength = <MTK_DRIVE_4mA>;
>> +			bias-pull-up;
>> +		};
>> +	};
>> +};
>> +
>>  &uart0 {
>>  	status = "okay";
>>  };
>> diff --git a/arch/arm/boot/dts/mt2701.dtsi b/arch/arm/boot/dts/mt2701.dtsi
>> index bdf8954..1eefce4 100644
>> --- a/arch/arm/boot/dts/mt2701.dtsi
>> +++ b/arch/arm/boot/dts/mt2701.dtsi
>> @@ -227,6 +227,18 @@
>>  		status = "disabled";
>>  	};
>>
>> +	nor_flash: spi@11014000 {
>> +		compatible = "mediatek,mt2701-nor",
>> +			     "mediatek,mt8173-nor";
>
> Why define both here? Is "mediatek,mt8173-nor" really providing a
> subset of the features supported by "mediatek,mt2701-nor"?
>

I think even if the ip block is the same, we should provide both 
bindings, just in case in the future we find out that mt2701 has some 
hidden bug, feature or bug-feature. This way even if we update the 
driver, we stay compatible with older device tree blobs in the wild.

We can drop the mt2701-nor in the bindings definition if you want.

Regards,
Matthias

>> +		reg = <0 0x11014000 0 0xe0>;
>> +		clocks = <&pericfg CLK_PERI_FLASH>,
>> +			 <&topckgen CLK_TOP_FLASH_SEL>;
>> +		clock-names = "spi", "sf";
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		status = "disabled";
>> +	};
>> +
>>  	mmsys: syscon@14000000 {
>>  		compatible = "mediatek,mt2701-mmsys", "syscon";
>>  		reg = <0 0x14000000 0 0x1000>;
>

[toc] | [prev] | [next] | [standalone]

#1558482

FromBoris Brezillon <boris.brezillon@free-electrons.com>
Date2017-01-13 16:30 +0100
Message-ID<sZbXI-2vV-27@gated-at.bofh.it>
In reply to#1558467
On Fri, 13 Jan 2017 16:12:20 +0100
Matthias Brugger <matthias.bgg@gmail.com> wrote:

> On 13/01/17 15:17, Boris Brezillon wrote:
> > On Fri, 13 Jan 2017 15:13:29 +0800
> > Guochun Mao <guochun.mao@mediatek.com> wrote:
> >  
> >> Add Mediatek nor flash node.
> >>
> >> Signed-off-by: Guochun Mao <guochun.mao@mediatek.com>
> >> ---
> >>  arch/arm/boot/dts/mt2701-evb.dts |   25 +++++++++++++++++++++++++
> >>  arch/arm/boot/dts/mt2701.dtsi    |   12 ++++++++++++
> >>  2 files changed, 37 insertions(+)
> >>
> >> diff --git a/arch/arm/boot/dts/mt2701-evb.dts b/arch/arm/boot/dts/mt2701-evb.dts
> >> index 082ca88..85e5ae8 100644
> >> --- a/arch/arm/boot/dts/mt2701-evb.dts
> >> +++ b/arch/arm/boot/dts/mt2701-evb.dts
> >> @@ -24,6 +24,31 @@
> >>  	};
> >>  };
> >>
> >> +&nor_flash {
> >> +	pinctrl-names = "default";
> >> +	pinctrl-0 = <&nor_pins_default>;
> >> +	status = "okay";
> >> +	flash@0 {
> >> +		compatible = "jedec,spi-nor";
> >> +		reg = <0>;
> >> +	};
> >> +};
> >> +
> >> +&pio {
> >> +	nor_pins_default: nor {
> >> +		pins1 {
> >> +			pinmux = <MT2701_PIN_240_EXT_XCS__FUNC_EXT_XCS>,
> >> +				 <MT2701_PIN_241_EXT_SCK__FUNC_EXT_SCK>,
> >> +				 <MT2701_PIN_239_EXT_SDIO0__FUNC_EXT_SDIO0>,
> >> +				 <MT2701_PIN_238_EXT_SDIO1__FUNC_EXT_SDIO1>,
> >> +				 <MT2701_PIN_237_EXT_SDIO2__FUNC_EXT_SDIO2>,
> >> +				 <MT2701_PIN_236_EXT_SDIO3__FUNC_EXT_SDIO3>;
> >> +			drive-strength = <MTK_DRIVE_4mA>;
> >> +			bias-pull-up;
> >> +		};
> >> +	};
> >> +};
> >> +
> >>  &uart0 {
> >>  	status = "okay";
> >>  };
> >> diff --git a/arch/arm/boot/dts/mt2701.dtsi b/arch/arm/boot/dts/mt2701.dtsi
> >> index bdf8954..1eefce4 100644
> >> --- a/arch/arm/boot/dts/mt2701.dtsi
> >> +++ b/arch/arm/boot/dts/mt2701.dtsi
> >> @@ -227,6 +227,18 @@
> >>  		status = "disabled";
> >>  	};
> >>
> >> +	nor_flash: spi@11014000 {
> >> +		compatible = "mediatek,mt2701-nor",
> >> +			     "mediatek,mt8173-nor";  
> >
> > Why define both here? Is "mediatek,mt8173-nor" really providing a
> > subset of the features supported by "mediatek,mt2701-nor"?
> >  
> 
> I think even if the ip block is the same, we should provide both 
> bindings, just in case in the future we find out that mt2701 has some 
> hidden bug, feature or bug-feature. This way even if we update the 
> driver, we stay compatible with older device tree blobs in the wild.

I'm fine with this approach, but in this case, defining both is wrong.

> 
> We can drop the mt2701-nor in the bindings definition if you want.

Yes, please.

> 
> Regards,
> Matthias
> 
> >> +		reg = <0 0x11014000 0 0xe0>;
> >> +		clocks = <&pericfg CLK_PERI_FLASH>,
> >> +			 <&topckgen CLK_TOP_FLASH_SEL>;
> >> +		clock-names = "spi", "sf";
> >> +		#address-cells = <1>;
> >> +		#size-cells = <0>;
> >> +		status = "disabled";
> >> +	};
> >> +
> >>  	mmsys: syscon@14000000 {
> >>  		compatible = "mediatek,mt2701-mmsys", "syscon";
> >>  		reg = <0 0x14000000 0 0x1000>;  
> >

[toc] | [prev] | [next] | [standalone]

#1558550

FromMarek Vasut <marek.vasut@gmail.com>
Date2017-01-13 17:20 +0100
Message-ID<sZcK5-31P-5@gated-at.bofh.it>
In reply to#1558467
On 01/13/2017 04:12 PM, Matthias Brugger wrote:
> 
> 
> On 13/01/17 15:17, Boris Brezillon wrote:
>> On Fri, 13 Jan 2017 15:13:29 +0800
>> Guochun Mao <guochun.mao@mediatek.com> wrote:
>>
>>> Add Mediatek nor flash node.
>>>
>>> Signed-off-by: Guochun Mao <guochun.mao@mediatek.com>
>>> ---
>>>  arch/arm/boot/dts/mt2701-evb.dts |   25 +++++++++++++++++++++++++
>>>  arch/arm/boot/dts/mt2701.dtsi    |   12 ++++++++++++
>>>  2 files changed, 37 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/mt2701-evb.dts
>>> b/arch/arm/boot/dts/mt2701-evb.dts
>>> index 082ca88..85e5ae8 100644
>>> --- a/arch/arm/boot/dts/mt2701-evb.dts
>>> +++ b/arch/arm/boot/dts/mt2701-evb.dts
>>> @@ -24,6 +24,31 @@
>>>      };
>>>  };
>>>
>>> +&nor_flash {
>>> +    pinctrl-names = "default";
>>> +    pinctrl-0 = <&nor_pins_default>;
>>> +    status = "okay";
>>> +    flash@0 {
>>> +        compatible = "jedec,spi-nor";
>>> +        reg = <0>;
>>> +    };
>>> +};
>>> +
>>> +&pio {
>>> +    nor_pins_default: nor {
>>> +        pins1 {
>>> +            pinmux = <MT2701_PIN_240_EXT_XCS__FUNC_EXT_XCS>,
>>> +                 <MT2701_PIN_241_EXT_SCK__FUNC_EXT_SCK>,
>>> +                 <MT2701_PIN_239_EXT_SDIO0__FUNC_EXT_SDIO0>,
>>> +                 <MT2701_PIN_238_EXT_SDIO1__FUNC_EXT_SDIO1>,
>>> +                 <MT2701_PIN_237_EXT_SDIO2__FUNC_EXT_SDIO2>,
>>> +                 <MT2701_PIN_236_EXT_SDIO3__FUNC_EXT_SDIO3>;
>>> +            drive-strength = <MTK_DRIVE_4mA>;
>>> +            bias-pull-up;
>>> +        };
>>> +    };
>>> +};
>>> +
>>>  &uart0 {
>>>      status = "okay";
>>>  };
>>> diff --git a/arch/arm/boot/dts/mt2701.dtsi
>>> b/arch/arm/boot/dts/mt2701.dtsi
>>> index bdf8954..1eefce4 100644
>>> --- a/arch/arm/boot/dts/mt2701.dtsi
>>> +++ b/arch/arm/boot/dts/mt2701.dtsi
>>> @@ -227,6 +227,18 @@
>>>          status = "disabled";
>>>      };
>>>
>>> +    nor_flash: spi@11014000 {
>>> +        compatible = "mediatek,mt2701-nor",
>>> +                 "mediatek,mt8173-nor";
>>
>> Why define both here? Is "mediatek,mt8173-nor" really providing a
>> subset of the features supported by "mediatek,mt2701-nor"?
>>
> 
> I think even if the ip block is the same, we should provide both
> bindings, just in case in the future we find out that mt2701 has some
> hidden bug, feature or bug-feature. This way even if we update the
> driver, we stay compatible with older device tree blobs in the wild.
> 
> We can drop the mt2701-nor in the bindings definition if you want.

This exactly. We should have a DT compat in the form:
compatible = "vendor,<soc>-block", "vendor,<oldest-compat-soc>-block";
Then if we find a problem in the future, we can match on the
"vendor,<soc>-block" and still support the old DTs.

The question is, does the "vendor,<soc>-block" go into the binding
document as well or do we only have "vendor,<oldest-compat-soc>-block"
there ?

-- 
Best regards,
Marek Vasut

[toc] | [prev] | [next] | [standalone]

#1558557

FromBoris Brezillon <boris.brezillon@free-electrons.com>
Date2017-01-13 17:30 +0100
Message-ID<sZcTM-351-23@gated-at.bofh.it>
In reply to#1558550
On Fri, 13 Jan 2017 17:13:55 +0100
Marek Vasut <marek.vasut@gmail.com> wrote:

> On 01/13/2017 04:12 PM, Matthias Brugger wrote:
> > 
> > 
> > On 13/01/17 15:17, Boris Brezillon wrote:  
> >> On Fri, 13 Jan 2017 15:13:29 +0800
> >> Guochun Mao <guochun.mao@mediatek.com> wrote:
> >>  
> >>> Add Mediatek nor flash node.
> >>>
> >>> Signed-off-by: Guochun Mao <guochun.mao@mediatek.com>
> >>> ---
> >>>  arch/arm/boot/dts/mt2701-evb.dts |   25 +++++++++++++++++++++++++
> >>>  arch/arm/boot/dts/mt2701.dtsi    |   12 ++++++++++++
> >>>  2 files changed, 37 insertions(+)
> >>>
> >>> diff --git a/arch/arm/boot/dts/mt2701-evb.dts
> >>> b/arch/arm/boot/dts/mt2701-evb.dts
> >>> index 082ca88..85e5ae8 100644
> >>> --- a/arch/arm/boot/dts/mt2701-evb.dts
> >>> +++ b/arch/arm/boot/dts/mt2701-evb.dts
> >>> @@ -24,6 +24,31 @@
> >>>      };
> >>>  };
> >>>
> >>> +&nor_flash {
> >>> +    pinctrl-names = "default";
> >>> +    pinctrl-0 = <&nor_pins_default>;
> >>> +    status = "okay";
> >>> +    flash@0 {
> >>> +        compatible = "jedec,spi-nor";
> >>> +        reg = <0>;
> >>> +    };
> >>> +};
> >>> +
> >>> +&pio {
> >>> +    nor_pins_default: nor {
> >>> +        pins1 {
> >>> +            pinmux = <MT2701_PIN_240_EXT_XCS__FUNC_EXT_XCS>,
> >>> +                 <MT2701_PIN_241_EXT_SCK__FUNC_EXT_SCK>,
> >>> +                 <MT2701_PIN_239_EXT_SDIO0__FUNC_EXT_SDIO0>,
> >>> +                 <MT2701_PIN_238_EXT_SDIO1__FUNC_EXT_SDIO1>,
> >>> +                 <MT2701_PIN_237_EXT_SDIO2__FUNC_EXT_SDIO2>,
> >>> +                 <MT2701_PIN_236_EXT_SDIO3__FUNC_EXT_SDIO3>;
> >>> +            drive-strength = <MTK_DRIVE_4mA>;
> >>> +            bias-pull-up;
> >>> +        };
> >>> +    };
> >>> +};
> >>> +
> >>>  &uart0 {
> >>>      status = "okay";
> >>>  };
> >>> diff --git a/arch/arm/boot/dts/mt2701.dtsi
> >>> b/arch/arm/boot/dts/mt2701.dtsi
> >>> index bdf8954..1eefce4 100644
> >>> --- a/arch/arm/boot/dts/mt2701.dtsi
> >>> +++ b/arch/arm/boot/dts/mt2701.dtsi
> >>> @@ -227,6 +227,18 @@
> >>>          status = "disabled";
> >>>      };
> >>>
> >>> +    nor_flash: spi@11014000 {
> >>> +        compatible = "mediatek,mt2701-nor",
> >>> +                 "mediatek,mt8173-nor";  
> >>
> >> Why define both here? Is "mediatek,mt8173-nor" really providing a
> >> subset of the features supported by "mediatek,mt2701-nor"?
> >>  
> > 
> > I think even if the ip block is the same, we should provide both
> > bindings, just in case in the future we find out that mt2701 has some
> > hidden bug, feature or bug-feature. This way even if we update the
> > driver, we stay compatible with older device tree blobs in the wild.
> > 
> > We can drop the mt2701-nor in the bindings definition if you want. 

Oh, sorry, I misunderstood. What I meant is that if you want to
list/support all possible compatibles, maybe you should just put one
compatible in your DT and patch your driver (+ binding doc) to define
all of them.

> 
> This exactly. We should have a DT compat in the form:
> compatible = "vendor,<soc>-block", "vendor,<oldest-compat-soc>-block";
> Then if we find a problem in the future, we can match on the
> "vendor,<soc>-block" and still support the old DTs.

Not sure it's only in term of whose IP appeared first. My understanding
is that it's a way to provide inheritance. For example:

	"<soc-vendor>,<ip-version>", "<ip-vendor>,<ip-version>";

or

	"<soc-vendor>,<full-featured-ip-version>","<soc-vendor>,<basic-feature-ip-version>";

BTW, which one is the oldest between mt8173 and mt2701? :-)

[toc] | [prev] | [next] | [standalone]

#1558574

FromMarek Vasut <marek.vasut@gmail.com>
Date2017-01-13 17:50 +0100
Message-ID<sZdd9-3bn-25@gated-at.bofh.it>
In reply to#1558557
On 01/13/2017 05:28 PM, Boris Brezillon wrote:
> On Fri, 13 Jan 2017 17:13:55 +0100
> Marek Vasut <marek.vasut@gmail.com> wrote:
> 
>> On 01/13/2017 04:12 PM, Matthias Brugger wrote:
>>>
>>>
>>> On 13/01/17 15:17, Boris Brezillon wrote:  
>>>> On Fri, 13 Jan 2017 15:13:29 +0800
>>>> Guochun Mao <guochun.mao@mediatek.com> wrote:
>>>>  
>>>>> Add Mediatek nor flash node.
>>>>>
>>>>> Signed-off-by: Guochun Mao <guochun.mao@mediatek.com>
>>>>> ---
>>>>>  arch/arm/boot/dts/mt2701-evb.dts |   25 +++++++++++++++++++++++++
>>>>>  arch/arm/boot/dts/mt2701.dtsi    |   12 ++++++++++++
>>>>>  2 files changed, 37 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/mt2701-evb.dts
>>>>> b/arch/arm/boot/dts/mt2701-evb.dts
>>>>> index 082ca88..85e5ae8 100644
>>>>> --- a/arch/arm/boot/dts/mt2701-evb.dts
>>>>> +++ b/arch/arm/boot/dts/mt2701-evb.dts
>>>>> @@ -24,6 +24,31 @@
>>>>>      };
>>>>>  };
>>>>>
>>>>> +&nor_flash {
>>>>> +    pinctrl-names = "default";
>>>>> +    pinctrl-0 = <&nor_pins_default>;
>>>>> +    status = "okay";
>>>>> +    flash@0 {
>>>>> +        compatible = "jedec,spi-nor";
>>>>> +        reg = <0>;
>>>>> +    };
>>>>> +};
>>>>> +
>>>>> +&pio {
>>>>> +    nor_pins_default: nor {
>>>>> +        pins1 {
>>>>> +            pinmux = <MT2701_PIN_240_EXT_XCS__FUNC_EXT_XCS>,
>>>>> +                 <MT2701_PIN_241_EXT_SCK__FUNC_EXT_SCK>,
>>>>> +                 <MT2701_PIN_239_EXT_SDIO0__FUNC_EXT_SDIO0>,
>>>>> +                 <MT2701_PIN_238_EXT_SDIO1__FUNC_EXT_SDIO1>,
>>>>> +                 <MT2701_PIN_237_EXT_SDIO2__FUNC_EXT_SDIO2>,
>>>>> +                 <MT2701_PIN_236_EXT_SDIO3__FUNC_EXT_SDIO3>;
>>>>> +            drive-strength = <MTK_DRIVE_4mA>;
>>>>> +            bias-pull-up;
>>>>> +        };
>>>>> +    };
>>>>> +};
>>>>> +
>>>>>  &uart0 {
>>>>>      status = "okay";
>>>>>  };
>>>>> diff --git a/arch/arm/boot/dts/mt2701.dtsi
>>>>> b/arch/arm/boot/dts/mt2701.dtsi
>>>>> index bdf8954..1eefce4 100644
>>>>> --- a/arch/arm/boot/dts/mt2701.dtsi
>>>>> +++ b/arch/arm/boot/dts/mt2701.dtsi
>>>>> @@ -227,6 +227,18 @@
>>>>>          status = "disabled";
>>>>>      };
>>>>>
>>>>> +    nor_flash: spi@11014000 {
>>>>> +        compatible = "mediatek,mt2701-nor",
>>>>> +                 "mediatek,mt8173-nor";  
>>>>
>>>> Why define both here? Is "mediatek,mt8173-nor" really providing a
>>>> subset of the features supported by "mediatek,mt2701-nor"?
>>>>  
>>>
>>> I think even if the ip block is the same, we should provide both
>>> bindings, just in case in the future we find out that mt2701 has some
>>> hidden bug, feature or bug-feature. This way even if we update the
>>> driver, we stay compatible with older device tree blobs in the wild.
>>>
>>> We can drop the mt2701-nor in the bindings definition if you want. 
> 
> Oh, sorry, I misunderstood. What I meant is that if you want to
> list/support all possible compatibles, maybe you should just put one
> compatible in your DT and patch your driver (+ binding doc) to define
> all of them.

Uh, what ? I lost you here :-)

>> This exactly. We should have a DT compat in the form:
>> compatible = "vendor,<soc>-block", "vendor,<oldest-compat-soc>-block";
>> Then if we find a problem in the future, we can match on the
>> "vendor,<soc>-block" and still support the old DTs.
> 
> Not sure it's only in term of whose IP appeared first. My understanding
> is that it's a way to provide inheritance. For example:
> 
> 	"<soc-vendor>,<ip-version>", "<ip-vendor>,<ip-version>";
> 
> or
> 
> 	"<soc-vendor>,<full-featured-ip-version>","<soc-vendor>,<basic-feature-ip-version>";
> 
> BTW, which one is the oldest between mt8173 and mt2701? :-)

And that's another thing and I agree with you, but I don't think that's
what we're discussing in this thread. But (!), OT, I think we should
codify the rules in Documentation/ . This discussion came up multiple
times recently.

And my question still stands, what do we put into the DT here, IMO
compatible = "mediatek,mt2701-nor", "mediatek,mt8173-nor";
and what goes into the binding document ? I guess both too ?
-- 
Best regards,
Marek Vasut

[toc] | [prev] | [next] | [standalone]

#1558589

FromBoris Brezillon <boris.brezillon@free-electrons.com>
Date2017-01-13 18:00 +0100
Message-ID<sZdmN-3eM-19@gated-at.bofh.it>
In reply to#1558574
On Fri, 13 Jan 2017 17:44:12 +0100
Marek Vasut <marek.vasut@gmail.com> wrote:

> On 01/13/2017 05:28 PM, Boris Brezillon wrote:
> > On Fri, 13 Jan 2017 17:13:55 +0100
> > Marek Vasut <marek.vasut@gmail.com> wrote:
> >   
> >> On 01/13/2017 04:12 PM, Matthias Brugger wrote:  
> >>>
> >>>
> >>> On 13/01/17 15:17, Boris Brezillon wrote:    
> >>>> On Fri, 13 Jan 2017 15:13:29 +0800
> >>>> Guochun Mao <guochun.mao@mediatek.com> wrote:
> >>>>    
> >>>>> Add Mediatek nor flash node.
> >>>>>
> >>>>> Signed-off-by: Guochun Mao <guochun.mao@mediatek.com>
> >>>>> ---
> >>>>>  arch/arm/boot/dts/mt2701-evb.dts |   25 +++++++++++++++++++++++++
> >>>>>  arch/arm/boot/dts/mt2701.dtsi    |   12 ++++++++++++
> >>>>>  2 files changed, 37 insertions(+)
> >>>>>
> >>>>> diff --git a/arch/arm/boot/dts/mt2701-evb.dts
> >>>>> b/arch/arm/boot/dts/mt2701-evb.dts
> >>>>> index 082ca88..85e5ae8 100644
> >>>>> --- a/arch/arm/boot/dts/mt2701-evb.dts
> >>>>> +++ b/arch/arm/boot/dts/mt2701-evb.dts
> >>>>> @@ -24,6 +24,31 @@
> >>>>>      };
> >>>>>  };
> >>>>>
> >>>>> +&nor_flash {
> >>>>> +    pinctrl-names = "default";
> >>>>> +    pinctrl-0 = <&nor_pins_default>;
> >>>>> +    status = "okay";
> >>>>> +    flash@0 {
> >>>>> +        compatible = "jedec,spi-nor";
> >>>>> +        reg = <0>;
> >>>>> +    };
> >>>>> +};
> >>>>> +
> >>>>> +&pio {
> >>>>> +    nor_pins_default: nor {
> >>>>> +        pins1 {
> >>>>> +            pinmux = <MT2701_PIN_240_EXT_XCS__FUNC_EXT_XCS>,
> >>>>> +                 <MT2701_PIN_241_EXT_SCK__FUNC_EXT_SCK>,
> >>>>> +                 <MT2701_PIN_239_EXT_SDIO0__FUNC_EXT_SDIO0>,
> >>>>> +                 <MT2701_PIN_238_EXT_SDIO1__FUNC_EXT_SDIO1>,
> >>>>> +                 <MT2701_PIN_237_EXT_SDIO2__FUNC_EXT_SDIO2>,
> >>>>> +                 <MT2701_PIN_236_EXT_SDIO3__FUNC_EXT_SDIO3>;
> >>>>> +            drive-strength = <MTK_DRIVE_4mA>;
> >>>>> +            bias-pull-up;
> >>>>> +        };
> >>>>> +    };
> >>>>> +};
> >>>>> +
> >>>>>  &uart0 {
> >>>>>      status = "okay";
> >>>>>  };
> >>>>> diff --git a/arch/arm/boot/dts/mt2701.dtsi
> >>>>> b/arch/arm/boot/dts/mt2701.dtsi
> >>>>> index bdf8954..1eefce4 100644
> >>>>> --- a/arch/arm/boot/dts/mt2701.dtsi
> >>>>> +++ b/arch/arm/boot/dts/mt2701.dtsi
> >>>>> @@ -227,6 +227,18 @@
> >>>>>          status = "disabled";
> >>>>>      };
> >>>>>
> >>>>> +    nor_flash: spi@11014000 {
> >>>>> +        compatible = "mediatek,mt2701-nor",
> >>>>> +                 "mediatek,mt8173-nor";    
> >>>>
> >>>> Why define both here? Is "mediatek,mt8173-nor" really providing a
> >>>> subset of the features supported by "mediatek,mt2701-nor"?
> >>>>    
> >>>
> >>> I think even if the ip block is the same, we should provide both
> >>> bindings, just in case in the future we find out that mt2701 has some
> >>> hidden bug, feature or bug-feature. This way even if we update the
> >>> driver, we stay compatible with older device tree blobs in the wild.
> >>>
> >>> We can drop the mt2701-nor in the bindings definition if you want.   
> > 
> > Oh, sorry, I misunderstood. What I meant is that if you want to
> > list/support all possible compatibles, maybe you should just put one
> > compatible in your DT and patch your driver (+ binding doc) to define
> > all of them.  
> 
> Uh, what ? I lost you here :-)
> 
> >> This exactly. We should have a DT compat in the form:
> >> compatible = "vendor,<soc>-block", "vendor,<oldest-compat-soc>-block";
> >> Then if we find a problem in the future, we can match on the
> >> "vendor,<soc>-block" and still support the old DTs.  
> > 
> > Not sure it's only in term of whose IP appeared first. My understanding
> > is that it's a way to provide inheritance. For example:
> > 
> > 	"<soc-vendor>,<ip-version>", "<ip-vendor>,<ip-version>";
> > 
> > or
> > 
> > 	"<soc-vendor>,<full-featured-ip-version>","<soc-vendor>,<basic-feature-ip-version>";
> > 
> > BTW, which one is the oldest between mt8173 and mt2701? :-)  
> 
> And that's another thing and I agree with you, but I don't think that's
> what we're discussing in this thread. But (!), OT, I think we should
> codify the rules in Documentation/ . This discussion came up multiple
> times recently.
> 
> And my question still stands, what do we put into the DT here, IMO
> compatible = "mediatek,mt2701-nor", "mediatek,mt8173-nor";

I'd say

	compatible = "mediatek,mt8173-nor";

because both compatible are referring to very specific IP version. It's
not the same as

	compatible = "mediatek,mt8173-nor", "mediatek,mt81xx-nor";

where you clearly have a generic compatible which is overloaded by a
specific one.

But anyway, I'm not the one taking the decision here, let's wait for DT
maintainers reviews.

> and what goes into the binding document ? I guess both too ?

If both exist, they should be both documented.

[toc] | [prev] | [next] | [standalone]

#1558633

FromMarek Vasut <marek.vasut@gmail.com>
Date2017-01-13 18:40 +0100
Message-ID<sZdZv-3GV-15@gated-at.bofh.it>
In reply to#1558589
On 01/13/2017 05:56 PM, Boris Brezillon wrote:
> On Fri, 13 Jan 2017 17:44:12 +0100
> Marek Vasut <marek.vasut@gmail.com> wrote:
> 
>> On 01/13/2017 05:28 PM, Boris Brezillon wrote:
>>> On Fri, 13 Jan 2017 17:13:55 +0100
>>> Marek Vasut <marek.vasut@gmail.com> wrote:
>>>   
>>>> On 01/13/2017 04:12 PM, Matthias Brugger wrote:  
>>>>>
>>>>>
>>>>> On 13/01/17 15:17, Boris Brezillon wrote:    
>>>>>> On Fri, 13 Jan 2017 15:13:29 +0800
>>>>>> Guochun Mao <guochun.mao@mediatek.com> wrote:
>>>>>>    
>>>>>>> Add Mediatek nor flash node.
>>>>>>>
>>>>>>> Signed-off-by: Guochun Mao <guochun.mao@mediatek.com>
>>>>>>> ---
>>>>>>>  arch/arm/boot/dts/mt2701-evb.dts |   25 +++++++++++++++++++++++++
>>>>>>>  arch/arm/boot/dts/mt2701.dtsi    |   12 ++++++++++++
>>>>>>>  2 files changed, 37 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm/boot/dts/mt2701-evb.dts
>>>>>>> b/arch/arm/boot/dts/mt2701-evb.dts
>>>>>>> index 082ca88..85e5ae8 100644
>>>>>>> --- a/arch/arm/boot/dts/mt2701-evb.dts
>>>>>>> +++ b/arch/arm/boot/dts/mt2701-evb.dts
>>>>>>> @@ -24,6 +24,31 @@
>>>>>>>      };
>>>>>>>  };
>>>>>>>
>>>>>>> +&nor_flash {
>>>>>>> +    pinctrl-names = "default";
>>>>>>> +    pinctrl-0 = <&nor_pins_default>;
>>>>>>> +    status = "okay";
>>>>>>> +    flash@0 {
>>>>>>> +        compatible = "jedec,spi-nor";
>>>>>>> +        reg = <0>;
>>>>>>> +    };
>>>>>>> +};
>>>>>>> +
>>>>>>> +&pio {
>>>>>>> +    nor_pins_default: nor {
>>>>>>> +        pins1 {
>>>>>>> +            pinmux = <MT2701_PIN_240_EXT_XCS__FUNC_EXT_XCS>,
>>>>>>> +                 <MT2701_PIN_241_EXT_SCK__FUNC_EXT_SCK>,
>>>>>>> +                 <MT2701_PIN_239_EXT_SDIO0__FUNC_EXT_SDIO0>,
>>>>>>> +                 <MT2701_PIN_238_EXT_SDIO1__FUNC_EXT_SDIO1>,
>>>>>>> +                 <MT2701_PIN_237_EXT_SDIO2__FUNC_EXT_SDIO2>,
>>>>>>> +                 <MT2701_PIN_236_EXT_SDIO3__FUNC_EXT_SDIO3>;
>>>>>>> +            drive-strength = <MTK_DRIVE_4mA>;
>>>>>>> +            bias-pull-up;
>>>>>>> +        };
>>>>>>> +    };
>>>>>>> +};
>>>>>>> +
>>>>>>>  &uart0 {
>>>>>>>      status = "okay";
>>>>>>>  };
>>>>>>> diff --git a/arch/arm/boot/dts/mt2701.dtsi
>>>>>>> b/arch/arm/boot/dts/mt2701.dtsi
>>>>>>> index bdf8954..1eefce4 100644
>>>>>>> --- a/arch/arm/boot/dts/mt2701.dtsi
>>>>>>> +++ b/arch/arm/boot/dts/mt2701.dtsi
>>>>>>> @@ -227,6 +227,18 @@
>>>>>>>          status = "disabled";
>>>>>>>      };
>>>>>>>
>>>>>>> +    nor_flash: spi@11014000 {
>>>>>>> +        compatible = "mediatek,mt2701-nor",
>>>>>>> +                 "mediatek,mt8173-nor";    
>>>>>>
>>>>>> Why define both here? Is "mediatek,mt8173-nor" really providing a
>>>>>> subset of the features supported by "mediatek,mt2701-nor"?
>>>>>>    
>>>>>
>>>>> I think even if the ip block is the same, we should provide both
>>>>> bindings, just in case in the future we find out that mt2701 has some
>>>>> hidden bug, feature or bug-feature. This way even if we update the
>>>>> driver, we stay compatible with older device tree blobs in the wild.
>>>>>
>>>>> We can drop the mt2701-nor in the bindings definition if you want.   
>>>
>>> Oh, sorry, I misunderstood. What I meant is that if you want to
>>> list/support all possible compatibles, maybe you should just put one
>>> compatible in your DT and patch your driver (+ binding doc) to define
>>> all of them.  
>>
>> Uh, what ? I lost you here :-)
>>
>>>> This exactly. We should have a DT compat in the form:
>>>> compatible = "vendor,<soc>-block", "vendor,<oldest-compat-soc>-block";
>>>> Then if we find a problem in the future, we can match on the
>>>> "vendor,<soc>-block" and still support the old DTs.  
>>>
>>> Not sure it's only in term of whose IP appeared first. My understanding
>>> is that it's a way to provide inheritance. For example:
>>>
>>> 	"<soc-vendor>,<ip-version>", "<ip-vendor>,<ip-version>";
>>>
>>> or
>>>
>>> 	"<soc-vendor>,<full-featured-ip-version>","<soc-vendor>,<basic-feature-ip-version>";
>>>
>>> BTW, which one is the oldest between mt8173 and mt2701? :-)  
>>
>> And that's another thing and I agree with you, but I don't think that's
>> what we're discussing in this thread. But (!), OT, I think we should
>> codify the rules in Documentation/ . This discussion came up multiple
>> times recently.
>>
>> And my question still stands, what do we put into the DT here, IMO
>> compatible = "mediatek,mt2701-nor", "mediatek,mt8173-nor";
> 
> I'd say
> 
> 	compatible = "mediatek,mt8173-nor";
> 
> because both compatible are referring to very specific IP version. It's
> not the same as

But then you don't have the ability to handle a block in this particular
SoC in case there's a bug found in it in the future,
so IMO it should be:

compatible = "mediatek,mt2701-nor", "mediatek,mt8173-nor";

> 	compatible = "mediatek,mt8173-nor", "mediatek,mt81xx-nor";

This doesn't look right, since here we add two new compatibles ...

> where you clearly have a generic compatible which is overloaded by a
> specific one.
> 
> But anyway, I'm not the one taking the decision here, let's wait for DT
> maintainers reviews.
> 
>> and what goes into the binding document ? I guess both too ?
> 
> If both exist, they should be both documented.
> 


-- 
Best regards,
Marek Vasut

[toc] | [prev] | [next] | [standalone]

#1558867

FromBoris Brezillon <boris.brezillon@free-electrons.com>
Date2017-01-14 09:40 +0100
Message-ID<sZs2t-3He-1@gated-at.bofh.it>
In reply to#1558633
On Fri, 13 Jan 2017 18:33:40 +0100
Marek Vasut <marek.vasut@gmail.com> wrote:

> On 01/13/2017 05:56 PM, Boris Brezillon wrote:
> > On Fri, 13 Jan 2017 17:44:12 +0100
> > Marek Vasut <marek.vasut@gmail.com> wrote:
> >   
> >> On 01/13/2017 05:28 PM, Boris Brezillon wrote:  
> >>> On Fri, 13 Jan 2017 17:13:55 +0100
> >>> Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>     
> >>>> On 01/13/2017 04:12 PM, Matthias Brugger wrote:    
> >>>>>
> >>>>>
> >>>>> On 13/01/17 15:17, Boris Brezillon wrote:      
> >>>>>> On Fri, 13 Jan 2017 15:13:29 +0800
> >>>>>> Guochun Mao <guochun.mao@mediatek.com> wrote:
> >>>>>>      
> >>>>>>> Add Mediatek nor flash node.
> >>>>>>>
> >>>>>>> Signed-off-by: Guochun Mao <guochun.mao@mediatek.com>
> >>>>>>> ---
> >>>>>>>  arch/arm/boot/dts/mt2701-evb.dts |   25 +++++++++++++++++++++++++
> >>>>>>>  arch/arm/boot/dts/mt2701.dtsi    |   12 ++++++++++++
> >>>>>>>  2 files changed, 37 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/arch/arm/boot/dts/mt2701-evb.dts
> >>>>>>> b/arch/arm/boot/dts/mt2701-evb.dts
> >>>>>>> index 082ca88..85e5ae8 100644
> >>>>>>> --- a/arch/arm/boot/dts/mt2701-evb.dts
> >>>>>>> +++ b/arch/arm/boot/dts/mt2701-evb.dts
> >>>>>>> @@ -24,6 +24,31 @@
> >>>>>>>      };
> >>>>>>>  };
> >>>>>>>
> >>>>>>> +&nor_flash {
> >>>>>>> +    pinctrl-names = "default";
> >>>>>>> +    pinctrl-0 = <&nor_pins_default>;
> >>>>>>> +    status = "okay";
> >>>>>>> +    flash@0 {
> >>>>>>> +        compatible = "jedec,spi-nor";
> >>>>>>> +        reg = <0>;
> >>>>>>> +    };
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> +&pio {
> >>>>>>> +    nor_pins_default: nor {
> >>>>>>> +        pins1 {
> >>>>>>> +            pinmux = <MT2701_PIN_240_EXT_XCS__FUNC_EXT_XCS>,
> >>>>>>> +                 <MT2701_PIN_241_EXT_SCK__FUNC_EXT_SCK>,
> >>>>>>> +                 <MT2701_PIN_239_EXT_SDIO0__FUNC_EXT_SDIO0>,
> >>>>>>> +                 <MT2701_PIN_238_EXT_SDIO1__FUNC_EXT_SDIO1>,
> >>>>>>> +                 <MT2701_PIN_237_EXT_SDIO2__FUNC_EXT_SDIO2>,
> >>>>>>> +                 <MT2701_PIN_236_EXT_SDIO3__FUNC_EXT_SDIO3>;
> >>>>>>> +            drive-strength = <MTK_DRIVE_4mA>;
> >>>>>>> +            bias-pull-up;
> >>>>>>> +        };
> >>>>>>> +    };
> >>>>>>> +};
> >>>>>>> +
> >>>>>>>  &uart0 {
> >>>>>>>      status = "okay";
> >>>>>>>  };
> >>>>>>> diff --git a/arch/arm/boot/dts/mt2701.dtsi
> >>>>>>> b/arch/arm/boot/dts/mt2701.dtsi
> >>>>>>> index bdf8954..1eefce4 100644
> >>>>>>> --- a/arch/arm/boot/dts/mt2701.dtsi
> >>>>>>> +++ b/arch/arm/boot/dts/mt2701.dtsi
> >>>>>>> @@ -227,6 +227,18 @@
> >>>>>>>          status = "disabled";
> >>>>>>>      };
> >>>>>>>
> >>>>>>> +    nor_flash: spi@11014000 {
> >>>>>>> +        compatible = "mediatek,mt2701-nor",
> >>>>>>> +                 "mediatek,mt8173-nor";      
> >>>>>>
> >>>>>> Why define both here? Is "mediatek,mt8173-nor" really providing a
> >>>>>> subset of the features supported by "mediatek,mt2701-nor"?
> >>>>>>      
> >>>>>
> >>>>> I think even if the ip block is the same, we should provide both
> >>>>> bindings, just in case in the future we find out that mt2701 has some
> >>>>> hidden bug, feature or bug-feature. This way even if we update the
> >>>>> driver, we stay compatible with older device tree blobs in the wild.
> >>>>>
> >>>>> We can drop the mt2701-nor in the bindings definition if you want.     
> >>>
> >>> Oh, sorry, I misunderstood. What I meant is that if you want to
> >>> list/support all possible compatibles, maybe you should just put one
> >>> compatible in your DT and patch your driver (+ binding doc) to define
> >>> all of them.    
> >>
> >> Uh, what ? I lost you here :-)

I mean adding a new entry in the mtk_nor_of_ids table (in
mtk-quadspi.c) so that the mediatek,mt2701-nor compatible string can be
matched directly, and you won't need to define 2 compatible strings in
your device tree.

> >>  
> >>>> This exactly. We should have a DT compat in the form:
> >>>> compatible = "vendor,<soc>-block", "vendor,<oldest-compat-soc>-block";
> >>>> Then if we find a problem in the future, we can match on the
> >>>> "vendor,<soc>-block" and still support the old DTs.    
> >>>
> >>> Not sure it's only in term of whose IP appeared first. My understanding
> >>> is that it's a way to provide inheritance. For example:
> >>>
> >>> 	"<soc-vendor>,<ip-version>", "<ip-vendor>,<ip-version>";
> >>>
> >>> or
> >>>
> >>> 	"<soc-vendor>,<full-featured-ip-version>","<soc-vendor>,<basic-feature-ip-version>";
> >>>
> >>> BTW, which one is the oldest between mt8173 and mt2701? :-)    
> >>
> >> And that's another thing and I agree with you, but I don't think that's
> >> what we're discussing in this thread. But (!), OT, I think we should
> >> codify the rules in Documentation/ . This discussion came up multiple
> >> times recently.
> >>
> >> And my question still stands, what do we put into the DT here, IMO
> >> compatible = "mediatek,mt2701-nor", "mediatek,mt8173-nor";  
> > 
> > I'd say
> > 
> > 	compatible = "mediatek,mt8173-nor";
> > 
> > because both compatible are referring to very specific IP version. It's
> > not the same as  
> 
> But then you don't have the ability to handle a block in this particular
> SoC in case there's a bug found in it in the future,
> so IMO it should be:
> 
> compatible = "mediatek,mt2701-nor", "mediatek,mt8173-nor";

Sorry again, I meant

	compatible = "mediatek,mt2701-nor";

> 
> > 	compatible = "mediatek,mt8173-nor", "mediatek,mt81xx-nor";  
> 
> This doesn't look right, since here we add two new compatibles ...

That was just an example to describe how compatible inheritance works
(at least that's my understanding of it), it does not apply to this
particular use case.

> 
> > where you clearly have a generic compatible which is overloaded by a
> > specific one.
> > 
> > But anyway, I'm not the one taking the decision here, let's wait for DT
> > maintainers reviews.
> >   
> >> and what goes into the binding document ? I guess both too ?  
> > 
> > If both exist, they should be both documented.
> >   
> 
>

[toc] | [prev] | [next] | [standalone]

#1559121

FromMarek Vasut <marek.vasut@gmail.com>
Date2017-01-15 01:30 +0100
Message-ID<sZGRP-4bL-3@gated-at.bofh.it>
In reply to#1558867
On 01/14/2017 09:29 AM, Boris Brezillon wrote:
> On Fri, 13 Jan 2017 18:33:40 +0100
> Marek Vasut <marek.vasut@gmail.com> wrote:
> 
>> On 01/13/2017 05:56 PM, Boris Brezillon wrote:
>>> On Fri, 13 Jan 2017 17:44:12 +0100
>>> Marek Vasut <marek.vasut@gmail.com> wrote:
>>>   
>>>> On 01/13/2017 05:28 PM, Boris Brezillon wrote:  
>>>>> On Fri, 13 Jan 2017 17:13:55 +0100
>>>>> Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>     
>>>>>> On 01/13/2017 04:12 PM, Matthias Brugger wrote:    
>>>>>>>
>>>>>>>
>>>>>>> On 13/01/17 15:17, Boris Brezillon wrote:      
>>>>>>>> On Fri, 13 Jan 2017 15:13:29 +0800
>>>>>>>> Guochun Mao <guochun.mao@mediatek.com> wrote:
>>>>>>>>      
>>>>>>>>> Add Mediatek nor flash node.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Guochun Mao <guochun.mao@mediatek.com>
>>>>>>>>> ---
>>>>>>>>>  arch/arm/boot/dts/mt2701-evb.dts |   25 +++++++++++++++++++++++++
>>>>>>>>>  arch/arm/boot/dts/mt2701.dtsi    |   12 ++++++++++++
>>>>>>>>>  2 files changed, 37 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/arm/boot/dts/mt2701-evb.dts
>>>>>>>>> b/arch/arm/boot/dts/mt2701-evb.dts
>>>>>>>>> index 082ca88..85e5ae8 100644
>>>>>>>>> --- a/arch/arm/boot/dts/mt2701-evb.dts
>>>>>>>>> +++ b/arch/arm/boot/dts/mt2701-evb.dts
>>>>>>>>> @@ -24,6 +24,31 @@
>>>>>>>>>      };
>>>>>>>>>  };
>>>>>>>>>
>>>>>>>>> +&nor_flash {
>>>>>>>>> +    pinctrl-names = "default";
>>>>>>>>> +    pinctrl-0 = <&nor_pins_default>;
>>>>>>>>> +    status = "okay";
>>>>>>>>> +    flash@0 {
>>>>>>>>> +        compatible = "jedec,spi-nor";
>>>>>>>>> +        reg = <0>;
>>>>>>>>> +    };
>>>>>>>>> +};
>>>>>>>>> +
>>>>>>>>> +&pio {
>>>>>>>>> +    nor_pins_default: nor {
>>>>>>>>> +        pins1 {
>>>>>>>>> +            pinmux = <MT2701_PIN_240_EXT_XCS__FUNC_EXT_XCS>,
>>>>>>>>> +                 <MT2701_PIN_241_EXT_SCK__FUNC_EXT_SCK>,
>>>>>>>>> +                 <MT2701_PIN_239_EXT_SDIO0__FUNC_EXT_SDIO0>,
>>>>>>>>> +                 <MT2701_PIN_238_EXT_SDIO1__FUNC_EXT_SDIO1>,
>>>>>>>>> +                 <MT2701_PIN_237_EXT_SDIO2__FUNC_EXT_SDIO2>,
>>>>>>>>> +                 <MT2701_PIN_236_EXT_SDIO3__FUNC_EXT_SDIO3>;
>>>>>>>>> +            drive-strength = <MTK_DRIVE_4mA>;
>>>>>>>>> +            bias-pull-up;
>>>>>>>>> +        };
>>>>>>>>> +    };
>>>>>>>>> +};
>>>>>>>>> +
>>>>>>>>>  &uart0 {
>>>>>>>>>      status = "okay";
>>>>>>>>>  };
>>>>>>>>> diff --git a/arch/arm/boot/dts/mt2701.dtsi
>>>>>>>>> b/arch/arm/boot/dts/mt2701.dtsi
>>>>>>>>> index bdf8954..1eefce4 100644
>>>>>>>>> --- a/arch/arm/boot/dts/mt2701.dtsi
>>>>>>>>> +++ b/arch/arm/boot/dts/mt2701.dtsi
>>>>>>>>> @@ -227,6 +227,18 @@
>>>>>>>>>          status = "disabled";
>>>>>>>>>      };
>>>>>>>>>
>>>>>>>>> +    nor_flash: spi@11014000 {
>>>>>>>>> +        compatible = "mediatek,mt2701-nor",
>>>>>>>>> +                 "mediatek,mt8173-nor";      
>>>>>>>>
>>>>>>>> Why define both here? Is "mediatek,mt8173-nor" really providing a
>>>>>>>> subset of the features supported by "mediatek,mt2701-nor"?
>>>>>>>>      
>>>>>>>
>>>>>>> I think even if the ip block is the same, we should provide both
>>>>>>> bindings, just in case in the future we find out that mt2701 has some
>>>>>>> hidden bug, feature or bug-feature. This way even if we update the
>>>>>>> driver, we stay compatible with older device tree blobs in the wild.
>>>>>>>
>>>>>>> We can drop the mt2701-nor in the bindings definition if you want.     
>>>>>
>>>>> Oh, sorry, I misunderstood. What I meant is that if you want to
>>>>> list/support all possible compatibles, maybe you should just put one
>>>>> compatible in your DT and patch your driver (+ binding doc) to define
>>>>> all of them.    
>>>>
>>>> Uh, what ? I lost you here :-)
> 
> I mean adding a new entry in the mtk_nor_of_ids table (in
> mtk-quadspi.c) so that the mediatek,mt2701-nor compatible string can be
> matched directly, and you won't need to define 2 compatible strings in
> your device tree.

But then you grow the table in the driver, is that what we want if we
can avoid that ?

>>>>  
>>>>>> This exactly. We should have a DT compat in the form:
>>>>>> compatible = "vendor,<soc>-block", "vendor,<oldest-compat-soc>-block";
>>>>>> Then if we find a problem in the future, we can match on the
>>>>>> "vendor,<soc>-block" and still support the old DTs.    
>>>>>
>>>>> Not sure it's only in term of whose IP appeared first. My understanding
>>>>> is that it's a way to provide inheritance. For example:
>>>>>
>>>>> 	"<soc-vendor>,<ip-version>", "<ip-vendor>,<ip-version>";
>>>>>
>>>>> or
>>>>>
>>>>> 	"<soc-vendor>,<full-featured-ip-version>","<soc-vendor>,<basic-feature-ip-version>";
>>>>>
>>>>> BTW, which one is the oldest between mt8173 and mt2701? :-)    
>>>>
>>>> And that's another thing and I agree with you, but I don't think that's
>>>> what we're discussing in this thread. But (!), OT, I think we should
>>>> codify the rules in Documentation/ . This discussion came up multiple
>>>> times recently.
>>>>
>>>> And my question still stands, what do we put into the DT here, IMO
>>>> compatible = "mediatek,mt2701-nor", "mediatek,mt8173-nor";  
>>>
>>> I'd say
>>>
>>> 	compatible = "mediatek,mt8173-nor";
>>>
>>> because both compatible are referring to very specific IP version. It's
>>> not the same as  
>>
>> But then you don't have the ability to handle a block in this particular
>> SoC in case there's a bug found in it in the future,
>> so IMO it should be:
>>
>> compatible = "mediatek,mt2701-nor", "mediatek,mt8173-nor";
> 
> Sorry again, I meant
> 
> 	compatible = "mediatek,mt2701-nor";
> 
>>
>>> 	compatible = "mediatek,mt8173-nor", "mediatek,mt81xx-nor";  
>>
>> This doesn't look right, since here we add two new compatibles ...
> 
> That was just an example to describe how compatible inheritance works
> (at least that's my understanding of it), it does not apply to this
> particular use case.

Well this is OK I guess, but then you can also use "mediatek,mt8173-nor"
as the oldest supported compatible and be done with it, no ? It looks a
bit crappy though, I admit that ...

-- 
Best regards,
Marek Vasut

[toc] | [prev] | [next] | [standalone]

#1559520

FromBoris Brezillon <boris.brezillon@free-electrons.com>
Date2017-01-16 09:50 +0100
Message-ID<t0b9g-67e-11@gated-at.bofh.it>
In reply to#1559121
On Sun, 15 Jan 2017 01:23:48 +0100
Marek Vasut <marek.vasut@gmail.com> wrote:

> On 01/14/2017 09:29 AM, Boris Brezillon wrote:
> > On Fri, 13 Jan 2017 18:33:40 +0100
> > Marek Vasut <marek.vasut@gmail.com> wrote:
> >   
> >> On 01/13/2017 05:56 PM, Boris Brezillon wrote:  
> >>> On Fri, 13 Jan 2017 17:44:12 +0100
> >>> Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>     
> >>>> On 01/13/2017 05:28 PM, Boris Brezillon wrote:    
> >>>>> On Fri, 13 Jan 2017 17:13:55 +0100
> >>>>> Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>       
> >>>>>> On 01/13/2017 04:12 PM, Matthias Brugger wrote:      
> >>>>>>>
> >>>>>>>
> >>>>>>> On 13/01/17 15:17, Boris Brezillon wrote:        
> >>>>>>>> On Fri, 13 Jan 2017 15:13:29 +0800
> >>>>>>>> Guochun Mao <guochun.mao@mediatek.com> wrote:
> >>>>>>>>        
> >>>>>>>>> Add Mediatek nor flash node.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Guochun Mao <guochun.mao@mediatek.com>
> >>>>>>>>> ---
> >>>>>>>>>  arch/arm/boot/dts/mt2701-evb.dts |   25 +++++++++++++++++++++++++
> >>>>>>>>>  arch/arm/boot/dts/mt2701.dtsi    |   12 ++++++++++++
> >>>>>>>>>  2 files changed, 37 insertions(+)
> >>>>>>>>>
> >>>>>>>>> diff --git a/arch/arm/boot/dts/mt2701-evb.dts
> >>>>>>>>> b/arch/arm/boot/dts/mt2701-evb.dts
> >>>>>>>>> index 082ca88..85e5ae8 100644
> >>>>>>>>> --- a/arch/arm/boot/dts/mt2701-evb.dts
> >>>>>>>>> +++ b/arch/arm/boot/dts/mt2701-evb.dts
> >>>>>>>>> @@ -24,6 +24,31 @@
> >>>>>>>>>      };
> >>>>>>>>>  };
> >>>>>>>>>
> >>>>>>>>> +&nor_flash {
> >>>>>>>>> +    pinctrl-names = "default";
> >>>>>>>>> +    pinctrl-0 = <&nor_pins_default>;
> >>>>>>>>> +    status = "okay";
> >>>>>>>>> +    flash@0 {
> >>>>>>>>> +        compatible = "jedec,spi-nor";
> >>>>>>>>> +        reg = <0>;
> >>>>>>>>> +    };
> >>>>>>>>> +};
> >>>>>>>>> +
> >>>>>>>>> +&pio {
> >>>>>>>>> +    nor_pins_default: nor {
> >>>>>>>>> +        pins1 {
> >>>>>>>>> +            pinmux = <MT2701_PIN_240_EXT_XCS__FUNC_EXT_XCS>,
> >>>>>>>>> +                 <MT2701_PIN_241_EXT_SCK__FUNC_EXT_SCK>,
> >>>>>>>>> +                 <MT2701_PIN_239_EXT_SDIO0__FUNC_EXT_SDIO0>,
> >>>>>>>>> +                 <MT2701_PIN_238_EXT_SDIO1__FUNC_EXT_SDIO1>,
> >>>>>>>>> +                 <MT2701_PIN_237_EXT_SDIO2__FUNC_EXT_SDIO2>,
> >>>>>>>>> +                 <MT2701_PIN_236_EXT_SDIO3__FUNC_EXT_SDIO3>;
> >>>>>>>>> +            drive-strength = <MTK_DRIVE_4mA>;
> >>>>>>>>> +            bias-pull-up;
> >>>>>>>>> +        };
> >>>>>>>>> +    };
> >>>>>>>>> +};
> >>>>>>>>> +
> >>>>>>>>>  &uart0 {
> >>>>>>>>>      status = "okay";
> >>>>>>>>>  };
> >>>>>>>>> diff --git a/arch/arm/boot/dts/mt2701.dtsi
> >>>>>>>>> b/arch/arm/boot/dts/mt2701.dtsi
> >>>>>>>>> index bdf8954..1eefce4 100644
> >>>>>>>>> --- a/arch/arm/boot/dts/mt2701.dtsi
> >>>>>>>>> +++ b/arch/arm/boot/dts/mt2701.dtsi
> >>>>>>>>> @@ -227,6 +227,18 @@
> >>>>>>>>>          status = "disabled";
> >>>>>>>>>      };
> >>>>>>>>>
> >>>>>>>>> +    nor_flash: spi@11014000 {
> >>>>>>>>> +        compatible = "mediatek,mt2701-nor",
> >>>>>>>>> +                 "mediatek,mt8173-nor";        
> >>>>>>>>
> >>>>>>>> Why define both here? Is "mediatek,mt8173-nor" really providing a
> >>>>>>>> subset of the features supported by "mediatek,mt2701-nor"?
> >>>>>>>>        
> >>>>>>>
> >>>>>>> I think even if the ip block is the same, we should provide both
> >>>>>>> bindings, just in case in the future we find out that mt2701 has some
> >>>>>>> hidden bug, feature or bug-feature. This way even if we update the
> >>>>>>> driver, we stay compatible with older device tree blobs in the wild.
> >>>>>>>
> >>>>>>> We can drop the mt2701-nor in the bindings definition if you want.       
> >>>>>
> >>>>> Oh, sorry, I misunderstood. What I meant is that if you want to
> >>>>> list/support all possible compatibles, maybe you should just put one
> >>>>> compatible in your DT and patch your driver (+ binding doc) to define
> >>>>> all of them.      
> >>>>
> >>>> Uh, what ? I lost you here :-)  
> > 
> > I mean adding a new entry in the mtk_nor_of_ids table (in
> > mtk-quadspi.c) so that the mediatek,mt2701-nor compatible string can be
> > matched directly, and you won't need to define 2 compatible strings in
> > your device tree.  
> 
> But then you grow the table in the driver, is that what we want if we
> can avoid that ?

The space you save by not growing the mtk_nor_of_ids table is lost in
your dtbs, so I'm not sure the size argument is relevant here. Also,
note that distros are shipping a lot of dtbs, and you're likely to have
several boards based on the mt2701 SoC, so, for this specific use case,
it's better to make the in-driver of-id table grow than specifying 2
compatibles in the DT. But as I said, I'm not sure we should rely on
this argument to decide which approach to choose (we're talking about a
few bytes here).

> 
> >>>>    
> >>>>>> This exactly. We should have a DT compat in the form:
> >>>>>> compatible = "vendor,<soc>-block", "vendor,<oldest-compat-soc>-block";
> >>>>>> Then if we find a problem in the future, we can match on the
> >>>>>> "vendor,<soc>-block" and still support the old DTs.      
> >>>>>
> >>>>> Not sure it's only in term of whose IP appeared first. My understanding
> >>>>> is that it's a way to provide inheritance. For example:
> >>>>>
> >>>>> 	"<soc-vendor>,<ip-version>", "<ip-vendor>,<ip-version>";
> >>>>>
> >>>>> or
> >>>>>
> >>>>> 	"<soc-vendor>,<full-featured-ip-version>","<soc-vendor>,<basic-feature-ip-version>";
> >>>>>
> >>>>> BTW, which one is the oldest between mt8173 and mt2701? :-)      
> >>>>
> >>>> And that's another thing and I agree with you, but I don't think that's
> >>>> what we're discussing in this thread. But (!), OT, I think we should
> >>>> codify the rules in Documentation/ . This discussion came up multiple
> >>>> times recently.
> >>>>
> >>>> And my question still stands, what do we put into the DT here, IMO
> >>>> compatible = "mediatek,mt2701-nor", "mediatek,mt8173-nor";    
> >>>
> >>> I'd say
> >>>
> >>> 	compatible = "mediatek,mt8173-nor";
> >>>
> >>> because both compatible are referring to very specific IP version. It's
> >>> not the same as    
> >>
> >> But then you don't have the ability to handle a block in this particular
> >> SoC in case there's a bug found in it in the future,
> >> so IMO it should be:
> >>
> >> compatible = "mediatek,mt2701-nor", "mediatek,mt8173-nor";  
> > 
> > Sorry again, I meant
> > 
> > 	compatible = "mediatek,mt2701-nor";
> >   
> >>  
> >>> 	compatible = "mediatek,mt8173-nor", "mediatek,mt81xx-nor";    
> >>
> >> This doesn't look right, since here we add two new compatibles ...  
> > 
> > That was just an example to describe how compatible inheritance works
> > (at least that's my understanding of it), it does not apply to this
> > particular use case.  
> 
> Well this is OK I guess, but then you can also use "mediatek,mt8173-nor"
> as the oldest supported compatible and be done with it, no ? It looks a
> bit crappy though, I admit that ...
> 

Let's stop bikeshedding and wait for DT maintainers feedback
before taking a decision ;-).

Rob, Mark, any opinion?

[toc] | [prev] | [next] | [standalone]

#1559865

FromMarek Vasut <marek.vasut@gmail.com>
Date2017-01-16 17:10 +0100
Message-ID<t0i13-2KF-9@gated-at.bofh.it>
In reply to#1559520
On 01/16/2017 09:40 AM, Boris Brezillon wrote:
> On Sun, 15 Jan 2017 01:23:48 +0100
> Marek Vasut <marek.vasut@gmail.com> wrote:
> 
>> On 01/14/2017 09:29 AM, Boris Brezillon wrote:
>>> On Fri, 13 Jan 2017 18:33:40 +0100
>>> Marek Vasut <marek.vasut@gmail.com> wrote:
>>>   
>>>> On 01/13/2017 05:56 PM, Boris Brezillon wrote:  
>>>>> On Fri, 13 Jan 2017 17:44:12 +0100
>>>>> Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>     
>>>>>> On 01/13/2017 05:28 PM, Boris Brezillon wrote:    
>>>>>>> On Fri, 13 Jan 2017 17:13:55 +0100
>>>>>>> Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>       
>>>>>>>> On 01/13/2017 04:12 PM, Matthias Brugger wrote:      
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 13/01/17 15:17, Boris Brezillon wrote:        
>>>>>>>>>> On Fri, 13 Jan 2017 15:13:29 +0800
>>>>>>>>>> Guochun Mao <guochun.mao@mediatek.com> wrote:
>>>>>>>>>>        
>>>>>>>>>>> Add Mediatek nor flash node.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Guochun Mao <guochun.mao@mediatek.com>
>>>>>>>>>>> ---
>>>>>>>>>>>  arch/arm/boot/dts/mt2701-evb.dts |   25 +++++++++++++++++++++++++
>>>>>>>>>>>  arch/arm/boot/dts/mt2701.dtsi    |   12 ++++++++++++
>>>>>>>>>>>  2 files changed, 37 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/arch/arm/boot/dts/mt2701-evb.dts
>>>>>>>>>>> b/arch/arm/boot/dts/mt2701-evb.dts
>>>>>>>>>>> index 082ca88..85e5ae8 100644
>>>>>>>>>>> --- a/arch/arm/boot/dts/mt2701-evb.dts
>>>>>>>>>>> +++ b/arch/arm/boot/dts/mt2701-evb.dts
>>>>>>>>>>> @@ -24,6 +24,31 @@
>>>>>>>>>>>      };
>>>>>>>>>>>  };
>>>>>>>>>>>
>>>>>>>>>>> +&nor_flash {
>>>>>>>>>>> +    pinctrl-names = "default";
>>>>>>>>>>> +    pinctrl-0 = <&nor_pins_default>;
>>>>>>>>>>> +    status = "okay";
>>>>>>>>>>> +    flash@0 {
>>>>>>>>>>> +        compatible = "jedec,spi-nor";
>>>>>>>>>>> +        reg = <0>;
>>>>>>>>>>> +    };
>>>>>>>>>>> +};
>>>>>>>>>>> +
>>>>>>>>>>> +&pio {
>>>>>>>>>>> +    nor_pins_default: nor {
>>>>>>>>>>> +        pins1 {
>>>>>>>>>>> +            pinmux = <MT2701_PIN_240_EXT_XCS__FUNC_EXT_XCS>,
>>>>>>>>>>> +                 <MT2701_PIN_241_EXT_SCK__FUNC_EXT_SCK>,
>>>>>>>>>>> +                 <MT2701_PIN_239_EXT_SDIO0__FUNC_EXT_SDIO0>,
>>>>>>>>>>> +                 <MT2701_PIN_238_EXT_SDIO1__FUNC_EXT_SDIO1>,
>>>>>>>>>>> +                 <MT2701_PIN_237_EXT_SDIO2__FUNC_EXT_SDIO2>,
>>>>>>>>>>> +                 <MT2701_PIN_236_EXT_SDIO3__FUNC_EXT_SDIO3>;
>>>>>>>>>>> +            drive-strength = <MTK_DRIVE_4mA>;
>>>>>>>>>>> +            bias-pull-up;
>>>>>>>>>>> +        };
>>>>>>>>>>> +    };
>>>>>>>>>>> +};
>>>>>>>>>>> +
>>>>>>>>>>>  &uart0 {
>>>>>>>>>>>      status = "okay";
>>>>>>>>>>>  };
>>>>>>>>>>> diff --git a/arch/arm/boot/dts/mt2701.dtsi
>>>>>>>>>>> b/arch/arm/boot/dts/mt2701.dtsi
>>>>>>>>>>> index bdf8954..1eefce4 100644
>>>>>>>>>>> --- a/arch/arm/boot/dts/mt2701.dtsi
>>>>>>>>>>> +++ b/arch/arm/boot/dts/mt2701.dtsi
>>>>>>>>>>> @@ -227,6 +227,18 @@
>>>>>>>>>>>          status = "disabled";
>>>>>>>>>>>      };
>>>>>>>>>>>
>>>>>>>>>>> +    nor_flash: spi@11014000 {
>>>>>>>>>>> +        compatible = "mediatek,mt2701-nor",
>>>>>>>>>>> +                 "mediatek,mt8173-nor";        
>>>>>>>>>>
>>>>>>>>>> Why define both here? Is "mediatek,mt8173-nor" really providing a
>>>>>>>>>> subset of the features supported by "mediatek,mt2701-nor"?
>>>>>>>>>>        
>>>>>>>>>
>>>>>>>>> I think even if the ip block is the same, we should provide both
>>>>>>>>> bindings, just in case in the future we find out that mt2701 has some
>>>>>>>>> hidden bug, feature or bug-feature. This way even if we update the
>>>>>>>>> driver, we stay compatible with older device tree blobs in the wild.
>>>>>>>>>
>>>>>>>>> We can drop the mt2701-nor in the bindings definition if you want.       
>>>>>>>
>>>>>>> Oh, sorry, I misunderstood. What I meant is that if you want to
>>>>>>> list/support all possible compatibles, maybe you should just put one
>>>>>>> compatible in your DT and patch your driver (+ binding doc) to define
>>>>>>> all of them.      
>>>>>>
>>>>>> Uh, what ? I lost you here :-)  
>>>
>>> I mean adding a new entry in the mtk_nor_of_ids table (in
>>> mtk-quadspi.c) so that the mediatek,mt2701-nor compatible string can be
>>> matched directly, and you won't need to define 2 compatible strings in
>>> your device tree.  
>>
>> But then you grow the table in the driver, is that what we want if we
>> can avoid that ?
> 
> The space you save by not growing the mtk_nor_of_ids table is lost in
> your dtbs, so I'm not sure the size argument is relevant here. Also,
> note that distros are shipping a lot of dtbs, and you're likely to have
> several boards based on the mt2701 SoC, so, for this specific use case,
> it's better to make the in-driver of-id table grow than specifying 2
> compatibles in the DT. But as I said, I'm not sure we should rely on
> this argument to decide which approach to choose (we're talking about a
> few bytes here).
> 
>>
>>>>>>    
>>>>>>>> This exactly. We should have a DT compat in the form:
>>>>>>>> compatible = "vendor,<soc>-block", "vendor,<oldest-compat-soc>-block";
>>>>>>>> Then if we find a problem in the future, we can match on the
>>>>>>>> "vendor,<soc>-block" and still support the old DTs.      
>>>>>>>
>>>>>>> Not sure it's only in term of whose IP appeared first. My understanding
>>>>>>> is that it's a way to provide inheritance. For example:
>>>>>>>
>>>>>>> 	"<soc-vendor>,<ip-version>", "<ip-vendor>,<ip-version>";
>>>>>>>
>>>>>>> or
>>>>>>>
>>>>>>> 	"<soc-vendor>,<full-featured-ip-version>","<soc-vendor>,<basic-feature-ip-version>";
>>>>>>>
>>>>>>> BTW, which one is the oldest between mt8173 and mt2701? :-)      
>>>>>>
>>>>>> And that's another thing and I agree with you, but I don't think that's
>>>>>> what we're discussing in this thread. But (!), OT, I think we should
>>>>>> codify the rules in Documentation/ . This discussion came up multiple
>>>>>> times recently.
>>>>>>
>>>>>> And my question still stands, what do we put into the DT here, IMO
>>>>>> compatible = "mediatek,mt2701-nor", "mediatek,mt8173-nor";    
>>>>>
>>>>> I'd say
>>>>>
>>>>> 	compatible = "mediatek,mt8173-nor";
>>>>>
>>>>> because both compatible are referring to very specific IP version. It's
>>>>> not the same as    
>>>>
>>>> But then you don't have the ability to handle a block in this particular
>>>> SoC in case there's a bug found in it in the future,
>>>> so IMO it should be:
>>>>
>>>> compatible = "mediatek,mt2701-nor", "mediatek,mt8173-nor";  
>>>
>>> Sorry again, I meant
>>>
>>> 	compatible = "mediatek,mt2701-nor";
>>>   
>>>>  
>>>>> 	compatible = "mediatek,mt8173-nor", "mediatek,mt81xx-nor";    
>>>>
>>>> This doesn't look right, since here we add two new compatibles ...  
>>>
>>> That was just an example to describe how compatible inheritance works
>>> (at least that's my understanding of it), it does not apply to this
>>> particular use case.  
>>
>> Well this is OK I guess, but then you can also use "mediatek,mt8173-nor"
>> as the oldest supported compatible and be done with it, no ? It looks a
>> bit crappy though, I admit that ...
>>
> 
> Let's stop bikeshedding and wait for DT maintainers feedback
> before taking a decision ;-).

+1 :)

> Rob, Mark, any opinion?
> 


-- 
Best regards,
Marek Vasut

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web