[PATCH v1 0/2] add nor flash node for mt2701

[PATCH v1 0/2] add nor flash node for mt2701

[Guochun Mao]

This patch series based on v4.10-rc2, include MT2701 spinor node and bindings.

Dependent on "Add clock and power domain DT nodes for Mediatek MT2701"[1].

[1] http://lists.infradead.org/pipermail/linux-mediatek/2016-December/007637.html

Guochun Mao (2):
  Documentation: mtk-quadspi: update DT bindings
  arm: dts: mt2701: add nor flash node

 .../devicetree/bindings/mtd/mtk-quadspi.txt        |  4 +++-
 arch/arm/boot/dts/mt2701-evb.dts                   | 25 ++++++++++++++++++++++
 arch/arm/boot/dts/mt2701.dtsi                      | 12 +++++++++++
 3 files changed, 40 insertions(+), 1 deletion(-)

--
1.8.1.1.dirty

[PATCH v1 1/2] Documentation: mtk-quadspi: update DT bindings

[Guochun Mao]

Add "mediatek,mt2701-nor" for nor flash node's compatible.

Signed-off-by: Guochun Mao <guochun.mao@mediatek.com>
---
 .../devicetree/bindings/mtd/mtk-quadspi.txt        |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mtd/mtk-quadspi.txt b/Documentation/devicetree/bindings/mtd/mtk-quadspi.txt
index fb314f0..f83d31d 100644
--- a/Documentation/devicetree/bindings/mtd/mtk-quadspi.txt
+++ b/Documentation/devicetree/bindings/mtd/mtk-quadspi.txt
@@ -1,7 +1,9 @@
 * Serial NOR flash controller for MTK MT81xx (and similar)
 
 Required properties:
-- compatible: 	  should be "mediatek,mt8173-nor";
+- compatible: 	  should contain:
+		  "mediatek,mt2701-nor" for MT2701,
+		  "mediatek,mt8173-nor" for MT8173.
 - reg: 		  physical base address and length of the controller's register
 - clocks: 	  the phandle of the clocks needed by the nor controller
 - clock-names: 	  the names of the clocks
-- 
1.7.9.5

Re: [PATCH v1 1/2] Documentation: mtk-quadspi: update DT bindings

[Boris Brezillon]

On Fri, 13 Jan 2017 15:13:28 +0800
Guochun Mao <guochun.mao@mediatek.com> wrote:

> Add "mediatek,mt2701-nor" for nor flash node's compatible.
> 
> Signed-off-by: Guochun Mao <guochun.mao@mediatek.com>
> ---
>  .../devicetree/bindings/mtd/mtk-quadspi.txt        |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/mtk-quadspi.txt b/Documentation/devicetree/bindings/mtd/mtk-quadspi.txt
> index fb314f0..f83d31d 100644
> --- a/Documentation/devicetree/bindings/mtd/mtk-quadspi.txt
> +++ b/Documentation/devicetree/bindings/mtd/mtk-quadspi.txt
> @@ -1,7 +1,9 @@
>  * Serial NOR flash controller for MTK MT81xx (and similar)
>  
>  Required properties:
> -- compatible: 	  should be "mediatek,mt8173-nor";
> +- compatible: 	  should contain:
> +		  "mediatek,mt2701-nor" for MT2701,
> +		  "mediatek,mt8173-nor" for MT8173.

Do you need to define a new compatible? If the IPs are exactly the same
in both SoCs it shouldn't be needed.

>  - reg: 		  physical base address and length of the controller's register
>  - clocks: 	  the phandle of the clocks needed by the nor controller
>  - clock-names: 	  the names of the clocks

Re: [PATCH v1 1/2] Documentation: mtk-quadspi: update DT bindings

[Rob Herring]

