| Message ID | 20220504044624.951841-1-chris.packham@alliedtelesis.co.nz (mailing list archive) |
|---|---|
| Headers | show |
| Series | arm64: mvebu: Support for Marvell 98DX2530 (and variants) | expand |
Hi Chris, > arch/arm64/boot/dts/marvell/Makefile | 1 + > .../boot/dts/marvell/armada-98dx2530.dtsi | 310 ++++++++++++++++++ > arch/arm64/boot/dts/marvell/rd-ac5x.dts | 90 +++++ > 3 files changed, 401 insertions(+) Marvell is going to start the upstreaming of AC5X boards support, we have also patches with similar .dts(i) files but with different naming: ac5.dtsi ac5_rd.dts ac5_db.dts ac5x_db.dts What do you think about to use these naming scheme ? Regards, Vadym Kochan
On Wed, May 11, 2022 at 07:10:03PM +0300, Vadym Kochan wrote: > Hi Chris, > > > arch/arm64/boot/dts/marvell/Makefile | 1 + > > .../boot/dts/marvell/armada-98dx2530.dtsi | 310 ++++++++++++++++++ > > arch/arm64/boot/dts/marvell/rd-ac5x.dts | 90 +++++ > > 3 files changed, 401 insertions(+) > > Marvell is going to start the upstreaming of AC5X boards support, we have also patches with similar .dts(i) files > but with different naming: > > ac5.dtsi > ac5_rd.dts > ac5_db.dts > ac5x_db.dts > > What do you think about to use these naming scheme ? Chris has done all the hard work, he gets to pick the naming. And get his files merged first. However, now that i come to look in arch/arm64/boot/dts/marvell, i think most of the current filenames proposed don't match the current names. armada-98dx2530.dtsi fits the current pattern. However, Chris's board files should probably be armada-98dx2530-rd.dts and the other files should be armada-98dx2530-db.dts armada-98dx2530-x-db.dts What does the x in x_db mean? Does that refer to the board or the SoC? Andrew
On 12/05/22 04:20, Andrew Lunn wrote: > On Wed, May 11, 2022 at 07:10:03PM +0300, Vadym Kochan wrote: >> Hi Chris, >> >>> arch/arm64/boot/dts/marvell/Makefile | 1 + >>> .../boot/dts/marvell/armada-98dx2530.dtsi | 310 ++++++++++++++++++ >>> arch/arm64/boot/dts/marvell/rd-ac5x.dts | 90 +++++ >>> 3 files changed, 401 insertions(+) >> Marvell is going to start the upstreaming of AC5X boards support, That's good news. I'm probably the customer that's been nagging the Marvell support team. But I'm also impatient hence I started working on this series. The pinctrl and mvneta changes have already been accepted. >> we have also patches with similar .dts(i) files >> but with different naming: >> >> ac5.dtsi >> ac5_rd.dts >> ac5_db.dts >> ac5x_db.dts >> >> What do you think about to use these naming scheme ? Personally I thought they'd be rejected upstream as being too vague and generic. I settled on armada-98dx2530 as I saw the 98dx2530 name used on the Marvell Portal to refer to the CnM block for the AC5/AC5X. I was going to call the board file "rd-ac5x-32g16hvg6hlg.dts" as that's what the silkscreen on the board I have says but I shortened it to "rd-ac5x" as the switch port configuration is largely irrelevant to the board support I'm trying to get landed. > Chris has done all the hard work, he gets to pick the naming. And get > his files merged first. I'm not against changing if there is a consensus. On another thread the idea of armada-98dx25xx/armada-98dx35xx was mentioned. That might be a reasonable compromise (although technically there's no difference in the CPU block between the 25xx and 35xx). > However, now that i come to look in arch/arm64/boot/dts/marvell, i > think most of the current filenames proposed don't match the current names. > > armada-98dx2530.dtsi fits the current pattern. > > However, Chris's board files should probably be > > armada-98dx2530-rd.dts > > and the other files should be > > armada-98dx2530-db.dts > > armada-98dx2530-x-db.dts > > What does the x in x_db mean? Does that refer to the board or the SoC? The x is from AC5X so we'd actually have armada-98dx25xx-db.dts and armada-98dx35xx-db.dts. My board would be called armada-98dx35xx-rd.dts or perhaps armada-98dx3550-rd.dts.
On Wed, May 11, 2022 at 10:59:37PM +0000, Chris Packham wrote: > > On 12/05/22 04:20, Andrew Lunn wrote: > > On Wed, May 11, 2022 at 07:10:03PM +0300, Vadym Kochan wrote: > >> Hi Chris, > >> > >>> arch/arm64/boot/dts/marvell/Makefile | 1 + > >>> .../boot/dts/marvell/armada-98dx2530.dtsi | 310 ++++++++++++++++++ > >>> arch/arm64/boot/dts/marvell/rd-ac5x.dts | 90 +++++ > >>> 3 files changed, 401 insertions(+) > >> Marvell is going to start the upstreaming of AC5X boards support, > That's good news. I'm probably the customer that's been nagging the > Marvell support team. But I'm also impatient hence I started working on > this series. The pinctrl and mvneta changes have already been accepted. > >> we have also patches with similar .dts(i) files > >> but with different naming: > >> > >> ac5.dtsi > >> ac5_rd.dts > >> ac5_db.dts > >> ac5x_db.dts > >> > >> What do you think about to use these naming scheme ? > > Personally I thought they'd be rejected upstream as being too vague and > generic. Agreed. > I'm not against changing if there is a consensus. On another thread the > idea of armada-98dx25xx/armada-98dx35xx was mentioned. That might be a > reasonable compromise (although technically there's no difference in the > CPU block between the 25xx and 35xx). Until we find there is a difference. Marvell, can you confirm that the switch really is the only difference? This might also come down to ID registers. The DT could be enough to find the ID of the device, the rest is then done in the drivers, not DT. > > However, now that i come to look in arch/arm64/boot/dts/marvell, i > > think most of the current filenames proposed don't match the current names. > > > > armada-98dx2530.dtsi fits the current pattern. > > > > However, Chris's board files should probably be > > > > armada-98dx2530-rd.dts > > > > and the other files should be > > > > armada-98dx2530-db.dts > > > > armada-98dx2530-x-db.dts > > > > What does the x in x_db mean? Does that refer to the board or the SoC? > > The x is from AC5X so we'd actually have armada-98dx25xx-db.dts and > armada-98dx35xx-db.dts. My board would be called armada-98dx35xx-rd.dts > or perhaps armada-98dx3550-rd.dts. armada-98dx25xx and armada-98dx35xx does help with the naming. So it probably is worth having armada-98dx35xx which simply includes armada-98d25xx. Andrew
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Thursday, May 12, 2022 3:40 AM > To: Chris Packham <Chris.Packham@alliedtelesis.co.nz> > Cc: Vadym Kochan <vadym.kochan@plvision.eu>; > catalin.marinas@arm.com; will@kernel.org; robh+dt@kernel.org; > krzysztof.kozlowski+dt@linaro.org; gregory.clement@bootlin.com; > sebastian.hesselbarth@gmail.com; Kostya Porotchkin > <kostap@marvell.com>; robert.marko@sartura.hr; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org; Elad Nachman <enachman@marvell.com> > Subject: [EXT] Re: [PATCH v5 1/2] arm64: dts: marvell: Add Armada 98DX2530 > SoC and RD-AC5X board > > External Email > > ---------------------------------------------------------------------- > On Wed, May 11, 2022 at 10:59:37PM +0000, Chris Packham wrote: > > > > On 12/05/22 04:20, Andrew Lunn wrote: > > > On Wed, May 11, 2022 at 07:10:03PM +0300, Vadym Kochan wrote: > > >> Hi Chris, > > >> > > >>> arch/arm64/boot/dts/marvell/Makefile | 1 + > > >>> .../boot/dts/marvell/armada-98dx2530.dtsi | 310 > ++++++++++++++++++ > > >>> arch/arm64/boot/dts/marvell/rd-ac5x.dts | 90 +++++ > > >>> 3 files changed, 401 insertions(+) > > >> Marvell is going to start the upstreaming of AC5X boards support, > > That's good news. I'm probably the customer that's been nagging the > > Marvell support team. But I'm also impatient hence I started working > > on this series. The pinctrl and mvneta changes have already been accepted. > > >> we have also patches with similar .dts(i) files but with > > >> different naming: > > >> > > >> ac5.dtsi > > >> ac5_rd.dts > > >> ac5_db.dts > > >> ac5x_db.dts > > >> > > >> What do you think about to use these naming scheme ? > > > > Personally I thought they'd be rejected upstream as being too vague > > and generic. > > Agreed. > > > I'm not against changing if there is a consensus. On another thread > > the idea of armada-98dx25xx/armada-98dx35xx was mentioned. That > might > > be a reasonable compromise (although technically there's no difference > > in the CPU block between the 25xx and 35xx). > > Until we find there is a difference. Marvell, can you confirm that the switch > really is the only difference? Basically, the cpu-subsystems of Prestera 98DX25xx (AC5) and Prestera 98DX35xx (AC5X) are the same. There is a very small difference in how the default memory map starts after boot, which is reconfigurable in u-boot. This affects the switch (not part of the device tree anyway) and the PCIe. The PCIe window still overlap partially between AC5 and AC5X, however. The original Marvell DTSI overcomes this by defining only the PCIe overlapping part in the device tree, resulting in a single device tree which works on both AC5 and AC5X. The DTSI Chris proposed had the PCIe portion removed. We have PCIe support for AC5/AC5X so we would obviously like to include this portion in both the DTSI and as a patch to the Armada8K PCIe driver. > > This might also come down to ID registers. The DT could be enough to find > the ID of the device, the rest is then done in the drivers, not DT. > > > > However, now that i come to look in arch/arm64/boot/dts/marvell, i > > > think most of the current filenames proposed don't match the current > names. > > > > > > armada-98dx2530.dtsi fits the current pattern. > > > > > > However, Chris's board files should probably be > > > > > > armada-98dx2530-rd.dts > > > > > > and the other files should be > > > > > > armada-98dx2530-db.dts > > > > > > armada-98dx2530-x-db.dts > > > > > > What does the x in x_db mean? Does that refer to the board or the SoC? > > > > The x is from AC5X so we'd actually have armada-98dx25xx-db.dts and > > armada-98dx35xx-db.dts. My board would be called > > armada-98dx35xx-rd.dts or perhaps armada-98dx3550-rd.dts. > > armada-98dx25xx and armada-98dx35xx does help with the naming. So it > probably is worth having armada-98dx35xx which simply includes armada- > 98d25xx. > > Andrew Elad.
> Basically, the cpu-subsystems of Prestera 98DX25xx (AC5) and Prestera 98DX35xx (AC5X) are the same. Great, thanks for the conformation. > The DTSI Chris proposed had the PCIe portion removed. > We have PCIe support for AC5/AC5X so we would obviously like to > include this portion in both the DTSI and as a patch to the Armada8K > PCIe driver. So you can add the needed node to the .dtsi as part of the patch to the pci-aardvark.c driver. That sounds O.K. Andrew
On 13/05/22 00:47, Andrew Lunn wrote: >> Basically, the cpu-subsystems of Prestera 98DX25xx (AC5) and Prestera 98DX35xx (AC5X) are the same. > Great, thanks for the conformation. > >> The DTSI Chris proposed had the PCIe portion removed. >> We have PCIe support for AC5/AC5X so we would obviously like to >> include this portion in both the DTSI and as a patch to the Armada8K >> PCIe driver. > So you can add the needed node to the .dtsi as part of the patch to > the pci-aardvark.c driver. That sounds O.K. I deliberately left the pci stuff out of my submission to keep things simple and because I have no way of testing it. Eventually we (allied telesis) are planning a product variant that will need a 2nd switch connected via pci so I'll be revisiting the pci stuff then (unless someone else beats me to it).