Message-ID: <55C35194.60308@redhat.com>
Date:	Thu, 6 Aug 2015 14:22:44 +0200
From:	Laszlo Ersek <lersek@...hat.com>
To:	Stefan Hajnoczi <stefanha@...il.com>,
	Marc Marí <markmb@...hat.com>
Cc:	linux-kernel <linux-kernel@...r.kernel.org>,
	Drew <drjones@...hat.com>, "Kevin O'Connor" <kevin@...onnor.net>,
	Gerd Hoffmann <kraxel@...hat.com>
Subject: Re: [PATCH] QEMU fw_cfg DMA interface documentation

On 08/06/15 14:12, Stefan Hajnoczi wrote:
> On Thu, Aug 6, 2015 at 12:03 PM, Marc Marí <markmb@...hat.com> wrote:
>> Add fw_cfg DMA interface specfication in the fw_cfg documentation.
>>
>> Signed-off-by: Marc Marí <markmb@...hat.com>
>> ---
>>  Documentation/devicetree/bindings/arm/fw-cfg.txt | 36 ++++++++++++++++++++++++
>>  1 file changed, 36 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt b/Documentation/devicetree/bindings/arm/fw-cfg.txt
>> index 953fb64..c880eec 100644
>> --- a/Documentation/devicetree/bindings/arm/fw-cfg.txt
>> +++ b/Documentation/devicetree/bindings/arm/fw-cfg.txt
>> @@ -49,6 +49,41 @@ The guest kernel is not expected to use these registers (although it is
> 
> Please update the "=== Revision (Key 0x0001, FW_CFG_ID) ===" section
> to say that currently the revision is 2.

Sorry I haven't started reading the series yet, but this caught my eye
-- can we agree that this field should be a bitmap instead, and not a
counter-like value? I don't insist of course, because for the current
use case both approaches will work. But, for "future proofing", I think
it is useful to express features independently of each other. (See eg.
virtio feature flags.)

Just an idea.

Thanks
Laszlo

> 
>>  certainly allowed to); the device tree bindings are documented here because
>>  this is where device tree bindings reside in general.
>>
>> +Starting from revision 2, a DMA interface has also been added. This can be used
>> +through a write-only, 64-bit wide address register.
>> +
>> +In this register, a pointer to a FWCfgDmaAccess structure can be written, in
> 
> s/pointer/physical RAM address/ is clearer
> 
>> +big endian mode. This is the format of the FWCfgDmaAccess structure:
> 
> Please be explicit about the *order* of 32-bit writes to the 64-bit
> DMA register.
> 
> Big-endian only defines the layout of bits but it doesn't say in which
> order the two 32-bit sub-registers need to be written.
> 
>> +typedef struct FWCfgDmaAccess {
>> +    uint64_t address;
>> +    uint32_t length;
>> +    uint32_t control;
>> +} FWCfgDmaAccess;
>> +
>> +Once the address to this structure has been written, an DMA operation is
>> +started. If the "control" field has value 2, a read operation will be performed.
>> +"length" bytes for the current selector and offset will be mapped into the
>> +address specified by the "address" field.
> 
> "mapped" might be confusing.  "Copied" or "DMAed" is clearer.
> 
>> +If the field "address" has value 0, the read is considered a skip, and
>> +the data is not copied anywhere, but the offset is still incremented.
>> +
>> +To check result, read the control register:
> 
> FWCfgDmaAccess.control is not a register, it's a field.
> 
> s/register/field/
> 
>> +   error bit set     ->  something went wrong.
> 
> Which bit number is the error bit?
> 
>> +   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).
>> +
>> +Target address goes up and transfer length goes down as the transfer
>> +happens, so after a successful transfer the length register is zero
>> +and the address register points right after the memory block written.
> 
> s/register/field/
> 

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

Powered by blists - more mailing lists