On 08/27/2016 12:43 PM, Ard Biesheuvel wrote:
On 27 August 2016 at 17:24, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
On 27 August 2016 at 16:26, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
On 27 August 2016 at 15:30, Alan Ott alan@softiron.co.uk wrote:
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]
You are assuming that the CPU and the PCI host bridge have the same view of memory, which is not generally true on all architectures (although it is on x86). This means that even on a 32-bit platform, the PCI addresses returned by PciIo->Map() could have high bits set, and so 64bitness is fundamentally a property of the device (what the driver writer's guide calls the PCI controller), and of the driver, not of the architecture we happen to be running on.
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.
It could easily be detected at runtime, since sizeof() is not a compile time constant on EBC. But we shouldn't, given the above.
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).
Indeed. As an optimization, I think it should be possible to query the PCI layer whether it supports DMA above 4 GB, and enable the optimization in that case (which will also enable it for X64, since
s/in that case/if it doesn't/
UEFI on that platform typically does not support DMA >4 GB, and maps CPU:PCI 1:1)
Apologies for the repeated reply to self, but it seems the way to implement this according to the spec is to *not* set the attribute when enabling the device, but pass it into AllocateBuffer() when performing the consistent DMA mappings (and per my other email, the instance used for streaming DMA needs to go), and also use EfiPciOperationBusMasterRead64, EfiPciOperationBusMasterWrite64 etc explicitly when calling Map().
Hi Ard,
Make sure you are not confusing the flags for the PCI Root Bridge and the PCI Driver (often called PCI_IO*). It's a bit confusing on the naming, but PciIo->AllocateBuffer() will not take an attribute of EfiPciOperationBusMasterWrite64, because EfiPciOperationBusMasterWrite64 is a EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION, and not a EFI_PCI_IO_PROTOCOL_OPERATION, which is what PciIo->AllocateBuffer() expects.
See sections 13.4 vs 13.2 of version 2.6 of the UEFI specification.
Alan.
But let's wait and see if anyone from the Tianocore community has any advice (I have cc'ed all of you on the email to edk2-devel)
In the mean time, I will ack either approach as long as it does not assume 32-bit PCI on 32-bit architectures.
Great! This whole thing is made worse by the lack of examples in the edk2 source tree. Linux has spoiled us in that respect :)
Alan.