Re: [PATCH v7 2/3] arm64: dts: mediatek: add basic mt7986a support

[Sam Shih]

Hi,

On Fri, 2021-11-19 at 11:31 +0100, Matthias Brugger wrote:
> 	
> 
> On 18/11/2021 04:48, Sam Shih wrote:
> > Hi
> > 
> > On Tue, 2021-11-16 at 12:18 +0100, Matthias Brugger wrote:
> > > 
> > > On 16/11/2021 02:39, Sam Shih wrote:
> > > > Hi,
> > > > 
> > > > On Mon, 2021-11-15 at 17:27 +0100, Matthias Brugger wrote:
> > > > > Hi,
> > > > > 
> > > > > On 18/10/2021 13:40, Sam Shih wrote:
> > > > > > Add basic chip support for Mediatek mt7986a, include
> > > > > > basic uart nodes, rng node and watchdog node.
> > > > > > 
> > > > > > Add cpu node, timer node, gic node, psci and reserved-
> > > > > > memory
> > > > > > node
> > > > > > for ARM Trusted Firmware.
> > > > > > 
> > > > > 
> > > > > What is the exact difference between mt7986a and mt7986b?
> > > > > Right
> > > > > now,
> > > > > it's only
> > > > > the compatible, for that it makes no sense to split them.
> > > > > 
> > > > 
> > > > The difference between mt7986a and mt7986b is pinout which
> > > > described
> > > > in our pinctrl patch series
> > > > 
> > 
> > 
https://urldefense.com/v3/__https://lore.kernel.org/all/20211022124036.5291-3-sam.shih@xxxxxxxxxxxx/__;!!CTRNKA9wMg0ARbw!0kseU8x1KnHHXDErh6Yj6MKqecufPEfGyeumtTBism47e99UFO2Gs-HfWjL1_jUv$
> > > >   
> > > > 
> > > > You are right, in this "basic SoC support" patch series, only
> > > > show
> > > > compatible differences
> > > > 
> > > > > It would be good to see what the exact differences are, so
> > > > > that
> > > > > we
> > > > > can see if it
> > > > > makes sense to have one of the alternatives:
> > > > > 1) use a common mt7986.dtsi which get included by
> > > > > mt7986[a,b].dtsi
> > > > > 2) Use on mt7986.dtsi and only add one mt7986a.dtsi or
> > > > > mt7986b.dtsi
> > > > > which has
> > > > > add-ons.
> > > > > 
> > > > 
> > > > In this case, can we use solution (1) to create a generic
> > > > mt7986.dtsi
> > > > in this patch series, and add mt7986[a,b].dtsi to the dts part
> > > > of
> > > > the
> > > > pinctrl patch series to separate the difference nodes?
> > > > 
> > > 
> > > If the only difference is the GPIO controller then why not go
> > > with
> > > solution 2.
> > > Create a mt7986.dtsi which holds e.g. the node for pincontroller
> > > mt7986a and
> > > then create a mt7986b.dtsi that just changes compatible and gpio-
> > > ranges:
> > > 
> > > &pio {
> > >      compatible = "mediatek,mt7986b-pinctrl";
> > >      gpio-ranges = <&pio 0 0 41>, <&pio 66 66 35>;
> > > }
> > > 
> > > What do you think?
> > 
> > Ok,
> > 
> > For this basic patch series DTS, I will send the next version:
> > - Use "mt7986.dtsi" instead of "mt7986[a,b].dtsi",
> >    And make"mt7986.dtsi" get included by "mt7986[a,b]-rfb.dts"
> >    (No dedicated uart1/uart2 pinout for mt7986b-rfb, status of dts
> > node
> > shoud be set to "disabled")
> > 
> > 
> > For the pinctrl patch series DTS, I will send th next version:
> > - Add "mt7986b.dtsi" according to your suggestion,
> >    the new include
> > chain will be:
> >    mt7986a-rfb.dts <-- mt7986.dtsi (mt7986a pinctrl)
> >   
> > mt7986b-rfb.dts <-- mt7986b.dtsi (mt7986b pinctrl) <-- mt7986.dtsi
> > (mt7986a pinctrl)
> > 
> > Do you agree above proposal?
> > 
> 
> I mean something like this:
> mt7986a.dtsi:
> pio: pinctrl@1001f000 {
> 	compatible = "mediatek,mt7986a-pinctrl";
> 	reg = <0 0x1001f000 0 0x1000>,
> 	      <0 0x11c30000 0 0x1000>,
> 	      <0 0x11c40000 0 0x1000>,
> 	      <0 0x11e20000 0 0x1000>,
> 	      <0 0x11e30000 0 0x1000>,
> 	      <0 0x11f00000 0 0x1000>,
> 	      <0 0x11f10000 0 0x1000>,
> 	      <0 0x1000b000 0 0x1000>;
> 	reg-names = "gpio", "iocfg_rt", "iocfg_rb", "iocfg_lt",
> 		    "iocfg_lb", "iocfg_tr", "iocfg_tl", "eint";
> 	gpio-controller;
> 	#gpio-cells = <2>;
> 	gpio-ranges = <&pio 0 0 100>;
> 	interrupt-controller;
> 	interrupts = <GIC_SPI 225 IRQ_TYPE_LEVEL_HIGH>;
> 	interrupt-parent = <&gic>;
> 	#interrupt-cells = <2>;
> };
> 
> mt7986b.dtsi:
> #include "mt7986a.dtsi"
> 
> &pio {
>       compatible = "mediatek,mt7986b-pinctrl";
>       gpio-ranges = <&pio 0 0 41>, <&pio 66 66 35>;
> }
> 
> mt7986b-rfb.dts:
> #include "mt7986b.dtsi"
> 
> &pio {
> 	uart1_pins: uart1-pins {
> 		mux { [...]
> 
> 
> mt7986a-rfb.dts:
> #include "mt7986a.dtsi"
> 
> &pio {
> 	uart1_pins: uart1-pins {
> 		mux { [...]
> 
> 
> Makes sense?
> 

Okay,

I have sent new patch set based on your suggestion,

Basic Part:

https://lore.kernel.org/all/20211122123222.8016-1-sam.shih@xxxxxxxxxxxx/

Pinctrl:

https://lore.kernel.org/all/20211122123552.8218-1-sam.shih@xxxxxxxxxxxx/

Please take a look at those patches when you are free.

Regards,
Sam

> Regards,
> Matthias
>