| Message ID | 20220427162123.110458-1-maukka@ext.kapsi.fi |
|---|---|
| Headers | show |
| Series | ARM: orion5x: convert D-Link DNS-323 to the Device Tree | expand |
On Wed, Apr 27, 2022 at 6:21 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: > > Hello all, > > I am making an attempt to create a device tree for D-Link DNS-323 devices > but I am falling short on a few specific details. I am requesting a > general review of the device tree files. I have access to DNS-323 rev A1 > only and the B1 and C1 need to be tested separately, so I am reaching out > to people who might have them. Hi Mauri, It's really nice to see some progress on this! I don't have the hardware, but I'll try to answer some of your questions anyway. > The questions that I have at the moment are below. > > - some of resulting IRQs are different from what was requested in device tree > - logs say NR_IRQS is different from mach file one This is all normal: with a board file, all on-board IRQs are statically assigned to fixed numbers. With DT based boot, IRQ controllers usually define their own IRQ domains, which get a number space assigned according to probe order, and above the preallocated IRQ numbers. > - sata_mv fails to initialise with -22 (-EINVAL) No idea, I'd try inserting a printk in every code path that can return -EINVAL from there > - there is no concensus on how to get ascii formatted MAC address from mtd > partitions so eth is not fully functional without setting the MAC from > userspace Ideally this is handled by the boot loader, but that requires being able to update it. If you cannot, this could perhaps be done using something like https://github.com/zonque/pxa-impedance-matcher Arnd
On Wed, Apr 27, 2022 at 6:21 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: > > Convert D-Link DNS-323 to use the device tree and remove associated > mach file. > > Signed-off-by: Mauri Sandberg <maukka@ext.kapsi.fi> > --- > arch/arm/boot/dts/Makefile | 3 + > arch/arm/boot/dts/orion5x-dlink-dns323.dtsi | 217 ++++++ > arch/arm/boot/dts/orion5x-dlink-dns323a1.dts | 59 ++ > arch/arm/boot/dts/orion5x-dlink-dns323b1.dts | 38 + > arch/arm/boot/dts/orion5x-dlink-dns323c1.dts | 80 ++ > arch/arm/mach-orion5x/Kconfig | 6 +- > arch/arm/mach-orion5x/Makefile | 2 +- > arch/arm/mach-orion5x/board-dns323.c | 118 +++ > arch/arm/mach-orion5x/board-dt.c | 3 + > arch/arm/mach-orion5x/common.h | 6 + > arch/arm/mach-orion5x/dns323-setup.c | 724 ------------------- > 11 files changed, 528 insertions(+), 728 deletions(-) > create mode 100644 arch/arm/boot/dts/orion5x-dlink-dns323.dtsi > create mode 100644 arch/arm/boot/dts/orion5x-dlink-dns323a1.dts > create mode 100644 arch/arm/boot/dts/orion5x-dlink-dns323b1.dts > create mode 100644 arch/arm/boot/dts/orion5x-dlink-dns323c1.dts > create mode 100644 arch/arm/mach-orion5x/board-dns323.c > delete mode 100644 arch/arm/mach-orion5x/dns323-setup.c Having the combined patch is generally fine for review, but for actually merging it, I would prefer splitting the new DT from the code removal, as I send these through different trees to Linus. Arnd
> > - there is no concensus on how to get ascii formatted MAC address from mtd > > partitions so eth is not fully functional without setting the MAC from > > userspace > > Ideally this is handled by the boot loader, but that requires being > able to update > it. The mv643xx Ethernet driver is happy if it finds the MAC address already in the hardware. The vendor uboot often does this. Does tftp boot work in uboot? That would indicate it has access to the MAC address. Andrew
> - how to configure RTC to wake system from sleep?
The st,m41t80 binding document has:
Optional properties:
- interrupts: rtc alarm interrupt.
- clock-output-names: From common clock binding to override the default output
clock name
- wakeup-source: Enables wake up of host system on alarm
which you don't appear to have in your DT files.
Andrew
On 27/04/2022 18:21, Mauri Sandberg wrote: > Convert D-Link DNS-323 to use the device tree and remove associated > mach file. > Thank you for your patch. There is something to discuss/improve. > Signed-off-by: Mauri Sandberg <maukka@ext.kapsi.fi> > --- > arch/arm/boot/dts/Makefile | 3 + > arch/arm/boot/dts/orion5x-dlink-dns323.dtsi | 217 ++++++ > arch/arm/boot/dts/orion5x-dlink-dns323a1.dts | 59 ++ > arch/arm/boot/dts/orion5x-dlink-dns323b1.dts | 38 + > arch/arm/boot/dts/orion5x-dlink-dns323c1.dts | 80 ++ > arch/arm/mach-orion5x/Kconfig | 6 +- > arch/arm/mach-orion5x/Makefile | 2 +- > arch/arm/mach-orion5x/board-dns323.c | 118 +++ > arch/arm/mach-orion5x/board-dt.c | 3 + > arch/arm/mach-orion5x/common.h | 6 + > arch/arm/mach-orion5x/dns323-setup.c | 724 ------------------- > 11 files changed, 528 insertions(+), 728 deletions(-) > create mode 100644 arch/arm/boot/dts/orion5x-dlink-dns323.dtsi > create mode 100644 arch/arm/boot/dts/orion5x-dlink-dns323a1.dts > create mode 100644 arch/arm/boot/dts/orion5x-dlink-dns323b1.dts > create mode 100644 arch/arm/boot/dts/orion5x-dlink-dns323c1.dts > create mode 100644 arch/arm/mach-orion5x/board-dns323.c > delete mode 100644 arch/arm/mach-orion5x/dns323-setup.c DTS goes separately. > > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile > index 7c16f8a2b738..c7c5c0b6c843 100644 > --- a/arch/arm/boot/dts/Makefile > +++ b/arch/arm/boot/dts/Makefile > @@ -949,6 +949,9 @@ dtb-$(CONFIG_SOC_DRA7XX) += \ > dra71-evm.dtb \ > dra76-evm.dtb > dtb-$(CONFIG_ARCH_ORION5X) += \ > + orion5x-dlink-dns323a1.dtb \ > + orion5x-dlink-dns323b1.dtb \ > + orion5x-dlink-dns323c1.dtb \ > orion5x-kuroboxpro.dtb \ > orion5x-lacie-d2-network.dtb \ > orion5x-lacie-ethernet-disk-mini-v2.dtb \ > diff --git a/arch/arm/boot/dts/orion5x-dlink-dns323.dtsi b/arch/arm/boot/dts/orion5x-dlink-dns323.dtsi > new file mode 100644 > index 000000000000..2b033d37cbf8 > --- /dev/null > +++ b/arch/arm/boot/dts/orion5x-dlink-dns323.dtsi > @@ -0,0 +1,217 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2022 Mauri Sandberg <maukka@ext.kapsi.fi> > + * > + */ > + > +/ { > + model = "D-Link DNS-323"; > + compatible = "dlink,dns323", "marvell,orion5x"; > You need to also document the compatibles in bindings file for boards. It would be nice to convert Documentation/devicetree/bindings/arm/marvell/marvell,orion5x.txt to DT Schema (see armada-7k-8k.yaml for example), but it is not a requirement. Best regards, Krzysztof
Hi Arnd and thanks for your quick reply. On 27.4.2022 21.10, Arnd Bergmann wrote: > On Wed, Apr 27, 2022 at 6:21 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: >> >> Hello all, >> >> I am making an attempt to create a device tree for D-Link DNS-323 devices >> but I am falling short on a few specific details. I am requesting a >> general review of the device tree files. I have access to DNS-323 rev A1 >> only and the B1 and C1 need to be tested separately, so I am reaching out >> to people who might have them. > > Hi Mauri, > > It's really nice to see some progress on this! I don't have the hardware, > but I'll try to answer some of your questions anyway. > >> The questions that I have at the moment are below. >> >> - some of resulting IRQs are different from what was requested in device tree >> - logs say NR_IRQS is different from mach file one > Ok thanks, I won't worry about it anymore. > This is all normal: with a board file, all on-board IRQs are statically > assigned to fixed numbers. With DT based boot, IRQ controllers > usually define their own IRQ domains, which get a number space assigned > according to probe order, and above the preallocated IRQ numbers. > >> - sata_mv fails to initialise with -22 (-EINVAL) > > No idea, I'd try inserting a printk in every code path that can return -EINVAL > from there > I had something like that but I didn't get any wiser immediately. Then I suspected it's something to do with initialisation of the PCIe bus and that clashing with sata_mv initialisation and thought it's better to ask. The PCIe initialisation uses hardwired irq and maybe that was getting in the way. Is there a way to describe the PCIe bus in the device tree? The initalisation of that bus is done for rev A1 only. >> - there is no concensus on how to get ascii formatted MAC address from mtd >> partitions so eth is not fully functional without setting the MAC from >> userspace > > Ideally this is handled by the boot loader, but that requires being > able to update > it. If you cannot, this could perhaps be done using something like > https://github.com/zonque/pxa-impedance-matcher I had a look at the pxa-impedance-matcher but I am not sure how to use it. Should it be flashed on the device and then it would the boot the rest of the system? Maybe I'll have to add some dns323 specifics there too first. On the dns323 there are these mtd partitions MTD1 and MTD2, which I am not really sure what they are for. Maybe those could accommodate a 3rd stage loader. But I'll consider it as my last resort as they put it in their documentation too. In linux-mtd there's been a few attempts to find a solution and I am hoping one will be found eventually. Adding support in the u-boot was stalled back in the days for some reason and I am not sure I will be much wiser than the previous people that were at it. But I have jtag programmer that should be suitable. -- Mauri
On 27.4.2022 21.12, Arnd Bergmann wrote: > On Wed, Apr 27, 2022 at 6:21 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: >> >> Convert D-Link DNS-323 to use the device tree and remove associated >> mach file. >> >> Signed-off-by: Mauri Sandberg <maukka@ext.kapsi.fi> >> --- >> arch/arm/boot/dts/Makefile | 3 + >> arch/arm/boot/dts/orion5x-dlink-dns323.dtsi | 217 ++++++ >> arch/arm/boot/dts/orion5x-dlink-dns323a1.dts | 59 ++ >> arch/arm/boot/dts/orion5x-dlink-dns323b1.dts | 38 + >> arch/arm/boot/dts/orion5x-dlink-dns323c1.dts | 80 ++ >> arch/arm/mach-orion5x/Kconfig | 6 +- >> arch/arm/mach-orion5x/Makefile | 2 +- >> arch/arm/mach-orion5x/board-dns323.c | 118 +++ >> arch/arm/mach-orion5x/board-dt.c | 3 + >> arch/arm/mach-orion5x/common.h | 6 + >> arch/arm/mach-orion5x/dns323-setup.c | 724 ------------------- >> 11 files changed, 528 insertions(+), 728 deletions(-) >> create mode 100644 arch/arm/boot/dts/orion5x-dlink-dns323.dtsi >> create mode 100644 arch/arm/boot/dts/orion5x-dlink-dns323a1.dts >> create mode 100644 arch/arm/boot/dts/orion5x-dlink-dns323b1.dts >> create mode 100644 arch/arm/boot/dts/orion5x-dlink-dns323c1.dts >> create mode 100644 arch/arm/mach-orion5x/board-dns323.c >> delete mode 100644 arch/arm/mach-orion5x/dns323-setup.c > > Having the combined patch is generally fine for review, but for actually > merging it, I would prefer splitting the new DT from the code removal, > as I send these through different trees to Linus. You know, I had them initially in separate patches but then I looked at the existing commit history and at least a few ones were in a single commit so I merged them. :) Will split this in two in the next iteration.
Hello Krzysztof and thanks for your feedback On 28.4.2022 10.13, Krzysztof Kozlowski wrote: > On 27/04/2022 18:21, Mauri Sandberg wrote: >> Convert D-Link DNS-323 to use the device tree and remove associated >> mach file. >> > > Thank you for your patch. There is something to discuss/improve. > >> Signed-off-by: Mauri Sandberg <maukka@ext.kapsi.fi> >> --- >> arch/arm/boot/dts/Makefile | 3 + >> arch/arm/boot/dts/orion5x-dlink-dns323.dtsi | 217 ++++++ >> arch/arm/boot/dts/orion5x-dlink-dns323a1.dts | 59 ++ >> arch/arm/boot/dts/orion5x-dlink-dns323b1.dts | 38 + >> arch/arm/boot/dts/orion5x-dlink-dns323c1.dts | 80 ++ >> arch/arm/mach-orion5x/Kconfig | 6 +- >> arch/arm/mach-orion5x/Makefile | 2 +- >> arch/arm/mach-orion5x/board-dns323.c | 118 +++ >> arch/arm/mach-orion5x/board-dt.c | 3 + >> arch/arm/mach-orion5x/common.h | 6 + >> arch/arm/mach-orion5x/dns323-setup.c | 724 ------------------- >> 11 files changed, 528 insertions(+), 728 deletions(-) >> create mode 100644 arch/arm/boot/dts/orion5x-dlink-dns323.dtsi >> create mode 100644 arch/arm/boot/dts/orion5x-dlink-dns323a1.dts >> create mode 100644 arch/arm/boot/dts/orion5x-dlink-dns323b1.dts >> create mode 100644 arch/arm/boot/dts/orion5x-dlink-dns323c1.dts >> create mode 100644 arch/arm/mach-orion5x/board-dns323.c >> delete mode 100644 arch/arm/mach-orion5x/dns323-setup.c > > DTS goes separately. Duly noted. :) > >> >> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile >> index 7c16f8a2b738..c7c5c0b6c843 100644 >> --- a/arch/arm/boot/dts/Makefile >> +++ b/arch/arm/boot/dts/Makefile >> @@ -949,6 +949,9 @@ dtb-$(CONFIG_SOC_DRA7XX) += \ >> dra71-evm.dtb \ >> dra76-evm.dtb >> dtb-$(CONFIG_ARCH_ORION5X) += \ >> + orion5x-dlink-dns323a1.dtb \ >> + orion5x-dlink-dns323b1.dtb \ >> + orion5x-dlink-dns323c1.dtb \ >> orion5x-kuroboxpro.dtb \ >> orion5x-lacie-d2-network.dtb \ >> orion5x-lacie-ethernet-disk-mini-v2.dtb \ >> diff --git a/arch/arm/boot/dts/orion5x-dlink-dns323.dtsi b/arch/arm/boot/dts/orion5x-dlink-dns323.dtsi >> new file mode 100644 >> index 000000000000..2b033d37cbf8 >> --- /dev/null >> +++ b/arch/arm/boot/dts/orion5x-dlink-dns323.dtsi >> @@ -0,0 +1,217 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (C) 2022 Mauri Sandberg <maukka@ext.kapsi.fi> >> + * >> + */ >> + >> +/ { >> + model = "D-Link DNS-323"; >> + compatible = "dlink,dns323", "marvell,orion5x"; >> > > You need to also document the compatibles in bindings file for boards. > It would be nice to convert > Documentation/devicetree/bindings/arm/marvell/marvell,orion5x.txt to DT > Schema (see armada-7k-8k.yaml for example), but it is not a requirement. Ah, I hadn't even noticed them before. Will look into this.
Hello Andrew! On 28.4.2022 3.18, Andrew Lunn wrote: >>> - there is no concensus on how to get ascii formatted MAC address from mtd >>> partitions so eth is not fully functional without setting the MAC from >>> userspace >> >> Ideally this is handled by the boot loader, but that requires being >> able to update >> it. > > The mv643xx Ethernet driver is happy if it finds the MAC address > already in the hardware. The vendor uboot often does this. Does tftp > boot work in uboot? That would indicate it has access to the MAC > address. The u-boot is really limited and I am transferring new images over kermit. Tftp is not enabled.
On Thu, Apr 28, 2022 at 10:06 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: > > You know, I had them initially in separate patches but then I looked at > the existing commit history and at least a few ones were in a single > commit so I merged them. :) Just for context, we sometimes do a mass-conversion in a separate branch when there are a lot of changes that touch both DT and source files. If you only do a single board, I would not make that a separate top-level pull request, so it has to fit into the regular branches. Arnd
On Thu, Apr 28, 2022 at 10:01 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: > On 27.4.2022 21.10, Arnd Bergmann wrote: > > On Wed, Apr 27, 2022 at 6:21 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: > > This is all normal: with a board file, all on-board IRQs are statically > > assigned to fixed numbers. With DT based boot, IRQ controllers > > usually define their own IRQ domains, which get a number space assigned > > according to probe order, and above the preallocated IRQ numbers. > > > >> - sata_mv fails to initialise with -22 (-EINVAL) > > > > No idea, I'd try inserting a printk in every code path that can return -EINVAL > > from there > > > I had something like that but I didn't get any wiser immediately. Then I > suspected it's something to do with initialisation of the PCIe bus and > that clashing with sata_mv initialisation and thought it's better to > ask. The PCIe initialisation uses hardwired irq and maybe that was > getting in the way. Is there a way to describe the PCIe bus in the > device tree? The initalisation of that bus is done for rev A1 only. I'm not too familiar with the platform, but my interpretation is that the DT support here is incomplete: The DT based PCI probe using drivers/pci/controller/pci-mvebu.c is not hooked up in orion5x.dtsi, and the traditional pci code does not work with DT. I see that orion5x has two separate blocks -- a PCIe host that is similar to the kirkwood one, and a legacy PCI host that needs a completely separate driver. Which of the two do you actually need here? > >> - there is no concensus on how to get ascii formatted MAC address from mtd > >> partitions so eth is not fully functional without setting the MAC from > >> userspace > > > > Ideally this is handled by the boot loader, but that requires being > > able to update > > it. If you cannot, this could perhaps be done using something like > > https://github.com/zonque/pxa-impedance-matcher > > I had a look at the pxa-impedance-matcher but I am not sure how to use > it. Should it be flashed on the device and then it would the boot the > rest of the system? Maybe I'll have to add some dns323 specifics there > too first. On the dns323 there are these mtd partitions MTD1 and MTD2, > which I am not really sure what they are for. Maybe those could > accommodate a 3rd stage loader. But I'll consider it as my last resort > as they put it in their documentation too. In linux-mtd there's been a > few attempts to find a solution and I am hoping one will be found > eventually. > > Adding support in the u-boot was stalled back in the days for some > reason and I am not sure I will be much wiser than the previous people > that were at it. But I have jtag programmer that should be suitable. I think the idea of the impedance-matcher is that you can combine it with a DT-enabled kernel image into a file that looks to the existing boot loader like an old kernel and then provides both a way for code to run before booting the kernel, and for adding in the DT. Arnd
On Thu, Apr 28, 2022 at 11:25:00PM +0300, Mauri Sandberg wrote: > Hello Andrew! > > On 28.4.2022 3.18, Andrew Lunn wrote: > > > > - there is no concensus on how to get ascii formatted MAC address from mtd > > > > partitions so eth is not fully functional without setting the MAC from > > > > userspace > > > > > > Ideally this is handled by the boot loader, but that requires being > > > able to update > > > it. > > > > The mv643xx Ethernet driver is happy if it finds the MAC address > > already in the hardware. The vendor uboot often does this. Does tftp > > boot work in uboot? That would indicate it has access to the MAC > > address. > > The u-boot is really limited and I am transferring new images over kermit. Ouch! If you can, try kexec. You can use wget or similar to grab the kernel from you host machine, and then something like: kexec --append 'rootwait ro earlyprintk console=ttyS0,115200n8' bzImage wget will be much faster then kernel. Andrew
On 29.04.22 02:26, Andrew Lunn wrote: > On Thu, Apr 28, 2022 at 11:25:00PM +0300, Mauri Sandberg wrote: >> Hello Andrew! >> >> On 28.4.2022 3.18, Andrew Lunn wrote: >>>>> - there is no concensus on how to get ascii formatted MAC address from mtd >>>>> partitions so eth is not fully functional without setting the MAC from >>>>> userspace >>>> Ideally this is handled by the boot loader, but that requires being >>>> able to update >>>> it. >>> The mv643xx Ethernet driver is happy if it finds the MAC address >>> already in the hardware. The vendor uboot often does this. Does tftp >>> boot work in uboot? That would indicate it has access to the MAC >>> address. >> The u-boot is really limited and I am transferring new images over kermit. > Ouch! > > If you can, try kexec. You can use wget or similar to grab the kernel > from you host machine, and then something like: > > kexec --append 'rootwait ro earlyprintk console=ttyS0,115200n8' bzImage > > wget will be much faster then kernel. > Nice, this looks promising. Thanks for the tip.
On 28.4.2022 23.56, Arnd Bergmann wrote: > On Thu, Apr 28, 2022 at 10:01 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: >> On 27.4.2022 21.10, Arnd Bergmann wrote: >>> On Wed, Apr 27, 2022 at 6:21 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: >>>> - sata_mv fails to initialise with -22 (-EINVAL) >>> >>> No idea, I'd try inserting a printk in every code path that can return -EINVAL >>> from there >>> With debugging the reason for -EINVAL remains a bit mystery. - sata_mv calls ata_host_activate() [1] - later on, in request_threaded_irq(), there are sanity checks [2] - that fail with irq_settings_can_request() returning 0 [3] I cannot really put my finger on why the irq cannot be requested in DT approach. >> Is there a way to describe the PCIe bus in the >> device tree? The initalisation of that bus is done for rev A1 only. > > I'm not too familiar with the platform, but my interpretation is that the > DT support here is incomplete: > > The DT based PCI probe using drivers/pci/controller/pci-mvebu.c > is not hooked up in orion5x.dtsi, and the traditional pci code does > not work with DT. Can the existing pci code still be used to init the PCI bus and describe the rest in the DT or is it a futile attempt? > I see that orion5x has two separate blocks -- a PCIe host that is > similar to the kirkwood one, and a legacy PCI host that needs > a completely separate driver. > > Which of the two do you actually need here? > I really cannot say which one is it. How can I tell? The functions given in struct hw_pci find their way to drivers/pci/probe.c eventually and use pci_scan_root_bus_bridge(). Nothing seems to utilising mvebu or kirkwood explicitly at least. Here's the output from lspci if the ids reveal anything. # lspci -v -k 00:00.0 Class 0580: 11ab:5181 01:00.0 Class 0580: 11ab:5181 00:01.0 Class 0100: 11ab:7042 sata_mv -- Mauri [1] https://elixir.bootlin.com/linux/v5.17/source/drivers/ata/sata_mv.c#L4434 [2] https://elixir.bootlin.com/linux/v5.17/source/kernel/irq/manage.c#L2146 [3] https://elixir.bootlin.com/linux/v5.17/source/kernel/irq/settings.h#L100
On Sun, May 8, 2022 at 4:06 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: > On 28.4.2022 23.56, Arnd Bergmann wrote: > > On Thu, Apr 28, 2022 at 10:01 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: > >> On 27.4.2022 21.10, Arnd Bergmann wrote: > >>> On Wed, Apr 27, 2022 at 6:21 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: > >>>> - sata_mv fails to initialise with -22 (-EINVAL) > >>> > >>> No idea, I'd try inserting a printk in every code path that can return -EINVAL > >>> from there > >>> > > With debugging the reason for -EINVAL remains a bit mystery. > - sata_mv calls ata_host_activate() [1] > - later on, in request_threaded_irq(), there are sanity checks [2] > - that fail with irq_settings_can_request() returning 0 [3] > > I cannot really put my finger on why the irq cannot be requested in DT > approach. Are you sure the marvell,orion-intc driver is successfully probed at this point? If not, the interrupt won't be there. I see that the "sata_mv" driver can be used either as a platform driver for the orion5x on-chip controller, or as a PCI driver for an add-on chip connected to the external bus. It sounds like your system has both. Do you know which one fails? The PCI driver cannot work unless the PCI host works correctly, and that in turn requires a correct devicetree description for it. > >> Is there a way to describe the PCIe bus in the > >> device tree? The initalisation of that bus is done for rev A1 only. > > > > I'm not too familiar with the platform, but my interpretation is that the > > DT support here is incomplete: > > > > The DT based PCI probe using drivers/pci/controller/pci-mvebu.c > > is not hooked up in orion5x.dtsi, and the traditional pci code does > > not work with DT. > > Can the existing pci code still be used to init the PCI bus and describe > the rest in the DT or is it a futile attempt? > > > I see that orion5x has two separate blocks -- a PCIe host that is > > similar to the kirkwood one, and a legacy PCI host that needs > > a completely separate driver. > > > > Which of the two do you actually need here? > > > > I really cannot say which one is it. How can I tell? The functions given > in struct hw_pci find their way to drivers/pci/probe.c eventually and > use pci_scan_root_bus_bridge(). Nothing seems to utilising mvebu or > kirkwood explicitly at least. > > Here's the output from lspci if the ids reveal anything. > > # lspci -v -k > 00:00.0 Class 0580: 11ab:5181 > 01:00.0 Class 0580: 11ab:5181 > 00:01.0 Class 0100: 11ab:7042 sata_mv The first two seem to be the host bridges, but unfortunately they seem both have the same device ID, despite being very different devices. The first one (00:00.0) should be the PCIe driver, the second one (01.00.0) the legacy PCI one. In this case, the 11ab:7042 device is a PCIe device, and it's on the bus (00) of the first host bridge. I think this should work with drivers/pci/controller/pci-mvebu.c if you add the bits for probing. Thomas Petazzoni originally wrote the new driver, and I think he was planning at one point to use it for orion5x. I don't know if there were any major problems preventing this at the time, or if it just needs to get hooked up in the dtsi file. Arnd
On 08.05.22 18:22, Pali Rohár wrote: > On Sunday 08 May 2022 17:02:17 Arnd Bergmann wrote: >> On Sun, May 8, 2022 at 4:06 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: >>> On 28.4.2022 23.56, Arnd Bergmann wrote: >>>> On Thu, Apr 28, 2022 at 10:01 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: >>>>> On 27.4.2022 21.10, Arnd Bergmann wrote: >>>>>> On Wed, Apr 27, 2022 at 6:21 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: >>>>>>> - sata_mv fails to initialise with -22 (-EINVAL) >>>>>> >>>>>> No idea, I'd try inserting a printk in every code path that can return -EINVAL >>>>>> from there >>>>>> >>> >>> With debugging the reason for -EINVAL remains a bit mystery. >>> - sata_mv calls ata_host_activate() [1] >>> - later on, in request_threaded_irq(), there are sanity checks [2] >>> - that fail with irq_settings_can_request() returning 0 [3] >>> >>> I cannot really put my finger on why the irq cannot be requested in DT >>> approach. >> >> Are you sure the marvell,orion-intc driver is successfully probed >> at this point? If not, the interrupt won't be there. I made the pci setup to be the very last thing in the boot and results are still the same. There are other devices that do get their interrupts from intc. >> >> I see that the "sata_mv" driver can be used either as a platform >> driver for the orion5x on-chip controller, or as a PCI driver for >> an add-on chip connected to the external bus. It sounds like >> your system has both. Do you know which one fails? >> >> The PCI driver cannot work unless the PCI host works correctly, >> and that in turn requires a correct devicetree description for it. >> >>>>> Is there a way to describe the PCIe bus in the >>>>> device tree? The initalisation of that bus is done for rev A1 only. >>>> >>>> I'm not too familiar with the platform, but my interpretation is that the >>>> DT support here is incomplete: >>>> >>>> The DT based PCI probe using drivers/pci/controller/pci-mvebu.c >>>> is not hooked up in orion5x.dtsi, and the traditional pci code does >>>> not work with DT. >>> >>> Can the existing pci code still be used to init the PCI bus and describe >>> the rest in the DT or is it a futile attempt? > > Hello! Orion uses arch/arm/mach-orion5x/pci.c driver for both PCI and > PCIe buses. This is not device tree driver. > >>>> I see that orion5x has two separate blocks -- a PCIe host that is >>>> similar to the kirkwood one, and a legacy PCI host that needs >>>> a completely separate driver. >>>> >>>> Which of the two do you actually need here? >>>> >>> >>> I really cannot say which one is it. How can I tell? The functions given >>> in struct hw_pci find their way to drivers/pci/probe.c eventually and >>> use pci_scan_root_bus_bridge(). Nothing seems to utilising mvebu or >>> kirkwood explicitly at least. >>> >>> Here's the output from lspci if the ids reveal anything. >>> >>> # lspci -v -k >>> 00:00.0 Class 0580: 11ab:5181 >>> 01:00.0 Class 0580: 11ab:5181 >>> 00:01.0 Class 0100: 11ab:7042 sata_mv >> >> The first two seem to be the host bridges, but unfortunately they >> seem both have the same device ID, despite being very different >> devices. The first one (00:00.0) should be the PCIe driver, the >> second one (01.00.0) the legacy PCI one. In this case, the 11ab:7042 >> device is a PCIe device, and it's on the bus (00) of the first host >> bridge. I think this should work with drivers/pci/controller/pci-mvebu.c >> if you add the bits for probing. > > Last time when I looked on Orion PCIe controller registers, I though > that they are same as in Kirkwood PCIe controller registers. And > Kirkwood is already supported by pci-mvebu.c driver. > I seemed that way to me too on the first glance. And it looks like there are no devices using the PCI driver. I knocked off that part altogether and the boot log looks pretty much the same it was. Perhaps I can do with describing the PCIe bus only. > About PCI host bridge, I do not know. > > Beware that PCI Class Id and all PCI registers which are different for > Type 0 and Type 1 are _broken_ on all PCIe Root Ports form all 32-bit > Marvell SoCs. Those registers on Marvell SoCs have different meaning as > what is defined in PCI and PCIe specs. So it means that lspci _may_ > display bogus information about PCIe Root Port. pci-mvebu.c uses Root > Port emulator which fills correct data to make kernel and lspci happy. > > If you are going to extend pci-mvebu.c to support also Orion PCIe > controller, I could try to help with it. But I do not have any Orion > hardware, so just basic help... I could make an attempt at this. Should I try to look at an existing kirkwood based device first, say kirkwood-6281.dtsi? I didn't see anything SoC-specific in pci-mvebu.c. All different compatibles seem to share the same functionality. > > Links to Orion documentations, including PCIe errata is available in > kernel documentation. So this could help to understand some details: > https://www.kernel.org/doc/html/latest/arm/marvell.html > > Anyway, could you please provide 'lspci -nn -vv' and 'lspci -nn -t -v' > outputs from Orion? # lspci -nn -vv 0000:00:00.0 Memory controller [0580]: Marvell Technology Group Ltd. 88f5181 [Orion-1] ARM SoC [11ab:5181] (rev 03) Subsystem: Marvell Technology Group Ltd. Device [11ab:11ab] Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr+ Stepping- SERR+ FastB2B- DisINTx- Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Latency: 0, Cache Line Size: 32 bytes Interrupt: pin A routed to IRQ 0 Region 0: Memory at <ignored> (64-bit, prefetchable) Capabilities: [40] Power Management version 2 Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-) Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME- Capabilities: [50] MSI: Enable- Count=1/1 Maskable- 64bit+ Address: 0000000000000000 Data: 0000 Capabilities: [60] Express (v1) Root Port (Slot-), MSI 00 DevCap: MaxPayload 128 bytes, PhantFunc 0 ExtTag- RBE- DevCtl: CorrErr- NonFatalErr- FatalErr- UnsupReq- RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop- MaxPayload 128 bytes, MaxReadReq 512 bytes DevSta: CorrErr- NonFatalErr- FatalErr- UnsupReq- AuxPwr- TransPend- LnkCap: Port #0, Speed 2.5GT/s, Width x1, ASPM L0s, Exit Latency L0s <256ns ClockPM- Surprise- LLActRep- BwNot- ASPMOptComp- LnkCtl: ASPM Disabled; RCB 128 bytes, Disabled- CommClk- ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt- LnkSta: Speed 2.5GT/s (ok), Width x1 (ok) TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt- RootCap: CRSVisible- RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna- CRSVisible- RootSta: PME ReqID 0000, PMEStatus- PMEPending- 0000:00:01.0 SCSI storage controller [0100]: Marvell Technology Group Ltd. 88SX7042 PCI-e 4-port SATA-II [11ab:7042] (rev 02) Subsystem: Marvell Technology Group Ltd. Device [11ab:11ab] Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr+ Stepping- SERR+ FastB2B- DisINTx- Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Latency: 0, Cache Line Size: 32 bytes Interrupt: pin A routed to IRQ 12 Region 0: Memory at e0000000 (64-bit, non-prefetchable) [size=1M] Region 2: I/O ports at 1000 [size=256] Capabilities: [40] Power Management version 2 Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-) Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME- Capabilities: [50] MSI: Enable- Count=1/1 Maskable- 64bit+ Address: 0000000000000000 Data: 0000 Capabilities: [60] Express (v1) Legacy Endpoint, MSI 00 DevCap: MaxPayload 256 bytes, PhantFunc 0, Latency L0s <256ns, L1 <1us ExtTag- AttnBtn- AttnInd- PwrInd- RBE- FLReset- DevCtl: CorrErr- NonFatalErr- FatalErr- UnsupReq- RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop- MaxPayload 128 bytes, MaxReadReq 512 bytes DevSta: CorrErr- NonFatalErr- FatalErr- UnsupReq- AuxPwr- TransPend- LnkCap: Port #0, Speed 2.5GT/s, Width x4, ASPM L0s, Exit Latency L0s <256ns ClockPM- Surprise- LLActRep- BwNot- ASPMOptComp- LnkCtl: ASPM Disabled; RCB 128 bytes, Disabled- CommClk- ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt- LnkSta: Speed 2.5GT/s (ok), Width x1 (downgraded) TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt- Kernel driver in use: sata_mv 0001:01:00.0 Memory controller [0580]: Marvell Technology Group Ltd. 88f5181 [Orion-1] ARM SoC [11ab:5181] (rev 03) Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr+ Stepping- SERR+ FastB2B+ DisINTx- Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort+ >SERR- <PERR- INTx- Latency: 0, Cache Line Size: 32 bytes Interrupt: pin A routed to IRQ 0 BIST result: 00 Region 0: Memory at <unassigned> (64-bit, prefetchable) Region 2: Memory at <ignored> (64-bit, prefetchable) Region 4: Memory at <ignored> (64-bit, non-prefetchable) Expansion ROM at <ignored> [disabled] Capabilities: [40] Power Management version 2 Flags: PMEClk+ DSI- D1+ D2+ AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold-) Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME+ Capabilities: [48] Vital Product Data Not readable Capabilities: [50] MSI: Enable- Count=1/1 Maskable- 64bit+ Address: 0000000000000000 Data: 0000 Capabilities: [60] PCI-X non-bridge device Command: DPERE- ERO- RBC=512 OST=4 Status: Dev=ff:1f.0 64bit+ 133MHz+ SCD- USC- DC=bridge DMMRBC=512 DMOST=4 DMCRS=8 RSCEM- 266MHz- 533MHz- Capabilities: [68] CompactPCI hot-swap <?> # lspci -nn -t -v -+-[0001:01]---00.0 Marvell Technology Group Ltd. 88f5181 [Orion-1] ARM SoC [11ab:5181] \-[0000:00]-+-00.0 Marvell Technology Group Ltd. 88f5181 [Orion-1] ARM SoC [11ab:5181] \-01.0 Marvell Technology Group Ltd. 88SX7042 PCI-e 4-port SATA-II [11ab:7042] > >> Thomas Petazzoni originally wrote the new driver, and I think he was >> planning at one point to use it for orion5x. I don't know if there were >> any major problems preventing this at the time, or if it just needs to >> get hooked up in the dtsi file. >> >> Arnd -- Mauri
On Sun, May 8, 2022 at 5:41 PM Pali Rohár <pali@kernel.org> wrote: > On Sunday 08 May 2022 17:22:37 Pali Rohár wrote: > > On Sunday 08 May 2022 17:02:17 Arnd Bergmann wrote: > > > > > > > > Can the existing pci code still be used to init the PCI bus and describe > > > > the rest in the DT or is it a futile attempt? > > > > Hello! Orion uses arch/arm/mach-orion5x/pci.c driver for both PCI and > > PCIe buses. This is not device tree driver. > > Correction, Orion PCIe driver is arch/arm/plat-orion/pcie.c and it calls > common functions from mach-orion5x/pci.c driver. FWIW, I have an older patch series that turns the plat-orion/pcie.c and mach-orion5x/pci.c into standalone host bridge drivers that no longer use the arm/kernel/bios32.c helpers. If anyone wants to work on DT support for the legacy-pci side (not for this machine but maybe another orion5x one), I can try to rebase those patches to make it easier to add the missing DT support. Arnd
On 8.5.2022 18.41, Pali Rohár wrote: > On Sunday 08 May 2022 17:22:37 Pali Rohár wrote: >> On Sunday 08 May 2022 17:02:17 Arnd Bergmann wrote: >>> On Sun, May 8, 2022 at 4:06 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: >>>> On 28.4.2022 23.56, Arnd Bergmann wrote: >>>>> On Thu, Apr 28, 2022 at 10:01 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: >>>>>> On 27.4.2022 21.10, Arnd Bergmann wrote: >>>>>>> On Wed, Apr 27, 2022 at 6:21 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: >>>>>>>> - sata_mv fails to initialise with -22 (-EINVAL) >>>>>>> >>>>>>> No idea, I'd try inserting a printk in every code path that can return -EINVAL >>>>>>> from there >>>>>>> >>>> >>>> With debugging the reason for -EINVAL remains a bit mystery. >>>> - sata_mv calls ata_host_activate() [1] >>>> - later on, in request_threaded_irq(), there are sanity checks [2] >>>> - that fail with irq_settings_can_request() returning 0 [3] >>>> >>>> I cannot really put my finger on why the irq cannot be requested in DT >>>> approach. >>> >>> Are you sure the marvell,orion-intc driver is successfully probed >>> at this point? If not, the interrupt won't be there. >>> >>> I see that the "sata_mv" driver can be used either as a platform >>> driver for the orion5x on-chip controller, or as a PCI driver for >>> an add-on chip connected to the external bus. It sounds like >>> your system has both. Do you know which one fails? >>> >>> The PCI driver cannot work unless the PCI host works correctly, >>> and that in turn requires a correct devicetree description for it. >>> >>>>>> Is there a way to describe the PCIe bus in the >>>>>> device tree? The initalisation of that bus is done for rev A1 only. >>>>> >>>>> I'm not too familiar with the platform, but my interpretation is that the >>>>> DT support here is incomplete: >>>>> >>>>> The DT based PCI probe using drivers/pci/controller/pci-mvebu.c >>>>> is not hooked up in orion5x.dtsi, and the traditional pci code does >>>>> not work with DT. >>>> >>>> Can the existing pci code still be used to init the PCI bus and describe >>>> the rest in the DT or is it a futile attempt? >> >> Hello! Orion uses arch/arm/mach-orion5x/pci.c driver for both PCI and >> PCIe buses. This is not device tree driver. > > Correction, Orion PCIe driver is arch/arm/plat-orion/pcie.c and it calls > common functions from mach-orion5x/pci.c driver. > >>>>> I see that orion5x has two separate blocks -- a PCIe host that is >>>>> similar to the kirkwood one, and a legacy PCI host that needs >>>>> a completely separate driver. >>>>> >>>>> Which of the two do you actually need here? >>>>> >>>> >>>> I really cannot say which one is it. How can I tell? The functions given >>>> in struct hw_pci find their way to drivers/pci/probe.c eventually and >>>> use pci_scan_root_bus_bridge(). Nothing seems to utilising mvebu or >>>> kirkwood explicitly at least. >>>> >>>> Here's the output from lspci if the ids reveal anything. >>>> >>>> # lspci -v -k >>>> 00:00.0 Class 0580: 11ab:5181 >>>> 01:00.0 Class 0580: 11ab:5181 >>>> 00:01.0 Class 0100: 11ab:7042 sata_mv >>> >>> The first two seem to be the host bridges, but unfortunately they >>> seem both have the same device ID, despite being very different >>> devices. The first one (00:00.0) should be the PCIe driver, the >>> second one (01.00.0) the legacy PCI one. In this case, the 11ab:7042 >>> device is a PCIe device, and it's on the bus (00) of the first host >>> bridge. I think this should work with drivers/pci/controller/pci-mvebu.c >>> if you add the bits for probing. >> >> Last time when I looked on Orion PCIe controller registers, I though >> that they are same as in Kirkwood PCIe controller registers. And >> Kirkwood is already supported by pci-mvebu.c driver. >> >> About PCI host bridge, I do not know. >> >> Beware that PCI Class Id and all PCI registers which are different for >> Type 0 and Type 1 are _broken_ on all PCIe Root Ports form all 32-bit >> Marvell SoCs. Those registers on Marvell SoCs have different meaning as >> what is defined in PCI and PCIe specs. So it means that lspci _may_ >> display bogus information about PCIe Root Port. pci-mvebu.c uses Root >> Port emulator which fills correct data to make kernel and lspci happy. >> >> If you are going to extend pci-mvebu.c to support also Orion PCIe >> controller, I could try to help with it. But I do not have any Orion >> hardware, so just basic help... >> >> Links to Orion documentations, including PCIe errata is available in >> kernel documentation. So this could help to understand some details: >> https://www.kernel.org/doc/html/latest/arm/marvell.html >> >> Anyway, could you please provide 'lspci -nn -vv' and 'lspci -nn -t -v' >> outputs from Orion? >> >>> Thomas Petazzoni originally wrote the new driver, and I think he was >>> planning at one point to use it for orion5x. I don't know if there were >>> any major problems preventing this at the time, or if it just needs to >>> get hooked up in the dtsi file. >>> >>> Arnd > > There is Orion-specific errata that config space via CF8/CFC registers > is broken. Workaround documented in errata documented (linked from above > documentation) does not work when DMA is used and instead other > undocumented workaround is needed (implemented in arch/arm) which maps > config space to memory (and therefore avoids usage of broken CF8/CFC > memory mapped registers). So basically I should look at arch/arm/plat-orion/pcie.c for the configuration part, add new compatible to pci-mvebu.c for orion5x and alter the probing function accoringly for the same. Did I get it correctly? If so, sounds simple when said out lout but I might need some more pointers to get started. Like with configuration people generally mean setting BARs and WINs? Or is there more to it? :) If you could lay out the basic steps that are needed I would really appreciate it.
On 8.5.2022 23.10, Pali Rohár wrote: > On Sunday 08 May 2022 22:34:19 Mauri Sandberg wrote: >> On 08.05.22 18:22, Pali Rohár wrote: >>> On Sunday 08 May 2022 17:02:17 Arnd Bergmann wrote: >>>> On Sun, May 8, 2022 at 4:06 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: >>>>> On 28.4.2022 23.56, Arnd Bergmann wrote: >>>>>> On Thu, Apr 28, 2022 at 10:01 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: >>>>>>> On 27.4.2022 21.10, Arnd Bergmann wrote: >>>>>>>> On Wed, Apr 27, 2022 at 6:21 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: >>>>>>>>> - sata_mv fails to initialise with -22 (-EINVAL) >>>>>>>> >>>>>>>> No idea, I'd try inserting a printk in every code path that can return -EINVAL >>>>>>>> from there >>>>>>>> >>>>> >>>>> With debugging the reason for -EINVAL remains a bit mystery. >>>>> - sata_mv calls ata_host_activate() [1] >>>>> - later on, in request_threaded_irq(), there are sanity checks [2] >>>>> - that fail with irq_settings_can_request() returning 0 [3] >>>>> >>>>> I cannot really put my finger on why the irq cannot be requested in DT >>>>> approach. >>>> >>>> Are you sure the marvell,orion-intc driver is successfully probed >>>> at this point? If not, the interrupt won't be there. >> >> I made the pci setup to be the very last thing in the boot and >> results are still the same. There are other devices that do get >> their interrupts from intc. >> >>>> >>>> I see that the "sata_mv" driver can be used either as a platform >>>> driver for the orion5x on-chip controller, or as a PCI driver for >>>> an add-on chip connected to the external bus. It sounds like >>>> your system has both. Do you know which one fails? >>>> >>>> The PCI driver cannot work unless the PCI host works correctly, >>>> and that in turn requires a correct devicetree description for it. >>>> >>>>>>> Is there a way to describe the PCIe bus in the >>>>>>> device tree? The initalisation of that bus is done for rev A1 only. >>>>>> >>>>>> I'm not too familiar with the platform, but my interpretation is that the >>>>>> DT support here is incomplete: >>>>>> >>>>>> The DT based PCI probe using drivers/pci/controller/pci-mvebu.c >>>>>> is not hooked up in orion5x.dtsi, and the traditional pci code does >>>>>> not work with DT. >>>>> >>>>> Can the existing pci code still be used to init the PCI bus and describe >>>>> the rest in the DT or is it a futile attempt? >>> >>> Hello! Orion uses arch/arm/mach-orion5x/pci.c driver for both PCI and >>> PCIe buses. This is not device tree driver. >>> >>>>>> I see that orion5x has two separate blocks -- a PCIe host that is >>>>>> similar to the kirkwood one, and a legacy PCI host that needs >>>>>> a completely separate driver. >>>>>> >>>>>> Which of the two do you actually need here? >>>>>> >>>>> >>>>> I really cannot say which one is it. How can I tell? The functions given >>>>> in struct hw_pci find their way to drivers/pci/probe.c eventually and >>>>> use pci_scan_root_bus_bridge(). Nothing seems to utilising mvebu or >>>>> kirkwood explicitly at least. >>>>> >>>>> Here's the output from lspci if the ids reveal anything. >>>>> >>>>> # lspci -v -k >>>>> 00:00.0 Class 0580: 11ab:5181 >>>>> 01:00.0 Class 0580: 11ab:5181 >>>>> 00:01.0 Class 0100: 11ab:7042 sata_mv >>>> >>>> The first two seem to be the host bridges, but unfortunately they >>>> seem both have the same device ID, despite being very different >>>> devices. The first one (00:00.0) should be the PCIe driver, the >>>> second one (01.00.0) the legacy PCI one. In this case, the 11ab:7042 >>>> device is a PCIe device, and it's on the bus (00) of the first host >>>> bridge. I think this should work with drivers/pci/controller/pci-mvebu.c >>>> if you add the bits for probing. >>> >>> Last time when I looked on Orion PCIe controller registers, I though >>> that they are same as in Kirkwood PCIe controller registers. And >>> Kirkwood is already supported by pci-mvebu.c driver. >>> >> >> I seemed that way to me too on the first glance. And it looks like there >> are no devices using the PCI driver. I knocked off that part altogether and >> the boot log looks pretty much the same it was. Perhaps I can do >> with describing the PCIe bus only. >> >>> About PCI host bridge, I do not know. >>> >>> Beware that PCI Class Id and all PCI registers which are different for >>> Type 0 and Type 1 are _broken_ on all PCIe Root Ports form all 32-bit >>> Marvell SoCs. Those registers on Marvell SoCs have different meaning as >>> what is defined in PCI and PCIe specs. So it means that lspci _may_ >>> display bogus information about PCIe Root Port. pci-mvebu.c uses Root >>> Port emulator which fills correct data to make kernel and lspci happy. >>> >>> If you are going to extend pci-mvebu.c to support also Orion PCIe >>> controller, I could try to help with it. But I do not have any Orion >>> hardware, so just basic help... >> >> I could make an attempt at this. Should I try to look at an existing >> kirkwood based device first, say kirkwood-6281.dtsi? I didn't see >> anything SoC-specific in pci-mvebu.c. All different compatibles seem >> to share the same functionality. > > Yes, this could be a good starting point. But you will need new > compatible string for orion, specially to implement workaround for > accessing config space. > >>> >>> Links to Orion documentations, including PCIe errata is available in >>> kernel documentation. So this could help to understand some details: >>> https://www.kernel.org/doc/html/latest/arm/marvell.html >>> >>> Anyway, could you please provide 'lspci -nn -vv' and 'lspci -nn -t -v' >>> outputs from Orion? >> >> # lspci -nn -vv >> 0000:00:00.0 Memory controller [0580]: Marvell Technology Group Ltd. 88f5181 >> [Orion-1] ARM SoC [11ab:5181] (rev 03) >> Subsystem: Marvell Technology Group Ltd. Device [11ab:11ab] >> Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr+ >> Stepping- SERR+ FastB2B- DisINTx- >> Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- >> <MAbort- >SERR- <PERR- INTx- >> Latency: 0, Cache Line Size: 32 bytes >> Interrupt: pin A routed to IRQ 0 >> Region 0: Memory at <ignored> (64-bit, prefetchable) >> Capabilities: [40] Power Management version 2 >> Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-) >> Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME- >> Capabilities: [50] MSI: Enable- Count=1/1 Maskable- 64bit+ >> Address: 0000000000000000 Data: 0000 >> Capabilities: [60] Express (v1) Root Port (Slot-), MSI 00 >> DevCap: MaxPayload 128 bytes, PhantFunc 0 >> ExtTag- RBE- >> DevCtl: CorrErr- NonFatalErr- FatalErr- UnsupReq- >> RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop- >> MaxPayload 128 bytes, MaxReadReq 512 bytes >> DevSta: CorrErr- NonFatalErr- FatalErr- UnsupReq- AuxPwr- TransPend- >> LnkCap: Port #0, Speed 2.5GT/s, Width x1, ASPM L0s, Exit Latency L0s >> <256ns >> ClockPM- Surprise- LLActRep- BwNot- ASPMOptComp- >> LnkCtl: ASPM Disabled; RCB 128 bytes, Disabled- CommClk- >> ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt- >> LnkSta: Speed 2.5GT/s (ok), Width x1 (ok) >> TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt- >> RootCap: CRSVisible- >> RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna- CRSVisible- >> RootSta: PME ReqID 0000, PMEStatus- PMEPending- >> >> 0000:00:01.0 SCSI storage controller [0100]: Marvell Technology Group Ltd. >> 88SX7042 PCI-e 4-port SATA-II [11ab:7042] (rev 02) >> Subsystem: Marvell Technology Group Ltd. Device [11ab:11ab] >> Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr+ >> Stepping- SERR+ FastB2B- DisINTx- >> Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- >> <MAbort- >SERR- <PERR- INTx- >> Latency: 0, Cache Line Size: 32 bytes >> Interrupt: pin A routed to IRQ 12 >> Region 0: Memory at e0000000 (64-bit, non-prefetchable) [size=1M] >> Region 2: I/O ports at 1000 [size=256] >> Capabilities: [40] Power Management version 2 >> Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-) >> Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME- >> Capabilities: [50] MSI: Enable- Count=1/1 Maskable- 64bit+ >> Address: 0000000000000000 Data: 0000 >> Capabilities: [60] Express (v1) Legacy Endpoint, MSI 00 >> DevCap: MaxPayload 256 bytes, PhantFunc 0, Latency L0s <256ns, L1 <1us >> ExtTag- AttnBtn- AttnInd- PwrInd- RBE- FLReset- >> DevCtl: CorrErr- NonFatalErr- FatalErr- UnsupReq- >> RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop- >> MaxPayload 128 bytes, MaxReadReq 512 bytes >> DevSta: CorrErr- NonFatalErr- FatalErr- UnsupReq- AuxPwr- TransPend- >> LnkCap: Port #0, Speed 2.5GT/s, Width x4, ASPM L0s, Exit Latency L0s >> <256ns >> ClockPM- Surprise- LLActRep- BwNot- ASPMOptComp- >> LnkCtl: ASPM Disabled; RCB 128 bytes, Disabled- CommClk- >> ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt- >> LnkSta: Speed 2.5GT/s (ok), Width x1 (downgraded) >> TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt- >> Kernel driver in use: sata_mv >> >> 0001:01:00.0 Memory controller [0580]: Marvell Technology Group Ltd. 88f5181 >> [Orion-1] ARM SoC [11ab:5181] (rev 03) >> Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr+ >> Stepping- SERR+ FastB2B+ DisINTx- >> Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- >> <MAbort+ >SERR- <PERR- INTx- >> Latency: 0, Cache Line Size: 32 bytes >> Interrupt: pin A routed to IRQ 0 >> BIST result: 00 >> Region 0: Memory at <unassigned> (64-bit, prefetchable) >> Region 2: Memory at <ignored> (64-bit, prefetchable) >> Region 4: Memory at <ignored> (64-bit, non-prefetchable) >> Expansion ROM at <ignored> [disabled] >> Capabilities: [40] Power Management version 2 >> Flags: PMEClk+ DSI- D1+ D2+ AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold-) >> Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME+ >> Capabilities: [48] Vital Product Data >> Not readable >> Capabilities: [50] MSI: Enable- Count=1/1 Maskable- 64bit+ >> Address: 0000000000000000 Data: 0000 >> Capabilities: [60] PCI-X non-bridge device >> Command: DPERE- ERO- RBC=512 OST=4 >> Status: Dev=ff:1f.0 64bit+ 133MHz+ SCD- USC- DC=bridge DMMRBC=512 DMOST=4 >> DMCRS=8 RSCEM- 266MHz- 533MHz- >> Capabilities: [68] CompactPCI hot-swap <?> >> >> # lspci -nn -t -v >> -+-[0001:01]---00.0 Marvell Technology Group Ltd. 88f5181 [Orion-1] ARM SoC >> [11ab:5181] >> \-[0000:00]-+-00.0 Marvell Technology Group Ltd. 88f5181 [Orion-1] ARM SoC >> [11ab:5181] >> \-01.0 Marvell Technology Group Ltd. 88SX7042 PCI-e 4-port >> SATA-II [11ab:7042] > > Ok, so domain 0 is PCIe bus for sure. > 0000:00:00.0 is PCIe Root Port (PCI-to-PCI bridge) incorrectly detected > as Memory controller (known HW issue on all 32-bit Marvell SoCs). > 0000:00:01.0 seems to be that SATA controller and this device is > connected behind the PCIe Root Port. Topology is also incorrectly > reports due to same known issue. > > Then there is domain 1 (first line in -t output) on which is just one > device 0001:01:00.0 detected as Memory controller and has capability of > "PCI-X non-bridge device". This seems to be PCI bus. I guess that Memory > controller is also bogus information. > > What is "PCI-X non-bridge device"? I thought that "root" of the PCI bus > should be Host Bridge. > > Anyway, there is my pending patch which should fix Class ID to not > report incorrect Memory controller identification: > https://lore.kernel.org/linux-pci/20211102171259.9590-1-pali@kernel.org/#Z31arch:arm:mach-orion5x:pci.c With the patch the roots are identified as follows: # lspci -nn -vv 0000:00:00.0 Host bridge [0600]: Marvell Technology Group Ltd. 88f5181 [Orion-1] ARM SoC [11ab:5181] (rev 03) ... 0001:01:00.0 Host bridge [0600]: Marvell Technology Group Ltd. 88f5181 [Orion-1] ARM SoC [11ab:5181] (rev 03) Everything else remained the same. >>> >>>> Thomas Petazzoni originally wrote the new driver, and I think he was >>>> planning at one point to use it for orion5x. I don't know if there were >>>> any major problems preventing this at the time, or if it just needs to >>>> get hooked up in the dtsi file. >>>> >>>> Arnd >> >> -- Mauri
Hello all, This is the second iteration of my series to add D-Link DNS323 devices, which are based on a device tree. It adds the bindings, the device tree source files and a new board file to take care of initialising what little remains that cannot be done via device tree. The initialisation of MAC address will be neater once method for passing ASCII string based address is agreed on. Functionally this series has a dependency to Pali's mvebu pci series, which adds support for Orion PCIe and which should be scheduled for a merge window any time soon: https://lore.kernel.org/linux-pci/20220905192310.22786-1-pali@kernel.org/ What currently is tested and works in rev A1 - leds - keys - fan - temperature sensor - shutdown - reboot - mtd partitions - ethernet - PCIe and with that sata_mv And the variants B1 and C1 need testing still. Thanks, Mauri Mauri Sandberg (3): dt-bindings: arm: add DT binding for D-Link DNS-323 ARM: dts: orion5x: Add D-Link DNS-323 Device Tree ARM: orion5x: Add D-Link DNS-323 based on Device Tree .../bindings/arm/marvell/marvell,orion5x.txt | 12 + arch/arm/boot/dts/Makefile | 3 + arch/arm/boot/dts/orion5x-dlink-dns323.dtsi | 215 ++++++++++++++++++ arch/arm/boot/dts/orion5x-dlink-dns323a1.dts | 44 ++++ arch/arm/boot/dts/orion5x-dlink-dns323b1.dts | 39 ++++ arch/arm/boot/dts/orion5x-dlink-dns323c1.dts | 81 +++++++ arch/arm/mach-orion5x/Kconfig | 7 + arch/arm/mach-orion5x/Makefile | 1 + arch/arm/mach-orion5x/board-dns323.c | 208 +++++++++++++++++ arch/arm/mach-orion5x/board-dt.c | 3 + arch/arm/mach-orion5x/common.h | 6 + 11 files changed, 619 insertions(+) create mode 100644 arch/arm/boot/dts/orion5x-dlink-dns323.dtsi create mode 100644 arch/arm/boot/dts/orion5x-dlink-dns323a1.dts create mode 100644 arch/arm/boot/dts/orion5x-dlink-dns323b1.dts create mode 100644 arch/arm/boot/dts/orion5x-dlink-dns323c1.dts create mode 100644 arch/arm/mach-orion5x/board-dns323.c base-commit: c69cf88cda5faca0e411babb67ac0d8bfd8b4646 -- 2.25.1
On Thu, Sep 22, 2022 at 11:24:56PM +0300, Mauri Sandberg wrote: > Add bindings for D-Link DNS-323. It introduces altogether four new > compatibles: dlink,dns323, dlink,dns323a1, dlink,dns323b1 and > dlink,dns323c1. One is for the common parts between the devices and > then there is one for each three hardware variants. > > Signed-off-by: Mauri Sandberg <maukka@ext.kapsi.fi> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
> + chosen { > + stdout-path = "serial0:115200n8"; > + bootargs = "console=ttyS0,115200n8 earlyprintk"; bootargs is generally not liked. Since you have stdout-path do you need console=ttyS0,115200n8? earlyprintk should not be needed now you have something which boots. > +&mdio { > + status = "okay"; > + > + ethphy: ethernet-phy { > + reg = <8>; Since you have a reg value, this should be ethernet-phy@9 Andrew
> +static void __init dns323_dt_eth_fixup(void) > +{ > + struct device_node *np; > + u8 addr[ETH_ALEN]; > + int ret; > + > + /* > + * The ethernet interfaces forget the MAC address assigned by u-boot > + * if the clocks are turned off. Usually, u-boot on orion boards > + * has no DT support to properly set local-mac-address property. > + * As a workaround, we get the MAC address that is stored in flash > + * and update the port device node if no valid MAC address is set. > + */ This is true for Kirkwood, but orion5x does not have any clocks to gate. So i'm pretty sure this is not true. You copied this code for a different reason. Please document here the real reason for this code. > + ret = dns323_read_mac_addr(addr); > + > + if (ret) { > + pr_warn("Unable to find MAC address in flash memory\n"); > + return; > + } > + > + np = of_find_compatible_node(NULL, NULL, "marvell,orion-eth-port"); > + > + if (!IS_ERR(np)) { > + struct device_node *pnp = of_get_parent(np); > + struct clk *clk; > + struct property *pmac; > + u8 tmpmac[ETH_ALEN]; > + u8 *macaddr; > + int i; > + > + if (!pnp) > + return; > + > + /* skip disabled nodes or nodes with valid MAC address*/ > + if (!of_device_is_available(pnp) || > + !of_get_mac_address(np, tmpmac)) > + goto eth_fixup_skip; > + > + clk = of_clk_get(pnp, 0); > + if (IS_ERR(clk)) > + goto eth_fixup_skip; > + > + /* ensure port clock is not gated to not hang CPU */ > + clk_prepare_enable(clk); I'm pretty sure this clock stuff is not needed. Please comment it out and see if the machine locks up. Kirkwood just stops dead if you access registers when there clocks are disabled. For Orion5x, the ethernet should always have a clock. > + > + /* store MAC address register contents in local-mac-address */ > + pmac = kzalloc(sizeof(*pmac) + 6, GFP_KERNEL); > + if (!pmac) > + goto eth_fixup_no_mem; > + > + pmac->value = pmac + 1; > + pmac->length = ETH_ALEN; > + pmac->name = kstrdup("local-mac-address", GFP_KERNEL); > + if (!pmac->name) { > + kfree(pmac); > + goto eth_fixup_no_mem; > + } > + > + macaddr = pmac->value; > + for (i = 0; i < ETH_ALEN; i++) > + macaddr[i] = addr[i]; > + > + of_update_property(np, pmac); > + > +eth_fixup_no_mem: > + clk_disable_unprepare(clk); > + clk_put(clk); > +eth_fixup_skip: > + of_node_put(pnp); > + } > +} > + > +void __init dns323_init_dt(void) > +{ > + if (of_machine_is_compatible("dlink,dns323a1")) { > + writel(0, MPP_DEV_CTRL); /* DEV_D[31:16] */ I spotted this in dns323-setup.c as well. Do you have any idea what it does? Andrew
On Thursday 22 September 2022 22:45:47 Andrew Lunn wrote: > > +&mdio { > > + status = "okay"; > > + > > + ethphy: ethernet-phy { > > + reg = <8>; > > Since you have a reg value, this should be ethernet-phy@9 ethernet-phy@8 no?
On Thu, Sep 22, 2022 at 11:14:08PM +0200, Pali Rohár wrote: > On Thursday 22 September 2022 22:45:47 Andrew Lunn wrote: > > > +&mdio { > > > + status = "okay"; > > > + > > > + ethphy: ethernet-phy { > > > + reg = <8>; > > > > Since you have a reg value, this should be ethernet-phy@9 > > ethernet-phy@8 no? Yes, @8. Sorry. Andrew
On Thu, Sep 22, 2022, at 10:24 PM, Mauri Sandberg wrote: > + > +/* dns323_parse_hex_*() taken from tsx09-common.c; should a common > copy of these > + * functions be kept somewhere? > + */ > +static int __init dns323_parse_hex_nibble(char n) > +{ > + if (n >= '0' && n <= '9') > + return n - '0'; > + > + if (n >= 'A' && n <= 'F') > + return n - 'A' + 10; > + > + if (n >= 'a' && n <= 'f') > + return n - 'a' + 10; > + > + return -1; > +} > + > +static int __init dns323_parse_hex_byte(const char *b) > +{ > + int hi; > + int lo; > + > + hi = dns323_parse_hex_nibble(b[0]); > + lo = dns323_parse_hex_nibble(b[1]); > + > + if (hi < 0 || lo < 0) > + return -1; > + > + return (hi << 4) | lo; > +} > + Can you use simple_strntoull() to parse the address into a u64 instead? > +static int __init dns323_read_mac_addr(u8 *addr) > +{ > + int i; > + char *mac_page; > + > + /* MAC address is stored as a regular ol' string in /dev/mtdblock4 > + * (0x007d0000-0x00800000) starting at offset 196480 (0x2ff80). > + */ > + mac_page = ioremap(DNS323_NOR_BOOT_BASE + 0x7d0000 + 196480, 1024); > + if (!mac_page) > + return -ENOMEM; It would be nicer to use of_iomap() on the nor device than a hardcoded physical address here, at least if that doesn't add too much extra complexity. Arnd
On 23.9.2022 00:10, Andrew Lunn wrote: >> +static void __init dns323_dt_eth_fixup(void) >> +{ >> + struct device_node *np; >> + u8 addr[ETH_ALEN]; >> + int ret; >> + >> + /* >> + * The ethernet interfaces forget the MAC address assigned by u-boot >> + * if the clocks are turned off. Usually, u-boot on orion boards >> + * has no DT support to properly set local-mac-address property. >> + * As a workaround, we get the MAC address that is stored in flash >> + * and update the port device node if no valid MAC address is set. >> + */ > > This is true for Kirkwood, but orion5x does not have any clocks to > gate. So i'm pretty sure this is not true. You copied this code for a > different reason. Please document here the real reason for this code. > Yes, will do. To my understanding it looks like uboot does not pass anything to the kernel. >> + ret = dns323_read_mac_addr(addr); >> + >> + if (ret) { >> + pr_warn("Unable to find MAC address in flash memory\n"); >> + return; >> + } >> + >> + np = of_find_compatible_node(NULL, NULL, "marvell,orion-eth-port"); >> + >> + if (!IS_ERR(np)) { >> + struct device_node *pnp = of_get_parent(np); >> + struct clk *clk; >> + struct property *pmac; >> + u8 tmpmac[ETH_ALEN]; >> + u8 *macaddr; >> + int i; >> + >> + if (!pnp) >> + return; >> + >> + /* skip disabled nodes or nodes with valid MAC address*/ >> + if (!of_device_is_available(pnp) || >> + !of_get_mac_address(np, tmpmac)) >> + goto eth_fixup_skip; >> + >> + clk = of_clk_get(pnp, 0); >> + if (IS_ERR(clk)) >> + goto eth_fixup_skip; >> + >> + /* ensure port clock is not gated to not hang CPU */ >> + clk_prepare_enable(clk); > > I'm pretty sure this clock stuff is not needed. Please comment it out > and see if the machine locks up. Kirkwood just stops dead if you > access registers when there clocks are disabled. For Orion5x, the > ethernet should always have a clock. > Will do. >> + >> + /* store MAC address register contents in local-mac-address */ >> + pmac = kzalloc(sizeof(*pmac) + 6, GFP_KERNEL); >> + if (!pmac) >> + goto eth_fixup_no_mem; >> + >> + pmac->value = pmac + 1; >> + pmac->length = ETH_ALEN; >> + pmac->name = kstrdup("local-mac-address", GFP_KERNEL); >> + if (!pmac->name) { >> + kfree(pmac); >> + goto eth_fixup_no_mem; >> + } >> + >> + macaddr = pmac->value; >> + for (i = 0; i < ETH_ALEN; i++) >> + macaddr[i] = addr[i]; >> + >> + of_update_property(np, pmac); >> + >> +eth_fixup_no_mem: >> + clk_disable_unprepare(clk); >> + clk_put(clk); >> +eth_fixup_skip: >> + of_node_put(pnp); >> + } >> +} >> + >> +void __init dns323_init_dt(void) >> +{ >> + if (of_machine_is_compatible("dlink,dns323a1")) { >> + writel(0, MPP_DEV_CTRL); /* DEV_D[31:16] */ > > I spotted this in dns323-setup.c as well. Do you have any idea what it > does? > No idea. I have tried to replicate what was in dns323-setup.c as exactly as possible. I can try to leave it out and see if anything changes.
On 23.9.2022 00:39, Arnd Bergmann wrote: > On Thu, Sep 22, 2022, at 10:24 PM, Mauri Sandberg wrote: >> + >> +/* dns323_parse_hex_*() taken from tsx09-common.c; should a common >> copy of these >> + * functions be kept somewhere? >> + */ >> +static int __init dns323_parse_hex_nibble(char n) >> +{ >> + if (n >= '0' && n <= '9') >> + return n - '0'; >> + >> + if (n >= 'A' && n <= 'F') >> + return n - 'A' + 10; >> + >> + if (n >= 'a' && n <= 'f') >> + return n - 'a' + 10; >> + >> + return -1; >> +} >> + >> +static int __init dns323_parse_hex_byte(const char *b) >> +{ >> + int hi; >> + int lo; >> + >> + hi = dns323_parse_hex_nibble(b[0]); >> + lo = dns323_parse_hex_nibble(b[1]); >> + >> + if (hi < 0 || lo < 0) >> + return -1; >> + >> + return (hi << 4) | lo; >> +} >> + > > Can you use simple_strntoull() to parse the address into a u64 instead? > Nice idea. Its current replacement seems to be kstrtoull(). I'll have to do it byte by byte, right? Or what do you have in mind with 64bit unsigned? >> +static int __init dns323_read_mac_addr(u8 *addr) >> +{ >> + int i; >> + char *mac_page; >> + >> + /* MAC address is stored as a regular ol' string in /dev/mtdblock4 >> + * (0x007d0000-0x00800000) starting at offset 196480 (0x2ff80). >> + */ >> + mac_page = ioremap(DNS323_NOR_BOOT_BASE + 0x7d0000 + 196480, 1024); >> + if (!mac_page) >> + return -ENOMEM; > > It would be nicer to use of_iomap() on the nor device than a > hardcoded physical address here, at least if that doesn't add > too much extra complexity. > I'll look into this.
On 22/09/2022 22:24, Mauri Sandberg wrote: > Add a device tree for D-Link DNS-323. The device has three different > variants; A1, B1 and C1. Common parts are included in a .dtsi file > and each hardware variant has its own .dts file. Thank you for your patch. There is something to discuss/improve. > + > +/delete-node/ &sata; > + > +&gpio0 { > + pinctrl-0 = <&pmx_gpio_misc>; > + pinctrl-names = "default"; > + > + /* The DNS323 rev A1 power LED requires GPIO 4 to be low. */ > + pin_gpio0_4 { No underscores in node names. > + gpio-hog; > + gpios = <4 GPIO_ACTIVE_LOW>; > + output-high; > + line-name = "Power led enable"; > + }; > +}; > + > +&pmx_gpio_misc { > + marvell,pins = "mpp4"; > +}; > + > +&pmx_ge { > + marvell,pins = "mpp11", "mpp12", "mpp13", "mpp14", "mpp15", > + "mpp16", "mpp17", "mpp18", "mpp19"; > +}; > diff --git a/arch/arm/boot/dts/orion5x-dlink-dns323b1.dts b/arch/arm/boot/dts/orion5x-dlink-dns323b1.dts > new file mode 100644 > index 000000000000..e01ba809ffca > --- /dev/null > +++ b/arch/arm/boot/dts/orion5x-dlink-dns323b1.dts > @@ -0,0 +1,39 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2022 Mauri Sandberg <maukka@ext.kapsi.fi> > + * > + */ > + > +/dts-v1/; > + > +#include <dt-bindings/gpio/gpio.h> > +#include <dt-bindings/input/input.h> > +#include <dt-bindings/leds/common.h> > +#include "orion5x-mv88f5182.dtsi" > +#include "orion5x-dlink-dns323.dtsi" > + > +/ { > + model = "D-Link DNS-323 rev B1"; > + compatible = "dlink,dns323b1", "dlink,dns323", "marvell,orion5x-88f5182", > + "marvell,orion5x"; > +}; > + > +&gpio0 { > + pinctrl-0 = <&pmx_gpio_misc>; > + pinctrl-names = "default"; > + > + /* The rev B1 has a flag to indicate the system is up. > + * Without this flag set, power LED will flash and cannot be > + * controlled via gpio-leds. > + */ > + pin_gpio0_3 { No underscores in node names. Best regards, Krzysztof
On Fri, Sep 23, 2022, at 11:13 AM, maukka@ext.kapsi.fi wrote: > On 23.9.2022 00:39, Arnd Bergmann wrote: >> Can you use simple_strntoull() to parse the address into a u64 instead? >> > Nice idea. Its current replacement seems to be kstrtoull(). Right. > I'll have to do > it byte by byte, right? Or what do you have in mind with 64bit unsigned? I misread your code and assumed it was stored as a 12-digit hexadecimal number, but you are right: with the colons you still need to iterate the bytes. I looked a little further and found the mac_pton() function that I think does exactly what you need. Arnd
> > > + if (of_machine_is_compatible("dlink,dns323a1")) { > > > + writel(0, MPP_DEV_CTRL); /* DEV_D[31:16] */ > > > > I spotted this in dns323-setup.c as well. Do you have any idea what it > > does? > > > > No idea. I have tried to replicate what was in dns323-setup.c as exactly as > possible. > I can try to leave it out and see if anything changes. It is best to keep what we don't understand. It will be there for a reason. Andrew
On Friday 23 September 2022 14:12:24 Andrew Lunn wrote: > > > > + if (of_machine_is_compatible("dlink,dns323a1")) { > > > > + writel(0, MPP_DEV_CTRL); /* DEV_D[31:16] */ > > > > > > I spotted this in dns323-setup.c as well. Do you have any idea what it > > > does? > > > > > > > No idea. I have tried to replicate what was in dns323-setup.c as exactly as > > possible. > > I can try to leave it out and see if anything changes. > > It is best to keep what we don't understand. It will be there for a > reason. > > Andrew Hello! I tried to index all publicly available Marvell SoC documentations into kernel documentation subfolder: https://docs.kernel.org/arm/marvell.html For Orion there is linked Datasheet and User Manual, so you could try to find in those documents that mentioned register and check what it is doing.
On 23.9.2022 21:02, Pali Rohár wrote: > On Friday 23 September 2022 14:12:24 Andrew Lunn wrote: >> > > > + if (of_machine_is_compatible("dlink,dns323a1")) { >> > > > + writel(0, MPP_DEV_CTRL); /* DEV_D[31:16] */ >> > > >> > > I spotted this in dns323-setup.c as well. Do you have any idea what it >> > > does? >> > > >> > >> > No idea. I have tried to replicate what was in dns323-setup.c as exactly as >> > possible. >> > I can try to leave it out and see if anything changes. >> >> It is best to keep what we don't understand. It will be there for a >> reason. >> >> Andrew > > Hello! I tried to index all publicly available Marvell SoC > documentations into kernel documentation subfolder: > https://docs.kernel.org/arm/marvell.html > > For Orion there is linked Datasheet and User Manual, so you could try > to > find in those documents that mentioned register and check what it is > doing. MPP_DEV_CTRL refers to register at address 0x10008. According to the 88F5152 user manual it's 'Device Multiplex Control Register' Offset: 0x10008. Bits Field Type/InitVal Description [31:0] Reserved RES 0x03FF0000 Reserved. NOTE: Must be 0x03FF0000'. DEV_D[31:16] receives no hits in the documentation, only to DEV_D[15:0] are referred. Maybe 88F5151 is different, hard to say.
Hello Elad! I hope that would not bothering you. We are doing here cleanup of kernel code for older Marvell SoCs (Orion) and there one unknown thing about 88F5181's 0x10008 register. See below. On Monday 26 September 2022 14:56:48 maukka@ext.kapsi.fi wrote: > On 23.9.2022 21:02, Pali Rohár wrote: > > On Friday 23 September 2022 14:12:24 Andrew Lunn wrote: > > > > > > + if (of_machine_is_compatible("dlink,dns323a1")) { > > > > > > + writel(0, MPP_DEV_CTRL); /* DEV_D[31:16] */ > > > > > > > > > > I spotted this in dns323-setup.c as well. Do you have any idea what it > > > > > does? > > > > > > > > > > > > > No idea. I have tried to replicate what was in dns323-setup.c as exactly as > > > > possible. > > > > I can try to leave it out and see if anything changes. > > > > > > It is best to keep what we don't understand. It will be there for a > > > reason. > > > > > > Andrew > > > > Hello! I tried to index all publicly available Marvell SoC > > documentations into kernel documentation subfolder: > > https://docs.kernel.org/arm/marvell.html > > > > For Orion there is linked Datasheet and User Manual, so you could try to > > find in those documents that mentioned register and check what it is > > doing. > > MPP_DEV_CTRL refers to register at address 0x10008. According to the 88F5152 > user manual it's > 'Device Multiplex Control Register' Offset: 0x10008. > > Bits Field Type/InitVal Description > [31:0] Reserved RES 0x03FF0000 Reserved. NOTE: Must be 0x03FF0000'. > > DEV_D[31:16] receives no hits in the documentation, only to DEV_D[15:0] are > referred. In linked public document I found same thing. Register is for 88F5182 reserved. (You have typo in comment, it is 88F5182, not *52). > Maybe 88F5151 is different, hard to say. I have feeling that for 88F5181 it is not reserved and has to be configured correctly. (Also typo in your comment, it is 88F5181, not *51). But I have not found any copy of 88F5181 user manual document on internet. In past 88F518x and 88F528x documents and user manuals were available publicly on Marvell website, visible from web archive: https://web.archive.org/web/20080607215437/http://www.marvell.com/products/media/index.jsp But Marvell deleted these documents from their public website and for kernel developers they are now probably lost. I do not know about any other backups. Elad, could you please help us? Do you have access to functional specifications / user manuals for 88F518x and 88F528x or have information how kernel developers can get access to those documents? Hopefully they were not totally lost. We just need explanation for register 'Device Multiplex Control Register' Offset: 0x10008.
Hi Pali, I do not have documentation for this controller, as it is almost 20 years old... I did manage, however, to find some very old u-boot code for it. From reverse engineering this u-boot code, it looks like this is a "DEV" MPP function register, similar to the MPP0_7, MPP8_15 and the MPP16_23 registers. Basically, setting bits of this registers assign the pin to the special purpose, while clearing it makes it a GPP (General Purpose Pin). For all of the boards (over half a dozen) support by this u-boot, this register is set to zero (see above). From user guides I have found for few of these boards, only MPPs up to MPP19 are used, hence it make sense to leave these MPPs as GPPs . Hopefully this helps in some way. FYI, Elad. -----Original Message----- From: Pali Rohár <pali@kernel.org> Sent: Monday, September 26, 2022 3:23 PM To: Elad Nachman <enachman@marvell.com> Cc: maukka@ext.kapsi.fi; Andrew Lunn <andrew@lunn.ch>; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; arnd@arndb.de; olof@lixom.net; sebastian.hesselbarth@gmail.com; gregory.clement@bootlin.com; linux@armlinux.org.uk; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org Subject: [EXT] Re: [PATCH v2 3/3] ARM: orion5x: Add D-Link DNS-323 based on Device Tree External Email ---------------------------------------------------------------------- Hello Elad! I hope that would not bothering you. We are doing here cleanup of kernel code for older Marvell SoCs (Orion) and there one unknown thing about 88F5181's 0x10008 register. See below. On Monday 26 September 2022 14:56:48 maukka@ext.kapsi.fi wrote: > On 23.9.2022 21:02, Pali Rohár wrote: > > On Friday 23 September 2022 14:12:24 Andrew Lunn wrote: > > > > > > + if (of_machine_is_compatible("dlink,dns323a1")) { > > > > > > + writel(0, MPP_DEV_CTRL); /* DEV_D[31:16] */ > > > > > > > > > > I spotted this in dns323-setup.c as well. Do you have any idea > > > > > what it does? > > > > > > > > > > > > > No idea. I have tried to replicate what was in dns323-setup.c as > > > > exactly as possible. > > > > I can try to leave it out and see if anything changes. > > > > > > It is best to keep what we don't understand. It will be there for > > > a reason. > > > > > > Andrew > > > > Hello! I tried to index all publicly available Marvell SoC > > documentations into kernel documentation subfolder: > > https://urldefense.proofpoint.com/v2/url?u=https-3A__docs.kernel.org > > _arm_marvell.html&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=eTeNTLEK5-TxXc > > zjOcKPhANIFtlB9pP4lq9qhdlFrwQ&m=QnvtICgYrknBcrJ4SBYkL8zUxNtqo3A40bjE > > TmCHhBbdWQOUaRffkiMgtuRkQ2WE&s=QiNvxcOSpDNOTgiK8nuCZ18pgJRKBtgVu-SeG > > E9n7CY&e= > > > > For Orion there is linked Datasheet and User Manual, so you could > > try to find in those documents that mentioned register and check > > what it is doing. > > MPP_DEV_CTRL refers to register at address 0x10008. According to the > 88F5152 user manual it's 'Device Multiplex Control Register' Offset: > 0x10008. > > Bits Field Type/InitVal Description > [31:0] Reserved RES 0x03FF0000 Reserved. NOTE: Must be 0x03FF0000'. > > DEV_D[31:16] receives no hits in the documentation, only to > DEV_D[15:0] are referred. In linked public document I found same thing. Register is for 88F5182 reserved. (You have typo in comment, it is 88F5182, not *52). > Maybe 88F5151 is different, hard to say. I have feeling that for 88F5181 it is not reserved and has to be configured correctly. (Also typo in your comment, it is 88F5181, not *51). But I have not found any copy of 88F5181 user manual document on internet. In past 88F518x and 88F528x documents and user manuals were available publicly on Marvell website, visible from web archive: https://urldefense.proofpoint.com/v2/url?u=https-3A__web.archive.org_web_20080607215437_http-3A__www.marvell.com_products_media_index.jsp&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=eTeNTLEK5-TxXczjOcKPhANIFtlB9pP4lq9qhdlFrwQ&m=QnvtICgYrknBcrJ4SBYkL8zUxNtqo3A40bjETmCHhBbdWQOUaRffkiMgtuRkQ2WE&s=k1vn2-NVEU2OsJYVTmuWMRKdN2t1MQ9pduTkGaasU4s&e= But Marvell deleted these documents from their public website and for kernel developers they are now probably lost. I do not know about any other backups. Elad, could you please help us? Do you have access to functional specifications / user manuals for 88F518x and 88F528x or have information how kernel developers can get access to those documents? Hopefully they were not totally lost. We just need explanation for register 'Device Multiplex Control Register' Offset: 0x10008.
Hello Elad! Thank you very much for this information! With information from other resources, I think I understood it. Andrew & Mauri: here is my recap: On https://docs.kernel.org/arm/marvell.html is Datasheet document for 88F5281 SoC (it is different! not 88F5181!) and in its section 3.2 Device Pins Multiplexing it is documented. SoC pins named MPP[0], ... MPP[19] are configured via registers 0x10000, 0x10004 and 0x10050 as documented in User Manual. And SoC pins named DEV_D[16], ... DEV_D[31] are configured via that undocumented register 0x10008. Normally 32 DEV_D pins on 88F5281 SoC are used for Device Bus Interface but when SoC has configured only 16-bit or 8-bit Device Bus or it does not use Device Bus Interface at all then pins DEV_D[16] ... DEV_D[31] can be used as GPIOs. Elad wrote that clearing particular bit _i_ in 0x10008 sets DEV_D[i] on 88F5181 to GPIO but datasheet for 88F5281 says that clearing bit i (value 0x0) sets DEV_D[i] to Device Bus mode. I have no idea if 88F5281 and 88F5181 have inverted logic or if documentation has bugs. But at least it this explanation makes sense for me. So code "writel(0, MPP_DEV_CTRL);" either changes all DEV_D pins to GPIO mode or to Device Bus Interface mode. In most cases Device Bus is used for connecting Parallel NAND or any similar Flash memory device. Mauri, you can check if your board has such memory. Or if it uses GPIOs connected on DEV_D pins. If not then it does not matter how you set that register. Anyway, just for completeness, the "proper" way for using MPP_DEV_CTRL should have been in pinctrl/mvebu/pinctrl-orion.c driver. But I think it does not make sense to spend another time for this old board to convert this code into proper pinctrl driver. Hopes that this helps to finally understand that old undocumented mystery. On Wednesday 28 September 2022 13:32:27 Elad Nachman wrote: > Hi Pali, > > I do not have documentation for this controller, as it is almost 20 years old... > > I did manage, however, to find some very old u-boot code for it. > > From reverse engineering this u-boot code, it looks like this is a "DEV" MPP function register, similar to the MPP0_7, MPP8_15 and the MPP16_23 registers. > > Basically, setting bits of this registers assign the pin to the special purpose, while clearing it makes it a GPP (General Purpose Pin). > > For all of the boards (over half a dozen) support by this u-boot, this register is set to zero (see above). > From user guides I have found for few of these boards, only MPPs up to MPP19 are used, hence it make sense to leave these MPPs as GPPs . > > Hopefully this helps in some way. > > FYI, > > Elad. > > > -----Original Message----- > From: Pali Rohár <pali@kernel.org> > Sent: Monday, September 26, 2022 3:23 PM > To: Elad Nachman <enachman@marvell.com> > Cc: maukka@ext.kapsi.fi; Andrew Lunn <andrew@lunn.ch>; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; arnd@arndb.de; olof@lixom.net; sebastian.hesselbarth@gmail.com; gregory.clement@bootlin.com; linux@armlinux.org.uk; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: [EXT] Re: [PATCH v2 3/3] ARM: orion5x: Add D-Link DNS-323 based on Device Tree > > External Email > > ---------------------------------------------------------------------- > Hello Elad! I hope that would not bothering you. We are doing here cleanup of kernel code for older Marvell SoCs (Orion) and there one unknown thing about 88F5181's 0x10008 register. See below. > > On Monday 26 September 2022 14:56:48 maukka@ext.kapsi.fi wrote: > > On 23.9.2022 21:02, Pali Rohár wrote: > > > On Friday 23 September 2022 14:12:24 Andrew Lunn wrote: > > > > > > > + if (of_machine_is_compatible("dlink,dns323a1")) { > > > > > > > + writel(0, MPP_DEV_CTRL); /* DEV_D[31:16] */ > > > > > > > > > > > > I spotted this in dns323-setup.c as well. Do you have any idea > > > > > > what it does? > > > > > > > > > > > > > > > > No idea. I have tried to replicate what was in dns323-setup.c as > > > > > exactly as possible. > > > > > I can try to leave it out and see if anything changes. > > > > > > > > It is best to keep what we don't understand. It will be there for > > > > a reason. > > > > > > > > Andrew > > > > > > Hello! I tried to index all publicly available Marvell SoC > > > documentations into kernel documentation subfolder: > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__docs.kernel.org > > > _arm_marvell.html&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=eTeNTLEK5-TxXc > > > zjOcKPhANIFtlB9pP4lq9qhdlFrwQ&m=QnvtICgYrknBcrJ4SBYkL8zUxNtqo3A40bjE > > > TmCHhBbdWQOUaRffkiMgtuRkQ2WE&s=QiNvxcOSpDNOTgiK8nuCZ18pgJRKBtgVu-SeG > > > E9n7CY&e= > > > > > > For Orion there is linked Datasheet and User Manual, so you could > > > try to find in those documents that mentioned register and check > > > what it is doing. > > > > MPP_DEV_CTRL refers to register at address 0x10008. According to the > > 88F5152 user manual it's 'Device Multiplex Control Register' Offset: > > 0x10008. > > > > Bits Field Type/InitVal Description > > [31:0] Reserved RES 0x03FF0000 Reserved. NOTE: Must be 0x03FF0000'. > > > > DEV_D[31:16] receives no hits in the documentation, only to > > DEV_D[15:0] are referred. > > In linked public document I found same thing. Register is for 88F5182 reserved. (You have typo in comment, it is 88F5182, not *52). > > > Maybe 88F5151 is different, hard to say. > > I have feeling that for 88F5181 it is not reserved and has to be configured correctly. (Also typo in your comment, it is 88F5181, not *51). > But I have not found any copy of 88F5181 user manual document on internet. > > In past 88F518x and 88F528x documents and user manuals were available publicly on Marvell website, visible from web archive: > https://urldefense.proofpoint.com/v2/url?u=https-3A__web.archive.org_web_20080607215437_http-3A__www.marvell.com_products_media_index.jsp&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=eTeNTLEK5-TxXczjOcKPhANIFtlB9pP4lq9qhdlFrwQ&m=QnvtICgYrknBcrJ4SBYkL8zUxNtqo3A40bjETmCHhBbdWQOUaRffkiMgtuRkQ2WE&s=k1vn2-NVEU2OsJYVTmuWMRKdN2t1MQ9pduTkGaasU4s&e= > > But Marvell deleted these documents from their public website and for kernel developers they are now probably lost. I do not know about any other backups. > > > Elad, could you please help us? Do you have access to functional specifications / user manuals for 88F518x and 88F528x or have information how kernel developers can get access to those documents? > Hopefully they were not totally lost. We just need explanation for register 'Device Multiplex Control Register' Offset: 0x10008.