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

  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>;
---
 Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml | 1 +
 1 file changed, 1 insertion(+)
  

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