| Message ID | 20230404082426.3880812-7-rick.wertenbroek@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: rockchip: Fix RK3399 PCIe endpoint controller driver | expand |
On 04/04/2023 10:24, Rick Wertenbroek wrote: > Update the example in the documentation a valid example. > The default max-outbound-regions is 32 but the example showed 16. This is not reason to be invalid. It is perfectly fine to change default values to desired ones. What is not actually obvious is to change some value to a default one, instead of removing it... > Address for mem-base was invalid. Added pinctrl. > > Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com> > --- > .../devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml > index 88386a6d7011..0c67e96096eb 100644 > --- a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml > +++ b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml > @@ -47,14 +47,15 @@ examples: > > pcie-ep@f8000000 { > compatible = "rockchip,rk3399-pcie-ep"; > - reg = <0x0 0xfd000000 0x0 0x1000000>, <0x0 0x80000000 0x0 0x20000>; > - reg-names = "apb-base", "mem-base"; Reg (and reg-names) is usually second property, why moving it? What is incorrect in the placement? > + rockchip,max-outbound-regions = <32>; > clocks = <&cru ACLK_PCIE>, <&cru ACLK_PERF_PCIE>, > <&cru PCLK_PCIE>, <&cru SCLK_PCIE_PM>; > clock-names = "aclk", "aclk-perf", > "hclk", "pm"; > max-functions = /bits/ 8 <8>; > num-lanes = <4>; > + reg = <0x0 0xfd000000 0x0 0x1000000>, <0x0 0xfa000000 0x0 0x2000000>; > + reg-names = "apb-base", "mem-base"; > resets = <&cru SRST_PCIE_CORE>, <&cru SRST_PCIE_MGMT>, > <&cru SRST_PCIE_MGMT_STICKY>, <&cru SRST_PCIE_PIPE> , > <&cru SRST_PCIE_PM>, <&cru SRST_P_PCIE>, <&cru SRST_A_PCIE>; > @@ -62,7 +63,8 @@ examples: > "pm", "pclk", "aclk"; > phys = <&pcie_phy 0>, <&pcie_phy 1>, <&pcie_phy 2>, <&pcie_phy 3>; > phy-names = "pcie-phy-0", "pcie-phy-1", "pcie-phy-2", "pcie-phy-3"; > - rockchip,max-outbound-regions = <16>; > + pinctrl-names = "default"; > + pinctrl-0 = <&pcie_clkreqnb_cpm>; > }; > }; > ... Best regards, Krzysztof
On Tue, Apr 4, 2023 at 10:45 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 04/04/2023 10:24, Rick Wertenbroek wrote: > > Update the example in the documentation a valid example. > > The default max-outbound-regions is 32 but the example showed 16. > > This is not reason to be invalid. It is perfectly fine to change default > values to desired ones. What is not actually obvious is to change some > value to a default one, instead of removing it... Hello, the example value <0x0 0x80000000 0x0 0x20000>; is plain wrong and will crash the kernel. This is a value that point to an address that falls in the DDR RAM region but depending on the amount of RAM on the board this address may not even exist (e.g., board with 2GB or less). Also this address requires pointing to where the PCIe controller has the windows from AXI Physical space to PCIe space. This address is allocated when the SoC address map is created so it can only be that one unless rockchip refabs the SoC with another address map. The example never worked with the values given as reported by e.g., https://stackoverflow.com/questions/73586703/device-tree-issues-with-rockpro64-pcie-endpoint and here they set it to 0 (base of the DDR, which is a "valid" address as to it exists even on boards with less than 2GB) but it is still wrong to do so. > > > Address for mem-base was invalid. Added pinctrl. > > > > Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com> > > --- > > .../devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml > > index 88386a6d7011..0c67e96096eb 100644 > > --- a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml > > +++ b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml > > @@ -47,14 +47,15 @@ examples: > > > > pcie-ep@f8000000 { > > compatible = "rockchip,rk3399-pcie-ep"; > > - reg = <0x0 0xfd000000 0x0 0x1000000>, <0x0 0x80000000 0x0 0x20000>; > > - reg-names = "apb-base", "mem-base"; > > Reg (and reg-names) is usually second property, why moving it? What is > incorrect in the placement? Sorry, I was not aware there was a standard ordering, the reason I moved so that it follows the ordering I had in the entry I added to the .dtsi file (which therefore also is in the non standard order). Could you be kind enough to share with me the link to the documentation for the order, so that I can both update the .dtsi and this file, this way it will be in order and coherent for both. Thank you. > > > + rockchip,max-outbound-regions = <32>; > > clocks = <&cru ACLK_PCIE>, <&cru ACLK_PERF_PCIE>, > > <&cru PCLK_PCIE>, <&cru SCLK_PCIE_PM>; > > clock-names = "aclk", "aclk-perf", > > "hclk", "pm"; > > max-functions = /bits/ 8 <8>; > > num-lanes = <4>; > > + reg = <0x0 0xfd000000 0x0 0x1000000>, <0x0 0xfa000000 0x0 0x2000000>; > > + reg-names = "apb-base", "mem-base"; > > resets = <&cru SRST_PCIE_CORE>, <&cru SRST_PCIE_MGMT>, > > <&cru SRST_PCIE_MGMT_STICKY>, <&cru SRST_PCIE_PIPE> , > > <&cru SRST_PCIE_PM>, <&cru SRST_P_PCIE>, <&cru SRST_A_PCIE>; > > @@ -62,7 +63,8 @@ examples: > > "pm", "pclk", "aclk"; > > phys = <&pcie_phy 0>, <&pcie_phy 1>, <&pcie_phy 2>, <&pcie_phy 3>; > > phy-names = "pcie-phy-0", "pcie-phy-1", "pcie-phy-2", "pcie-phy-3"; > > - rockchip,max-outbound-regions = <16>; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pcie_clkreqnb_cpm>; > > }; > > }; > > ... > > Best regards, > Krzysztof > Thank you for you comments, Sincerely Rick
On 04/04/2023 10:58, Rick Wertenbroek wrote: > On Tue, Apr 4, 2023 at 10:45 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 04/04/2023 10:24, Rick Wertenbroek wrote: >>> Update the example in the documentation a valid example. >>> The default max-outbound-regions is 32 but the example showed 16. >> >> This is not reason to be invalid. It is perfectly fine to change default >> values to desired ones. What is not actually obvious is to change some >> value to a default one, instead of removing it... > > Hello, the example value <0x0 0x80000000 0x0 0x20000>; is plain wrong > and will crash the kernel. This is a value that point to an address that falls > in the DDR RAM region but depending on the amount of RAM on the > board this address may not even exist (e.g., board with 2GB or less). We talk about max-outbound-regions. > > Also this address requires pointing to where the PCIe controller has the > windows from AXI Physical space to PCIe space. This address is > allocated when the SoC address map is created so it can only be that > one unless rockchip refabs the SoC with another address map. > > The example never worked with the values given as reported by e.g., > https://stackoverflow.com/questions/73586703/device-tree-issues-with-rockpro64-pcie-endpoint > and here they set it to 0 (base of the DDR, which is a "valid" address > as to it exists even on boards with less than 2GB) but it is still wrong > to do so. Again, my comment was under max-outbound-regions, not under some other pieces. Does this all apply? Best regards, Krzysztof
On Tue, Apr 4, 2023 at 3:29 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 04/04/2023 10:58, Rick Wertenbroek wrote: > > On Tue, Apr 4, 2023 at 10:45 AM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 04/04/2023 10:24, Rick Wertenbroek wrote: > >>> Update the example in the documentation a valid example. > >>> The default max-outbound-regions is 32 but the example showed 16. > >> > >> This is not reason to be invalid. It is perfectly fine to change default > >> values to desired ones. What is not actually obvious is to change some > >> value to a default one, instead of removing it... > > > > Hello, the example value <0x0 0x80000000 0x0 0x20000>; is plain wrong > > and will crash the kernel. This is a value that point to an address that falls > > in the DDR RAM region but depending on the amount of RAM on the > > board this address may not even exist (e.g., board with 2GB or less). > > We talk about max-outbound-regions. Okay, sorry, I didn't get that, you are right, there is nothing wrong with 16. I'll remove that change and leave it be. > > > > > Also this address requires pointing to where the PCIe controller has the > > windows from AXI Physical space to PCIe space. This address is > > allocated when the SoC address map is created so it can only be that > > one unless rockchip refabs the SoC with another address map. > > > > The example never worked with the values given as reported by e.g., > > https://stackoverflow.com/questions/73586703/device-tree-issues-with-rockpro64-pcie-endpoint > > and here they set it to 0 (base of the DDR, which is a "valid" address > > as to it exists even on boards with less than 2GB) but it is still wrong > > to do so. > > Again, my comment was under max-outbound-regions, not under some other > pieces. Does this all apply? > > Best regards, > Krzysztof > I'll remove the change to the max-outbound-regions, it is not needed. I'll place the registers as second parameter, both on the dtsi entry and here. I'll keep the change to the register value because it is necessary along with the added pinctrl. This will simplify the patch, avoid unnecessary changes, and make things clearer. Sorry for the misunderstanding. Regards, Rick
diff --git a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml index 88386a6d7011..0c67e96096eb 100644 --- a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml +++ b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml @@ -47,14 +47,15 @@ examples: pcie-ep@f8000000 { compatible = "rockchip,rk3399-pcie-ep"; - reg = <0x0 0xfd000000 0x0 0x1000000>, <0x0 0x80000000 0x0 0x20000>; - reg-names = "apb-base", "mem-base"; + rockchip,max-outbound-regions = <32>; clocks = <&cru ACLK_PCIE>, <&cru ACLK_PERF_PCIE>, <&cru PCLK_PCIE>, <&cru SCLK_PCIE_PM>; clock-names = "aclk", "aclk-perf", "hclk", "pm"; max-functions = /bits/ 8 <8>; num-lanes = <4>; + reg = <0x0 0xfd000000 0x0 0x1000000>, <0x0 0xfa000000 0x0 0x2000000>; + reg-names = "apb-base", "mem-base"; resets = <&cru SRST_PCIE_CORE>, <&cru SRST_PCIE_MGMT>, <&cru SRST_PCIE_MGMT_STICKY>, <&cru SRST_PCIE_PIPE> , <&cru SRST_PCIE_PM>, <&cru SRST_P_PCIE>, <&cru SRST_A_PCIE>; @@ -62,7 +63,8 @@ examples: "pm", "pclk", "aclk"; phys = <&pcie_phy 0>, <&pcie_phy 1>, <&pcie_phy 2>, <&pcie_phy 3>; phy-names = "pcie-phy-0", "pcie-phy-1", "pcie-phy-2", "pcie-phy-3"; - rockchip,max-outbound-regions = <16>; + pinctrl-names = "default"; + pinctrl-0 = <&pcie_clkreqnb_cpm>; }; }; ...
Update the example in the documentation a valid example. The default max-outbound-regions is 32 but the example showed 16. Address for mem-base was invalid. Added pinctrl. Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com> --- .../devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)