Re: [PATCH v2 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller【请注意，邮件由robherring2 at gmail.com代发】

[xxm]

Hi Rob,

在 2021/1/22 23:55, Rob Herring 写道:
> On Wed, Jan 20, 2021 at 06:07:29PM +0100, Johan Jonker wrote:
>> Hi Simon,
>>
>> Thanks you for version 2.
>> A few comments, have a look if it is useful or that you disagree.
>>
>> This patch has no commit message. Add one in version 3.
>>
>> Submit all patches in one batch with the same sort message ID to all
>> maintainers including Heiko.
>>
>> Heiko Stuebner <heiko at sntech.de>
>>
>> Example message ID:
>> 20210120101554.241029-1-xxm at rock-chips.com
>>
>> /////
>>
>> Included is a copy of the Rockchip pcie nodes in a sort of test.dts below.
>> Could you confirm that the properties in that dts are the one that we
>> can expect for Linux mainline and can base our YAML document on?
>>
>> With rk3568-cru.h and rk3568-power.h manualy added we do some tests with
>> the following commands:
>>
>> make ARCH=arm64 dt_binding_check
>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
>>
>> make ARCH=arm64 dtbs_check
>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
>>
>> make ARCH=arm64 dtbs_check
>> DT_SCHEMA_FILES=~/.local/lib/python3.5/site-packages/dtschema/schemas/pci/pci-bus.yaml
>>
>> /////
>>
>> Example notifications:
>>
>> /arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie at fe270000: reg: [[3,
>> 3225419776, 0, 4194304], [0, 4263968768, 0, 65536]] is too long
>>
>> /arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie at fe270000: ranges:
>> 'oneOf' conditional failed, one must be fixed:
>>
>> Before you submit version 3 make sure that all warnings gone as much as
>> possible.
>>
>> On 1/20/21 11:15 AM, Simon Xue wrote:
>>> Signed-off-by: Simon Xue <xxm at rock-chips.com>
>>> ---
>>>   .../bindings/pci/rockchip-dw-pcie.yaml        | 140 ++++++++++++++++++
>>>   1 file changed, 140 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
>>> new file mode 100644
>>> index 000000000000..9d3a57f5305e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
>>> @@ -0,0 +1,140 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: DesignWare based PCIe RC controller on Rockchip SoCs
>>> +
>>> +maintainers:
>>> +  - Shawn Lin <shawn.lin at rock-chips.com>
>>> +  - Simon Xue <xxm at rock-chips.com>
>>       - Heiko Stuebner <heiko at sntech.de> ;)
>>> +
>>> +description: |+
>>> +  RK3568 SoC PCIe host controller is based on the Synopsys DesignWare
>>> +  PCIe IP and thus inherits all the common properties defined in
>>> +  designware-pcie.txt.
>>> +
>>> +allOf:
>>> +  - $ref: /schemas/pci/pci-bus.yaml#
>>> +
>>> +# We need a select here so we don't match all nodes with 'snps,dw-pcie'
>>> +select:
>>> +  properties:
>>> +    compatible:
>>> +      contains:
>>> +        const: rockchip,rk3568-pcie
>>> +  required:
>>> +    - compatible
>>> +
>>> +properties:
>>> +  compatible:
>>> +    item:
>>      items:
>>
>>> +      - const: rockchip,rk3568-pcie
>>> +      - const: snps,dw-pcie
>> Add empty line
>>
>>> +  reg:    items:
>>        - description:
>>        - description:
>>
>> Add some description for regs.
>>
>>> +    maxItems: 1
>> remove
>>
>> This reg maxItems gives errors.
>>
>>> +
>>> +  interrupt:
>> interrupts:
>>     items:
>>
>>> +      - description: system information
>>> +      - description: power management control
>>> +      - description: PCIe message
>>> +      - description: legacy interrupt
>>> +      - description: error report
>>> +
>>> +  interrupt-names:
>>> +    items:
>>> +      - const: sys
>>> +      - const: pmc
>>> +      - const: msg
> MSI? If so, use 'msi'. The DWC core will handle setting it up now.

No, it is SoC designed interrupt for some purpose. Currently, we don't 
use interrupt, will remove

these.

>>> +      - const: legacy
>>> +      - const: err
>>> +
>>> +  clocks:
>>> +    items:
>>> +      - description: AHB clock for PCIe master
>>> +      - description: AHB clock for PCIe slave
>>> +      - description: AHB clock for PCIe dbi
>>> +      - description: APB clock for PCIe
>>> +      - description: Auxiliary clock for PCIe
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: aclk_mst
>>> +      - const: aclk_slv
>>> +      - const: aclk_dbi
>>> +      - const: pclk
>>> +      - const: aux
>>> +
>>> +  msi-map: true
>>> +
>>> +  power-domains:
>>> +    maxItems: 1
>> /////
>> These properties come from designware-pcie.txt
>> Maybe add them here for now till there's a common yaml?
>>
>>    num-ib-windows: number of inbound address translation windows
>>    num-ob-windows: number of outbound address translation windows
> These can be and are now detected at runtime.

Will remove.

Simon Xue

>
> Rob
>
>
>