| Message ID | 20240627004142.8106-7-fancer.lancer@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: pcs: xpcs: Add memory-mapped device support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next-0 |
On Thu, Jun 27, 2024 at 03:41:26AM +0300, Serge Semin wrote: > + clocks: > + description: > + Both MCI and APB3 interfaces are supposed to be equipped with a clock > + source connected via the clk_csr_i line. > + > + PCS/PMA layer can be clocked by an internal reference clock source > + (phyN_core_refclk) or by an externally connected (phyN_pad_refclk) clock > + generator. Both clocks can be supplied at a time. > + minItems: 1 > + maxItems: 3 > + > + clock-names: > + oneOf: > + - minItems: 1 > + items: > + - enum: [core, pad] > + - const: pad > + - minItems: 1 > + items: > + - const: pclk > + - enum: [core, pad] > + - const: pad While reading this, I'm kinda struggling to map "clk_csr_i" to a clock name. Is that pclk? And why pclk if it is connected to "clk_csr_i"? If two interfaces are meant to be "equipped" with that clock, how come it is optional? I'm probably missing something... Otherwise this binding looks fine to me. Wee bit confused, Conor.
On Thu, Jun 27, 2024 at 04:51:22PM +0100, Conor Dooley wrote: > On Thu, Jun 27, 2024 at 03:41:26AM +0300, Serge Semin wrote: > > + clocks: > > + description: > > + Both MCI and APB3 interfaces are supposed to be equipped with a clock > > + source connected via the clk_csr_i line. > > + > > + PCS/PMA layer can be clocked by an internal reference clock source > > + (phyN_core_refclk) or by an externally connected (phyN_pad_refclk) clock > > + generator. Both clocks can be supplied at a time. > > + minItems: 1 > > + maxItems: 3 > > + > > + clock-names: > > + oneOf: > > + - minItems: 1 > > + items: > > + - enum: [core, pad] > > + - const: pad > > + - minItems: 1 > > + items: > > + - const: pclk > > + - enum: [core, pad] > > + - const: pad > > While reading this, I'm kinda struggling to map "clk_csr_i" to a clock > name. Is that pclk? And why pclk if it is connected to "clk_csr_i"? Right. It's "pclk". The reason of using the "pclk" name is that it has turned to be a de-facto standard name in the DT-bindings for the peripheral bus clock sources utilized for the CSR-space IO buses. Moreover the STMMAC driver responsible for the parental DW *MAC devices handling also has the "pclk" name utilized for the clk_csr_i signal. So using the "pclk" name in the tightly coupled devices (MAC and PCS) for the same signal seemed a good idea. > If two interfaces are meant to be "equipped" with that clock, how come > it is optional? I'm probably missing something... MCI and APB3 interfaces are basically the same from the bindings pointer of view. Both of them can be utilized for the DW XPCS installed on the SoC system bus, so the device could be accessed using the simple MMIO ops. The first "clock-names" schema is meant to be applied on the DW XPCS accessible over an _MDIO_ bus, which obviously doesn't have any special CSR IO bus. In that case the DW XPCS device is supposed to be defined as a subnode of the MDIO-bus DT-node. The second "clock-names" constraint is supposed to be applied to the DW XPCS synthesized with the MCI/APB3 CSRs IO interface. The device in that case should be defined in the DT source file as a normal memory mapped device. > > Otherwise this binding looks fine to me. Shall I add a note to the clock description that the "clk_csr_i" signal is named as "pclk"? I'll need to resubmit the series anyway. Thanks -Serge(y) > > Wee bit confused, > Conor.
On Thu, Jun 27, 2024 at 08:10:48PM +0300, Serge Semin wrote: > On Thu, Jun 27, 2024 at 04:51:22PM +0100, Conor Dooley wrote: > > On Thu, Jun 27, 2024 at 03:41:26AM +0300, Serge Semin wrote: > > > + clocks: > > > + description: > > > + Both MCI and APB3 interfaces are supposed to be equipped with a clock > > > + source connected via the clk_csr_i line. > > > + > > > + PCS/PMA layer can be clocked by an internal reference clock source > > > + (phyN_core_refclk) or by an externally connected (phyN_pad_refclk) clock > > > + generator. Both clocks can be supplied at a time. > > > + minItems: 1 > > > + maxItems: 3 > > > + > > > + clock-names: > > > + oneOf: > > > + - minItems: 1 > > > + items: > > > + - enum: [core, pad] > > > + - const: pad > > > + - minItems: 1 > > > + items: > > > + - const: pclk > > > + - enum: [core, pad] > > > + - const: pad > > > > > While reading this, I'm kinda struggling to map "clk_csr_i" to a clock > > name. Is that pclk? And why pclk if it is connected to "clk_csr_i"? > > Right. It's "pclk". The reason of using the "pclk" name is that it has > turned to be a de-facto standard name in the DT-bindings for the > peripheral bus clock sources utilized for the CSR-space IO buses. > Moreover the STMMAC driver responsible for the parental DW *MAC > devices handling also has the "pclk" name utilized for the clk_csr_i > signal. So using the "pclk" name in the tightly coupled devices (MAC > and PCS) for the same signal seemed a good idea. > > > If two interfaces are meant to be "equipped" with that clock, how come > > it is optional? I'm probably missing something... > > MCI and APB3 interfaces are basically the same from the bindings > pointer of view. Both of them can be utilized for the DW XPCS > installed on the SoC system bus, so the device could be accessed using > the simple MMIO ops. > > The first "clock-names" schema is meant to be applied on the DW XPCS > accessible over an _MDIO_ bus, which obviously doesn't have any > special CSR IO bus. In that case the DW XPCS device is supposed to be > defined as a subnode of the MDIO-bus DT-node. > > The second "clock-names" constraint is supposed to be applied to the > DW XPCS synthesized with the MCI/APB3 CSRs IO interface. The device in > that case should be defined in the DT source file as a normal memory > mapped device. > > > > > Otherwise this binding looks fine to me. > > Shall I add a note to the clock description that the "clk_csr_i" > signal is named as "pclk"? I'll need to resubmit the series anyway. Better yet, could you mention MDIO? It wasn't clear to me (but I'm just reviewing bindings not a dwmac-ist) that MCI and APB3 were only two of the options and that the first clock-names was for MDIO. Maybe something like: clock-names: oneOf: - minItems: 1 items: # MDIO - enum: [core, pad] - const: pad - minItems: 1 items: # MCI or APB - const: pclk - enum: [core, pad] - const: pad
On Fri, Jun 28, 2024 at 05:42:58PM +0100, Conor Dooley wrote: > On Thu, Jun 27, 2024 at 08:10:48PM +0300, Serge Semin wrote: > > On Thu, Jun 27, 2024 at 04:51:22PM +0100, Conor Dooley wrote: > > > On Thu, Jun 27, 2024 at 03:41:26AM +0300, Serge Semin wrote: > > > > + clocks: > > > > + description: > > > > + Both MCI and APB3 interfaces are supposed to be equipped with a clock > > > > + source connected via the clk_csr_i line. > > > > + > > > > + PCS/PMA layer can be clocked by an internal reference clock source > > > > + (phyN_core_refclk) or by an externally connected (phyN_pad_refclk) clock > > > > + generator. Both clocks can be supplied at a time. > > > > + minItems: 1 > > > > + maxItems: 3 > > > > + > > > > + clock-names: > > > > + oneOf: > > > > + - minItems: 1 > > > > + items: > > > > + - enum: [core, pad] > > > > + - const: pad > > > > + - minItems: 1 > > > > + items: > > > > + - const: pclk > > > > + - enum: [core, pad] > > > > + - const: pad > > > > > > > > While reading this, I'm kinda struggling to map "clk_csr_i" to a clock > > > name. Is that pclk? And why pclk if it is connected to "clk_csr_i"? > > > > Right. It's "pclk". The reason of using the "pclk" name is that it has > > turned to be a de-facto standard name in the DT-bindings for the > > peripheral bus clock sources utilized for the CSR-space IO buses. > > Moreover the STMMAC driver responsible for the parental DW *MAC > > devices handling also has the "pclk" name utilized for the clk_csr_i > > signal. So using the "pclk" name in the tightly coupled devices (MAC > > and PCS) for the same signal seemed a good idea. > > > > > If two interfaces are meant to be "equipped" with that clock, how come > > > it is optional? I'm probably missing something... > > > > MCI and APB3 interfaces are basically the same from the bindings > > pointer of view. Both of them can be utilized for the DW XPCS > > installed on the SoC system bus, so the device could be accessed using > > the simple MMIO ops. > > > > The first "clock-names" schema is meant to be applied on the DW XPCS > > accessible over an _MDIO_ bus, which obviously doesn't have any > > special CSR IO bus. In that case the DW XPCS device is supposed to be > > defined as a subnode of the MDIO-bus DT-node. > > > > The second "clock-names" constraint is supposed to be applied to the > > DW XPCS synthesized with the MCI/APB3 CSRs IO interface. The device in > > that case should be defined in the DT source file as a normal memory > > mapped device. > > > > > > > > Otherwise this binding looks fine to me. > > > > Shall I add a note to the clock description that the "clk_csr_i" > > signal is named as "pclk"? I'll need to resubmit the series anyway. > > Better yet, could you mention MDIO? It wasn't clear to me (but I'm just > reviewing bindings not a dwmac-ist) that MCI and APB3 were only two of > the options and that the first clock-names was for MDIO. Maybe something > like: > > clock-names: > oneOf: > - minItems: 1 > items: # MDIO > - enum: [core, pad] > - const: pad > - minItems: 1 > items: # MCI or APB > - const: pclk > - enum: [core, pad] > - const: pad Agreed. I'll add the comments to the oneOf-subschemas as you suggested. Thanks, -Serge(y)
On Thu, Jun 27, 2024 at 08:10:48PM +0300, Serge Semin wrote: > On Thu, Jun 27, 2024 at 04:51:22PM +0100, Conor Dooley wrote: > > On Thu, Jun 27, 2024 at 03:41:26AM +0300, Serge Semin wrote: > > > + clocks: > > > + description: > > > + Both MCI and APB3 interfaces are supposed to be equipped with a clock > > > + source connected via the clk_csr_i line. > > > + > > > + PCS/PMA layer can be clocked by an internal reference clock source > > > + (phyN_core_refclk) or by an externally connected (phyN_pad_refclk) clock > > > + generator. Both clocks can be supplied at a time. > > > + minItems: 1 > > > + maxItems: 3 > > > + > > > + clock-names: > > > + oneOf: > > > + - minItems: 1 > > > + items: > > > + - enum: [core, pad] > > > + - const: pad > > > + - minItems: 1 > > > + items: > > > + - const: pclk > > > + - enum: [core, pad] > > > + - const: pad > > > > > While reading this, I'm kinda struggling to map "clk_csr_i" to a clock > > name. Is that pclk? And why pclk if it is connected to "clk_csr_i"? > > Right. It's "pclk". The reason of using the "pclk" name is that it has > turned to be a de-facto standard name in the DT-bindings for the > peripheral bus clock sources utilized for the CSR-space IO buses. > Moreover the STMMAC driver responsible for the parental DW *MAC > devices handling also has the "pclk" name utilized for the clk_csr_i > signal. So using the "pclk" name in the tightly coupled devices (MAC > and PCS) for the same signal seemed a good idea. It is? That's really just the name of the bus clock for APB (Arm Peripheral Bus). If there's a name that matches the docs, use that. Though I'd drop 'clk_' part. Rob
diff --git a/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml b/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml new file mode 100644 index 000000000000..d0f2c2bfbf35 --- /dev/null +++ b/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml @@ -0,0 +1,136 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/pcs/snps,dw-xpcs.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Synopsys DesignWare Ethernet PCS + +maintainers: + - Serge Semin <fancer.lancer@gmail.com> + +description: + Synopsys DesignWare Ethernet Physical Coding Sublayer provides an interface + between Media Access Control and Physical Medium Attachment Sublayer through + the Media Independent Interface (XGMII, USXGMII, XLGMII, GMII, etc) + controlled by means of the IEEE std. Clause 45 registers set. The PCS can be + optionally synthesized with a vendor-specific interface connected to + Synopsys PMA (also called DesignWare Consumer/Enterprise PHY) although in + general it can be used to communicate with any compatible PHY. + + The PCS CSRs can be accessible either over the Ethernet MDIO bus or directly + by means of the APB3/MCI interfaces. In the later case the XPCS can be mapped + right to the system IO memory space. + +properties: + compatible: + oneOf: + - description: Synopsys DesignWare XPCS with none or unknown PMA + const: snps,dw-xpcs + - description: Synopsys DesignWare XPCS with Consumer Gen1 3G PMA + const: snps,dw-xpcs-gen1-3g + - description: Synopsys DesignWare XPCS with Consumer Gen2 3G PMA + const: snps,dw-xpcs-gen2-3g + - description: Synopsys DesignWare XPCS with Consumer Gen2 6G PMA + const: snps,dw-xpcs-gen2-6g + - description: Synopsys DesignWare XPCS with Consumer Gen4 3G PMA + const: snps,dw-xpcs-gen4-3g + - description: Synopsys DesignWare XPCS with Consumer Gen4 6G PMA + const: snps,dw-xpcs-gen4-6g + - description: Synopsys DesignWare XPCS with Consumer Gen5 10G PMA + const: snps,dw-xpcs-gen5-10g + - description: Synopsys DesignWare XPCS with Consumer Gen5 12G PMA + const: snps,dw-xpcs-gen5-12g + + reg: + items: + - description: + In case of the MDIO management interface this just a 5-bits ID + of the MDIO bus device. If DW XPCS CSRs space is accessed over the + MCI or APB3 management interfaces, then the space mapping can be + either 'direct' or 'indirect'. In the former case all Clause 45 + registers are contiguously mapped within the address space + MMD '[20:16]', Reg '[15:0]'. In the later case the space is divided + to the multiple 256 register sets. There is a special viewport CSR + which is responsible for the set selection. The upper part of + the CSR address MMD+REG[20:8] is supposed to be written in there + so the corresponding subset would be mapped to the lowest 255 CSRs. + + reg-names: + items: + - enum: [ direct, indirect ] + + reg-io-width: + description: + The way the CSRs are mapped to the memory is platform depended. Since + each Clause 45 CSR is of 16-bits wide the access instructions must be + two bytes aligned at least. + default: 2 + enum: [ 2, 4 ] + + interrupts: + description: + System interface interrupt output (sbd_intr_o) indicating Clause 73/37 + auto-negotiation events':' Page received, AN is completed or incompatible + link partner. + maxItems: 1 + + clocks: + description: + Both MCI and APB3 interfaces are supposed to be equipped with a clock + source connected via the clk_csr_i line. + + PCS/PMA layer can be clocked by an internal reference clock source + (phyN_core_refclk) or by an externally connected (phyN_pad_refclk) clock + generator. Both clocks can be supplied at a time. + minItems: 1 + maxItems: 3 + + clock-names: + oneOf: + - minItems: 1 + items: + - enum: [core, pad] + - const: pad + - minItems: 1 + items: + - const: pclk + - enum: [core, pad] + - const: pad + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + + ethernet-pcs@1f05d000 { + compatible = "snps,dw-xpcs"; + reg = <0x1f05d000 0x1000>; + reg-names = "indirect"; + + reg-io-width = <4>; + + interrupts = <79 IRQ_TYPE_LEVEL_HIGH>; + + clocks = <&ccu_pclk>, <&ccu_core>, <&ccu_pad>; + clock-names = "pclk", "core", "pad"; + }; + - | + mdio-bus { + #address-cells = <1>; + #size-cells = <0>; + + ethernet-pcs@0 { + compatible = "snps,dw-xpcs"; + reg = <0>; + + clocks = <&ccu_core>, <&ccu_pad>; + clock-names = "core", "pad"; + }; + }; +...
Synopsys DesignWare XPCS IP-core is a Physical Coding Sublayer (PCS) layer providing an interface between the Media Access Control (MAC) and Physical Medium Attachment Sublayer (PMA) through a Media independent interface. From software point of view it exposes IEEE std. Clause 45 CSR space and can be accessible either by MDIO or MCI/APB3 bus interfaces. In the former case the PCS device is supposed to be defined under the respective MDIO bus DT-node. In the later case the DW xPCS will be just a normal IO memory-mapped device. Besides of that DW XPCS DT-nodes can have an interrupt signal and clock source properties specified. The former one indicates the Clause 73/37 auto-negotiation events like: negotiation page received, AN is completed or incompatible link partner. The clock DT-properties can describe up to three clock sources: peripheral bus clock source, internal reference clock and the externally connected reference clock. Finally the DW XPCS IP-core can be optionally synthesized with a vendor-specific interface connected to the Synopsys PMA (also called DesignWare Consumer/Enterprise PHY). Alas that isn't auto-detectable in a portable way. So if the DW XPCS device has the respective PMA attached then it should be reflected in the DT-node compatible string so the driver would be aware of the PMA-specific device capabilities (mainly connected with CSRs available for the fine-tunings). Signed-off-by: Serge Semin <fancer.lancer@gmail.com> --- Changelog v2: - Drop the Management Interface DT-node bindings. DW xPCS with MCI/APB3 interface is just a normal memory-mapped device. Changelog v3: - Implement the ordered clocks constraint. (@Rob) --- .../bindings/net/pcs/snps,dw-xpcs.yaml | 136 ++++++++++++++++++ 1 file changed, 136 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml