On Thu, Jan 6, 2022 at 7:15 PM Tinghan Shen <tinghan.shen@xxxxxxxxxxxx> wrote:
> On Thu, 2021-12-23 at 17:59 +0800, Chen-Yu Tsai wrote:
> > On Mon, Dec 20, 2021 at 8:20 PM Tinghan Shen <tinghan.shen@xxxxxxxxxxxx> wrote:
[...]
> > [...]
> >
> > > + xhci0: usb@11200000 {
> > > + compatible = "mediatek,mt8195-xhci",
> > > + "mediatek,mtk-xhci";
> > > + reg = <0 0x11200000 0 0x1000>,
> > > + <0 0x11203e00 0 0x0100>;
> > > + reg-names = "mac", "ippc";
> > > + interrupts = <GIC_SPI 129
> > > IRQ_TYPE_LEVEL_HIGH 0>;
> > > + phys = <&u2port0 PHY_TYPE_USB2>,
> > > + <&u3port0 PHY_TYPE_USB3>;
> > > + assigned-clocks = <&topckgen
> > > CLK_TOP_USB_TOP>,
> > > + <&topckgen
> > > CLK_TOP_SSUSB_XHCI>;
> > > + assigned-clock-parents = <&topckgen
> > > CLK_TOP_UNIVPLL_D5_D4>,
> > > + <&topckgen
> > > CLK_TOP_UNIVPLL_D5_D4>;
> > > + clocks = <&infracfg_ao CLK_INFRA_AO_SSUSB>,
> > > + <&infracfg_ao
> > > CLK_INFRA_AO_SSUSB_XHCI>,
> > > + <&topckgen CLK_TOP_SSUSB_REF>,
> > > + <&apmixedsys CLK_APMIXED_USB1PLL>;
> > > + clock-names = "sys_ck", "xhci_ck",
> > > "ref_ck", "mcu_ck";
> >
> > The binding for this needs to be fixed. It expects clocks in the
> > order
> > specified in the binding, and this doesn't match.
>
> ok
>
> > Also, "dma_ck" is missing
> > and will likely cause warnings to be generated.
>
> only sys_ck is required, others are optional as described in binding
I understand, but the bindings language is somewhat limited and right now
is written in a way that if "dma_ck" is missing it would fail the DT
bindings check.
> >
> > This goes for all the xhci device nodes.
> >
> > > + status = "disabled";
> > > + };
> > > +
> > > + mmc0: mmc@11230000 {
> > > + compatible = "mediatek,mt8195-mmc",
> > > + "mediatek,mt8183-mmc";
> > > + reg = <0 0x11230000 0 0x10000>,
> > > + <0 0x11f50000 0 0x1000>;
> >
> > The binding only allows one entry. Please fix the binding first.
> > This was added with MT8183, and the fix should list the relavent
> > commit.
> >
> > > + interrupts = <GIC_SPI 131
> > > IRQ_TYPE_LEVEL_HIGH 0>;
> > > + clocks = <&topckgen CLK_TOP_MSDC50_0>,
> > > + <&infracfg_ao CLK_INFRA_AO_MSDC0>,
> > > + <&infracfg_ao
> > > CLK_INFRA_AO_MSDC0_SRC>;
> > > + clock-names = "source", "hclk",
> > > "source_cg";
> > > + status = "disabled";
> > > + };
> > > +
> >
> > [...]
> >
> > > +
> > > + xhci1: usb@11290000 {
> > > + compatible = "mediatek,mt8195-xhci",
> > > + "mediatek,mtk-xhci";
> > > + reg = <0 0x11290000 0 0x1000>,
> > > + <0 0x11293e00 0 0x0100>;
> > > + reg-names = "mac", "ippc";
> > > + interrupts = <GIC_SPI 530
> > > IRQ_TYPE_LEVEL_HIGH 0>;
> > > + phys = <&u2port1 PHY_TYPE_USB2>;
> >
> > Shouldn't there be a USB3 phy?
>
> currently only enable usb2, usb3 phy is used by pcie.
Got it.
> >
> > > + assigned-clocks = <&topckgen
> > > CLK_TOP_USB_TOP_1P>,
> > > + <&topckgen
> > > CLK_TOP_SSUSB_XHCI_1P>;
> > > + assigned-clock-parents = <&topckgen
> > > CLK_TOP_UNIVPLL_D5_D4>,
> > > + <&topckgen
> > > CLK_TOP_UNIVPLL_D5_D4>;
> > > + clocks = <&pericfg_ao
> > > CLK_PERI_AO_SSUSB_1P_BUS>,
> > > + <&topckgen CLK_TOP_SSUSB_P1_REF>,
> > > + <&pericfg_ao
> > > CLK_PERI_AO_SSUSB_1P_XHCI>,
> > > + <&apmixedsys CLK_APMIXED_USB1PLL>;
> > > + clock-names = "sys_ck", "ref_ck",
> > > "xhci_ck", "mcu_ck";
> > > + status = "disabled";
> > > + };
> > > +
> > > + xhci2: usb@112a0000 {
> > > + compatible = "mediatek,mt8195-xhci",
> > > + "mediatek,mtk-xhci";
> > > + reg = <0 0x112a0000 0 0x1000>,
> > > + <0 0x112a3e00 0 0x0100>;
> > > + reg-names = "mac", "ippc";
> > > + interrupts = <GIC_SPI 533
> > > IRQ_TYPE_LEVEL_HIGH 0>;
> > > + phys = <&u2port2 PHY_TYPE_USB2>;
> > > + assigned-clocks = <&topckgen
> > > CLK_TOP_USB_TOP_2P>,
> > > + <&topckgen
> > > CLK_TOP_SSUSB_XHCI_2P>;
> > > + assigned-clock-parents = <&topckgen
> > > CLK_TOP_UNIVPLL_D5_D4>,
> > > + <&topckgen
> > > CLK_TOP_UNIVPLL_D5_D4>;
> > > + clocks = <&pericfg_ao
> > > CLK_PERI_AO_SSUSB_2P_BUS>,
> > > + <&topckgen CLK_TOP_SSUSB_P2_REF>,
> > > + <&pericfg_ao
> > > CLK_PERI_AO_SSUSB_2P_XHCI>;
> > > + clock-names = "sys_ck", "ref_ck",
> > > "xhci_ck";
> > > + status = "disabled";
> > > + };
> > > +
> > > + xhci3: usb@112b0000 {
> > > + compatible = "mediatek,mt8195-xhci",
> > > + "mediatek,mtk-xhci";
> > > + reg = <0 0x112b0000 0 0x1000>,
> > > + <0 0x112b3e00 0 0x0100>;
> > > + reg-names = "mac", "ippc";
> > > + interrupts = <GIC_SPI 536
> > > IRQ_TYPE_LEVEL_HIGH 0>;
> > > + phys = <&u2port3 PHY_TYPE_USB2>;
> > > + assigned-clocks = <&topckgen
> > > CLK_TOP_USB_TOP_3P>,
> > > + <&topckgen
> > > CLK_TOP_SSUSB_XHCI_3P>;
> > > + assigned-clock-parents = <&topckgen
> > > CLK_TOP_UNIVPLL_D5_D4>,
> > > + <&topckgen
> > > CLK_TOP_UNIVPLL_D5_D4>;
> > > + clocks = <&pericfg_ao
> > > CLK_PERI_AO_SSUSB_3P_BUS>,
> > > + <&pericfg_ao
> > > CLK_PERI_AO_SSUSB_3P_XHCI>,
> > > + <&topckgen CLK_TOP_SSUSB_P3_REF>;
> > > + clock-names = "sys_ck", "xhci_ck",
> > > "ref_ck";
> > > + usb2-lpm-disable;
> >
> > Could you explain why this is needed only for this controller?
>
> This controller is fixed with a BT, there is something issue when
> enable usb2 lpm, so just disabled tmp.
Please add a comment explaining things.
> > > + status = "disabled";
> > > + };
> > > +
> > > + u3phy2: t-phy@11c40000 {
> >
> > Just "phy" for the node name. (Or maybe "serdes".) t-phy is not
> > generic.
>
> following t-phy’s dt-binding.
> here using t-phy is to avoid dt-check warning, because it has some sub-
> phys.
I see. t-phy it is, then.
> > > + compatible = "mediatek,mt8195-tphy",
> > > "mediatek,generic-tphy-v3";
> > > + #address-cells = <1>;
> > > + #size-cells = <1>;
> > > + ranges = <0 0 0x11c40000 0x700>;
> > > + status = "disabled";
> > > +
> > > + u2port2: usb-phy@0 {
> > > + reg = <0x0 0x700>;
> > > + clocks = <&topckgen
> > > CLK_TOP_SSUSB_PHY_P2_REF>;
> > > + clock-names = "ref";
> > > + #phy-cells = <1>;
> > > + };
> > > + };
> > > +
> >
> > [...]
> >
> > > + ufsphy: ufs-phy@11fa0000 {
> >
> > I would have preferred "phy" for the device node, but this seems
> > already
> > defined in the binding.
> >
> > This IP block is not listed in the datasheet I have, so I am unable
> > to
> > verify the properties listed here.
> >
> > > + compatible = "mediatek,mt8195-ufsphy",
> > > "mediatek,mt8183-ufsphy";
> > > + reg = <0 0x11fa0000 0 0xc000>;
> > > + clocks = <&clk26m>, <&clk26m>;
> > > + clock-names = "unipro", "mp";
> > > + #phy-cells = <0>;
> > > + status = "disabled";
> > > + };
> > > +
> >
> > Most of the issues I raised in this version were issues with things
> > not
> > matching the bindings. Please apply your patches on -next and run
> > `make dtbs_check`.
>
> ok. I'll apply comments at next version. Thank you.
>
> >
> >
> > ChenYu
>
