| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20220621074244epcms2p23cb631babfb87e5abb416467fb66e521@epcms2p2>
Date: Tue, 21 Jun 2022 16:42:44 +0900
From: Wangseok Lee <wangseok.lee@...sung.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"krzk+dt@...nel.org" <krzk+dt@...nel.org>,
"kishon@...com" <kishon@...com>,
"vkoul@...nel.org" <vkoul@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"jesper.nilsson@...s.com" <jesper.nilsson@...s.com>,
"lars.persson@...s.com" <lars.persson@...s.com>,
"bhelgaas@...gle.com" <bhelgaas@...gle.com>,
"linux-phy@...ts.infradead.org" <linux-phy@...ts.infradead.org>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"lorenzo.pieralisi@....com" <lorenzo.pieralisi@....com>,
"kw@...ux.com" <kw@...ux.com>,
"linux-arm-kernel@...s.com" <linux-arm-kernel@...s.com>,
"kernel@...s.com" <kernel@...s.com>
CC: Moon-Ki Jun <moonki.jun@...sung.com>,
Sang Min Kim <hypmean.kim@...sung.com>,
Dongjin Yang <dj76.yang@...sung.com>,
Yeeun Kim <yeeun119.kim@...sung.com>
Subject: Re: [PATCH v3 1/5] dt-bindings: pci: Add ARTPEC-8 PCIe controller
On 20/06/2022 17:42, Krzysztof Kozlowski wrote:
> On 20/06/2022 09:55, Wangseok Lee wrote:
>> On 17/06/2022 07:54, Krzysztof Kozlowski wrote:
>>> On 13/06/2022 18:27, Wangseok Lee wrote:
>>>> Add description to support Axis, ARTPEC-8 SoC.
>>>> ARTPEC-8 is the SoC platform of Axis Communications
>>>> and PCIe controller is designed based on Design-Ware PCIe controller.
>>>>
>>>> Signed-off-by: Wangseok Lee <wangseok.lee@...sung.com>
>>>> ---
>>>> v2->v3 :
>>>> - modify version history to fit the linux commit rule
>>>> - remove 'Device Tree Bindings' on title
>>>> - remove the interrupt-names, phy-names entries
>>>> - remove '_clk' suffix
>>>> - add the compatible entries on required
>>>> - change node name to soc from artpec8 on examples
>>>>
>>>> v1->v2 :
>>>> -'make dt_binding_check' result improvement
>>>> -Add the missing property list
>>>> -Align the indentation of continued lines/entries
>>>> ---
>>>> .../bindings/pci/axis,artpec8-pcie-ep.yaml | 109 +++++++++++++++++++
>>>> .../devicetree/bindings/pci/axis,artpec8-pcie.yaml | 120 +++++++++++++++++++++
>>>> 2 files changed, 229 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml
>>>> create mode 100644 Documentation/devicetree/bindings/pci/axis,artpec8-pcie.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml
>>>> new file mode 100644
>>>> index 0000000..d802bba
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml
>>>> @@ -0,0 +1,109 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: https://protect2.fireeye.com/v1/url?k=87636683-e61e8c00-8762edcc-74fe48600158-e7a1c3794076f0b9&q=1&e=35e09b7f-4fb1-4c8f-83ac-7ec33e124d44&u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fpci%2Faxis%2Cartpec8-pcie-ep.yaml%23
>>>> +$schema: https://protect2.fireeye.com/v1/url?k=36f56c4e-578886cd-36f4e701-74fe48600158-afd7270f84937054&q=1&e=35e09b7f-4fb1-4c8f-83ac-7ec33e124d44&u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
>>>> +
>>>> +title: ARTPEC-8 SoC PCIe Controller
>>>> +
>>>> +maintainers:
>>>> + - Jesper Nilsson <jesper.nilsson@...s.com>
>>>> +
>>>> +description: |+
>>>> + This PCIe end-point controller is based on the Synopsys DesignWare PCIe IP
>>>> + and thus inherits all the common properties defined in snps,dw-pcie-ep.yaml.
>>>> +
>>>> +allOf:
>>>> + - $ref: /schemas/pci/snps,dw-pcie-ep.yaml#
>>>> +
>>>> +properties:
>>>> + compatible:
>>>> + const: axis,artpec8-pcie-ep
>>>> +
>>>> + reg:
>>>> + items:
>>>> + - description: Data Bus Interface (DBI) registers.
>>>> + - description: Data Bus Interface (DBI2) registers.
>>>> + - description: PCIe address space region.
>>>> +
>>>> + reg-names:
>>>> + items:
>>>> + - const: dbi
>>>> + - const: dbi2
>>>> + - const: addr_space
>>>> +
>>>> + interrupts:
>>>> + maxItems: 1
>>>> +
>>>> + clocks:
>>>> + items:
>>>> + - description: PIPE clock, used by the controller to clock the PIPE
>>>> + - description: PCIe dbi clock, ungated version
>>>> + - description: PCIe master clock, ungated version
>>>> + - description: PCIe slave clock, ungated version
>>>> +
>>>> + clock-names:
>>>> + items:
>>>> + - const: pipe
>>>> + - const: dbi
>>>> + - const: mstr
>>>> + - const: slv
>>>> +
>>>> + phys:
>>>> + maxItems: 1
>>>> +
>>>> + num-lanes:
>>>> + const: 2
>>>> +
>>>> +required:
>>>> + - compatible
>>>> + - reg
>>>> + - reg-names
>>>> + - interrupts
>>>> + - interrupt-names
>>>> + - clocks
>>>> + - clock-names
>>>> + - samsung,fsys-sysreg
>>>> + - samsung,syscon-phandle
>>>> + - samsung,syscon-bus-s-fsys
>>>> + - samsung,syscon-bus-p-fsys
>>>
>>>
>>> We are making circles... This was before and I commented already it is
>>> wrong. You cannot have some unknown/random properties in "required:"
>>> without describing them in "properties:". Please list all your
>>> properties in "properties:", except the ones coming from snps
>>> bindings/schema.
>>>
>>
>> I missed that when adding new items to "required",
>> it should also be added to "properties".
>> I will add the following items to the property.
>>
>> samsung,fsys-sysreg:
>> description:
>> Phandle to system register of fsys block.
>> $ref: /schemas/types.yaml#/definitions/phandle
>
> This is ok.
>
>>
>> samsung,syscon-phandle:
>> description:
>> Phandle to the PMU system controller node.
>> $ref: /schemas/types.yaml#/definitions/phandle
>
> This is ok.
>
>>
>> samsung,syscon-bus-s-fsys:
>> description:
>> Phandle to bus-s path of fsys block, this register
>> are used for enabling bus-s.
>> $ref: /schemas/types.yaml#/definitions/phandle
>>
>> samsung,syscon-bus-p-fsys:
>> description:
>> Phandle to bus-p path of fsys block, this register
>> are used for enabling bus-p.
>> $ref: /schemas/types.yaml#/definitions/phandle
>
> This two look unspecific and hacky workaround for missing drivers. Looks
> like instead of implementing interconnect or clock driver, you decided
> to poke some other registers. Why this cannot be an interconnect driver?
>
>
bus-s, bus-p is a register that exists in the sysreg of the fsys block.
It is the same block as "fsys-sysreg" but is separated separately in
hardware.
So, get resource is performed separately from "fsys-sysreg".
They set pcie slave, dbi related control settings,
naming "bus-x" seems to be interconnect.
I will add this description to property.
I don't think it need to use the interconnect driver,
so please let me know your opinion.
> Best regards,
> Krzysztof
Thank you for kindness reivew.
Best regards,
Wangseok Lee
Powered by blists - more mailing lists