On Fri, Jan 13, 2017 at 03:13:26PM +0100, Boris Brezillon wrote:
> On Fri, 13 Jan 2017 15:13:28 +0800
> Guochun Mao <guochun.mao@mediatek.com> wrote:
> 
> > Add "mediatek,mt2701-nor" for nor flash node's compatible.
> > 
> > Signed-off-by: Guochun Mao <guochun.mao@mediatek.com>
> > ---
> >  .../devicetree/bindings/mtd/mtk-quadspi.txt        |    4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mtd/mtk-quadspi.txt b/Documentation/devicetree/bindings/mtd/mtk-quadspi.txt
> > index fb314f0..f83d31d 100644
> > --- a/Documentation/devicetree/bindings/mtd/mtk-quadspi.txt
> > +++ b/Documentation/devicetree/bindings/mtd/mtk-quadspi.txt
> > @@ -1,7 +1,9 @@
> >  * Serial NOR flash controller for MTK MT81xx (and similar)
> >  
> >  Required properties:
> > -- compatible: 	  should be "mediatek,mt8173-nor";
> > +- compatible: 	  should contain:
> > +		  "mediatek,mt2701-nor" for MT2701,
> > +		  "mediatek,mt8173-nor" for MT8173.
> 
> Do you need to define a new compatible? If the IPs are exactly the same
> in both SoCs it shouldn't be needed.

Rather the 2701 should contain both strings in that case.

Rob

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

[Guochun Mao]

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

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

[Marek Vasut]

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

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

[Boris Brezillon]

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>;

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

[Matthias Brugger]

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>;
>

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

[Boris Brezillon]

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>;  
> >  

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

[Marek Vasut]

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

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

[Boris Brezillon]

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? :-)

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

[Marek Vasut]

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

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

[Boris Brezillon]

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.

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

[Marek Vasut]

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

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

[Boris Brezillon]

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.
> >   
> 
> 

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

[Marek Vasut]

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

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

[Boris Brezillon]

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?

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

[Marek Vasut]

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

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

[Thomas Petazzoni]

Hello,

(Side note: you guys should learn about stripping irrelevant parts of
an e-mail when replying!)

On Mon, 16 Jan 2017 09:40:32 +0100, Boris Brezillon wrote:

> > 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?

I agree that a clarification would be good. There are really two
options:

 1. Have two compatible strings in the DT, the one that matches the
    exact SoC where the IP is found (first compatible string) and the
    one that matches some other SoC where the same IP is found (second
    compatible string). Originally, Linux only supports the second
    compatible string in its device driver, but if it happens that a
    difference is found between two IPs that we thought were the same,
    we can add support for the first compatible string in the driver,
    with a slightly different behavior.

 2. Have a single compatible string in the DT, matching the exact SoC
    where the IP is found. This involves adding immediately this
    compatible string in the corresponding driver.

I've not really been able to figure out which of the two options is the
most future-proof/appropriate.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Rob Herring]

On Tue, Jan 17, 2017 at 02:36:50PM +1100, Thomas Petazzoni wrote:
> Hello,
> 
> (Side note: you guys should learn about stripping irrelevant parts of
> an e-mail when replying!)
>
> On Mon, 16 Jan 2017 09:40:32 +0100, Boris Brezillon wrote:
> 
> > > 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?
>

Sigh, is how to do compatibles really not yet understood?
 
> I agree that a clarification would be good. There are really two
> options:
> 
>  1. Have two compatible strings in the DT, the one that matches the
>     exact SoC where the IP is found (first compatible string) and the
>     one that matches some other SoC where the same IP is found (second
>     compatible string). Originally, Linux only supports the second
>     compatible string in its device driver, but if it happens that a
>     difference is found between two IPs that we thought were the same,
>     we can add support for the first compatible string in the driver,
>     with a slightly different behavior.

This. And no wildcards in the compatible string. 

>  2. Have a single compatible string in the DT, matching the exact SoC
>     where the IP is found. This involves adding immediately this
>     compatible string in the corresponding driver.

