Re: [PATCH v7 4/4] arm64: dts: Add mediatek SoC mt8195 and evaluation board

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
>




[Index of Archives]     [Linux SPI]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux