mbox series

[v2,0/4] Refactoring Microchip PCIe driver and add StarFive PCIe

Message ID 20230727103949.26149-1-minda.chen@starfivetech.com
Headers show
Series Refactoring Microchip PCIe driver and add StarFive PCIe | expand

Message

Minda Chen July 27, 2023, 10:39 a.m. UTC
This patchset final purpose is add PCIe driver for StarFive JH7110 SoC.
JH7110 using PLDA XpressRICH PCIe IP. Microchip PolarFire Using the
same IP and have commit their codes, which are mixed with PLDA
controller codes and Microchip platform codes.

For re-use the PLDA controller codes, I request refactoring microchip
codes, move PLDA common codes to PLDA files.
Desigware and Cadence is good example for refactoring codes.

So first step is extract the PLDA common codes from microchip, and
refactoring the microchip codes.(patch1 - 2)
Then, add Starfive codes. (patch3 - 4)

This patchset is base on v6.5-rc1

patch1 is move PLDA XpressRICH PCIe host common properties dt-binding
       docs from microchip,pcie-host.yaml
patch2 is extracting the PLDA common codes from microchip Polarfire PCIe
       codes. The change list in the commit message.
patch3 is add StarFive JH7110 PCIe dt-binding doc.
patch4 is add StarFive JH7110 Soc PCIe codes.

I have noticed that Daire have changed microchip's codes.
https://patchwork.kernel.org/project/linux-pci/cover/20230630154859.2049521-1-daire.mcnamara@microchip.com/
I have changed patch2 base on their commits. StarFive
PCIe driver still can work. But their codes is under reviewed and
maybe changing. Do not base on their changes first.
I will base on their commit to change patch2 as soon as
their commits are accepted.

previous version:
v1:https://patchwork.kernel.org/project/linux-pci/cover/20230719102057.22329-1-minda.chen@starfivetech.com/

change:
  v2:
    patch1:
      - squash dt-bindings patches to patch1
      - add 'required' list. 
      - plda doc rename to plda,xpressrich-axi-common.yaml
    patch2:
      - squash the microchip modification patch to patch 2.
    patch3:
      - remove the plda common required property.
    patch4:
      - Sync the hide rc bar ops with config read function.
      - Revert the T_PVPERL to 100ms and add comments for the source.
      - Replace the link check function by the standard link ops.
      - Convert to new pm ops marcos.
      - Some formats modification.
      - pcie-plda-host modification merge to patch4.
    other:
      - remove the pcie-plda-plat.c
      - remove the starfive dts patch first. for it depends on
        stg clock and syscon setting.

Minda Chen (4):
  dt-bindings: PCI: Add PLDA XpressRICH PCIe host common properties
  PCI: plda: Get common codes from Microchip PolarFire host
  dt-bindings: PCI: Add StarFive JH7110 PCIe controller
  PCI: starfive: Add JH7110 PCIe controller

 .../bindings/pci/microchip,pcie-host.yaml     |  49 +-
 .../pci/plda,xpressrich3-axi-common.yaml      |  69 ++
 .../bindings/pci/starfive,jh7110-pcie.yaml    | 133 ++++
 MAINTAINERS                                   |  19 +-
 drivers/pci/controller/Kconfig                |   9 +-
 drivers/pci/controller/Makefile               |   2 +-
 drivers/pci/controller/plda/Kconfig           |  31 +
 drivers/pci/controller/plda/Makefile          |   4 +
 .../{ => plda}/pcie-microchip-host.c          | 594 ++--------------
 drivers/pci/controller/plda/pcie-plda-host.c  | 665 ++++++++++++++++++
 drivers/pci/controller/plda/pcie-plda.h       | 242 +++++++
 drivers/pci/controller/plda/pcie-starfive.c   | 438 ++++++++++++
 12 files changed, 1645 insertions(+), 610 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/plda,xpressrich3-axi-common.yaml
 create mode 100644 Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
 create mode 100644 drivers/pci/controller/plda/Kconfig
 create mode 100644 drivers/pci/controller/plda/Makefile
 rename drivers/pci/controller/{ => plda}/pcie-microchip-host.c (50%)
 create mode 100644 drivers/pci/controller/plda/pcie-plda-host.c
 create mode 100644 drivers/pci/controller/plda/pcie-plda.h
 create mode 100644 drivers/pci/controller/plda/pcie-starfive.c