I wouldn't object to this from a DT perspective as I have no clue 
generally if IP blocks are "the same" or not. Subsystem maintainers will 
object though.

> I've not really been able to figure out which of the two options is the
> most future-proof/appropriate.

They are both future-proof. #2 has the disadvantage of requiring a 
kernel update for a new SoC. 

Rob

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

[Thomas Petazzoni]

Hello,

On Wed, 18 Jan 2017 16:20:10 -0600, Rob Herring wrote:

> > > Rob, Mark, any opinion?  
> >  
> 
> Sigh, is how to do compatibles really not yet understood?

Well, it seems like not everyone necessarily understands what is the
best strategy to adopt (me included).

> > I agree that a clarification would be good. There are really two
> > options:
> > 
> >  1. Have two compatible strings in the DT, the one that matches the
> >     exact SoC where the IP is found (first compatible string) and the
> >     one that matches some other SoC where the same IP is found (second
> >     compatible string). Originally, Linux only supports the second
> >     compatible string in its device driver, but if it happens that a
> >     difference is found between two IPs that we thought were the same,
> >     we can add support for the first compatible string in the driver,
> >     with a slightly different behavior.  
> 
> This. And no wildcards in the compatible string. 

OK. So it means that today we do something like:

	compatible = "baz,foo-12", "baz,foo-00";

and support only baz,foo-00 in the driver. If tomorrow we discover
that there is in fact a difference between the two IP blocks, we can
add support for baz,foo-12 in the driver, and handle the differences.

But then, the DT still contains:

	compatible = "baz,foo-12", "baz,foo-00";

and therefore pretends that the IP block is compatible with
"baz,foo-00" which is in fact *not* the case. It was a mistake to
consider it as compatible. So we keep living with a DT that has
incorrect information.

> 
> >  2. Have a single compatible string in the DT, matching the exact SoC
> >     where the IP is found. This involves adding immediately this
> >     compatible string in the corresponding driver.  
> 
> I wouldn't object to this from a DT perspective as I have no clue 
> generally if IP blocks are "the same" or not. Subsystem maintainers will 
> object though.

Knowing if IP blocks are "the same" is in fact not necessarily trivial.
What appears to be identical IP blocks today might be discovered later
as actually having subtle differences (sometimes not even visible in
the datasheet).

> > I've not really been able to figure out which of the two options is the
> > most future-proof/appropriate.  
> 
> They are both future-proof. #2 has the disadvantage of requiring a 
> kernel update for a new SoC. 

Which is generally anyway needed because a new SoC will almost always
require some new drivers, adjusting pin-muxing or clock drivers, etc.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Rob Herring]

On Wed, Jan 18, 2017 at 5:38 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Hello,
>
> On Wed, 18 Jan 2017 16:20:10 -0600, Rob Herring wrote:
>
>> > > Rob, Mark, any opinion?
>> >
>>
>> Sigh, is how to do compatibles really not yet understood?
>
> Well, it seems like not everyone necessarily understands what is the
> best strategy to adopt (me included).
>
>> > I agree that a clarification would be good. There are really two
>> > options:
>> >
>> >  1. Have two compatible strings in the DT, the one that matches the
>> >     exact SoC where the IP is found (first compatible string) and the
>> >     one that matches some other SoC where the same IP is found (second
>> >     compatible string). Originally, Linux only supports the second
>> >     compatible string in its device driver, but if it happens that a
>> >     difference is found between two IPs that we thought were the same,
>> >     we can add support for the first compatible string in the driver,
>> >     with a slightly different behavior.
>>
>> This. And no wildcards in the compatible string.
>
> OK. So it means that today we do something like:
>
>         compatible = "baz,foo-12", "baz,foo-00";
>
> and support only baz,foo-00 in the driver. If tomorrow we discover
> that there is in fact a difference between the two IP blocks, we can
> add support for baz,foo-12 in the driver, and handle the differences.
>
> But then, the DT still contains:
>
>         compatible = "baz,foo-12", "baz,foo-00";
>
> and therefore pretends that the IP block is compatible with
> "baz,foo-00" which is in fact *not* the case. It was a mistake to
> consider it as compatible. So we keep living with a DT that has
> incorrect information.

