On 10/08/15 17:03, Marc Marí wrote:
> Add fw_cfg DMA interface specfication in the fw_cfg documentation.
>
> Signed-off-by: Marc Marí <markmb@xxxxxxxxxx>
> ---
> Documentation/devicetree/bindings/arm/fw-cfg.txt | 52 +++++++++++++++++++++++-
> 1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt b/Documentation/devicetree/bindings/arm/fw-cfg.txt
> index 953fb64..0633aad 100644
> --- a/Documentation/devicetree/bindings/arm/fw-cfg.txt
> +++ b/Documentation/devicetree/bindings/arm/fw-cfg.txt
> @@ -38,6 +38,9 @@ The presence of the registers can be verified by selecting the "signature" blob
> with key 0x0000, and reading four bytes from the data register. The returned
> signature is "QEMU".
>
> +If the DMA interface is available, then reading the DMA Address Register
> +returns 0x51454d5520434647 ("QEMU CFG" in big-endian format).
> +
marking this for a later argument: (*)
> The outermost protocol (involving the write / read sequences of the control and
> data registers) is expected to be versioned, and/or described by feature bits.
> The interface revision / feature bitmap can be retrieved with key 0x0001. The
> @@ -45,6 +48,51 @@ blob to be read from the data register has size 4, and it is to be interpreted
> as a uint32_t value in little endian byte order. The current value
> (corresponding to the above outer protocol) is zero.
>
> +If bit 1 of the feature bitmap is set, the DMA interface is present. This
> +can be used through the 64-bit wide address register.
> +
> +The address register is in big-endian format. The value for the register is 0
> +at startup and after an operation. A write to the lower half triggers an
> +operation. This means, that operations with 32-bit addresses can be triggered
> +with just one write, whereas operations with 64-bit addresses can be triggered
> +with one 64-bit write or two 32-bit writes, starting with the higher part.
Please sync this language with the "least significant half" / "most
significant half" wording that I requested after your QEMU
[PATCH v4 2/7] fw_cfg DMA interface documentation
and that you implemented in your present QEMU
[PATCH v5 2/6] fw_cfg DMA interface documentation
> +
> +In this register, the physical address of a FWCfgDmaAccess structure in RAM
> +should be written. This is the format of the FWCfgDmaAccess structure:
> +
> +typedef struct FWCfgDmaAccess {
> + uint32_t control;
> + uint32_t length;
> + uint64_t address;
> +} FWCfgDmaAccess;
> +
> +The fields of the structure are in big endian mode, and the field at the lowest
> +address is the "control" field.
> +
> +The "control" field has the following bits:
> + - Bit 0: Error
> + - Bit 1: Read
> + - Bit 2: Skip
> + - Bit 3: Select. The upper 16 bits are the selected index.
> +
> +When an operation is triggered, if the "control" field has bit 3 set, the
> +upper 16 bits are interpreted as an index of a firmware configuration item.
> +This has the same effect as writing the selector register.
> +
> +If the "control" field has bit 1 set, a read operation will be performed.
> +"length" bytes for the current selector and offset will be copied into the
> +physical RAM address specified by the "address" field.
> +
> +If the "control" field has bit 2 set (and not bit 1), a skip operation will be
> +performed. The offset for the current selector will be advanced "length" bytes.
> +
> +To check the result, read the "control" field:
> + error bit set -> something went wrong.
> + all bits cleared -> transfer finished successfully.
> + otherwise -> transfer still in progress (doesn't happen
> + today due to implementation not being async,
> + but may in the future).
> +
> The guest kernel is not expected to use these registers (although it is
> certainly allowed to); the device tree bindings are documented here because
> this is where device tree bindings reside in general.
I guess this is all coming verbatim from the QEMU spec. I think that's
okay, it doesn't speak about selector values etc, only about transport.
I think it's okay (and consistent with the current text) to have all the
details here.
> @@ -56,6 +104,8 @@ Required properties:
> - reg: the MMIO region used by the device.
> * Bytes 0x0 to 0x7 cover the data register.
> * Bytes 0x8 to 0x9 cover the selector register.
> + * With DMA interface enabled: Bytes 0x10 to 0x17 cover the DMA address
> + register.
> * Further registers may be appended to the region in case of future interface
> revisions / feature bits.
>
> @@ -66,7 +116,7 @@ Example:
> #address-cells = <0x2>;
>
> fw-cfg@9020000 {
> + reg = <0x0 0x9020000 0x0 0x18>;
> compatible = "qemu,fw-cfg-mmio";
> - reg = <0x0 0x9020000 0x0 0xa>;
> };
> };
>
Please make this a bit more precise. As already mentioned by Peter (in
another part of the discussion), you can't read the DMA address register
until you know that it exists. Therefore the part I marked with (*)
above is only usable in the ioport-mapped, x86 case for feature
detection. On ARM, what we key off of primarily is the *size* of the
register block, and only if the DMA address register fits in there, can
we read the feature bitmap (for double-checking), and/or read a
signature from the DMA address register.
Therefore, I think the example device tree snippet is okay, but the hunk
above it should sound like:
* Bytes 0x10 to 0x17, if they exist in the region -- and the
feature bitmap confirms the presence of the DMA interface --
cover the DMA address register.
The AAVMF client code does just this. (I.e., checks the size of the
region first, then reads the feature bitmap *without* DMA, then performs
further reads with DMA.)
Thanks
Laszlo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
