Re: [PATCH v2 1/8] dt-bindings: PCI: Add binding for qps615

[Krzysztof Kozlowski <krzk@kernel.org>]

On 05/08/2024 07:26, Krishna Chaitanya Chundru wrote:
> 
> 
> On 8/5/2024 10:44 AM, Krzysztof Kozlowski wrote:
>> On 05/08/2024 06:11, Krishna Chaitanya Chundru wrote:
>>
>>
>>>>> +
>>>>> +  qcom,nfts:
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint8
>>>>> +    description:
>>>>> +      Fast Training Sequence (FTS) is the mechanism that
>>>>> +      is used for bit and Symbol lock.
>>>>
>>>> What are the values? Why this is uint8?
>>>>
>>> These represents number of fast training sequence and doesn't have
>>> any units and the maximum value for this is 0xFF only so we used
>>> uint8.
>>>> You described the desired Linux feature or behavior, not the actual
>>>> hardware. The bindings are about the latter, so instead you need to
>>>> rephrase the property and its description to match actual hardware
>>>> capabilities/features/configuration etc.
>>> ack.
>>>>
>>>>> +
>>>>> +allOf:
>>>>> +  - $ref: /schemas/pci/pci-bus-common.yaml#
>>>>> +  - if:
>>>>> +      properties:
>>>>> +        compatible:
>>>>> +          contains:
>>>>> +            const: pci1179,0623
>>>>> +      required:
>>>>> +        - compatible
>>>>
>>>> Why do you have entire if? You do not have multiple variants, drop.
>>>>
>>> The child nodes also referencing the qcom,qps615.yaml# node, I tried
>>> to use this way to say "the below properties are for the required for
>>> parent and optional for child".
>>
>> I don't understand how child device can be exactly the same as parent
>> device. How does it look in terms of hardware? Pins and supplies?
>>
>>>>> +    then:
>>>>> +      required:
>>>>> +        - vdd18-supply
>>>>> +        - vdd09-supply
>>>>> +        - vddc-supply
>>>>> +        - vddio1-supply
>>>>> +        - vddio2-supply
>>>>> +        - vddio18-supply
>>>>> +        - qcom,qps615-controller
>>>>> +        - reset-gpios
>>>>> +
>>>>> +patternProperties:
>>>>> +  "@1?[0-9a-f](,[0-7])?$":
>>>>> +    type: object
>>>>> +    $ref: qcom,qps615.yaml#
>>>>> +    additionalProperties: true
>>>>
>>>> Nope, drop pattern Properties or explain what is this.
>>>>
>>> the child nodes represent the downstream ports of the PCIe
>>> switch which wants to use same properties that is why
>>> I tried to use this pattern properties.
>>
>> Downstream port is not the same as device. Why downstream port has the
>> same supplies? To which pins are they connected?
>>
>>
> Hi Krzysztof,
> 
> Downstream ports dosen't have pins or supplies to power on.
> 
> But there are properties like qcom,l0s-entry-delay-ns,
> qcom,l1-entry-delay-ns,  qcom,tx-amplitude-millivolt etc which
> applicable for child nodes also. Instead of re-declaring the
> these properties again I tried to use pattern properties.

You could use $defs for them, but I don't understand how does these
properties apply for both main device and ports. It seems you are
writing binding to match some driver behavior. Let's start from basics -
describe the hardware.

Best regards,
Krzysztof