Re: [RFC v1 02/12] dt-bindings: PCI: mediatek-gen3: add support for mt7986

[Frank Wunderlich]

Am 19. Oktober 2022 10:28:59 MESZ schrieb AngeloGioacchino Del Regno <angelogioacchino.delregno at collabora.com>:
>Il 17/10/22 12:41, Frank Wunderlich ha scritto:

>If this SoC has a different clock tree... then you should add bindings for this
>kind of clock tree.
>
>CLK_INFRA_IPCIER_CK is *not* a peri clock: "peri" means PERICFG, which does not
>seem to be present in this SoC... so no, you can't assign it to "peri_26m", nor
>you can assign it to tl_32k, as that's not a 32KHz clock.
>
>CLK_INFRA_PCIEB_CK can be a "top_133m" clock... as it is gating "sysaxi_sel",
>which is a topckgen clock.
>
>CLK_INFRA_IPCIE_CK is your "tl_(something)" clock, as that's effectively gating
>"pextp_tl_ck_sel" (which is the PCIe Transaction Layer clock mux).
>
>CLK_INFRA_IPCIE_PIPE_CK seems to be parented to "top_xtal", frequency = 40MHz,
>so I don't see how can this be a pl_250m? Looks like being a 40m clock and I
>wish we didn't have clock frequencies specified in the names, as "pl" would fit,
>but "pl_250m" does not.
>I wonder if we can change the clock names and reflect the changes to the mt8192
>devicetree (mt8195 does not have any pcie node yet), and if that would be a good
>idea right now.
>
>...and I've left the first for last, because...
>
>CLK_INFRA_PCIE_SEL: I have no datasheet for this SoC, but if you're sure that
>this clock is selecting the source clock to CLK_INFRA_IPCIE_CK, then the clock
>driver is wrong...
>
>Right now, I see the following:
>
>static const char *const infra_pcie_parents[] __initconst = {
>	"top_rtc_32p7k", "csw_f26m_sel", "top_xtal", "pextp_tl_ck_sel"
>};
>
>GATE_INFRA2(CLK_INFRA_IPCIE_CK, "infra_ipcie", "pextp_tl_ck_sel", 12),
>
>MUX_GATE_CLR_SET_UPD(CLK_INFRA_PCIE_SEL, "infra_pcie_sel",
>		     infra_pcie_parents, 0x0028, 0x0020, 0x0024, 0, 2,
>		     -1, -1, -1),
>
>....so if you're right, we should instead have:
>
>GATE_INFRA2(CLK_INFRA_IPCIE_CK, "infra_ipcie", "infra_pcie_sel", 12),
>
>....with this meaning that adding CLK_INFRA_PCIE_SEL in devicetree is useless.
>
>This clock tree looks a bit unclear (because again, there's no datasheet around),
>but that's what I understand with a rather fast look in the clock drivers and
>with some experience on other MTK SoCs.
>
>Then again, if this tree is effectively incompatible with the one from MT8192 and
>MT8195, you should have different clock names... and just as a fast idea:
>
>clock-names = "axi", "tl", "pl", "top";
>
>with clocks, in order:
>CLK_INFRA_PCIEB_CK, CLK_INFRA_IPCIE_CK,
>CLK_INFRA_IPCIE_PIPE_CK, CLK_INFRA_IPCIER_CK.
>
>...but feel free to reiterate that :-)
>Hope that was helpful.
>
>Cheers,
>Angelo

Hi, thanks for digging into the clock-driver. Currently i have mapped it like this (see comment to  part7)

clocks = <&infracfg CLK_INFRA_IPCIE_PIPE_CK>,
				 <&infracfg CLK_INFRA_IPCIE_CK>,
				 <&clk40m>,
				 <&clk40m>,
				 <&infracfg CLK_INFRA_IPCIER_CK>,
				 <&infracfg CLK_INFRA_IPCIEB_CK>;
			clock-names = "pl_250m", "tl_26m", "tl_96m",
				 "tl_32k", "peri_26m", "top_133m";

Mtk says it has same IP block and except missing clocks it is compatible with mt8xxx gen3 pcie driver/binding. Pcie driver only enables the clocks in bulk without names,but binding requires the names property so mapping needs to be correct.

regards Frank