base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5

Comments

Minda Chen Aug. 3, 2023, 6:44 a.m. UTC | #1
On 2023/7/27 23:21, Randy Dunlap wrote:
> Hi--
> 
> On 7/27/23 03:39, Minda Chen wrote:
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 8e4f9d5dca55..ec59c6d00bf9 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -16170,6 +16170,14 @@ S:	Maintained
>>  F:	Documentation/devicetree/bindings/pci/cdns,*
>>  F:	drivers/pci/controller/cadence/
>>  
> 
> This new entry is not in the correct location.
> It should be in alphabetical order, so it goes between
> 
> PCI DRIVER FOR NXP LAYERSCAPE GEN4 CONTROLLER
> and
> PCI DRIVER FOR RENESAS R-CAR
> 
> Thanks.
> 
Got it. Thanks
>> +PCI DRIVER FOR PLDA PCIE IP
>> +M:	Daire McNamara <daire.mcnamara@microchip.com>
>> +M:	Kevin Xie <kevin.xie@starfivetech.com>
>> +L:	linux-pci@vger.kernel.org
>> +S:	Maintained
>> +F:	Documentation/devicetree/bindings/pci/plda,*
>> +F:	drivers/pci/controller/plda/*plda*
>> +
>>  PCI DRIVER FOR FREESCALE LAYERSCAPE
>>  M:	Minghuan Lian <minghuan.Lian@nxp.com>
>>  M:	Mingkai Hu <mingkai.hu@nxp.com>
>
Minda Chen Aug. 4, 2023, 1:46 a.m. UTC | #2
On 2023/7/27 18:39, Minda Chen wrote:
> This patchset final purpose is add PCIe driver for StarFive JH7110 SoC.
> JH7110 using PLDA XpressRICH PCIe IP. Microchip PolarFire Using the
> same IP and have commit their codes, which are mixed with PLDA
> controller codes and Microchip platform codes.
> 
> For re-use the PLDA controller codes, I request refactoring microchip
> codes, move PLDA common codes to PLDA files.
> Desigware and Cadence is good example for refactoring codes.
> 
> So first step is extract the PLDA common codes from microchip, and
> refactoring the microchip codes.(patch1 - 2)
> Then, add Starfive codes. (patch3 - 4)
> 
> This patchset is base on v6.5-rc1
> 
> patch1 is move PLDA XpressRICH PCIe host common properties dt-binding
>        docs from microchip,pcie-host.yaml
> patch2 is extracting the PLDA common codes from microchip Polarfire PCIe
>        codes. The change list in the commit message.
> patch3 is add StarFive JH7110 PCIe dt-binding doc.
> patch4 is add StarFive JH7110 Soc PCIe codes.
> 
Hi Rob, Krzysztof(K.K) and Conor
  Do you have any comments for dts-binding doc patch? (patch 1 and patch3) Thanks.
Conor Dooley Aug. 4, 2023, 6:23 a.m. UTC | #3
Hey Minda,

On Fri, Aug 04, 2023 at 09:46:30AM +0800, Minda Chen wrote:
> On 2023/7/27 18:39, Minda Chen wrote:
> > This patchset final purpose is add PCIe driver for StarFive JH7110 SoC.
> > JH7110 using PLDA XpressRICH PCIe IP. Microchip PolarFire Using the
> > same IP and have commit their codes, which are mixed with PLDA
> > controller codes and Microchip platform codes.
> > 
> > For re-use the PLDA controller codes, I request refactoring microchip
> > codes, move PLDA common codes to PLDA files.
> > Desigware and Cadence is good example for refactoring codes.
> > 
> > So first step is extract the PLDA common codes from microchip, and
> > refactoring the microchip codes.(patch1 - 2)
> > Then, add Starfive codes. (patch3 - 4)
> > 
> > This patchset is base on v6.5-rc1
> > 
> > patch1 is move PLDA XpressRICH PCIe host common properties dt-binding
> >        docs from microchip,pcie-host.yaml
> > patch2 is extracting the PLDA common codes from microchip Polarfire PCIe
> >        codes. The change list in the commit message.
> > patch3 is add StarFive JH7110 PCIe dt-binding doc.
> > patch4 is add StarFive JH7110 Soc PCIe codes.
> >

> Hi Rob, Krzysztof(K.K) and Conor

>   Do you have any comments for dts-binding doc patch? (patch 1 and patch3) Thanks.  

Yeah, I do intend looking at this! I think, because I am wearing more
than one hat for this series, it ended up not in my dt-binding review
queue. I'll make sure to have a look today.
Conor Dooley Aug. 4, 2023, 7:10 a.m. UTC | #4
On Thu, Jul 27, 2023 at 06:39:48PM +0800, Minda Chen wrote:
> Add StarFive JH7110 SoC PCIe controller dt-bindings.
> JH7110 using PLDA XpressRICH PCIe host controller IP.
> 
> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>  .../bindings/pci/starfive,jh7110-pcie.yaml    | 133 ++++++++++++++++++
>  1 file changed, 133 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml b/Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
> new file mode 100644
> index 000000000000..9273e029fb20
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
> @@ -0,0 +1,133 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/starfive,jh7110-pcie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive JH7110 PCIe host controller
> +
> +maintainers:
> +  - Kevin Xie <kevin.xie@starfivetech.com>
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-bus.yaml#
> +  - $ref: plda,xpressrich3-axi-common.yaml#
> +  - $ref: /schemas/interrupt-controller/msi-controller.yaml#
> +  - $ref: /schemas/gpio/gpio-consumer-common.yaml#
> +
> +properties:
> +  compatible:
> +    const: starfive,jh7110-pcie
> +
> +  clocks:
> +    items:
> +      - description: NOC bus clock
> +      - description: Transport layer clock
> +      - description: AXI MST0 clock
> +      - description: APB clock
> +
> +  clock-names:
> +    items:
> +      - const: noc
> +      - const: tl
> +      - const: axi_mst0
> +      - const: apb
> +
> +  resets:
> +    items:
> +      - description: AXI MST0 reset
> +      - description: AXI SLAVE0 reset
> +      - description: AXI SLAVE reset
> +      - description: PCIE BRIDGE reset
> +      - description: PCIE CORE reset
> +      - description: PCIE APB reset
> +
> +  reset-names:
> +    items:
> +      - const: mst0
> +      - const: slv0
> +      - const: slv
> +      - const: brg
> +      - const: core
> +      - const: apb
> +
> +  starfive,stg-syscon:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - items:
> +          - description: phandle to System Register Controller stg_syscon node.
> +          - description: register0 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
> +          - description: register1 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
> +          - description: register2 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
> +          - description: register3 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
> +    description:
> +      The phandle to System Register Controller syscon node and the offset
> +      of STG_SYSCONSAIF__SYSCFG register for PCIe. Total 4 regsisters offset
> +      for PCIe.

These property names tie them closely with naming on the jh7110, but
there's little value in specifying all of these offsets when you have
one implementation where they are all fixed.
Do you know what the jh81xx stuff is going to do yet w.r.t. PCI and if
so, how could you reuse this property?
Particularly, saying "register 0" seems unlikely to transfer well
between SoCs.
I'd be inclined to drop the offsets entirely & rely on match data to
provide them if needed in the future.

> +
> +  phys:
> +    description:
> +      Specified PHY is attached to PCIe controller.
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - clocks
> +  - resets
> +  - starfive,stg-syscon
> +  - "#interrupt-cells"
> +  - interrupt-map-mask
> +  - interrupt-map
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        pcie0: pcie@2b000000 {

nit: you don't need labels in examples if they are not referenced
anywhere.

Otherwise, this looks good to me.

Thanks,
Conor.

> +            compatible = "starfive,jh7110-pcie";
> +            reg = <0x9 0x40000000 0x0 0x10000000>,
> +                  <0x0 0x2b000000 0x0 0x1000000>;
> +            reg-names = "cfg", "apb";
> +            #address-cells = <3>;
> +            #size-cells = <2>;
> +            #interrupt-cells = <1>;
> +            device_type = "pci";
> +            ranges = <0x82000000  0x0 0x30000000  0x0 0x30000000 0x0 0x08000000>,
> +                     <0xc3000000  0x9 0x00000000  0x9 0x00000000 0x0 0x40000000>;
> +            starfive,stg-syscon = <&stg_syscon 0xc0 0xc4 0x130 0x1b8>;
> +            bus-range = <0x0 0xff>;
> +            interrupt-parent = <&plic>;
> +            interrupts = <56>;
> +            interrupt-map-mask = <0x0 0x0 0x0 0x7>;
> +            interrupt-map = <0x0 0x0 0x0 0x1 &pcie_intc0 0x1>,
> +                            <0x0 0x0 0x0 0x2 &pcie_intc0 0x2>,
> +                            <0x0 0x0 0x0 0x3 &pcie_intc0 0x3>,
> +                            <0x0 0x0 0x0 0x4 &pcie_intc0 0x4>;
> +            msi-parent = <&pcie0>;
> +            msi-controller;
> +            clocks = <&syscrg 86>,
> +                     <&stgcrg 10>,
> +                     <&stgcrg 8>,
> +                     <&stgcrg 9>;
> +            clock-names = "noc", "tl", "axi_mst0", "apb";
> +            resets = <&stgcrg 11>,
> +                     <&stgcrg 12>,
> +                     <&stgcrg 13>,
> +                     <&stgcrg 14>,
> +                     <&stgcrg 15>,
> +                     <&stgcrg 16>;
> +            reset-gpios = <&gpios 26 GPIO_ACTIVE_LOW>;
> +            phys = <&pciephy0>;
> +
> +            pcie_intc0: interrupt-controller {
> +                #address-cells = <0>;
> +                #interrupt-cells = <1>;
> +                interrupt-controller;
> +            };
> +        };
> +    };
> -- 
> 2.17.1
>
Conor Dooley Aug. 4, 2023, 7:12 a.m. UTC | #5
On Thu, Jul 27, 2023 at 06:39:46PM +0800, Minda Chen wrote:
> Add PLDA XpressRICH PCIe host common properties dt-binding doc.
> Microchip PolarFire PCIe host using PLDA IP.
> Move common properties from Microchip PolarFire PCIe host
> to PLDA files.
> 
> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>

This also seems okay to me.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.
Emil Renner Berthing Aug. 5, 2023, 1:05 p.m. UTC | #6
On Thu, 27 Jul 2023 at 12:40, Minda Chen <minda.chen@starfivetech.com> wrote:
>
> This patchset final purpose is add PCIe driver for StarFive JH7110 SoC.
> JH7110 using PLDA XpressRICH PCIe IP. Microchip PolarFire Using the
> same IP and have commit their codes, which are mixed with PLDA
> controller codes and Microchip platform codes.
>
> For re-use the PLDA controller codes, I request refactoring microchip
> codes, move PLDA common codes to PLDA files.
> Desigware and Cadence is good example for refactoring codes.
>
> So first step is extract the PLDA common codes from microchip, and
> refactoring the microchip codes.(patch1 - 2)
> Then, add Starfive codes. (patch3 - 4)
>
> This patchset is base on v6.5-rc1
>
> patch1 is move PLDA XpressRICH PCIe host common properties dt-binding
>        docs from microchip,pcie-host.yaml
> patch2 is extracting the PLDA common codes from microchip Polarfire PCIe
>        codes. The change list in the commit message.
> patch3 is add StarFive JH7110 PCIe dt-binding doc.
> patch4 is add StarFive JH7110 Soc PCIe codes.

Hi Minda,

To test this series properly it needs matching nodes in the VisionFive
2 device trees, but it seems to be missing from this version of the
patch series. If I apply the device tree patch from v1 I get errors
like this:

  pcie-starfive 2b000000.pcie: invalid resource (null)
  pcie-starfive 2b000000.pcie: error -EINVAL: failed to map reg memory

It would be great if you included the device tree patch in the next
series so this can actually be tested.

/Emil

> I have noticed that Daire have changed microchip's codes.
> https://patchwork.kernel.org/project/linux-pci/cover/20230630154859.2049521-1-daire.mcnamara@microchip.com/
> I have changed patch2 base on their commits. StarFive
> PCIe driver still can work. But their codes is under reviewed and
> maybe changing. Do not base on their changes first.
> I will base on their commit to change patch2 as soon as
> their commits are accepted.
>
> previous version:
> v1:https://patchwork.kernel.org/project/linux-pci/cover/20230719102057.22329-1-minda.chen@starfivetech.com/
>
> change:
>   v2:
>     patch1:
>       - squash dt-bindings patches to patch1
>       - add 'required' list.
>       - plda doc rename to plda,xpressrich-axi-common.yaml
>     patch2:
>       - squash the microchip modification patch to patch 2.
>     patch3:
>       - remove the plda common required property.
>     patch4:
>       - Sync the hide rc bar ops with config read function.
>       - Revert the T_PVPERL to 100ms and add comments for the source.
>       - Replace the link check function by the standard link ops.
>       - Convert to new pm ops marcos.
>       - Some formats modification.
>       - pcie-plda-host modification merge to patch4.
>     other:
>       - remove the pcie-plda-plat.c
>       - remove the starfive dts patch first. for it depends on
>         stg clock and syscon setting.
>
> Minda Chen (4):
>   dt-bindings: PCI: Add PLDA XpressRICH PCIe host common properties
>   PCI: plda: Get common codes from Microchip PolarFire host
>   dt-bindings: PCI: Add StarFive JH7110 PCIe controller
>   PCI: starfive: Add JH7110 PCIe controller
>
>  .../bindings/pci/microchip,pcie-host.yaml     |  49 +-
>  .../pci/plda,xpressrich3-axi-common.yaml      |  69 ++
>  .../bindings/pci/starfive,jh7110-pcie.yaml    | 133 ++++
>  MAINTAINERS                                   |  19 +-
>  drivers/pci/controller/Kconfig                |   9 +-
>  drivers/pci/controller/Makefile               |   2 +-
>  drivers/pci/controller/plda/Kconfig           |  31 +
>  drivers/pci/controller/plda/Makefile          |   4 +
>  .../{ => plda}/pcie-microchip-host.c          | 594 ++--------------
>  drivers/pci/controller/plda/pcie-plda-host.c  | 665 ++++++++++++++++++
>  drivers/pci/controller/plda/pcie-plda.h       | 242 +++++++
>  drivers/pci/controller/plda/pcie-starfive.c   | 438 ++++++++++++
>  12 files changed, 1645 insertions(+), 610 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/pci/plda,xpressrich3-axi-common.yaml
>  create mode 100644 Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
>  create mode 100644 drivers/pci/controller/plda/Kconfig
>  create mode 100644 drivers/pci/controller/plda/Makefile
>  rename drivers/pci/controller/{ => plda}/pcie-microchip-host.c (50%)
>  create mode 100644 drivers/pci/controller/plda/pcie-plda-host.c
>  create mode 100644 drivers/pci/controller/plda/pcie-plda.h
>  create mode 100644 drivers/pci/controller/plda/pcie-starfive.c
>
>
> base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
> --
> 2.17.1
>
Minda Chen Aug. 7, 2023, 6:54 a.m. UTC | #7
On 2023/8/4 15:10, Conor Dooley wrote:
> On Thu, Jul 27, 2023 at 06:39:48PM +0800, Minda Chen wrote:
>> Add StarFive JH7110 SoC PCIe controller dt-bindings.
>> JH7110 using PLDA XpressRICH PCIe host controller IP.
>> 
>> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
>> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
>> ---
>>  .../bindings/pci/starfive,jh7110-pcie.yaml    | 133 ++++++++++++++++++
>>  1 file changed, 133 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml b/Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
>> new file mode 100644
>> index 000000000000..9273e029fb20
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
>> @@ -0,0 +1,133 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pci/starfive,jh7110-pcie.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: StarFive JH7110 PCIe host controller
>> +
>> +maintainers:
>> +  - Kevin Xie <kevin.xie@starfivetech.com>
>> +
>> +allOf:
>> +  - $ref: /schemas/pci/pci-bus.yaml#
>> +  - $ref: plda,xpressrich3-axi-common.yaml#
>> +  - $ref: /schemas/interrupt-controller/msi-controller.yaml#
>> +  - $ref: /schemas/gpio/gpio-consumer-common.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    const: starfive,jh7110-pcie
>> +
>> +  clocks:
>> +    items:
>> +      - description: NOC bus clock
>> +      - description: Transport layer clock
>> +      - description: AXI MST0 clock
>> +      - description: APB clock
>> +
>> +  clock-names:
>> +    items:
>> +      - const: noc
>> +      - const: tl
>> +      - const: axi_mst0
>> +      - const: apb
>> +
>> +  resets:
>> +    items:
>> +      - description: AXI MST0 reset
>> +      - description: AXI SLAVE0 reset
>> +      - description: AXI SLAVE reset
>> +      - description: PCIE BRIDGE reset
>> +      - description: PCIE CORE reset
>> +      - description: PCIE APB reset
>> +
>> +  reset-names:
>> +    items:
>> +      - const: mst0
>> +      - const: slv0
>> +      - const: slv
>> +      - const: brg
>> +      - const: core
>> +      - const: apb
>> +
>> +  starfive,stg-syscon:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    items:
>> +      - items:
>> +          - description: phandle to System Register Controller stg_syscon node.
>> +          - description: register0 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
>> +          - description: register1 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
>> +          - description: register2 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
>> +          - description: register3 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
>> +    description:
>> +      The phandle to System Register Controller syscon node and the offset
>> +      of STG_SYSCONSAIF__SYSCFG register for PCIe. Total 4 regsisters offset
>> +      for PCIe.
> 
> These property names tie them closely with naming on the jh7110, but
> there's little value in specifying all of these offsets when you have
> one implementation where they are all fixed.
Yes, the offset value is tied to SoC. 
> Do you know what the jh81xx stuff is going to do yet w.r.t. PCI and if
> so, how could you reuse this property?
I do not participate in jh8100. But I heard sys-syscon is exist in 81xx.
But I think stg-syscon and sys-syscon  can be move to a common dt-binding doc.
Bot 71x0 and 81x0 driver can use this. 
> Particularly, saying "register 0" seems unlikely to transfer well
> between SoCs.
> I'd be inclined to drop the offsets entirely & rely on match data to
> provide them if needed in the future.
That's ok. The dts can change to starfive,stg-syscon = <&stg_syscon>;
I will try to move the offset to driver match data.
>> +
>> +  phys:
>> +    description:
>> +      Specified PHY is attached to PCIe controller.
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +  - clocks
>> +  - resets
>> +  - starfive,stg-syscon
>> +  - "#interrupt-cells"
>> +  - interrupt-map-mask
>> +  - interrupt-map
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
>> +    soc {
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
>> +        pcie0: pcie@2b000000 {
> 
> nit: you don't need labels in examples if they are not referenced
> anywhere.
> 
> Otherwise, this looks good to me.
> 
> Thanks,
> Conor.
> 
ok, thanks.
>> +            compatible = "starfive,jh7110-pcie";
>> +            reg = <0x9 0x40000000 0x0 0x10000000>,
>> +                  <0x0 0x2b000000 0x0 0x1000000>;
>> +            reg-names = "cfg", "apb";
>> +            #address-cells = <3>;
>> +            #size-cells = <2>;
>> +            #interrupt-cells = <1>;
>> +            device_type = "pci";
>> +            ranges = <0x82000000  0x0 0x30000000  0x0 0x30000000 0x0 0x08000000>,
>> +                     <0xc3000000  0x9 0x00000000  0x9 0x00000000 0x0 0x40000000>;
>> +            starfive,stg-syscon = <&stg_syscon 0xc0 0xc4 0x130 0x1b8>;
>> +            bus-range = <0x0 0xff>;
>> +            interrupt-parent = <&plic>;
>> +            interrupts = <56>;
>> +            interrupt-map-mask = <0x0 0x0 0x0 0x7>;
>> +            interrupt-map = <0x0 0x0 0x0 0x1 &pcie_intc0 0x1>,
>> +                            <0x0 0x0 0x0 0x2 &pcie_intc0 0x2>,
>> +                            <0x0 0x0 0x0 0x3 &pcie_intc0 0x3>,
>> +                            <0x0 0x0 0x0 0x4 &pcie_intc0 0x4>;
>> +            msi-parent = <&pcie0>;
>> +            msi-controller;
>> +            clocks = <&syscrg 86>,
>> +                     <&stgcrg 10>,
>> +                     <&stgcrg 8>,
>> +                     <&stgcrg 9>;
>> +            clock-names = "noc", "tl", "axi_mst0", "apb";
>> +            resets = <&stgcrg 11>,
>> +                     <&stgcrg 12>,
>> +                     <&stgcrg 13>,
>> +                     <&stgcrg 14>,
>> +                     <&stgcrg 15>,
>> +                     <&stgcrg 16>;
>> +            reset-gpios = <&gpios 26 GPIO_ACTIVE_LOW>;
>> +            phys = <&pciephy0>;
>> +
>> +            pcie_intc0: interrupt-controller {
>> +                #address-cells = <0>;
>> +                #interrupt-cells = <1>;
>> +                interrupt-controller;
>> +            };
>> +        };
>> +    };
>> -- 
>> 2.17.1
>>
Conor Dooley Aug. 7, 2023, 7:45 a.m. UTC | #8
On Mon, Aug 07, 2023 at 02:54:50PM +0800, Minda Chen wrote:
> On 2023/8/4 15:10, Conor Dooley wrote:
> > On Thu, Jul 27, 2023 at 06:39:48PM +0800, Minda Chen wrote:
> >> +  starfive,stg-syscon:
> >> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> >> +    items:
> >> +      - items:
> >> +          - description: phandle to System Register Controller stg_syscon node.
> >> +          - description: register0 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
> >> +          - description: register1 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
> >> +          - description: register2 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
> >> +          - description: register3 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
> >> +    description:
> >> +      The phandle to System Register Controller syscon node and the offset
> >> +      of STG_SYSCONSAIF__SYSCFG register for PCIe. Total 4 regsisters offset
> >> +      for PCIe.
> > 
> > These property names tie them closely with naming on the jh7110, but
> > there's little value in specifying all of these offsets when you have
> > one implementation where they are all fixed.
> Yes, the offset value is tied to SoC. 
> > Do you know what the jh81xx stuff is going to do yet w.r.t. PCI and if
> > so, how could you reuse this property?
> I do not participate in jh8100. But I heard sys-syscon is exist in 81xx.
> But I think stg-syscon and sys-syscon  can be move to a common dt-binding doc.
> Bot 71x0 and 81x0 driver can use this. 
> > Particularly, saying "register 0" seems unlikely to transfer well
> > between SoCs.
> > I'd be inclined to drop the offsets entirely & rely on match data to
> > provide them if needed in the future.
> That's ok. The dts can change to starfive,stg-syscon = <&stg_syscon>;
> I will try to move the offset to driver match data.

To be clear, you don't need match data now, only when the jh8100 stuff
shows up. Until then the values can be hard coded in the driver as there
is only one device it works for.

Thanks,
Conor.