| Message ID | 20250414140128.390400-3-lukma@denx.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | net: mtip: Add support for MTIP imx287 L2 switch driver | expand |
Hi Lukasz, Am 14.04.25 um 16:01 schrieb Lukasz Majewski: > The current range of 'reg' property is too small to allow full control > of the L2 switch on imx287. > > As this IP block also uses ENET-MAC blocks for its operation, the address > range for it must be included as well. > > Moreover, some SoC common properties (like compatible, clocks, interrupts > numbers) have been moved to this node. > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > --- > Changes for v2: > - adding extra properties (like compatible, clocks, interupts) > > Changes for v3: > - None > > Changes for v4: > - Rename imx287 with imx28 (as the former is not used in kernel anymore) > > Changes for v5: > - None > --- > arch/arm/boot/dts/nxp/mxs/imx28.dtsi | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/boot/dts/nxp/mxs/imx28.dtsi b/arch/arm/boot/dts/nxp/mxs/imx28.dtsi > index bbea8b77386f..a0b565ffc83d 100644 > --- a/arch/arm/boot/dts/nxp/mxs/imx28.dtsi > +++ b/arch/arm/boot/dts/nxp/mxs/imx28.dtsi > @@ -1321,8 +1321,12 @@ mac1: ethernet@800f4000 { > status = "disabled"; > }; > > - eth_switch: switch@800f8000 { > - reg = <0x800f8000 0x8000>; > + eth_switch: switch@800f0000 { > + compatible = "nxp,imx28-mtip-switch"; > + reg = <0x800f0000 0x20000>; > + interrupts = <100>, <101>, <102>; > + clocks = <&clks 57>, <&clks 57>, <&clks 64>, <&clks 35>; > + clock-names = "ipg", "ahb", "enet_out", "ptp"; > status = "disabled"; from my understanding of device tree this file should describe the hardware, not the software implementation. After this change the switch memory region overlaps the existing mac0 and mac1 nodes. Definition in the i.MX28 reference manual: ENET MAC0 ENET 0x800F0000 - 0x800F3FFF 16KB ENET MAC1 ENET 0x800F4000 - 0x800F7FFF 16KB ENT Switch SWITCH 0x800F8000 - 0x800FFFFF 32KB I'm not the expert how to solve this properly. Maybe two node references to mac0 and mac1 under eth_switch in order to allocate the memory regions separately. Sorry, if i missed a possible discussion about this before. Regards > }; > };
> > - eth_switch: switch@800f8000 { > > - reg = <0x800f8000 0x8000>; > > + eth_switch: switch@800f0000 { > > + compatible = "nxp,imx28-mtip-switch"; > > + reg = <0x800f0000 0x20000>; > > + interrupts = <100>, <101>, <102>; > > + clocks = <&clks 57>, <&clks 57>, <&clks 64>, <&clks 35>; > > + clock-names = "ipg", "ahb", "enet_out", "ptp"; > > status = "disabled"; > from my understanding of device tree this file should describe the hardware, > not the software implementation. After this change the switch memory region > overlaps the existing mac0 and mac1 nodes. > > Definition in the i.MX28 reference manual: > ENET MAC0 ENET 0x800F0000 - 0x800F3FFF 16KB > ENET MAC1 ENET 0x800F4000 - 0x800F7FFF 16KB > ENT Switch SWITCH 0x800F8000 - 0x800FFFFF 32KB > > I'm not the expert how to solve this properly. Maybe two node references to > mac0 and mac1 under eth_switch in order to allocate the memory regions > separately. I get what you are saying about describing the hardware, but... The hardware can be used in two different ways. 1) Two FEC devices, and the switch it left unused. For this, it makes sense that each FEC has its own memory range, there are two entries, and each has a compatible, since there are two devices. 2) A switch and MAC conglomerate device, which makes use of all three blocks in a single driver. The three hardware blocks have to be used as one consistent whole, by a single driver. There is one compatible for the whole. Given the ranges are contiguous, it makes little sense to map them individually, it would just make the driver needlessly more complex. It should also be noted that 1) and 2) are mutually exclusive, so i don't think it matters the address ranges overlap. Bad things are going to happen independent of this if you enable both at once. Andrew
Hi Andrew, Am 16.04.25 um 23:58 schrieb Andrew Lunn: >>> - eth_switch: switch@800f8000 { >>> - reg = <0x800f8000 0x8000>; >>> + eth_switch: switch@800f0000 { >>> + compatible = "nxp,imx28-mtip-switch"; >>> + reg = <0x800f0000 0x20000>; >>> + interrupts = <100>, <101>, <102>; >>> + clocks = <&clks 57>, <&clks 57>, <&clks 64>, <&clks 35>; >>> + clock-names = "ipg", "ahb", "enet_out", "ptp"; >>> status = "disabled"; >> from my understanding of device tree this file should describe the hardware, >> not the software implementation. After this change the switch memory region >> overlaps the existing mac0 and mac1 nodes. >> >> Definition in the i.MX28 reference manual: >> ENET MAC0 ENET 0x800F0000 - 0x800F3FFF 16KB >> ENET MAC1 ENET 0x800F4000 - 0x800F7FFF 16KB >> ENT Switch SWITCH 0x800F8000 - 0x800FFFFF 32KB >> >> I'm not the expert how to solve this properly. Maybe two node references to >> mac0 and mac1 under eth_switch in order to allocate the memory regions >> separately. > I get what you are saying about describing the hardware, but... > > The hardware can be used in two different ways. > > 1) Two FEC devices, and the switch it left unused. > > For this, it makes sense that each FEC has its own memory range, there > are two entries, and each has a compatible, since there are two > devices. > > 2) A switch and MAC conglomerate device, which makes use of all three > blocks in a single driver. > > The three hardware blocks have to be used as one consistent whole, by > a single driver. There is one compatible for the whole. Given the > ranges are contiguous, it makes little sense to map them individually, > it would just make the driver needlessly more complex. > > It should also be noted that 1) and 2) are mutually exclusive, so i > don't think it matters the address ranges overlap. Bad things are > going to happen independent of this if you enable both at once. > > Andrew i wasn't aware how critical possible overlapping memory regions are. I was just surprised because it wasn't mention in the commit message. As long as everyone is fine with this approach, please ignore my last comment. Regards
Hi Andrew, > > > - eth_switch: switch@800f8000 { > > > - reg = <0x800f8000 0x8000>; > > > + eth_switch: switch@800f0000 { > > > + compatible = "nxp,imx28-mtip-switch"; > > > + reg = <0x800f0000 0x20000>; > > > + interrupts = <100>, <101>, <102>; > > > + clocks = <&clks 57>, <&clks 57>, <&clks > > > 64>, <&clks 35>; > > > + clock-names = "ipg", "ahb", "enet_out", > > > "ptp"; status = "disabled"; > > from my understanding of device tree this file should describe the > > hardware, not the software implementation. After this change the > > switch memory region overlaps the existing mac0 and mac1 nodes. > > > > Definition in the i.MX28 reference manual: > > ENET MAC0 ENET 0x800F0000 - 0x800F3FFF 16KB > > ENET MAC1 ENET 0x800F4000 - 0x800F7FFF 16KB > > ENT Switch SWITCH 0x800F8000 - 0x800FFFFF 32KB > > > > I'm not the expert how to solve this properly. Maybe two node > > references to mac0 and mac1 under eth_switch in order to allocate > > the memory regions separately. > > I get what you are saying about describing the hardware, but... > > The hardware can be used in two different ways. > > 1) Two FEC devices, and the switch it left unused. > > For this, it makes sense that each FEC has its own memory range, there > are two entries, and each has a compatible, since there are two > devices. > > 2) A switch and MAC conglomerate device, which makes use of all three > blocks in a single driver. > > The three hardware blocks have to be used as one consistent whole, by > a single driver. There is one compatible for the whole. Given the > ranges are contiguous, it makes little sense to map them individually, > it would just make the driver needlessly more complex. > > It should also be noted that 1) and 2) are mutually exclusive, so i > don't think it matters the address ranges overlap. Bad things are > going to happen independent of this if you enable both at once. > +1 > Andrew Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Stefan, > Hi Andrew, > > Am 16.04.25 um 23:58 schrieb Andrew Lunn: > >>> - eth_switch: switch@800f8000 { > >>> - reg = <0x800f8000 0x8000>; > >>> + eth_switch: switch@800f0000 { > >>> + compatible = "nxp,imx28-mtip-switch"; > >>> + reg = <0x800f0000 0x20000>; > >>> + interrupts = <100>, <101>, <102>; > >>> + clocks = <&clks 57>, <&clks 57>, <&clks > >>> 64>, <&clks 35>; > >>> + clock-names = "ipg", "ahb", "enet_out", > >>> "ptp"; status = "disabled"; > >> from my understanding of device tree this file should describe the > >> hardware, not the software implementation. After this change the > >> switch memory region overlaps the existing mac0 and mac1 nodes. > >> > >> Definition in the i.MX28 reference manual: > >> ENET MAC0 ENET 0x800F0000 - 0x800F3FFF 16KB > >> ENET MAC1 ENET 0x800F4000 - 0x800F7FFF 16KB > >> ENT Switch SWITCH 0x800F8000 - 0x800FFFFF 32KB > >> > >> I'm not the expert how to solve this properly. Maybe two node > >> references to mac0 and mac1 under eth_switch in order to allocate > >> the memory regions separately. > > I get what you are saying about describing the hardware, but... > > > > The hardware can be used in two different ways. > > > > 1) Two FEC devices, and the switch it left unused. > > > > For this, it makes sense that each FEC has its own memory range, > > there are two entries, and each has a compatible, since there are > > two devices. > > > > 2) A switch and MAC conglomerate device, which makes use of all > > three blocks in a single driver. > > > > The three hardware blocks have to be used as one consistent whole, > > by a single driver. There is one compatible for the whole. Given the > > ranges are contiguous, it makes little sense to map them > > individually, it would just make the driver needlessly more complex. > > > > It should also be noted that 1) and 2) are mutually exclusive, so i > > don't think it matters the address ranges overlap. Bad things are > > going to happen independent of this if you enable both at once. > > > > Andrew > i wasn't aware how critical possible overlapping memory regions are. > I was just surprised because it wasn't mention in the commit message. > As long as everyone is fine with this approach, please ignore my last > comment. > Just for the record - there was an attempt to "re-use" FEC enet driver in switch [1], but this approach has been rejected as one not very robust and "clear by design" (i.e. similar to cpsw_new.c driver). And I do agree with Andrew - that approach presented in this patch set is the correct one. Links: [1] - https://source.denx.de/linux/linux-imx28-l2switch/-/commits/imx28-v5.12-L2-upstream-switchdev-RFC_v1 > Regards > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
diff --git a/arch/arm/boot/dts/nxp/mxs/imx28.dtsi b/arch/arm/boot/dts/nxp/mxs/imx28.dtsi index bbea8b77386f..a0b565ffc83d 100644 --- a/arch/arm/boot/dts/nxp/mxs/imx28.dtsi +++ b/arch/arm/boot/dts/nxp/mxs/imx28.dtsi @@ -1321,8 +1321,12 @@ mac1: ethernet@800f4000 { status = "disabled"; }; - eth_switch: switch@800f8000 { - reg = <0x800f8000 0x8000>; + eth_switch: switch@800f0000 { + compatible = "nxp,imx28-mtip-switch"; + reg = <0x800f0000 0x20000>; + interrupts = <100>, <101>, <102>; + clocks = <&clks 57>, <&clks 57>, <&clks 64>, <&clks 35>; + clock-names = "ipg", "ahb", "enet_out", "ptp"; status = "disabled"; }; };