Re: [PATCH v6] SATA: OCTEON: support SATA on OCTEON platform

[Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>]

Hi,

On 03/02/16 15:31, Arnd Bergmann wrote:
> On Wednesday 03 February 2016 14:44:32 Zubair Lutfullah Kakakhel wrote:
>>
>> Possible in the example.
>>
>> I'll update the example to
>>
>>          uctl@118006c000000 {
>>                  compatible = "cavium,octeon-7130-sata-uctl";
>>                  reg = <0x11800 0x6c000000 0x0 0x100>;
>>                  ranges; /* Direct mapping */
>>                  dma-ranges;
>>                  #address-cells = <1>;
>>                  #size-cells = <2>;
>>
>>                  sata: sata@0 {
>>                          compatible = "cavium,octeon-7130-ahci";
>>                          reg = <0x16c00 0x00000000 0x0 0x200>;
>>                          interrupt-parent = <&cibsata>;
>>                          interrupts = <2 4>; /* Bit: 2, level */
>>                  };
>>          };
>
> Sorry, I should have been clearer. What I meant is
>
>
>           uctl@118006c000000 {
>                  compatible = "cavium,octeon-7130-sata-uctl";
>                  reg = <0x11800 0x6c000000 0x0 0x100>;
>                  ranges = <0   0x16c00 0x00000000   0xffffffff>;
>                  dma-ranges;
>                  #address-cells = <1>;
>                  #size-cells = <1>;
>
>                  sata: sata@0 {
>                          compatible = "cavium,octeon-7130-ahci";
>                          reg = <0x00000000 0x200>;
>                          interrupt-parent = <&cibsata>;
>                          interrupts = <2 4>; /* Bit: 2, level */
>                  };
>          };
>
>
> However, I realized that this would break the dma-ranges, if the
> child device is indeed 64-bit DMA capable. When #address-cells and/or
> #size-cells don't match between parent and child, you have to provide
> non-empty ranges as well as dma-ranges, and the dma-ranges for
> #address-cells=<1> would imply only supporting 32-bit DMA.
>
>
> It could still be
>
>           uctl@118006c000000 {
>                  compatible = "cavium,octeon-7130-sata-uctl";
>                  reg = <0x11800 0x6c000000 0x0 0x100>;
>                  ranges = <0 0  0x16c00 0x00000000   1 0>;
>                  dma-ranges;
>                  #address-cells = <1>;
>                  #size-cells = <1>;
>
>                  sata: sata@0 {
>                          compatible = "cavium,octeon-7130-ahci";
>                          reg = <0 0  0x200>;
>                          interrupt-parent = <&cibsata>;
>                          interrupts = <2 4>; /* Bit: 2, level */
>                  };
>          };
>
> to have a ranges property that shows we are only translating one 4GB
> segment of MMIO addresses into the child, but that the child has DMA
> access to the entire CPU address space (including 64-bit wide RAM
> as well as all MMIO).
>

The above might seem simple to the learned eye.

But it really convolutes my head.

The original binding from Cavium indicates direct mapping.

Given the restricted child device register range, your restricted ranges might make sense.

But I can't make sense of the mappings in the manual I have.

Perhaps David Daney can give more insight on what the hardware implements.

¯\_(ツ)_/¯
    |__|
    /  \

Keep in mind. this sata-uctl might seem like a bus driver

But it is only sitting in front of a sata ahci block.

There is little need for over-engineering address ranges.

Hence, I find the original binding simpler and easier to understand when looking at the hardware manual

+	uctl@118006c000000 {
+		compatible = "cavium,octeon-7130-sata-uctl";
+		reg = <0x11800 0x6c000000 0x0 0x100>;
+		ranges; /* Direct mapping */
+		dma-ranges;
+		#address-cells = <2>;
+		#size-cells = <2>;
+
+		sata: sata@16c0000000000 {
+			compatible = "cavium,octeon-7130-ahci";
+			reg = <0x16c00 0x00000000 0x0 0x200>;
+			interrupt-parent = <&cibsata>;
+			interrupts = <2 4>; /* Bit: 2, level */
+		};
+	};

Is it possible to let this example go in as is?

Regards,
ZubairLK

> 	Arnd
>