| Message ID | 9f049a5fccd080bd5d8e9a697b96d4c40a413a0a.1583127977.git.eswara.kota@linux.intel.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add Intel ComboPhy driver | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | fail | build log |
| robh/checkpatch | success | |
| robh/dt-meta-schema | success |
On Mon, Mar 02, 2020 at 04:43:24PM +0800, Dilip Kota wrote: > Combophy subsystem provides PHY support to various > controllers, viz. PCIe, SATA and EMAC. > Adding YAML schemas for the same. > > Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> > --- > Changes on v4: > No changes. > > Changes on v3: > > Add include/dt-bindings/phy/phy-intel-combphy.h for phy modes. > Add SoC specific compatible "intel,combophy-lgm". > Correct the nodename pattern. > clocks description removed and add maxItems entry. > Remove "simple-bus" as it expects minimum one address > cell and size cell in the children node. Call devm_of_platform_populate() > in the driver to perform "simple-bus" functionality. > > Changes on v2: > > Add custom 'select' > Pass hardware instance entries with phandles and > remove cell-index and bid entries > Clock, register address space, are same for the children. > So move them to parent node. > Two PHY instances cannot run in different modes, > so move the phy-mode entry to parent node. > Add second child entry in the DT example. > > .../devicetree/bindings/phy/intel,combo-phy.yaml | 123 +++++++++++++++++++++ > include/dt-bindings/phy/phy-intel-combophy.h | 10 ++ > 2 files changed, 133 insertions(+) > create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml > create mode 100644 include/dt-bindings/phy/phy-intel-combophy.h > > 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..f9bae37fab17 > --- /dev/null > +++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml > @@ -0,0 +1,123 @@ > +# 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 ComboPhy Subsystem > + > +maintainers: > + - Dilip Kota <eswara.kota@linux.intel.com> > + > +description: | > + Intel Combophy subsystem supports PHYs for PCIe, EMAC and SATA > + controllers. A single Combophy provides two PHY instances. > + > +properties: > + $nodename: > + pattern: "combophy(@.*|-[0-9a-f])*$" > + > + compatible: > + items: > + - const: intel,combophy-lgm > + - const: intel,combo-phy > + > + clocks: > + maxItems: 1 > + > + reg: > + items: > + - description: ComboPhy core registers > + - description: PCIe app core control registers > + > + reg-names: > + items: > + - const: core > + - const: app > + > + resets: > + maxItems: 2 > + > + reset-names: > + items: > + - const: phy > + - const: core > + > + intel,syscfg: > + $ref: /schemas/types.yaml#/definitions/phandle-array > + description: Chip configuration registers handle and ComboPhy instance id > + > + intel,hsio: > + $ref: /schemas/types.yaml#/definitions/phandle-array > + description: HSIO registers handle and ComboPhy instance id on NOC > + > + intel,aggregation: > + type: boolean > + description: | > + Specify the flag to configure ComboPHY in dual lane mode. > + > + intel,phy-mode: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: | > + Mode of the two phys in ComboPhy. > + See dt-bindings/phy/phy-intel-combophy.h for values. > + > +patternProperties: > + "^phy@[0-9]+$": > + type: object > + > + properties: > + compatible: > + const: intel,phydev > + > + "#phy-cells": > + const: 0 > + > + resets: > + description: | > + reset handle according to the PHY mode. > + See ../reset/reset.txt for details. > + > + required: > + - compatible > + - "#phy-cells" > + > +required: > + - compatible > + - clocks > + - reg > + - reg-names > + - intel,syscfg > + - intel,hsio > + - intel,phy-mode > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/phy/phy-intel-combophy.h> > + combophy@d0a00000 { > + compatible = "intel,combophy-lgm", "intel,combo-phy"; > + clocks = <&cgu0 1>; > + reg = <0xd0a00000 0x40000>, > + <0xd0a40000 0x1000>; > + reg-names = "core", "app"; > + resets = <&rcu0 0x50 6>, > + <&rcu0 0x50 17>; > + reset-names = "phy", "core"; > + intel,syscfg = <&sysconf 0>; > + intel,hsio = <&hsiol 0>; > + intel,phy-mode = <COMBO_PHY_PCIE>; > + > + phy@0 { You need a 'reg' property to go with a unit-address. Really, I'd just simplify this to make parent 'resets' be 4 entries and put '#phy-cells = <1>;' in the parent. Then you don't need these child nodes. > + compatible = "intel,phydev"; > + #phy-cells = <0>; > + resets = <&rcu0 0x50 23>; > + }; > + > + phy@1 { > + compatible = "intel,phydev"; > + #phy-cells = <0>; > + resets = <&rcu0 0x50 24>; > + }; > + }; > diff --git a/include/dt-bindings/phy/phy-intel-combophy.h b/include/dt-bindings/phy/phy-intel-combophy.h > new file mode 100644 > index 000000000000..bd7f6377f4ef > --- /dev/null > +++ b/include/dt-bindings/phy/phy-intel-combophy.h > @@ -0,0 +1,10 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +#ifndef _DT_BINDINGS_INTEL_COMBOPHY > +#define _DT_BINDINGS_INTEL_COMBOPHY > + > +#define COMBO_PHY_PCIE 0 > +#define COMBO_PHY_XPCS 1 > +#define COMBO_PHY_SATA 2 Use the PHY_TYPE_* defines we already have and extend as you need to. > + > +#endif /* _DT_BINDINGS_INTEL_COMBOPHY*/ > -- > 2.11.0 >
On 3/3/2020 9:50 AM, Rob Herring wrote: > On Mon, Mar 02, 2020 at 04:43:24PM +0800, Dilip Kota wrote: >> Combophy subsystem provides PHY support to various >> controllers, viz. PCIe, SATA and EMAC. >> Adding YAML schemas for the same. >> >> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> >> --- >> Changes on v4: >> No changes. ... >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/phy/phy-intel-combophy.h> >> + combophy@d0a00000 { >> + compatible = "intel,combophy-lgm", "intel,combo-phy"; >> + clocks = <&cgu0 1>; >> + reg = <0xd0a00000 0x40000>, >> + <0xd0a40000 0x1000>; >> + reg-names = "core", "app"; >> + resets = <&rcu0 0x50 6>, >> + <&rcu0 0x50 17>; >> + reset-names = "phy", "core"; >> + intel,syscfg = <&sysconf 0>; >> + intel,hsio = <&hsiol 0>; >> + intel,phy-mode = <COMBO_PHY_PCIE>; >> + >> + phy@0 { > You need a 'reg' property to go with a unit-address. > > Really, I'd just simplify this to make parent 'resets' be 4 entries and > put '#phy-cells = <1>;' in the parent. Then you don't need these child > nodes. If child nodes are not present, use case like PCIe controller-0 using phy@0 and PCIe controller-1 using phy@1 wont be possible. > >> + compatible = "intel,phydev"; >> + #phy-cells = <0>; >> + resets = <&rcu0 0x50 23>; >> + }; >> + >> + phy@1 { >> + compatible = "intel,phydev"; >> + #phy-cells = <0>; >> + resets = <&rcu0 0x50 24>; >> + }; >> + }; >> diff --git a/include/dt-bindings/phy/phy-intel-combophy.h b/include/dt-bindings/phy/phy-intel-combophy.h >> new file mode 100644 >> index 000000000000..bd7f6377f4ef >> --- /dev/null >> +++ b/include/dt-bindings/phy/phy-intel-combophy.h >> @@ -0,0 +1,10 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> + >> +#ifndef _DT_BINDINGS_INTEL_COMBOPHY >> +#define _DT_BINDINGS_INTEL_COMBOPHY >> + >> +#define COMBO_PHY_PCIE 0 >> +#define COMBO_PHY_XPCS 1 >> +#define COMBO_PHY_SATA 2 > Use the PHY_TYPE_* defines we already have and extend as you need to. Sure, will do the same. Regards, Dilip > >> + >> +#endif /* _DT_BINDINGS_INTEL_COMBOPHY*/ >> -- >> 2.11.0 >>
On Tue, Mar 3, 2020 at 3:24 AM Dilip Kota <eswara.kota@linux.intel.com> wrote: > > > On 3/3/2020 9:50 AM, Rob Herring wrote: > > On Mon, Mar 02, 2020 at 04:43:24PM +0800, Dilip Kota wrote: > >> Combophy subsystem provides PHY support to various > >> controllers, viz. PCIe, SATA and EMAC. > >> Adding YAML schemas for the same. > >> > >> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> > >> --- > >> Changes on v4: > >> No changes. > ... > >> +additionalProperties: false > >> + > >> +examples: > >> + - | > >> + #include <dt-bindings/phy/phy-intel-combophy.h> > >> + combophy@d0a00000 { > >> + compatible = "intel,combophy-lgm", "intel,combo-phy"; > >> + clocks = <&cgu0 1>; > >> + reg = <0xd0a00000 0x40000>, > >> + <0xd0a40000 0x1000>; > >> + reg-names = "core", "app"; > >> + resets = <&rcu0 0x50 6>, > >> + <&rcu0 0x50 17>; > >> + reset-names = "phy", "core"; > >> + intel,syscfg = <&sysconf 0>; > >> + intel,hsio = <&hsiol 0>; > >> + intel,phy-mode = <COMBO_PHY_PCIE>; > >> + > >> + phy@0 { > > You need a 'reg' property to go with a unit-address. > > > > Really, I'd just simplify this to make parent 'resets' be 4 entries and > > put '#phy-cells = <1>;' in the parent. Then you don't need these child > > nodes. > If child nodes are not present, use case like PCIe controller-0 using > phy@0 and PCIe controller-1 using phy@1 wont be possible. Yes, it will be. For controller-0: phys = <&phy 0>; For controller-1: phys = <&phy 1>; Rob
On 3/4/2020 12:26 AM, Rob Herring wrote: > On Tue, Mar 3, 2020 at 3:24 AM Dilip Kota <eswara.kota@linux.intel.com> wrote: >> >> On 3/3/2020 9:50 AM, Rob Herring wrote: >>> On Mon, Mar 02, 2020 at 04:43:24PM +0800, Dilip Kota wrote: >>>> Combophy subsystem provides PHY support to various >>>> controllers, viz. PCIe, SATA and EMAC. >>>> Adding YAML schemas for the same. >>>> >>>> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> >>>> --- >>>> Changes on v4: >>>> No changes. >> ... >>>> +additionalProperties: false >>>> + >>>> +examples: >>>> + - | >>>> + #include <dt-bindings/phy/phy-intel-combophy.h> >>>> + combophy@d0a00000 { >>>> + compatible = "intel,combophy-lgm", "intel,combo-phy"; >>>> + clocks = <&cgu0 1>; >>>> + reg = <0xd0a00000 0x40000>, >>>> + <0xd0a40000 0x1000>; >>>> + reg-names = "core", "app"; >>>> + resets = <&rcu0 0x50 6>, >>>> + <&rcu0 0x50 17>; >>>> + reset-names = "phy", "core"; >>>> + intel,syscfg = <&sysconf 0>; >>>> + intel,hsio = <&hsiol 0>; >>>> + intel,phy-mode = <COMBO_PHY_PCIE>; >>>> + >>>> + phy@0 { >>> You need a 'reg' property to go with a unit-address. >>> >>> Really, I'd just simplify this to make parent 'resets' be 4 entries and >>> put '#phy-cells = <1>;' in the parent. Then you don't need these child >>> nodes. >> If child nodes are not present, use case like PCIe controller-0 using >> phy@0 and PCIe controller-1 using phy@1 wont be possible. > Yes, it will be. > > For controller-0: > phys = <&phy 0>; > > For controller-1: > phys = <&phy 1>; OH got it, arg cell can be utilized for PHY id. I started working on your suggestion in simplifying it, but below point is haunting while doing the changes. So felt to check with you whether the better one is going with existing DT node or the one without child nodes!. Existing DT node skeleton, replicates hardware design ComboPhy with 2 PHYs. (ComboPhy as parent node and 2PHYs as child nodes) Regards, Dilip > > Rob
Hi Rob, On 3/4/2020 5:16 PM, Dilip Kota wrote: > > On 3/4/2020 12:26 AM, Rob Herring wrote: >> On Tue, Mar 3, 2020 at 3:24 AM Dilip Kota >> <eswara.kota@linux.intel.com> wrote: >>> >>> On 3/3/2020 9:50 AM, Rob Herring wrote: >>>> On Mon, Mar 02, 2020 at 04:43:24PM +0800, Dilip Kota wrote: >>>>> Combophy subsystem provides PHY support to various >>>>> controllers, viz. PCIe, SATA and EMAC. >>>>> Adding YAML schemas for the same. ... >>>>> + - | >>>>> + #include <dt-bindings/phy/phy-intel-combophy.h> >>>>> + combophy@d0a00000 { >>>>> + compatible = "intel,combophy-lgm", "intel,combo-phy"; >>>>> + clocks = <&cgu0 1>; >>>>> + reg = <0xd0a00000 0x40000>, >>>>> + <0xd0a40000 0x1000>; >>>>> + reg-names = "core", "app"; >>>>> + resets = <&rcu0 0x50 6>, >>>>> + <&rcu0 0x50 17>; >>>>> + reset-names = "phy", "core"; >>>>> + intel,syscfg = <&sysconf 0>; >>>>> + intel,hsio = <&hsiol 0>; >>>>> + intel,phy-mode = <COMBO_PHY_PCIE>; >>>>> + >>>>> + phy@0 { >>>> You need a 'reg' property to go with a unit-address. >>>> >>>> Really, I'd just simplify this to make parent 'resets' be 4 entries >>>> and >>>> put '#phy-cells = <1>;' in the parent. Then you don't need these child >>>> nodes. >>> If child nodes are not present, use case like PCIe controller-0 using >>> phy@0 and PCIe controller-1 using phy@1 wont be possible. >> Yes, it will be. >> >> For controller-0: >> phys = <&phy 0>; >> >> For controller-1: >> phys = <&phy 1>; > > OH got it, arg cell can be utilized for PHY id. > I started working on your suggestion in simplifying it, but below > point is haunting while doing the changes. So felt to check with you > whether the better one is going with existing DT node or the one > without child nodes!. > Existing DT node skeleton, replicates hardware design ComboPhy > with 2 PHYs. (ComboPhy as parent node and 2PHYs as child nodes) In the patchwork, i see the patch state is 'Change Requested', so felt to keep a remainder mail for your inputs on above query. I have waiting to push the appropriate code changes based on your comment. Thanks, Dilip > > Regards, > Dilip > >> >> Rob
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..f9bae37fab17 --- /dev/null +++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml @@ -0,0 +1,123 @@ +# 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 ComboPhy Subsystem + +maintainers: + - Dilip Kota <eswara.kota@linux.intel.com> + +description: | + Intel Combophy subsystem supports PHYs for PCIe, EMAC and SATA + controllers. A single Combophy provides two PHY instances. + +properties: + $nodename: + pattern: "combophy(@.*|-[0-9a-f])*$" + + compatible: + items: + - const: intel,combophy-lgm + - const: intel,combo-phy + + clocks: + maxItems: 1 + + reg: + items: + - description: ComboPhy core registers + - description: PCIe app core control registers + + reg-names: + items: + - const: core + - const: app + + resets: + maxItems: 2 + + reset-names: + items: + - const: phy + - const: core + + intel,syscfg: + $ref: /schemas/types.yaml#/definitions/phandle-array + description: Chip configuration registers handle and ComboPhy instance id + + intel,hsio: + $ref: /schemas/types.yaml#/definitions/phandle-array + description: HSIO registers handle and ComboPhy instance id on NOC + + intel,aggregation: + type: boolean + description: | + Specify the flag to configure ComboPHY in dual lane mode. + + intel,phy-mode: + $ref: /schemas/types.yaml#/definitions/uint32 + description: | + Mode of the two phys in ComboPhy. + See dt-bindings/phy/phy-intel-combophy.h for values. + +patternProperties: + "^phy@[0-9]+$": + type: object + + properties: + compatible: + const: intel,phydev + + "#phy-cells": + const: 0 + + resets: + description: | + reset handle according to the PHY mode. + See ../reset/reset.txt for details. + + required: + - compatible + - "#phy-cells" + +required: + - compatible + - clocks + - reg + - reg-names + - intel,syscfg + - intel,hsio + - intel,phy-mode + +additionalProperties: false + +examples: + - | + #include <dt-bindings/phy/phy-intel-combophy.h> + combophy@d0a00000 { + compatible = "intel,combophy-lgm", "intel,combo-phy"; + clocks = <&cgu0 1>; + reg = <0xd0a00000 0x40000>, + <0xd0a40000 0x1000>; + reg-names = "core", "app"; + resets = <&rcu0 0x50 6>, + <&rcu0 0x50 17>; + reset-names = "phy", "core"; + intel,syscfg = <&sysconf 0>; + intel,hsio = <&hsiol 0>; + intel,phy-mode = <COMBO_PHY_PCIE>; + + phy@0 { + compatible = "intel,phydev"; + #phy-cells = <0>; + resets = <&rcu0 0x50 23>; + }; + + phy@1 { + compatible = "intel,phydev"; + #phy-cells = <0>; + resets = <&rcu0 0x50 24>; + }; + }; diff --git a/include/dt-bindings/phy/phy-intel-combophy.h b/include/dt-bindings/phy/phy-intel-combophy.h new file mode 100644 index 000000000000..bd7f6377f4ef --- /dev/null +++ b/include/dt-bindings/phy/phy-intel-combophy.h @@ -0,0 +1,10 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef _DT_BINDINGS_INTEL_COMBOPHY +#define _DT_BINDINGS_INTEL_COMBOPHY + +#define COMBO_PHY_PCIE 0 +#define COMBO_PHY_XPCS 1 +#define COMBO_PHY_SATA 2 + +#endif /* _DT_BINDINGS_INTEL_COMBOPHY*/
Combophy subsystem provides PHY support to various controllers, viz. PCIe, SATA and EMAC. Adding YAML schemas for the same. Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> --- Changes on v4: No changes. Changes on v3: Add include/dt-bindings/phy/phy-intel-combphy.h for phy modes. Add SoC specific compatible "intel,combophy-lgm". Correct the nodename pattern. clocks description removed and add maxItems entry. Remove "simple-bus" as it expects minimum one address cell and size cell in the children node. Call devm_of_platform_populate() in the driver to perform "simple-bus" functionality. Changes on v2: Add custom 'select' Pass hardware instance entries with phandles and remove cell-index and bid entries Clock, register address space, are same for the children. So move them to parent node. Two PHY instances cannot run in different modes, so move the phy-mode entry to parent node. Add second child entry in the DT example. .../devicetree/bindings/phy/intel,combo-phy.yaml | 123 +++++++++++++++++++++ include/dt-bindings/phy/phy-intel-combophy.h | 10 ++ 2 files changed, 133 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml create mode 100644 include/dt-bindings/phy/phy-intel-combophy.h