I wouldn't say it's a mistake necessarily. The old compatible would
probably work to some extent. I'd assume it was tested to some level.
Or it could be other changes exposing a difference.

>> >  2. Have a single compatible string in the DT, matching the exact SoC
>> >     where the IP is found. This involves adding immediately this
>> >     compatible string in the corresponding driver.
>>
>> I wouldn't object to this from a DT perspective as I have no clue
>> generally if IP blocks are "the same" or not. Subsystem maintainers will
>> object though.
>
> Knowing if IP blocks are "the same" is in fact not necessarily trivial.
> What appears to be identical IP blocks today might be discovered later
> as actually having subtle differences (sometimes not even visible in
> the datasheet).

Yes, I know. That's exactly when you should have multiple compatibles.
Trying to guarantee things are the same is just going to get you in
trouble. You only need to figure out if blocks are obviously different
and only drop the old compatible in that case.

>> > I've not really been able to figure out which of the two options is the
>> > most future-proof/appropriate.
>>
>> They are both future-proof. #2 has the disadvantage of requiring a
>> kernel update for a new SoC.
>
> Which is generally anyway needed because a new SoC will almost always
> require some new drivers, adjusting pin-muxing or clock drivers, etc.

Yes, but you don't want to have to update every single driver.

Rob

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

[Boris Brezillon]

Hi Rob,

On Wed, 18 Jan 2017 20:51:08 -0600
Rob Herring <robh@kernel.org> wrote:

> On Wed, Jan 18, 2017 at 5:38 PM, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
> > Hello,
> >
> > On Wed, 18 Jan 2017 16:20:10 -0600, Rob Herring wrote:
> >  
> >> > > Rob, Mark, any opinion?  
> >> >  
> >>
> >> Sigh, is how to do compatibles really not yet understood?  
> >
> > Well, it seems like not everyone necessarily understands what is the
> > best strategy to adopt (me included).
> >  
> >> > I agree that a clarification would be good. There are really two
> >> > options:
> >> >
> >> >  1. Have two compatible strings in the DT, the one that matches the
> >> >     exact SoC where the IP is found (first compatible string) and the
> >> >     one that matches some other SoC where the same IP is found (second
> >> >     compatible string). Originally, Linux only supports the second
> >> >     compatible string in its device driver, but if it happens that a
> >> >     difference is found between two IPs that we thought were the same,
> >> >     we can add support for the first compatible string in the driver,
> >> >     with a slightly different behavior.  
> >>
> >> This. And no wildcards in the compatible string.  
> >
> > OK. So it means that today we do something like:
> >
> >         compatible = "baz,foo-12", "baz,foo-00";
> >
> > and support only baz,foo-00 in the driver. If tomorrow we discover
> > that there is in fact a difference between the two IP blocks, we can
> > add support for baz,foo-12 in the driver, and handle the differences.
> >
> > But then, the DT still contains:
> >
> >         compatible = "baz,foo-12", "baz,foo-00";
> >
> > and therefore pretends that the IP block is compatible with
> > "baz,foo-00" which is in fact *not* the case. It was a mistake to
> > consider it as compatible. So we keep living with a DT that has
> > incorrect information.  
> 
> I wouldn't say it's a mistake necessarily. The old compatible would
> probably work to some extent. I'd assume it was tested to some level.
> Or it could be other changes exposing a difference.

One last question and I'm done: is something like that acceptable?

	compatible = "<vendor>,<old-soc>","<vendor>,<new-soc>";

This can happen when someone adds support for an unsupported feature
on a brand new SoC, and then someone else use the same driver for an
older SoC embedding the same IP but still wants to add a new compatible
just in case these 2 IPs appear to be slightly different.

