| Message ID | 20220628221404.1444200-2-sean.anderson@seco.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | net: dpaa: Convert to phylink | expand |
On Tue, 28 Jun 2022 18:13:30 -0400, Sean Anderson wrote: > This adds a binding for the SerDes module found on QorIQ processors. The > phy reference has two cells, one for the first lane and one for the > last. This should allow for good support of multi-lane protocols when > (if) they are added. There is no protocol option, because the driver is > designed to be able to completely reconfigure lanes at runtime. > Generally, the phy consumer can select the appropriate protocol using > set_mode. For the most part there is only one protocol controller > (consumer) per lane/protocol combination. The exception to this is the > B4860 processor, which has some lanes which can be connected to > multiple MACs. For that processor, I anticipate the easiest way to > resolve this will be to add an additional cell with a "protocol > controller instance" property. > > Each serdes has a unique set of supported protocols (and lanes). The > support matrix is stored in the driver and is selected based on the > compatible string. It is anticipated that a new compatible string will > need to be added for each serdes on each SoC that drivers support is > added for. There is no "generic" compatible string for this reason. > > There are two PLLs, each of which can be used as the master clock for > each lane. Each PLL has its own reference. For the moment they are > required, because it simplifies the driver implementation. Absent > reference clocks can be modeled by a fixed-clock with a rate of 0. > > Signed-off-by: Sean Anderson <sean.anderson@seco.com> > --- > > Changes in v2: > - Add #clock-cells. This will allow using assigned-clocks* to configure > the PLLs. > - Allow a value of 1 for phy-cells. This allows for compatibility with > the similar (but according to Ioana Ciornei different enough) lynx-28g > binding. > - Document phy cells in the description > - Document the structure of the compatible strings > - Fix example binding having too many cells in regs > - Move compatible first > - Refer to the device in the documentation, rather than the binding > - Remove minItems > - Rename to fsl,lynx-10g.yaml > - Use list for clock-names > > .../devicetree/bindings/phy/fsl,lynx-10g.yaml | 93 +++++++++++++++++++ > 1 file changed, 93 insertions(+) > create mode 100644 Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml: patternProperties:^thermistor@:properties:adi,excitation-current-nanoamp: '$ref' should not be valid under {'const': '$ref'} hint: Standard unit suffix properties don't need a type $ref from schema $id: http://devicetree.org/meta-schemas/core.yaml# /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml: ignoring, error in schema: patternProperties: ^thermistor@: properties: adi,excitation-current-nanoamp Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.example.dtb:0:0: /example-0/spi/ltc2983@0: failed to match any schema with compatible: ['adi,ltc2983'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/ This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
On 6/28/22 10:09 PM, Rob Herring wrote: > On Tue, 28 Jun 2022 18:13:30 -0400, Sean Anderson wrote: >> This adds a binding for the SerDes module found on QorIQ processors. The >> phy reference has two cells, one for the first lane and one for the >> last. This should allow for good support of multi-lane protocols when >> (if) they are added. There is no protocol option, because the driver is >> designed to be able to completely reconfigure lanes at runtime. >> Generally, the phy consumer can select the appropriate protocol using >> set_mode. For the most part there is only one protocol controller >> (consumer) per lane/protocol combination. The exception to this is the >> B4860 processor, which has some lanes which can be connected to >> multiple MACs. For that processor, I anticipate the easiest way to >> resolve this will be to add an additional cell with a "protocol >> controller instance" property. >> >> Each serdes has a unique set of supported protocols (and lanes). The >> support matrix is stored in the driver and is selected based on the >> compatible string. It is anticipated that a new compatible string will >> need to be added for each serdes on each SoC that drivers support is >> added for. There is no "generic" compatible string for this reason. >> >> There are two PLLs, each of which can be used as the master clock for >> each lane. Each PLL has its own reference. For the moment they are >> required, because it simplifies the driver implementation. Absent >> reference clocks can be modeled by a fixed-clock with a rate of 0. >> >> Signed-off-by: Sean Anderson <sean.anderson@seco.com> >> --- >> >> Changes in v2: >> - Add #clock-cells. This will allow using assigned-clocks* to configure >> the PLLs. >> - Allow a value of 1 for phy-cells. This allows for compatibility with >> the similar (but according to Ioana Ciornei different enough) lynx-28g >> binding. >> - Document phy cells in the description >> - Document the structure of the compatible strings >> - Fix example binding having too many cells in regs >> - Move compatible first >> - Refer to the device in the documentation, rather than the binding >> - Remove minItems >> - Rename to fsl,lynx-10g.yaml >> - Use list for clock-names >> >> .../devicetree/bindings/phy/fsl,lynx-10g.yaml | 93 +++++++++++++++++++ >> 1 file changed, 93 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml >> > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' > on your patch (DT_CHECKER_FLAGS is new in v5.13): > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml: patternProperties:^thermistor@:properties:adi,excitation-current-nanoamp: '$ref' should not be valid under {'const': '$ref'} > hint: Standard unit suffix properties don't need a type $ref > from schema $id: http://devicetree.org/meta-schemas/core.yaml# > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml: ignoring, error in schema: patternProperties: ^thermistor@: properties: adi,excitation-current-nanoamp > Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.example.dtb:0:0: /example-0/spi/ltc2983@0: failed to match any schema with compatible: ['adi,ltc2983'] This looks unrelated to this patch AFAICT. --Sean
On Tue, Jun 28, 2022 at 06:13:30PM -0400, Sean Anderson wrote: > This adds a binding for the SerDes module found on QorIQ processors. The > phy reference has two cells, one for the first lane and one for the > last. This should allow for good support of multi-lane protocols when > (if) they are added. There is no protocol option, because the driver is > designed to be able to completely reconfigure lanes at runtime. > Generally, the phy consumer can select the appropriate protocol using > set_mode. For the most part there is only one protocol controller > (consumer) per lane/protocol combination. The exception to this is the > B4860 processor, which has some lanes which can be connected to > multiple MACs. For that processor, I anticipate the easiest way to > resolve this will be to add an additional cell with a "protocol > controller instance" property. > > Each serdes has a unique set of supported protocols (and lanes). The > support matrix is stored in the driver and is selected based on the > compatible string. It is anticipated that a new compatible string will > need to be added for each serdes on each SoC that drivers support is > added for. There is no "generic" compatible string for this reason. > > There are two PLLs, each of which can be used as the master clock for > each lane. Each PLL has its own reference. For the moment they are > required, because it simplifies the driver implementation. Absent > reference clocks can be modeled by a fixed-clock with a rate of 0. > > Signed-off-by: Sean Anderson <sean.anderson@seco.com> > --- > > Changes in v2: > - Add #clock-cells. This will allow using assigned-clocks* to configure > the PLLs. > - Allow a value of 1 for phy-cells. This allows for compatibility with > the similar (but according to Ioana Ciornei different enough) lynx-28g > binding. > - Document phy cells in the description > - Document the structure of the compatible strings > - Fix example binding having too many cells in regs > - Move compatible first > - Refer to the device in the documentation, rather than the binding > - Remove minItems > - Rename to fsl,lynx-10g.yaml > - Use list for clock-names > > .../devicetree/bindings/phy/fsl,lynx-10g.yaml | 93 +++++++++++++++++++ > 1 file changed, 93 insertions(+) > create mode 100644 Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml > > diff --git a/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml b/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml > new file mode 100644 > index 000000000000..b5a6f631df9f > --- /dev/null > +++ b/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml > @@ -0,0 +1,93 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/phy/fsl,lynx-10g.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: NXP Lynx 10G SerDes > + > +maintainers: > + - Sean Anderson <sean.anderson@seco.com> > + > +description: | > + These Lynx "SerDes" devices are found in NXP's QorIQ line of processors. The > + SerDes provides up to eight lanes. Each lane may be configured individually, > + or may be combined with adjacent lanes for a multi-lane protocol. The SerDes > + supports a variety of protocols, including up to 10G Ethernet, PCIe, SATA, and > + others. The specific protocols supported for each lane depend on the > + particular SoC. > + > +properties: > + compatible: > + description: | > + Each compatible is of the form "fsl,<soc-name>-serdes-<instance>". > + Although many registers are compatible between different SoCs, the > + supported protocols and lane assignments tend to be unique to each SerDes. > + Additionally, the method of activating protocols may also be unique. We typically have properties for handling these variables. Numbering instances is something we avoid. > + Because of this, each SerDes instance will need its own compatible string. > + In cases where two SoCs share the same SerDes implementation (such as the > + LS1046A and LS1026A), both SoCs should share the same compatible strings. No, not unless the 2 parts are just fuse or package pinout differences. Otherwise, there's always the chance they are not 'the same' and have different bugs. You could have "fsl,ls1046a-serdes", "fsl,ls1026a-serdes" (whichever SoC is older last) if you think they are 'the same'. > + enum: > + - fsl,ls1046a-serdes-1 > + - fsl,ls1046a-serdes-2 > + > + "#clock-cells": > + const: 1 > + description: | > + The cell contains the index of the PLL, starting from 0. Note that when > + assigning a rate to a PLL, the PLLs' rates are divided by 1000 to avoid > + overflow. A rate of 5000000 corresponds to 5GHz. > + > + "#phy-cells": > + minimum: 1 > + maximum: 2 > + description: | > + The cells contain the following arguments: > + - The first lane in the group. Lanes are numbered based on the register > + offsets, not the I/O ports. This corresponds to the letter-based ("Lane > + A") naming scheme, and not the number-based ("Lane 0") naming scheme. On > + most SoCs, "Lane A" is "Lane 0", but not always. > + - Last lane. For single-lane protocols, this should be the same as the > + first lane. > + If no lanes in a SerDes can be grouped, then #phy-cells may be 1, and the > + first cell will specify the only lane in the group. Usually when we have per lane phys, the consumer side will have a 'phys' entry per lane. Having a variable number of cells isn't great either. We usually only do that when we have to extend an existing binding. > + > + clocks: > + maxItems: 2 > + description: | > + Clock for each PLL reference clock input. > + > + clock-names: > + items: > + - enum: &clocks > + - ref0 > + - ref1 > + - enum: *clocks Whoa, there's a first. Don't use YAML anchors and references. We only support JSON subset of YAML. > + > + reg: > + maxItems: 1 > + > +required: > + - "#clock-cells" > + - "#phy-cells" > + - compatible > + - clocks > + - clock-names > + - reg > + > +additionalProperties: false > + > +examples: > + - | > + serdes1: phy@1ea0000 { > + #clock-cells = <1>; > + #phy-cells = <2>; > + compatible = "fsl,ls1046a-serdes-1"; > + reg = <0x1ea0000 0x2000>; > + clocks = <&clk_100mhz>, <&clk_156mhz>; > + clock-names = "ref0", "ref1"; > + assigned-clocks = <&serdes1 0>; > + assigned-clock-rates = <5000000>; > + }; > + > +... > -- > 2.35.1.1320.gc452695387.dirty > >
Hi Rob, On 6/30/22 1:27 PM, Rob Herring wrote: > On Tue, Jun 28, 2022 at 06:13:30PM -0400, Sean Anderson wrote: >> This adds a binding for the SerDes module found on QorIQ processors. The >> phy reference has two cells, one for the first lane and one for the >> last. This should allow for good support of multi-lane protocols when >> (if) they are added. There is no protocol option, because the driver is >> designed to be able to completely reconfigure lanes at runtime. >> Generally, the phy consumer can select the appropriate protocol using >> set_mode. For the most part there is only one protocol controller >> (consumer) per lane/protocol combination. The exception to this is the >> B4860 processor, which has some lanes which can be connected to >> multiple MACs. For that processor, I anticipate the easiest way to >> resolve this will be to add an additional cell with a "protocol >> controller instance" property. >> >> Each serdes has a unique set of supported protocols (and lanes). The >> support matrix is stored in the driver and is selected based on the >> compatible string. It is anticipated that a new compatible string will >> need to be added for each serdes on each SoC that drivers support is >> added for. There is no "generic" compatible string for this reason. >> >> There are two PLLs, each of which can be used as the master clock for >> each lane. Each PLL has its own reference. For the moment they are >> required, because it simplifies the driver implementation. Absent >> reference clocks can be modeled by a fixed-clock with a rate of 0. >> >> Signed-off-by: Sean Anderson <sean.anderson@seco.com> >> --- >> >> Changes in v2: >> - Add #clock-cells. This will allow using assigned-clocks* to configure >> the PLLs. >> - Allow a value of 1 for phy-cells. This allows for compatibility with >> the similar (but according to Ioana Ciornei different enough) lynx-28g >> binding. >> - Document phy cells in the description >> - Document the structure of the compatible strings >> - Fix example binding having too many cells in regs >> - Move compatible first >> - Refer to the device in the documentation, rather than the binding >> - Remove minItems >> - Rename to fsl,lynx-10g.yaml >> - Use list for clock-names >> >> .../devicetree/bindings/phy/fsl,lynx-10g.yaml | 93 +++++++++++++++++++ >> 1 file changed, 93 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml >> >> diff --git a/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml b/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml >> new file mode 100644 >> index 000000000000..b5a6f631df9f >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml >> @@ -0,0 +1,93 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/phy/fsl,lynx-10g.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: NXP Lynx 10G SerDes >> + >> +maintainers: >> + - Sean Anderson <sean.anderson@seco.com> >> + >> +description: | >> + These Lynx "SerDes" devices are found in NXP's QorIQ line of processors. The >> + SerDes provides up to eight lanes. Each lane may be configured individually, >> + or may be combined with adjacent lanes for a multi-lane protocol. The SerDes >> + supports a variety of protocols, including up to 10G Ethernet, PCIe, SATA, and >> + others. The specific protocols supported for each lane depend on the >> + particular SoC. >> + >> +properties: >> + compatible: >> + description: | >> + Each compatible is of the form "fsl,<soc-name>-serdes-<instance>". >> + Although many registers are compatible between different SoCs, the >> + supported protocols and lane assignments tend to be unique to each SerDes. >> + Additionally, the method of activating protocols may also be unique. > > We typically have properties for handling these variables. Numbering > instances is something we avoid. On v1, Krzysztof said that this was a better route... >> + Because of this, each SerDes instance will need its own compatible string. > >> + In cases where two SoCs share the same SerDes implementation (such as the >> + LS1046A and LS1026A), both SoCs should share the same compatible strings. > > No, not unless the 2 parts are just fuse or package pinout differences. > Otherwise, there's always the chance they are not 'the same' and have > different bugs. > > You could have "fsl,ls1046a-serdes", "fsl,ls1026a-serdes" (whichever SoC > is older last) if you think they are 'the same'. Fine by me. >> + enum: >> + - fsl,ls1046a-serdes-1 >> + - fsl,ls1046a-serdes-2 >> + >> + "#clock-cells": >> + const: 1 >> + description: | >> + The cell contains the index of the PLL, starting from 0. Note that when >> + assigning a rate to a PLL, the PLLs' rates are divided by 1000 to avoid >> + overflow. A rate of 5000000 corresponds to 5GHz. >> + >> + "#phy-cells": >> + minimum: 1 >> + maximum: 2 >> + description: | >> + The cells contain the following arguments: >> + - The first lane in the group. Lanes are numbered based on the register >> + offsets, not the I/O ports. This corresponds to the letter-based ("Lane >> + A") naming scheme, and not the number-based ("Lane 0") naming scheme. On >> + most SoCs, "Lane A" is "Lane 0", but not always. >> + - Last lane. For single-lane protocols, this should be the same as the >> + first lane. >> + If no lanes in a SerDes can be grouped, then #phy-cells may be 1, and the >> + first cell will specify the only lane in the group. > > Usually when we have per lane phys, the consumer side will have a 'phys' > entry per lane. The problem is that it is hard to coordinate multiple lanes coming up at once. There are particular ordering requirements when multiple lanes are grouped together: > Note that if the lanes being reconfigured are grouped for multi-lane or synchronous > mode, then the master source clock lane for the group must have TRST_B set to 1 > after all the other lanes in the group. The master source clock lane of the group > is indicated by LNmGCR0[1STLANE]=1. All lanes p<m (if LNmGCR1[TRSTDIR]=0) or p> (if LNmGCR1[TRSTDIR]=1) of the master source clock lane until the next lane with > LNmGCR0[1STLANE]=1, or the end of the SerDes, are grouped with the master source > clock lane. All grouped lanes must have the same setting of TRSTDIR. This is trivial to do if we enable things as a "group" but not so easy if each lane is a separate phy. In particular, we have to know up front whether lanes are being grouped together in order to program 1STLANE correctly. I think the enable order corresponds to the order in the dtb, but AFAIK that is not guaranteed by the phy subsystem. > Having a variable number of cells isn't great either. We usually only do > that when we have to extend an existing binding. This is mainly to align closer to the lynx-28g binding. It can always be restricted. >> + >> + clocks: >> + maxItems: 2 >> + description: | >> + Clock for each PLL reference clock input. >> + >> + clock-names: >> + items: >> + - enum: &clocks >> + - ref0 >> + - ref1 >> + - enum: *clocks > > Whoa, there's a first. Don't use YAML anchors and references. We only > support JSON subset of YAML. How else should this be expressed? I would like to allow both clock-names = "ref0", "ref1"; and clock-names = "ref1", "ref0"; ideally without repeating myself. --Sean >> + >> + reg: >> + maxItems: 1 >> + >> +required: >> + - "#clock-cells" >> + - "#phy-cells" >> + - compatible >> + - clocks >> + - clock-names >> + - reg >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + serdes1: phy@1ea0000 { >> + #clock-cells = <1>; >> + #phy-cells = <2>; >> + compatible = "fsl,ls1046a-serdes-1"; >> + reg = <0x1ea0000 0x2000>; >> + clocks = <&clk_100mhz>, <&clk_156mhz>; >> + clock-names = "ref0", "ref1"; >> + assigned-clocks = <&serdes1 0>; >> + assigned-clock-rates = <5000000>; >> + }; >> + >> +... >> -- >> 2.35.1.1320.gc452695387.dirty >> >> >
On 30/06/2022 20:01, Sean Anderson wrote: > Hi Rob, > > On 6/30/22 1:27 PM, Rob Herring wrote: >> On Tue, Jun 28, 2022 at 06:13:30PM -0400, Sean Anderson wrote: >>> This adds a binding for the SerDes module found on QorIQ processors. The >>> phy reference has two cells, one for the first lane and one for the >>> last. This should allow for good support of multi-lane protocols when >>> (if) they are added. There is no protocol option, because the driver is >>> designed to be able to completely reconfigure lanes at runtime. >>> Generally, the phy consumer can select the appropriate protocol using >>> set_mode. For the most part there is only one protocol controller >>> (consumer) per lane/protocol combination. The exception to this is the >>> B4860 processor, which has some lanes which can be connected to >>> multiple MACs. For that processor, I anticipate the easiest way to >>> resolve this will be to add an additional cell with a "protocol >>> controller instance" property. >>> >>> Each serdes has a unique set of supported protocols (and lanes). The >>> support matrix is stored in the driver and is selected based on the >>> compatible string. It is anticipated that a new compatible string will >>> need to be added for each serdes on each SoC that drivers support is >>> added for. There is no "generic" compatible string for this reason. >>> >>> There are two PLLs, each of which can be used as the master clock for >>> each lane. Each PLL has its own reference. For the moment they are >>> required, because it simplifies the driver implementation. Absent >>> reference clocks can be modeled by a fixed-clock with a rate of 0. >>> >>> Signed-off-by: Sean Anderson <sean.anderson@seco.com> >>> --- >>> >>> Changes in v2: >>> - Add #clock-cells. This will allow using assigned-clocks* to configure >>> the PLLs. >>> - Allow a value of 1 for phy-cells. This allows for compatibility with >>> the similar (but according to Ioana Ciornei different enough) lynx-28g >>> binding. >>> - Document phy cells in the description >>> - Document the structure of the compatible strings >>> - Fix example binding having too many cells in regs >>> - Move compatible first >>> - Refer to the device in the documentation, rather than the binding >>> - Remove minItems >>> - Rename to fsl,lynx-10g.yaml >>> - Use list for clock-names >>> >>> .../devicetree/bindings/phy/fsl,lynx-10g.yaml | 93 +++++++++++++++++++ >>> 1 file changed, 93 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml b/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml >>> new file mode 100644 >>> index 000000000000..b5a6f631df9f >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml >>> @@ -0,0 +1,93 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/phy/fsl,lynx-10g.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: NXP Lynx 10G SerDes >>> + >>> +maintainers: >>> + - Sean Anderson <sean.anderson@seco.com> >>> + >>> +description: | >>> + These Lynx "SerDes" devices are found in NXP's QorIQ line of processors. The >>> + SerDes provides up to eight lanes. Each lane may be configured individually, >>> + or may be combined with adjacent lanes for a multi-lane protocol. The SerDes >>> + supports a variety of protocols, including up to 10G Ethernet, PCIe, SATA, and >>> + others. The specific protocols supported for each lane depend on the >>> + particular SoC. >>> + >>> +properties: >>> + compatible: >>> + description: | >>> + Each compatible is of the form "fsl,<soc-name>-serdes-<instance>". >>> + Although many registers are compatible between different SoCs, the >>> + supported protocols and lane assignments tend to be unique to each SerDes. >>> + Additionally, the method of activating protocols may also be unique. >> >> We typically have properties for handling these variables. Numbering >> instances is something we avoid. > > On v1, Krzysztof said that this was a better route... I commented about "-1" and "-2" saying you have to make them properties. You disagreed and with long messages were convincing me that "-1" and "-2" is the only reasonable approach. I never said it is a better route. I explicitly asked in several places for defining these as properties, not as compatibles. You are twisting the entire discussion now. Best regards, Krzysztof
On 6/30/22 2:08 PM, Krzysztof Kozlowski wrote: > On 30/06/2022 20:01, Sean Anderson wrote: >> Hi Rob, >> >> On 6/30/22 1:27 PM, Rob Herring wrote: >>> On Tue, Jun 28, 2022 at 06:13:30PM -0400, Sean Anderson wrote: >>>> This adds a binding for the SerDes module found on QorIQ processors. The >>>> phy reference has two cells, one for the first lane and one for the >>>> last. This should allow for good support of multi-lane protocols when >>>> (if) they are added. There is no protocol option, because the driver is >>>> designed to be able to completely reconfigure lanes at runtime. >>>> Generally, the phy consumer can select the appropriate protocol using >>>> set_mode. For the most part there is only one protocol controller >>>> (consumer) per lane/protocol combination. The exception to this is the >>>> B4860 processor, which has some lanes which can be connected to >>>> multiple MACs. For that processor, I anticipate the easiest way to >>>> resolve this will be to add an additional cell with a "protocol >>>> controller instance" property. >>>> >>>> Each serdes has a unique set of supported protocols (and lanes). The >>>> support matrix is stored in the driver and is selected based on the >>>> compatible string. It is anticipated that a new compatible string will >>>> need to be added for each serdes on each SoC that drivers support is >>>> added for. There is no "generic" compatible string for this reason. >>>> >>>> There are two PLLs, each of which can be used as the master clock for >>>> each lane. Each PLL has its own reference. For the moment they are >>>> required, because it simplifies the driver implementation. Absent >>>> reference clocks can be modeled by a fixed-clock with a rate of 0. >>>> >>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com> >>>> --- >>>> >>>> Changes in v2: >>>> - Add #clock-cells. This will allow using assigned-clocks* to configure >>>> the PLLs. >>>> - Allow a value of 1 for phy-cells. This allows for compatibility with >>>> the similar (but according to Ioana Ciornei different enough) lynx-28g >>>> binding. >>>> - Document phy cells in the description >>>> - Document the structure of the compatible strings >>>> - Fix example binding having too many cells in regs >>>> - Move compatible first >>>> - Refer to the device in the documentation, rather than the binding >>>> - Remove minItems >>>> - Rename to fsl,lynx-10g.yaml >>>> - Use list for clock-names >>>> >>>> .../devicetree/bindings/phy/fsl,lynx-10g.yaml | 93 +++++++++++++++++++ >>>> 1 file changed, 93 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml >>>> >>>> diff --git a/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml b/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml >>>> new file mode 100644 >>>> index 000000000000..b5a6f631df9f >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml >>>> @@ -0,0 +1,93 @@ >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>> +%YAML 1.2 >>>> +--- >>>> +$id: http://devicetree.org/schemas/phy/fsl,lynx-10g.yaml# >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>> + >>>> +title: NXP Lynx 10G SerDes >>>> + >>>> +maintainers: >>>> + - Sean Anderson <sean.anderson@seco.com> >>>> + >>>> +description: | >>>> + These Lynx "SerDes" devices are found in NXP's QorIQ line of processors. The >>>> + SerDes provides up to eight lanes. Each lane may be configured individually, >>>> + or may be combined with adjacent lanes for a multi-lane protocol. The SerDes >>>> + supports a variety of protocols, including up to 10G Ethernet, PCIe, SATA, and >>>> + others. The specific protocols supported for each lane depend on the >>>> + particular SoC. >>>> + >>>> +properties: >>>> + compatible: >>>> + description: | >>>> + Each compatible is of the form "fsl,<soc-name>-serdes-<instance>". >>>> + Although many registers are compatible between different SoCs, the >>>> + supported protocols and lane assignments tend to be unique to each SerDes. >>>> + Additionally, the method of activating protocols may also be unique. >>> >> >> On v1, Krzysztof said that this was a better route... > > I commented about "-1" and "-2" saying you have to make them properties. > You disagreed and with long messages were convincing me that "-1" and > "-2" is the only reasonable approach. I never said it is a better route. > I explicitly asked in several places for defining these as properties, > not as compatibles. > > You are twisting the entire discussion now. Sorry, I didn't mean to come off that way. >>> We typically have properties for handling these variables. Numbering >>> instances is something we avoid. But I would like to point out that for the phy subsystem, it is typical to select using the compatible (or just write a new driver). --Sean
diff --git a/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml b/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml new file mode 100644 index 000000000000..b5a6f631df9f --- /dev/null +++ b/Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml @@ -0,0 +1,93 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/phy/fsl,lynx-10g.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: NXP Lynx 10G SerDes + +maintainers: + - Sean Anderson <sean.anderson@seco.com> + +description: | + These Lynx "SerDes" devices are found in NXP's QorIQ line of processors. The + SerDes provides up to eight lanes. Each lane may be configured individually, + or may be combined with adjacent lanes for a multi-lane protocol. The SerDes + supports a variety of protocols, including up to 10G Ethernet, PCIe, SATA, and + others. The specific protocols supported for each lane depend on the + particular SoC. + +properties: + compatible: + description: | + Each compatible is of the form "fsl,<soc-name>-serdes-<instance>". + Although many registers are compatible between different SoCs, the + supported protocols and lane assignments tend to be unique to each SerDes. + Additionally, the method of activating protocols may also be unique. + Because of this, each SerDes instance will need its own compatible string. + In cases where two SoCs share the same SerDes implementation (such as the + LS1046A and LS1026A), both SoCs should share the same compatible strings. + enum: + - fsl,ls1046a-serdes-1 + - fsl,ls1046a-serdes-2 + + "#clock-cells": + const: 1 + description: | + The cell contains the index of the PLL, starting from 0. Note that when + assigning a rate to a PLL, the PLLs' rates are divided by 1000 to avoid + overflow. A rate of 5000000 corresponds to 5GHz. + + "#phy-cells": + minimum: 1 + maximum: 2 + description: | + The cells contain the following arguments: + - The first lane in the group. Lanes are numbered based on the register + offsets, not the I/O ports. This corresponds to the letter-based ("Lane + A") naming scheme, and not the number-based ("Lane 0") naming scheme. On + most SoCs, "Lane A" is "Lane 0", but not always. + - Last lane. For single-lane protocols, this should be the same as the + first lane. + If no lanes in a SerDes can be grouped, then #phy-cells may be 1, and the + first cell will specify the only lane in the group. + + clocks: + maxItems: 2 + description: | + Clock for each PLL reference clock input. + + clock-names: + items: + - enum: &clocks + - ref0 + - ref1 + - enum: *clocks + + reg: + maxItems: 1 + +required: + - "#clock-cells" + - "#phy-cells" + - compatible + - clocks + - clock-names + - reg + +additionalProperties: false + +examples: + - | + serdes1: phy@1ea0000 { + #clock-cells = <1>; + #phy-cells = <2>; + compatible = "fsl,ls1046a-serdes-1"; + reg = <0x1ea0000 0x2000>; + clocks = <&clk_100mhz>, <&clk_156mhz>; + clock-names = "ref0", "ref1"; + assigned-clocks = <&serdes1 0>; + assigned-clock-rates = <5000000>; + }; + +...
This adds a binding for the SerDes module found on QorIQ processors. The phy reference has two cells, one for the first lane and one for the last. This should allow for good support of multi-lane protocols when (if) they are added. There is no protocol option, because the driver is designed to be able to completely reconfigure lanes at runtime. Generally, the phy consumer can select the appropriate protocol using set_mode. For the most part there is only one protocol controller (consumer) per lane/protocol combination. The exception to this is the B4860 processor, which has some lanes which can be connected to multiple MACs. For that processor, I anticipate the easiest way to resolve this will be to add an additional cell with a "protocol controller instance" property. Each serdes has a unique set of supported protocols (and lanes). The support matrix is stored in the driver and is selected based on the compatible string. It is anticipated that a new compatible string will need to be added for each serdes on each SoC that drivers support is added for. There is no "generic" compatible string for this reason. There are two PLLs, each of which can be used as the master clock for each lane. Each PLL has its own reference. For the moment they are required, because it simplifies the driver implementation. Absent reference clocks can be modeled by a fixed-clock with a rate of 0. Signed-off-by: Sean Anderson <sean.anderson@seco.com> --- Changes in v2: - Add #clock-cells. This will allow using assigned-clocks* to configure the PLLs. - Allow a value of 1 for phy-cells. This allows for compatibility with the similar (but according to Ioana Ciornei different enough) lynx-28g binding. - Document phy cells in the description - Document the structure of the compatible strings - Fix example binding having too many cells in regs - Move compatible first - Refer to the device in the documentation, rather than the binding - Remove minItems - Rename to fsl,lynx-10g.yaml - Use list for clock-names .../devicetree/bindings/phy/fsl,lynx-10g.yaml | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml