Quoting Bjorn Andersson (2019-01-15 23:19:39)
> DMA addresses for devices on the soc bus must be constrained to the 36
> address bits that the bus provides. When no IOMMU is present then this
> is easy--DMA addresses are just physical addresses and physical
> addresses are (by definition) within the address bits of the bus. When
> an IOMMU is present, however, DMA addresses are virtual addresses.
> Despite these addresses being virtual, however, they are still
> constrained by the 36 address bits of the bus.
>
> Unless dma-ranges is specified, which causes bus_dma_mask to be set, DMA
> allocations for devices on the platform_bus will use all 48 address bits
> available by the ARM SMMU. Causing addresses to be truncated on the bus.
I read it three times and still got confused by the statement that DMA
addresses are virtual addresses. There are CPU virtual addresses, DMA
addresses, IOVAs, and physical addresses. Stating that DMA addresses are
virtual addresses makes it sound like we're talking about CPU virtual
addresses.
Maybe it would be clearer if it stated that DMA addresses are 1:1 with
IOVAs (IO virtual addresses) when a device has an iommus property and
1:1 with physical addresses when that property is absent? When devices
use an iommus property the DMA addresses that are generated in the
absence of a dma-ranges property can have as many as 48 bits, as opposed
to the default of 32 bits in the absence of an iommus property.
Therefore we need to constrain the DMA addresses that devices which
reside in the SoC node (including the SMMU) can use to be at most 36
bits because that's the largest physical address the internal bus
supports. This expands the size of DMA addresses that devices without an
iommus property can use while also limiting the size that devices using
SMMU can use.
>
> This patch increases the #size-cells to 2, in order to be able to define
> dma-ranges describe the 36 bit DMA capability of the bus, and bumps
Did the commit text get cut off here?
>
> While touching all reg properties, addresses are padded to 8 digits.
>
I liked the paint the way it was without the padding. It matched the
unit address that way and didn't make anyone think they should pad that
out in the node name with leading zeros so that 'reg' and unit address
match.
> Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> ---
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index c98b1937353b..fc01b1c93fe4 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -346,14 +346,15 @@
> };
>
> soc: soc {
> - #address-cells = <1>;
> - #size-cells = <1>;
> - ranges = <0 0 0 0xffffffff>;
> + #address-cells = <2>;
> + #size-cells = <2>;
Do we need #size-cells = <2>? Maybe it could be #size-cells = <1> and we
can avoid having to specify a second size entry that's always going to
be 0 because we don't have devices with huge IO regions in the SoC. Or
does it need to be 2 for the large size of the size element of
dma-ranges and ranges here? Looking at the code I think we can rely on
the size-cells of the parent node so I think it will work with size of 1
here.
> + ranges = <0 0 0 0 0x10 0>;
> + dma-ranges = <0 0 0 0 0x10 0>;
> compatible = "simple-bus";
It might also be a good idea to split the patch into two. The first
patch would expand the reg properties and the second one would do the
0x10 change and add dma-ranges. Then if anything goes wrong with the
second patch a quick revert is easier and smaller vs. the obviously good
reg property expanding patch that should be a no-op.
> @@ -378,25 +379,25 @@
>
> rng: rng@793000 {
> compatible = "qcom,prng-ee";
> - reg = <0x00793000 0x1000>;
> + reg = <0 0x00793000 0 0x1000>;
> clocks = <&gcc GCC_PRNG_AHB_CLK>;
> clock-names = "core";
> };
>
> qupv3_id_0: geniqup@8c0000 {
> compatible = "qcom,geni-se-qup";
> - reg = <0x8c0000 0x6000>;
> + reg = <0 0x008c0000 0 0x6000>;
> clock-names = "m-ahb", "s-ahb";
> clocks = <&gcc GCC_QUPV3_WRAP_0_M_AHB_CLK>,
> <&gcc GCC_QUPV3_WRAP_0_S_AHB_CLK>;
> - #address-cells = <1>;
> - #size-cells = <1>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> ranges;
> status = "disabled";
>
> i2c0: i2c@880000 {
> compatible = "qcom,geni-i2c";
> - reg = <0x880000 0x4000>;
> + reg = <0 0x00880000 0 0x4000>;
> clock-names = "se";
> clocks = <&gcc GCC_QUPV3_WRAP0_S0_CLK>;
> pinctrl-names = "default";
> @@ -693,18 +694,18 @@
>
> qupv3_id_1: geniqup@ac0000 {
> compatible = "qcom,geni-se-qup";
> - reg = <0xac0000 0x6000>;
> + reg = <0 0x00ac0000 0 0x6000>;
> clock-names = "m-ahb", "s-ahb";
> clocks = <&gcc GCC_QUPV3_WRAP_1_M_AHB_CLK>,
> <&gcc GCC_QUPV3_WRAP_1_S_AHB_CLK>;
> - #address-cells = <1>;
> - #size-cells = <1>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> ranges;
> status = "disabled";
>
> i2c8: i2c@a80000 {
> compatible = "qcom,geni-i2c";
> - reg = <0xa80000 0x4000>;
> + reg = <0 0x00a80000 0 0x4000>;
> clock-names = "se";
> clocks = <&gcc GCC_QUPV3_WRAP1_S0_CLK>;
> pinctrl-names = "default";
> @@ -1044,9 +1045,9 @@
>
> ufs_mem_phy: phy@1d87000 {
> compatible = "qcom,sdm845-qmp-ufs-phy";
> - reg = <0x1d87000 0x18c>;
> - #address-cells = <1>;
> - #size-cells = <1>;
> + reg = <0 0x01d87000 0 0x18c>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> ranges;
> clock-names = "ref",
> "ref_aux";
> @@ -1536,13 +1537,13 @@
>
> usb_1_qmpphy: phy@88e9000 {
> compatible = "qcom,sdm845-qmp-usb3-phy";
> - reg = <0x88e9000 0x18c>,
> - <0x88e8000 0x10>;
> + reg = <0 0x088e9000 0 0x18c>,
> + <0 0x088e8000 0 0x10>;
> reg-names = "reg-base", "dp_com";
> status = "disabled";
> #clock-cells = <1>;
> - #address-cells = <1>;
> - #size-cells = <1>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> ranges;
>
> clocks = <&gcc GCC_USB3_PRIM_PHY_AUX_CLK>,
> @@ -1571,11 +1572,11 @@
>
> usb_2_qmpphy: phy@88eb000 {
> compatible = "qcom,sdm845-qmp-usb3-uni-phy";
> - reg = <0x88eb000 0x18c>;
> + reg = <0 0x088eb000 0 0x18c>;
> status = "disabled";
> #clock-cells = <1>;
> - #address-cells = <1>;
> - #size-cells = <1>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> ranges;
>
> clocks = <&gcc GCC_USB3_SEC_PHY_AUX_CLK>,
> @@ -1602,10 +1603,10 @@
>
> usb_1: usb@a6f8800 {
> compatible = "qcom,sdm845-dwc3", "qcom,dwc3";
> - reg = <0xa6f8800 0x400>;
> + reg = <0 0x0a6f8800 0 0x400>;
> status = "disabled";
> - #address-cells = <1>;
> - #size-cells = <1>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> ranges;
>
> clocks = <&gcc GCC_CFG_NOC_USB3_PRIM_AXI_CLK>,
> @@ -1644,10 +1645,10 @@
>
> usb_2: usb@a8f8800 {
> compatible = "qcom,sdm845-dwc3", "qcom,dwc3";
> - reg = <0xa8f8800 0x400>;
> + reg = <0 0x0a8f8800 0 0x400>;
> status = "disabled";
> - #address-cells = <1>;
> - #size-cells = <1>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> ranges;
>
> clocks = <&gcc GCC_CFG_NOC_USB3_SEC_AXI_CLK>,
> @@ -1686,7 +1687,7 @@
>
> videocc: clock-controller@ab00000 {
> compatible = "qcom,sdm845-videocc";
> - reg = <0x0ab00000 0x10000>;
> + reg = <0 0x0ab00000 0 0x10000>;
> #clock-cells = <1>;
> #power-domain-cells = <1>;
> #reset-cells = <1>;
> @@ -1694,7 +1695,7 @@
>
> mdss: mdss@ae00000 {
> compatible = "qcom,sdm845-mdss";
> - reg = <0x0ae00000 0x1000>;
> + reg = <0 0x0ae00000 0 0x1000>;
> reg-names = "mdss";
>
> power-domains = <&dispcc MDSS_GDSC>;
> @@ -1716,14 +1717,14 @@
>
> status = "disabled";
>
> - #address-cells = <1>;
> - #size-cells = <1>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> ranges;
>
> mdss_mdp: mdp@ae01000 {
> compatible = "qcom,sdm845-dpu";
> - reg = <0x0ae01000 0x8f000>,
> - <0x0aeb0000 0x2008>;
> + reg = <0 0x0ae01000 0 0x8f000>,
> + <0 0x0aeb0000 0 0x2008>;
> reg-names = "mdp", "vbif";
>
> clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
> @@ -2061,85 +2062,85 @@
>
> intc: interrupt-controller@17a00000 {
> compatible = "arm,gic-v3";
> - #address-cells = <1>;
> - #size-cells = <1>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> ranges;
> #interrupt-cells = <3>;
> interrupt-controller;
> - reg = <0x17a00000 0x10000>, /* GICD */
> - <0x17a60000 0x100000>; /* GICR * 8 */
> + reg = <0 0x17a00000 0 0x10000>, /* GICD */
> + <0 0x17a60000 0 0x100000>; /* GICR * 8 */
> interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
>
> gic-its@17a40000 {
> compatible = "arm,gic-v3-its";
> msi-controller;
> #msi-cells = <1>;
> - reg = <0x17a40000 0x20000>;
> + reg = <0 0x17a40000 0 0x20000>;
> status = "disabled";
> };
> };
>
> timer@17c90000 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> ranges;
These could be written with ranges to remap the child nodes into the
address space of the parent. It would be nice to not change these
wrapper nodes and reduce the diff in this patch by using different
ranges properties.
> compatible = "arm,armv7-timer-mem";
> - reg = <0x17c90000 0x1000>;
> + reg = <0 0x17c90000 0 0x1000>;
>
> frame@17ca0000 {
> frame-number = <0>;
> interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
> - reg = <0x17ca0000 0x1000>,
> - <0x17cb0000 0x1000>;
> + reg = <0 0x17ca0000 0 0x1000>,