Here the order of compat strings is no longer following a clear rule
like 'most-specific compatible first' or 'newest IP/SoC version first',
it's completely dependent on the order these IPs were supported in the
OS (Linux). I'm perfectly fine with that BTW, just want to make sure
this is authorized.

Regards,

Boris

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

[Rob Herring]

On Thu, Jan 19, 2017 at 2:14 AM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> Hi Rob,
>
> On Wed, 18 Jan 2017 20:51:08 -0600
> Rob Herring <robh@kernel.org> wrote:
>
>> On Wed, Jan 18, 2017 at 5:38 PM, Thomas Petazzoni
>> <thomas.petazzoni@free-electrons.com> wrote:
>> > Hello,
>> >
>> > On Wed, 18 Jan 2017 16:20:10 -0600, Rob Herring wrote:
>> >
>> >> > > Rob, Mark, any opinion?
>> >> >
>> >>
>> >> Sigh, is how to do compatibles really not yet understood?
>> >
>> > Well, it seems like not everyone necessarily understands what is the
>> > best strategy to adopt (me included).
>> >
>> >> > I agree that a clarification would be good. There are really two
>> >> > options:
>> >> >
>> >> >  1. Have two compatible strings in the DT, the one that matches the
>> >> >     exact SoC where the IP is found (first compatible string) and the
>> >> >     one that matches some other SoC where the same IP is found (second
>> >> >     compatible string). Originally, Linux only supports the second
>> >> >     compatible string in its device driver, but if it happens that a
>> >> >     difference is found between two IPs that we thought were the same,
>> >> >     we can add support for the first compatible string in the driver,
>> >> >     with a slightly different behavior.
>> >>
>> >> This. And no wildcards in the compatible string.
>> >
>> > OK. So it means that today we do something like:
>> >
>> >         compatible = "baz,foo-12", "baz,foo-00";
>> >
>> > and support only baz,foo-00 in the driver. If tomorrow we discover
>> > that there is in fact a difference between the two IP blocks, we can
>> > add support for baz,foo-12 in the driver, and handle the differences.
>> >
>> > But then, the DT still contains:
>> >
>> >         compatible = "baz,foo-12", "baz,foo-00";
>> >
>> > and therefore pretends that the IP block is compatible with
>> > "baz,foo-00" which is in fact *not* the case. It was a mistake to
>> > consider it as compatible. So we keep living with a DT that has
>> > incorrect information.
>>
>> I wouldn't say it's a mistake necessarily. The old compatible would
>> probably work to some extent. I'd assume it was tested to some level.
>> Or it could be other changes exposing a difference.
>
> One last question and I'm done: is something like that acceptable?
>
>         compatible = "<vendor>,<old-soc>","<vendor>,<new-soc>";
>
> This can happen when someone adds support for an unsupported feature
> on a brand new SoC, and then someone else use the same driver for an
> older SoC embedding the same IP but still wants to add a new compatible
> just in case these 2 IPs appear to be slightly different.

Yes, it's old and new compatible strings in this case and it's newest
compatible string first.

> Here the order of compat strings is no longer following a clear rule
> like 'most-specific compatible first' or 'newest IP/SoC version first',
> it's completely dependent on the order these IPs were supported in the
> OS (Linux). I'm perfectly fine with that BTW, just want to make sure
> this is authorized.

I guess we should say "newest compatible for IP first" instead. There
are some exceptions where we add fallbacks later on, but that falls
under the most-specific part.

It's order that the bindings are defined, not Linux support really,
but in practice those are the same.

Rob

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

[Guochun Mao]

