arm64: amd-seattle: Adding device tree for AMD Seattle platform
Quoted 26 lines · Arnd Bergmann
Quoted 3 lines · On Tuesday 28 October 2014 08:36:54 [email protected] wrote:
From: Suravee Suthikulpanit <[email protected]>
Initial revision of device tree for AMD Seattle platform
Sorry for not looking at this earlier in enough detail.
Quoted 6 lines
(quoted diff) +6
-0+ dma0: dma@0500000 { + compatible = "arm,pl330", "arm,primecell"; + reg = <0 0x0500000 0 0x1000>; + interrupts = + <0 368 4>, + <0 369 4>,Review
Quoted 10 lines
(quoted diff) +10
-0+ <0 370 4>, + <0 371 4>, + <0 372 4>, + <0 373 4>, + <0 374 4>, + <0 375 4>; + clocks = <&dmaclk_500mhz>; + clock-names = "apb_pclk"; + #dma-cells = <1>; + };Is this device cache-coherent?
Quoted 1 line
Does it support larger than 32-bit DMA addresses?
The pl330 is only 32-bit DMA addressable, and need to be used with the smmu (not yet included here) before it can be used in the system. Therefore, it should be cache coherent by the virtue of the SMMU.
I¹ll remove this until the SMMU stuff is tested and ready.
Quoted 12 lines
Review
Quoted 9 lines
(quoted diff) +8
-0+ sata0: sata@00300000 { + compatible = "snps,dwc-ahci"; + reg = <0 0x300000 0 0x800>; + interrupts = <0 355 4>; + clocks = <&sataclk_333mhz>; + clock-names = "apb_pclk"; + dma-coherent; + };Same here: you list it as coherent, but not 64-bit DMA capable. Is that intentional?
Correct me if I am wrong, but I didn't think that we need to specify here since the AHCI platform driver determines the DMA bitness by checking struct ahci_host_priv.cap for HOST_CAP_64 (see drivers/ata/libahci_platform.c).
However, based on the conversation on the IRC, I¹ll add the dma-ranges in the motherboard level.
Quoted 20 lines
Quoted 20 lines
(quoted diff) +19
-0+ i2c@1000000 { + compatible = "snps,designware-i2c"; + reg = <0 0x01000000 0 0x1000>; + interrupts = <0 357 4>; + clocks = <&uartspiclk_100mhz>; + clock-names = "apb_pclk"; + }; + + serial0: serial@1010000 { + compatible = "arm,pl011", "arm,primecell"; + reg = <0 0x1010000 0 0x1000>; + interrupts = <0 328 4>; + clocks = <&uartspiclk_100mhz>, <&uartspiclk_100mhz>; + clock-names = "uartclk", "apb_pclk"; + }; + + ssp@1020000 { + compatible = "arm,pl022", "arm,primecell"; + #gpio-cells = <2>;
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ reg = <0 0x1020000 0 0x1000>; + spi-controller; + interrupts = <0 330 4>; + clocks = <&uartspiclk_100mhz>; + clock-names = "apb_pclk"; + };Should these three be connected to the DMA engine?
It doesn't do DMA. Only PCI devices, XGBE, and SATA do DMA.
Quoted 11 lines
Review
Quoted 7 lines
(quoted diff) +6
-0+ ccp: ccp@00100000 { + compatible = "amd,ccp-seattle-v1a"; + reg = <0 0x00100000 0 0x10000>; + interrupts = <0 3 4>; + dma-coherent; + };I see the driver hacks an 48-bit DMA mask into this one. Please fix the driver and add an appropriate dma-ranges property.
ok
Quoted 4 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ /* This entry is modified by UEFI */Can you explain which parts are modified by UEFI?
Actually, I need to remove this comment for now. I believe it was going to be needed for the smmu stuff to specify the stream-id of each end-point device.
Quoted 11 lines
Review
Quoted 9 lines
(quoted diff) +9
-0+ pcie0: pcie-controller{ + compatible = "pci-host-ecam-generic"; + #address-cells = <3>; + #size-cells = <2>; + device_type = "pci"; + bus-range = <0 0xff>; + reg = <0 0xf0000000 0 0x10000000>; + dma-coherent; + msi-parent = <&v2m0>;This surely needs a dma-ranges property to allow larger than 32-bit DMA.
So, I assume this will also need dma-range handling code to be added to the PCI generic host driver.
Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +3
-0+ interrupts = + <0 320 4>, /* ioc_soc_serr */ + <0 321 4>; /* ioc_soc_sci */The pci-host-ecam-generic binding does not allow an interrupts property.
You seem to be missing an interrupt-map property.
I¹ll look into this.
Quoted 9 lines
Review
Quoted 5 lines
(quoted diff) +3
-0+ ranges = + /* I/O Memory (size=64K) */ + <0x01000000 0x00 0xefff0000 0x00 0xefff0000 0x00 0x00010000>,Are you able to map the I/O space to bus address zero instead in the firmware? This looks like a firmware bug, I/O space should not be identity-mapped but is normally expected to have low port numbers.
I¹ll look into this.
Quoted 16 lines
Review
Quoted 12 lines
(quoted diff) +11
-0+ /* Non-Pref 32-bit MMIO (size=512M) */ + <0x02000000 0x00 0x40000000 0x00 0x40000000 0x00 0x20000000>, + + /* Non-Pref 32-bit MMIO (size=512M) */ + <0x02000000 0x00 0x60000000 0x00 0x60000000 0x00 0x20000000>, + + /* Non-Pref 32-bit MMIO (size=512M) */ + <0x02000000 0x00 0x80000000 0x00 0x80000000 0x00 0x20000000>, + + /* Non-Pref 32-bit MMIO (size=512M) */ + <0x02000000 0x00 0xa0000000 0x00 0xa0000000 0x00 0x20000000>,I don't understand why you use distinct ranges here and below. These are all
contiguous, so why not collapse them into one logical range.
I'll collapse them.
Quoted 14 lines
Review
Quoted 9 lines
(quoted diff) +8
-0+ smb { + compatible = "simple-bus"; + #address-cells = <2>; + #size-cells = <2>; + ranges = <0 0 0 0xE0000000 0 0x01300000>; + + /include/ "amd-seattle-periph.dtsi" + };I would put the smb node into the other file and move the include statement to the
top level.
Quoted 1 line
Please use lowercase characters for the address.
I will made the changes accordingly.
Thanks,
Suravee
Quoted 3 lines
Arnd
Quoted 27 lines · Suravee
Quoted 26 lines · Arnd Bergmann
Quoted 3 lines · On Tuesday 28 October 2014 08:36:54 suravee.suthikulpanit at amd.com wrote:
From: Suravee Suthikulpanit <[email protected]>
Initial revision of device tree for AMD Seattle platform
Sorry for not looking at this earlier in enough detail.
Quoted 6 lines
(quoted diff) +6
-0+ dma0: dma at 0500000 { + compatible = "arm,pl330", "arm,primecell"; + reg = <0 0x0500000 0 0x1000>; + interrupts = + <0 368 4>, + <0 369 4>,Review
Quoted 10 lines
(quoted diff) +10
-0+ <0 370 4>, + <0 371 4>, + <0 372 4>, + <0 373 4>, + <0 374 4>, + <0 375 4>; + clocks = <&dmaclk_500mhz>; + clock-names = "apb_pclk"; + #dma-cells = <1>; + };Is this device cache-coherent?
Quoted 7 lines
Quoted 1 line
Does it support larger than 32-bit DMA addresses?
The pl330 is only 32-bit DMA addressable, and need to be used with the smmu (not yet included here) before it can be used in the system. Therefore, it should be cache coherent by the virtue of the SMMU.
I?ll remove this until the SMMU stuff is tested and ready.
Ok, makes sense.
Quoted 13 lines
Quoted 12 lines
Review
Quoted 9 lines
(quoted diff) +8
-0+ sata0: sata at 00300000 { + compatible = "snps,dwc-ahci"; + reg = <0 0x300000 0 0x800>; + interrupts = <0 355 4>; + clocks = <&sataclk_333mhz>; + clock-names = "apb_pclk"; + dma-coherent; + };Same here: you list it as coherent, but not 64-bit DMA capable. Is that intentional?
Quoted 4 lines
Correct me if I am wrong, but I didn't think that we need to specify here since the AHCI platform driver determines the DMA bitness by checking struct ahci_host_priv.cap for
HOST_CAP_64(seedrivers/ata/libahci_platform.c).
No, the actual DMA mask that gets used is the combination of what the device claims to support and what the bus can do. Without the dma-ranges property, the bus will be seen as 32-bit only, so we won't allow high DMA transfers for devices that can do it.
This is the same way we have to treat any PCI device as well, since a lot of PCI devices can do 64-bit DMA, but they can also be connected to a pci host bridge that sits on a 32-bit bus and has no supported IOMMU.
Quoted 2 lines
However, based on the conversation on the IRC, I?ll add the dma-ranges in the motherboard level.
Ok.
Quoted 29 lines
Quoted 20 lines
Quoted 20 lines
(quoted diff) +19
-0+ i2c at 1000000 { + compatible = "snps,designware-i2c"; + reg = <0 0x01000000 0 0x1000>; + interrupts = <0 357 4>; + clocks = <&uartspiclk_100mhz>; + clock-names = "apb_pclk"; + }; + + serial0: serial at 1010000 { + compatible = "arm,pl011", "arm,primecell"; + reg = <0 0x1010000 0 0x1000>; + interrupts = <0 328 4>; + clocks = <&uartspiclk_100mhz>, <&uartspiclk_100mhz>; + clock-names = "uartclk", "apb_pclk"; + }; + + ssp at 1020000 { + compatible = "arm,pl022", "arm,primecell"; + #gpio-cells = <2>;Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ reg = <0 0x1020000 0 0x1000>; + spi-controller; + interrupts = <0 330 4>; + clocks = <&uartspiclk_100mhz>; + clock-names = "apb_pclk"; + };Should these three be connected to the DMA engine?
Quoted 1 line
It doesn't do DMA. Only PCI devices, XGBE, and SATA do DMA.
What is the pl330 connected to then? It's very common for pl011 and pl022 to be used in combination with a pl330 in order to do DMA.
Quoted 12 lines
Quoted 11 lines
Review
Quoted 9 lines
(quoted diff) +9
-0+ pcie0: pcie-controller{ + compatible = "pci-host-ecam-generic"; + #address-cells = <3>; + #size-cells = <2>; + device_type = "pci"; + bus-range = <0 0xff>; + reg = <0 0xf0000000 0 0x10000000>; + dma-coherent; + msi-parent = <&v2m0>;This surely needs a dma-ranges property to allow larger than 32-bit DMA.
Quoted 2 lines
So, I assume this will also need dma-range handling code to be added to the PCI generic host driver.
Yes, good point.
Quoted 1 line
I will made the changes accordingly.
thanks,
Arnd