| Message ID | 20240415182052.374494-1-mr.nuke.me@gmail.com |
|---|---|
| Headers | show |
| Series | ipq9574: Enable PCI-Express support | expand |
On Mon, 15 Apr 2024 at 21:21, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote: > > The IPQ9574 has four PCIe "pipe" clocks. These clocks are required by > PCIe PHYs. Port the pipe clocks from the downstream 5.4 kernel. > > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> > --- > drivers/clk/qcom/gcc-ipq9574.c | 76 ++++++++++++++++++++++++++++++++++ > 1 file changed, 76 insertions(+) Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
On Mon, 15 Apr 2024 at 21:22, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote: > > Add support for the PCIe on IPQ9574. The main difference from ipq6018 > is that the "iface" clock is not necessarry. Add a special case in > qcom_pcie_get_resources_2_9_0() to handle this. > > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> > --- > drivers/pci/controller/dwc/pcie-qcom.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 14772edcf0d3..10560d6d6336 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -1101,15 +1101,19 @@ static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie) > struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0; > struct dw_pcie *pci = pcie->pci; > struct device *dev = pci->dev; > - int ret; > + int ret, num_clks = ARRAY_SIZE(res->clks) - 1; > > - res->clks[0].id = "iface"; > + res->clks[0].id = "rchng"; > res->clks[1].id = "axi_m"; > res->clks[2].id = "axi_s"; > res->clks[3].id = "axi_bridge"; > - res->clks[4].id = "rchng"; > > - ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks); > + if (!of_device_is_compatible(dev->of_node, "qcom,pcie-ipq9574")) { > + res->clks[4].id = "iface"; > + num_clks++; > + } > + > + ret = devm_clk_bulk_get(dev, num_clks, res->clks); Just use devm_clk_bulk_get_optional() here. > if (ret < 0) > return ret; > > @@ -1664,6 +1668,7 @@ static const struct of_device_id qcom_pcie_match[] = { > { .compatible = "qcom,pcie-ipq8064-v2", .data = &cfg_2_1_0 }, > { .compatible = "qcom,pcie-ipq8074", .data = &cfg_2_3_3 }, > { .compatible = "qcom,pcie-ipq8074-gen3", .data = &cfg_2_9_0 }, > + { .compatible = "qcom,pcie-ipq9574", .data = &cfg_2_9_0 }, > { .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 }, > { .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 }, > { .compatible = "qcom,pcie-sa8540p", .data = &cfg_sc8280xp }, > -- > 2.40.1 > >
On 4/15/24 15:04, Dmitry Baryshkov wrote: > On Mon, 15 Apr 2024 at 21:22, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote: >> >> Add support for the PCIe on IPQ9574. The main difference from ipq6018 >> is that the "iface" clock is not necessarry. Add a special case in >> qcom_pcie_get_resources_2_9_0() to handle this. >> >> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> >> --- >> drivers/pci/controller/dwc/pcie-qcom.c | 13 +++++++++---- >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c >> index 14772edcf0d3..10560d6d6336 100644 >> --- a/drivers/pci/controller/dwc/pcie-qcom.c >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c >> @@ -1101,15 +1101,19 @@ static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie) >> struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0; >> struct dw_pcie *pci = pcie->pci; >> struct device *dev = pci->dev; >> - int ret; >> + int ret, num_clks = ARRAY_SIZE(res->clks) - 1; >> >> - res->clks[0].id = "iface"; >> + res->clks[0].id = "rchng"; >> res->clks[1].id = "axi_m"; >> res->clks[2].id = "axi_s"; >> res->clks[3].id = "axi_bridge"; >> - res->clks[4].id = "rchng"; >> >> - ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks); >> + if (!of_device_is_compatible(dev->of_node, "qcom,pcie-ipq9574")) { >> + res->clks[4].id = "iface"; >> + num_clks++; >> + } >> + >> + ret = devm_clk_bulk_get(dev, num_clks, res->clks); > > Just use devm_clk_bulk_get_optional() here. Thank you! I wasn't sure if this was the correct solution here. I will get this updated in v4. Alex >> if (ret < 0) >> return ret; >> >> @@ -1664,6 +1668,7 @@ static const struct of_device_id qcom_pcie_match[] = { >> { .compatible = "qcom,pcie-ipq8064-v2", .data = &cfg_2_1_0 }, >> { .compatible = "qcom,pcie-ipq8074", .data = &cfg_2_3_3 }, >> { .compatible = "qcom,pcie-ipq8074-gen3", .data = &cfg_2_9_0 }, >> + { .compatible = "qcom,pcie-ipq9574", .data = &cfg_2_9_0 }, >> { .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 }, >> { .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 }, >> { .compatible = "qcom,pcie-sa8540p", .data = &cfg_sc8280xp }, >> -- >> 2.40.1 >> >> > >
On Mon, 15 Apr 2024 at 21:23, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote: > > Add support for the gen3x2 PCIe PHY on IPQ9574, ported form downstream > 5.4 kernel. Only the serdes and pcs_misc tables are new, the others > being reused from IPQ8074 and IPQ6018 PHYs. > > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> > --- > drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 136 +++++++++++++++++- > .../phy/qualcomm/phy-qcom-qmp-pcs-pcie-v5.h | 14 ++ > 2 files changed, 149 insertions(+), 1 deletion(-) > [skipped] > @@ -2448,7 +2542,7 @@ static inline void qphy_clrbits(void __iomem *base, u32 offset, u32 val) > > /* list of clocks required by phy */ > static const char * const qmp_pciephy_clk_l[] = { > - "aux", "cfg_ahb", "ref", "refgen", "rchng", "phy_aux", > + "aux", "cfg_ahb", "ref", "refgen", "rchng", "phy_aux", "anoc", "snoc" Are the NoC clocks really necessary to drive the PHY? I think they are usually connected to the controllers, not the PHYs. > }; > > /* list of regulators */ > @@ -2499,6 +2593,16 @@ static const struct qmp_pcie_offsets qmp_pcie_offsets_v4x1 = { > .rx = 0x0400, > }; > > +static const struct qmp_pcie_offsets qmp_pcie_offsets_ipq9574 = { > + .serdes = 0, > + .pcs = 0x1000, > + .pcs_misc = 0x1400, > + .tx = 0x0200, > + .rx = 0x0400, > + .tx2 = 0x0600, > + .rx2 = 0x0800, > +}; > + > static const struct qmp_pcie_offsets qmp_pcie_offsets_v4x2 = { > .serdes = 0, > .pcs = 0x0a00, > @@ -2728,6 +2832,33 @@ static const struct qmp_phy_cfg sm8250_qmp_gen3x1_pciephy_cfg = { > .phy_status = PHYSTATUS, > }; > > +static const struct qmp_phy_cfg ipq9574_pciephy_gen3x2_cfg = { > + .lanes = 2, > + > + .offsets = &qmp_pcie_offsets_ipq9574, > + > + .tbls = { > + .serdes = ipq9574_gen3x2_pcie_serdes_tbl, > + .serdes_num = ARRAY_SIZE(ipq9574_gen3x2_pcie_serdes_tbl), > + .tx = ipq8074_pcie_gen3_tx_tbl, > + .tx_num = ARRAY_SIZE(ipq8074_pcie_gen3_tx_tbl), > + .rx = ipq6018_pcie_rx_tbl, > + .rx_num = ARRAY_SIZE(ipq6018_pcie_rx_tbl), > + .pcs = ipq6018_pcie_pcs_tbl, > + .pcs_num = ARRAY_SIZE(ipq6018_pcie_pcs_tbl), > + .pcs_misc = ipq9574_gen3x2_pcie_pcs_misc_tbl, > + .pcs_misc_num = ARRAY_SIZE(ipq9574_gen3x2_pcie_pcs_misc_tbl), > + }, > + .reset_list = ipq8074_pciephy_reset_l, > + .num_resets = ARRAY_SIZE(ipq8074_pciephy_reset_l), > + .vreg_list = NULL, > + .num_vregs = 0, > + .regs = pciephy_v4_regs_layout, So, is it v4 or v5? > + > + .pwrdn_ctrl = SW_PWRDN | REFCLK_DRV_DSBL, > + .phy_status = PHYSTATUS, > +}; > + > static const struct qmp_phy_cfg sm8250_qmp_gen3x2_pciephy_cfg = { > .lanes = 2, > -- With best wishes Dmitry
On 4/15/24 15:10, Dmitry Baryshkov wrote: > On Mon, 15 Apr 2024 at 21:23, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote: >> >> Add support for the gen3x2 PCIe PHY on IPQ9574, ported form downstream >> 5.4 kernel. Only the serdes and pcs_misc tables are new, the others >> being reused from IPQ8074 and IPQ6018 PHYs. >> >> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> >> --- >> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 136 +++++++++++++++++- >> .../phy/qualcomm/phy-qcom-qmp-pcs-pcie-v5.h | 14 ++ >> 2 files changed, 149 insertions(+), 1 deletion(-) >> > > [skipped] > >> @@ -2448,7 +2542,7 @@ static inline void qphy_clrbits(void __iomem *base, u32 offset, u32 val) >> >> /* list of clocks required by phy */ >> static const char * const qmp_pciephy_clk_l[] = { >> - "aux", "cfg_ahb", "ref", "refgen", "rchng", "phy_aux", >> + "aux", "cfg_ahb", "ref", "refgen", "rchng", "phy_aux", "anoc", "snoc" > > Are the NoC clocks really necessary to drive the PHY? I think they are > usually connected to the controllers, not the PHYs. The system will hang if these clocks are not enabled. They are also attached to the PHY in the QCA 5.4 downstream kernel. >> }; >> >> /* list of regulators */ >> @@ -2499,6 +2593,16 @@ static const struct qmp_pcie_offsets qmp_pcie_offsets_v4x1 = { >> .rx = 0x0400, >> }; >> >> +static const struct qmp_pcie_offsets qmp_pcie_offsets_ipq9574 = { >> + .serdes = 0, >> + .pcs = 0x1000, >> + .pcs_misc = 0x1400, >> + .tx = 0x0200, >> + .rx = 0x0400, >> + .tx2 = 0x0600, >> + .rx2 = 0x0800, >> +}; >> + >> static const struct qmp_pcie_offsets qmp_pcie_offsets_v4x2 = { >> .serdes = 0, >> .pcs = 0x0a00, >> @@ -2728,6 +2832,33 @@ static const struct qmp_phy_cfg sm8250_qmp_gen3x1_pciephy_cfg = { >> .phy_status = PHYSTATUS, >> }; >> >> +static const struct qmp_phy_cfg ipq9574_pciephy_gen3x2_cfg = { >> + .lanes = 2, >> + >> + .offsets = &qmp_pcie_offsets_ipq9574, >> + >> + .tbls = { >> + .serdes = ipq9574_gen3x2_pcie_serdes_tbl, >> + .serdes_num = ARRAY_SIZE(ipq9574_gen3x2_pcie_serdes_tbl), >> + .tx = ipq8074_pcie_gen3_tx_tbl, >> + .tx_num = ARRAY_SIZE(ipq8074_pcie_gen3_tx_tbl), >> + .rx = ipq6018_pcie_rx_tbl, >> + .rx_num = ARRAY_SIZE(ipq6018_pcie_rx_tbl), >> + .pcs = ipq6018_pcie_pcs_tbl, >> + .pcs_num = ARRAY_SIZE(ipq6018_pcie_pcs_tbl), >> + .pcs_misc = ipq9574_gen3x2_pcie_pcs_misc_tbl, >> + .pcs_misc_num = ARRAY_SIZE(ipq9574_gen3x2_pcie_pcs_misc_tbl), >> + }, >> + .reset_list = ipq8074_pciephy_reset_l, >> + .num_resets = ARRAY_SIZE(ipq8074_pciephy_reset_l), >> + .vreg_list = NULL, >> + .num_vregs = 0, >> + .regs = pciephy_v4_regs_layout, > > So, is it v4 or v5? Please give me a day or so to go over my notes and give you a more coherent explanation of why this versioning was chosen. I am only working from the QCA 5.4 downstream sources. I don't have any documentation for the silicon Alex > >> + >> + .pwrdn_ctrl = SW_PWRDN | REFCLK_DRV_DSBL, >> + .phy_status = PHYSTATUS, >> +}; >> + >> static const struct qmp_phy_cfg sm8250_qmp_gen3x2_pciephy_cfg = { >> .lanes = 2, >> > > > > -- > With best wishes > Dmitry
Hi Dmitry, On 4/15/24 16:25, mr.nuke.me@gmail.com wrote: > > > On 4/15/24 15:10, Dmitry Baryshkov wrote: >> On Mon, 15 Apr 2024 at 21:23, Alexandru Gagniuc <mr.nuke.me@gmail.com> >> wrote: >>> >>> Add support for the gen3x2 PCIe PHY on IPQ9574, ported form downstream >>> 5.4 kernel. Only the serdes and pcs_misc tables are new, the others >>> being reused from IPQ8074 and IPQ6018 PHYs. >>> >>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> >>> --- >>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 136 +++++++++++++++++- >>> .../phy/qualcomm/phy-qcom-qmp-pcs-pcie-v5.h | 14 ++ >>> 2 files changed, 149 insertions(+), 1 deletion(-) >>> >> >> [skipped] >> >>> @@ -2448,7 +2542,7 @@ static inline void qphy_clrbits(void __iomem >>> *base, u32 offset, u32 val) >>> >>> /* list of clocks required by phy */ >>> static const char * const qmp_pciephy_clk_l[] = { >>> - "aux", "cfg_ahb", "ref", "refgen", "rchng", "phy_aux", >>> + "aux", "cfg_ahb", "ref", "refgen", "rchng", "phy_aux", >>> "anoc", "snoc" >> >> Are the NoC clocks really necessary to drive the PHY? I think they are >> usually connected to the controllers, not the PHYs. > > The system will hang if these clocks are not enabled. They are also > attached to the PHY in the QCA 5.4 downstream kernel. > They are named "anoc_lane", and "snoc_lane" in the downstream kernel. Would you like me to use these names instead? e>>> }; >>> >>> /* list of regulators */ >>> @@ -2499,6 +2593,16 @@ static const struct qmp_pcie_offsets >>> qmp_pcie_offsets_v4x1 = { >>> .rx = 0x0400, >>> }; >>> >>> +static const struct qmp_pcie_offsets qmp_pcie_offsets_ipq9574 = { >>> + .serdes = 0, >>> + .pcs = 0x1000, >>> + .pcs_misc = 0x1400, >>> + .tx = 0x0200, >>> + .rx = 0x0400, >>> + .tx2 = 0x0600, >>> + .rx2 = 0x0800, >>> +}; >>> + >>> static const struct qmp_pcie_offsets qmp_pcie_offsets_v4x2 = { >>> .serdes = 0, >>> .pcs = 0x0a00, >>> @@ -2728,6 +2832,33 @@ static const struct qmp_phy_cfg >>> sm8250_qmp_gen3x1_pciephy_cfg = { >>> .phy_status = PHYSTATUS, >>> }; >>> >>> +static const struct qmp_phy_cfg ipq9574_pciephy_gen3x2_cfg = { >>> + .lanes = 2, >>> + >>> + .offsets = &qmp_pcie_offsets_ipq9574, >>> + >>> + .tbls = { >>> + .serdes = ipq9574_gen3x2_pcie_serdes_tbl, >>> + .serdes_num = >>> ARRAY_SIZE(ipq9574_gen3x2_pcie_serdes_tbl), >>> + .tx = ipq8074_pcie_gen3_tx_tbl, >>> + .tx_num = ARRAY_SIZE(ipq8074_pcie_gen3_tx_tbl), >>> + .rx = ipq6018_pcie_rx_tbl, >>> + .rx_num = ARRAY_SIZE(ipq6018_pcie_rx_tbl), >>> + .pcs = ipq6018_pcie_pcs_tbl, >>> + .pcs_num = ARRAY_SIZE(ipq6018_pcie_pcs_tbl), >>> + .pcs_misc = ipq9574_gen3x2_pcie_pcs_misc_tbl, >>> + .pcs_misc_num = >>> ARRAY_SIZE(ipq9574_gen3x2_pcie_pcs_misc_tbl), >>> + }, >>> + .reset_list = ipq8074_pciephy_reset_l, >>> + .num_resets = ARRAY_SIZE(ipq8074_pciephy_reset_l), >>> + .vreg_list = NULL, >>> + .num_vregs = 0, >>> + .regs = pciephy_v4_regs_layout, >> >> So, is it v4 or v5? > > Please give me a day or so to go over my notes and give you a more > coherent explanation of why this versioning was chosen. I am only > working from the QCA 5.4 downstream sources. I don't have any > documentation for the silicon The downstream QCA kernel uses the same table for ipq6018, ipq8074-gen3, and ipq9574. It is named "ipq_pciephy_gen3_regs_layout". Thus, it made sense to use the same upstream table for ipq9574, "pciephy_v4_regs_layout". As far as the register tables go, the pcs/pcs_misc are squashed into the same table in the downstream 5.4 kernel. I was able to separate the two tables because the pcs_misc registers were defined with an offset of 0x400. For example: /* QMP V2 PHY for PCIE gen3 2 Lane ports - PCS Misc registers */ #define PCS_PCIE_X2_POWER_STATE_CONFIG2 0x40c #define PCS_PCIE_X2_POWER_STATE_CONFIG4 0x414 #define PCS_PCIE_X2_ENDPOINT_REFCLK_DRIVE 0x420 #define PCS_PCIE_X2_L1P1_WAKEUP_DLY_TIME_AUXCLK_L 0x444 #define PCS_PCIE_X2_L1P1_WAKEUP_DLY_TIME_AUXCLK_H 0x448 #define PCS_PCIE_X2_L1P2_WAKEUP_DLY_TIME_AUXCLK_L 0x44c #define PCS_PCIE_X2_L1P2_WAKEUP_DLY_TIME_AUXCLK_H 0x450 ... Here, QPHY_V4_PCS_PCIE_POWER_STATE_CONFIG2 = 0xc would be correct, assuming a pcs_misc offset of 0x400. However, starting with ENDPOINT_REFCLK_DRIVE, the register would be QPHY_V4_PCS_PCIE_ENDPOINT_REFCLK_DRIVE = 0x1c. Our offsets are off-by 0x4. The existing V5 offsets, on the other hand, were all correct. For this reason, I considered that V5 is the most likely place to add the missing PCS misc definitions. Is this explanation sufficiently convincing? Where does the v4/v5 scheme in upstream kernel originate? Alex
On Wed, 17 Apr 2024 at 00:25, Alex G. <mr.nuke.me@gmail.com> wrote: > > Hi Dmitry, > > On 4/15/24 16:25, mr.nuke.me@gmail.com wrote: > > > > > > On 4/15/24 15:10, Dmitry Baryshkov wrote: > >> On Mon, 15 Apr 2024 at 21:23, Alexandru Gagniuc <mr.nuke.me@gmail.com> > >> wrote: > >>> > >>> Add support for the gen3x2 PCIe PHY on IPQ9574, ported form downstream > >>> 5.4 kernel. Only the serdes and pcs_misc tables are new, the others > >>> being reused from IPQ8074 and IPQ6018 PHYs. > >>> > >>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> > >>> --- > >>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 136 +++++++++++++++++- > >>> .../phy/qualcomm/phy-qcom-qmp-pcs-pcie-v5.h | 14 ++ > >>> 2 files changed, 149 insertions(+), 1 deletion(-) > >>> > >> > >> [skipped] > >> > >>> @@ -2448,7 +2542,7 @@ static inline void qphy_clrbits(void __iomem > >>> *base, u32 offset, u32 val) > >>> > >>> /* list of clocks required by phy */ > >>> static const char * const qmp_pciephy_clk_l[] = { > >>> - "aux", "cfg_ahb", "ref", "refgen", "rchng", "phy_aux", > >>> + "aux", "cfg_ahb", "ref", "refgen", "rchng", "phy_aux", > >>> "anoc", "snoc" > >> > >> Are the NoC clocks really necessary to drive the PHY? I think they are > >> usually connected to the controllers, not the PHYs. > > > > The system will hang if these clocks are not enabled. They are also > > attached to the PHY in the QCA 5.4 downstream kernel. Interesting. I see that Varadarajan is converting these clocks into interconnects. Maybe it's better to wait for those patches to land and use interconnects instead. I think it would better suit the infrastructure. Varadarajan, could you please comment, are these interconnects connected to the PHY too or just to the PCIe controller? > > > They are named "anoc_lane", and "snoc_lane" in the downstream kernel. > Would you like me to use these names instead? I'm fine either way. > e>>> }; > >>> > >>> /* list of regulators */ > >>> @@ -2499,6 +2593,16 @@ static const struct qmp_pcie_offsets > >>> qmp_pcie_offsets_v4x1 = { > >>> .rx = 0x0400, > >>> }; > >>> > >>> +static const struct qmp_pcie_offsets qmp_pcie_offsets_ipq9574 = { > >>> + .serdes = 0, > >>> + .pcs = 0x1000, > >>> + .pcs_misc = 0x1400, > >>> + .tx = 0x0200, > >>> + .rx = 0x0400, > >>> + .tx2 = 0x0600, > >>> + .rx2 = 0x0800, > >>> +}; > >>> + > >>> static const struct qmp_pcie_offsets qmp_pcie_offsets_v4x2 = { > >>> .serdes = 0, > >>> .pcs = 0x0a00, > >>> @@ -2728,6 +2832,33 @@ static const struct qmp_phy_cfg > >>> sm8250_qmp_gen3x1_pciephy_cfg = { > >>> .phy_status = PHYSTATUS, > >>> }; > >>> > >>> +static const struct qmp_phy_cfg ipq9574_pciephy_gen3x2_cfg = { > >>> + .lanes = 2, > >>> + > >>> + .offsets = &qmp_pcie_offsets_ipq9574, > >>> + > >>> + .tbls = { > >>> + .serdes = ipq9574_gen3x2_pcie_serdes_tbl, > >>> + .serdes_num = > >>> ARRAY_SIZE(ipq9574_gen3x2_pcie_serdes_tbl), > >>> + .tx = ipq8074_pcie_gen3_tx_tbl, > >>> + .tx_num = ARRAY_SIZE(ipq8074_pcie_gen3_tx_tbl), > >>> + .rx = ipq6018_pcie_rx_tbl, > >>> + .rx_num = ARRAY_SIZE(ipq6018_pcie_rx_tbl), > >>> + .pcs = ipq6018_pcie_pcs_tbl, > >>> + .pcs_num = ARRAY_SIZE(ipq6018_pcie_pcs_tbl), > >>> + .pcs_misc = ipq9574_gen3x2_pcie_pcs_misc_tbl, > >>> + .pcs_misc_num = > >>> ARRAY_SIZE(ipq9574_gen3x2_pcie_pcs_misc_tbl), > >>> + }, > >>> + .reset_list = ipq8074_pciephy_reset_l, > >>> + .num_resets = ARRAY_SIZE(ipq8074_pciephy_reset_l), > >>> + .vreg_list = NULL, > >>> + .num_vregs = 0, > >>> + .regs = pciephy_v4_regs_layout, > >> > >> So, is it v4 or v5? > > > > Please give me a day or so to go over my notes and give you a more > > coherent explanation of why this versioning was chosen. I am only > > working from the QCA 5.4 downstream sources. I don't have any > > documentation for the silicon > > The downstream QCA kernel uses the same table for ipq6018, ipq8074-gen3, > and ipq9574. It is named "ipq_pciephy_gen3_regs_layout". Thus, it made > sense to use the same upstream table for ipq9574, "pciephy_v4_regs_layout". > > As far as the register tables go, the pcs/pcs_misc are squashed into the > same table in the downstream 5.4 kernel. I was able to separate the two > tables because the pcs_misc registers were defined with an offset of > 0x400. For example: > > /* QMP V2 PHY for PCIE gen3 2 Lane ports - PCS Misc registers */ > #define PCS_PCIE_X2_POWER_STATE_CONFIG2 0x40c > #define PCS_PCIE_X2_POWER_STATE_CONFIG4 0x414 > #define PCS_PCIE_X2_ENDPOINT_REFCLK_DRIVE 0x420 > #define PCS_PCIE_X2_L1P1_WAKEUP_DLY_TIME_AUXCLK_L 0x444 > #define PCS_PCIE_X2_L1P1_WAKEUP_DLY_TIME_AUXCLK_H 0x448 > #define PCS_PCIE_X2_L1P2_WAKEUP_DLY_TIME_AUXCLK_L 0x44c > #define PCS_PCIE_X2_L1P2_WAKEUP_DLY_TIME_AUXCLK_H 0x450 > ... > > Here, QPHY_V4_PCS_PCIE_POWER_STATE_CONFIG2 = 0xc would be correct, > assuming a pcs_misc offset of 0x400. However, starting with > ENDPOINT_REFCLK_DRIVE, the register would be > QPHY_V4_PCS_PCIE_ENDPOINT_REFCLK_DRIVE = 0x1c. Our offsets are off-by 0x4. > > The existing V5 offsets, on the other hand, were all correct. For this > reason, I considered that V5 is the most likely place to add the missing > PCS misc definitions. Ok, sounds sane. Please use _v5 for the regs layout. > > Is this explanation sufficiently convincing? Where does the v4/v5 scheme > in upstream kernel originate? Sometimes it's vendor kernels, sometimes it's a feedback from devs that have access to actual specs. -- With best wishes Dmitry
On Mon, Apr 15, 2024 at 03:07:02PM -0500, mr.nuke.me@gmail.com wrote: > > > On 4/15/24 15:04, Dmitry Baryshkov wrote: > > On Mon, 15 Apr 2024 at 21:22, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote: > > > > > > Add support for the PCIe on IPQ9574. The main difference from ipq6018 > > > is that the "iface" clock is not necessarry. Add a special case in > > > qcom_pcie_get_resources_2_9_0() to handle this. > > > > > > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> > > > --- > > > drivers/pci/controller/dwc/pcie-qcom.c | 13 +++++++++---- > > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > > > index 14772edcf0d3..10560d6d6336 100644 > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > > > @@ -1101,15 +1101,19 @@ static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie) > > > struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0; > > > struct dw_pcie *pci = pcie->pci; > > > struct device *dev = pci->dev; > > > - int ret; > > > + int ret, num_clks = ARRAY_SIZE(res->clks) - 1; > > > > > > - res->clks[0].id = "iface"; > > > + res->clks[0].id = "rchng"; > > > res->clks[1].id = "axi_m"; > > > res->clks[2].id = "axi_s"; > > > res->clks[3].id = "axi_bridge"; > > > - res->clks[4].id = "rchng"; > > > > > > - ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks); > > > + if (!of_device_is_compatible(dev->of_node, "qcom,pcie-ipq9574")) { > > > + res->clks[4].id = "iface"; > > > + num_clks++; > > > + } > > > + > > > + ret = devm_clk_bulk_get(dev, num_clks, res->clks); > > > > Just use devm_clk_bulk_get_optional() here. > > Thank you! I wasn't sure if this was the correct solution here. I will get > this updated in v4. > Please rebase on top of [1] and mention the dependency in cover letter. - Mani [1] https://lore.kernel.org/linux-pci/20240417-pci-qcom-clk-bulk-v1-1-52ca19b3d6b2@linaro.org/ > Alex > > > > if (ret < 0) > > > return ret; > > > > > > @@ -1664,6 +1668,7 @@ static const struct of_device_id qcom_pcie_match[] = { > > > { .compatible = "qcom,pcie-ipq8064-v2", .data = &cfg_2_1_0 }, > > > { .compatible = "qcom,pcie-ipq8074", .data = &cfg_2_3_3 }, > > > { .compatible = "qcom,pcie-ipq8074-gen3", .data = &cfg_2_9_0 }, > > > + { .compatible = "qcom,pcie-ipq9574", .data = &cfg_2_9_0 }, > > > { .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 }, > > > { .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 }, > > > { .compatible = "qcom,pcie-sa8540p", .data = &cfg_sc8280xp }, > > > -- > > > 2.40.1 > > > > > > > > > >
On Mon, Apr 15, 2024 at 01:20:49PM -0500, Alexandru Gagniuc wrote: > Add support for the PCIe on IPQ9574. The main difference from ipq6018 > is that the "iface" clock is not necessarry. Add a special case in > qcom_pcie_get_resources_2_9_0() to handle this. > Could you add more information about the PCIe controller used in this SoC? Like controller version, supported data rate, PCIe generation etc... - Mani > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> > --- > drivers/pci/controller/dwc/pcie-qcom.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 14772edcf0d3..10560d6d6336 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -1101,15 +1101,19 @@ static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie) > struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0; > struct dw_pcie *pci = pcie->pci; > struct device *dev = pci->dev; > - int ret; > + int ret, num_clks = ARRAY_SIZE(res->clks) - 1; > > - res->clks[0].id = "iface"; > + res->clks[0].id = "rchng"; > res->clks[1].id = "axi_m"; > res->clks[2].id = "axi_s"; > res->clks[3].id = "axi_bridge"; > - res->clks[4].id = "rchng"; > > - ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks); > + if (!of_device_is_compatible(dev->of_node, "qcom,pcie-ipq9574")) { > + res->clks[4].id = "iface"; > + num_clks++; > + } > + > + ret = devm_clk_bulk_get(dev, num_clks, res->clks); > if (ret < 0) > return ret; > > @@ -1664,6 +1668,7 @@ static const struct of_device_id qcom_pcie_match[] = { > { .compatible = "qcom,pcie-ipq8064-v2", .data = &cfg_2_1_0 }, > { .compatible = "qcom,pcie-ipq8074", .data = &cfg_2_3_3 }, > { .compatible = "qcom,pcie-ipq8074-gen3", .data = &cfg_2_9_0 }, > + { .compatible = "qcom,pcie-ipq9574", .data = &cfg_2_9_0 }, > { .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 }, > { .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 }, > { .compatible = "qcom,pcie-sa8540p", .data = &cfg_sc8280xp }, > -- > 2.40.1 >
On Mon, Apr 15, 2024 at 01:20:52PM -0500, Alexandru Gagniuc wrote: > On ipq9574, there are 4 PCIe controllers. Describe the pcie2 node, and > its PHY in devicetree. > > Only pcie2 is described, because only hardware using that controller > was available for testing. > You should describe all the instances in DT. Since the unused ones will be 'disabled', it won't affect anyone. > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> > --- > arch/arm64/boot/dts/qcom/ipq9574.dtsi | 93 ++++++++++++++++++++++++++- > 1 file changed, 92 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > index 7f2e5cbf3bbb..f075e2715300 100644 > --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi > +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > @@ -300,7 +300,7 @@ gcc: clock-controller@1800000 { > <0>, > <0>, > <0>, > - <0>, > + <&pcie2_phy>, > <0>, > <0>; > #clock-cells = <1>; > @@ -745,6 +745,97 @@ frame@b128000 { > status = "disabled"; > }; > }; > + > + pcie2_phy: phy@8c000 { > + compatible = "qcom,ipq9574-qmp-gen3x2-pcie-phy"; > + reg = <0x0008c000 0x14f4>; > + > + clocks = <&gcc GCC_PCIE2_AUX_CLK>, > + <&gcc GCC_PCIE2_AHB_CLK>, > + <&gcc GCC_PCIE2_PIPE_CLK>, > + <&gcc GCC_ANOC_PCIE2_2LANE_M_CLK>, > + <&gcc GCC_SNOC_PCIE2_2LANE_S_CLK>; > + clock-names = "aux", > + "cfg_ahb", > + "pipe", > + "anoc", > + "snoc"; > + > + clock-output-names = "pcie_phy2_pipe_clk"; > + #clock-cells = <0>; > + #phy-cells = <0>; > + > + resets = <&gcc GCC_PCIE2_PHY_BCR>, > + <&gcc GCC_PCIE2PHY_PHY_BCR>; > + reset-names = "phy", > + "common"; > + status = "disabled"; > + }; > + > + pcie2: pcie@20000000 { > + compatible = "qcom,pcie-ipq9574"; > + reg = <0x20000000 0xf1d>, > + <0x20000f20 0xa8>, > + <0x20001000 0x1000>, > + <0x00088000 0x4000>, > + <0x20100000 0x1000>; > + reg-names = "dbi", "elbi", "atu", "parf", "config"; > + > + ranges = <0x81000000 0x0 0x20200000 0x20200000 0x0 0x00100000>, /* I/O */ Please use below range: <0x01000000 0x0 0x00000000 0x20200000 0x0 0x00100000> <0x02000000 0x0 0x20300000 0x20300000 0x0 0x07d00000> > + <0x82000000 0x0 0x20300000 0x20300000 0x0 0x07d00000>; /* MEM */ > + > + device_type = "pci"; > + linux,pci-domain = <3>; > + bus-range = <0x00 0xff>; > + num-lanes = <2>; > + max-link-speed = <3>; > + #address-cells = <3>; > + #size-cells = <2>; > + > + phys = <&pcie2_phy>; > + phy-names = "pciephy"; > + > + interrupts = <GIC_SPI 126 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "msi"; > + > + #interrupt-cells = <1>; > + interrupt-map-mask = <0 0 0 0x7>; > + interrupt-map = <0 0 0 1 &intc 0 0 164 > + IRQ_TYPE_LEVEL_HIGH>, /* int_a */ > + <0 0 0 2 &intc 0 0 165 > + IRQ_TYPE_LEVEL_HIGH>, /* int_b */ > + <0 0 0 3 &intc 0 0 186 > + IRQ_TYPE_LEVEL_HIGH>, /* int_c */ > + <0 0 0 4 &intc 0 0 187 > + IRQ_TYPE_LEVEL_HIGH>; /* int_d */ Use a single line for each INTX entry even if it exceeds 80 column width. - Mani
On 4/17/24 02:34, Manivannan Sadhasivam wrote: > On Mon, Apr 15, 2024 at 01:20:52PM -0500, Alexandru Gagniuc wrote: >> On ipq9574, there are 4 PCIe controllers. Describe the pcie2 node, and >> its PHY in devicetree. >> >> Only pcie2 is described, because only hardware using that controller >> was available for testing. >> > > You should describe all the instances in DT. Since the unused ones will be > 'disabled', it won't affect anyone. My concern with untested but "disabled" descriptions is that someone may think it's supported, try to enable it on their board, and run into issues. Theoretically, we could describe pcie3, as it uses the same gen3x2 phy. The pcie0 and pcie1 use a gen3x1 phy, which I do not support in this series. I would have to leave the "phys" and "phy-names" for these nodes, leading to an incomplete description Given this info, do you still wish that I add all other pcie nodes? >> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> >> --- >> arch/arm64/boot/dts/qcom/ipq9574.dtsi | 93 ++++++++++++++++++++++++++- >> 1 file changed, 92 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi >> index 7f2e5cbf3bbb..f075e2715300 100644 >> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi >> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi >> @@ -300,7 +300,7 @@ gcc: clock-controller@1800000 { >> <0>, >> <0>, >> <0>, >> - <0>, >> + <&pcie2_phy>, >> <0>, >> <0>; >> #clock-cells = <1>; >> @@ -745,6 +745,97 @@ frame@b128000 { >> status = "disabled"; >> }; >> }; >> + >> + pcie2_phy: phy@8c000 { >> + compatible = "qcom,ipq9574-qmp-gen3x2-pcie-phy"; >> + reg = <0x0008c000 0x14f4>; >> + >> + clocks = <&gcc GCC_PCIE2_AUX_CLK>, >> + <&gcc GCC_PCIE2_AHB_CLK>, >> + <&gcc GCC_PCIE2_PIPE_CLK>, >> + <&gcc GCC_ANOC_PCIE2_2LANE_M_CLK>, >> + <&gcc GCC_SNOC_PCIE2_2LANE_S_CLK>; >> + clock-names = "aux", >> + "cfg_ahb", >> + "pipe", >> + "anoc", >> + "snoc"; >> + >> + clock-output-names = "pcie_phy2_pipe_clk"; >> + #clock-cells = <0>; >> + #phy-cells = <0>; >> + >> + resets = <&gcc GCC_PCIE2_PHY_BCR>, >> + <&gcc GCC_PCIE2PHY_PHY_BCR>; >> + reset-names = "phy", >> + "common"; >> + status = "disabled"; >> + }; >> + >> + pcie2: pcie@20000000 { >> + compatible = "qcom,pcie-ipq9574"; >> + reg = <0x20000000 0xf1d>, >> + <0x20000f20 0xa8>, >> + <0x20001000 0x1000>, >> + <0x00088000 0x4000>, >> + <0x20100000 0x1000>; >> + reg-names = "dbi", "elbi", "atu", "parf", "config"; >> + >> + ranges = <0x81000000 0x0 0x20200000 0x20200000 0x0 0x00100000>, /* I/O */ > > Please use below range: > > <0x01000000 0x0 0x00000000 0x20200000 0x0 0x00100000> > <0x02000000 0x0 0x20300000 0x20300000 0x0 0x07d00000> > Of course, thank you. >> + <0x82000000 0x0 0x20300000 0x20300000 0x0 0x07d00000>; /* MEM */ >> + >> + device_type = "pci"; >> + linux,pci-domain = <3>; >> + bus-range = <0x00 0xff>; >> + num-lanes = <2>; >> + max-link-speed = <3>; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + >> + phys = <&pcie2_phy>; >> + phy-names = "pciephy"; >> + >> + interrupts = <GIC_SPI 126 IRQ_TYPE_LEVEL_HIGH>; >> + interrupt-names = "msi"; >> + >> + #interrupt-cells = <1>; >> + interrupt-map-mask = <0 0 0 0x7>; >> + interrupt-map = <0 0 0 1 &intc 0 0 164 >> + IRQ_TYPE_LEVEL_HIGH>, /* int_a */ >> + <0 0 0 2 &intc 0 0 165 >> + IRQ_TYPE_LEVEL_HIGH>, /* int_b */ >> + <0 0 0 3 &intc 0 0 186 >> + IRQ_TYPE_LEVEL_HIGH>, /* int_c */ >> + <0 0 0 4 &intc 0 0 187 >> + IRQ_TYPE_LEVEL_HIGH>; /* int_d */ > > Use a single line for each INTX entry even if it exceeds 80 column width. Yes. Will do. > - Mani >
On 4/15/2024 11:50 PM, Alexandru Gagniuc wrote: > There are four PCIe ports on IPQ9574, pcie0 thru pcie3. This series > addresses pcie2, which is a gen3x2 port. The board I have only uses > pcie2, and that's the only one enabled in this series. > > I believe this makes sense as a monolithic series, as the individual > pieces are not that useful by themselves. > > In v2, I've had some issues regarding the dt schema checks. For > transparency, I used the following test invocations to test v3: > > make dt_binding_check DT_SCHEMA_FILES=qcom,pcie.yaml:qcom,ipq8074-qmp-pcie-phy.yaml > make dtbs_check DT_SCHEMA_FILES=qcom,pcie.yaml:qcom,ipq8074-qmp-pcie-phy.yaml > > Alexandru, Thanks for your contributions to the Qualcomm IPQ chipsets. I would like to inform you that we have also submitted the patches to enable the PCIe support on IPQ9574[1][2] and waiting for the ICC support[3] to land to enable the NOC clocks. [1] https://lore.kernel.org/linux-arm-msm/20230519090219.15925-1-quic_devipriy@quicinc.com/ [2] https://lore.kernel.org/linux-arm-msm/20230519085723.15601-1-quic_devipriy@quicinc.com/ [3] https://lore.kernel.org/linux-arm-msm/20240418092305.2337429-1-quic_varada@quicinc.com/ Please take a look at these patches as well. Thanks, Kathiravan T. > Changes since v2: > - reworked resets in qcom,pcie.yaml to resolve dt schema errors > - constrained "reg" in qcom,pcie.yaml > - reworked min/max intems in qcom,ipq8074-qmp-pcie-phy.yaml > - dropped msi-parent for pcie node, as it is handled by "msi" IRQ > > Changes since v1: > - updated new tables in phy-qcom-qmp-pcie.c to use lowercase hex numbers > - reorganized qcom,ipq8074-qmp-pcie-phy.yaml to use a single list of clocks > - reorganized qcom,pcie.yaml to include clocks+resets per compatible > - Renamed "pcie2_qmp_phy" label to "pcie2_phy" > - moved "ranges" property of pcie@20000000 higher up > > Alexandru Gagniuc (7): > dt-bindings: clock: Add PCIe pipe related clocks for IPQ9574 > clk: qcom: gcc-ipq9574: Add PCIe pipe clocks > dt-bindings: PCI: qcom: Add IPQ9574 PCIe controller > PCI: qcom: Add support for IPQ9574 > dt-bindings: phy: qcom,ipq8074-qmp-pcie: add ipq9574 gen3x2 PHY > phy: qcom-qmp-pcie: add support for ipq9574 gen3x2 PHY > arm64: dts: qcom: ipq9574: add PCIe2 nodes > > .../devicetree/bindings/pci/qcom,pcie.yaml | 35 +++++ > .../phy/qcom,ipq8074-qmp-pcie-phy.yaml | 36 ++++- > arch/arm64/boot/dts/qcom/ipq9574.dtsi | 93 +++++++++++- > drivers/clk/qcom/gcc-ipq9574.c | 76 ++++++++++ > drivers/pci/controller/dwc/pcie-qcom.c | 13 +- > drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 136 +++++++++++++++++- > .../phy/qualcomm/phy-qcom-qmp-pcs-pcie-v5.h | 14 ++ > include/dt-bindings/clock/qcom,ipq9574-gcc.h | 4 + > 8 files changed, 400 insertions(+), 7 deletions(-) >
Hi Mani. On 4/17/24 02:06, Manivannan Sadhasivam wrote: > On Mon, Apr 15, 2024 at 03:07:02PM -0500, mr.nuke.me@gmail.com wrote: >> >> >> On 4/15/24 15:04, Dmitry Baryshkov wrote: >>> On Mon, 15 Apr 2024 at 21:22, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote: >>>> >>>> Add support for the PCIe on IPQ9574. The main difference from ipq6018 >>>> is that the "iface" clock is not necessarry. Add a special case in >>>> qcom_pcie_get_resources_2_9_0() to handle this. >>>> >>>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> >>>> --- >>>> drivers/pci/controller/dwc/pcie-qcom.c | 13 +++++++++---- >>>> 1 file changed, 9 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c >>>> index 14772edcf0d3..10560d6d6336 100644 >>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c >>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c >>>> @@ -1101,15 +1101,19 @@ static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie) >>>> struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0; >>>> struct dw_pcie *pci = pcie->pci; >>>> struct device *dev = pci->dev; >>>> - int ret; >>>> + int ret, num_clks = ARRAY_SIZE(res->clks) - 1; >>>> >>>> - res->clks[0].id = "iface"; >>>> + res->clks[0].id = "rchng"; >>>> res->clks[1].id = "axi_m"; >>>> res->clks[2].id = "axi_s"; >>>> res->clks[3].id = "axi_bridge"; >>>> - res->clks[4].id = "rchng"; >>>> >>>> - ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks); >>>> + if (!of_device_is_compatible(dev->of_node, "qcom,pcie-ipq9574")) { >>>> + res->clks[4].id = "iface"; >>>> + num_clks++; >>>> + } >>>> + >>>> + ret = devm_clk_bulk_get(dev, num_clks, res->clks); >>> >>> Just use devm_clk_bulk_get_optional() here. >> >> Thank you! I wasn't sure if this was the correct solution here. I will get >> this updated in v4. >> > > Please rebase on top of [1] and mention the dependency in cover letter. I am very hesitant to depend on another patch series. Is it okay if I include your patch in v4 of this series? Alex
Hi Kathiravan, On 4/19/24 09:28, Kathiravan Thirumoorthy wrote: > > > On 4/15/2024 11:50 PM, Alexandru Gagniuc wrote: >> There are four PCIe ports on IPQ9574, pcie0 thru pcie3. This series >> addresses pcie2, which is a gen3x2 port. The board I have only uses >> pcie2, and that's the only one enabled in this series. >> >> I believe this makes sense as a monolithic series, as the individual >> pieces are not that useful by themselves. >> >> In v2, I've had some issues regarding the dt schema checks. For >> transparency, I used the following test invocations to test v3: >> >> make dt_binding_check >> DT_SCHEMA_FILES=qcom,pcie.yaml:qcom,ipq8074-qmp-pcie-phy.yaml >> make dtbs_check >> DT_SCHEMA_FILES=qcom,pcie.yaml:qcom,ipq8074-qmp-pcie-phy.yaml >> >> > > Alexandru, > > Thanks for your contributions to the Qualcomm IPQ chipsets. > > I would like to inform you that we have also submitted the patches to > enable the PCIe support on IPQ9574[1][2] and waiting for the ICC > support[3] to land to enable the NOC clocks. > > [1] > https://lore.kernel.org/linux-arm-msm/20230519090219.15925-1-quic_devipriy@quicinc.com/ > [2] > https://lore.kernel.org/linux-arm-msm/20230519085723.15601-1-quic_devipriy@quicinc.com/ > [3] > https://lore.kernel.org/linux-arm-msm/20240418092305.2337429-1-quic_varada@quicinc.com/ > > Please take a look at these patches as well. I think I've seen [1] before -- I thought the series was abandoned. Since we have the dt-schema and applicability on mainline resolved here, do you want to use this series as the base for any new PCIe work? Alex > Thanks, > Kathiravan T. > > >> Changes since v2: >> - reworked resets in qcom,pcie.yaml to resolve dt schema errors >> - constrained "reg" in qcom,pcie.yaml >> - reworked min/max intems in qcom,ipq8074-qmp-pcie-phy.yaml >> - dropped msi-parent for pcie node, as it is handled by "msi" IRQ >> >> Changes since v1: >> - updated new tables in phy-qcom-qmp-pcie.c to use lowercase hex >> numbers >> - reorganized qcom,ipq8074-qmp-pcie-phy.yaml to use a single list of >> clocks >> - reorganized qcom,pcie.yaml to include clocks+resets per compatible >> - Renamed "pcie2_qmp_phy" label to "pcie2_phy" >> - moved "ranges" property of pcie@20000000 higher up >> >> Alexandru Gagniuc (7): >> dt-bindings: clock: Add PCIe pipe related clocks for IPQ9574 >> clk: qcom: gcc-ipq9574: Add PCIe pipe clocks >> dt-bindings: PCI: qcom: Add IPQ9574 PCIe controller >> PCI: qcom: Add support for IPQ9574 >> dt-bindings: phy: qcom,ipq8074-qmp-pcie: add ipq9574 gen3x2 PHY >> phy: qcom-qmp-pcie: add support for ipq9574 gen3x2 PHY >> arm64: dts: qcom: ipq9574: add PCIe2 nodes >> >> .../devicetree/bindings/pci/qcom,pcie.yaml | 35 +++++ >> .../phy/qcom,ipq8074-qmp-pcie-phy.yaml | 36 ++++- >> arch/arm64/boot/dts/qcom/ipq9574.dtsi | 93 +++++++++++- >> drivers/clk/qcom/gcc-ipq9574.c | 76 ++++++++++ >> drivers/pci/controller/dwc/pcie-qcom.c | 13 +- >> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 136 +++++++++++++++++- >> .../phy/qualcomm/phy-qcom-qmp-pcs-pcie-v5.h | 14 ++ >> include/dt-bindings/clock/qcom,ipq9574-gcc.h | 4 + >> 8 files changed, 400 insertions(+), 7 deletions(-) >>
Quoting Alexandru Gagniuc (2024-04-15 11:20:47) > diff --git a/drivers/clk/qcom/gcc-ipq9574.c b/drivers/clk/qcom/gcc-ipq9574.c > index 0a3f846695b8..c748d2f124f3 100644 > --- a/drivers/clk/qcom/gcc-ipq9574.c > +++ b/drivers/clk/qcom/gcc-ipq9574.c > @@ -1569,6 +1569,24 @@ static struct clk_regmap_phy_mux pcie0_pipe_clk_src = { > }, > }; > > +static struct clk_branch gcc_pcie0_pipe_clk = { > + .halt_reg = 0x28044, > + .halt_check = BRANCH_HALT_DELAY, > + .clkr = { > + .enable_reg = 0x28044, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "gcc_pcie0_pipe_clk", > + .parent_hws = (const struct clk_hw *[]) { > + &pcie0_pipe_clk_src.clkr.hw > + }, > + .num_parents = 1, > + .flags = CLK_SET_RATE_PARENT, > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > + > static struct clk_regmap_phy_mux pcie1_pipe_clk_src = { > .reg = 0x29064, > .clkr = { > @@ -1583,6 +1601,24 @@ static struct clk_regmap_phy_mux pcie1_pipe_clk_src = { > }, > }; > > +static struct clk_branch gcc_pcie1_pipe_clk = { > + .halt_reg = 0x29044, > + .halt_check = BRANCH_HALT_DELAY, > + .clkr = { > + .enable_reg = 0x29044, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ const clk_init_data please.
On Thu, Apr 18, 2024 at 10:33:23AM -0500, mr.nuke.me@gmail.com wrote: > > > On 4/17/24 02:34, Manivannan Sadhasivam wrote: > > On Mon, Apr 15, 2024 at 01:20:52PM -0500, Alexandru Gagniuc wrote: > > > On ipq9574, there are 4 PCIe controllers. Describe the pcie2 node, and > > > its PHY in devicetree. > > > > > > Only pcie2 is described, because only hardware using that controller > > > was available for testing. > > > > > > > You should describe all the instances in DT. Since the unused ones will be > > 'disabled', it won't affect anyone. > > My concern with untested but "disabled" descriptions is that someone may > think it's supported, try to enable it on their board, and run into issues. > Theoretically, we could describe pcie3, as it uses the same gen3x2 phy. > Okay. > The pcie0 and pcie1 use a gen3x1 phy, which I do not support in this series. > I would have to leave the "phys" and "phy-names" for these nodes, leading to > an incomplete description > Fine then. Please describe at least pcie3. Also add a TODO above the first pcie node mentioning that someone need to populate others too. - Mani > Given this info, do you still wish that I add all other pcie nodes? > > > > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> > > > --- > > > arch/arm64/boot/dts/qcom/ipq9574.dtsi | 93 ++++++++++++++++++++++++++- > > > 1 file changed, 92 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > > > index 7f2e5cbf3bbb..f075e2715300 100644 > > > --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi > > > +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > > > @@ -300,7 +300,7 @@ gcc: clock-controller@1800000 { > > > <0>, > > > <0>, > > > <0>, > > > - <0>, > > > + <&pcie2_phy>, > > > <0>, > > > <0>; > > > #clock-cells = <1>; > > > @@ -745,6 +745,97 @@ frame@b128000 { > > > status = "disabled"; > > > }; > > > }; > > > + > > > + pcie2_phy: phy@8c000 { > > > + compatible = "qcom,ipq9574-qmp-gen3x2-pcie-phy"; > > > + reg = <0x0008c000 0x14f4>; > > > + > > > + clocks = <&gcc GCC_PCIE2_AUX_CLK>, > > > + <&gcc GCC_PCIE2_AHB_CLK>, > > > + <&gcc GCC_PCIE2_PIPE_CLK>, > > > + <&gcc GCC_ANOC_PCIE2_2LANE_M_CLK>, > > > + <&gcc GCC_SNOC_PCIE2_2LANE_S_CLK>; > > > + clock-names = "aux", > > > + "cfg_ahb", > > > + "pipe", > > > + "anoc", > > > + "snoc"; > > > + > > > + clock-output-names = "pcie_phy2_pipe_clk"; > > > + #clock-cells = <0>; > > > + #phy-cells = <0>; > > > + > > > + resets = <&gcc GCC_PCIE2_PHY_BCR>, > > > + <&gcc GCC_PCIE2PHY_PHY_BCR>; > > > + reset-names = "phy", > > > + "common"; > > > + status = "disabled"; > > > + }; > > > + > > > + pcie2: pcie@20000000 { > > > + compatible = "qcom,pcie-ipq9574"; > > > + reg = <0x20000000 0xf1d>, > > > + <0x20000f20 0xa8>, > > > + <0x20001000 0x1000>, > > > + <0x00088000 0x4000>, > > > + <0x20100000 0x1000>; > > > + reg-names = "dbi", "elbi", "atu", "parf", "config"; > > > + > > > + ranges = <0x81000000 0x0 0x20200000 0x20200000 0x0 0x00100000>, /* I/O */ > > > > Please use below range: > > > > <0x01000000 0x0 0x00000000 0x20200000 0x0 0x00100000> > > <0x02000000 0x0 0x20300000 0x20300000 0x0 0x07d00000> > > > Of course, thank you. > > > > + <0x82000000 0x0 0x20300000 0x20300000 0x0 0x07d00000>; /* MEM */ > > > + > > > + device_type = "pci"; > > > + linux,pci-domain = <3>; > > > + bus-range = <0x00 0xff>; > > > + num-lanes = <2>; > > > + max-link-speed = <3>; > > > + #address-cells = <3>; > > > + #size-cells = <2>; > > > + > > > + phys = <&pcie2_phy>; > > > + phy-names = "pciephy"; > > > + > > > + interrupts = <GIC_SPI 126 IRQ_TYPE_LEVEL_HIGH>; > > > + interrupt-names = "msi"; > > > + > > > + #interrupt-cells = <1>; > > > + interrupt-map-mask = <0 0 0 0x7>; > > > + interrupt-map = <0 0 0 1 &intc 0 0 164 > > > + IRQ_TYPE_LEVEL_HIGH>, /* int_a */ > > > + <0 0 0 2 &intc 0 0 165 > > > + IRQ_TYPE_LEVEL_HIGH>, /* int_b */ > > > + <0 0 0 3 &intc 0 0 186 > > > + IRQ_TYPE_LEVEL_HIGH>, /* int_c */ > > > + <0 0 0 4 &intc 0 0 187 > > > + IRQ_TYPE_LEVEL_HIGH>; /* int_d */ > > > > Use a single line for each INTX entry even if it exceeds 80 column width. > > Yes. Will do. > > > - Mani > >
On Fri, Apr 19, 2024 at 02:44:36PM -0500, mr.nuke.me@gmail.com wrote: > Hi Mani. > > On 4/17/24 02:06, Manivannan Sadhasivam wrote: > > On Mon, Apr 15, 2024 at 03:07:02PM -0500, mr.nuke.me@gmail.com wrote: > > > > > > > > > On 4/15/24 15:04, Dmitry Baryshkov wrote: > > > > On Mon, 15 Apr 2024 at 21:22, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote: > > > > > > > > > > Add support for the PCIe on IPQ9574. The main difference from ipq6018 > > > > > is that the "iface" clock is not necessarry. Add a special case in > > > > > qcom_pcie_get_resources_2_9_0() to handle this. > > > > > > > > > > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> > > > > > --- > > > > > drivers/pci/controller/dwc/pcie-qcom.c | 13 +++++++++---- > > > > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > > > > > index 14772edcf0d3..10560d6d6336 100644 > > > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c > > > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > > > > > @@ -1101,15 +1101,19 @@ static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie) > > > > > struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0; > > > > > struct dw_pcie *pci = pcie->pci; > > > > > struct device *dev = pci->dev; > > > > > - int ret; > > > > > + int ret, num_clks = ARRAY_SIZE(res->clks) - 1; > > > > > > > > > > - res->clks[0].id = "iface"; > > > > > + res->clks[0].id = "rchng"; > > > > > res->clks[1].id = "axi_m"; > > > > > res->clks[2].id = "axi_s"; > > > > > res->clks[3].id = "axi_bridge"; > > > > > - res->clks[4].id = "rchng"; > > > > > > > > > > - ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks); > > > > > + if (!of_device_is_compatible(dev->of_node, "qcom,pcie-ipq9574")) { > > > > > + res->clks[4].id = "iface"; > > > > > + num_clks++; > > > > > + } > > > > > + > > > > > + ret = devm_clk_bulk_get(dev, num_clks, res->clks); > > > > > > > > Just use devm_clk_bulk_get_optional() here. > > > > > > Thank you! I wasn't sure if this was the correct solution here. I will get > > > this updated in v4. > > > > > > > Please rebase on top of [1] and mention the dependency in cover letter. > > I am very hesitant to depend on another patch series. Is it okay if I > include your patch in v4 of this series? > Feel free to. - Mani
[...] >>>> + >>>> + #interrupt-cells = <1>; >>>> + interrupt-map-mask = <0 0 0 0x7>; >>>> + interrupt-map = <0 0 0 1 &intc 0 0 164 >>>> + IRQ_TYPE_LEVEL_HIGH>, /* int_a */ >>>> + <0 0 0 2 &intc 0 0 165 >>>> + IRQ_TYPE_LEVEL_HIGH>, /* int_b */ >>>> + <0 0 0 3 &intc 0 0 186 >>>> + IRQ_TYPE_LEVEL_HIGH>, /* int_c */ >>>> + <0 0 0 4 &intc 0 0 187 >>>> + IRQ_TYPE_LEVEL_HIGH>; /* int_d */ >>> >>> Use a single line for each INTX entry even if it exceeds 80 column width. >> >> Yes. Will do. And drop the /* int_x */ comments Konrad
On 4/20/2024 1:17 AM, mr.nuke.me@gmail.com wrote: > Hi Kathiravan, > > On 4/19/24 09:28, Kathiravan Thirumoorthy wrote: >> >> >> On 4/15/2024 11:50 PM, Alexandru Gagniuc wrote: >>> There are four PCIe ports on IPQ9574, pcie0 thru pcie3. This series >>> addresses pcie2, which is a gen3x2 port. The board I have only uses >>> pcie2, and that's the only one enabled in this series. >>> >>> I believe this makes sense as a monolithic series, as the individual >>> pieces are not that useful by themselves. >>> >>> In v2, I've had some issues regarding the dt schema checks. For >>> transparency, I used the following test invocations to test v3: >>> >>> make dt_binding_check >>> DT_SCHEMA_FILES=qcom,pcie.yaml:qcom,ipq8074-qmp-pcie-phy.yaml >>> make dtbs_check >>> DT_SCHEMA_FILES=qcom,pcie.yaml:qcom,ipq8074-qmp-pcie-phy.yaml >>> >>> >> >> Alexandru, >> >> Thanks for your contributions to the Qualcomm IPQ chipsets. >> >> I would like to inform you that we have also submitted the patches to >> enable the PCIe support on IPQ9574[1][2] and waiting for the ICC >> support[3] to land to enable the NOC clocks. >> >> [1] >> https://lore.kernel.org/linux-arm-msm/20230519090219.15925-1-quic_devipriy@quicinc.com/ >> [2] >> https://lore.kernel.org/linux-arm-msm/20230519085723.15601-1-quic_devipriy@quicinc.com/ >> [3] >> https://lore.kernel.org/linux-arm-msm/20240418092305.2337429-1-quic_varada@quicinc.com/ >> >> Please take a look at these patches as well. > > I think I've seen [1] before -- I thought the series was abandoned. > Since we have the dt-schema and applicability on mainline resolved here, > do you want to use this series as the base for any new PCIe work? Sure Alex. I believe some of the code review comments are already addressed in the series which I pointed out. If you could have picked those and re-posted the next version, it could have been better. > > Alex > >> Thanks, >> Kathiravan T. >> >> >>> Changes since v2: >>> - reworked resets in qcom,pcie.yaml to resolve dt schema errors >>> - constrained "reg" in qcom,pcie.yaml >>> - reworked min/max intems in qcom,ipq8074-qmp-pcie-phy.yaml >>> - dropped msi-parent for pcie node, as it is handled by "msi" IRQ >>> >>> Changes since v1: >>> - updated new tables in phy-qcom-qmp-pcie.c to use lowercase hex >>> numbers >>> - reorganized qcom,ipq8074-qmp-pcie-phy.yaml to use a single list >>> of clocks >>> - reorganized qcom,pcie.yaml to include clocks+resets per compatible >>> - Renamed "pcie2_qmp_phy" label to "pcie2_phy" >>> - moved "ranges" property of pcie@20000000 higher up >>> >>> Alexandru Gagniuc (7): >>> dt-bindings: clock: Add PCIe pipe related clocks for IPQ9574 >>> clk: qcom: gcc-ipq9574: Add PCIe pipe clocks >>> dt-bindings: PCI: qcom: Add IPQ9574 PCIe controller >>> PCI: qcom: Add support for IPQ9574 >>> dt-bindings: phy: qcom,ipq8074-qmp-pcie: add ipq9574 gen3x2 PHY >>> phy: qcom-qmp-pcie: add support for ipq9574 gen3x2 PHY >>> arm64: dts: qcom: ipq9574: add PCIe2 nodes >>> >>> .../devicetree/bindings/pci/qcom,pcie.yaml | 35 +++++ >>> .../phy/qcom,ipq8074-qmp-pcie-phy.yaml | 36 ++++- >>> arch/arm64/boot/dts/qcom/ipq9574.dtsi | 93 +++++++++++- >>> drivers/clk/qcom/gcc-ipq9574.c | 76 ++++++++++ >>> drivers/pci/controller/dwc/pcie-qcom.c | 13 +- >>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 136 +++++++++++++++++- >>> .../phy/qualcomm/phy-qcom-qmp-pcs-pcie-v5.h | 14 ++ >>> include/dt-bindings/clock/qcom,ipq9574-gcc.h | 4 + >>> 8 files changed, 400 insertions(+), 7 deletions(-) >>>
On Wed, Apr 17, 2024 at 12:50:49AM +0300, Dmitry Baryshkov wrote: > On Wed, 17 Apr 2024 at 00:25, Alex G. <mr.nuke.me@gmail.com> wrote: > > > > Hi Dmitry, > > > > On 4/15/24 16:25, mr.nuke.me@gmail.com wrote: > > > > > > > > > On 4/15/24 15:10, Dmitry Baryshkov wrote: > > >> On Mon, 15 Apr 2024 at 21:23, Alexandru Gagniuc <mr.nuke.me@gmail.com> > > >> wrote: > > >>> > > >>> Add support for the gen3x2 PCIe PHY on IPQ9574, ported form downstream > > >>> 5.4 kernel. Only the serdes and pcs_misc tables are new, the others > > >>> being reused from IPQ8074 and IPQ6018 PHYs. > > >>> > > >>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> > > >>> --- > > >>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 136 +++++++++++++++++- > > >>> .../phy/qualcomm/phy-qcom-qmp-pcs-pcie-v5.h | 14 ++ > > >>> 2 files changed, 149 insertions(+), 1 deletion(-) > > >>> > > >> > > >> [skipped] > > >> > > >>> @@ -2448,7 +2542,7 @@ static inline void qphy_clrbits(void __iomem > > >>> *base, u32 offset, u32 val) > > >>> > > >>> /* list of clocks required by phy */ > > >>> static const char * const qmp_pciephy_clk_l[] = { > > >>> - "aux", "cfg_ahb", "ref", "refgen", "rchng", "phy_aux", > > >>> + "aux", "cfg_ahb", "ref", "refgen", "rchng", "phy_aux", > > >>> "anoc", "snoc" > > >> > > >> Are the NoC clocks really necessary to drive the PHY? I think they are > > >> usually connected to the controllers, not the PHYs. > > > > > > The system will hang if these clocks are not enabled. They are also > > > attached to the PHY in the QCA 5.4 downstream kernel. > > Interesting. > I see that Varadarajan is converting these clocks into interconnects. > Maybe it's better to wait for those patches to land and use > interconnects instead. I think it would better suit the > infrastructure. > > Varadarajan, could you please comment, are these interconnects > connected to the PHY too or just to the PCIe controller? Sorry for the late response. Missed this e-mail. These 2 clks are related to AXI port clk on Aggnoc/SNOC, not directly connected to PCIE wrapper, but it should be enabled to generate pcie traffic. Thanks Varada > > They are named "anoc_lane", and "snoc_lane" in the downstream kernel. > > Would you like me to use these names instead? > > I'm fine either way. > > > e>>> }; > > >>> > > >>> /* list of regulators */ > > >>> @@ -2499,6 +2593,16 @@ static const struct qmp_pcie_offsets > > >>> qmp_pcie_offsets_v4x1 = { > > >>> .rx = 0x0400, > > >>> }; > > >>> > > >>> +static const struct qmp_pcie_offsets qmp_pcie_offsets_ipq9574 = { > > >>> + .serdes = 0, > > >>> + .pcs = 0x1000, > > >>> + .pcs_misc = 0x1400, > > >>> + .tx = 0x0200, > > >>> + .rx = 0x0400, > > >>> + .tx2 = 0x0600, > > >>> + .rx2 = 0x0800, > > >>> +}; > > >>> + > > >>> static const struct qmp_pcie_offsets qmp_pcie_offsets_v4x2 = { > > >>> .serdes = 0, > > >>> .pcs = 0x0a00, > > >>> @@ -2728,6 +2832,33 @@ static const struct qmp_phy_cfg > > >>> sm8250_qmp_gen3x1_pciephy_cfg = { > > >>> .phy_status = PHYSTATUS, > > >>> }; > > >>> > > >>> +static const struct qmp_phy_cfg ipq9574_pciephy_gen3x2_cfg = { > > >>> + .lanes = 2, > > >>> + > > >>> + .offsets = &qmp_pcie_offsets_ipq9574, > > >>> + > > >>> + .tbls = { > > >>> + .serdes = ipq9574_gen3x2_pcie_serdes_tbl, > > >>> + .serdes_num = > > >>> ARRAY_SIZE(ipq9574_gen3x2_pcie_serdes_tbl), > > >>> + .tx = ipq8074_pcie_gen3_tx_tbl, > > >>> + .tx_num = ARRAY_SIZE(ipq8074_pcie_gen3_tx_tbl), > > >>> + .rx = ipq6018_pcie_rx_tbl, > > >>> + .rx_num = ARRAY_SIZE(ipq6018_pcie_rx_tbl), > > >>> + .pcs = ipq6018_pcie_pcs_tbl, > > >>> + .pcs_num = ARRAY_SIZE(ipq6018_pcie_pcs_tbl), > > >>> + .pcs_misc = ipq9574_gen3x2_pcie_pcs_misc_tbl, > > >>> + .pcs_misc_num = > > >>> ARRAY_SIZE(ipq9574_gen3x2_pcie_pcs_misc_tbl), > > >>> + }, > > >>> + .reset_list = ipq8074_pciephy_reset_l, > > >>> + .num_resets = ARRAY_SIZE(ipq8074_pciephy_reset_l), > > >>> + .vreg_list = NULL, > > >>> + .num_vregs = 0, > > >>> + .regs = pciephy_v4_regs_layout, > > >> > > >> So, is it v4 or v5? > > > > > > Please give me a day or so to go over my notes and give you a more > > > coherent explanation of why this versioning was chosen. I am only > > > working from the QCA 5.4 downstream sources. I don't have any > > > documentation for the silicon > > > > The downstream QCA kernel uses the same table for ipq6018, ipq8074-gen3, > > and ipq9574. It is named "ipq_pciephy_gen3_regs_layout". Thus, it made > > sense to use the same upstream table for ipq9574, "pciephy_v4_regs_layout". > > > > As far as the register tables go, the pcs/pcs_misc are squashed into the > > same table in the downstream 5.4 kernel. I was able to separate the two > > tables because the pcs_misc registers were defined with an offset of > > 0x400. For example: > > > > /* QMP V2 PHY for PCIE gen3 2 Lane ports - PCS Misc registers */ > > #define PCS_PCIE_X2_POWER_STATE_CONFIG2 0x40c > > #define PCS_PCIE_X2_POWER_STATE_CONFIG4 0x414 > > #define PCS_PCIE_X2_ENDPOINT_REFCLK_DRIVE 0x420 > > #define PCS_PCIE_X2_L1P1_WAKEUP_DLY_TIME_AUXCLK_L 0x444 > > #define PCS_PCIE_X2_L1P1_WAKEUP_DLY_TIME_AUXCLK_H 0x448 > > #define PCS_PCIE_X2_L1P2_WAKEUP_DLY_TIME_AUXCLK_L 0x44c > > #define PCS_PCIE_X2_L1P2_WAKEUP_DLY_TIME_AUXCLK_H 0x450 > > ... > > > > Here, QPHY_V4_PCS_PCIE_POWER_STATE_CONFIG2 = 0xc would be correct, > > assuming a pcs_misc offset of 0x400. However, starting with > > ENDPOINT_REFCLK_DRIVE, the register would be > > QPHY_V4_PCS_PCIE_ENDPOINT_REFCLK_DRIVE = 0x1c. Our offsets are off-by 0x4. > > > > The existing V5 offsets, on the other hand, were all correct. For this > > reason, I considered that V5 is the most likely place to add the missing > > PCS misc definitions. > > Ok, sounds sane. Please use _v5 for the regs layout. > > > > > Is this explanation sufficiently convincing? Where does the v4/v5 scheme > > in upstream kernel originate? > > Sometimes it's vendor kernels, sometimes it's a feedback from devs > that have access to actual specs. > > > -- > With best wishes > Dmitry
On Mon, 29 Apr 2024 at 09:20, Varadarajan Narayanan <quic_varada@quicinc.com> wrote: > > On Wed, Apr 17, 2024 at 12:50:49AM +0300, Dmitry Baryshkov wrote: > > On Wed, 17 Apr 2024 at 00:25, Alex G. <mr.nuke.me@gmail.com> wrote: > > > > > > Hi Dmitry, > > > > > > On 4/15/24 16:25, mr.nuke.me@gmail.com wrote: > > > > > > > > > > > > On 4/15/24 15:10, Dmitry Baryshkov wrote: > > > >> On Mon, 15 Apr 2024 at 21:23, Alexandru Gagniuc <mr.nuke.me@gmail.com> > > > >> wrote: > > > >>> > > > >>> Add support for the gen3x2 PCIe PHY on IPQ9574, ported form downstream > > > >>> 5.4 kernel. Only the serdes and pcs_misc tables are new, the others > > > >>> being reused from IPQ8074 and IPQ6018 PHYs. > > > >>> > > > >>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> > > > >>> --- > > > >>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 136 +++++++++++++++++- > > > >>> .../phy/qualcomm/phy-qcom-qmp-pcs-pcie-v5.h | 14 ++ > > > >>> 2 files changed, 149 insertions(+), 1 deletion(-) > > > >>> > > > >> > > > >> [skipped] > > > >> > > > >>> @@ -2448,7 +2542,7 @@ static inline void qphy_clrbits(void __iomem > > > >>> *base, u32 offset, u32 val) > > > >>> > > > >>> /* list of clocks required by phy */ > > > >>> static const char * const qmp_pciephy_clk_l[] = { > > > >>> - "aux", "cfg_ahb", "ref", "refgen", "rchng", "phy_aux", > > > >>> + "aux", "cfg_ahb", "ref", "refgen", "rchng", "phy_aux", > > > >>> "anoc", "snoc" > > > >> > > > >> Are the NoC clocks really necessary to drive the PHY? I think they are > > > >> usually connected to the controllers, not the PHYs. > > > > > > > > The system will hang if these clocks are not enabled. They are also > > > > attached to the PHY in the QCA 5.4 downstream kernel. > > > > Interesting. > > I see that Varadarajan is converting these clocks into interconnects. > > Maybe it's better to wait for those patches to land and use > > interconnects instead. I think it would better suit the > > infrastructure. > > > > Varadarajan, could you please comment, are these interconnects > > connected to the PHY too or just to the PCIe controller? > > Sorry for the late response. Missed this e-mail. > > These 2 clks are related to AXI port clk on Aggnoc/SNOC, not > directly connected to PCIE wrapper, but it should be enabled to > generate pcie traffic. So, are they required for the PHY or are they required for the PCIe controller only? > > Thanks > Varada > > > > They are named "anoc_lane", and "snoc_lane" in the downstream kernel. > > > Would you like me to use these names instead? > > > > I'm fine either way. > >
On Mon, Apr 29, 2024 at 01:55:32PM +0300, Dmitry Baryshkov wrote: > On Mon, 29 Apr 2024 at 09:20, Varadarajan Narayanan > <quic_varada@quicinc.com> wrote: > > > > On Wed, Apr 17, 2024 at 12:50:49AM +0300, Dmitry Baryshkov wrote: > > > On Wed, 17 Apr 2024 at 00:25, Alex G. <mr.nuke.me@gmail.com> wrote: > > > > > > > > Hi Dmitry, > > > > > > > > On 4/15/24 16:25, mr.nuke.me@gmail.com wrote: > > > > > > > > > > > > > > > On 4/15/24 15:10, Dmitry Baryshkov wrote: > > > > >> On Mon, 15 Apr 2024 at 21:23, Alexandru Gagniuc <mr.nuke.me@gmail.com> > > > > >> wrote: > > > > >>> > > > > >>> Add support for the gen3x2 PCIe PHY on IPQ9574, ported form downstream > > > > >>> 5.4 kernel. Only the serdes and pcs_misc tables are new, the others > > > > >>> being reused from IPQ8074 and IPQ6018 PHYs. > > > > >>> > > > > >>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> > > > > >>> --- > > > > >>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 136 +++++++++++++++++- > > > > >>> .../phy/qualcomm/phy-qcom-qmp-pcs-pcie-v5.h | 14 ++ > > > > >>> 2 files changed, 149 insertions(+), 1 deletion(-) > > > > >>> > > > > >> > > > > >> [skipped] > > > > >> > > > > >>> @@ -2448,7 +2542,7 @@ static inline void qphy_clrbits(void __iomem > > > > >>> *base, u32 offset, u32 val) > > > > >>> > > > > >>> /* list of clocks required by phy */ > > > > >>> static const char * const qmp_pciephy_clk_l[] = { > > > > >>> - "aux", "cfg_ahb", "ref", "refgen", "rchng", "phy_aux", > > > > >>> + "aux", "cfg_ahb", "ref", "refgen", "rchng", "phy_aux", > > > > >>> "anoc", "snoc" > > > > >> > > > > >> Are the NoC clocks really necessary to drive the PHY? I think they are > > > > >> usually connected to the controllers, not the PHYs. > > > > > > > > > > The system will hang if these clocks are not enabled. They are also > > > > > attached to the PHY in the QCA 5.4 downstream kernel. > > > > > > Interesting. > > > I see that Varadarajan is converting these clocks into interconnects. > > > Maybe it's better to wait for those patches to land and use > > > interconnects instead. I think it would better suit the > > > infrastructure. > > > > > > Varadarajan, could you please comment, are these interconnects > > > connected to the PHY too or just to the PCIe controller? > > > > Sorry for the late response. Missed this e-mail. > > > > These 2 clks are related to AXI port clk on Aggnoc/SNOC, not > > directly connected to PCIE wrapper, but it should be enabled to > > generate pcie traffic. > > So, are they required for the PHY or are they required for the PCIe > controller only? These 2 clks are required for PCIe controller only. PCIE controller need these clks to send/receive axi pkts. Thanks Varada > > > > They are named "anoc_lane", and "snoc_lane" in the downstream kernel. > > > > Would you like me to use these names instead? > > > > > > I'm fine either way. > > > > > > > -- > With best wishes > Dmitry
On 4/30/24 1:31 AM, Varadarajan Narayanan wrote: > On Mon, Apr 29, 2024 at 01:55:32PM +0300, Dmitry Baryshkov wrote: >> On Mon, 29 Apr 2024 at 09:20, Varadarajan Narayanan >> <quic_varada@quicinc.com> wrote: >>> >>> On Wed, Apr 17, 2024 at 12:50:49AM +0300, Dmitry Baryshkov wrote: >>>> On Wed, 17 Apr 2024 at 00:25, Alex G. <mr.nuke.me@gmail.com> wrote: >>>>> >>>>> Hi Dmitry, >>>>> >>>>> On 4/15/24 16:25, mr.nuke.me@gmail.com wrote: >>>>>> >>>>>> >>>>>> On 4/15/24 15:10, Dmitry Baryshkov wrote: >>>>>>> On Mon, 15 Apr 2024 at 21:23, Alexandru Gagniuc <mr.nuke.me@gmail.com> >>>>>>> wrote: >>>>>>>> >>>>>>>> Add support for the gen3x2 PCIe PHY on IPQ9574, ported form downstream >>>>>>>> 5.4 kernel. Only the serdes and pcs_misc tables are new, the others >>>>>>>> being reused from IPQ8074 and IPQ6018 PHYs. >>>>>>>> >>>>>>>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> >>>>>>>> --- >>>>>>>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 136 +++++++++++++++++- >>>>>>>> .../phy/qualcomm/phy-qcom-qmp-pcs-pcie-v5.h | 14 ++ >>>>>>>> 2 files changed, 149 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>> >>>>>>> [skipped] >>>>>>> >>>>>>>> @@ -2448,7 +2542,7 @@ static inline void qphy_clrbits(void __iomem >>>>>>>> *base, u32 offset, u32 val) >>>>>>>> >>>>>>>> /* list of clocks required by phy */ >>>>>>>> static const char * const qmp_pciephy_clk_l[] = { >>>>>>>> - "aux", "cfg_ahb", "ref", "refgen", "rchng", "phy_aux", >>>>>>>> + "aux", "cfg_ahb", "ref", "refgen", "rchng", "phy_aux", >>>>>>>> "anoc", "snoc" >>>>>>> >>>>>>> Are the NoC clocks really necessary to drive the PHY? I think they are >>>>>>> usually connected to the controllers, not the PHYs. >>>>>> >>>>>> The system will hang if these clocks are not enabled. They are also >>>>>> attached to the PHY in the QCA 5.4 downstream kernel. >>>> >>>> Interesting. >>>> I see that Varadarajan is converting these clocks into interconnects. >>>> Maybe it's better to wait for those patches to land and use >>>> interconnects instead. I think it would better suit the >>>> infrastructure. >>>> >>>> Varadarajan, could you please comment, are these interconnects >>>> connected to the PHY too or just to the PCIe controller? >>> >>> Sorry for the late response. Missed this e-mail. >>> >>> These 2 clks are related to AXI port clk on Aggnoc/SNOC, not >>> directly connected to PCIE wrapper, but it should be enabled to >>> generate pcie traffic. >> >> So, are they required for the PHY or are they required for the PCIe >> controller only? > > These 2 clks are required for PCIe controller only. > PCIE controller need these clks to send/receive axi pkts. Very interesting information, thank you! Dmitry, In light of this information do you want me to move these clocks out of the PHY and into the PCIe controller? Alex > Thanks > Varada > >>>>> They are named "anoc_lane", and "snoc_lane" in the downstream kernel. >>>>> Would you like me to use these names instead? >>>> >>>> I'm fine either way. >>>> >> >> >> >> -- >> With best wishes >> Dmitry
On Wed, 1 May 2024 at 00:51, <mr.nuke.me@gmail.com> wrote: > > On 4/30/24 1:31 AM, Varadarajan Narayanan wrote: > > On Mon, Apr 29, 2024 at 01:55:32PM +0300, Dmitry Baryshkov wrote: > >> On Mon, 29 Apr 2024 at 09:20, Varadarajan Narayanan > >> <quic_varada@quicinc.com> wrote: > >>> > >>> On Wed, Apr 17, 2024 at 12:50:49AM +0300, Dmitry Baryshkov wrote: > >>>> On Wed, 17 Apr 2024 at 00:25, Alex G. <mr.nuke.me@gmail.com> wrote: > >>>>> > >>>>> Hi Dmitry, > >>>>> > >>>>> On 4/15/24 16:25, mr.nuke.me@gmail.com wrote: > >>>>>> > >>>>>> > >>>>>> On 4/15/24 15:10, Dmitry Baryshkov wrote: > >>>>>>> On Mon, 15 Apr 2024 at 21:23, Alexandru Gagniuc <mr.nuke.me@gmail.com> > >>>>>>> wrote: > >>>>>>>> > >>>>>>>> Add support for the gen3x2 PCIe PHY on IPQ9574, ported form downstream > >>>>>>>> 5.4 kernel. Only the serdes and pcs_misc tables are new, the others > >>>>>>>> being reused from IPQ8074 and IPQ6018 PHYs. > >>>>>>>> > >>>>>>>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> > >>>>>>>> --- > >>>>>>>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 136 +++++++++++++++++- > >>>>>>>> .../phy/qualcomm/phy-qcom-qmp-pcs-pcie-v5.h | 14 ++ > >>>>>>>> 2 files changed, 149 insertions(+), 1 deletion(-) > >>>>>>>> > >>>>>>> > >>>>>>> [skipped] > >>>>>>> > >>>>>>>> @@ -2448,7 +2542,7 @@ static inline void qphy_clrbits(void __iomem > >>>>>>>> *base, u32 offset, u32 val) > >>>>>>>> > >>>>>>>> /* list of clocks required by phy */ > >>>>>>>> static const char * const qmp_pciephy_clk_l[] = { > >>>>>>>> - "aux", "cfg_ahb", "ref", "refgen", "rchng", "phy_aux", > >>>>>>>> + "aux", "cfg_ahb", "ref", "refgen", "rchng", "phy_aux", > >>>>>>>> "anoc", "snoc" > >>>>>>> > >>>>>>> Are the NoC clocks really necessary to drive the PHY? I think they are > >>>>>>> usually connected to the controllers, not the PHYs. > >>>>>> > >>>>>> The system will hang if these clocks are not enabled. They are also > >>>>>> attached to the PHY in the QCA 5.4 downstream kernel. > >>>> > >>>> Interesting. > >>>> I see that Varadarajan is converting these clocks into interconnects. > >>>> Maybe it's better to wait for those patches to land and use > >>>> interconnects instead. I think it would better suit the > >>>> infrastructure. > >>>> > >>>> Varadarajan, could you please comment, are these interconnects > >>>> connected to the PHY too or just to the PCIe controller? > >>> > >>> Sorry for the late response. Missed this e-mail. > >>> > >>> These 2 clks are related to AXI port clk on Aggnoc/SNOC, not > >>> directly connected to PCIE wrapper, but it should be enabled to > >>> generate pcie traffic. > >> > >> So, are they required for the PHY or are they required for the PCIe > >> controller only? > > > > These 2 clks are required for PCIe controller only. > > PCIE controller need these clks to send/receive axi pkts. > > Very interesting information, thank you! > > Dmitry, In light of this information do you want me to move these clocks > out of the PHY and into the PCIe controller? That's what I was thinking about. > > Alex > > > Thanks > > Varada > > > >>>>> They are named "anoc_lane", and "snoc_lane" in the downstream kernel. > >>>>> Would you like me to use these names instead? > >>>> > >>>> I'm fine either way. > >>>> > >> > >> > >> > >> -- > >> With best wishes > >> Dmitry
There are four PCIe ports on IPQ9574, pcie0 thru pcie3. This series addresses pcie2, which is a gen3x2 port. The board I have only uses pcie2, and that's the only one enabled in this series. I believe this makes sense as a monolithic series, as the individual pieces are not that useful by themselves. In v2, I've had some issues regarding the dt schema checks. For transparency, I used the following test invocations to test v3: make dt_binding_check DT_SCHEMA_FILES=qcom,pcie.yaml:qcom,ipq8074-qmp-pcie-phy.yaml make dtbs_check DT_SCHEMA_FILES=qcom,pcie.yaml:qcom,ipq8074-qmp-pcie-phy.yaml Changes since v2: - reworked resets in qcom,pcie.yaml to resolve dt schema errors - constrained "reg" in qcom,pcie.yaml - reworked min/max intems in qcom,ipq8074-qmp-pcie-phy.yaml - dropped msi-parent for pcie node, as it is handled by "msi" IRQ Changes since v1: - updated new tables in phy-qcom-qmp-pcie.c to use lowercase hex numbers - reorganized qcom,ipq8074-qmp-pcie-phy.yaml to use a single list of clocks - reorganized qcom,pcie.yaml to include clocks+resets per compatible - Renamed "pcie2_qmp_phy" label to "pcie2_phy" - moved "ranges" property of pcie@20000000 higher up Alexandru Gagniuc (7): dt-bindings: clock: Add PCIe pipe related clocks for IPQ9574 clk: qcom: gcc-ipq9574: Add PCIe pipe clocks dt-bindings: PCI: qcom: Add IPQ9574 PCIe controller PCI: qcom: Add support for IPQ9574 dt-bindings: phy: qcom,ipq8074-qmp-pcie: add ipq9574 gen3x2 PHY phy: qcom-qmp-pcie: add support for ipq9574 gen3x2 PHY arm64: dts: qcom: ipq9574: add PCIe2 nodes .../devicetree/bindings/pci/qcom,pcie.yaml | 35 +++++ .../phy/qcom,ipq8074-qmp-pcie-phy.yaml | 36 ++++- arch/arm64/boot/dts/qcom/ipq9574.dtsi | 93 +++++++++++- drivers/clk/qcom/gcc-ipq9574.c | 76 ++++++++++ drivers/pci/controller/dwc/pcie-qcom.c | 13 +- drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 136 +++++++++++++++++- .../phy/qualcomm/phy-qcom-qmp-pcs-pcie-v5.h | 14 ++ include/dt-bindings/clock/qcom,ipq9574-gcc.h | 4 + 8 files changed, 400 insertions(+), 7 deletions(-)