Re: [PATCH 1/2] dt-bindings: phy: Add YAML schemas for Intel Combo phy

[Rob Herring]

On Wed, Jan 15, 2020 at 1:52 AM Dilip Kota <eswara.kota@xxxxxxxxxxxxxxx> wrote:
>
>
> On 1/14/2020 10:31 PM, Rob Herring wrote:
> > On Tue, Jan 14, 2020 at 3:18 AM Dilip Kota <eswara.kota@xxxxxxxxxxxxxxx> wrote:
> >>
> >> On 1/8/2020 10:18 PM, Rob Herring wrote:
> >>> On Fri, Dec 20, 2019 at 03:28:27PM +0800, Dilip Kota wrote:
> >>>> Combo phy subsystem provides PHY support to number of
> >>>> controllers, viz. PCIe, SATA and EMAC.
> >>>> Adding YAML schemas for the same.
> >>>>
> >>>> Signed-off-by: Dilip Kota <eswara.kota@xxxxxxxxxxxxxxx>
> >>>> ---
> >>>>    .../devicetree/bindings/phy/intel,combo-phy.yaml   | 147 +++++++++++++++++++++
> >>>>    1 file changed, 147 insertions(+)
> >>>>    create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
> >>>> new file mode 100644
> >>>> index 000000000000..fc9cbad9dd88
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
> >>>> @@ -0,0 +1,147 @@
> >>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >>>> +%YAML 1.2
> >>>> +---
> >>>> +$id: http://devicetree.org/schemas/phy/intel,combo-phy.yaml#
> >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>> +
> >>>> +title: Intel Combo phy Subsystem
> >>>> +
> >>>> +maintainers:
> >>>> +  - Dilip Kota <eswara.kota@xxxxxxxxxxxxxxx>
> >>>> +
> >>>> +description: |
> >>>> +  Intel combo phy subsystem supports PHYs for PCIe, EMAC and SATA
> >>>> +  controllers. A single combo phy provides two PHY instances.
> >>>> +
> >>>> +properties:
> >>>> +  $nodename:
> >>>> +    pattern: "^combophy@[0-9]+$"
> >>>> +
> >>>> +  compatible:
> >>>> +    items:
> >>>> +      - const: intel,combo-phy
> >>>> +      - const: simple-bus
> >>> This will cause the schema to be applied to every 'simple-bus'. You need
> >>> a custom 'select' to prevent that. There's several examples in the tree.
> >> Ok, i will add as below:
> >>
> >> # We need a select here so we don't match all nodes with 'simple-bus'
> >> select:
> >>     properties:
> >>       compatible:
> >>         contains:
> >>           const: intel,combo-phy
> >>     required:
> >>       - compatible
> >>
> >>> Though I'm not sure you need child nodes here.
> >>>
> >>>> +
> >>>> +  cell-index:
> >>>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>>> +    description: Index of Combo phy hardware instance.
> >>> Drop this. Not used for FDT.
> >> Ok, I will remove this and use the 'aliases' to get the hardware instance.
> >>>> +
> >>>> +  resets:
> >>>> +    maxItems: 2
> >>>> +
> >>>> +  reset-names:
> >>>> +    items:
> >>>> +      - const: phy
> >>>> +      - const: core
> >>>> +
> >>>> +  intel,syscfg:
> >>>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>>> +    description: Chip configuration registers handle
> >>>> +
> >>>> +  intel,hsio:
> >>>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>>> +    description: HSIO registers handle
> >>>> +
> >>>> +  intel,bid:
> >>>> +    description: Index of HSIO bus
> >>>> +    allOf:
> >>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
> >>>> +      - minimum: 0
> >>>> +      - maximum: 1
> >>> If this is related to intel,hsio, just make it an args cell for
> >>> intel,hsio.
> >> No. Actually, this is specific to the combophy instance on the HSIO bus.
> >> I see , this can be removed and value can be derived from the hardware
> >> instance value mentioned through 'aliases'
> > Generally, 'aliases' should be optional. Why do you need an index?
> > What's the difference between the blocks?
> >
> > If it wasn't clear, I was suggesting doing:
> >
> > intel,hsio = <&hsio 1>;
> On the SoC, total 4 combophy (0,1,2 and 3) instances are present ->
> 'cell-index'
> 2 instances (0,1) are present on the HSIOL NoC
> Other 2 instances (2,3) are present on the HSIOR NoC
> On the both HSIO NoCs, combophy instances are referred as 0 and 1 -> 'bid'

So you would have:

<&hsiol 0>
<&hsiol 1>
<&hsior 0>
<&hsior 1>

However, if HSIO is a bus and the combo phys are not on any other bus,
then perhaps you should be describing the buses in DT and then this
property goes away as these would be child nodes of the bus and
whatever addressing identifiers there are would go in 'reg'.

> 'bid' is required while accessing the registers in hsio block, to
> configure the COMBOPHY mode and clock
> 'cell-index' is required while accessing sysconfig registers to enable
> the pcie phy pad ref clock.

Do the same thing for the sysconfig handle:

<&sysconfig 0|1>

This is the common pattern for these types of properties with misc
extra register bits to go configure. Though more typically the cell
value is a register offset and bit position.

> <&hsio 1>
> 'bid' is specific to the combophy, not all the DT nodes using &hsio has
> a need.
> I think it is better to pass the bid value as a entry of combophy DT node.

intel,hsio is an entry in the combo phy. The meaning of any arg cells
is defined by the combo phy binding (and driver).

> I will add dt entry something like 'hw-instance-id' instead of
> cell-index or aliases.

As I said, we don't do h/w index properties.

Rob