| Message ID | 1484291609-20195-3-git-send-email-guochun.mao@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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>;
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>; >
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>; > >
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 ?
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? :-)
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 ?
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.
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. >
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. > > > >
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 ...
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?
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? >
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
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
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
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
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
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
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
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
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.
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
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
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>;
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(+)