Re: [RFC PATCH 1/2] dt-bindings: pcie: Document QCOM PCIE ECAM compatible root complex

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 18/04/2024 20:56, Mayank Rana wrote:
> 
> 
> On 4/8/2024 11:21 PM, Krzysztof Kozlowski wrote:
>> On 08/04/2024 21:09, Mayank Rana wrote:
>>>>> +  Firmware configures PCIe controller in RC mode with static iATU window mappings
>>>>> +  of configuration space for entire supported bus range in ECAM compatible mode.
>>>>> +
>>>>> +maintainers:
>>>>> +  - Mayank Rana <quic_mrana@quicinc.com>
>>>>> +
>>>>> +allOf:
>>>>> +  - $ref: /schemas/pci/pci-bus.yaml#
>>>>> +  - $ref: /schemas/power-domain/power-domain-consumer.yaml
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: qcom,pcie-ecam-rc
>>>>
>>>> No, this must have SoC specific compatibles.
>>> This driver is proposed to work with any PCIe controller supported ECAM
>>> functionality on Qualcomm platform
>>> where firmware running on other VM/processor is controlling PCIe PHY and
>>> controller for PCIe link up functionality.
>>> Do you still suggest to have SoC specific compatibles here ?
>>
>> What does the writing-bindings document say? Why this is different than
>> all other bindings?
> Thank you for all your review comment and suggestions.
> 
> If it is must to have SOC name, then I am not sure how 
> pci-host-generic.c driver having non SOC prefix for standard ECAM 
> driver. I am here saying this is QCOM vendor specific generic ECAM 
> driver. saying that it seems that I would be updating now 
> pci-host-generic.c driver to add generic functionality as Rob suggested 

I don't see any problem here. I talk about bindings, not driver. You can
have also fallback, so how is it different than from existing code?

> part of review comment. With
> that I am seeing possible options as i.e. continue using default generic 
> compatible as "pcie-host-ecam-generic" OR use new as 
> "qcom,pcie-host-ecam-generic". will this work ?>>>> +

Compatible and bindings focus on the hardware, so just write them
describing the hardware. You keep asking it in context of driver, but I
would say it does not matter. Is this generic hardware/firmware
implementation or not?

>>>>> +  reg:
>>>>> +    minItems: 1
>>>>
>>>> maxItems instead
>>>>
>>>>> +    description: ECAM address space starting from root port till supported bus range
>>>>> +
>>>>> +  interrupts:
>>>>> +    minItems: 1
>>>>> +    maxItems: 8
>>>>
>>>> This is way too unspecific.
>>> will review and update.
>>>>> +
>>>>> +  ranges:
>>>>> +    minItems: 2
>>>>> +    maxItems: 3
>>>>
>>>> Why variable?
>>> It depends on how ECAM configured to support 32-bit and 64-bit based
>>> prefetch address space.
>>> So there are different combination of prefetch (32-bit or 64-bit or
>>> both) and non-prefetch (32-bit), and IO address space available. hence
>>> kept it as variable with based on required use case and address space
>>> availability.
>>
>> Really? So same device has it configured once for 32 once for 64-bit
>> address space? Randomly?
> no. as binding is not saying for any specific SOC. Depends on memory map
> on particular SOC, how PCIe address space available based on that this 

So specific to the SoC, so this is not variable.

> would change for particular SOC variant.

So this is not variable and you did not provide sufficient
argumentation. You basically did not provide any argument, just
disagreed with me. Bindings must be specific and all fields should be
constrained, when reasonable.

Best regards,
Krzysztof