Re: [Linaro-uefi] [PATCH v2 5/6] Drivers/Net/MarvellYukon: Add 64-bit DMA support

On 08/26/2016 07:15 PM, Daniil Egranov wrote:

Hi Alan,

On 08/25/2016 05:39 PM, Alan Ott wrote:

...

Add support for 64-bit DMA transfers, since some 64-bit platforms don't have the ability to generate DMA addresses which can fit in 32-bits.

This code came from the FreeBSD driver, the one from which this driver was derived.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk

---

Drivers/Net/MarvellYukonDxe/if_msk.c | 57 ++++++++++++++++++++++++++++----- Drivers/Net/MarvellYukonDxe/if_mskreg.h | 13 ++++++++ 2 files changed, 62 insertions(+), 8 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 9bd0d12..3cab084 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -492,6 +492,7 @@ msk_init_rx_ring ( struct msk_rxdesc *rxd; INTN i; INTN prod;

• INTN nbuf; EFI_STATUS Status; sc_if->msk_cdata.msk_rx_cons = 0;

@@ -500,17 +501,22 @@ msk_init_rx_ring ( rd = &sc_if->msk_rdata; gBS->SetMem (rd->msk_rx_ring, MSK_RX_RING_SZ, 0);

• prod = sc_if->msk_cdata.msk_rx_prod;
• for (i = 0; i < MSK_RX_RING_CNT; i++) {

• for (i = prod = 0; i < MSK_RX_RING_CNT; i++) { rxd = &sc_if->msk_cdata.msk_rxdesc[prod]; gBS->SetMem (&rxd->rx_m, sizeof (MSK_DMA_BUF), 0); rxd->rx_le = &rd->msk_rx_ring[prod];

• Status = msk_newbuf (sc_if, prod);
• if (EFI_ERROR (Status)) {

•  return Status;
  

• } MSK_INC (prod, MSK_RX_RING_CNT); }

• nbuf = MSK_RX_BUF_CNT;
• prod = 0;
• for (i = 0; i < nbuf; i++) {

• Status = msk_newbuf (sc_if, prod);
  

• if (EFI_ERROR (Status)) {
  

•   return Status;
  

• }
  

• MSK_RX_INC(prod, MSK_RX_RING_CNT);
  

• } // Update prefetch unit. sc_if->msk_cdata.msk_rx_prod = MSK_RX_RING_CNT - 1;

@@ -532,6 +538,7 @@ msk_init_tx_ring ( sc_if->msk_cdata.msk_tx_prod = 0; sc_if->msk_cdata.msk_tx_cons = 0; sc_if->msk_cdata.msk_tx_cnt = 0;

• sc_if->msk_cdata.msk_tx_high_addr = 0; rd = &sc_if->msk_rdata; gBS->SetMem (rd->msk_tx_ring, sizeof (struct msk_tx_desc) *

MSK_TX_RING_CNT, 0); @@ -556,6 +563,13 @@ msk_discard_rxbuf ( DEBUG ((EFI_D_NET, "Marvell Yukon: discard rxbuf\n")); +#ifdef MSK_64BIT_DMA

• rxd = &sc_if->msk_cdata.msk_rxdesc[idx];
• rx_le = rxd->rx_le;
• rx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER);
• MSK_INC(idx, MSK_RX_RING_CNT);

+#endif

• rxd = &sc_if->msk_cdata.msk_rxdesc[idx]; DmaBuffer = &rxd->rx_m; rx_le = rxd->rx_le;

@@ -594,6 +608,14 @@ msk_newbuf ( return Status; } +#ifdef MSK_64BIT_DMA

• rx_le = rxd->rx_le;
• rx_le->msk_addr = htole32(MSK_ADDR_HI(PhysAddr));
• rx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER);
• MSK_INC(idx, MSK_RX_RING_CNT);
• rxd = &sc_if->msk_cdata.msk_rxdesc[idx];

+#endif

• gBS->SetMem (&(rxd->rx_m), sizeof (MSK_DMA_BUF), 0); rxd->rx_m.DmaMapping = Mapping; rxd->rx_m.Buf = Buffer;

@@ -1649,6 +1671,19 @@ msk_encap ( control = 0; +#ifdef MSK_64BIT_DMA

• if (MSK_ADDR_HI(BusPhysAddr) !=
• sc_if->msk_cdata.msk_tx_high_addr) {

•  sc_if->msk_cdata.msk_tx_high_addr =
  

•      MSK_ADDR_HI(BusPhysAddr);
  

•  tx_le = &sc_if->msk_rdata.msk_tx_ring[prod];
  

•  tx_le->msk_addr = htole32(MSK_ADDR_HI(BusPhysAddr));
  

•  tx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER);
  

•  sc_if->msk_cdata.msk_tx_cnt++;
  

•  MSK_INC(prod, MSK_TX_RING_CNT);
  

• }

+#endif

• si = prod; tx_le = &sc_if->msk_rdata.msk_tx_ring[prod]; tx_le->msk_addr = htole32 (MSK_ADDR_LO (BusPhysAddr));

@@ -1866,6 +1901,12 @@ msk_rxeof ( break; } +#ifdef MSK_64BIT_DMA

• rxd = &sc_if->msk_cdata.msk_rxdesc[(cons + 1) % MSK_RX_RING_CNT];

+#else

• rxd = &sc_if->msk_cdata.msk_rxdesc[cons];

+#endif

• m = rxd->rx_m;
  Status = msk_newbuf (sc_if, cons);
  if (EFI_ERROR (Status)) {
  

@@ -1910,8 +1951,8 @@ msk_rxeof ( } } while (0);

• MSK_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT);
• MSK_INC (sc_if->msk_cdata.msk_rx_prod, MSK_RX_RING_CNT);

• MSK_RX_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT);
• MSK_RX_INC (sc_if->msk_cdata.msk_rx_prod, MSK_RX_RING_CNT); } static

