[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-dev][PATCH 2/2] virtio-spi: add the device specification
Hi Harald Mommer,
Thank you for your comments and please refer to my following replies.
On 8/10/2023 6:40 PM, Harald Mommer wrote:
Hello,a quick incomplete on. I'm currently with both hands in an attempt to implement a virtio SPI device for our proprietary hypervisor based on the draft specification and your E-Mail. Means I see now some more things while implementing.u32 len; /* @len: transfer length.*/ is this a byte or a word count? The comment in Linux for the length field in struct spi_ioc_transfer is/* @len: Length of tx and rx buffers, in bytes. */ so I assume this here is also a byte count.As we have only one len I think it's still needed to look at the device readable and device writable descriptor sizes to decide on half duplex read, half duplex write or full duplex. Having still to do this the transfer length in the len field may be redundant (superfluous) information.
"len" is byte count.You are right, "len" parameter seems redundant. Virtqueue used to pass parameters between front-end and back-end and its descriptor is defined as follows(refer to: https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/listings/virtio_queue.h):
struct virtq_desc {
/* Address (guest-physical). */
le64 addr;
/* Length. */
le32 len;
/* The flags as indicated above. */
le16 flags;
/* We chain unused descriptors via this, too */
le16 next;
};
For the rx_buf or tx_buf descriptor, len member shows the transfer
length, so it is not necessary to pass "len" in the head structure.
I will remove "len" in next version.
No problem with this, I'm now only at a point where I want to be sure whether this is meant as a byte length (vs. a word length).Then there is no u32. There is le32. Only RPMB uses be32 but they have a special reason to do this. The byte order must be defined for 16 and 32 bit values!
Yes! Use "le32" instead of "u32" in next patch.
On 20.07.23 09:50, Haixu Cui wrote:May become "struct spi_transfer_result result" for naming reasons with same content, see below. Just a proposal which may not be followed, see below.Hi Harald Mommer, ÂÂÂ Thank you so much for all your helpful advice and info! ÂÂÂ I have updated the transfer request as follows: ÂÂÂ struct spi_transfer_head { ÂÂÂÂÂÂÂÂÂ u8 slave_id; ÂÂÂÂÂÂÂÂÂ u8 bits_per_word; ÂÂÂÂÂÂÂÂÂ u8 cs_change; ÂÂÂÂÂÂÂÂÂ u8 tx_nbits; ÂÂÂÂÂÂÂÂÂ u8 rx_nbits; ÂÂÂÂÂÂÂÂÂ u8 reserved[3]; ÂÂÂÂÂÂÂÂÂ u32 len; ÂÂÂÂÂÂÂÂÂ u32 mode; ÂÂÂÂÂÂÂÂÂ u32 freq; ÂÂÂÂÂÂÂÂÂ u32 word_delay_ns; ÂÂÂÂÂÂÂÂÂ u32 cs_setup_ns; ÂÂÂÂÂÂÂÂÂ u32 cs_delay_hold_ns; ÂÂÂÂÂÂÂÂÂ u32 cs_change_delay_inactive_ns; ÂÂÂ }; ÂÂÂ struct spi_transfer_end { ÂÂÂÂÂÂÂÂÂ u8 result; ÂÂÂ };ÂÂÂ struct virtio_spi_transfer_req { ÂÂÂÂÂÂÂ struct spi_transfer_head head; ÂÂÂÂÂÂÂ u8 rx_buf[]; ÂÂÂÂÂÂÂ u8 tx_buf[]; ÂÂÂÂÂÂÂ struct spi_end end;Above this was "struct spi_transfer_end"ÂÂÂ };
Agreed! Will change to "struct spi_transfer_result".
Device readable must go before device writable. So rx_buf[] and tx_buf[] still have to be swapped.
Yes, rx_buf and tx_buf should be swapped.
I need to separate struct virtio_spi_transfer_req in my C implementation to struct virtio_spi_transfer_req_out { /* Device readable */ ÂÂ Âstruct spi_transfer_head head;ÂÂ Âu8 tx_buf[]; /* Variable array at the end of the structure, gcc is happy */}; struct virtio_spi_transfer_req_in { /* Device writable */ ÂÂÂ u8 rx_buf[];ÂÂÂ /* "struct spi_transfer_end result;" is behind rx_buf but variable array must be last element for gcc for reasons */};Means you have to calculate the address where the result code is to be written from some length information. Can be done. But then I should be sure about the address. And this I can only be after all the length information (head, device writable) are checked for consistency. Clumsy and asking for mistakes.However what also could be done (but you may have to obey alignment requirement for rx_buf[] when words are transferred and may have to add some padding in "struct spi_transfer_result", I don't know this currently):struct virtio_spi_transfer_req_in { /* Device writable */ÂÂÂ struct spi_transfer_result result; /* The result code is the 1st device writable byte */ÂÂÂ u8 rx_buf[]; };With this definition there was no need to calculate the address of the result byte. Just a thought to make life easier.
As I just mentioned, front-end sends the buffer base address and transfer length to the back-end using virtq_desc structure, rather than sending the data directly. And the virtq_desc length is always 16 bytes, so there has nothing to do with the sending order.
Also I add the member and field definition as follows: ÂÂÂ @slave_id: chipselect index the SPI transfer used. ÂÂÂ @bits_per_word: the number of bits in each SPI transfer word. ÂÂÂ @cs_change: whether to deselect device after finishing this transfer ÂÂÂÂÂÂÂ before starting the next transfer, 0 means cs keep asserted and ÂÂÂÂÂÂÂ 1 means cs deasserted then asserted again. ÂÂÂ @tx_nbits: bus width for write transfer. ÂÂÂÂÂÂÂ 0,1: bus width is 1, also known as SINGLE ÂÂÂÂÂÂÂ 2Â : bus width is 2, also known as DUAL ÂÂÂÂÂÂÂ 4Â : bus width is 4, also known as QUAD ÂÂÂÂÂÂÂ 8Â : bus width is 8, also known as OCTAL ÂÂÂÂÂÂÂ other values are invalid. ÂÂÂ @rx_nbits: bus width for read transfer. ÂÂÂÂÂÂÂ 0,1: bus width is 1, also known as SINGLE ÂÂÂÂÂÂÂ 2Â : bus width is 2, also known as DUAL ÂÂÂÂÂÂÂ 4Â : bus width is 4, also known as QUAD ÂÂÂÂÂÂÂ 8Â : bus width is 8, also known as OCTAL ÂÂÂÂÂÂÂ other values are invalid. ÂÂÂ @reserved: for future use. ÂÂÂ @len: transfer length.Improve comment!
Actually, it is byte count. len will be removed.
ÂÂÂ @mode: SPI transfer mode. ÂÂÂÂÂÂÂ bit 0: CPHA, determines the timing (i.e. phase) of the data ÂÂÂÂÂÂÂÂÂÂÂ bits relative to the clock pulses.For CPHA=0, the ÂÂÂÂÂÂÂÂÂÂÂ "out" side changes the data on the trailing edge of the ÂÂÂÂÂÂÂÂÂÂÂ preceding clock cycle, while the "in" side captures the data ÂÂÂÂÂÂÂÂÂÂÂ on (or shortly after) the leading edge of the clock cycle. ÂÂÂÂÂÂÂÂÂÂÂ For CPHA=1, the "out" side changes the data on the leading ÂÂÂÂÂÂÂÂÂÂÂ edge of the current clock cycle, while the "in" side ÂÂÂÂÂÂÂÂÂÂÂ captures the data on (or shortly after) the trailing edge of ÂÂÂÂÂÂÂÂÂÂÂ the clock cycle. ÂÂÂÂÂÂÂ bit 1: CPOL, determines the polarity of the clock. CPOL=0 is a ÂÂÂÂÂÂÂÂÂÂÂ clock which idles at 0, and each cycle consists of a pulse ÂÂÂÂÂÂÂÂÂÂÂ of 1. CPOL=1 is a clock which idles at 1, and each cycle ÂÂÂÂÂÂÂÂÂÂÂ consists of a pulse of 0. ÂÂÂÂÂÂÂ bit 2: CS_HIGH, if 1, chip select active high, else active low. ÂÂÂÂÂÂÂ bit 3: LSB_FIRST, determines per-word bits-on-wire, if 0, MSB ÂÂÂÂÂÂÂÂÂÂÂ first, else LSB first. ÂÂÂÂÂÂÂ bit 4: LOOP, loopback mode. ÂÂÂ @freq: the transfer speed in Hz. ÂÂÂ @word_delay_ns: delay to be inserted between consecutive words of a ÂÂÂÂÂÂÂ transfer, in ns unit. ÂÂÂ @cs_setup_ns: delay to be introduced after CS is asserted, in ns ÂÂÂÂÂÂÂ unit. ÂÂÂ @cs_delay_hold_ns: delay to be introduced before CS is deasserted ÂÂÂÂÂÂÂ for each transfer, in ns unit. ÂÂÂ @cs_change_delay_inactive_ns: delay to be introduced after CS is ÂÂÂÂÂÂÂ deasserted and before next asserted, in ns unit.ÂÂÂ Could you please help review the transfer request above again to check if the interface is clear and enough for back-end to perform SPI transfers. Thank you again for your comments.I'm working and I'll come back on this.Best RegardsHaixu CuiRegards Harald Mommer
According to your helpful comments and suggestion, I will submit another patch as soon as possible.
Best Regards & Thanks Haixu Cui
--------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]