Hi,
On Thu, 2017-01-19 at 08:18 -0600, Rob Herring wrote:
> On Thu, Jan 19, 2017 at 2:14 AM, Boris Brezillon
> > One last question and I'm done: is something like that acceptable?
> >
> >         compatible = "<vendor>,<old-soc>","<vendor>,<new-soc>";
> >
> > This can happen when someone adds support for an unsupported feature
> > on a brand new SoC, and then someone else use the same driver for an
> > older SoC embedding the same IP but still wants to add a new compatible
> > just in case these 2 IPs appear to be slightly different.
> 
> Yes, it's old and new compatible strings in this case and it's newest
> compatible string first.
> 
> > Here the order of compat strings is no longer following a clear rule
> > like 'most-specific compatible first' or 'newest IP/SoC version first',
> > it's completely dependent on the order these IPs were supported in the
> > OS (Linux). I'm perfectly fine with that BTW, just want to make sure
> > this is authorized.
> 
> I guess we should say "newest compatible for IP first" instead. There
> are some exceptions where we add fallbacks later on, but that falls
> under the most-specific part.
> 
> It's order that the bindings are defined, not Linux support really,
> but in practice those are the same.
> 
> Rob

Thanks for all your effort for code reviewing.
Our mt2701-nor's hardware is designed base on mt8713-nor,
even so, there would be some slight difference.
If I don't misunderstand your viewpoint in this discussion,
there's no need to drop mt2701-nor compatible.
And if not, is there any other suggestion?

Thanks.

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

[Boris Brezillon]

On Sun, 22 Jan 2017 10:36:40 +0800
Guochun Mao <guochun.mao@mediatek.com> wrote:

> Hi,
> On Thu, 2017-01-19 at 08:18 -0600, Rob Herring wrote:
> > On Thu, Jan 19, 2017 at 2:14 AM, Boris Brezillon  
> > > One last question and I'm done: is something like that acceptable?
> > >
> > >         compatible = "<vendor>,<old-soc>","<vendor>,<new-soc>";
> > >
> > > This can happen when someone adds support for an unsupported feature
> > > on a brand new SoC, and then someone else use the same driver for an
> > > older SoC embedding the same IP but still wants to add a new compatible
> > > just in case these 2 IPs appear to be slightly different.  
> > 
> > Yes, it's old and new compatible strings in this case and it's newest
> > compatible string first.
> >   
> > > Here the order of compat strings is no longer following a clear rule
> > > like 'most-specific compatible first' or 'newest IP/SoC version first',
> > > it's completely dependent on the order these IPs were supported in the
> > > OS (Linux). I'm perfectly fine with that BTW, just want to make sure
> > > this is authorized.  
> > 
> > I guess we should say "newest compatible for IP first" instead. There
> > are some exceptions where we add fallbacks later on, but that falls
> > under the most-specific part.
> > 
> > It's order that the bindings are defined, not Linux support really,
> > but in practice those are the same.
> > 
> > Rob  
> 
> Thanks for all your effort for code reviewing.
> Our mt2701-nor's hardware is designed base on mt8713-nor,
> even so, there would be some slight difference.
> If I don't misunderstand your viewpoint in this discussion,
> there's no need to drop mt2701-nor compatible.

No, just update the documentation as suggested by Rob.

> And if not, is there any other suggestion?

Nope, and my apologies for being so insistent on something I obviously
misunderstood.

Regards,

Boris

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

[John Crispin]

