| Message ID | 20221017104141.7338-3-linux@fw-web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add BananaPi R3 | expand |
On 17/10/2022 06:41, Frank Wunderlich wrote: > From: Frank Wunderlich <frank-w@public-files.de> > > Add compatible string for mt7986. > > Signed-off-by: Frank Wunderlich <frank-w@public-files.de> > --- > mt7986a.dtsi misses clock-names which are now required since support of > MT8192/MT8188/MT8195. This change also introduces a 6th clock which is > now needed for all pcie-gen3 dts. > > i do not know how to map the clocks to the names... > > mediatek-pcie-gen3.yaml: > > clock-names: > items: > - const: pl_250m > - const: tl_26m > - const: tl_96m > - const: tl_32k > - const: peri_26m > - enum: > - top_133m # for MT8192 > - peri_mem # for MT8188/MT8195 > > mt7986a.dtsi: > > clocks = <&infracfg CLK_INFRA_PCIE_SEL>, > <&infracfg CLK_INFRA_IPCIE_CK>, > <&infracfg CLK_INFRA_IPCIE_PIPE_CK>, > <&infracfg CLK_INFRA_IPCIER_CK>, > <&infracfg CLK_INFRA_IPCIEB_CK>; Maybe the clock is not required on mt7986? Anyway, for the bindings: Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
Hi, > Gesendet: Dienstag, 18. Oktober 2022 um 17:35 Uhr > Von: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org> > On 17/10/2022 06:41, Frank Wunderlich wrote: > > From: Frank Wunderlich <frank-w@public-files.de> > Maybe the clock is not required on mt7986? yes, mt7986 does not have all clocks currently defined in binding for gen3-pcie (currently mt8xxx) the mapping is as followed: CLK_INFRA_IPCIER_CK: peri_26m CLK_INFRA_IPCIEB_CK: top_133m CLK_INFRA_IPCIE_CK: pcie working clock from SoC, in MT7986 it is equal to tl_26m + tl_96m + tl_32k in MT8192 CLK_INFRA_PCIE_SEL: clock mux to select source clock to CLK_INFRA_IPCIE_CK CLK_INFRA_IPCIE_PIPE_CK : pcie working clock from PHY, pl_250m as far as i see the driver only enables the clocks in bulk (no access to the clock-names), but binding needs the names got it solved with help from mtk (see my comment on part 7) with filling missing clocks with a fixed-clock-node. If this is the right way this binding-change is enough ;) > Anyway, for the bindings: > > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> thx regards Frank
Il 17/10/22 12:41, Frank Wunderlich ha scritto: > From: Frank Wunderlich <frank-w@public-files.de> > > Add compatible string for mt7986. > > Signed-off-by: Frank Wunderlich <frank-w@public-files.de> > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > --- > mt7986a.dtsi misses clock-names which are now required since support of > MT8192/MT8188/MT8195. This change also introduces a 6th clock which is > now needed for all pcie-gen3 dts. > > i do not know how to map the clocks to the names... > > mediatek-pcie-gen3.yaml: > > clock-names: > items: > - const: pl_250m > - const: tl_26m > - const: tl_96m > - const: tl_32k > - const: peri_26m > - enum: > - top_133m # for MT8192 > - peri_mem # for MT8188/MT8195 > > mt7986a.dtsi: > > clocks = <&infracfg CLK_INFRA_PCIE_SEL>, > <&infracfg CLK_INFRA_IPCIE_CK>, > <&infracfg CLK_INFRA_IPCIE_PIPE_CK>, > <&infracfg CLK_INFRA_IPCIER_CK>, > <&infracfg CLK_INFRA_IPCIEB_CK>; 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
Am 19. Oktober 2022 10:28:59 MESZ schrieb AngeloGioacchino Del Regno <angelogioacchino.delregno@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
diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml index c00be39af64e..b372c8351d9a 100644 --- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml +++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml @@ -51,6 +51,7 @@ properties: oneOf: - items: - enum: + - mediatek,mt7986-pcie - mediatek,mt8188-pcie - mediatek,mt8195-pcie - const: mediatek,mt8192-pcie