Re: [PATCH v4 01/10] dt-bindings: PCI: Add binding for Toshiba TC956x PCIe switch

Krishna Chaitanya Chundru
2025-02-27T03:53:56Z

On 2/26/2025 1:00 PM, Krzysztof Kozlowski wrote:
> On Tue, Feb 25, 2025 at 03:03:58PM +0530, Krishna Chaitanya Chundru wrote:
>> From: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>>
>> Add a device tree binding for the Toshiba TC956x PCIe switch, which
>> provides an Ethernet MAC integrated to the 3rd downstream port and two
>> downstream PCIe ports.
>>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> Reviewed-by: Bjorn Andersson <andersson@kernel.org>
>
> Drop, file was named entirely different. I see other changes, altough
> comparing with b4 is impossible.
>
ack
> Why b4 does not work for this patch?
>
> b4 diff '20250225-qps615_v4_1-v4-1-e08633a7bdf8@oss.qualcomm.com'
> Checking for older revisions
> Grabbing search results from lore.kernel.org
> Nothing matching that query.
>
> Looks like you use b4 but decide to not use b4 changesets/versions. Why
> making it difficult for reviewers and for yourself?
>
There is workspace issue at my end instead of fixing them I tried to
create new branch. I will try to use same changeset from next series
onwards.
>
>> ---
>> .../devicetree/bindings/pci/toshiba,tc956x.yaml | 178 +++++++++++++++++++++
>> 1 file changed, 178 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/toshiba,tc956x.yaml b/Documentation/devicetree/bindings/pci/toshiba,tc956x.yaml
>> new file mode 100644
>> index 000000000000..ffed23004f0d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/toshiba,tc956x.yaml
>
> What is "x" here? Wildcard?
>
>> @@ -0,0 +1,178 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pci/toshiba,tc956x.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Toshiba TC956x PCIe switch
>> +
>> +maintainers:
>> + - Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> +
>> +description: |
>> + Toshiba TC956x PCIe switch has one upstream and three downstream
>
> TC9560? Which one are you using here?
>
'x' is the series of the switch and we use tc9563
>> + ports. The 3rd downstream port has integrated endpoint device of
>> + Ethernet MAC. Other two downstream ports are supposed to connect
>> + to external device.
>> +
>> + The TC956x PCIe switch can be configured through I2C interface before
>> + PCIe link is established to change FTS, ASPM related entry delays,
>> + tx amplitude etc for better power efficiency and functionality.
>> +
>> +properties:
>> + compatible:
>> + items:
>> + - enum:
>> + - "pci1179,0623"
>
> Why quotes?
>
I will remove the, in the next patch.
>> + - const: pciclass,0604
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + i2c-parent:
>> + $ref: /schemas/types.yaml#/definitions/phandle-array
>> + description:
>> + A phandle to the parent I2C node and the slave address of the device
>> + used to do configure tc956x to change FTS, tx amplitude etc.
>> + items:
>> + - description: Phandle to the I2C controller node
>> + - description: I2C slave address
>> +
>> + vdd18-supply: true
>> +
>> + vdd09-supply: true
>> +
>> + vddc-supply: true
>> +
>> + vddio1-supply: true
>> +
>> + vddio2-supply: true
>> +
>> + vddio18-supply: true
>> +
>> + reset-gpios:
>> + maxItems: 1
>> + description:
>> + GPIO controlling the RESX# pin.
>> +
>> +allOf:
>> + - $ref: "#/$defs/tc956x-node"
>> +
>> +patternProperties:
>> + "^pcie@[1-3],0$":
>> + description:
>> + child nodes describing the internal downstream ports
>> + the tc956x switch.
>> + type: object
>> + $ref: "#/$defs/tc956x-node"
>> + unevaluatedProperties: false
>> +
>> +$defs:
>> + tc956x-node:
>> + type: object
>> +
>> + properties:
>> + tc956x,tx-amplitude-microvolt:
>
> You already got comments on this.
>
In V3 I got a comment saying "-microvolt does not work for you?"
so based on this we changed to microvolt.
>> + $ref: /schemas/types.yaml#/definitions/uint32
>
> Never tested.
> >
>> + description:
>> + Change Tx Margin setting for low power consumption.
>> +
>> + tc956x,no-dfe-support:
>
> There is no such vendor prefix and you already got exactly the same
> comment at v3. How did you resolve that comment?
>
It is my mistake I taught the comment was applicable to removed property
"qps615,axi-clk-freq-hz". It is applicable to all the properties I will
correct it in the next patch.

For some reasons there are no errors shown in my workspace when I run
make dt_binding_check DT_SCHEMA_FILES=toshiba,tc956x.yaml
& make CHECK_DTBS=y qcom/qcs6490-rb3gen2.dtb

I will check my tools versions etc and will fix them.

>> + type: boolean
>> + description:
>> + Disable DFE (Decision Feedback Equalizer), which mitigates
>> + intersymbol interference and some reflections caused by impedance mismatches.
>> +
>> + allOf:
>> + - $ref: /schemas/pci/pci-pci-bridge.yaml#
>> +
>> +unevaluatedProperties: false
>
> Keep order as in example-schema.
>
ack.

- Krishna Chaitanya.
> Best regards,
> Krzysztof
>