From: "Cédric Le Goater" <clg@kaod.org>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>,
"Erik Smit" <erik.lucas.smit@gmail.com>,
"Peter Maydell" <peter.maydell@linaro.org>,
"Andrew Jeffery" <andrew@aj.id.au>,
"Joel Stanley" <joel@jms.id.au>,
"Jason Wang" <jasowang@redhat.com>,
qemu-arm@nongnu.org, qemu-devel@nongnu.org
Subject: Re: [PATCH] ftgmac100: Implement variable descriptor size
Date: Wed, 3 Jun 2020 10:29:45 +0200 [thread overview]
Message-ID: <aae38f77-ebaa-9375-e59b-b78ea075325d@kaod.org> (raw)
In-Reply-To: <902ebbb5-3e9d-6720-745f-8535a5cd25e8@redhat.com>
On 6/3/20 9:08 AM, Philippe Mathieu-Daudé wrote:
> On 6/2/20 6:47 PM, Erik Smit wrote:
>> The hardware supports variable descriptor sizes, configured with the DBLAC
>> register.
>>
>> Most drivers use the default 2*8, which is currently hardcoded in qemu, but
>> the implementation of the driver in Supermicro BMC SMT_X11_158 uses 4*8.
>>
>> --
>> The implementation of the driver in Supermicro BMC SMT_X11_158 adds 4 extra
>> 4-bytes entries:
>> https://github.com/ya-mouse/openwrt-linux-aspeed/blob/master/drivers/net/ftgmac100_26.h#L387-L391
>>
>> And sets DBLAC to 0x44f97:
>> https://github.com/ya-mouse/openwrt-linux-aspeed/blob/master/drivers/net/ftgmac100_26.c#L449
>>
>> There's not a lot of public documentation on this hardware, but the
>> current linux driver shows the meaning of these registers:
>>
>> https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/faraday/ftgmac100.c#L280-L281
>>
>> iowrite32(FTGMAC100_DBLAC_RXDES_SIZE(2) | /* 2*8 bytes RX descs */
>> FTGMAC100_DBLAC_TXDES_SIZE(2) | /* 2*8 bytes TX descs */
>>
>> Without this patch, networking in SMT_X11_158 does not pass data.
>
> Does it really 'pass' *all* the data?
>
> This patch seems incomplete...
>
> IMO you should 1/ declare FTGMAC100Desc as:
>
> typedef struct {
> uint32_t des0;
> uint32_t des1;
> } FTGMAC100Desc;
The TX and RX descriptors have 4 words in the architecture which are
used by HW but software can use bigger size for its own purpose. This
is what is doing the Aspeed SDK.
> 2/ Replace the code using static '2' by dynamic use of
> FTGMAC100_DBLAC_xXDES_SIZE(dblac):
>
> static int ftgmac100_read_bd(FTGMAC100Desc **bd, dma_addr_t addr)
> {
> unsigned bd_idx;
>
> if (dma_memory_read(&address_space_memory, addr, bd, sizeof(*bd))) {
> qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to read descriptor
> @ 0x%"
> HWADDR_PRIx "\n", __func__, addr);
> return -1;
> }
> for (bd_idx = 0; bd_idx< FTGMAC100_DBLAC_RXDES_SIZE(s->dblac);
> bd_idx++) {
> bd[bd_idx]->des0 = le32_to_cpu(bd[bd_idx]->des0);
> bd[bd_idx]->des1 = le32_to_cpu(bd[bd_idx]->des1);
> }
>
> return 0;
> }
>
> Etc...
>
The current ftgmac100_read_bd() and ftgmac100_write_bd() routines model
the HW perspective and they are fine as they are I think.
C.
> Maybe worth introduce the bd_to_cpu()/cpu_to_bd() helpers too
> (respectively calling le32_to_cpu & cpu_to_le32).
>>
>> Signed-off-by: Erik Smit <erik.lucas.smit@gmail.com
>> <mailto:erik.lucas.smit@gmail.com>>
>> ---
>> hw/net/ftgmac100.c | 17 +++++++++++++++--
>> 1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
>> index 25ebee7ec2..1640b24b23 100644
>> --- a/hw/net/ftgmac100.c
>> +++ b/hw/net/ftgmac100.c
>> @@ -79,6 +79,19 @@
>> #define FTGMAC100_APTC_TXPOLL_CNT(x) (((x) >> 8) & 0xf)
>> #define FTGMAC100_APTC_TXPOLL_TIME_SEL (1 << 12)
>>
>> +/*
>> + * DMA burst length and arbitration control register
>> + */
>> +#define FTGMAC100_DBLAC_RXFIFO_LTHR(x) (((x) >> 0) & 0x7)
>> +#define FTGMAC100_DBLAC_RXFIFO_HTHR(x) (((x) >> 3) & 0x7)
>> +#define FTGMAC100_DBLAC_RX_THR_EN (1 << 6)
>> +#define FTGMAC100_DBLAC_RXBURST_SIZE(x) (((x) >> 8) & 0x3)
>> +#define FTGMAC100_DBLAC_TXBURST_SIZE(x) (((x) >> 10) & 0x3)
>> +#define FTGMAC100_DBLAC_RXDES_SIZE(x) (((x) >> 12) & 0xf)
>> +#define FTGMAC100_DBLAC_TXDES_SIZE(x) (((x) >> 16) & 0xf)
>> +#define FTGMAC100_DBLAC_IFG_CNT(x) (((x) >> 20) & 0x7)
>> +#define FTGMAC100_DBLAC_IFG_INC (1 << 23)
>> +
>> /*
>> * PHY control register
>> */
>> @@ -553,7 +566,7 @@ static void ftgmac100_do_tx(FTGMAC100State *s,
>> uint32_t tx_ring,
>> if (bd.des0 & s->txdes0_edotr) {
>> addr = tx_ring;
>> } else {
>> - addr += sizeof(FTGMAC100Desc);
>> + addr += (FTGMAC100_DBLAC_TXDES_SIZE(s->dblac)) * 8;
>
> Extra parenthesis not needed.
>
> After doing 1/ you can now replace '8' by sizeof(FTGMAC100Desc).
>
>> }
>> }
>>
>> @@ -982,7 +995,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc,
>> const uint8_t *buf,
>> if (bd.des0 & s->rxdes0_edorr) {
>> addr = s->rx_ring;
>> } else {
>> - addr += sizeof(FTGMAC100Desc);
>> + addr += (FTGMAC100_DBLAC_RXDES_SIZE(s->dblac)) * 8;
>> }
>> }
>> s->rx_descriptor = addr;
>> --
>> 2.25.1
>
next prev parent reply other threads:[~2020-06-03 8:30 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-02 16:47 [PATCH] ftgmac100: Implement variable descriptor size Erik Smit
2020-06-03 7:08 ` Philippe Mathieu-Daudé
2020-06-03 8:29 ` Cédric Le Goater [this message]
2020-06-03 8:16 ` Cédric Le Goater
2020-06-04 10:54 ` Erik Smit
2020-06-04 11:42 ` Cédric Le Goater
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aae38f77-ebaa-9375-e59b-b78ea075325d@kaod.org \
--to=clg@kaod.org \
--cc=andrew@aj.id.au \
--cc=erik.lucas.smit@gmail.com \
--cc=jasowang@redhat.com \
--cc=joel@jms.id.au \
--cc=peter.maydell@linaro.org \
--cc=philmd@redhat.com \
--cc=qemu-arm@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).