Messages in this thread |  | | Date | Fri, 19 Nov 2021 11:31:15 +0100 | From | Matthias Brugger <> | Subject | Re: [PATCH v7 2/3] arm64: dts: mediatek: add basic mt7986a support |
| |
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@mediatek.com/__;!!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?
Regards, Matthias
|  |