Re: [PATCH V3] arm64: amd-seattle: Adding device tree for AMD Seattle platform

[Suthikulpanit, Suravee]

On 11/13/14 18:29, Arnd Bergmann wrote:
> On Tuesday 28 October 2014 08:36:54 suravee.suthikulpanit@xxxxxxx wrote:
>> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@xxxxxxx>
>>
>> Initial revision of device tree for AMD Seattle platform
>
> Sorry for not looking at this earlier in enough detail.
>
>> +	dma0: dma@0500000 {
>> +		compatible = "arm,pl330", "arm,primecell";
>> +		reg = <0 0x0500000 0 0x1000>;
>> +		interrupts =
>> +			<0 368 4>,
>> +			<0 369 4>,
>> +			<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?
>
> 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.

>
>> +	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.

>
>> +	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>;
>> +		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.

>
>> +	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

>> +	/* 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.

>> +	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.

>
>> +		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.
>
>
>> +		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.
>
>> +			/* 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.
>
>> +	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.
>
> Please use lowercase characters for the address.

I will made the changes accordingly.

Thanks,

Suravee

>
> 	Arnd
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/