* [PATCH 0/2] Introduce ICSSG based ethernet Driver
@ 2022-05-06 5:24 Puranjay Mohan
2022-05-06 5:24 ` [PATCH 1/2] dt-bindings: net: Add ICSSG Ethernet Driver bindings Puranjay Mohan
[not found] ` <20220506052433.28087-3-p-mohan@ti.com>
0 siblings, 2 replies; 9+ messages in thread
From: Puranjay Mohan @ 2022-05-06 5:24 UTC (permalink / raw)
To: linux-kernel
Cc: nm, devicetree, grygorii.strashko, vigneshr, andrew, netdev,
kishon, rogerq, afd, edumazet, robh+dt, krzysztof.kozlowski+dt,
ssantosh, p-mohan, davem, linux-arm-kernel
The Programmable Real-time Unit and Industrial Communication Subsystem
Gigabit (PRU_ICSSG) is a low-latency microcontroller subsystem in the TI
SoCs. This subsystem is provided for the use cases like implementation of
custom peripheral interfaces, offloading of tasks from the other
processor cores of the SoC, etc.
The subsystem includes many accelerators for data processing like
multiplier and multiplier-accumulator. It also has peripherals like
UART, MII/RGMII, MDIO, etc. Every ICSSG core includes two 32-bit
load/store RISC CPU cores called PRUs.
The above features allow it to be used for implementing custom firmware
based peripherals like ethernet.
This series adds the YAML documentation and the driver with basic EMAC
support for TI AM654 Silicon Rev 2 SoC with the PRU_ICSSG Sub-system.
running dual-EMAC firmware.
This currently supports basic EMAC with 1Gbps and 100Mbps link. 10M and
half-duplex modes are not yet supported because they require the support
of an IEP, which will be added later.
Advanced features like switch-dev and timestamping will be added later.
This series depends on two patch series that are not yet merged, one in
the remoteproc tree and another in the soc tree. the first one is titled
Introduce PRU remoteproc consumer API and the second one is titled
Introduce PRU platform consumer API.
Both of these are required for this driver.
To explain this dependency and to get reviews, I had earlier posted all
three of these as an RFC[1], this can be seen for understanding the
dependencies.
I then posted the remoteproc[2] and soc[3] series seperately to their
respective trees.
[1] https://lore.kernel.org/all/20220406094358.7895-1-p-mohan@ti.com/
[2] https://patchwork.kernel.org/project/linux-remoteproc/cover/20220418104118.12878-1-p-mohan@ti.com/
[3] https://patchwork.kernel.org/project/linux-remoteproc/cover/20220418123004.9332-1-p-mohan@ti.com/
Thanks and Regards,
Puranjay Mohan
Puranjay Mohan (1):
dt-bindings: net: Add ICSSG Ethernet Driver bindings
Roger Quadros (1):
net: ti: icssg-prueth: Add ICSSG ethernet driver
.../bindings/net/ti,icssg-prueth.yaml | 174 ++
drivers/net/ethernet/ti/Kconfig | 15 +
drivers/net/ethernet/ti/Makefile | 3 +
drivers/net/ethernet/ti/icssg_classifier.c | 375 ++++
drivers/net/ethernet/ti/icssg_config.c | 443 ++++
drivers/net/ethernet/ti/icssg_config.h | 200 ++
drivers/net/ethernet/ti/icssg_ethtool.c | 301 +++
drivers/net/ethernet/ti/icssg_mii_cfg.c | 104 +
drivers/net/ethernet/ti/icssg_mii_rt.h | 151 ++
drivers/net/ethernet/ti/icssg_prueth.c | 1891 +++++++++++++++++
drivers/net/ethernet/ti/icssg_prueth.h | 247 +++
drivers/net/ethernet/ti/icssg_switch_map.h | 183 ++
include/linux/pruss.h | 1 +
13 files changed, 4088 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
create mode 100644 drivers/net/ethernet/ti/icssg_classifier.c
create mode 100644 drivers/net/ethernet/ti/icssg_config.c
create mode 100644 drivers/net/ethernet/ti/icssg_config.h
create mode 100644 drivers/net/ethernet/ti/icssg_ethtool.c
create mode 100644 drivers/net/ethernet/ti/icssg_mii_cfg.c
create mode 100644 drivers/net/ethernet/ti/icssg_mii_rt.h
create mode 100644 drivers/net/ethernet/ti/icssg_prueth.c
create mode 100644 drivers/net/ethernet/ti/icssg_prueth.h
create mode 100644 drivers/net/ethernet/ti/icssg_switch_map.h
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/2] dt-bindings: net: Add ICSSG Ethernet Driver bindings 2022-05-06 5:24 [PATCH 0/2] Introduce ICSSG based ethernet Driver Puranjay Mohan @ 2022-05-06 5:24 ` Puranjay Mohan 2022-05-06 13:37 ` Rob Herring 2022-05-10 18:07 ` Rob Herring [not found] ` <20220506052433.28087-3-p-mohan@ti.com> 1 sibling, 2 replies; 9+ messages in thread From: Puranjay Mohan @ 2022-05-06 5:24 UTC (permalink / raw) To: linux-kernel Cc: nm, devicetree, grygorii.strashko, vigneshr, andrew, netdev, kishon, rogerq, afd, edumazet, robh+dt, krzysztof.kozlowski+dt, ssantosh, p-mohan, davem, linux-arm-kernel 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> --- .../bindings/net/ti,icssg-prueth.yaml | 174 ++++++++++++++++++ 1 file changed, 174 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..ca6f0af411cf --- /dev/null +++ b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml @@ -0,0 +1,174 @@ +# 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: + - Puranjay Mohan <p-mohan@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 + + sram: + description: | + phandle to MSMC SRAM node + + dmas: + minItems: 10 + maxItems: 10 + description: | + list of phandles and specifiers to UDMA as specified in bindings/dma/ti/k3-udma.txt. + + 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: + type: object + properties: + '#address-cells': + const: 1 + '#size-cells': + const: 0 + + patternProperties: + ^port@[0-1]$: + type: object + description: ICSSG PRUETH external ports + + $ref: ethernet-controller.yaml# + + properties: + reg: + items: + - enum: [0, 1] + description: ICSSG PRUETH port number + + ti,syscon-rgmii-delay: + $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 + + additionalProperties: false + + 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: + minItems: 2 + maxItems: 2 + description: | + Interrupt specifiers to TX timestamp IRQ. + + interrupt-names: + items: + - const: tx_ts0 + - const: tx_ts1 + +required: + - compatible + - sram + - ti,mii-g-rt + - dmas + - dma-names + - ethernet-ports + - interrupts + - interrupt-names + +unevaluatedProperties: false + +examples: + - | + + /* Example k3-am654 base board SR2.0, dual-emac */ + pruss2_eth: pruss2_eth { + compatible = "ti,am654-icssg-prueth"; + pinctrl-names = "default"; + pinctrl-0 = <&icssg2_rgmii_pins_default>; + 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>; + ti,mii-g-rt = <&icssg2_mii_g_rt>; + 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"; + interrupts = <24 0 2>, <25 1 3>; + interrupt-names = "tx_ts0", "tx_ts1"; + ethernet-ports { + #address-cells = <1>; + #size-cells = <0>; + pruss2_emac0: port@0 { + reg = <0>; + phy-handle = <&pruss2_eth0_phy>; + phy-mode = "rgmii-rxid"; + interrupts-extended = <&icssg2_intc 24>; + ti,syscon-rgmii-delay = <&scm_conf 0x4120>; + /* Filled in by bootloader */ + local-mac-address = [00 00 00 00 00 00]; + }; + + pruss2_emac1: port@1 { + reg = <1>; + phy-handle = <&pruss2_eth1_phy>; + phy-mode = "rgmii-rxid"; + interrupts-extended = <&icssg2_intc 25>; + ti,syscon-rgmii-delay = <&scm_conf 0x4124>; + /* Filled in by bootloader */ + local-mac-address = [00 00 00 00 00 00]; + }; + }; + }; -- 2.17.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] dt-bindings: net: Add ICSSG Ethernet Driver bindings 2022-05-06 5:24 ` [PATCH 1/2] dt-bindings: net: Add ICSSG Ethernet Driver bindings Puranjay Mohan @ 2022-05-06 13:37 ` Rob Herring 2022-05-10 18:07 ` Rob Herring 1 sibling, 0 replies; 9+ messages in thread From: Rob Herring @ 2022-05-06 13:37 UTC (permalink / raw) To: Puranjay Mohan Cc: nm, andrew, grygorii.strashko, vigneshr, devicetree, netdev, ssantosh, linux-kernel, kishon, rogerq, edumazet, robh+dt, krzysztof.kozlowski+dt, afd, davem, linux-arm-kernel On Fri, 06 May 2022 10:54:32 +0530, Puranjay Mohan wrote: > 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> > --- > .../bindings/net/ti,icssg-prueth.yaml | 174 ++++++++++++++++++ > 1 file changed, 174 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/linux-dt-review/Documentation/devicetree/bindings/net/ti,icssg-prueth.example.dtb: pruss2_eth: False schema does not allow {'compatible': ['ti,am654-icssg-prueth'], 'pinctrl-names': ['default'], 'pinctrl-0': [[4294967295]], '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]], 'ti,mii-g-rt': [[4294967295]], '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'], '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-rxid'], '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-rxid'], 'interrupts-extended': [[4294967295, 25]], 'ti,syscon-rgmii-delay': [[4294967295, 16676]], 'local-mac-address': [[0, 0, 0, 0, 0, 0]]}}, '$nodename': ['pruss2_eth']} From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/ti,icssg-prueth.example.dtb: pruss2_eth: Unevaluated properties are not allowed ('firmware-name', 'ti,prus', 'ti,pruss-gp-mux-sel' were unexpected) From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] dt-bindings: net: Add ICSSG Ethernet Driver bindings 2022-05-06 5:24 ` [PATCH 1/2] dt-bindings: net: Add ICSSG Ethernet Driver bindings Puranjay Mohan 2022-05-06 13:37 ` Rob Herring @ 2022-05-10 18:07 ` Rob Herring 1 sibling, 0 replies; 9+ messages in thread From: Rob Herring @ 2022-05-10 18:07 UTC (permalink / raw) To: Puranjay Mohan Cc: nm, devicetree, grygorii.strashko, vigneshr, andrew, netdev, linux-kernel, kishon, rogerq, afd, edumazet, krzysztof.kozlowski+dt, ssantosh, davem, linux-arm-kernel On Fri, May 06, 2022 at 10:54:32AM +0530, Puranjay Mohan wrote: > 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> > --- > .../bindings/net/ti,icssg-prueth.yaml | 174 ++++++++++++++++++ > 1 file changed, 174 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..ca6f0af411cf > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml > @@ -0,0 +1,174 @@ > +# 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: > + - Puranjay Mohan <p-mohan@ti.com> > + > +description: |+ Don't need '|+' > + Ethernet based on the Programmable Real-Time Unit and Industrial Communication Subsystem. Wrap the line at 80. > + > +allOf: > + - $ref: /schemas/remoteproc/ti,pru-consumer.yaml# > + > +properties: > + compatible: > + enum: > + - ti,am654-icssg-prueth # for AM65x SoC family > + > + sram: > + description: | > + phandle to MSMC SRAM node > + > + dmas: > + minItems: 10 > + maxItems: 10 Just maxItems is enough. > + description: | > + list of phandles and specifiers to UDMA as specified in bindings/dma/ti/k3-udma.txt. Drop. First, we don't want new references to .txt files. Second, the specific provider is generally outside the scope of a binding. > + > + 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: > + type: object > + properties: > + '#address-cells': > + const: 1 > + '#size-cells': > + const: 0 > + > + patternProperties: > + ^port@[0-1]$: ethernet-port is preferred. > + type: object > + description: ICSSG PRUETH external ports > + > + $ref: ethernet-controller.yaml# unevaluatedProperties: false > + > + properties: > + reg: > + items: > + - enum: [0, 1] > + description: ICSSG PRUETH port number > + > + ti,syscon-rgmii-delay: > + $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 > + > + additionalProperties: false For indented cases, I think it is easier to read if this is above 'properties'. > + > + 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: > + minItems: 2 > + maxItems: 2 > + description: | > + Interrupt specifiers to TX timestamp IRQ. > + > + interrupt-names: > + items: > + - const: tx_ts0 > + - const: tx_ts1 > + > +required: > + - compatible > + - sram > + - ti,mii-g-rt > + - dmas > + - dma-names > + - ethernet-ports > + - interrupts > + - interrupt-names > + > +unevaluatedProperties: false > + > +examples: > + - | > + > + /* Example k3-am654 base board SR2.0, dual-emac */ > + pruss2_eth: pruss2_eth { Indentation here should be 4. > + compatible = "ti,am654-icssg-prueth"; > + pinctrl-names = "default"; > + pinctrl-0 = <&icssg2_rgmii_pins_default>; > + sram = <&msmc_ram>; > + > + ti,prus = <&pru2_0>, <&rtu2_0>, <&tx_pru2_0>, <&pru2_1>, <&rtu2_1>, <&tx_pru2_1>; Required? You should also list this in properties and define how many entries. > + 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>; > + ti,mii-g-rt = <&icssg2_mii_g_rt>; > + 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"; > + interrupts = <24 0 2>, <25 1 3>; > + interrupt-names = "tx_ts0", "tx_ts1"; > + ethernet-ports { > + #address-cells = <1>; > + #size-cells = <0>; > + pruss2_emac0: port@0 { > + reg = <0>; > + phy-handle = <&pruss2_eth0_phy>; > + phy-mode = "rgmii-rxid"; > + interrupts-extended = <&icssg2_intc 24>; > + ti,syscon-rgmii-delay = <&scm_conf 0x4120>; > + /* Filled in by bootloader */ > + local-mac-address = [00 00 00 00 00 00]; > + }; > + > + pruss2_emac1: port@1 { > + reg = <1>; > + phy-handle = <&pruss2_eth1_phy>; > + phy-mode = "rgmii-rxid"; > + interrupts-extended = <&icssg2_intc 25>; > + ti,syscon-rgmii-delay = <&scm_conf 0x4124>; > + /* Filled in by bootloader */ > + local-mac-address = [00 00 00 00 00 00]; > + }; > + }; > + }; > -- > 2.17.1 > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <20220506052433.28087-3-p-mohan@ti.com>]
* Re: [PATCH 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver [not found] ` <20220506052433.28087-3-p-mohan@ti.com> @ 2022-05-06 16:44 ` Andrew Lunn 2022-05-09 10:20 ` Puranjay Mohan 2022-05-06 16:49 ` Andrew Lunn 1 sibling, 1 reply; 9+ messages in thread From: Andrew Lunn @ 2022-05-06 16:44 UTC (permalink / raw) To: Puranjay Mohan Cc: nm, devicetree, grygorii.strashko, vigneshr, netdev, linux-kernel, kishon, rogerq, afd, edumazet, robh+dt, krzysztof.kozlowski+dt, ssantosh, davem, linux-arm-kernel > +void icssg_config_ipg(struct prueth_emac *emac) > +{ > + struct prueth *prueth = emac->prueth; > + int slice = prueth_emac_slice(emac); > + > + switch (emac->speed) { > + case SPEED_1000: > + icssg_mii_update_ipg(prueth->mii_rt, slice, MII_RT_TX_IPG_1G); > + break; > + case SPEED_100: > + icssg_mii_update_ipg(prueth->mii_rt, slice, MII_RT_TX_IPG_100M); > + break; > + default: > + /* Other links speeds not supported */ > + pr_err("Unsupported link speed\n"); dev_err() or netdev_err(). You then get an idea which device somebody is trying to configure into an unsupported mode. checkpatch probably also warned about that? > +static void icssg_init_emac_mode(struct prueth *prueth) > +{ > + u8 mac[ETH_ALEN] = { 0 }; > + > + if (prueth->emacs_initialized) > + return; > + > + regmap_update_bits(prueth->miig_rt, FDB_GEN_CFG1, SMEM_VLAN_OFFSET_MASK, 0); > + regmap_write(prueth->miig_rt, FDB_GEN_CFG2, 0); > + /* Clear host MAC address */ > + icssg_class_set_host_mac_addr(prueth->miig_rt, mac); Seems an odd thing to do, set it to 00:00:00:00:00:00. You probably want to add a comment why you do this odd thing. > +int emac_set_port_state(struct prueth_emac *emac, > + enum icssg_port_state_cmd cmd) > +{ > + struct icssg_r30_cmd *p; > + int ret = -ETIMEDOUT; > + int timeout = 10; > + int i; > + > + p = emac->dram.va + MGR_R30_CMD_OFFSET; > + > + if (cmd >= ICSSG_EMAC_PORT_MAX_COMMANDS) { > + netdev_err(emac->ndev, "invalid port command\n"); > + return -EINVAL; > + } > + > + /* only one command at a time allowed to firmware */ > + mutex_lock(&emac->cmd_lock); > + > + for (i = 0; i < 4; i++) > + writel(emac_r32_bitmask[cmd].cmd[i], &p->cmd[i]); > + > + /* wait for done */ > + while (timeout) { > + if (emac_r30_is_done(emac)) { > + ret = 0; > + break; > + } > + > + usleep_range(1000, 2000); > + timeout--; > + } linux/iopoll.h > +void icssg_config_set_speed(struct prueth_emac *emac) > +{ > + u8 fw_speed; > + > + switch (emac->speed) { > + case SPEED_1000: > + fw_speed = FW_LINK_SPEED_1G; > + break; > + case SPEED_100: > + fw_speed = FW_LINK_SPEED_100M; > + break; > + default: > + /* Other links speeds not supported */ > + pr_err("Unsupported link speed\n"); dev_err() or netdev_err(). > +static int emac_get_link_ksettings(struct net_device *ndev, > + struct ethtool_link_ksettings *ecmd) > +{ > + struct prueth_emac *emac = netdev_priv(ndev); > + > + if (!emac->phydev) > + return -EOPNOTSUPP; > + > + phy_ethtool_ksettings_get(emac->phydev, ecmd); > + return 0; > +} phy_ethtool_get_link_ksettings(). You should keep phydev in ndev, not your priv structure. > + > +static int emac_set_link_ksettings(struct net_device *ndev, > + const struct ethtool_link_ksettings *ecmd) > +{ > + struct prueth_emac *emac = netdev_priv(ndev); > + > + if (!emac->phydev) > + return -EOPNOTSUPP; > + > + return phy_ethtool_ksettings_set(emac->phydev, ecmd); phy_ethtool_set_link_ksettings() > +static int emac_nway_reset(struct net_device *ndev) > +{ > + struct prueth_emac *emac = netdev_priv(ndev); > + > + if (!emac->phydev) > + return -EOPNOTSUPP; > + > + return genphy_restart_aneg(emac->phydev); phy_ethtool_nway_reset() > +static void emac_get_ethtool_stats(struct net_device *ndev, > + struct ethtool_stats *stats, u64 *data) > +{ > + struct prueth_emac *emac = netdev_priv(ndev); > + struct prueth *prueth = emac->prueth; > + int i; > + int slice = prueth_emac_slice(emac); > + u32 base = stats_base[slice]; > + u32 val; Reverse Christmas tree. Move i to the end. There are other places in the driver you need to fix up as well. > +static int debug_level = -1; > +module_param(debug_level, int, 0644); > +MODULE_PARM_DESC(debug_level, "PRUETH debug level (NETIF_MSG bits)"); Module parameters are not liked any more. Yes, lots of drivers have this one, but you have the ethtool setting, so you should not need this. > +/* called back by PHY layer if there is change in link state of hw port*/ > +static void emac_adjust_link(struct net_device *ndev) > +{ > + struct prueth_emac *emac = netdev_priv(ndev); > + struct phy_device *phydev = emac->phydev; > + struct prueth *prueth = emac->prueth; > + bool new_state = false; > + unsigned long flags; > + > + if (phydev->link) { > + /* check the mode of operation - full/half duplex */ > + if (phydev->duplex != emac->duplex) { > + new_state = true; > + emac->duplex = phydev->duplex; > + } > + if (phydev->speed != emac->speed) { > + new_state = true; > + emac->speed = phydev->speed; > + } > + if (!emac->link) { > + new_state = true; > + emac->link = 1; > + } > + } else if (emac->link) { > + new_state = true; > + emac->link = 0; > + /* defaults for no link */ > + > + /* f/w should support 100 & 1000 */ > + emac->speed = SPEED_1000; > + > + /* half duplex may not supported by f/w */ > + emac->duplex = DUPLEX_FULL; Why set speed and duplex when you have just lost the link? They are meaningless until the link comes back. > + } > + > + if (new_state) { > + phy_print_status(phydev); > + > + /* update RGMII and MII configuration based on PHY negotiated > + * values > + */ > + if (emac->link) { > + /* Set the RGMII cfg for gig en and full duplex */ > + icssg_update_rgmii_cfg(prueth->miig_rt, emac); > + > + /* update the Tx IPG based on 100M/1G speed */ > + spin_lock_irqsave(&emac->lock, flags); > + icssg_config_ipg(emac); > + spin_unlock_irqrestore(&emac->lock, flags); > + icssg_config_set_speed(emac); > + emac_set_port_state(emac, ICSSG_EMAC_PORT_FORWARD); > + > + } else { > + emac_set_port_state(emac, ICSSG_EMAC_PORT_DISABLE); > + } > + } > + > + if (emac->link) { > + /* link ON */ > + netif_carrier_on(ndev); phylib will do this for you. > + /* reactivate the transmit queue */ > + netif_tx_wake_all_queues(ndev); Not something you see other drivers do. Why is it here? > + } else { > + /* link OFF */ > + netif_carrier_off(ndev); > + netif_tx_stop_all_queues(ndev); Same as above, for both. > +static int emac_ndo_open(struct net_device *ndev) > +{ > + struct prueth_emac *emac = netdev_priv(ndev); > + int ret, i, num_data_chn = emac->tx_ch_num; > + struct prueth *prueth = emac->prueth; > + int slice = prueth_emac_slice(emac); > + struct device *dev = prueth->dev; > + int max_rx_flows; > + int rx_flow; > + > + /* clear SMEM and MSMC settings for all slices */ > + if (!prueth->emacs_initialized) { > + memset_io(prueth->msmcram.va, 0, prueth->msmcram.size); > + memset_io(prueth->shram.va, 0, ICSSG_CONFIG_OFFSET_SLICE1 * PRUETH_NUM_MACS); > + } > + > + /* set h/w MAC as user might have re-configured */ > + ether_addr_copy(emac->mac_addr, ndev->dev_addr); > + > + icssg_class_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr); > + icssg_ft1_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr); > + > + icssg_class_default(prueth->miig_rt, slice, 0); > + > + netif_carrier_off(ndev); It should default to off. phylib will turn it on for you when you get link. > +static int emac_ndo_ioctl(struct net_device *ndev, struct ifreq *ifr, int cmd) > +{ > + struct prueth_emac *emac = netdev_priv(ndev); > + > + if (!emac->phydev) > + return -EOPNOTSUPP; > + > + return phy_mii_ioctl(emac->phydev, ifr, cmd); > +} phy_do_ioctl() > +extern const struct ethtool_ops icssg_ethtool_ops; Should really by in a header file. > +static int prueth_probe(struct platform_device *pdev) > +{ > + struct prueth *prueth; > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + struct device_node *eth_ports_node; > + struct device_node *eth_node; > + struct device_node *eth0_node, *eth1_node; > + const struct of_device_id *match; > + struct pruss *pruss; > + int i, ret; > + u32 msmc_ram_size; > + struct genpool_data_align gp_data = { > + .align = SZ_64K, > + }; > + > + match = of_match_device(prueth_dt_match, dev); > + if (!match) > + return -ENODEV; > + > + prueth = devm_kzalloc(dev, sizeof(*prueth), GFP_KERNEL); > + if (!prueth) > + return -ENOMEM; > + > + dev_set_drvdata(dev, prueth); > + prueth->pdev = pdev; > + prueth->pdata = *(const struct prueth_pdata *)match->data; > + > + prueth->dev = dev; > + eth_ports_node = of_get_child_by_name(np, "ethernet-ports"); > + if (!eth_ports_node) > + return -ENOENT; > + > + for_each_child_of_node(eth_ports_node, eth_node) { > + u32 reg; > + > + if (strcmp(eth_node->name, "port")) > + continue; > + ret = of_property_read_u32(eth_node, "reg", ®); > + if (ret < 0) { > + dev_err(dev, "%pOF error reading port_id %d\n", > + eth_node, ret); > + } > + > + if (reg == 0) > + eth0_node = eth_node; > + else if (reg == 1) > + eth1_node = eth_node; and if reg == 42? Or reg 0 appears twice? Andrew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver 2022-05-06 16:44 ` [PATCH 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver Andrew Lunn @ 2022-05-09 10:20 ` Puranjay Mohan 2022-05-09 12:32 ` Andrew Lunn 0 siblings, 1 reply; 9+ messages in thread From: Puranjay Mohan @ 2022-05-09 10:20 UTC (permalink / raw) To: Andrew Lunn Cc: nm, devicetree, grygorii.strashko, vigneshr, netdev, linux-kernel, kishon, rogerq, afd, edumazet, robh+dt, krzysztof.kozlowski+dt, ssantosh, davem, linux-arm-kernel Hi Andrew, Thanks for your comments. On 06/05/22 22:14, Andrew Lunn wrote: >> +void icssg_config_ipg(struct prueth_emac *emac) >> +{ >> + struct prueth *prueth = emac->prueth; >> + int slice = prueth_emac_slice(emac); >> + >> + switch (emac->speed) { >> + case SPEED_1000: >> + icssg_mii_update_ipg(prueth->mii_rt, slice, MII_RT_TX_IPG_1G); >> + break; >> + case SPEED_100: >> + icssg_mii_update_ipg(prueth->mii_rt, slice, MII_RT_TX_IPG_100M); >> + break; >> + default: >> + /* Other links speeds not supported */ >> + pr_err("Unsupported link speed\n"); > > dev_err() or netdev_err(). You then get an idea which device somebody > is trying to configure into an unsupported mode. I will use netdev_err() in next version. > > checkpatch probably also warned about that? unfortunately it didn't. > >> +static void icssg_init_emac_mode(struct prueth *prueth) >> +{ >> + u8 mac[ETH_ALEN] = { 0 }; >> + >> + if (prueth->emacs_initialized) >> + return; >> + >> + regmap_update_bits(prueth->miig_rt, FDB_GEN_CFG1, SMEM_VLAN_OFFSET_MASK, 0); >> + regmap_write(prueth->miig_rt, FDB_GEN_CFG2, 0); >> + /* Clear host MAC address */ >> + icssg_class_set_host_mac_addr(prueth->miig_rt, mac); > > Seems an odd thing to do, set it to 00:00:00:00:00:00. You probably > want to add a comment why you do this odd thing. Actually, this is when the device is configured as a bridge, the host mac address has to be set to zero to while bringing it back to emac mode. I will add a comment to explain this. > >> +int emac_set_port_state(struct prueth_emac *emac, >> + enum icssg_port_state_cmd cmd) >> +{ >> + struct icssg_r30_cmd *p; >> + int ret = -ETIMEDOUT; >> + int timeout = 10; >> + int i; >> + >> + p = emac->dram.va + MGR_R30_CMD_OFFSET; >> + >> + if (cmd >= ICSSG_EMAC_PORT_MAX_COMMANDS) { >> + netdev_err(emac->ndev, "invalid port command\n"); >> + return -EINVAL; >> + } >> + >> + /* only one command at a time allowed to firmware */ >> + mutex_lock(&emac->cmd_lock); >> + >> + for (i = 0; i < 4; i++) >> + writel(emac_r32_bitmask[cmd].cmd[i], &p->cmd[i]); >> + >> + /* wait for done */ >> + while (timeout) { >> + if (emac_r30_is_done(emac)) { >> + ret = 0; >> + break; >> + } >> + >> + usleep_range(1000, 2000); >> + timeout--; >> + } > > linux/iopoll.h will add in next version > >> +void icssg_config_set_speed(struct prueth_emac *emac) >> +{ >> + u8 fw_speed; >> + >> + switch (emac->speed) { >> + case SPEED_1000: >> + fw_speed = FW_LINK_SPEED_1G; >> + break; >> + case SPEED_100: >> + fw_speed = FW_LINK_SPEED_100M; >> + break; >> + default: >> + /* Other links speeds not supported */ >> + pr_err("Unsupported link speed\n"); > > dev_err() or netdev_err(). > > >> +static int emac_get_link_ksettings(struct net_device *ndev, >> + struct ethtool_link_ksettings *ecmd) >> +{ >> + struct prueth_emac *emac = netdev_priv(ndev); >> + >> + if (!emac->phydev) >> + return -EOPNOTSUPP; >> + >> + phy_ethtool_ksettings_get(emac->phydev, ecmd); >> + return 0; >> +} > > phy_ethtool_get_link_ksettings(). > > You should keep phydev in ndev, not your priv structure. Okay, I will make this change in the whole driver. > >> + >> +static int emac_set_link_ksettings(struct net_device *ndev, >> + const struct ethtool_link_ksettings *ecmd) >> +{ >> + struct prueth_emac *emac = netdev_priv(ndev); >> + >> + if (!emac->phydev) >> + return -EOPNOTSUPP; >> + >> + return phy_ethtool_ksettings_set(emac->phydev, ecmd); > > phy_ethtool_set_link_ksettings() > >> +static int emac_nway_reset(struct net_device *ndev) >> +{ >> + struct prueth_emac *emac = netdev_priv(ndev); >> + >> + if (!emac->phydev) >> + return -EOPNOTSUPP; >> + >> + return genphy_restart_aneg(emac->phydev); > > phy_ethtool_nway_reset() > >> +static void emac_get_ethtool_stats(struct net_device *ndev, >> + struct ethtool_stats *stats, u64 *data) >> +{ >> + struct prueth_emac *emac = netdev_priv(ndev); >> + struct prueth *prueth = emac->prueth; >> + int i; >> + int slice = prueth_emac_slice(emac); >> + u32 base = stats_base[slice]; >> + u32 val; > > Reverse Christmas tree. Move i to the end. There are other places in > the driver you need to fix up as well. > >> +static int debug_level = -1; >> +module_param(debug_level, int, 0644); >> +MODULE_PARM_DESC(debug_level, "PRUETH debug level (NETIF_MSG bits)"); > > Module parameters are not liked any more. Yes, lots of drivers have > this one, but you have the ethtool setting, so you should not need > this. > >> +/* called back by PHY layer if there is change in link state of hw port*/ >> +static void emac_adjust_link(struct net_device *ndev) >> +{ >> + struct prueth_emac *emac = netdev_priv(ndev); >> + struct phy_device *phydev = emac->phydev; >> + struct prueth *prueth = emac->prueth; >> + bool new_state = false; >> + unsigned long flags; >> + >> + if (phydev->link) { >> + /* check the mode of operation - full/half duplex */ >> + if (phydev->duplex != emac->duplex) { >> + new_state = true; >> + emac->duplex = phydev->duplex; >> + } >> + if (phydev->speed != emac->speed) { >> + new_state = true; >> + emac->speed = phydev->speed; >> + } >> + if (!emac->link) { >> + new_state = true; >> + emac->link = 1; >> + } >> + } else if (emac->link) { >> + new_state = true; >> + emac->link = 0; >> + /* defaults for no link */ >> + >> + /* f/w should support 100 & 1000 */ >> + emac->speed = SPEED_1000; >> + >> + /* half duplex may not supported by f/w */ >> + emac->duplex = DUPLEX_FULL; > > Why set speed and duplex when you have just lost the link? They are > meaningless until the link comes back. These were just the default values that we added. What do you suggest I put here? > >> + } >> + >> + if (new_state) { >> + phy_print_status(phydev); >> + >> + /* update RGMII and MII configuration based on PHY negotiated >> + * values >> + */ >> + if (emac->link) { >> + /* Set the RGMII cfg for gig en and full duplex */ >> + icssg_update_rgmii_cfg(prueth->miig_rt, emac); >> + >> + /* update the Tx IPG based on 100M/1G speed */ >> + spin_lock_irqsave(&emac->lock, flags); >> + icssg_config_ipg(emac); >> + spin_unlock_irqrestore(&emac->lock, flags); >> + icssg_config_set_speed(emac); >> + emac_set_port_state(emac, ICSSG_EMAC_PORT_FORWARD); >> + >> + } else { >> + emac_set_port_state(emac, ICSSG_EMAC_PORT_DISABLE); >> + } >> + } >> + >> + if (emac->link) { >> + /* link ON */ >> + netif_carrier_on(ndev); > > phylib will do this for you. > >> + /* reactivate the transmit queue */ >> + netif_tx_wake_all_queues(ndev); > > Not something you see other drivers do. Why is it here? > >> + } else { >> + /* link OFF */ >> + netif_carrier_off(ndev); >> + netif_tx_stop_all_queues(ndev); > > Same as above, for both. > >> +static int emac_ndo_open(struct net_device *ndev) >> +{ >> + struct prueth_emac *emac = netdev_priv(ndev); >> + int ret, i, num_data_chn = emac->tx_ch_num; >> + struct prueth *prueth = emac->prueth; >> + int slice = prueth_emac_slice(emac); >> + struct device *dev = prueth->dev; >> + int max_rx_flows; >> + int rx_flow; >> + >> + /* clear SMEM and MSMC settings for all slices */ >> + if (!prueth->emacs_initialized) { >> + memset_io(prueth->msmcram.va, 0, prueth->msmcram.size); >> + memset_io(prueth->shram.va, 0, ICSSG_CONFIG_OFFSET_SLICE1 * PRUETH_NUM_MACS); >> + } >> + >> + /* set h/w MAC as user might have re-configured */ >> + ether_addr_copy(emac->mac_addr, ndev->dev_addr); >> + >> + icssg_class_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr); >> + icssg_ft1_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr); >> + >> + icssg_class_default(prueth->miig_rt, slice, 0); >> + >> + netif_carrier_off(ndev); > > It should default to off. phylib will turn it on for you when you get > link. > >> +static int emac_ndo_ioctl(struct net_device *ndev, struct ifreq *ifr, int cmd) >> +{ >> + struct prueth_emac *emac = netdev_priv(ndev); >> + >> + if (!emac->phydev) >> + return -EOPNOTSUPP; >> + >> + return phy_mii_ioctl(emac->phydev, ifr, cmd); >> +} > > phy_do_ioctl() > >> +extern const struct ethtool_ops icssg_ethtool_ops; > > Should really by in a header file. > >> +static int prueth_probe(struct platform_device *pdev) >> +{ >> + struct prueth *prueth; >> + struct device *dev = &pdev->dev; >> + struct device_node *np = dev->of_node; >> + struct device_node *eth_ports_node; >> + struct device_node *eth_node; >> + struct device_node *eth0_node, *eth1_node; >> + const struct of_device_id *match; >> + struct pruss *pruss; >> + int i, ret; >> + u32 msmc_ram_size; >> + struct genpool_data_align gp_data = { >> + .align = SZ_64K, >> + }; >> + >> + match = of_match_device(prueth_dt_match, dev); >> + if (!match) >> + return -ENODEV; >> + >> + prueth = devm_kzalloc(dev, sizeof(*prueth), GFP_KERNEL); >> + if (!prueth) >> + return -ENOMEM; >> + >> + dev_set_drvdata(dev, prueth); >> + prueth->pdev = pdev; >> + prueth->pdata = *(const struct prueth_pdata *)match->data; >> + >> + prueth->dev = dev; >> + eth_ports_node = of_get_child_by_name(np, "ethernet-ports"); >> + if (!eth_ports_node) >> + return -ENOENT; >> + >> + for_each_child_of_node(eth_ports_node, eth_node) { >> + u32 reg; >> + >> + if (strcmp(eth_node->name, "port")) >> + continue; >> + ret = of_property_read_u32(eth_node, "reg", ®); >> + if (ret < 0) { >> + dev_err(dev, "%pOF error reading port_id %d\n", >> + eth_node, ret); >> + } >> + >> + if (reg == 0) >> + eth0_node = eth_node; >> + else if (reg == 1) >> + eth1_node = eth_node; > > and if reg == 4 > > Or reg 0 appears twice? In both of the cases that you mentioned, the device tree schema check will fail, hence, we can safely assume that this will be 0 and 1 only. > > Andrew Thanks, Puranjay Mohan _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver 2022-05-09 10:20 ` Puranjay Mohan @ 2022-05-09 12:32 ` Andrew Lunn 2022-05-16 5:09 ` Puranjay Mohan 0 siblings, 1 reply; 9+ messages in thread From: Andrew Lunn @ 2022-05-09 12:32 UTC (permalink / raw) To: Puranjay Mohan Cc: nm, devicetree, grygorii.strashko, vigneshr, netdev, linux-kernel, kishon, rogerq, afd, edumazet, robh+dt, krzysztof.kozlowski+dt, ssantosh, davem, linux-arm-kernel > >> +static void icssg_init_emac_mode(struct prueth *prueth) > >> +{ > >> + u8 mac[ETH_ALEN] = { 0 }; > >> + > >> + if (prueth->emacs_initialized) > >> + return; > >> + > >> + regmap_update_bits(prueth->miig_rt, FDB_GEN_CFG1, SMEM_VLAN_OFFSET_MASK, 0); > >> + regmap_write(prueth->miig_rt, FDB_GEN_CFG2, 0); > >> + /* Clear host MAC address */ > >> + icssg_class_set_host_mac_addr(prueth->miig_rt, mac); > > > > Seems an odd thing to do, set it to 00:00:00:00:00:00. You probably > > want to add a comment why you do this odd thing. > > Actually, this is when the device is configured as a bridge, the host > mac address has to be set to zero to while bringing it back to emac > mode. I will add a comment to explain this. I don't see any switchdev interface. How does it get into bridge mode? > >> + } else if (emac->link) { > >> + new_state = true; > >> + emac->link = 0; > >> + /* defaults for no link */ > >> + > >> + /* f/w should support 100 & 1000 */ > >> + emac->speed = SPEED_1000; > >> + > >> + /* half duplex may not supported by f/w */ > >> + emac->duplex = DUPLEX_FULL; > > > > Why set speed and duplex when you have just lost the link? They are > > meaningless until the link comes back. > > These were just the default values that we added. > What do you suggest I put here? Nothing. If the link is down, they are meaningless. If something is accessing them when the link is down, that code is broken. So i suppose you could give them poison values to help find your broken code. > >> + for_each_child_of_node(eth_ports_node, eth_node) { > >> + u32 reg; > >> + > >> + if (strcmp(eth_node->name, "port")) > >> + continue; > >> + ret = of_property_read_u32(eth_node, "reg", ®); > >> + if (ret < 0) { > >> + dev_err(dev, "%pOF error reading port_id %d\n", > >> + eth_node, ret); > >> + } > >> + > >> + if (reg == 0) > >> + eth0_node = eth_node; > >> + else if (reg == 1) > >> + eth1_node = eth_node; > > > > and if reg == 4 > > > > Or reg 0 appears twice? > > In both of the cases that you mentioned, the device tree schema check > will fail, hence, we can safely assume that this will be 0 and 1 only. Nothing forces you to run the scheme checker. It is not run by the kernel before it starts accessing the DT blob. You should assume it is invalid until you have proven it to be valid. Andrew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver 2022-05-09 12:32 ` Andrew Lunn @ 2022-05-16 5:09 ` Puranjay Mohan 0 siblings, 0 replies; 9+ messages in thread From: Puranjay Mohan @ 2022-05-16 5:09 UTC (permalink / raw) To: Andrew Lunn Cc: linux-kernel, davem, edumazet, krzysztof.kozlowski+dt, netdev, devicetree, nm, ssantosh, s-anna, linux-arm-kernel, rogerq, grygorii.strashko, vigneshr, kishon, robh+dt, afd Hi Andrew, On 09/05/22 18:02, Andrew Lunn wrote: >>>> +static void icssg_init_emac_mode(struct prueth *prueth) >>>> +{ >>>> + u8 mac[ETH_ALEN] = { 0 }; >>>> + >>>> + if (prueth->emacs_initialized) >>>> + return; >>>> + >>>> + regmap_update_bits(prueth->miig_rt, FDB_GEN_CFG1, SMEM_VLAN_OFFSET_MASK, 0); >>>> + regmap_write(prueth->miig_rt, FDB_GEN_CFG2, 0); >>>> + /* Clear host MAC address */ >>>> + icssg_class_set_host_mac_addr(prueth->miig_rt, mac); >>> >>> Seems an odd thing to do, set it to 00:00:00:00:00:00. You probably >>> want to add a comment why you do this odd thing. >> >> Actually, this is when the device is configured as a bridge, the host >> mac address has to be set to zero to while bringing it back to emac >> mode. I will add a comment to explain this. > > I don't see any switchdev interface. How does it get into bridge mode? I will be sending patches to add the switch mode support after this series gets merged. > >>>> + } else if (emac->link) { >>>> + new_state = true; >>>> + emac->link = 0; >>>> + /* defaults for no link */ >>>> + >>>> + /* f/w should support 100 & 1000 */ >>>> + emac->speed = SPEED_1000; >>>> + >>>> + /* half duplex may not supported by f/w */ >>>> + emac->duplex = DUPLEX_FULL; >>> >>> Why set speed and duplex when you have just lost the link? They are >>> meaningless until the link comes back. >> >> These were just the default values that we added. >> What do you suggest I put here? > > Nothing. If the link is down, they are meaningless. If something is > accessing them when the link is down, that code is broken. So i > suppose you could give them poison values to help find your broken > code. Okay, I will remove it in next version. > >>>> + for_each_child_of_node(eth_ports_node, eth_node) { >>>> + u32 reg; >>>> + >>>> + if (strcmp(eth_node->name, "port")) >>>> + continue; >>>> + ret = of_property_read_u32(eth_node, "reg", ®); >>>> + if (ret < 0) { >>>> + dev_err(dev, "%pOF error reading port_id %d\n", >>>> + eth_node, ret); >>>> + } >>>> + >>>> + if (reg == 0) >>>> + eth0_node = eth_node; >>>> + else if (reg == 1) >>>> + eth1_node = eth_node; >>> >>> and if reg == 4 >>> >>> Or reg 0 appears twice? >> >> In both of the cases that you mentioned, the device tree schema check >> will fail, hence, we can safely assume that this will be 0 and 1 only. > > Nothing forces you to run the scheme checker. It is not run by the > kernel before it starts accessing the DT blob. You should assume it is > invalid until you have proven it to be valid. I will add error checking here to make sure it is handled. > > Andrew Thanks, Puranjay Mohan _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver [not found] ` <20220506052433.28087-3-p-mohan@ti.com> 2022-05-06 16:44 ` [PATCH 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver Andrew Lunn @ 2022-05-06 16:49 ` Andrew Lunn 1 sibling, 0 replies; 9+ messages in thread From: Andrew Lunn @ 2022-05-06 16:49 UTC (permalink / raw) To: Puranjay Mohan Cc: nm, devicetree, grygorii.strashko, vigneshr, netdev, linux-kernel, kishon, rogerq, afd, edumazet, robh+dt, krzysztof.kozlowski+dt, ssantosh, davem, linux-arm-kernel > +static int prueth_config_rgmiidelay(struct prueth *prueth, > + struct device_node *eth_np, > + phy_interface_t phy_if) > +{ > + struct device *dev = prueth->dev; > + struct regmap *ctrl_mmr; > + u32 rgmii_tx_id = 0; > + u32 icssgctrl_reg; > + > + if (!phy_interface_mode_is_rgmii(phy_if)) > + return 0; > + > + ctrl_mmr = syscon_regmap_lookup_by_phandle(eth_np, "ti,syscon-rgmii-delay"); > + if (IS_ERR(ctrl_mmr)) { > + dev_err(dev, "couldn't get ti,syscon-rgmii-delay\n"); > + return -ENODEV; > + } > + > + if (of_property_read_u32_index(eth_np, "ti,syscon-rgmii-delay", 1, > + &icssgctrl_reg)) { > + dev_err(dev, "couldn't get ti,rgmii-delay reg. offset\n"); > + return -ENODEV; > + } > + > + 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); I know we discussed this before. Why are you adding a delay here in the MAC? If you do add the delay here, you need to mask what you pass to phy_connect(), otherwise the PHY is also going to add a delay for "id" and "txid". In general, it is best to leave all delays to the PHY. Andrew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-05-16 5:11 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-05-06 5:24 [PATCH 0/2] Introduce ICSSG based ethernet Driver Puranjay Mohan
2022-05-06 5:24 ` [PATCH 1/2] dt-bindings: net: Add ICSSG Ethernet Driver bindings Puranjay Mohan
2022-05-06 13:37 ` Rob Herring
2022-05-10 18:07 ` Rob Herring
[not found] ` <20220506052433.28087-3-p-mohan@ti.com>
2022-05-06 16:44 ` [PATCH 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver Andrew Lunn
2022-05-09 10:20 ` Puranjay Mohan
2022-05-09 12:32 ` Andrew Lunn
2022-05-16 5:09 ` Puranjay Mohan
2022-05-06 16:49 ` Andrew Lunn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).