| Message ID | 20230119112453.3393911-1-abel.vesa@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/2] dt-bindings: PCI: qcom: Add SM8550 compatible | expand |
On 19/01/2023 12:24, Abel Vesa wrote: > Add the SM8550 platform to the binding. > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > > The v2 was here: > https://lore.kernel.org/all/20230118111704.3553542-1-abel.vesa@linaro.org/ > > Changes since v2: > * dropped the pipe from clock-names > * removed the pcie instance number from aggre clock-names comment > * renamed aggre clock-names to noc_aggr > * dropped the _pcie infix from cnoc_pcie_sf_axi > * renamed pcie_1_link_down_reset to simply link_down > * added enable-gpios back, since pcie1 node will use it > > Changes since v1: > * Switched to single compatible for both PCIes (qcom,pcie-sm8550) > * dropped enable-gpios property > * dropped interconnects related properties, the power-domains > * properties > and resets related properties the sm8550 specific allOf:if:then > * dropped pipe_mux, phy_pipe and ref clocks from the sm8550 specific > allOf:if:then clock-names array and decreased the minItems and > maxItems for clocks property accordingly > * added "minItems: 1" to interconnects, since sm8550 pcie uses just one, > same for interconnect-names > > > .../devicetree/bindings/pci/qcom,pcie.yaml | 44 +++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml > index a5859bb3dc28..93e86dfdd6fe 100644 > --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml > @@ -34,6 +34,7 @@ properties: > - qcom,pcie-sm8250 > - qcom,pcie-sm8450-pcie0 > - qcom,pcie-sm8450-pcie1 > + - qcom,pcie-sm8550 > - qcom,pcie-ipq6018 > > reg: > @@ -65,9 +66,11 @@ properties: > dma-coherent: true > > interconnects: > + minItems: 1 > maxItems: 2 1. Why do you skip cpu-pcie interconnect on SM8550? 2. This should not be allowed on other variants. > > interconnect-names: > + minItems: 1 > items: > - const: pcie-mem > - const: cpu-pcie > @@ -102,6 +105,10 @@ properties: > power-domains: > maxItems: 1 > > + enable-gpios: > + description: GPIO controlled connection to ENABLE# signal > + maxItems: 1 > + > perst-gpios: > description: GPIO controlled connection to PERST# signal > maxItems: 1 > @@ -197,6 +204,7 @@ allOf: > - qcom,pcie-sm8250 > - qcom,pcie-sm8450-pcie0 > - qcom,pcie-sm8450-pcie1 > + - qcom,pcie-sm8550 > then: > properties: > reg: > @@ -611,6 +619,41 @@ allOf: > items: > - const: pci # PCIe core reset > > + - if: > + properties: > + compatible: > + contains: > + enum: > + - qcom,pcie-sm8550 > + then: > + properties: > + clocks: > + minItems: 7 > + maxItems: 8 > + clock-names: > + minItems: 7 > + items: > + - const: aux # Auxiliary clock > + - const: cfg # Configuration clock > + - const: bus_master # Master AXI clock > + - const: bus_slave # Slave AXI clock > + - const: slave_q2a # Slave Q2A clock > + - const: ddrss_sf_tbu # PCIe SF TBU clock > + - const: noc_aggr # Aggre NoC PCIe AXI clock > + - const: cnoc_sf_axi # Config NoC PCIe1 AXI clock > + iommus: > + maxItems: 1 > + iommu-map: > + maxItems: 2 1. Don't define new properties in allOf. It makes the binding unmaintainable. 2. Why only SM8550? > + resets: > + minItems: 1 Why second reset is optional? > + maxItems: 2 > + reset-names: > + minItems: 1 > + items: > + - const: pci # PCIe core reset > + - const: link_down # PCIe link down reset > + > - if: > properties: > compatible: > @@ -694,6 +737,7 @@ allOf: > - qcom,pcie-sm8250 > - qcom,pcie-sm8450-pcie0 > - qcom,pcie-sm8450-pcie1 > + - qcom,pcie-sm8550 > then: > oneOf: > - properties: Best regards, Krzysztof
On 19/01/2023 12:24, Abel Vesa wrote: > Add compatible for both PCIe found on SM8550. > Also add the cnoc_pcie_sf_axi clock needed by the SM8550. > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > > The v2 was here: > https://lore.kernel.org/all/20230118111704.3553542-2-abel.vesa@linaro.org/ > > Changes since v2: > * none > > Changes since v1: > * changed the subject line prefix for the patch to match the history, > like Bjorn Helgaas suggested. > * added Konrad's R-b tag > > > drivers/pci/controller/dwc/pcie-qcom.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 77e5dc7b88ad..85988b3fd4f6 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -182,7 +182,7 @@ struct qcom_pcie_resources_2_3_3 { > > /* 6 clocks typically, 7 for sm8250 */ > struct qcom_pcie_resources_2_7_0 { > - struct clk_bulk_data clks[12]; > + struct clk_bulk_data clks[13]; > int num_clks; > struct regulator_bulk_data supplies[2]; > struct reset_control *pci_reset; > @@ -1208,6 +1208,7 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie) > res->clks[idx++].id = "noc_aggr_4"; > res->clks[idx++].id = "noc_aggr_south_sf"; > res->clks[idx++].id = "cnoc_qx"; > + res->clks[idx++].id = "cnoc_pcie_sf_axi"; > > num_opt_clks = idx - num_clks; > res->num_clks = idx; > @@ -1828,6 +1829,7 @@ static const struct of_device_id qcom_pcie_match[] = { > { .compatible = "qcom,pcie-sm8250", .data = &cfg_1_9_0 }, > { .compatible = "qcom,pcie-sm8450-pcie0", .data = &cfg_1_9_0 }, > { .compatible = "qcom,pcie-sm8450-pcie1", .data = &cfg_1_9_0 }, > + { .compatible = "qcom,pcie-sm8550", .data = &cfg_1_9_0 }, This does not match your bindings. Basically, this is very similar to other models but bindings suggest otherwise - several new properties, optional existing properties. Best regards, Krzysztof
On 23-01-20 09:37:32, Krzysztof Kozlowski wrote: > On 19/01/2023 12:24, Abel Vesa wrote: > > Add the SM8550 platform to the binding. > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > --- > > > > The v2 was here: > > https://lore.kernel.org/all/20230118111704.3553542-1-abel.vesa@linaro.org/ > > > > Changes since v2: > > * dropped the pipe from clock-names > > * removed the pcie instance number from aggre clock-names comment > > * renamed aggre clock-names to noc_aggr > > * dropped the _pcie infix from cnoc_pcie_sf_axi > > * renamed pcie_1_link_down_reset to simply link_down > > * added enable-gpios back, since pcie1 node will use it > > > > Changes since v1: > > * Switched to single compatible for both PCIes (qcom,pcie-sm8550) > > * dropped enable-gpios property > > * dropped interconnects related properties, the power-domains > > * properties > > and resets related properties the sm8550 specific allOf:if:then > > * dropped pipe_mux, phy_pipe and ref clocks from the sm8550 specific > > allOf:if:then clock-names array and decreased the minItems and > > maxItems for clocks property accordingly > > * added "minItems: 1" to interconnects, since sm8550 pcie uses just one, > > same for interconnect-names > > > > > > .../devicetree/bindings/pci/qcom,pcie.yaml | 44 +++++++++++++++++++ > > 1 file changed, 44 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml > > index a5859bb3dc28..93e86dfdd6fe 100644 > > --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml > > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml > > @@ -34,6 +34,7 @@ properties: > > - qcom,pcie-sm8250 > > - qcom,pcie-sm8450-pcie0 > > - qcom,pcie-sm8450-pcie1 > > + - qcom,pcie-sm8550 > > - qcom,pcie-ipq6018 > > > > reg: > > @@ -65,9 +66,11 @@ properties: > > dma-coherent: true > > > > interconnects: > > + minItems: 1 > > maxItems: 2 > > 1. Why do you skip cpu-pcie interconnect on SM8550? > 2. This should not be allowed on other variants. That is a good point. Will add the cpu-pcie in v5. > > > > > interconnect-names: > > + minItems: 1 > > items: > > - const: pcie-mem > > - const: cpu-pcie > > @@ -102,6 +105,10 @@ properties: > > power-domains: > > maxItems: 1 > > > > + enable-gpios: > > + description: GPIO controlled connection to ENABLE# signal > > + maxItems: 1 > > + > > perst-gpios: > > description: GPIO controlled connection to PERST# signal > > maxItems: 1 > > @@ -197,6 +204,7 @@ allOf: > > - qcom,pcie-sm8250 > > - qcom,pcie-sm8450-pcie0 > > - qcom,pcie-sm8450-pcie1 > > + - qcom,pcie-sm8550 > > then: > > properties: > > reg: > > @@ -611,6 +619,41 @@ allOf: > > items: > > - const: pci # PCIe core reset > > > > + - if: > > + properties: > > + compatible: > > + contains: > > + enum: > > + - qcom,pcie-sm8550 > > + then: > > + properties: > > + clocks: > > + minItems: 7 > > + maxItems: 8 > > + clock-names: > > + minItems: 7 > > + items: > > + - const: aux # Auxiliary clock > > + - const: cfg # Configuration clock > > + - const: bus_master # Master AXI clock > > + - const: bus_slave # Slave AXI clock > > + - const: slave_q2a # Slave Q2A clock > > + - const: ddrss_sf_tbu # PCIe SF TBU clock > > + - const: noc_aggr # Aggre NoC PCIe AXI clock > > + - const: cnoc_sf_axi # Config NoC PCIe1 AXI clock > > + iommus: > > + maxItems: 1 > > + iommu-map: > > + maxItems: 2 > > 1. Don't define new properties in allOf. It makes the binding > unmaintainable. > > 2. Why only SM8550? Good point again. Will make both iommus and iommu-map properties global as even SM8450 has them. > > > + resets: > > + minItems: 1 > > Why second reset is optional? link_down reset is needed only by g4x2 pcie, AFAICT. > > > + maxItems: 2 > > + reset-names: > > + minItems: 1 > > + items: > > + - const: pci # PCIe core reset > > + - const: link_down # PCIe link down reset > > + > > - if: > > properties: > > compatible: > > @@ -694,6 +737,7 @@ allOf: > > - qcom,pcie-sm8250 > > - qcom,pcie-sm8450-pcie0 > > - qcom,pcie-sm8450-pcie1 > > + - qcom,pcie-sm8550 > > then: > > oneOf: > > - properties: > > Best regards, > Krzysztof >
On 23/01/2023 17:40, Abel Vesa wrote: > >> >>> + resets: >>> + minItems: 1 >> >> Why second reset is optional? > > link_down reset is needed only by g4x2 pcie, AFAICT. OK Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml index a5859bb3dc28..93e86dfdd6fe 100644 --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml @@ -34,6 +34,7 @@ properties: - qcom,pcie-sm8250 - qcom,pcie-sm8450-pcie0 - qcom,pcie-sm8450-pcie1 + - qcom,pcie-sm8550 - qcom,pcie-ipq6018 reg: @@ -65,9 +66,11 @@ properties: dma-coherent: true interconnects: + minItems: 1 maxItems: 2 interconnect-names: + minItems: 1 items: - const: pcie-mem - const: cpu-pcie @@ -102,6 +105,10 @@ properties: power-domains: maxItems: 1 + enable-gpios: + description: GPIO controlled connection to ENABLE# signal + maxItems: 1 + perst-gpios: description: GPIO controlled connection to PERST# signal maxItems: 1 @@ -197,6 +204,7 @@ allOf: - qcom,pcie-sm8250 - qcom,pcie-sm8450-pcie0 - qcom,pcie-sm8450-pcie1 + - qcom,pcie-sm8550 then: properties: reg: @@ -611,6 +619,41 @@ allOf: items: - const: pci # PCIe core reset + - if: + properties: + compatible: + contains: + enum: + - qcom,pcie-sm8550 + then: + properties: + clocks: + minItems: 7 + maxItems: 8 + clock-names: + minItems: 7 + items: + - const: aux # Auxiliary clock + - const: cfg # Configuration clock + - const: bus_master # Master AXI clock + - const: bus_slave # Slave AXI clock + - const: slave_q2a # Slave Q2A clock + - const: ddrss_sf_tbu # PCIe SF TBU clock + - const: noc_aggr # Aggre NoC PCIe AXI clock + - const: cnoc_sf_axi # Config NoC PCIe1 AXI clock + iommus: + maxItems: 1 + iommu-map: + maxItems: 2 + resets: + minItems: 1 + maxItems: 2 + reset-names: + minItems: 1 + items: + - const: pci # PCIe core reset + - const: link_down # PCIe link down reset + - if: properties: compatible: @@ -694,6 +737,7 @@ allOf: - qcom,pcie-sm8250 - qcom,pcie-sm8450-pcie0 - qcom,pcie-sm8450-pcie1 + - qcom,pcie-sm8550 then: oneOf: - properties:
Add the SM8550 platform to the binding. Signed-off-by: Abel Vesa <abel.vesa@linaro.org> --- The v2 was here: https://lore.kernel.org/all/20230118111704.3553542-1-abel.vesa@linaro.org/ Changes since v2: * dropped the pipe from clock-names * removed the pcie instance number from aggre clock-names comment * renamed aggre clock-names to noc_aggr * dropped the _pcie infix from cnoc_pcie_sf_axi * renamed pcie_1_link_down_reset to simply link_down * added enable-gpios back, since pcie1 node will use it Changes since v1: * Switched to single compatible for both PCIes (qcom,pcie-sm8550) * dropped enable-gpios property * dropped interconnects related properties, the power-domains * properties and resets related properties the sm8550 specific allOf:if:then * dropped pipe_mux, phy_pipe and ref clocks from the sm8550 specific allOf:if:then clock-names array and decreased the minItems and maxItems for clocks property accordingly * added "minItems: 1" to interconnects, since sm8550 pcie uses just one, same for interconnect-names .../devicetree/bindings/pci/qcom,pcie.yaml | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+)