| Message ID | 20230206060708.3574472-1-danishanwar@ti.com |
|---|---|
| Headers | show |
| Series | Introduce ICSSG based ethernet Driver | expand |
Hi Krzysztof, Thanks for the comments. On 06/02/23 13:20, Krzysztof Kozlowski wrote: > On 06/02/2023 07:07, MD Danish Anwar wrote: >> From: Puranjay Mohan <p-mohan@ti.com> >> >> Add a YAML binding document for the ICSSG Programmable real time unit >> based Ethernet driver. This driver uses the PRU and PRUSS consumer APIs > > You add a binding for the hardware, not for driver. > >> to interface the PRUs and load/run the firmware for supporting ethernet >> functionality. > > Subject: drop second/last, redundant "driver bindings". The > "dt-bindings" prefix is already stating that these are bindings. > Sure I will modify the subject and commit description. >> >> Signed-off-by: Puranjay Mohan <p-mohan@ti.com> >> Signed-off-by: Md Danish Anwar <danishanwar@ti.com> >> --- >> .../bindings/net/ti,icssg-prueth.yaml | 179 ++++++++++++++++++ >> 1 file changed, 179 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml >> >> diff --git a/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml >> new file mode 100644 >> index 000000000000..e4dee01a272a >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml >> @@ -0,0 +1,179 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/net/ti,icssg-prueth.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Texas Instruments ICSSG PRUSS Ethernet >> + >> +maintainers: >> + - Md Danish Anwar <danishanwar@ti.com> >> + >> +description: >> + Ethernet based on the Programmable Real-Time >> + Unit and Industrial Communication Subsystem. >> + >> +allOf: >> + - $ref: /schemas/remoteproc/ti,pru-consumer.yaml# >> + >> +properties: >> + compatible: >> + enum: >> + - ti,am654-icssg-prueth # for AM65x SoC family >> + >> + ti,sram: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: >> + phandle to MSMC SRAM node >> + >> + dmas: >> + maxItems: 10 >> + >> + dma-names: >> + items: >> + - const: tx0-0 >> + - const: tx0-1 >> + - const: tx0-2 >> + - const: tx0-3 >> + - const: tx1-0 >> + - const: tx1-1 >> + - const: tx1-2 >> + - const: tx1-3 >> + - const: rx0 >> + - const: rx1 >> + >> + ethernet-ports: > > Bring some order or logic in the order of the properties. Keep the > ethernet-ports as last property. > Sure, I will re-arrange them. >> + type: object >> + additionalProperties: false > > Blank line > >> + properties: >> + '#address-cells': >> + const: 1 >> + '#size-cells': >> + const: 0 >> + >> + patternProperties: >> + ^port@[0-1]$: >> + type: object >> + description: ICSSG PRUETH external ports >> + At least one ethernet port is required. Should I add the below line here for this? minItems: 1 >> Drop blank line > >> + $ref: ethernet-controller.yaml# >> + > > Drop blank line > >> + unevaluatedProperties: false >> + >> + properties: >> + reg: >> + items: >> + - enum: [0, 1] >> + description: ICSSG PRUETH port number >> + >> + interrupts-extended: > > Just "interrupts" I will drop / add blank lines and change this to just "interrupts". >> + maxItems: 1 >> + >> + ti,syscon-rgmii-delay: >> + items: >> + - items: >> + - description: phandle to system controller node >> + - description: The offset to ICSSG control register >> + $ref: /schemas/types.yaml#/definitions/phandle-array >> + description: >> + phandle to system controller node and register offset >> + to ICSSG control register for RGMII transmit delay >> + >> + required: >> + - reg > > required for ethernet-ports - at least one port is required, isn't it? Yes, at least one ethernet port is required. Should I add "minItems: 1" in patternProperties section or should I add a new required section in patternProperties and mention something like port@0 as required? >> + >> + ti,mii-g-rt: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: | >> + phandle to MII_G_RT module's syscon regmap. >> + >> + ti,mii-rt: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: | >> + phandle to MII_RT module's syscon regmap >> + >> + interrupts: >> + maxItems: 2 >> + description: | >> + Interrupt specifiers to TX timestamp IRQ. >> + >> + interrupt-names: >> + items: >> + - const: tx_ts0 >> + - const: tx_ts1 >> + >> +required: >> + - compatible >> + - ti,sram >> + - dmas >> + - dma-names >> + - ethernet-ports >> + - ti,mii-g-rt >> + - interrupts >> + - interrupt-names >> + >> +unevaluatedProperties: false >> + >> +examples: >> + - | >> + /* Example k3-am654 base board SR2.0, dual-emac */ >> + pruss2_eth: ethernet { >> + compatible = "ti,am654-icssg-prueth"; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&icssg2_rgmii_pins_default>; >> + ti,sram = <&msmc_ram>; >> + >> + ti,prus = <&pru2_0>, <&rtu2_0>, <&tx_pru2_0>, >> + <&pru2_1>, <&rtu2_1>, <&tx_pru2_1>; >> + firmware-name = "ti-pruss/am65x-pru0-prueth-fw.elf", >> + "ti-pruss/am65x-rtu0-prueth-fw.elf", >> + "ti-pruss/am65x-txpru0-prueth-fw.elf", >> + "ti-pruss/am65x-pru1-prueth-fw.elf", >> + "ti-pruss/am65x-rtu1-prueth-fw.elf", >> + "ti-pruss/am65x-txpru1-prueth-fw.elf"; >> + ti,pruss-gp-mux-sel = <2>, /* MII mode */ >> + <2>, >> + <2>, >> + <2>, /* MII mode */ >> + <2>, >> + <2>; >> + dmas = <&main_udmap 0xc300>, /* egress slice 0 */ >> + <&main_udmap 0xc301>, /* egress slice 0 */ >> + <&main_udmap 0xc302>, /* egress slice 0 */ >> + <&main_udmap 0xc303>, /* egress slice 0 */ >> + <&main_udmap 0xc304>, /* egress slice 1 */ >> + <&main_udmap 0xc305>, /* egress slice 1 */ >> + <&main_udmap 0xc306>, /* egress slice 1 */ >> + <&main_udmap 0xc307>, /* egress slice 1 */ >> + <&main_udmap 0x4300>, /* ingress slice 0 */ >> + <&main_udmap 0x4301>; /* ingress slice 1 */ >> + dma-names = "tx0-0", "tx0-1", "tx0-2", "tx0-3", >> + "tx1-0", "tx1-1", "tx1-2", "tx1-3", >> + "rx0", "rx1"; >> + ti,mii-g-rt = <&icssg2_mii_g_rt>; >> + interrupts = <24 0 2>, <25 1 3>; > > Aren't you open-coding some IRQ flags? > These values are not open coded here. These values are the expected values for interrupt node. Here, Cell 1 -> PRU System event number Cell 2 -> PRU channel Cell 3 -> PRU host_event (target) This is documented in detail in Documentation/devicetree/bindings/interrupt-controller/ti,pruss-intc.yaml I also missed mentioning "interrupt-parent" in example section. I will add interrupt-parent in next revision. interrupt-parent = <&icssg2_intc>; >> + interrupt-names = "tx_ts0", "tx_ts1"; >> + ethernet-ports { > > > Best regards, > Krzysztof >
On Mon, 06 Feb 2023 11:37:07 +0530, MD Danish Anwar wrote: > From: Puranjay Mohan <p-mohan@ti.com> > > Add a YAML binding document for the ICSSG Programmable real time unit > based Ethernet driver. This driver uses the PRU and PRUSS consumer APIs > to interface the PRUs and load/run the firmware for supporting ethernet > functionality. > > Signed-off-by: Puranjay Mohan <p-mohan@ti.com> > Signed-off-by: Md Danish Anwar <danishanwar@ti.com> > --- > .../bindings/net/ti,icssg-prueth.yaml | 179 ++++++++++++++++++ > 1 file changed, 179 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/ti,icssg-prueth.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: ./Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/remoteproc/ti,pru-consumer.yaml /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ti,icssg-prueth.example.dtb: ethernet: False schema does not allow {'compatible': ['ti,am654-icssg-prueth'], 'pinctrl-names': ['default'], 'pinctrl-0': [[4294967295]], 'ti,sram': [[4294967295]], 'ti,prus': [[4294967295, 4294967295, 4294967295, 4294967295, 4294967295, 4294967295]], 'firmware-name': ['ti-pruss/am65x-pru0-prueth-fw.elf', 'ti-pruss/am65x-rtu0-prueth-fw.elf', 'ti-pruss/am65x-txpru0-prueth-fw.elf', 'ti-pruss/am65x-pru1-prueth-fw.elf', 'ti-pruss/am65x-rtu1-prueth-fw.elf', 'ti-pruss/am65x-txpru1-prueth-fw.elf'], 'ti,pruss-gp-mux-sel': [[2, 2, 2, 2, 2, 2]], 'dmas': [[4294967295, 49920], [4294967295, 49921], [4294967295, 49922], [4294967295, 49923], [4294967295, 49924], [4294967295, 49925], [4294967295, 49926], [4294967295, 49927], [4294967295, 17152], [4294967295, 17153]], 'dma-names': ['tx0-0', 'tx0-1', 'tx0-2', 'tx0-3', 'tx1-0', 'tx1-1', 'tx1-2', 'tx1-3', 'rx0', 'rx1'], 'ti,mii-g-rt': [[4294967295]] , 'interrupts': [[24, 0, 2], [25, 1, 3]], 'interrupt-names': ['tx_ts0', 'tx_ts1'], 'ethernet-ports': {'#address-cells': [[1]], '#size-cells': [[0]], 'port@0': {'reg': [[0]], 'phy-handle': [[4294967295]], 'phy-mode': ['rgmii-id'], 'interrupts-extended': [[4294967295, 24]], 'ti,syscon-rgmii-delay': [[4294967295, 16672]], 'local-mac-address': [[0, 0, 0, 0, 0, 0]]}, 'port@1': {'reg': [[1]], 'phy-handle': [[4294967295]], 'phy-mode': ['rgmii-id'], 'interrupts-extended': [[4294967295, 25]], 'ti,syscon-rgmii-delay': [[4294967295, 16676]], 'local-mac-address': [[0, 0, 0, 0, 0, 0]]}}, '$nodename': ['ethernet']} From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ti,icssg-prueth.example.dtb: ethernet: Unevaluated properties are not allowed ('firmware-name', 'ti,prus', 'ti,pruss-gp-mux-sel' were unexpected) From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230206060708.3574472-2-danishanwar@ti.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. 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 after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
Hi Rob, On 06/02/23 19:16, Rob Herring wrote: > > On Mon, 06 Feb 2023 11:37:07 +0530, MD Danish Anwar wrote: >> From: Puranjay Mohan <p-mohan@ti.com> >> >> Add a YAML binding document for the ICSSG Programmable real time unit >> based Ethernet driver. This driver uses the PRU and PRUSS consumer APIs >> to interface the PRUs and load/run the firmware for supporting ethernet >> functionality. >> >> Signed-off-by: Puranjay Mohan <p-mohan@ti.com> >> Signed-off-by: Md Danish Anwar <danishanwar@ti.com> >> --- >> .../bindings/net/ti,icssg-prueth.yaml | 179 ++++++++++++++++++ >> 1 file changed, 179 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/net/ti,icssg-prueth.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: > ./Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/remoteproc/ti,pru-consumer.yaml This error is coming because this series depends on series [1]. The ti,pru-consumer.yaml is upstreamed through series [1]. The series is approved and will hopefully be merged soon. [1] https://lore.kernel.org/all/20230106121046.886863-1-danishanwar@ti.com/ > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ti,icssg-prueth.example.dtb: ethernet: False schema does not allow {'compatible': ['ti,am654-icssg-prueth'], 'pinctrl-names': ['default'], 'pinctrl-0': [[4294967295]], 'ti,sram': [[4294967295]], 'ti,prus': [[4294967295, 4294967295, 4294967295, 4294967295, 4294967295, 4294967295]], 'firmware-name': ['ti-pruss/am65x-pru0-prueth-fw.elf', 'ti-pruss/am65x-rtu0-prueth-fw.elf', 'ti-pruss/am65x-txpru0-prueth-fw.elf', 'ti-pruss/am65x-pru1-prueth-fw.elf', 'ti-pruss/am65x-rtu1-prueth-fw.elf', 'ti-pruss/am65x-txpru1-prueth-fw.elf'], 'ti,pruss-gp-mux-sel': [[2, 2, 2, 2, 2, 2]], 'dmas': [[4294967295, 49920], [4294967295, 49921], [4294967295, 49922], [4294967295, 49923], [4294967295, 49924], [4294967295, 49925], [4294967295, 49926], [4294967295, 49927], [4294967295, 17152], [4294967295, 17153]], 'dma-names': ['tx0-0', 'tx0-1', 'tx0-2', 'tx0-3', 'tx1-0', 'tx1-1', 'tx1-2', 'tx1-3', 'rx0', 'rx1'], 'ti,mii-g-rt': [[429! > 4967295]], 'interrupts': [[24, 0, 2], [25, 1, 3]], 'interrupt-names': ['tx_ts0', 'tx_ts1'], 'ethernet-ports': {'#address-cells': [[1]], '#size-cells': [[0]], 'port@0': {'reg': [[0]], 'phy-handle': [[4294967295]], 'phy-mode': ['rgmii-id'], 'interrupts-extended': [[4294967295, 24]], 'ti,syscon-rgmii-delay': [[4294967295, 16672]], 'local-mac-address': [[0, 0, 0, 0, 0, 0]]}, 'port@1': {'reg': [[1]], 'phy-handle': [[4294967295]], 'phy-mode': ['rgmii-id'], 'interrupts-extended': [[4294967295, 25]], 'ti,syscon-rgmii-delay': [[4294967295, 16676]], 'local-mac-address': [[0, 0, 0, 0, 0, 0]]}}, '$nodename': ['ethernet']} > From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ti,icssg-prueth.example.dtb: ethernet: Unevaluated properties are not allowed ('firmware-name', 'ti,prus', 'ti,pruss-gp-mux-sel' were unexpected) > From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml > > doc reference errors (make refcheckdocs): > > See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230206060708.3574472-2-danishanwar@ti.com > > The base for the series is generally the latest rc1. A different dependency > should be noted in *this* patch. > > 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 after running the above command yourself. Note > that DT_SCHEMA_FILES can be set to your schema file to speed up checking > your schema. However, it must be unset to test all examples with your schema. >
Hi Andrew, On 06/02/23 19:45, Andrew Lunn wrote: >> +enum mii_mode { >> + MII_MODE_MII = 0, >> + MII_MODE_RGMII, >> + MII_MODE_SGMII > > There is no mention of SGMII anywhere else. And in a couple of places, > the code makes the assumption that if it is not RGMII it is MII. > > Does the hardware really support SGMII? > As far as I know, the hardware does support SGMII but it's not yet supported by the driver. I will drop the SGMII because it's not needed as of now. If in future support for SGMII is there, I'll add it. >> +static int prueth_config_rgmiidelay(struct prueth *prueth, >> + struct device_node *eth_np, >> + phy_interface_t phy_if) >> +{ > > ... > >> + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || >> + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) >> + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; >> + >> + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); > > Here you are adding the TX delay if the phy-mode indicates it should > be added. > >> +static int prueth_netdev_init(struct prueth *prueth, >> + struct device_node *eth_node) >> +{ > >> + ret = of_get_phy_mode(eth_node, &emac->phy_if); >> + if (ret) { >> + dev_err(prueth->dev, "could not get phy-mode property\n"); >> + goto free; >> + } > >> + ret = prueth_config_rgmiidelay(prueth, eth_node, emac->phy_if); >> + if (ret) >> + goto free; >> + > > Reading it from DT and calling the delay function. > >> +static int prueth_probe(struct platform_device *pdev) >> +{ > > >> + /* register the network devices */ >> + if (eth0_node) { >> + ret = register_netdev(prueth->emac[PRUETH_MAC0]->ndev); >> + if (ret) { >> + dev_err(dev, "can't register netdev for port MII0"); >> + goto netdev_exit; >> + } >> + >> + prueth->registered_netdevs[PRUETH_MAC0] = prueth->emac[PRUETH_MAC0]->ndev; >> + >> + emac_phy_connect(prueth->emac[PRUETH_MAC0]); > > And this is connecting the MAC and the PHY, where emac_phy_connect() > passes emac->phy_if to phylib. > > What i don't see anywhere is you changing emac->phy_if to indicate the > MAC has inserted the TX delay, and so the PHY should not. > Yes, there is no indication whether MAC has enabled TX delay or not. I have changed the phy-mode in DT from "rgmii-rxid" to "rgmii-id" as per your suggestion in previous revision. I will keep Tx Internal delay as it is(getting configured in MAC) and inside emac_phy_connect() API, while calling of_phy_connect() instead of passing emac->phy_if (which is rgmii-id as per DT), I will pass PHY_INTERFACE_MODE_RGMII_RXID. This will make sure that phy only enables Rx delay and keep the existing approach of keepping Tx delay in MAC. Currently, in emac_phy_connect() API, /* connect PHY */ ndev->phydev = of_phy_connect(emac->ndev, emac->phy_node, &emac_adjust_link, 0, emac->phy_if); I will change it to, /* connect PHY */ ndev->phydev = of_phy_connect(emac->ndev, emac->phy_node, &emac_adjust_link, 0, PHY_INTERFACE_MODE_RGMII_RXID); Let me know if this looks OK. > Andrew >
Danish, On 07/02/2023 17:29, Md Danish Anwar wrote: > Hi Andrew, > > On 06/02/23 19:45, Andrew Lunn wrote: >>> +enum mii_mode { >>> + MII_MODE_MII = 0, >>> + MII_MODE_RGMII, >>> + MII_MODE_SGMII >> >> There is no mention of SGMII anywhere else. And in a couple of places, >> the code makes the assumption that if it is not RGMII it is MII. >> >> Does the hardware really support SGMII? >> > > As far as I know, the hardware does support SGMII but it's not yet supported by > the driver. I will drop the SGMII because it's not needed as of now. If in > future support for SGMII is there, I'll add it. > >>> +static int prueth_config_rgmiidelay(struct prueth *prueth, >>> + struct device_node *eth_np, >>> + phy_interface_t phy_if) >>> +{ >> >> ... >> >>> + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || >>> + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) >>> + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; >>> + >>> + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); This is only applicable to some devices so you need to restrict this only to those devices. And only when you enable MAC TX delay you need to change emac->phy_if to PHY_INTERFACE_MODE_RGMII_RXID if it was PHY_INTERFACE_MODE_RGMII_ID. >> >> Here you are adding the TX delay if the phy-mode indicates it should >> be added. >> >>> +static int prueth_netdev_init(struct prueth *prueth, >>> + struct device_node *eth_node) >>> +{ >> >>> + ret = of_get_phy_mode(eth_node, &emac->phy_if); >>> + if (ret) { >>> + dev_err(prueth->dev, "could not get phy-mode property\n"); >>> + goto free; >>> + } >> >>> + ret = prueth_config_rgmiidelay(prueth, eth_node, emac->phy_if); >>> + if (ret) >>> + goto free; >>> + >> >> Reading it from DT and calling the delay function. >> >>> +static int prueth_probe(struct platform_device *pdev) >>> +{ >> >> >>> + /* register the network devices */ >>> + if (eth0_node) { >>> + ret = register_netdev(prueth->emac[PRUETH_MAC0]->ndev); >>> + if (ret) { >>> + dev_err(dev, "can't register netdev for port MII0"); >>> + goto netdev_exit; >>> + } >>> + >>> + prueth->registered_netdevs[PRUETH_MAC0] = prueth->emac[PRUETH_MAC0]->ndev; >>> + >>> + emac_phy_connect(prueth->emac[PRUETH_MAC0]); >> >> And this is connecting the MAC and the PHY, where emac_phy_connect() >> passes emac->phy_if to phylib. >> >> What i don't see anywhere is you changing emac->phy_if to indicate the >> MAC has inserted the TX delay, and so the PHY should not. >> > > Yes, there is no indication whether MAC has enabled TX delay or not. I have > changed the phy-mode in DT from "rgmii-rxid" to "rgmii-id" as per your > suggestion in previous revision. I will keep Tx Internal delay as it is(getting > configured in MAC) and inside emac_phy_connect() API, while calling > of_phy_connect() instead of passing emac->phy_if (which is rgmii-id as per DT), > I will pass PHY_INTERFACE_MODE_RGMII_RXID. This will make sure that phy only > enables Rx delay and keep the existing approach of keepping Tx delay in MAC. > > Currently, in emac_phy_connect() API, > > /* connect PHY */ > ndev->phydev = of_phy_connect(emac->ndev, emac->phy_node, > &emac_adjust_link, 0, > emac->phy_if); > I will change it to, > > /* connect PHY */ > ndev->phydev = of_phy_connect(emac->ndev, emac->phy_node, > &emac_adjust_link, 0, > PHY_INTERFACE_MODE_RGMII_RXID); > > Let me know if this looks OK. No, this is not OK. Please keep this as emac->phy_if. In prueth_config_rgmiidelay(), you can change emac->phy_if to PHY_INTERFACE_MODE_RGMII_RXID only if it was RGMII mode *and* MAC TX delay was enabled. cheers, -roger
Hi Roger and Andrew, On 08/02/23 01:26, Roger Quadros wrote: > Danish, > > On 07/02/2023 17:29, Md Danish Anwar wrote: >> Hi Andrew, >> >> On 06/02/23 19:45, Andrew Lunn wrote: >>>> +enum mii_mode { >>>> + MII_MODE_MII = 0, >>>> + MII_MODE_RGMII, >>>> + MII_MODE_SGMII >>> >>> There is no mention of SGMII anywhere else. And in a couple of places, >>> the code makes the assumption that if it is not RGMII it is MII. >>> >>> Does the hardware really support SGMII? >>> >> >> As far as I know, the hardware does support SGMII but it's not yet supported by >> the driver. I will drop the SGMII because it's not needed as of now. If in >> future support for SGMII is there, I'll add it. >> >>>> +static int prueth_config_rgmiidelay(struct prueth *prueth, >>>> + struct device_node *eth_np, >>>> + phy_interface_t phy_if) >>>> +{ >>> >>> ... >>> >>>> + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || >>>> + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) >>>> + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; >>>> + >>>> + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); > > This is only applicable to some devices so you need to restrict this only > to those devices. > Currently ICSSG driver is getting upstreamed for AM65 SR2.0 device, so I don't think there is any need for any device related restriction. Once support for other devices are enabled for upstream, we can modify this accordingly. > And only when you enable MAC TX delay you need to change emac->phy_if to PHY_INTERFACE_MODE_RGMII_RXID > if it was PHY_INTERFACE_MODE_RGMII_ID. > >>> >>> Here you are adding the TX delay if the phy-mode indicates it should >>> be added. >>> >>>> +static int prueth_netdev_init(struct prueth *prueth, >>>> + struct device_node *eth_node) >>>> +{ >>> >>>> + ret = of_get_phy_mode(eth_node, &emac->phy_if); >>>> + if (ret) { >>>> + dev_err(prueth->dev, "could not get phy-mode property\n"); >>>> + goto free; >>>> + } >>> >>>> + ret = prueth_config_rgmiidelay(prueth, eth_node, emac->phy_if); >>>> + if (ret) >>>> + goto free; >>>> + >>> >>> Reading it from DT and calling the delay function. >>> >>>> +static int prueth_probe(struct platform_device *pdev) >>>> +{ >>> >>> >>>> + /* register the network devices */ >>>> + if (eth0_node) { >>>> + ret = register_netdev(prueth->emac[PRUETH_MAC0]->ndev); >>>> + if (ret) { >>>> + dev_err(dev, "can't register netdev for port MII0"); >>>> + goto netdev_exit; >>>> + } >>>> + >>>> + prueth->registered_netdevs[PRUETH_MAC0] = prueth->emac[PRUETH_MAC0]->ndev; >>>> + >>>> + emac_phy_connect(prueth->emac[PRUETH_MAC0]); >>> >>> And this is connecting the MAC and the PHY, where emac_phy_connect() >>> passes emac->phy_if to phylib. >>> >>> What i don't see anywhere is you changing emac->phy_if to indicate the >>> MAC has inserted the TX delay, and so the PHY should not. >>> >> >> Yes, there is no indication whether MAC has enabled TX delay or not. I have >> changed the phy-mode in DT from "rgmii-rxid" to "rgmii-id" as per your >> suggestion in previous revision. I will keep Tx Internal delay as it is(getting >> configured in MAC) and inside emac_phy_connect() API, while calling >> of_phy_connect() instead of passing emac->phy_if (which is rgmii-id as per DT), >> I will pass PHY_INTERFACE_MODE_RGMII_RXID. This will make sure that phy only >> enables Rx delay and keep the existing approach of keepping Tx delay in MAC. >> >> Currently, in emac_phy_connect() API, >> >> /* connect PHY */ >> ndev->phydev = of_phy_connect(emac->ndev, emac->phy_node, >> &emac_adjust_link, 0, >> emac->phy_if); >> I will change it to, >> >> /* connect PHY */ >> ndev->phydev = of_phy_connect(emac->ndev, emac->phy_node, >> &emac_adjust_link, 0, >> PHY_INTERFACE_MODE_RGMII_RXID); >> >> Let me know if this looks OK. > > No, this is not OK. > > Please keep this as emac->phy_if. > > In prueth_config_rgmiidelay(), you can change emac->phy_if to > PHY_INTERFACE_MODE_RGMII_RXID only if it was RGMII mode > *and* MAC TX delay was enabled. > I checked the latest Technical Reference Manual [1] (Section 5.1.3.4.49, Table 5-624) for AM65 Silicon Revision 2.0. Below is the description in Table 5-624 BIT : 24 Field : RGMII0_ID_MODE Type : R/W Reset : 0h Description : Controls the PRU_ICSSG0 RGMII0 port internal transmit delay 0h - Internal transmit delay is enabled 1h - Reserved The TX internal delay is always enabled and couldn't be disabled as 1h is reserved. So hardware support for disabling TX internal delay is not there. As, TX internal delay is always there, there is no need to enable it in MAC or PHY. So no need of API prueth_config_rgmiidelay(). My approach to handle delay would be as below. *) Keep phy-mode = "rgmii-id" in DT as asked by Andrew. *) Let TX internal delay enabled in Hardware. *) Let PHY configure RX internal delay. *) Remove prueth_config_rgmiidelay() API is there is no use of this. TX Internal delay is always enabled. *) Instead of calling prueth_config_rgmiidelay() API in prueth_netdev_init() API, add below if condition. if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID) emac->phy_if == PHY_INTERFACE_MODE_RGMII_RXID This will check if phy-mode is "rgmii-id", then change the phy-mode to "rgmii-rxid". The same emac->phy_if is passed to emac_phy_connect which passes emac->phy_if to phylib. So that PHY will only enable RX internal delay and TX internal delay is always enabled by default(in Hardware) As of now ICSSG driver is getting upstreamed with support for only AM65 SR2.0, In future when ICSSG driver starts supporting other devices, we can modify this condition to something like below if(device_is_AM65 && emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID) emac->phy_if == PHY_INTERFACE_MODE_RGMII_RXID So that, for other devices, phy-mode should remain as it is, as other devices don't have hardware restriction. Please let me know if this is the right approach. [1] https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf?ts=1675841023830&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FAM6548#%5B%7B%22num%22%3A1706%2C%22gen%22%3A0%7D%2C%7B%22name%22%3A%22XYZ%22%7D%2C0%2C717.9%2C0%5D > cheers, > -roger
Hi Andrew, On 08/02/23 18:26, Andrew Lunn wrote: >>>>>> +static int prueth_config_rgmiidelay(struct prueth *prueth, >>>>>> + struct device_node *eth_np, >>>>>> + phy_interface_t phy_if) >>>>>> +{ >>>>> >>>>> ... >>>>> >>>>>> + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || >>>>>> + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) >>>>>> + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; >>>>>> + >>>>>> + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); >>> >>> This is only applicable to some devices so you need to restrict this only >>> to those devices. >>> >> >> Currently ICSSG driver is getting upstreamed for AM65 SR2.0 device, so I don't >> think there is any need for any device related restriction. Once support for >> other devices are enabled for upstream, we can modify this accordingly. > > The problem is, this is a board property, not a SoC property. What if > somebody designs a board with extra long clock lines in order to add > the delay? > >> I checked the latest Technical Reference Manual [1] (Section 5.1.3.4.49, Table >> 5-624) for AM65 Silicon Revision 2.0. >> >> Below is the description in Table 5-624 >> >> BIT : 24 >> Field : RGMII0_ID_MODE >> Type : R/W >> Reset : 0h >> Description : Controls the PRU_ICSSG0 RGMII0 port internal transmit delay >> 0h - Internal transmit delay is enabled >> 1h - Reserved >> >> The TX internal delay is always enabled and couldn't be disabled as 1h is >> reserved. So hardware support for disabling TX internal delay is not there. > > So if somebody passes a phy-mode which requires it disabled, you need > to return -EINVAL, to indicate the hardware cannot actually do it. > Sure, I'll do that. In the list of all phy modes described in [1], I can only see phy-mode "rgmii-txid", for which we can return -EINVAL. Is there any other phy-mode that requires enabling/disabling TX internal delays? Please let me know if any other phy-mode also needs this. I will add check for that as well. >> As, TX internal delay is always there, there is no need to enable it in MAC or >> PHY. So no need of API prueth_config_rgmiidelay(). >> >> My approach to handle delay would be as below. >> >> *) Keep phy-mode = "rgmii-id" in DT as asked by Andrew. > > As i said this depends on the board, not the SoC. In theory, you could > design a board with an extra long RX clock line, and then use phy-mode > rgmii-txid, meaning the MAC/PHY combination needs to add the TX delay. > Yes I understand that board can have any phy-mode in it's DTS. We need to be able to handle all different phy modes. >> *) Let TX internal delay enabled in Hardware. >> *) Let PHY configure RX internal delay. >> *) Remove prueth_config_rgmiidelay() API is there is no use of this. TX >> Internal delay is always enabled. >> *) Instead of calling prueth_config_rgmiidelay() API in prueth_netdev_init() >> API, add below if condition. >> >> if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID) >> emac->phy_if == PHY_INTERFACE_MODE_RGMII_RXID > > You should handle all cases where a TX delay is requested, not just > ID. > So there could be four different RGMII phy modes as described in [1]. Below is the handling mechanism for different phy modes. 1) # RGMII with internal RX and TX delays provided by the PHY, # the MAC should not add the RX or TX delays in this case - rgmii-id For phy-mode="rgmii-id", phy needs to add both TX and RX internal delays. But in our SoC TX internal delay is always enabled. So to handle this, we'll change the phy-mode in driver to "rgmii-rxid" and then pass it ti PHY, so that PHY will enable RX internal delay only. 2) # RGMII with internal RX delay provided by the PHY, the MAC # should not add an RX delay in this case - rgmii-rxid For phy-mode="rgmii-rxid", phy needs to add only RX internal delay. We will do nothing in the driver and just pass the same mode to phy, so that PHY will enable RX internal delay only. 3) # RGMII with internal TX delay provided by the PHY, the MAC # should not add an TX delay in this case - rgmii-txid For phy-mode="rgmii-txid", phy needs to add only TX internal delay,the MAC should not add an TX delay in this case. But in our SoC TX internal delay is always enabled. So this scenario can not be handled. We will return -EINVAL in this case. 4) # RX and TX delays are added by the MAC when required - rgmii For phy-mode="rgmii", MAC needs to add both TX and RX delays. But in our SoC TX internal delay is always enabled so no need to add TX delay. For RX I am not sure what should we do as there is no provision of adding RX delay in MAC currently. Should we ask PHY to add RX delay? Andrew, Roger, Can you please comment on this. Apart from Case 4, below code change will be able to handle all other cases. if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID) emac->phy_if = PHY_INTERFACE_MODE_RGMII_RXID; if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_TXID) return -EINVAL; Please let me know if I am missing any other phy modes. [1] Documentation/devicetree/bindings/net/ethernet-controller.yaml > Andrew
On 09/02/2023 12:29, Md Danish Anwar wrote: > Hi Andrew, > > On 08/02/23 18:26, Andrew Lunn wrote: >>>>>>> +static int prueth_config_rgmiidelay(struct prueth *prueth, >>>>>>> + struct device_node *eth_np, >>>>>>> + phy_interface_t phy_if) >>>>>>> +{ >>>>>> >>>>>> ... >>>>>> >>>>>>> + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || >>>>>>> + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) >>>>>>> + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; >>>>>>> + >>>>>>> + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); >>>> >>>> This is only applicable to some devices so you need to restrict this only >>>> to those devices. >>>> >>> >>> Currently ICSSG driver is getting upstreamed for AM65 SR2.0 device, so I don't >>> think there is any need for any device related restriction. Once support for >>> other devices are enabled for upstream, we can modify this accordingly. >> >> The problem is, this is a board property, not a SoC property. What if >> somebody designs a board with extra long clock lines in order to add >> the delay? >> >>> I checked the latest Technical Reference Manual [1] (Section 5.1.3.4.49, Table >>> 5-624) for AM65 Silicon Revision 2.0. >>> >>> Below is the description in Table 5-624 >>> >>> BIT : 24 >>> Field : RGMII0_ID_MODE >>> Type : R/W >>> Reset : 0h >>> Description : Controls the PRU_ICSSG0 RGMII0 port internal transmit delay >>> 0h - Internal transmit delay is enabled >>> 1h - Reserved >>> >>> The TX internal delay is always enabled and couldn't be disabled as 1h is >>> reserved. So hardware support for disabling TX internal delay is not there. >> >> So if somebody passes a phy-mode which requires it disabled, you need >> to return -EINVAL, to indicate the hardware cannot actually do it. >> > > Sure, I'll do that. In the list of all phy modes described in [1], I can only > see phy-mode "rgmii-txid", for which we can return -EINVAL. Is there any other > phy-mode that requires enabling/disabling TX internal delays? Please let me > know if any other phy-mode also needs this. I will add check for that as well. > >>> As, TX internal delay is always there, there is no need to enable it in MAC or >>> PHY. So no need of API prueth_config_rgmiidelay(). >>> >>> My approach to handle delay would be as below. >>> >>> *) Keep phy-mode = "rgmii-id" in DT as asked by Andrew. >> >> As i said this depends on the board, not the SoC. In theory, you could >> design a board with an extra long RX clock line, and then use phy-mode >> rgmii-txid, meaning the MAC/PHY combination needs to add the TX delay. >> > > Yes I understand that board can have any phy-mode in it's DTS. We need to be > able to handle all different phy modes. > >>> *) Let TX internal delay enabled in Hardware. >>> *) Let PHY configure RX internal delay. >>> *) Remove prueth_config_rgmiidelay() API is there is no use of this. TX >>> Internal delay is always enabled. >>> *) Instead of calling prueth_config_rgmiidelay() API in prueth_netdev_init() >>> API, add below if condition. >>> >>> if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID) >>> emac->phy_if == PHY_INTERFACE_MODE_RGMII_RXID >> >> You should handle all cases where a TX delay is requested, not just >> ID. >> > > So there could be four different RGMII phy modes as described in [1]. Below is > the handling mechanism for different phy modes. > > 1) # RGMII with internal RX and TX delays provided by the PHY, > # the MAC should not add the RX or TX delays in this case > - rgmii-id > > For phy-mode="rgmii-id", phy needs to add both TX and RX internal delays. But > in our SoC TX internal delay is always enabled. So to handle this, we'll change OK. I thought that this MAC forced TX delay issue was fixed in Later Silicon Revisions. But it looks like it hasn't been fixed yet. > the phy-mode in driver to "rgmii-rxid" and then pass it ti PHY, so that PHY > will enable RX internal delay only. OK. > > 2) # RGMII with internal RX delay provided by the PHY, the MAC > # should not add an RX delay in this case > - rgmii-rxid > > For phy-mode="rgmii-rxid", phy needs to add only RX internal delay. We will do > nothing in the driver and just pass the same mode to phy, so that PHY will > enable RX internal delay only. But the MAC is forcing TX-delay right? So this case can't be implemented. you have to return error. > > 3) # RGMII with internal TX delay provided by the PHY, the MAC > # should not add an TX delay in this case > - rgmii-txid > > For phy-mode="rgmii-txid", phy needs to add only TX internal delay,the MAC > should not add an TX delay in this case. But in our SoC TX internal delay is > always enabled. So this scenario can not be handled. We will return -EINVAL in > this case. As you didn't return error for 1st case "rgmii-id" even though TX delay was requested for PHY but you added it in the MAC I see no reason to return error here. You just do the delay in MAC and pass "rgmii" to the PHY. > > > 4) # RX and TX delays are added by the MAC when required > - rgmii > > For phy-mode="rgmii", MAC needs to add both TX and RX delays. But in our SoC TX > internal delay is always enabled so no need to add TX delay. For RX I am not > sure what should we do as there is no provision of adding RX delay in MAC > currently. Should we ask PHY to add RX delay? > I don't think it will work so you can error out in this case. > Andrew, Roger, Can you please comment on this. > > Apart from Case 4, below code change will be able to handle all other cases. > > if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID) > emac->phy_if = PHY_INTERFACE_MODE_RGMII_RXID; > if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_TXID) > return -EINVAL; > > Please let me know if I am missing any other phy modes. > > [1] Documentation/devicetree/bindings/net/ethernet-controller.yaml > >> Andrew > cheers, -roger
On 09/02/23 18:28, Roger Quadros wrote: > > > On 09/02/2023 12:29, Md Danish Anwar wrote: >> Hi Andrew, >> >> On 08/02/23 18:26, Andrew Lunn wrote: >>>>>>>> +static int prueth_config_rgmiidelay(struct prueth *prueth, >>>>>>>> + struct device_node *eth_np, >>>>>>>> + phy_interface_t phy_if) >>>>>>>> +{ >>>>>>> >>>>>>> ... >>>>>>> >>>>>>>> + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || >>>>>>>> + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) >>>>>>>> + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; >>>>>>>> + >>>>>>>> + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); >>>>> >>>>> This is only applicable to some devices so you need to restrict this only >>>>> to those devices. >>>>> >>>> >>>> Currently ICSSG driver is getting upstreamed for AM65 SR2.0 device, so I don't >>>> think there is any need for any device related restriction. Once support for >>>> other devices are enabled for upstream, we can modify this accordingly. >>> >>> The problem is, this is a board property, not a SoC property. What if >>> somebody designs a board with extra long clock lines in order to add >>> the delay? >>> >>>> I checked the latest Technical Reference Manual [1] (Section 5.1.3.4.49, Table >>>> 5-624) for AM65 Silicon Revision 2.0. >>>> >>>> Below is the description in Table 5-624 >>>> >>>> BIT : 24 >>>> Field : RGMII0_ID_MODE >>>> Type : R/W >>>> Reset : 0h >>>> Description : Controls the PRU_ICSSG0 RGMII0 port internal transmit delay >>>> 0h - Internal transmit delay is enabled >>>> 1h - Reserved >>>> >>>> The TX internal delay is always enabled and couldn't be disabled as 1h is >>>> reserved. So hardware support for disabling TX internal delay is not there. >>> >>> So if somebody passes a phy-mode which requires it disabled, you need >>> to return -EINVAL, to indicate the hardware cannot actually do it. >>> >> >> Sure, I'll do that. In the list of all phy modes described in [1], I can only >> see phy-mode "rgmii-txid", for which we can return -EINVAL. Is there any other >> phy-mode that requires enabling/disabling TX internal delays? Please let me >> know if any other phy-mode also needs this. I will add check for that as well. >> >>>> As, TX internal delay is always there, there is no need to enable it in MAC or >>>> PHY. So no need of API prueth_config_rgmiidelay(). >>>> >>>> My approach to handle delay would be as below. >>>> >>>> *) Keep phy-mode = "rgmii-id" in DT as asked by Andrew. >>> >>> As i said this depends on the board, not the SoC. In theory, you could >>> design a board with an extra long RX clock line, and then use phy-mode >>> rgmii-txid, meaning the MAC/PHY combination needs to add the TX delay. >>> >> >> Yes I understand that board can have any phy-mode in it's DTS. We need to be >> able to handle all different phy modes. >> >>>> *) Let TX internal delay enabled in Hardware. >>>> *) Let PHY configure RX internal delay. >>>> *) Remove prueth_config_rgmiidelay() API is there is no use of this. TX >>>> Internal delay is always enabled. >>>> *) Instead of calling prueth_config_rgmiidelay() API in prueth_netdev_init() >>>> API, add below if condition. >>>> >>>> if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID) >>>> emac->phy_if == PHY_INTERFACE_MODE_RGMII_RXID >>> >>> You should handle all cases where a TX delay is requested, not just >>> ID. >>> >> >> So there could be four different RGMII phy modes as described in [1]. Below is >> the handling mechanism for different phy modes. >> >> 1) # RGMII with internal RX and TX delays provided by the PHY, >> # the MAC should not add the RX or TX delays in this case >> - rgmii-id >> >> For phy-mode="rgmii-id", phy needs to add both TX and RX internal delays. But >> in our SoC TX internal delay is always enabled. So to handle this, we'll change > > OK. I thought that this MAC forced TX delay issue was fixed in Later Silicon Revisions. > But it looks like it hasn't been fixed yet. > Yes, I confirmed with the Hardware folks, the forced TX delay issue is still there. It's not fixed as of now. > >> the phy-mode in driver to "rgmii-rxid" and then pass it ti PHY, so that PHY >> will enable RX internal delay only. > > OK. > Noted. >> >> 2) # RGMII with internal RX delay provided by the PHY, the MAC >> # should not add an RX delay in this case >> - rgmii-rxid >> >> For phy-mode="rgmii-rxid", phy needs to add only RX internal delay. We will do >> nothing in the driver and just pass the same mode to phy, so that PHY will >> enable RX internal delay only. > > But the MAC is forcing TX-delay right? So this case can't be implemented. > you have to return error. > Yes, My bad. "rgmii-rxid" is not possible. I'll return -EINVAL here. >> >> 3) # RGMII with internal TX delay provided by the PHY, the MAC >> # should not add an TX delay in this case >> - rgmii-txid >> >> For phy-mode="rgmii-txid", phy needs to add only TX internal delay,the MAC >> should not add an TX delay in this case. But in our SoC TX internal delay is >> always enabled. So this scenario can not be handled. We will return -EINVAL in >> this case. > > As you didn't return error for 1st case "rgmii-id" even though TX delay was requested > for PHY but you added it in the MAC I see no reason to return error here. > > You just do the delay in MAC and pass "rgmii" to the PHY. > Yes Noted, TX delay will be forced and phy-mode "rgmii" will be passed to PHY. >> >> >> 4) # RX and TX delays are added by the MAC when required >> - rgmii >> >> For phy-mode="rgmii", MAC needs to add both TX and RX delays. But in our SoC TX >> internal delay is always enabled so no need to add TX delay. For RX I am not >> sure what should we do as there is no provision of adding RX delay in MAC >> currently. Should we ask PHY to add RX delay? >> > > I don't think it will work so you can error out in this case. > Sure, Noted. So just to summarize, For "rgmii-id" phy mode, pass "rgmii-rxid" to phy. For "rgmii-rxid", return error. For "rgmii-txid", TX delay is forced, pass "rgmii" to PHY. For "rgmii", return error. >> Andrew, Roger, Can you please comment on this. >> >> Apart from Case 4, below code change will be able to handle all other cases. >> >> if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID) >> emac->phy_if = PHY_INTERFACE_MODE_RGMII_RXID; >> if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_TXID) >> return -EINVAL; >> >> Please let me know if I am missing any other phy modes. >> >> [1] Documentation/devicetree/bindings/net/ethernet-controller.yaml >> >>> Andrew >> > > cheers, > -roger
> Sure, I'll do that. In the list of all phy modes described in [1], I can only > see phy-mode "rgmii-txid", for which we can return -EINVAL. Is there any other > phy-mode that requires enabling/disabling TX internal delays? Please let me > know if any other phy-mode also needs this. I will add check for that as well. There are 4 phy-modes for RGMII. rgmii, rgmii-id, rmgii-rxid, rgmii-txid. rgmii-id, rgmii-txid both require TX delays. If you do that in the MAC you then need to pass rgmii-rxid and rgmii to the PHY respectively. rmii and rgmii-rxid requires no TX delays, which your SoC cannot do, so you need to return -EINVAl, The interpretation of these properties is all really messy and historically not very uniformly done. Which is why i recommend the MAC does nothing, leaving it to the PHY. That generally works since the PHYs have a pretty uniform implementation. But in your case, you don't have that option. So i suggest you do what is described above. Andrew
On 09/02/23 19:24, Andrew Lunn wrote: >> Sure, I'll do that. In the list of all phy modes described in [1], I can only >> see phy-mode "rgmii-txid", for which we can return -EINVAL. Is there any other >> phy-mode that requires enabling/disabling TX internal delays? Please let me >> know if any other phy-mode also needs this. I will add check for that as well. > > There are 4 phy-modes for RGMII. > > rgmii, rgmii-id, rmgii-rxid, rgmii-txid. > > rgmii-id, rgmii-txid both require TX delays. If you do that in the MAC > you then need to pass rgmii-rxid and rgmii to the PHY respectively. > > rmii and rgmii-rxid requires no TX delays, which your SoC cannot do, > so you need to return -EINVAl, > > The interpretation of these properties is all really messy and > historically not very uniformly done. Which is why i recommend the MAC > does nothing, leaving it to the PHY. That generally works since the > PHYs have a pretty uniform implementation. But in your case, you don't > have that option. So i suggest you do what is described above. > > Andrew Sure Andrew, I will post new revision with the changes described above.