diff --git a/Drivers/Net/MarvellYukonDxe/if_mskreg.h b/Drivers/Net/MarvellYukonDxe/if_mskreg.h index f0dd05e..6e835f2 100644 --- a/Drivers/Net/MarvellYukonDxe/if_mskreg.h +++ b/Drivers/Net/MarvellYukonDxe/if_mskreg.h @@ -2239,6 +2239,9 @@ struct msk_stat_desc { #define BMU_UDP_CHECK (0x57<<16) // Descr with UDP ext (YUKON only) #define BMU_BBC 0xffff // Bit 15.. 0: Buffer Byte Counter +#if MAX_ADDRESS > 0xffffffff +#define MSK_64BIT_DMA +#endif

This will be a problem with EBC architecture independent approach as it defines the architecture conditional build. To support EBC version of the driver, this condition have to be handled during the run time. Could you take a look on it and see if such build time conditions can be removed?

The 32-bit DMA case of this driver, as far as I can tell, can be treated as an optimization on 32-bit platforms. It saves items in the RX ring. On 32-bit platforms the high addresses will just be zero.[1]

Since the point of EBC drivers is that they don't know whether they are running on a 32- or 64-bit system (since the EBC environment provides a 64-bit interface to the driver), I don't see how this can be detected at runtime. It's part of the point of EBC to not know about the underlying platform. Maybe someone can show me wrong.

Back to your assertion that this will be a problem with EBC, are you sure that's true? Since EBC driver runs in a 64-bit environment, it seems to me that the code above, when built for EBC, will always set MSK_64BIT_DMA.

Failing that, my recommendation is to #define MSK_64BIT_DMA here in all cases (and not depend on the MAX_ADDRESS). I don't think we lose anything there (except loss of a slight optimization on 32-bit platforms).

Alan.

[1] It's unclear to me the effect of setting EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE on a 32-bit platform, because of course on a 32-bit platform, the dual address cycle will never happen (since the it is a requirement that the dual address cycle not happen if the high bits are all zero).