On 24/01/2017 11:31, Boris Brezillon wrote:
> On Sun, 22 Jan 2017 10:36:40 +0800
> Guochun Mao <guochun.mao@mediatek.com> wrote:
> 
>> Hi,
>> On Thu, 2017-01-19 at 08:18 -0600, Rob Herring wrote:
>>> On Thu, Jan 19, 2017 at 2:14 AM, Boris Brezillon  
>>>> One last question and I'm done: is something like that acceptable?
>>>>
>>>>         compatible = "<vendor>,<old-soc>","<vendor>,<new-soc>";
>>>>
>>>> This can happen when someone adds support for an unsupported feature
>>>> on a brand new SoC, and then someone else use the same driver for an
>>>> older SoC embedding the same IP but still wants to add a new compatible
>>>> just in case these 2 IPs appear to be slightly different.  
>>>
>>> Yes, it's old and new compatible strings in this case and it's newest
>>> compatible string first.
>>>   
>>>> Here the order of compat strings is no longer following a clear rule
>>>> like 'most-specific compatible first' or 'newest IP/SoC version first',
>>>> it's completely dependent on the order these IPs were supported in the
>>>> OS (Linux). I'm perfectly fine with that BTW, just want to make sure
>>>> this is authorized.  
>>>
>>> I guess we should say "newest compatible for IP first" instead. There
>>> are some exceptions where we add fallbacks later on, but that falls
>>> under the most-specific part.
>>>
>>> It's order that the bindings are defined, not Linux support really,
>>> but in practice those are the same.
>>>
>>> Rob  
>>
>> Thanks for all your effort for code reviewing.
>> Our mt2701-nor's hardware is designed base on mt8713-nor,
>> even so, there would be some slight difference.
>> If I don't misunderstand your viewpoint in this discussion,
>> there's no need to drop mt2701-nor compatible.
> 
> No, just update the documentation as suggested by Rob.
> 
>> And if not, is there any other suggestion?
> 
> Nope, and my apologies for being so insistent on something I obviously
> misunderstood.
> 
> Regards,
> 
> Boris


Hi,

could you please add the mt7623 compat string to the documentation
aswell while at it ? mt7623/mt2701 are essentially the same and it'll
safe us time and effort and not cause merge order conflicts. otherwise i
would need to wait till the mt2701 patch is merged before i can send the
mt7623 one

Thanks !
	John

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

[Boris Brezillon]

On Wed, 18 Jan 2017 16:20:10 -0600
Rob Herring <robh@kernel.org> wrote:

> On Tue, Jan 17, 2017 at 02:36:50PM +1100, Thomas Petazzoni wrote:
> > Hello,
> > 
> > (Side note: you guys should learn about stripping irrelevant parts of
> > an e-mail when replying!)
> >
> > On Mon, 16 Jan 2017 09:40:32 +0100, Boris Brezillon wrote:
> >   
> > > > 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?  
> >  
> 
> Sigh, is how to do compatibles really not yet understood?

Apparently not, and I fear this is not the last misunderstanding on my
side ;-).

>  
> > I agree that a clarification would be good. There are really two
> > options:
> > 
> >  1. Have two compatible strings in the DT, the one that matches the
> >     exact SoC where the IP is found (first compatible string) and the
> >     one that matches some other SoC where the same IP is found (second
> >     compatible string). Originally, Linux only supports the second
> >     compatible string in its device driver, but if it happens that a
> >     difference is found between two IPs that we thought were the same,
> >     we can add support for the first compatible string in the driver,
> >     with a slightly different behavior.  
> 
> This. And no wildcards in the compatible string. 
> 
> >  2. Have a single compatible string in the DT, matching the exact SoC
> >     where the IP is found. This involves adding immediately this
> >     compatible string in the corresponding driver.  
> 
> I wouldn't object to this from a DT perspective as I have no clue 
> generally if IP blocks are "the same" or not. Subsystem maintainers will 
> object though.
> 
> > I've not really been able to figure out which of the two options is the
> > most future-proof/appropriate.  
> 
> They are both future-proof. #2 has the disadvantage of requiring a 
> kernel update for a new SoC. 
> 
> Rob

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

[Thomas Petazzoni]

Hello,

On Fri, 13 Jan 2017 17:56:28 +0100, Boris Brezillon wrote:

> because both compatible are referring to very specific IP version. It's
> not the same as
> 
> 	compatible = "mediatek,mt8173-nor", "mediatek,mt81xx-nor";

mt81xx-nor is a bogus compatible string, and DT binding maintainers
will not accept it. They don't want compatible strings with "wildcards".

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com