* [Qemu-devel] [PATCH v2 00/15] i.MX FEC and SD changes
@ 2017-12-14 14:52 Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 01/15] imx_fec: Do not link to netdev Andrey Smirnov
` (14 more replies)
0 siblings, 15 replies; 25+ messages in thread
From: Andrey Smirnov @ 2017-12-14 14:52 UTC (permalink / raw)
To: qemu-arm
Cc: Andrey Smirnov, Peter Maydell, Jason Wang,
Philippe Mathieu-Daudé, qemu-devel, yurovsky
Hi everyone,
This patchset is v2 of a spin-off from original i.MX7 support submission
found here [1], containing all of the patchest that are more or less
agreed upon and are ready (hopefully!) for inclusion.
Changes since [v1]:
- Collected Reviewed-by tags from Peter
- Implemented forgotten feedback from Peter for
"sdhci: Add i.MX specific subtype of SDHCI"
- "imx_fec: Move Tx frame buffer away from the stack" sent out
properly (fingers crossed)
- Added trivial patch to swith i.MX6 over to TYPE_IMX_USDHC
- Added "sd: Check for READ_MULTIPLE_BLOCK size limit
violation first" to address a small problem I was seeing
when tesiting SD against latest vanilla kernel
Changes since [1]:
- Rx buffer in FEC was moved from stack to heap to allow
worry-free expansion to 16K limit.
- Added more comments explaining rather convoluted bit-moving
in eSHDC emuation code
- Triple Tx ring DMA "VMState" code was changed to follow
Peter's recommendations (avoiding the need to incrememnt the
version_id)
- FSL_IMX25_FEC_SIZE is used as a size of FEC's register file
- Removed leftover code from "imx_fec: Change queue flushing
heuristics"
[v1] https://lists.gnu.org/archive/html/qemu-arm/2017-12/msg00085.html
[1] https://lists.gnu.org/archive/html/qemu-arm/2017-11/msg00045.html
Andrey Smirnov (15):
imx_fec: Do not link to netdev
imx_fec: Refactor imx_eth_enable_rx()
imx_fec: Change queue flushing heuristics
imx_fec: Move Tx frame buffer away from the stack
imx_fec: Use ENET_FTRL to determine truncation length
imx_fec: Use MIN instead of explicit ternary operator
imx_fec: Emulate SHIFT16 in ENETx_RACC
imx_fec: Add support for multiple Tx DMA rings
imx_fec: Use correct length for packet size
imx_fec: Fix a typo in imx_enet_receive()
imx_fec: Reserve full FSL_IMX25_FEC_SIZE page for the register file
sdhci: Add i.MX specific subtype of SDHCI
hw: i.MX: Convert i.MX6 to use TYPE_IMX_USDHC
sd: Check for READ_MULTIPLE_BLOCK size limit violation first
sdhci: Implement write method of ACMD12ERRSTS register
hw/arm/fsl-imx6.c | 3 +-
hw/net/imx_fec.c | 210 ++++++++++++++++++++++++++++++++---------
hw/sd/sd.c | 16 ++--
hw/sd/sdhci-internal.h | 19 ++++
hw/sd/sdhci.c | 231 ++++++++++++++++++++++++++++++++++++++++++++-
include/hw/arm/fsl-imx25.h | 1 -
include/hw/net/imx_fec.h | 27 +++++-
include/hw/sd/sdhci.h | 14 +++
8 files changed, 461 insertions(+), 60 deletions(-)
--
2.14.3
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 01/15] imx_fec: Do not link to netdev
2017-12-14 14:52 [Qemu-devel] [PATCH v2 00/15] i.MX FEC and SD changes Andrey Smirnov
@ 2017-12-14 14:52 ` Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 02/15] imx_fec: Refactor imx_eth_enable_rx() Andrey Smirnov
` (13 subsequent siblings)
14 siblings, 0 replies; 25+ messages in thread
From: Andrey Smirnov @ 2017-12-14 14:52 UTC (permalink / raw)
To: qemu-arm
Cc: Andrey Smirnov, Peter Maydell, Jason Wang,
Philippe Mathieu-Daudé, qemu-devel, yurovsky
Binding to a particular netdev doesn't seem to belong to this layer
and should probably be done as a part of board or SoC specific code.
Convert all of the users of this IP block to use
qdev_set_nic_properties() instead.
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
hw/arm/fsl-imx6.c | 1 +
hw/net/imx_fec.c | 2 --
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index 26fd214004..2ed7146c52 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -385,6 +385,7 @@ static void fsl_imx6_realize(DeviceState *dev, Error **errp)
spi_table[i].irq));
}
+ qdev_set_nic_properties(DEVICE(&s->eth), &nd_table[0]);
object_property_set_bool(OBJECT(&s->eth), true, "realized", &err);
if (err) {
error_propagate(errp, err);
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 90e6ee35ba..88b4b049d7 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -1171,8 +1171,6 @@ static void imx_eth_realize(DeviceState *dev, Error **errp)
qemu_macaddr_default_if_unset(&s->conf.macaddr);
- s->conf.peers.ncs[0] = nd_table[0].netdev;
-
s->nic = qemu_new_nic(&imx_eth_net_info, &s->conf,
object_get_typename(OBJECT(dev)),
DEVICE(dev)->id, s);
--
2.14.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 02/15] imx_fec: Refactor imx_eth_enable_rx()
2017-12-14 14:52 [Qemu-devel] [PATCH v2 00/15] i.MX FEC and SD changes Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 01/15] imx_fec: Do not link to netdev Andrey Smirnov
@ 2017-12-14 14:52 ` Andrey Smirnov
2017-12-15 10:06 ` Philippe Mathieu-Daudé
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 03/15] imx_fec: Change queue flushing heuristics Andrey Smirnov
` (12 subsequent siblings)
14 siblings, 1 reply; 25+ messages in thread
From: Andrey Smirnov @ 2017-12-14 14:52 UTC (permalink / raw)
To: qemu-arm
Cc: Andrey Smirnov, Peter Maydell, Jason Wang,
Philippe Mathieu-Daudé, qemu-devel, yurovsky
Refactor imx_eth_enable_rx() to have more meaningfull variable name
than 'tmp' and to reduce number of logical negations done.
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
hw/net/imx_fec.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 88b4b049d7..8b2e4b8ffe 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -536,19 +536,19 @@ static void imx_eth_do_tx(IMXFECState *s)
static void imx_eth_enable_rx(IMXFECState *s)
{
IMXFECBufDesc bd;
- bool tmp;
+ bool rx_ring_full;
imx_fec_read_bd(&bd, s->rx_descriptor);
- tmp = ((bd.flags & ENET_BD_E) != 0);
+ rx_ring_full = !(bd.flags & ENET_BD_E);
- if (!tmp) {
+ if (rx_ring_full) {
FEC_PRINTF("RX buffer full\n");
} else if (!s->regs[ENET_RDAR]) {
qemu_flush_queued_packets(qemu_get_queue(s->nic));
}
- s->regs[ENET_RDAR] = tmp ? ENET_RDAR_RDAR : 0;
+ s->regs[ENET_RDAR] = rx_ring_full ? 0 : ENET_RDAR_RDAR;
}
static void imx_eth_reset(DeviceState *d)
--
2.14.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 03/15] imx_fec: Change queue flushing heuristics
2017-12-14 14:52 [Qemu-devel] [PATCH v2 00/15] i.MX FEC and SD changes Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 01/15] imx_fec: Do not link to netdev Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 02/15] imx_fec: Refactor imx_eth_enable_rx() Andrey Smirnov
@ 2017-12-14 14:52 ` Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 04/15] imx_fec: Move Tx frame buffer away from the stack Andrey Smirnov
` (11 subsequent siblings)
14 siblings, 0 replies; 25+ messages in thread
From: Andrey Smirnov @ 2017-12-14 14:52 UTC (permalink / raw)
To: qemu-arm
Cc: Andrey Smirnov, Peter Maydell, Jason Wang,
Philippe Mathieu-Daudé, qemu-devel, yurovsky
In current implementation, packet queue flushing logic seem to suffer
from a deadlock like scenario if a packet is received by the interface
before before Rx ring is initialized by Guest's driver. Consider the
following sequence of events:
1. A QEMU instance is started against a TAP device on Linux
host, running Linux guest, e. g., something to the effect
of:
qemu-system-arm \
-net nic,model=imx.fec,netdev=lan0 \
netdev tap,id=lan0,ifname=tap0,script=no,downscript=no \
... rest of the arguments ...
2. Once QEMU starts, but before guest reaches the point where
FEC deriver is done initializing the HW, Guest, via TAP
interface, receives a number of multicast MDNS packets from
Host (not necessarily true for every OS, but it happens at
least on Fedora 25)
3. Recieving a packet in such a state results in
imx_eth_can_receive() returning '0', which in turn causes
tap_send() to disable corresponding event (tap.c:203)
4. Once Guest's driver reaches the point where it is ready to
recieve packets it prepares Rx ring descriptors and writes
ENET_RDAR_RDAR to ENET_RDAR register to indicate to HW that
more descriptors are ready. And at this points emulation
layer does this:
s->regs[index] = ENET_RDAR_RDAR;
imx_eth_enable_rx(s);
which, combined with:
if (!s->regs[ENET_RDAR]) {
qemu_flush_queued_packets(qemu_get_queue(s->nic));
}
results in Rx queue never being flushed and corresponding
I/O event beign disabled.
To prevent the problem, change the code to always flush packet queue
when ENET_RDAR transitions 0 -> ENET_RDAR_RDAR.
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
hw/net/imx_fec.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 8b2e4b8ffe..eb034ffd0c 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -533,7 +533,7 @@ static void imx_eth_do_tx(IMXFECState *s)
}
}
-static void imx_eth_enable_rx(IMXFECState *s)
+static void imx_eth_enable_rx(IMXFECState *s, bool flush)
{
IMXFECBufDesc bd;
bool rx_ring_full;
@@ -544,7 +544,7 @@ static void imx_eth_enable_rx(IMXFECState *s)
if (rx_ring_full) {
FEC_PRINTF("RX buffer full\n");
- } else if (!s->regs[ENET_RDAR]) {
+ } else if (flush) {
qemu_flush_queued_packets(qemu_get_queue(s->nic));
}
@@ -807,7 +807,7 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
if (s->regs[ENET_ECR] & ENET_ECR_ETHEREN) {
if (!s->regs[index]) {
s->regs[index] = ENET_RDAR_RDAR;
- imx_eth_enable_rx(s);
+ imx_eth_enable_rx(s, true);
}
} else {
s->regs[index] = 0;
@@ -930,7 +930,7 @@ static int imx_eth_can_receive(NetClientState *nc)
FEC_PRINTF("\n");
- return s->regs[ENET_RDAR] ? 1 : 0;
+ return !!s->regs[ENET_RDAR];
}
static ssize_t imx_fec_receive(NetClientState *nc, const uint8_t *buf,
@@ -1020,7 +1020,7 @@ static ssize_t imx_fec_receive(NetClientState *nc, const uint8_t *buf,
}
}
s->rx_descriptor = addr;
- imx_eth_enable_rx(s);
+ imx_eth_enable_rx(s, false);
imx_eth_update(s);
return len;
}
@@ -1116,7 +1116,7 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
}
}
s->rx_descriptor = addr;
- imx_eth_enable_rx(s);
+ imx_eth_enable_rx(s, false);
imx_eth_update(s);
return len;
}
--
2.14.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 04/15] imx_fec: Move Tx frame buffer away from the stack
2017-12-14 14:52 [Qemu-devel] [PATCH v2 00/15] i.MX FEC and SD changes Andrey Smirnov
` (2 preceding siblings ...)
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 03/15] imx_fec: Change queue flushing heuristics Andrey Smirnov
@ 2017-12-14 14:52 ` Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 05/15] imx_fec: Use ENET_FTRL to determine truncation length Andrey Smirnov
` (10 subsequent siblings)
14 siblings, 0 replies; 25+ messages in thread
From: Andrey Smirnov @ 2017-12-14 14:52 UTC (permalink / raw)
To: qemu-arm
Cc: Andrey Smirnov, Peter Maydell, Jason Wang,
Philippe Mathieu-Daudé, qemu-devel, yurovsky
Make Tx frame assembly buffer to be a paort of IMXFECState structure
to avoid a concern about having large data buffer on the stack.
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
hw/net/imx_fec.c | 22 +++++++++++-----------
include/hw/net/imx_fec.h | 3 +++
2 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index eb034ffd0c..56cb72273c 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -405,8 +405,7 @@ static void imx_eth_update(IMXFECState *s)
static void imx_fec_do_tx(IMXFECState *s)
{
int frame_size = 0, descnt = 0;
- uint8_t frame[ENET_MAX_FRAME_SIZE];
- uint8_t *ptr = frame;
+ uint8_t *ptr = s->frame;
uint32_t addr = s->tx_descriptor;
while (descnt++ < IMX_MAX_DESC) {
@@ -431,8 +430,8 @@ static void imx_fec_do_tx(IMXFECState *s)
frame_size += len;
if (bd.flags & ENET_BD_L) {
/* Last buffer in frame. */
- qemu_send_packet(qemu_get_queue(s->nic), frame, frame_size);
- ptr = frame;
+ qemu_send_packet(qemu_get_queue(s->nic), s->frame, frame_size);
+ ptr = s->frame;
frame_size = 0;
s->regs[ENET_EIR] |= ENET_INT_TXF;
}
@@ -456,8 +455,7 @@ static void imx_fec_do_tx(IMXFECState *s)
static void imx_enet_do_tx(IMXFECState *s)
{
int frame_size = 0, descnt = 0;
- uint8_t frame[ENET_MAX_FRAME_SIZE];
- uint8_t *ptr = frame;
+ uint8_t *ptr = s->frame;
uint32_t addr = s->tx_descriptor;
while (descnt++ < IMX_MAX_DESC) {
@@ -482,13 +480,13 @@ static void imx_enet_do_tx(IMXFECState *s)
frame_size += len;
if (bd.flags & ENET_BD_L) {
if (bd.option & ENET_BD_PINS) {
- struct ip_header *ip_hd = PKT_GET_IP_HDR(frame);
+ struct ip_header *ip_hd = PKT_GET_IP_HDR(s->frame);
if (IP_HEADER_VERSION(ip_hd) == 4) {
- net_checksum_calculate(frame, frame_size);
+ net_checksum_calculate(s->frame, frame_size);
}
}
if (bd.option & ENET_BD_IINS) {
- struct ip_header *ip_hd = PKT_GET_IP_HDR(frame);
+ struct ip_header *ip_hd = PKT_GET_IP_HDR(s->frame);
/* We compute checksum only for IPv4 frames */
if (IP_HEADER_VERSION(ip_hd) == 4) {
uint16_t csum;
@@ -498,8 +496,10 @@ static void imx_enet_do_tx(IMXFECState *s)
}
}
/* Last buffer in frame. */
- qemu_send_packet(qemu_get_queue(s->nic), frame, len);
- ptr = frame;
+
+ qemu_send_packet(qemu_get_queue(s->nic), s->frame, len);
+ ptr = s->frame;
+
frame_size = 0;
if (bd.option & ENET_BD_TX_INT) {
s->regs[ENET_EIR] |= ENET_INT_TXF;
diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
index 62ad473b05..67993870a2 100644
--- a/include/hw/net/imx_fec.h
+++ b/include/hw/net/imx_fec.h
@@ -252,6 +252,9 @@ typedef struct IMXFECState {
uint32_t phy_int_mask;
bool is_fec;
+
+ /* Buffer used to assemble a Tx frame */
+ uint8_t frame[ENET_MAX_FRAME_SIZE];
} IMXFECState;
#endif
--
2.14.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 05/15] imx_fec: Use ENET_FTRL to determine truncation length
2017-12-14 14:52 [Qemu-devel] [PATCH v2 00/15] i.MX FEC and SD changes Andrey Smirnov
` (3 preceding siblings ...)
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 04/15] imx_fec: Move Tx frame buffer away from the stack Andrey Smirnov
@ 2017-12-14 14:52 ` Andrey Smirnov
2017-12-15 10:15 ` Philippe Mathieu-Daudé
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 06/15] imx_fec: Use MIN instead of explicit ternary operator Andrey Smirnov
` (9 subsequent siblings)
14 siblings, 1 reply; 25+ messages in thread
From: Andrey Smirnov @ 2017-12-14 14:52 UTC (permalink / raw)
To: qemu-arm
Cc: Andrey Smirnov, Peter Maydell, Jason Wang,
Philippe Mathieu-Daudé, qemu-devel, yurovsky
Frame truncation length, TRUNC_FL, is determined by the contents of
ENET_FTRL register, so convert the code to use it instead of a
hardcoded constant.
To avoid the case where TRUNC_FL is greater that ENET_MAX_FRAME_SIZE,
increase the value of the latter to its theoretical maximum of 16K.
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
hw/net/imx_fec.c | 4 ++--
include/hw/net/imx_fec.h | 3 ++-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 56cb72273c..50da91bf9e 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -1052,8 +1052,8 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
crc_ptr = (uint8_t *) &crc;
/* Huge frames are truncted. */
- if (size > ENET_MAX_FRAME_SIZE) {
- size = ENET_MAX_FRAME_SIZE;
+ if (size > s->regs[ENET_FTRL]) {
+ size = s->regs[ENET_FTRL];
flags |= ENET_BD_TR | ENET_BD_LG;
}
diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
index 67993870a2..a390d704a6 100644
--- a/include/hw/net/imx_fec.h
+++ b/include/hw/net/imx_fec.h
@@ -86,7 +86,6 @@
#define ENET_TCCR3 393
#define ENET_MAX 400
-#define ENET_MAX_FRAME_SIZE 2032
/* EIR and EIMR */
#define ENET_INT_HB (1 << 31)
@@ -155,6 +154,8 @@
#define ENET_RCR_NLC (1 << 30)
#define ENET_RCR_GRS (1 << 31)
+#define ENET_MAX_FRAME_SIZE (1 << ENET_RCR_MAX_FL_LENGTH)
+
/* TCR */
#define ENET_TCR_GTS (1 << 0)
#define ENET_TCR_FDEN (1 << 2)
--
2.14.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 06/15] imx_fec: Use MIN instead of explicit ternary operator
2017-12-14 14:52 [Qemu-devel] [PATCH v2 00/15] i.MX FEC and SD changes Andrey Smirnov
` (4 preceding siblings ...)
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 05/15] imx_fec: Use ENET_FTRL to determine truncation length Andrey Smirnov
@ 2017-12-14 14:52 ` Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 07/15] imx_fec: Emulate SHIFT16 in ENETx_RACC Andrey Smirnov
` (8 subsequent siblings)
14 siblings, 0 replies; 25+ messages in thread
From: Andrey Smirnov @ 2017-12-14 14:52 UTC (permalink / raw)
To: qemu-arm
Cc: Andrey Smirnov, Peter Maydell, Jason Wang,
Philippe Mathieu-Daudé, qemu-devel, yurovsky
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
hw/net/imx_fec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 50da91bf9e..6feda18742 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -1076,7 +1076,7 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
TYPE_IMX_FEC, __func__);
break;
}
- buf_len = (size <= s->regs[ENET_MRBR]) ? size : s->regs[ENET_MRBR];
+ buf_len = MIN(size, s->regs[ENET_MRBR]);
bd.length = buf_len;
size -= buf_len;
--
2.14.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 07/15] imx_fec: Emulate SHIFT16 in ENETx_RACC
2017-12-14 14:52 [Qemu-devel] [PATCH v2 00/15] i.MX FEC and SD changes Andrey Smirnov
` (5 preceding siblings ...)
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 06/15] imx_fec: Use MIN instead of explicit ternary operator Andrey Smirnov
@ 2017-12-14 14:52 ` Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 08/15] imx_fec: Add support for multiple Tx DMA rings Andrey Smirnov
` (7 subsequent siblings)
14 siblings, 0 replies; 25+ messages in thread
From: Andrey Smirnov @ 2017-12-14 14:52 UTC (permalink / raw)
To: qemu-arm
Cc: Andrey Smirnov, Peter Maydell, Jason Wang,
Philippe Mathieu-Daudé, qemu-devel, yurovsky
Needed to support latest Linux kernel driver which relies on that
functionality.
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
hw/net/imx_fec.c | 23 +++++++++++++++++++++++
include/hw/net/imx_fec.h | 2 ++
2 files changed, 25 insertions(+)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 6feda18742..825c879a28 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -1037,6 +1037,7 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
uint8_t *crc_ptr;
unsigned int buf_len;
size_t size = len;
+ bool shift16 = s->regs[ENET_RACC] & ENET_RACC_SHIFT16;
FEC_PRINTF("len %d\n", (int)size);
@@ -1051,6 +1052,10 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
crc = cpu_to_be32(crc32(~0, buf, size));
crc_ptr = (uint8_t *) &crc;
+ if (shift16) {
+ size += 2;
+ }
+
/* Huge frames are truncted. */
if (size > s->regs[ENET_FTRL]) {
size = s->regs[ENET_FTRL];
@@ -1087,6 +1092,24 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
buf_len += size - 4;
}
buf_addr = bd.data;
+
+ if (shift16) {
+ /*
+ * If SHIFT16 bit of ENETx_RACC register is set we need to
+ * align the payload to 4-byte boundary.
+ */
+ const uint8_t zeros[2] = { 0 };
+
+ dma_memory_write(&address_space_memory, buf_addr,
+ zeros, sizeof(zeros));
+
+ buf_addr += sizeof(zeros);
+ buf_len -= sizeof(zeros);
+
+ /* We only do this once per Ethernet frame */
+ shift16 = false;
+ }
+
dma_memory_write(&address_space_memory, buf_addr, buf, buf_len);
buf += buf_len;
if (size < 4) {
diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
index a390d704a6..af0840a0fa 100644
--- a/include/hw/net/imx_fec.h
+++ b/include/hw/net/imx_fec.h
@@ -170,6 +170,8 @@
#define ENET_TWFR_TFWR_LENGTH (6)
#define ENET_TWFR_STRFWD (1 << 8)
+#define ENET_RACC_SHIFT16 BIT(7)
+
/* Buffer Descriptor. */
typedef struct {
uint16_t length;
--
2.14.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 08/15] imx_fec: Add support for multiple Tx DMA rings
2017-12-14 14:52 [Qemu-devel] [PATCH v2 00/15] i.MX FEC and SD changes Andrey Smirnov
` (6 preceding siblings ...)
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 07/15] imx_fec: Emulate SHIFT16 in ENETx_RACC Andrey Smirnov
@ 2017-12-14 14:52 ` Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 09/15] imx_fec: Use correct length for packet size Andrey Smirnov
` (6 subsequent siblings)
14 siblings, 0 replies; 25+ messages in thread
From: Andrey Smirnov @ 2017-12-14 14:52 UTC (permalink / raw)
To: qemu-arm
Cc: Andrey Smirnov, Peter Maydell, Jason Wang,
Philippe Mathieu-Daudé, qemu-devel, yurovsky
More recent version of the IP block support more than one Tx DMA ring,
so add the code implementing that feature.
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
hw/net/imx_fec.c | 133 ++++++++++++++++++++++++++++++++++++++++-------
include/hw/net/imx_fec.h | 18 ++++++-
2 files changed, 130 insertions(+), 21 deletions(-)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 825c879a28..77d27f763e 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -196,6 +196,31 @@ static const char *imx_eth_reg_name(IMXFECState *s, uint32_t index)
}
}
+/*
+ * Versions of this device with more than one TX descriptor save the
+ * 2nd and 3rd descriptors in a subsection, to maintain migration
+ * compatibility with previous versions of the device that only
+ * supported a single descriptor.
+ */
+static bool imx_eth_is_multi_tx_ring(void *opaque)
+{
+ IMXFECState *s = IMX_FEC(opaque);
+
+ return s->tx_ring_num > 1;
+}
+
+static const VMStateDescription vmstate_imx_eth_txdescs = {
+ .name = "imx.fec/txdescs",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = imx_eth_is_multi_tx_ring,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(tx_descriptor[1], IMXFECState),
+ VMSTATE_UINT32(tx_descriptor[2], IMXFECState),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static const VMStateDescription vmstate_imx_eth = {
.name = TYPE_IMX_FEC,
.version_id = 2,
@@ -203,15 +228,18 @@ static const VMStateDescription vmstate_imx_eth = {
.fields = (VMStateField[]) {
VMSTATE_UINT32_ARRAY(regs, IMXFECState, ENET_MAX),
VMSTATE_UINT32(rx_descriptor, IMXFECState),
- VMSTATE_UINT32(tx_descriptor, IMXFECState),
-
+ VMSTATE_UINT32(tx_descriptor[0], IMXFECState),
VMSTATE_UINT32(phy_status, IMXFECState),
VMSTATE_UINT32(phy_control, IMXFECState),
VMSTATE_UINT32(phy_advertise, IMXFECState),
VMSTATE_UINT32(phy_int, IMXFECState),
VMSTATE_UINT32(phy_int_mask, IMXFECState),
VMSTATE_END_OF_LIST()
- }
+ },
+ .subsections = (const VMStateDescription * []) {
+ &vmstate_imx_eth_txdescs,
+ NULL
+ },
};
#define PHY_INT_ENERGYON (1 << 7)
@@ -406,7 +434,7 @@ static void imx_fec_do_tx(IMXFECState *s)
{
int frame_size = 0, descnt = 0;
uint8_t *ptr = s->frame;
- uint32_t addr = s->tx_descriptor;
+ uint32_t addr = s->tx_descriptor[0];
while (descnt++ < IMX_MAX_DESC) {
IMXFECBufDesc bd;
@@ -447,16 +475,47 @@ static void imx_fec_do_tx(IMXFECState *s)
}
}
- s->tx_descriptor = addr;
+ s->tx_descriptor[0] = addr;
imx_eth_update(s);
}
-static void imx_enet_do_tx(IMXFECState *s)
+static void imx_enet_do_tx(IMXFECState *s, uint32_t index)
{
int frame_size = 0, descnt = 0;
+
uint8_t *ptr = s->frame;
- uint32_t addr = s->tx_descriptor;
+ uint32_t addr, int_txb, int_txf, tdsr;
+ size_t ring;
+
+ switch (index) {
+ case ENET_TDAR:
+ ring = 0;
+ int_txb = ENET_INT_TXB;
+ int_txf = ENET_INT_TXF;
+ tdsr = ENET_TDSR;
+ break;
+ case ENET_TDAR1:
+ ring = 1;
+ int_txb = ENET_INT_TXB1;
+ int_txf = ENET_INT_TXF1;
+ tdsr = ENET_TDSR1;
+ break;
+ case ENET_TDAR2:
+ ring = 2;
+ int_txb = ENET_INT_TXB2;
+ int_txf = ENET_INT_TXF2;
+ tdsr = ENET_TDSR2;
+ break;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: bogus value for index %x\n",
+ __func__, index);
+ abort();
+ break;
+ }
+
+ addr = s->tx_descriptor[ring];
while (descnt++ < IMX_MAX_DESC) {
IMXENETBufDesc bd;
@@ -502,32 +561,32 @@ static void imx_enet_do_tx(IMXFECState *s)
frame_size = 0;
if (bd.option & ENET_BD_TX_INT) {
- s->regs[ENET_EIR] |= ENET_INT_TXF;
+ s->regs[ENET_EIR] |= int_txf;
}
}
if (bd.option & ENET_BD_TX_INT) {
- s->regs[ENET_EIR] |= ENET_INT_TXB;
+ s->regs[ENET_EIR] |= int_txb;
}
bd.flags &= ~ENET_BD_R;
/* Write back the modified descriptor. */
imx_enet_write_bd(&bd, addr);
/* Advance to the next descriptor. */
if ((bd.flags & ENET_BD_W) != 0) {
- addr = s->regs[ENET_TDSR];
+ addr = s->regs[tdsr];
} else {
addr += sizeof(bd);
}
}
- s->tx_descriptor = addr;
+ s->tx_descriptor[ring] = addr;
imx_eth_update(s);
}
-static void imx_eth_do_tx(IMXFECState *s)
+static void imx_eth_do_tx(IMXFECState *s, uint32_t index)
{
if (!s->is_fec && (s->regs[ENET_ECR] & ENET_ECR_EN1588)) {
- imx_enet_do_tx(s);
+ imx_enet_do_tx(s, index);
} else {
imx_fec_do_tx(s);
}
@@ -585,7 +644,7 @@ static void imx_eth_reset(DeviceState *d)
}
s->rx_descriptor = 0;
- s->tx_descriptor = 0;
+ memset(s->tx_descriptor, 0, sizeof(s->tx_descriptor));
/* We also reset the PHY */
phy_reset(s);
@@ -791,6 +850,7 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
unsigned size)
{
IMXFECState *s = IMX_FEC(opaque);
+ const bool single_tx_ring = !imx_eth_is_multi_tx_ring(s);
uint32_t index = offset >> 2;
FEC_PRINTF("reg[%s] <= 0x%" PRIx32 "\n", imx_eth_reg_name(s, index),
@@ -813,10 +873,18 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
s->regs[index] = 0;
}
break;
- case ENET_TDAR:
+ case ENET_TDAR1: /* FALLTHROUGH */
+ case ENET_TDAR2: /* FALLTHROUGH */
+ if (unlikely(single_tx_ring)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "[%s]%s: trying to access TDAR2 or TDAR1\n",
+ TYPE_IMX_FEC, __func__);
+ return;
+ }
+ case ENET_TDAR: /* FALLTHROUGH */
if (s->regs[ENET_ECR] & ENET_ECR_ETHEREN) {
s->regs[index] = ENET_TDAR_TDAR;
- imx_eth_do_tx(s);
+ imx_eth_do_tx(s, index);
}
s->regs[index] = 0;
break;
@@ -828,8 +896,12 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
if ((s->regs[index] & ENET_ECR_ETHEREN) == 0) {
s->regs[ENET_RDAR] = 0;
s->rx_descriptor = s->regs[ENET_RDSR];
- s->regs[ENET_TDAR] = 0;
- s->tx_descriptor = s->regs[ENET_TDSR];
+ s->regs[ENET_TDAR] = 0;
+ s->regs[ENET_TDAR1] = 0;
+ s->regs[ENET_TDAR2] = 0;
+ s->tx_descriptor[0] = s->regs[ENET_TDSR];
+ s->tx_descriptor[1] = s->regs[ENET_TDSR1];
+ s->tx_descriptor[2] = s->regs[ENET_TDSR2];
}
break;
case ENET_MMFR:
@@ -907,7 +979,29 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
} else {
s->regs[index] = value & ~7;
}
- s->tx_descriptor = s->regs[index];
+ s->tx_descriptor[0] = s->regs[index];
+ break;
+ case ENET_TDSR1:
+ if (unlikely(single_tx_ring)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "[%s]%s: trying to access TDSR1\n",
+ TYPE_IMX_FEC, __func__);
+ return;
+ }
+
+ s->regs[index] = value & ~7;
+ s->tx_descriptor[1] = s->regs[index];
+ break;
+ case ENET_TDSR2:
+ if (unlikely(single_tx_ring)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "[%s]%s: trying to access TDSR2\n",
+ TYPE_IMX_FEC, __func__);
+ return;
+ }
+
+ s->regs[index] = value & ~7;
+ s->tx_descriptor[2] = s->regs[index];
break;
case ENET_MRBR:
s->regs[index] = value & 0x00003ff0;
@@ -1203,6 +1297,7 @@ static void imx_eth_realize(DeviceState *dev, Error **errp)
static Property imx_eth_properties[] = {
DEFINE_NIC_PROPERTIES(IMXFECState, conf),
+ DEFINE_PROP_UINT32("tx-ring-num", IMXFECState, tx_ring_num, 1),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
index af0840a0fa..91ef8f89a6 100644
--- a/include/hw/net/imx_fec.h
+++ b/include/hw/net/imx_fec.h
@@ -52,6 +52,8 @@
#define ENET_TFWR 81
#define ENET_FRBR 83
#define ENET_FRSR 84
+#define ENET_TDSR1 89
+#define ENET_TDSR2 92
#define ENET_RDSR 96
#define ENET_TDSR 97
#define ENET_MRBR 98
@@ -66,6 +68,8 @@
#define ENET_FTRL 108
#define ENET_TACC 112
#define ENET_RACC 113
+#define ENET_TDAR1 121
+#define ENET_TDAR2 123
#define ENET_MIIGSK_CFGR 192
#define ENET_MIIGSK_ENR 194
#define ENET_ATCR 256
@@ -105,13 +109,18 @@
#define ENET_INT_WAKEUP (1 << 17)
#define ENET_INT_TS_AVAIL (1 << 16)
#define ENET_INT_TS_TIMER (1 << 15)
+#define ENET_INT_TXF2 (1 << 7)
+#define ENET_INT_TXB2 (1 << 6)
+#define ENET_INT_TXF1 (1 << 3)
+#define ENET_INT_TXB1 (1 << 2)
#define ENET_INT_MAC (ENET_INT_HB | ENET_INT_BABR | ENET_INT_BABT | \
ENET_INT_GRA | ENET_INT_TXF | ENET_INT_TXB | \
ENET_INT_RXF | ENET_INT_RXB | ENET_INT_MII | \
ENET_INT_EBERR | ENET_INT_LC | ENET_INT_RL | \
ENET_INT_UN | ENET_INT_PLR | ENET_INT_WAKEUP | \
- ENET_INT_TS_AVAIL)
+ ENET_INT_TS_AVAIL | ENET_INT_TXF1 | \
+ ENET_INT_TXB1 | ENET_INT_TXF2 | ENET_INT_TXB2)
/* RDAR */
#define ENET_RDAR_RDAR (1 << 24)
@@ -234,6 +243,9 @@ typedef struct {
#define ENET_BD_BDU (1 << 31)
+#define ENET_TX_RING_NUM 3
+
+
typedef struct IMXFECState {
/*< private >*/
SysBusDevice parent_obj;
@@ -246,7 +258,9 @@ typedef struct IMXFECState {
uint32_t regs[ENET_MAX];
uint32_t rx_descriptor;
- uint32_t tx_descriptor;
+
+ uint32_t tx_descriptor[ENET_TX_RING_NUM];
+ uint32_t tx_ring_num;
uint32_t phy_status;
uint32_t phy_control;
--
2.14.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 09/15] imx_fec: Use correct length for packet size
2017-12-14 14:52 [Qemu-devel] [PATCH v2 00/15] i.MX FEC and SD changes Andrey Smirnov
` (7 preceding siblings ...)
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 08/15] imx_fec: Add support for multiple Tx DMA rings Andrey Smirnov
@ 2017-12-14 14:52 ` Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 10/15] imx_fec: Fix a typo in imx_enet_receive() Andrey Smirnov
` (5 subsequent siblings)
14 siblings, 0 replies; 25+ messages in thread
From: Andrey Smirnov @ 2017-12-14 14:52 UTC (permalink / raw)
To: qemu-arm
Cc: Andrey Smirnov, Peter Maydell, Jason Wang,
Philippe Mathieu-Daudé, qemu-devel, yurovsky
Use 'frame_size' instead of 'len' when calling qemu_send_packet(),
failing to do so results in malformed packets send in case when that
packed is fragmented into multiple DMA transactions.
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
hw/net/imx_fec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 77d27f763e..6cb9e2e20e 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -556,7 +556,7 @@ static void imx_enet_do_tx(IMXFECState *s, uint32_t index)
}
/* Last buffer in frame. */
- qemu_send_packet(qemu_get_queue(s->nic), s->frame, len);
+ qemu_send_packet(qemu_get_queue(s->nic), s->frame, frame_size);
ptr = s->frame;
frame_size = 0;
--
2.14.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 10/15] imx_fec: Fix a typo in imx_enet_receive()
2017-12-14 14:52 [Qemu-devel] [PATCH v2 00/15] i.MX FEC and SD changes Andrey Smirnov
` (8 preceding siblings ...)
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 09/15] imx_fec: Use correct length for packet size Andrey Smirnov
@ 2017-12-14 14:52 ` Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 11/15] imx_fec: Reserve full FSL_IMX25_FEC_SIZE page for the register file Andrey Smirnov
` (4 subsequent siblings)
14 siblings, 0 replies; 25+ messages in thread
From: Andrey Smirnov @ 2017-12-14 14:52 UTC (permalink / raw)
To: qemu-arm
Cc: Andrey Smirnov, Peter Maydell, Jason Wang,
Philippe Mathieu-Daudé, qemu-devel, yurovsky
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
hw/net/imx_fec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 6cb9e2e20e..c1cf7f9c58 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -1150,7 +1150,7 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
size += 2;
}
- /* Huge frames are truncted. */
+ /* Huge frames are truncated. */
if (size > s->regs[ENET_FTRL]) {
size = s->regs[ENET_FTRL];
flags |= ENET_BD_TR | ENET_BD_LG;
--
2.14.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 11/15] imx_fec: Reserve full FSL_IMX25_FEC_SIZE page for the register file
2017-12-14 14:52 [Qemu-devel] [PATCH v2 00/15] i.MX FEC and SD changes Andrey Smirnov
` (9 preceding siblings ...)
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 10/15] imx_fec: Fix a typo in imx_enet_receive() Andrey Smirnov
@ 2017-12-14 14:52 ` Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 12/15] sdhci: Add i.MX specific subtype of SDHCI Andrey Smirnov
` (3 subsequent siblings)
14 siblings, 0 replies; 25+ messages in thread
From: Andrey Smirnov @ 2017-12-14 14:52 UTC (permalink / raw)
To: qemu-arm
Cc: Andrey Smirnov, Peter Maydell, Jason Wang,
Philippe Mathieu-Daudé, qemu-devel, yurovsky
Some i.MX SoCs (e.g. i.MX7) have FEC registers going as far as offset
0x614, so to avoid getting aborts when accessing those on QEMU, extend
the register file to cover FSL_IMX25_FEC_SIZE(16K) of address space
instead of just 1K.
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
hw/net/imx_fec.c | 2 +-
include/hw/arm/fsl-imx25.h | 1 -
include/hw/net/imx_fec.h | 1 +
3 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index c1cf7f9c58..4fb48f62ba 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -1281,7 +1281,7 @@ static void imx_eth_realize(DeviceState *dev, Error **errp)
SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
memory_region_init_io(&s->iomem, OBJECT(dev), &imx_eth_ops, s,
- TYPE_IMX_FEC, 0x400);
+ TYPE_IMX_FEC, FSL_IMX25_FEC_SIZE);
sysbus_init_mmio(sbd, &s->iomem);
sysbus_init_irq(sbd, &s->irq[0]);
sysbus_init_irq(sbd, &s->irq[1]);
diff --git a/include/hw/arm/fsl-imx25.h b/include/hw/arm/fsl-imx25.h
index d0e8e9d956..65a73714ef 100644
--- a/include/hw/arm/fsl-imx25.h
+++ b/include/hw/arm/fsl-imx25.h
@@ -192,7 +192,6 @@ typedef struct FslIMX25State {
#define FSL_IMX25_UART5_ADDR 0x5002C000
#define FSL_IMX25_UART5_SIZE 0x4000
#define FSL_IMX25_FEC_ADDR 0x50038000
-#define FSL_IMX25_FEC_SIZE 0x4000
#define FSL_IMX25_CCM_ADDR 0x53F80000
#define FSL_IMX25_CCM_SIZE 0x4000
#define FSL_IMX25_GPT4_ADDR 0x53F84000
diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
index 91ef8f89a6..7b3faa4019 100644
--- a/include/hw/net/imx_fec.h
+++ b/include/hw/net/imx_fec.h
@@ -245,6 +245,7 @@ typedef struct {
#define ENET_TX_RING_NUM 3
+#define FSL_IMX25_FEC_SIZE 0x4000
typedef struct IMXFECState {
/*< private >*/
--
2.14.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 12/15] sdhci: Add i.MX specific subtype of SDHCI
2017-12-14 14:52 [Qemu-devel] [PATCH v2 00/15] i.MX FEC and SD changes Andrey Smirnov
` (10 preceding siblings ...)
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 11/15] imx_fec: Reserve full FSL_IMX25_FEC_SIZE page for the register file Andrey Smirnov
@ 2017-12-14 14:52 ` Andrey Smirnov
2017-12-15 2:31 ` Philippe Mathieu-Daudé
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 13/15] hw: i.MX: Convert i.MX6 to use TYPE_IMX_USDHC Andrey Smirnov
` (2 subsequent siblings)
14 siblings, 1 reply; 25+ messages in thread
From: Andrey Smirnov @ 2017-12-14 14:52 UTC (permalink / raw)
To: qemu-arm
Cc: Andrey Smirnov, Peter Maydell, Jason Wang,
Philippe Mathieu-Daudé, qemu-devel, yurovsky
IP block found on several generations of i.MX family does not use
vanilla SDHCI implementation and it comes with a number of quirks.
Introduce i.MX SDHCI subtype of SDHCI block to add code necessary to
support unmodified Linux guest driver.
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
hw/sd/sdhci-internal.h | 19 +++++
hw/sd/sdhci.c | 228 ++++++++++++++++++++++++++++++++++++++++++++++++-
include/hw/sd/sdhci.h | 14 +++
3 files changed, 259 insertions(+), 2 deletions(-)
diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index 161177cf39..b86ac0791b 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -85,12 +85,18 @@
/* R/W Host control Register 0x0 */
#define SDHC_HOSTCTL 0x28
+#define SDHC_CTRL_LED 0x01
#define SDHC_CTRL_DMA_CHECK_MASK 0x18
#define SDHC_CTRL_SDMA 0x00
#define SDHC_CTRL_ADMA1_32 0x08
#define SDHC_CTRL_ADMA2_32 0x10
#define SDHC_CTRL_ADMA2_64 0x18
#define SDHC_DMA_TYPE(x) ((x) & SDHC_CTRL_DMA_CHECK_MASK)
+#define SDHC_CTRL_4BITBUS 0x02
+#define SDHC_CTRL_8BITBUS 0x20
+#define SDHC_CTRL_CDTEST_INS 0x40
+#define SDHC_CTRL_CDTEST_EN 0x80
+
/* R/W Power Control Register 0x0 */
#define SDHC_PWRCON 0x29
@@ -229,4 +235,17 @@ enum {
extern const VMStateDescription sdhci_vmstate;
+
+#define ESDHC_MIX_CTRL 0x48
+#define ESDHC_VENDOR_SPEC 0xc0
+#define ESDHC_DLL_CTRL 0x60
+
+#define ESDHC_TUNING_CTRL 0xcc
+#define ESDHC_TUNE_CTRL_STATUS 0x68
+#define ESDHC_WTMK_LVL 0x44
+
+#define ESDHC_CTRL_4BITBUS (0x1 << 1)
+#define ESDHC_CTRL_8BITBUS (0x2 << 1)
+
+
#endif
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 6d6a791ee9..758af067f9 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -265,7 +265,8 @@ static void sdhci_send_command(SDHCIState *s)
}
}
- if ((s->norintstsen & SDHC_NISEN_TRSCMP) &&
+ if (!(s->quirks & SDHCI_QUIRK_NO_BUSY_IRQ) &&
+ (s->norintstsen & SDHC_NISEN_TRSCMP) &&
(s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY) {
s->norintsts |= SDHC_NIS_TRSCMP;
}
@@ -1191,6 +1192,8 @@ static void sdhci_initfn(SDHCIState *s)
s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s);
s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s);
+
+ s->io_ops = &sdhci_mmio_ops;
}
static void sdhci_uninitfn(SDHCIState *s)
@@ -1347,7 +1350,7 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
s->buf_maxsz = sdhci_get_fifolen(s);
s->fifo_buffer = g_malloc0(s->buf_maxsz);
sysbus_init_irq(sbd, &s->irq);
- memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
+ memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci",
SDHC_REGISTERS_MAP_SIZE);
sysbus_init_mmio(sbd, &s->iomem);
}
@@ -1386,11 +1389,232 @@ static const TypeInfo sdhci_bus_info = {
.class_init = sdhci_bus_class_init,
};
+static uint64_t usdhc_read(void *opaque, hwaddr offset, unsigned size)
+{
+ SDHCIState *s = SYSBUS_SDHCI(opaque);
+ uint32_t ret;
+ uint16_t hostctl;
+
+ switch (offset) {
+ default:
+ return sdhci_read(opaque, offset, size);
+
+ case SDHC_HOSTCTL:
+ /*
+ * For a detailed explanation on the following bit
+ * manipulation code see comments in a similar part of
+ * usdhc_write()
+ */
+ hostctl = SDHC_DMA_TYPE(s->hostctl) << (8 - 3);
+
+ if (s->hostctl & SDHC_CTRL_8BITBUS) {
+ hostctl |= ESDHC_CTRL_8BITBUS;
+ }
+
+ if (s->hostctl & SDHC_CTRL_4BITBUS) {
+ hostctl |= ESDHC_CTRL_4BITBUS;
+ }
+
+ ret = hostctl;
+ ret |= (uint32_t)s->blkgap << 16;
+ ret |= (uint32_t)s->wakcon << 24;
+
+ break;
+
+ case ESDHC_DLL_CTRL:
+ case ESDHC_TUNE_CTRL_STATUS:
+ case 0x6c:
+ case ESDHC_TUNING_CTRL:
+ case ESDHC_VENDOR_SPEC:
+ case ESDHC_MIX_CTRL:
+ case ESDHC_WTMK_LVL:
+ ret = 0;
+ break;
+ }
+
+ return ret;
+}
+
+static void
+usdhc_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
+{
+ SDHCIState *s = SYSBUS_SDHCI(opaque);
+ uint8_t hostctl;
+ uint32_t value = (uint32_t)val;
+
+ switch (offset) {
+ case ESDHC_DLL_CTRL:
+ case ESDHC_TUNE_CTRL_STATUS:
+ case 0x6c:
+ case ESDHC_TUNING_CTRL:
+ case ESDHC_WTMK_LVL:
+ case ESDHC_VENDOR_SPEC:
+ break;
+
+ case SDHC_HOSTCTL:
+ /*
+ * Here's What ESDHCI has at offset 0x28 (SDHC_HOSTCTL)
+ *
+ * 7 6 5 4 3 2 1 0
+ * |-----------+--------+--------+-----------+----------+---------|
+ * | Card | Card | Endian | DATA3 | Data | Led |
+ * | Detect | Detect | Mode | as Card | Transfer | Control |
+ * | Signal | Test | | Detection | Width | |
+ * | Selection | Level | | Pin | | |
+ * |-----------+--------+--------+-----------+----------+---------|
+ *
+ * and 0x29
+ *
+ * 15 10 9 8
+ * |----------+------|
+ * | Reserved | DMA |
+ * | | Sel. |
+ * | | |
+ * |----------+------|
+ *
+ * and here's what SDCHI spec expects those offsets to be:
+ *
+ * 0x28 (Host Control Register)
+ *
+ * 7 6 5 4 3 2 1 0
+ * |--------+--------+----------+------+--------+----------+---------|
+ * | Card | Card | Extended | DMA | High | Data | LED |
+ * | Detect | Detect | Data | Sel. | Speed | Transfer | Control |
+ * | Signal | Test | Transfer | | Enable | Width | |
+ * | Sel. | Level | Width | | | | |
+ * |--------+--------+----------+------+--------+----------+---------|
+ *
+ * and 0x29 (Power Control Register)
+ *
+ * |----------------------------------|
+ * | Power Control Register |
+ * | |
+ * | Description omitted, |
+ * | since it has no analog in ESDHCI |
+ * | |
+ * |----------------------------------|
+ *
+ * Since offsets 0x2A and 0x2B should be compatible between
+ * both IP specs we only need to reconcile least 16-bit of the
+ * word we've been given.
+ */
+
+ /*
+ * First, save bits 7 6 and 0 since they are identical
+ */
+ hostctl = value & (SDHC_CTRL_LED |
+ SDHC_CTRL_CDTEST_INS |
+ SDHC_CTRL_CDTEST_EN);
+ /*
+ * Second, split "Data Transfer Width" from bits 2 and 1 in to
+ * bits 5 and 1
+ */
+ if (value & ESDHC_CTRL_8BITBUS) {
+ hostctl |= SDHC_CTRL_8BITBUS;
+ }
+
+ if (value & ESDHC_CTRL_4BITBUS) {
+ hostctl |= ESDHC_CTRL_4BITBUS;
+ }
+
+ /*
+ * Third, move DMA select from bits 9 and 8 to bits 4 and 3
+ */
+ hostctl |= SDHC_DMA_TYPE(value >> (8 - 3));
+
+ /*
+ * Now place the corrected value into low 16-bit of the value
+ * we are going to give standard SDHCI write function
+ *
+ * NOTE: This transformation should be the inverse of what can
+ * be found in drivers/mmc/host/sdhci-esdhc-imx.c in Linux
+ * kernel
+ */
+ value &= ~UINT16_MAX;
+ value |= hostctl;
+ value |= (uint16_t)s->pwrcon << 8;
+
+ sdhci_write(opaque, offset, value, size);
+ break;
+
+ case ESDHC_MIX_CTRL:
+ /*
+ * So, when SD/MMC stack in Linux tries to write to "Transfer
+ * Mode Register", ESDHC i.MX quirk code will translate it
+ * into a write to ESDHC_MIX_CTRL, so we do the opposite in
+ * order to get where we started
+ *
+ * Note that Auto CMD23 Enable bit is located in a wrong place
+ * on i.MX, but since it is not used by QEMU we do not care.
+ *
+ * We don't want to call sdhci_write(.., SDHC_TRNMOD, ...)
+ * here becuase it will result in a call to
+ * sdhci_send_command(s) which we don't want.
+ *
+ */
+ s->trnmod = value & UINT16_MAX;
+ break;
+ case SDHC_TRNMOD:
+ /*
+ * Similar to above, but this time a write to "Command
+ * Register" will be translated into a 4-byte write to
+ * "Transfer Mode register" where lower 16-bit of value would
+ * be set to zero. So what we do is fill those bits with
+ * cached value from s->trnmod and let the SDHCI
+ * infrastructure handle the rest
+ */
+ sdhci_write(opaque, offset, val | s->trnmod, size);
+ break;
+ case SDHC_BLKSIZE:
+ /*
+ * ESDHCI does not implement "Host SDMA Buffer Boundary", and
+ * Linux driver will try to zero this field out which will
+ * break the rest of SDHCI emulation.
+ *
+ * Linux defaults to maximum possible setting (512K boundary)
+ * and it seems to be the only option that i.MX IP implements,
+ * so we artificially set it to that value.
+ */
+ val |= 0x7 << 12;
+ /* FALLTHROUGH */
+ default:
+ sdhci_write(opaque, offset, val, size);
+ break;
+ }
+}
+
+
+static const MemoryRegionOps usdhc_mmio_ops = {
+ .read = usdhc_read,
+ .write = usdhc_write,
+ .valid = {
+ .min_access_size = 1,
+ .max_access_size = 4,
+ .unaligned = false
+ },
+ .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void imx_usdhc_init(Object *obj)
+{
+ SDHCIState *s = SYSBUS_SDHCI(obj);
+
+ s->io_ops = &usdhc_mmio_ops;
+ s->quirks = SDHCI_QUIRK_NO_BUSY_IRQ;
+}
+
+static const TypeInfo imx_usdhc_info = {
+ .name = TYPE_IMX_USDHC,
+ .parent = TYPE_SYSBUS_SDHCI,
+ .instance_init = imx_usdhc_init,
+};
+
static void sdhci_register_types(void)
{
type_register_static(&sdhci_pci_info);
type_register_static(&sdhci_sysbus_info);
type_register_static(&sdhci_bus_info);
+ type_register_static(&imx_usdhc_info);
}
type_init(sdhci_register_types)
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 0f0c3f1e64..b56836a2fc 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -39,6 +39,7 @@ typedef struct SDHCIState {
};
SDBus sdbus;
MemoryRegion iomem;
+ const MemoryRegionOps *io_ops;
QEMUTimer *insert_timer; /* timer for 'changing' sd card. */
QEMUTimer *transfer_timer;
@@ -83,8 +84,19 @@ typedef struct SDHCIState {
/* Force Event Auto CMD12 Error Interrupt Reg - write only */
/* Force Event Error Interrupt Register- write only */
/* RO Host Controller Version Register always reads as 0x2401 */
+
+ uint32_t quirks;
} SDHCIState;
+/*
+ * Controller does not provide transfer-complete interrupt when not
+ * busy.
+ *
+ * NOTE: This definition is taken out of Linux kernel and so the
+ * original bit number is preserved
+ */
+#define SDHCI_QUIRK_NO_BUSY_IRQ BIT(14)
+
#define TYPE_PCI_SDHCI "sdhci-pci"
#define PCI_SDHCI(obj) OBJECT_CHECK(SDHCIState, (obj), TYPE_PCI_SDHCI)
@@ -92,4 +104,6 @@ typedef struct SDHCIState {
#define SYSBUS_SDHCI(obj) \
OBJECT_CHECK(SDHCIState, (obj), TYPE_SYSBUS_SDHCI)
+#define TYPE_IMX_USDHC "imx-usdhc"
+
#endif /* SDHCI_H */
--
2.14.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 13/15] hw: i.MX: Convert i.MX6 to use TYPE_IMX_USDHC
2017-12-14 14:52 [Qemu-devel] [PATCH v2 00/15] i.MX FEC and SD changes Andrey Smirnov
` (11 preceding siblings ...)
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 12/15] sdhci: Add i.MX specific subtype of SDHCI Andrey Smirnov
@ 2017-12-14 14:52 ` Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 14/15] sd: Check for READ_MULTIPLE_BLOCK size limit violation first Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 15/15] sdhci: Implement write method of ACMD12ERRSTS register Andrey Smirnov
14 siblings, 0 replies; 25+ messages in thread
From: Andrey Smirnov @ 2017-12-14 14:52 UTC (permalink / raw)
To: qemu-arm
Cc: Andrey Smirnov, Peter Maydell, Jason Wang,
Philippe Mathieu-Daudé, qemu-devel, yurovsky
Convert i.MX6 to use TYPE_IMX_USDHC since that's what real HW comes
with.
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
hw/arm/fsl-imx6.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index 2ed7146c52..b9249dbfc8 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -93,7 +93,7 @@ static void fsl_imx6_init(Object *obj)
}
for (i = 0; i < FSL_IMX6_NUM_ESDHCS; i++) {
- object_initialize(&s->esdhc[i], sizeof(s->esdhc[i]), TYPE_SYSBUS_SDHCI);
+ object_initialize(&s->esdhc[i], sizeof(s->esdhc[i]), TYPE_IMX_USDHC);
qdev_set_parent_bus(DEVICE(&s->esdhc[i]), sysbus_get_default());
snprintf(name, NAME_SIZE, "sdhc%d", i + 1);
object_property_add_child(obj, name, OBJECT(&s->esdhc[i]), NULL);
--
2.14.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 14/15] sd: Check for READ_MULTIPLE_BLOCK size limit violation first
2017-12-14 14:52 [Qemu-devel] [PATCH v2 00/15] i.MX FEC and SD changes Andrey Smirnov
` (12 preceding siblings ...)
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 13/15] hw: i.MX: Convert i.MX6 to use TYPE_IMX_USDHC Andrey Smirnov
@ 2017-12-14 14:52 ` Andrey Smirnov
2017-12-14 16:45 ` Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 15/15] sdhci: Implement write method of ACMD12ERRSTS register Andrey Smirnov
14 siblings, 1 reply; 25+ messages in thread
From: Andrey Smirnov @ 2017-12-14 14:52 UTC (permalink / raw)
To: qemu-arm
Cc: Andrey Smirnov, Peter Maydell, Jason Wang,
Philippe Mathieu-Daudé, qemu-devel, yurovsky
Check for READ_MULTIPLE_BLOCK size limit violation first as opposed to
doing at the end of the command handler. Consider the following
scenario:
Emulated host driver is trying to read last byte of the last sector
via CMD18/ADMA, so what would happen is the following:
1. "ret" is filled with desired byte and "sd->data_offset" is
incremented to 512 by
ret = sd->data[sd->data_offset ++];
2. sd->data_offset >= io_len becomes true, so
sd->data_start += io_len;
moves "sd->data_start" past valid data boundaries.
3. as a result "sd->data_start + io_len > sd->size" check becomes true
and sd->card_status is marked with ADDRESS_ERROR, telling emulated
host that the last CMD18 read failed, despite nothing bad/illegal
happening.
To avoid having this false positive, move out-of-bounds check to
happen before BLK_READ_BLOCK(), so this way it will only trigger if
illegal read is truly about to happen.
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
hw/sd/sd.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index ba47bff4db..ce4ef17be3 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1797,8 +1797,17 @@ uint8_t sd_read_data(SDState *sd)
break;
case 18: /* CMD18: READ_MULTIPLE_BLOCK */
- if (sd->data_offset == 0)
+ if (sd->data_offset == 0) {
+ if (sd->data_start + io_len > sd->size) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Trying to read past card's capacity\n",
+ __func__);
+ sd->card_status |= ADDRESS_ERROR;
+ return 0x00;
+ }
+
BLK_READ_BLOCK(sd->data_start, io_len);
+ }
ret = sd->data[sd->data_offset ++];
if (sd->data_offset >= io_len) {
@@ -1812,11 +1821,6 @@ uint8_t sd_read_data(SDState *sd)
break;
}
}
-
- if (sd->data_start + io_len > sd->size) {
- sd->card_status |= ADDRESS_ERROR;
- break;
- }
}
break;
--
2.14.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH v2 15/15] sdhci: Implement write method of ACMD12ERRSTS register
2017-12-14 14:52 [Qemu-devel] [PATCH v2 00/15] i.MX FEC and SD changes Andrey Smirnov
` (13 preceding siblings ...)
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 14/15] sd: Check for READ_MULTIPLE_BLOCK size limit violation first Andrey Smirnov
@ 2017-12-14 14:52 ` Andrey Smirnov
2017-12-15 2:18 ` Philippe Mathieu-Daudé
14 siblings, 1 reply; 25+ messages in thread
From: Andrey Smirnov @ 2017-12-14 14:52 UTC (permalink / raw)
To: qemu-arm
Cc: Andrey Smirnov, Peter Maydell, Jason Wang,
Philippe Mathieu-Daudé, qemu-devel, yurovsky
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
hw/sd/sdhci.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 758af067f9..cb9e0db9fb 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1139,6 +1139,9 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
s->admasysaddr = (s->admasysaddr & (0x00000000FFFFFFFFULL |
((uint64_t)mask << 32))) | ((uint64_t)value << 32);
break;
+ case SDHC_ACMD12ERRSTS:
+ MASKED_WRITE(s->acmd12errsts, mask, value);
+ break;
case SDHC_FEAER:
s->acmd12errsts |= value;
s->errintsts |= (value >> 16) & s->errintstsen;
--
2.14.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 14/15] sd: Check for READ_MULTIPLE_BLOCK size limit violation first
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 14/15] sd: Check for READ_MULTIPLE_BLOCK size limit violation first Andrey Smirnov
@ 2017-12-14 16:45 ` Andrey Smirnov
0 siblings, 0 replies; 25+ messages in thread
From: Andrey Smirnov @ 2017-12-14 16:45 UTC (permalink / raw)
To: open list:ARM
Cc: Andrey Smirnov, Peter Maydell, Jason Wang,
Philippe Mathieu-Daudé, QEMU Developers, Andrey Yurovsky
On Thu, Dec 14, 2017 at 6:52 AM, Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
> Check for READ_MULTIPLE_BLOCK size limit violation first as opposed to
> doing at the end of the command handler. Consider the following
> scenario:
>
> Emulated host driver is trying to read last byte of the last sector
> via CMD18/ADMA, so what would happen is the following:
>
> 1. "ret" is filled with desired byte and "sd->data_offset" is
> incremented to 512 by
>
> ret = sd->data[sd->data_offset ++];
>
> 2. sd->data_offset >= io_len becomes true, so
>
> sd->data_start += io_len;
>
> moves "sd->data_start" past valid data boundaries.
>
> 3. as a result "sd->data_start + io_len > sd->size" check becomes true
> and sd->card_status is marked with ADDRESS_ERROR, telling emulated
> host that the last CMD18 read failed, despite nothing bad/illegal
> happening.
>
> To avoid having this false positive, move out-of-bounds check to
> happen before BLK_READ_BLOCK(), so this way it will only trigger if
> illegal read is truly about to happen.
>
Disregard this patch, exactly this fix is already present in latest QEMU master.
Sorry for the noise.
Thanks,
Andrey Smirnov
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
> hw/sd/sd.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index ba47bff4db..ce4ef17be3 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1797,8 +1797,17 @@ uint8_t sd_read_data(SDState *sd)
> break;
>
> case 18: /* CMD18: READ_MULTIPLE_BLOCK */
> - if (sd->data_offset == 0)
> + if (sd->data_offset == 0) {
> + if (sd->data_start + io_len > sd->size) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Trying to read past card's capacity\n",
> + __func__);
> + sd->card_status |= ADDRESS_ERROR;
> + return 0x00;
> + }
> +
> BLK_READ_BLOCK(sd->data_start, io_len);
> + }
> ret = sd->data[sd->data_offset ++];
>
> if (sd->data_offset >= io_len) {
> @@ -1812,11 +1821,6 @@ uint8_t sd_read_data(SDState *sd)
> break;
> }
> }
> -
> - if (sd->data_start + io_len > sd->size) {
> - sd->card_status |= ADDRESS_ERROR;
> - break;
> - }
> }
> break;
>
> --
> 2.14.3
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 15/15] sdhci: Implement write method of ACMD12ERRSTS register
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 15/15] sdhci: Implement write method of ACMD12ERRSTS register Andrey Smirnov
@ 2017-12-15 2:18 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 25+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-12-15 2:18 UTC (permalink / raw)
To: Andrey Smirnov, qemu-arm; +Cc: Peter Maydell, Jason Wang, qemu-devel, yurovsky
Hi Andrey,
On 12/14/2017 11:52 AM, Andrey Smirnov wrote:
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
I have a more complete patch coming implementing the
CTRL2_VOLTAGE_SWITCH (LSB part of the ACMD12ERRSTS register) but in the
meaning time your patch is fine.
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/sd/sdhci.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 758af067f9..cb9e0db9fb 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1139,6 +1139,9 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
> s->admasysaddr = (s->admasysaddr & (0x00000000FFFFFFFFULL |
> ((uint64_t)mask << 32))) | ((uint64_t)value << 32);
> break;
> + case SDHC_ACMD12ERRSTS:
> + MASKED_WRITE(s->acmd12errsts, mask, value);
> + break;
> case SDHC_FEAER:
> s->acmd12errsts |= value;
> s->errintsts |= (value >> 16) & s->errintstsen;
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 12/15] sdhci: Add i.MX specific subtype of SDHCI
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 12/15] sdhci: Add i.MX specific subtype of SDHCI Andrey Smirnov
@ 2017-12-15 2:31 ` Philippe Mathieu-Daudé
2017-12-15 18:51 ` Andrey Smirnov
0 siblings, 1 reply; 25+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-12-15 2:31 UTC (permalink / raw)
To: Andrey Smirnov, qemu-arm; +Cc: Peter Maydell, Jason Wang, qemu-devel, yurovsky
Hi Andrey, Peter.
I rather disagree with this patch, however I applied it on top of my
current tree and plan to refactor it. But if it is applied before, I can
survive :) Not a strong NACK.
See comment inlined.
On 12/14/2017 11:52 AM, Andrey Smirnov wrote:
> IP block found on several generations of i.MX family does not use
> vanilla SDHCI implementation and it comes with a number of quirks.
>
> Introduce i.MX SDHCI subtype of SDHCI block to add code necessary to
> support unmodified Linux guest driver.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
> hw/sd/sdhci-internal.h | 19 +++++
> hw/sd/sdhci.c | 228 ++++++++++++++++++++++++++++++++++++++++++++++++-
> include/hw/sd/sdhci.h | 14 +++
> 3 files changed, 259 insertions(+), 2 deletions(-)
>
> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
> index 161177cf39..b86ac0791b 100644
> --- a/hw/sd/sdhci-internal.h
> +++ b/hw/sd/sdhci-internal.h
> @@ -85,12 +85,18 @@
>
> /* R/W Host control Register 0x0 */
> #define SDHC_HOSTCTL 0x28
> +#define SDHC_CTRL_LED 0x01
> #define SDHC_CTRL_DMA_CHECK_MASK 0x18
> #define SDHC_CTRL_SDMA 0x00
> #define SDHC_CTRL_ADMA1_32 0x08
> #define SDHC_CTRL_ADMA2_32 0x10
> #define SDHC_CTRL_ADMA2_64 0x18
> #define SDHC_DMA_TYPE(x) ((x) & SDHC_CTRL_DMA_CHECK_MASK)
> +#define SDHC_CTRL_4BITBUS 0x02
> +#define SDHC_CTRL_8BITBUS 0x20
> +#define SDHC_CTRL_CDTEST_INS 0x40
> +#define SDHC_CTRL_CDTEST_EN 0x80
> +
>
> /* R/W Power Control Register 0x0 */
> #define SDHC_PWRCON 0x29
> @@ -229,4 +235,17 @@ enum {
>
> extern const VMStateDescription sdhci_vmstate;
>
> +
> +#define ESDHC_MIX_CTRL 0x48
> +#define ESDHC_VENDOR_SPEC 0xc0
> +#define ESDHC_DLL_CTRL 0x60
> +
> +#define ESDHC_TUNING_CTRL 0xcc
> +#define ESDHC_TUNE_CTRL_STATUS 0x68
> +#define ESDHC_WTMK_LVL 0x44
> +
> +#define ESDHC_CTRL_4BITBUS (0x1 << 1)
> +#define ESDHC_CTRL_8BITBUS (0x2 << 1)
> +
> +
> #endif
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 6d6a791ee9..758af067f9 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -265,7 +265,8 @@ static void sdhci_send_command(SDHCIState *s)
> }
> }
>
> - if ((s->norintstsen & SDHC_NISEN_TRSCMP) &&
> + if (!(s->quirks & SDHCI_QUIRK_NO_BUSY_IRQ) &&
> + (s->norintstsen & SDHC_NISEN_TRSCMP) &&
> (s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY) {
> s->norintsts |= SDHC_NIS_TRSCMP;
> }
> @@ -1191,6 +1192,8 @@ static void sdhci_initfn(SDHCIState *s)
>
> s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s);
> s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s);
> +
> + s->io_ops = &sdhci_mmio_ops;
> }
>
> static void sdhci_uninitfn(SDHCIState *s)
> @@ -1347,7 +1350,7 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
> s->buf_maxsz = sdhci_get_fifolen(s);
> s->fifo_buffer = g_malloc0(s->buf_maxsz);
> sysbus_init_irq(sbd, &s->irq);
> - memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
> + memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci",
> SDHC_REGISTERS_MAP_SIZE);
> sysbus_init_mmio(sbd, &s->iomem);
> }
> @@ -1386,11 +1389,232 @@ static const TypeInfo sdhci_bus_info = {
> .class_init = sdhci_bus_class_init,
> };
>
> +static uint64_t usdhc_read(void *opaque, hwaddr offset, unsigned size)
> +{
> + SDHCIState *s = SYSBUS_SDHCI(opaque);
> + uint32_t ret;
> + uint16_t hostctl;
> +
> + switch (offset) {
> + default:
> + return sdhci_read(opaque, offset, size);
> +
> + case SDHC_HOSTCTL:
> + /*
> + * For a detailed explanation on the following bit
> + * manipulation code see comments in a similar part of
> + * usdhc_write()
> + */
> + hostctl = SDHC_DMA_TYPE(s->hostctl) << (8 - 3);
> +
> + if (s->hostctl & SDHC_CTRL_8BITBUS) {
> + hostctl |= ESDHC_CTRL_8BITBUS;
> + }
> +
> + if (s->hostctl & SDHC_CTRL_4BITBUS) {
> + hostctl |= ESDHC_CTRL_4BITBUS;
> + }
> +
> + ret = hostctl;
> + ret |= (uint32_t)s->blkgap << 16;
> + ret |= (uint32_t)s->wakcon << 24;
> +
> + break;
> +
> + case ESDHC_DLL_CTRL:
> + case ESDHC_TUNE_CTRL_STATUS:
> + case 0x6c:
> + case ESDHC_TUNING_CTRL:
> + case ESDHC_VENDOR_SPEC:
> + case ESDHC_MIX_CTRL:
> + case ESDHC_WTMK_LVL:
> + ret = 0;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static void
> +usdhc_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
> +{
> + SDHCIState *s = SYSBUS_SDHCI(opaque);
> + uint8_t hostctl;
> + uint32_t value = (uint32_t)val;
> +
> + switch (offset) {
> + case ESDHC_DLL_CTRL:
> + case ESDHC_TUNE_CTRL_STATUS:
> + case 0x6c:
> + case ESDHC_TUNING_CTRL:
> + case ESDHC_WTMK_LVL:
> + case ESDHC_VENDOR_SPEC:
> + break;
> +
> + case SDHC_HOSTCTL:
> + /*
> + * Here's What ESDHCI has at offset 0x28 (SDHC_HOSTCTL)
> + *
> + * 7 6 5 4 3 2 1 0
> + * |-----------+--------+--------+-----------+----------+---------|
> + * | Card | Card | Endian | DATA3 | Data | Led |
> + * | Detect | Detect | Mode | as Card | Transfer | Control |
> + * | Signal | Test | | Detection | Width | |
> + * | Selection | Level | | Pin | | |
> + * |-----------+--------+--------+-----------+----------+---------|
> + *
> + * and 0x29
> + *
> + * 15 10 9 8
> + * |----------+------|
> + * | Reserved | DMA |
> + * | | Sel. |
> + * | | |
> + * |----------+------|
> + *
> + * and here's what SDCHI spec expects those offsets to be:
> + *
> + * 0x28 (Host Control Register)
> + *
> + * 7 6 5 4 3 2 1 0
> + * |--------+--------+----------+------+--------+----------+---------|
> + * | Card | Card | Extended | DMA | High | Data | LED |
> + * | Detect | Detect | Data | Sel. | Speed | Transfer | Control |
> + * | Signal | Test | Transfer | | Enable | Width | |
> + * | Sel. | Level | Width | | | | |
> + * |--------+--------+----------+------+--------+----------+---------|
> + *
> + * and 0x29 (Power Control Register)
> + *
> + * |----------------------------------|
> + * | Power Control Register |
> + * | |
> + * | Description omitted, |
> + * | since it has no analog in ESDHCI |
> + * | |
> + * |----------------------------------|
> + *
> + * Since offsets 0x2A and 0x2B should be compatible between
> + * both IP specs we only need to reconcile least 16-bit of the
> + * word we've been given.
> + */
> +
> + /*
> + * First, save bits 7 6 and 0 since they are identical
> + */
> + hostctl = value & (SDHC_CTRL_LED |
> + SDHC_CTRL_CDTEST_INS |
> + SDHC_CTRL_CDTEST_EN);
> + /*
> + * Second, split "Data Transfer Width" from bits 2 and 1 in to
> + * bits 5 and 1
> + */
> + if (value & ESDHC_CTRL_8BITBUS) {
> + hostctl |= SDHC_CTRL_8BITBUS;
> + }
> +
> + if (value & ESDHC_CTRL_4BITBUS) {
> + hostctl |= ESDHC_CTRL_4BITBUS;
> + }
> +
> + /*
> + * Third, move DMA select from bits 9 and 8 to bits 4 and 3
> + */
> + hostctl |= SDHC_DMA_TYPE(value >> (8 - 3));
> +
> + /*
> + * Now place the corrected value into low 16-bit of the value
> + * we are going to give standard SDHCI write function
> + *
> + * NOTE: This transformation should be the inverse of what can
> + * be found in drivers/mmc/host/sdhci-esdhc-imx.c in Linux
> + * kernel
> + */
> + value &= ~UINT16_MAX;
> + value |= hostctl;
> + value |= (uint16_t)s->pwrcon << 8;
> +
> + sdhci_write(opaque, offset, value, size);
> + break;
> +
> + case ESDHC_MIX_CTRL:
> + /*
> + * So, when SD/MMC stack in Linux tries to write to "Transfer
> + * Mode Register", ESDHC i.MX quirk code will translate it
> + * into a write to ESDHC_MIX_CTRL, so we do the opposite in
> + * order to get where we started
> + *
> + * Note that Auto CMD23 Enable bit is located in a wrong place
> + * on i.MX, but since it is not used by QEMU we do not care.
> + *
> + * We don't want to call sdhci_write(.., SDHC_TRNMOD, ...)
> + * here becuase it will result in a call to
> + * sdhci_send_command(s) which we don't want.
> + *
> + */
> + s->trnmod = value & UINT16_MAX;
> + break;
> + case SDHC_TRNMOD:
> + /*
> + * Similar to above, but this time a write to "Command
> + * Register" will be translated into a 4-byte write to
> + * "Transfer Mode register" where lower 16-bit of value would
> + * be set to zero. So what we do is fill those bits with
> + * cached value from s->trnmod and let the SDHCI
> + * infrastructure handle the rest
> + */
> + sdhci_write(opaque, offset, val | s->trnmod, size);
> + break;
> + case SDHC_BLKSIZE:
> + /*
> + * ESDHCI does not implement "Host SDMA Buffer Boundary", and
> + * Linux driver will try to zero this field out which will
> + * break the rest of SDHCI emulation.
> + *
> + * Linux defaults to maximum possible setting (512K boundary)
> + * and it seems to be the only option that i.MX IP implements,
> + * so we artificially set it to that value.
> + */
> + val |= 0x7 << 12;
> + /* FALLTHROUGH */
> + default:
> + sdhci_write(opaque, offset, val, size);
> + break;
> + }
> +}
> +
> +
> +static const MemoryRegionOps usdhc_mmio_ops = {
> + .read = usdhc_read,
> + .write = usdhc_write,
> + .valid = {
> + .min_access_size = 1,
> + .max_access_size = 4,
> + .unaligned = false
> + },
> + .endianness = DEVICE_LITTLE_ENDIAN,
> +};
I think the usdhc_mmio_ops can be avoided.
> +
> +static void imx_usdhc_init(Object *obj)
> +{
> + SDHCIState *s = SYSBUS_SDHCI(obj);
> +
> + s->io_ops = &usdhc_mmio_ops;
> + s->quirks = SDHCI_QUIRK_NO_BUSY_IRQ;
> +}
> +
> +static const TypeInfo imx_usdhc_info = {
> + .name = TYPE_IMX_USDHC,
> + .parent = TYPE_SYSBUS_SDHCI,
> + .instance_init = imx_usdhc_init,
> +};
there is no need to add yet another device. You can achieve the same
using a property bit (like the "pending-insert-quirk" one).
I think the correct way is to add a DEFINE_PROP_BIT64("quirks*").
> static void sdhci_register_types(void)
> {
> type_register_static(&sdhci_pci_info);
> type_register_static(&sdhci_sysbus_info);
> type_register_static(&sdhci_bus_info);
> + type_register_static(&imx_usdhc_info);
> }
>
> type_init(sdhci_register_types)
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index 0f0c3f1e64..b56836a2fc 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -39,6 +39,7 @@ typedef struct SDHCIState {
> };
> SDBus sdbus;
> MemoryRegion iomem;
> + const MemoryRegionOps *io_ops;
>
> QEMUTimer *insert_timer; /* timer for 'changing' sd card. */
> QEMUTimer *transfer_timer;
> @@ -83,8 +84,19 @@ typedef struct SDHCIState {
> /* Force Event Auto CMD12 Error Interrupt Reg - write only */
> /* Force Event Error Interrupt Register- write only */
> /* RO Host Controller Version Register always reads as 0x2401 */
> +
> + uint32_t quirks;
> } SDHCIState;
>
> +/*
> + * Controller does not provide transfer-complete interrupt when not
> + * busy.
> + *
> + * NOTE: This definition is taken out of Linux kernel and so the
> + * original bit number is preserved
> + */
> +#define SDHCI_QUIRK_NO_BUSY_IRQ BIT(14)
> +
> #define TYPE_PCI_SDHCI "sdhci-pci"
> #define PCI_SDHCI(obj) OBJECT_CHECK(SDHCIState, (obj), TYPE_PCI_SDHCI)
>
> @@ -92,4 +104,6 @@ typedef struct SDHCIState {
> #define SYSBUS_SDHCI(obj) \
> OBJECT_CHECK(SDHCIState, (obj), TYPE_SYSBUS_SDHCI)
>
> +#define TYPE_IMX_USDHC "imx-usdhc"
> +
> #endif /* SDHCI_H */
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 02/15] imx_fec: Refactor imx_eth_enable_rx()
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 02/15] imx_fec: Refactor imx_eth_enable_rx() Andrey Smirnov
@ 2017-12-15 10:06 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 25+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-12-15 10:06 UTC (permalink / raw)
To: Andrey Smirnov, qemu-arm; +Cc: Peter Maydell, Jason Wang, qemu-devel, yurovsky
On 12/14/2017 11:52 AM, Andrey Smirnov wrote:
> Refactor imx_eth_enable_rx() to have more meaningfull variable name
> than 'tmp' and to reduce number of logical negations done.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/net/imx_fec.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index 88b4b049d7..8b2e4b8ffe 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -536,19 +536,19 @@ static void imx_eth_do_tx(IMXFECState *s)
> static void imx_eth_enable_rx(IMXFECState *s)
> {
> IMXFECBufDesc bd;
> - bool tmp;
> + bool rx_ring_full;
>
> imx_fec_read_bd(&bd, s->rx_descriptor);
>
> - tmp = ((bd.flags & ENET_BD_E) != 0);
> + rx_ring_full = !(bd.flags & ENET_BD_E);
>
> - if (!tmp) {
> + if (rx_ring_full) {
> FEC_PRINTF("RX buffer full\n");
> } else if (!s->regs[ENET_RDAR]) {
> qemu_flush_queued_packets(qemu_get_queue(s->nic));
> }
>
> - s->regs[ENET_RDAR] = tmp ? ENET_RDAR_RDAR : 0;
> + s->regs[ENET_RDAR] = rx_ring_full ? 0 : ENET_RDAR_RDAR;
> }
>
> static void imx_eth_reset(DeviceState *d)
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 05/15] imx_fec: Use ENET_FTRL to determine truncation length
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 05/15] imx_fec: Use ENET_FTRL to determine truncation length Andrey Smirnov
@ 2017-12-15 10:15 ` Philippe Mathieu-Daudé
2017-12-17 0:41 ` Andrey Smirnov
0 siblings, 1 reply; 25+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-12-15 10:15 UTC (permalink / raw)
To: Andrey Smirnov, qemu-arm; +Cc: Peter Maydell, Jason Wang, qemu-devel, yurovsky
Hi Andrey,
On 12/14/2017 11:52 AM, Andrey Smirnov wrote:
> Frame truncation length, TRUNC_FL, is determined by the contents of
> ENET_FTRL register, so convert the code to use it instead of a
> hardcoded constant.
>
> To avoid the case where TRUNC_FL is greater that ENET_MAX_FRAME_SIZE,
> increase the value of the latter to its theoretical maximum of 16K.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
> hw/net/imx_fec.c | 4 ++--
> include/hw/net/imx_fec.h | 3 ++-
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index 56cb72273c..50da91bf9e 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -1052,8 +1052,8 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
> crc_ptr = (uint8_t *) &crc;
>
> /* Huge frames are truncted. */
> - if (size > ENET_MAX_FRAME_SIZE) {
> - size = ENET_MAX_FRAME_SIZE;
> + if (size > s->regs[ENET_FTRL]) {
Shouldn't this be:
if (size > s->regs[ENET_FTRL] + 1) {
> + size = s->regs[ENET_FTRL];
and:
size = s->regs[ENET_FTRL] + 1;
> flags |= ENET_BD_TR | ENET_BD_LG;
> }
>
> diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
> index 67993870a2..a390d704a6 100644
> --- a/include/hw/net/imx_fec.h
> +++ b/include/hw/net/imx_fec.h
> @@ -86,7 +86,6 @@
> #define ENET_TCCR3 393
> #define ENET_MAX 400
>
> -#define ENET_MAX_FRAME_SIZE 2032
>
> /* EIR and EIMR */
> #define ENET_INT_HB (1 << 31)
> @@ -155,6 +154,8 @@
> #define ENET_RCR_NLC (1 << 30)
> #define ENET_RCR_GRS (1 << 31)
>
> +#define ENET_MAX_FRAME_SIZE (1 << ENET_RCR_MAX_FL_LENGTH)
> +
> /* TCR */
> #define ENET_TCR_GTS (1 << 0)
> #define ENET_TCR_FDEN (1 << 2)
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 12/15] sdhci: Add i.MX specific subtype of SDHCI
2017-12-15 2:31 ` Philippe Mathieu-Daudé
@ 2017-12-15 18:51 ` Andrey Smirnov
2017-12-15 20:15 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 25+ messages in thread
From: Andrey Smirnov @ 2017-12-15 18:51 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: open list:ARM, Peter Maydell, Jason Wang, QEMU Developers,
Andrey Yurovsky
On Thu, Dec 14, 2017 at 6:31 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Andrey, Peter.
>
> I rather disagree with this patch, however I applied it on top of my
> current tree and plan to refactor it. But if it is applied before, I can
> survive :) Not a strong NACK.
>
Umm, Philippe, I didn't really ask you to refactor my code and I'd
really appreciate if you'd engage into a proper review process with me
on this patch, instead of just telling me "I don't like the code, but,
worry not, I'll bulldoze it away later anyway".
> See comment inlined.
>
> On 12/14/2017 11:52 AM, Andrey Smirnov wrote:
>> IP block found on several generations of i.MX family does not use
>> vanilla SDHCI implementation and it comes with a number of quirks.
>>
>> Introduce i.MX SDHCI subtype of SDHCI block to add code necessary to
>> support unmodified Linux guest driver.
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Cc: qemu-devel@nongnu.org
>> Cc: qemu-arm@nongnu.org
>> Cc: yurovsky@gmail.com
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>> hw/sd/sdhci-internal.h | 19 +++++
>> hw/sd/sdhci.c | 228 ++++++++++++++++++++++++++++++++++++++++++++++++-
>> include/hw/sd/sdhci.h | 14 +++
>> 3 files changed, 259 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
>> index 161177cf39..b86ac0791b 100644
>> --- a/hw/sd/sdhci-internal.h
>> +++ b/hw/sd/sdhci-internal.h
>> @@ -85,12 +85,18 @@
>>
>> /* R/W Host control Register 0x0 */
>> #define SDHC_HOSTCTL 0x28
>> +#define SDHC_CTRL_LED 0x01
>> #define SDHC_CTRL_DMA_CHECK_MASK 0x18
>> #define SDHC_CTRL_SDMA 0x00
>> #define SDHC_CTRL_ADMA1_32 0x08
>> #define SDHC_CTRL_ADMA2_32 0x10
>> #define SDHC_CTRL_ADMA2_64 0x18
>> #define SDHC_DMA_TYPE(x) ((x) & SDHC_CTRL_DMA_CHECK_MASK)
>> +#define SDHC_CTRL_4BITBUS 0x02
>> +#define SDHC_CTRL_8BITBUS 0x20
>> +#define SDHC_CTRL_CDTEST_INS 0x40
>> +#define SDHC_CTRL_CDTEST_EN 0x80
>> +
>>
>> /* R/W Power Control Register 0x0 */
>> #define SDHC_PWRCON 0x29
>> @@ -229,4 +235,17 @@ enum {
>>
>> extern const VMStateDescription sdhci_vmstate;
>>
>> +
>> +#define ESDHC_MIX_CTRL 0x48
>> +#define ESDHC_VENDOR_SPEC 0xc0
>> +#define ESDHC_DLL_CTRL 0x60
>> +
>> +#define ESDHC_TUNING_CTRL 0xcc
>> +#define ESDHC_TUNE_CTRL_STATUS 0x68
>> +#define ESDHC_WTMK_LVL 0x44
>> +
>> +#define ESDHC_CTRL_4BITBUS (0x1 << 1)
>> +#define ESDHC_CTRL_8BITBUS (0x2 << 1)
>> +
>> +
>> #endif
>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>> index 6d6a791ee9..758af067f9 100644
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -265,7 +265,8 @@ static void sdhci_send_command(SDHCIState *s)
>> }
>> }
>>
>> - if ((s->norintstsen & SDHC_NISEN_TRSCMP) &&
>> + if (!(s->quirks & SDHCI_QUIRK_NO_BUSY_IRQ) &&
>> + (s->norintstsen & SDHC_NISEN_TRSCMP) &&
>> (s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY) {
>> s->norintsts |= SDHC_NIS_TRSCMP;
>> }
>> @@ -1191,6 +1192,8 @@ static void sdhci_initfn(SDHCIState *s)
>>
>> s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s);
>> s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s);
>> +
>> + s->io_ops = &sdhci_mmio_ops;
>> }
>>
>> static void sdhci_uninitfn(SDHCIState *s)
>> @@ -1347,7 +1350,7 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
>> s->buf_maxsz = sdhci_get_fifolen(s);
>> s->fifo_buffer = g_malloc0(s->buf_maxsz);
>> sysbus_init_irq(sbd, &s->irq);
>> - memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
>> + memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci",
>> SDHC_REGISTERS_MAP_SIZE);
>> sysbus_init_mmio(sbd, &s->iomem);
>> }
>> @@ -1386,11 +1389,232 @@ static const TypeInfo sdhci_bus_info = {
>> .class_init = sdhci_bus_class_init,
>> };
>>
>> +static uint64_t usdhc_read(void *opaque, hwaddr offset, unsigned size)
>> +{
>> + SDHCIState *s = SYSBUS_SDHCI(opaque);
>> + uint32_t ret;
>> + uint16_t hostctl;
>> +
>> + switch (offset) {
>> + default:
>> + return sdhci_read(opaque, offset, size);
>> +
>> + case SDHC_HOSTCTL:
>> + /*
>> + * For a detailed explanation on the following bit
>> + * manipulation code see comments in a similar part of
>> + * usdhc_write()
>> + */
>> + hostctl = SDHC_DMA_TYPE(s->hostctl) << (8 - 3);
>> +
>> + if (s->hostctl & SDHC_CTRL_8BITBUS) {
>> + hostctl |= ESDHC_CTRL_8BITBUS;
>> + }
>> +
>> + if (s->hostctl & SDHC_CTRL_4BITBUS) {
>> + hostctl |= ESDHC_CTRL_4BITBUS;
>> + }
>> +
>> + ret = hostctl;
>> + ret |= (uint32_t)s->blkgap << 16;
>> + ret |= (uint32_t)s->wakcon << 24;
>> +
>> + break;
>> +
>> + case ESDHC_DLL_CTRL:
>> + case ESDHC_TUNE_CTRL_STATUS:
>> + case 0x6c:
>> + case ESDHC_TUNING_CTRL:
>> + case ESDHC_VENDOR_SPEC:
>> + case ESDHC_MIX_CTRL:
>> + case ESDHC_WTMK_LVL:
>> + ret = 0;
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static void
>> +usdhc_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>> +{
>> + SDHCIState *s = SYSBUS_SDHCI(opaque);
>> + uint8_t hostctl;
>> + uint32_t value = (uint32_t)val;
>> +
>> + switch (offset) {
>> + case ESDHC_DLL_CTRL:
>> + case ESDHC_TUNE_CTRL_STATUS:
>> + case 0x6c:
>> + case ESDHC_TUNING_CTRL:
>> + case ESDHC_WTMK_LVL:
>> + case ESDHC_VENDOR_SPEC:
>> + break;
>> +
>> + case SDHC_HOSTCTL:
>> + /*
>> + * Here's What ESDHCI has at offset 0x28 (SDHC_HOSTCTL)
>> + *
>> + * 7 6 5 4 3 2 1 0
>> + * |-----------+--------+--------+-----------+----------+---------|
>> + * | Card | Card | Endian | DATA3 | Data | Led |
>> + * | Detect | Detect | Mode | as Card | Transfer | Control |
>> + * | Signal | Test | | Detection | Width | |
>> + * | Selection | Level | | Pin | | |
>> + * |-----------+--------+--------+-----------+----------+---------|
>> + *
>> + * and 0x29
>> + *
>> + * 15 10 9 8
>> + * |----------+------|
>> + * | Reserved | DMA |
>> + * | | Sel. |
>> + * | | |
>> + * |----------+------|
>> + *
>> + * and here's what SDCHI spec expects those offsets to be:
>> + *
>> + * 0x28 (Host Control Register)
>> + *
>> + * 7 6 5 4 3 2 1 0
>> + * |--------+--------+----------+------+--------+----------+---------|
>> + * | Card | Card | Extended | DMA | High | Data | LED |
>> + * | Detect | Detect | Data | Sel. | Speed | Transfer | Control |
>> + * | Signal | Test | Transfer | | Enable | Width | |
>> + * | Sel. | Level | Width | | | | |
>> + * |--------+--------+----------+------+--------+----------+---------|
>> + *
>> + * and 0x29 (Power Control Register)
>> + *
>> + * |----------------------------------|
>> + * | Power Control Register |
>> + * | |
>> + * | Description omitted, |
>> + * | since it has no analog in ESDHCI |
>> + * | |
>> + * |----------------------------------|
>> + *
>> + * Since offsets 0x2A and 0x2B should be compatible between
>> + * both IP specs we only need to reconcile least 16-bit of the
>> + * word we've been given.
>> + */
>> +
>> + /*
>> + * First, save bits 7 6 and 0 since they are identical
>> + */
>> + hostctl = value & (SDHC_CTRL_LED |
>> + SDHC_CTRL_CDTEST_INS |
>> + SDHC_CTRL_CDTEST_EN);
>> + /*
>> + * Second, split "Data Transfer Width" from bits 2 and 1 in to
>> + * bits 5 and 1
>> + */
>> + if (value & ESDHC_CTRL_8BITBUS) {
>> + hostctl |= SDHC_CTRL_8BITBUS;
>> + }
>> +
>> + if (value & ESDHC_CTRL_4BITBUS) {
>> + hostctl |= ESDHC_CTRL_4BITBUS;
>> + }
>> +
>> + /*
>> + * Third, move DMA select from bits 9 and 8 to bits 4 and 3
>> + */
>> + hostctl |= SDHC_DMA_TYPE(value >> (8 - 3));
>> +
>> + /*
>> + * Now place the corrected value into low 16-bit of the value
>> + * we are going to give standard SDHCI write function
>> + *
>> + * NOTE: This transformation should be the inverse of what can
>> + * be found in drivers/mmc/host/sdhci-esdhc-imx.c in Linux
>> + * kernel
>> + */
>> + value &= ~UINT16_MAX;
>> + value |= hostctl;
>> + value |= (uint16_t)s->pwrcon << 8;
>> +
>> + sdhci_write(opaque, offset, value, size);
>> + break;
>> +
>> + case ESDHC_MIX_CTRL:
>> + /*
>> + * So, when SD/MMC stack in Linux tries to write to "Transfer
>> + * Mode Register", ESDHC i.MX quirk code will translate it
>> + * into a write to ESDHC_MIX_CTRL, so we do the opposite in
>> + * order to get where we started
>> + *
>> + * Note that Auto CMD23 Enable bit is located in a wrong place
>> + * on i.MX, but since it is not used by QEMU we do not care.
>> + *
>> + * We don't want to call sdhci_write(.., SDHC_TRNMOD, ...)
>> + * here becuase it will result in a call to
>> + * sdhci_send_command(s) which we don't want.
>> + *
>> + */
>> + s->trnmod = value & UINT16_MAX;
>> + break;
>> + case SDHC_TRNMOD:
>> + /*
>> + * Similar to above, but this time a write to "Command
>> + * Register" will be translated into a 4-byte write to
>> + * "Transfer Mode register" where lower 16-bit of value would
>> + * be set to zero. So what we do is fill those bits with
>> + * cached value from s->trnmod and let the SDHCI
>> + * infrastructure handle the rest
>> + */
>> + sdhci_write(opaque, offset, val | s->trnmod, size);
>> + break;
>> + case SDHC_BLKSIZE:
>> + /*
>> + * ESDHCI does not implement "Host SDMA Buffer Boundary", and
>> + * Linux driver will try to zero this field out which will
>> + * break the rest of SDHCI emulation.
>> + *
>> + * Linux defaults to maximum possible setting (512K boundary)
>> + * and it seems to be the only option that i.MX IP implements,
>> + * so we artificially set it to that value.
>> + */
>> + val |= 0x7 << 12;
>> + /* FALLTHROUGH */
>> + default:
>> + sdhci_write(opaque, offset, val, size);
>> + break;
>> + }
>> +}
>> +
>> +
>> +static const MemoryRegionOps usdhc_mmio_ops = {
>> + .read = usdhc_read,
>> + .write = usdhc_write,
>> + .valid = {
>> + .min_access_size = 1,
>> + .max_access_size = 4,
>> + .unaligned = false
>> + },
>> + .endianness = DEVICE_LITTLE_ENDIAN,
>> +};
>
> I think the usdhc_mmio_ops can be avoided.
>
OK, care to actually elaborate on this?
>> +
>> +static void imx_usdhc_init(Object *obj)
>> +{
>> + SDHCIState *s = SYSBUS_SDHCI(obj);
>> +
>> + s->io_ops = &usdhc_mmio_ops;
>> + s->quirks = SDHCI_QUIRK_NO_BUSY_IRQ;
>> +}
>> +
>> +static const TypeInfo imx_usdhc_info = {
>> + .name = TYPE_IMX_USDHC,
>> + .parent = TYPE_SYSBUS_SDHCI,
>> + .instance_init = imx_usdhc_init,
>> +};
>
> there is no need to add yet another device. You can achieve the same
> using a property bit (like the "pending-insert-quirk" one).
>
> I think the correct way is to add a DEFINE_PROP_BIT64("quirks*").
>
And how's that better? Now, every i.MX user instead of instantiating a
uSDCH IP block, which matches what i.MX RM say, would have to instead
create SDHCI (confusing) as well as to set an quirk, which means they
have to know what SDHCI_QUIRK_NO_BUSY_IRQ means. What's the point of
spilling those internal implementation guts all of the place just to
avoid re-defining "yet another device"?
Thanks,
Andrey Smirnov
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 12/15] sdhci: Add i.MX specific subtype of SDHCI
2017-12-15 18:51 ` Andrey Smirnov
@ 2017-12-15 20:15 ` Philippe Mathieu-Daudé
2017-12-17 0:21 ` Andrey Smirnov
0 siblings, 1 reply; 25+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-12-15 20:15 UTC (permalink / raw)
To: Andrey Smirnov
Cc: open list:ARM, Peter Maydell, Jason Wang, QEMU Developers,
Andrey Yurovsky
Hi Andrey,
>> I rather disagree with this patch, however I applied it on top of my
>> current tree and plan to refactor it. But if it is applied before, I can
>> survive :) Not a strong NACK.
>>
>
> Umm, Philippe, I didn't really ask you to refactor my code and I'd
> really appreciate if you'd engage into a proper review process with me
> on this patch, instead of just telling me "I don't like the code, but,
> worry not, I'll bulldoze it away later anyway".
Hey I'm sorry I didn't meant to be rude... reading my comment back I
don't find it offensive, so we might have english-proxy misunderstanding.
I posted a few series doing quite a lot of modifications in the SDHCI
code, and instead of having a race "the first merged, the second has
figure out himself" I offered to rework this patch on top of my series
and send it to you so you can test it.
I don't plan to 'bulldoze' it, there is no sens because I don't have all
your setups to test it, I just suggested to 'refactor' it to avoid merge
conflicts and head-aches.
By "But if it is applied before, I can survive" I meant:
"if it is applied before [my series get merged], I can survive [to adapt
my series to fix the resulting merge conflicts]"
>> On 12/14/2017 11:52 AM, Andrey Smirnov wrote:
>>> IP block found on several generations of i.MX family does not use
>>> vanilla SDHCI implementation and it comes with a number of quirks.
>>>
>>> Introduce i.MX SDHCI subtype of SDHCI block to add code necessary to
>>> support unmodified Linux guest driver.
>>>
>>> Cc: Peter Maydell <peter.maydell@linaro.org>
>>> Cc: Jason Wang <jasowang@redhat.com>
>>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> Cc: qemu-devel@nongnu.org
>>> Cc: qemu-arm@nongnu.org
>>> Cc: yurovsky@gmail.com
>>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>>> ---
>>> hw/sd/sdhci-internal.h | 19 +++++
>>> hw/sd/sdhci.c | 228 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>> include/hw/sd/sdhci.h | 14 +++
>>> 3 files changed, 259 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
>>> index 161177cf39..b86ac0791b 100644
>>> --- a/hw/sd/sdhci-internal.h
>>> +++ b/hw/sd/sdhci-internal.h
>>> @@ -85,12 +85,18 @@
>>>
>>> /* R/W Host control Register 0x0 */
>>> #define SDHC_HOSTCTL 0x28
>>> +#define SDHC_CTRL_LED 0x01
>>> #define SDHC_CTRL_DMA_CHECK_MASK 0x18
>>> #define SDHC_CTRL_SDMA 0x00
>>> #define SDHC_CTRL_ADMA1_32 0x08
>>> #define SDHC_CTRL_ADMA2_32 0x10
>>> #define SDHC_CTRL_ADMA2_64 0x18
>>> #define SDHC_DMA_TYPE(x) ((x) & SDHC_CTRL_DMA_CHECK_MASK)
>>> +#define SDHC_CTRL_4BITBUS 0x02
>>> +#define SDHC_CTRL_8BITBUS 0x20
>>> +#define SDHC_CTRL_CDTEST_INS 0x40
>>> +#define SDHC_CTRL_CDTEST_EN 0x80
>>> +
>>>
>>> /* R/W Power Control Register 0x0 */
>>> #define SDHC_PWRCON 0x29
>>> @@ -229,4 +235,17 @@ enum {
>>>
>>> extern const VMStateDescription sdhci_vmstate;
>>>
>>> +
>>> +#define ESDHC_MIX_CTRL 0x48
>>> +#define ESDHC_VENDOR_SPEC 0xc0
>>> +#define ESDHC_DLL_CTRL 0x60
>>> +
>>> +#define ESDHC_TUNING_CTRL 0xcc
>>> +#define ESDHC_TUNE_CTRL_STATUS 0x68
>>> +#define ESDHC_WTMK_LVL 0x44
>>> +
>>> +#define ESDHC_CTRL_4BITBUS (0x1 << 1)
>>> +#define ESDHC_CTRL_8BITBUS (0x2 << 1)
>>> +
>>> +
>>> #endif
>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>> index 6d6a791ee9..758af067f9 100644
>>> --- a/hw/sd/sdhci.c
>>> +++ b/hw/sd/sdhci.c
>>> @@ -265,7 +265,8 @@ static void sdhci_send_command(SDHCIState *s)
>>> }
>>> }
>>>
>>> - if ((s->norintstsen & SDHC_NISEN_TRSCMP) &&
>>> + if (!(s->quirks & SDHCI_QUIRK_NO_BUSY_IRQ) &&
>>> + (s->norintstsen & SDHC_NISEN_TRSCMP) &&
>>> (s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY) {
>>> s->norintsts |= SDHC_NIS_TRSCMP;
>>> }
>>> @@ -1191,6 +1192,8 @@ static void sdhci_initfn(SDHCIState *s)
>>>
>>> s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s);
>>> s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s);
>>> +
>>> + s->io_ops = &sdhci_mmio_ops;
>>> }
>>>
>>> static void sdhci_uninitfn(SDHCIState *s)
>>> @@ -1347,7 +1350,7 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
>>> s->buf_maxsz = sdhci_get_fifolen(s);
>>> s->fifo_buffer = g_malloc0(s->buf_maxsz);
>>> sysbus_init_irq(sbd, &s->irq);
>>> - memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
>>> + memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci",
>>> SDHC_REGISTERS_MAP_SIZE);
>>> sysbus_init_mmio(sbd, &s->iomem);
>>> }
>>> @@ -1386,11 +1389,232 @@ static const TypeInfo sdhci_bus_info = {
>>> .class_init = sdhci_bus_class_init,
>>> };
>>>
>>> +static uint64_t usdhc_read(void *opaque, hwaddr offset, unsigned size)
>>> +{
>>> + SDHCIState *s = SYSBUS_SDHCI(opaque);
>>> + uint32_t ret;
>>> + uint16_t hostctl;
>>> +
>>> + switch (offset) {
>>> + default:
>>> + return sdhci_read(opaque, offset, size);
>>> +
>>> + case SDHC_HOSTCTL:
>>> + /*
>>> + * For a detailed explanation on the following bit
>>> + * manipulation code see comments in a similar part of
>>> + * usdhc_write()
>>> + */
>>> + hostctl = SDHC_DMA_TYPE(s->hostctl) << (8 - 3);
>>> +
>>> + if (s->hostctl & SDHC_CTRL_8BITBUS) {
>>> + hostctl |= ESDHC_CTRL_8BITBUS;
>>> + }
>>> +
>>> + if (s->hostctl & SDHC_CTRL_4BITBUS) {
>>> + hostctl |= ESDHC_CTRL_4BITBUS;
>>> + }
>>> +
>>> + ret = hostctl;
>>> + ret |= (uint32_t)s->blkgap << 16;
>>> + ret |= (uint32_t)s->wakcon << 24;
>>> +
>>> + break;
>>> +
>>> + case ESDHC_DLL_CTRL:
>>> + case ESDHC_TUNE_CTRL_STATUS:
>>> + case 0x6c:
>>> + case ESDHC_TUNING_CTRL:
>>> + case ESDHC_VENDOR_SPEC:
>>> + case ESDHC_MIX_CTRL:
>>> + case ESDHC_WTMK_LVL:
>>> + ret = 0;
>>> + break;
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static void
>>> +usdhc_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>>> +{
>>> + SDHCIState *s = SYSBUS_SDHCI(opaque);
>>> + uint8_t hostctl;
>>> + uint32_t value = (uint32_t)val;
>>> +
>>> + switch (offset) {
>>> + case ESDHC_DLL_CTRL:
>>> + case ESDHC_TUNE_CTRL_STATUS:
>>> + case 0x6c:
>>> + case ESDHC_TUNING_CTRL:
>>> + case ESDHC_WTMK_LVL:
>>> + case ESDHC_VENDOR_SPEC:
>>> + break;
>>> +
>>> + case SDHC_HOSTCTL:
>>> + /*
>>> + * Here's What ESDHCI has at offset 0x28 (SDHC_HOSTCTL)
>>> + *
>>> + * 7 6 5 4 3 2 1 0
>>> + * |-----------+--------+--------+-----------+----------+---------|
>>> + * | Card | Card | Endian | DATA3 | Data | Led |
>>> + * | Detect | Detect | Mode | as Card | Transfer | Control |
>>> + * | Signal | Test | | Detection | Width | |
>>> + * | Selection | Level | | Pin | | |
>>> + * |-----------+--------+--------+-----------+----------+---------|
>>> + *
>>> + * and 0x29
>>> + *
>>> + * 15 10 9 8
>>> + * |----------+------|
>>> + * | Reserved | DMA |
>>> + * | | Sel. |
>>> + * | | |
>>> + * |----------+------|
>>> + *
>>> + * and here's what SDCHI spec expects those offsets to be:
>>> + *
>>> + * 0x28 (Host Control Register)
>>> + *
>>> + * 7 6 5 4 3 2 1 0
>>> + * |--------+--------+----------+------+--------+----------+---------|
>>> + * | Card | Card | Extended | DMA | High | Data | LED |
>>> + * | Detect | Detect | Data | Sel. | Speed | Transfer | Control |
>>> + * | Signal | Test | Transfer | | Enable | Width | |
>>> + * | Sel. | Level | Width | | | | |
>>> + * |--------+--------+----------+------+--------+----------+---------|
>>> + *
>>> + * and 0x29 (Power Control Register)
>>> + *
>>> + * |----------------------------------|
>>> + * | Power Control Register |
>>> + * | |
>>> + * | Description omitted, |
>>> + * | since it has no analog in ESDHCI |
>>> + * | |
>>> + * |----------------------------------|
>>> + *
>>> + * Since offsets 0x2A and 0x2B should be compatible between
>>> + * both IP specs we only need to reconcile least 16-bit of the
>>> + * word we've been given.
>>> + */
>>> +
>>> + /*
>>> + * First, save bits 7 6 and 0 since they are identical
>>> + */
>>> + hostctl = value & (SDHC_CTRL_LED |
>>> + SDHC_CTRL_CDTEST_INS |
>>> + SDHC_CTRL_CDTEST_EN);
>>> + /*
>>> + * Second, split "Data Transfer Width" from bits 2 and 1 in to
>>> + * bits 5 and 1
>>> + */
>>> + if (value & ESDHC_CTRL_8BITBUS) {
>>> + hostctl |= SDHC_CTRL_8BITBUS;
>>> + }
>>> +
>>> + if (value & ESDHC_CTRL_4BITBUS) {
>>> + hostctl |= ESDHC_CTRL_4BITBUS;
>>> + }
>>> +
>>> + /*
>>> + * Third, move DMA select from bits 9 and 8 to bits 4 and 3
>>> + */
>>> + hostctl |= SDHC_DMA_TYPE(value >> (8 - 3));
>>> +
>>> + /*
>>> + * Now place the corrected value into low 16-bit of the value
>>> + * we are going to give standard SDHCI write function
>>> + *
>>> + * NOTE: This transformation should be the inverse of what can
>>> + * be found in drivers/mmc/host/sdhci-esdhc-imx.c in Linux
>>> + * kernel
>>> + */
>>> + value &= ~UINT16_MAX;
>>> + value |= hostctl;
>>> + value |= (uint16_t)s->pwrcon << 8;
>>> +
>>> + sdhci_write(opaque, offset, value, size);
>>> + break;
>>> +
>>> + case ESDHC_MIX_CTRL:
>>> + /*
>>> + * So, when SD/MMC stack in Linux tries to write to "Transfer
>>> + * Mode Register", ESDHC i.MX quirk code will translate it
>>> + * into a write to ESDHC_MIX_CTRL, so we do the opposite in
>>> + * order to get where we started
>>> + *
>>> + * Note that Auto CMD23 Enable bit is located in a wrong place
>>> + * on i.MX, but since it is not used by QEMU we do not care.
>>> + *
>>> + * We don't want to call sdhci_write(.., SDHC_TRNMOD, ...)
>>> + * here becuase it will result in a call to
>>> + * sdhci_send_command(s) which we don't want.
>>> + *
>>> + */
>>> + s->trnmod = value & UINT16_MAX;
>>> + break;
>>> + case SDHC_TRNMOD:
>>> + /*
>>> + * Similar to above, but this time a write to "Command
>>> + * Register" will be translated into a 4-byte write to
>>> + * "Transfer Mode register" where lower 16-bit of value would
>>> + * be set to zero. So what we do is fill those bits with
>>> + * cached value from s->trnmod and let the SDHCI
>>> + * infrastructure handle the rest
>>> + */
>>> + sdhci_write(opaque, offset, val | s->trnmod, size);
>>> + break;
>>> + case SDHC_BLKSIZE:
>>> + /*
>>> + * ESDHCI does not implement "Host SDMA Buffer Boundary", and
>>> + * Linux driver will try to zero this field out which will
>>> + * break the rest of SDHCI emulation.
>>> + *
>>> + * Linux defaults to maximum possible setting (512K boundary)
>>> + * and it seems to be the only option that i.MX IP implements,
>>> + * so we artificially set it to that value.
>>> + */
>>> + val |= 0x7 << 12;
>>> + /* FALLTHROUGH */
>>> + default:
>>> + sdhci_write(opaque, offset, val, size);
>>> + break;
>>> + }
>>> +}
>>> +
>>> +
>>> +static const MemoryRegionOps usdhc_mmio_ops = {
>>> + .read = usdhc_read,
>>> + .write = usdhc_write,
>>> + .valid = {
>>> + .min_access_size = 1,
>>> + .max_access_size = 4,
>>> + .unaligned = false
>>> + },
>>> + .endianness = DEVICE_LITTLE_ENDIAN,
>>> +};
>>
>> I think the usdhc_mmio_ops can be avoided.
>>
>
> OK, care to actually elaborate on this?
We can avoid to keep it in this Arasan SDHCI IP file, see below.
>>> +
>>> +static void imx_usdhc_init(Object *obj)
>>> +{
>>> + SDHCIState *s = SYSBUS_SDHCI(obj);
>>> +
>>> + s->io_ops = &usdhc_mmio_ops;
>>> + s->quirks = SDHCI_QUIRK_NO_BUSY_IRQ;
>>> +}
>>> +
>>> +static const TypeInfo imx_usdhc_info = {
>>> + .name = TYPE_IMX_USDHC,
>>> + .parent = TYPE_SYSBUS_SDHCI,
>>> + .instance_init = imx_usdhc_init,
>>> +};
>>
>> there is no need to add yet another device. You can achieve the same
>> using a property bit (like the "pending-insert-quirk" one).
>>
>> I think the correct way is to add a DEFINE_PROP_BIT64("quirks*").
>>
>
> And how's that better? Now, every i.MX user instead of instantiating a
> uSDCH IP block, which matches what i.MX RM say, would have to instead
> create SDHCI (confusing) as well as to set an quirk, which means they
> have to know what SDHCI_QUIRK_NO_BUSY_IRQ means. What's the point of
> spilling those internal implementation guts all of the place just to
> avoid re-defining "yet another device"?
I see your point, you are correct, this is easier to model it this way.
I was more thinking move the uSDCH core in his own file (i.e.
imx_usdch.c), and keep this file with the Arasan IP implementation only.
So the imx_usdch.c only contains the specific USDHC registers/defines
and the TYPE_IMX_USDHC device would keep the SDHCI link hidden from the
user.
Using other words, the IMX_USDHC device would 'contains' a SDHCI block
instanciated with the SDHCI_QUIRK_NO_BUSY_IRQ property bit set.
Again, I appreciate the quality of the patches you send, and did not
intend to annoy you this way. I'll try to make some efforts to improve
my english communication. Please accept my apology.
Regards,
Phil.
>
> Thanks,
> Andrey Smirnov
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 12/15] sdhci: Add i.MX specific subtype of SDHCI
2017-12-15 20:15 ` Philippe Mathieu-Daudé
@ 2017-12-17 0:21 ` Andrey Smirnov
0 siblings, 0 replies; 25+ messages in thread
From: Andrey Smirnov @ 2017-12-17 0:21 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: open list:ARM, Peter Maydell, Jason Wang, QEMU Developers,
Andrey Yurovsky
On Fri, Dec 15, 2017 at 12:15 PM, Philippe Mathieu-Daudé
<f4bug@amsat.org> wrote:
> Hi Andrey,
>
>>> I rather disagree with this patch, however I applied it on top of my
>>> current tree and plan to refactor it. But if it is applied before, I can
>>> survive :) Not a strong NACK.
>>>
>>
>> Umm, Philippe, I didn't really ask you to refactor my code and I'd
>> really appreciate if you'd engage into a proper review process with me
>> on this patch, instead of just telling me "I don't like the code, but,
>> worry not, I'll bulldoze it away later anyway".
>
> Hey I'm sorry I didn't meant to be rude... reading my comment back I
> don't find it offensive, so we might have english-proxy misunderstanding.
>
> I posted a few series doing quite a lot of modifications in the SDHCI
> code, and instead of having a race "the first merged, the second has
> figure out himself" I offered to rework this patch on top of my series
> and send it to you so you can test it.
>
> I don't plan to 'bulldoze' it, there is no sens because I don't have all
> your setups to test it, I just suggested to 'refactor' it to avoid merge
> conflicts and head-aches.
>
> By "But if it is applied before, I can survive" I meant:
> "if it is applied before [my series get merged], I can survive [to adapt
> my series to fix the resulting merge conflicts]"
>
OK, gotcha. I think it would be the easiest for both of us to let your
work get merged first, since you patch is extensive and would warrant
me re-testing i.MX stuff anyway.
So how about this: I'll pull i.MX uSDHC patch back into original i.MX7
submission and by the time it is ready to be merged in, your code will
hopefully be in the master already, sounds good?
Meanwhile, do you have public git URL for your changes that I can use
to re-base on top of?
>>> On 12/14/2017 11:52 AM, Andrey Smirnov wrote:
>>>> IP block found on several generations of i.MX family does not use
>>>> vanilla SDHCI implementation and it comes with a number of quirks.
>>>>
>>>> Introduce i.MX SDHCI subtype of SDHCI block to add code necessary to
>>>> support unmodified Linux guest driver.
>>>>
>>>> Cc: Peter Maydell <peter.maydell@linaro.org>
>>>> Cc: Jason Wang <jasowang@redhat.com>
>>>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> Cc: qemu-devel@nongnu.org
>>>> Cc: qemu-arm@nongnu.org
>>>> Cc: yurovsky@gmail.com
>>>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>>>> ---
>>>> hw/sd/sdhci-internal.h | 19 +++++
>>>> hw/sd/sdhci.c | 228 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>>> include/hw/sd/sdhci.h | 14 +++
>>>> 3 files changed, 259 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
>>>> index 161177cf39..b86ac0791b 100644
>>>> --- a/hw/sd/sdhci-internal.h
>>>> +++ b/hw/sd/sdhci-internal.h
>>>> @@ -85,12 +85,18 @@
>>>>
>>>> /* R/W Host control Register 0x0 */
>>>> #define SDHC_HOSTCTL 0x28
>>>> +#define SDHC_CTRL_LED 0x01
>>>> #define SDHC_CTRL_DMA_CHECK_MASK 0x18
>>>> #define SDHC_CTRL_SDMA 0x00
>>>> #define SDHC_CTRL_ADMA1_32 0x08
>>>> #define SDHC_CTRL_ADMA2_32 0x10
>>>> #define SDHC_CTRL_ADMA2_64 0x18
>>>> #define SDHC_DMA_TYPE(x) ((x) & SDHC_CTRL_DMA_CHECK_MASK)
>>>> +#define SDHC_CTRL_4BITBUS 0x02
>>>> +#define SDHC_CTRL_8BITBUS 0x20
>>>> +#define SDHC_CTRL_CDTEST_INS 0x40
>>>> +#define SDHC_CTRL_CDTEST_EN 0x80
>>>> +
>>>>
>>>> /* R/W Power Control Register 0x0 */
>>>> #define SDHC_PWRCON 0x29
>>>> @@ -229,4 +235,17 @@ enum {
>>>>
>>>> extern const VMStateDescription sdhci_vmstate;
>>>>
>>>> +
>>>> +#define ESDHC_MIX_CTRL 0x48
>>>> +#define ESDHC_VENDOR_SPEC 0xc0
>>>> +#define ESDHC_DLL_CTRL 0x60
>>>> +
>>>> +#define ESDHC_TUNING_CTRL 0xcc
>>>> +#define ESDHC_TUNE_CTRL_STATUS 0x68
>>>> +#define ESDHC_WTMK_LVL 0x44
>>>> +
>>>> +#define ESDHC_CTRL_4BITBUS (0x1 << 1)
>>>> +#define ESDHC_CTRL_8BITBUS (0x2 << 1)
>>>> +
>>>> +
>>>> #endif
>>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>>> index 6d6a791ee9..758af067f9 100644
>>>> --- a/hw/sd/sdhci.c
>>>> +++ b/hw/sd/sdhci.c
>>>> @@ -265,7 +265,8 @@ static void sdhci_send_command(SDHCIState *s)
>>>> }
>>>> }
>>>>
>>>> - if ((s->norintstsen & SDHC_NISEN_TRSCMP) &&
>>>> + if (!(s->quirks & SDHCI_QUIRK_NO_BUSY_IRQ) &&
>>>> + (s->norintstsen & SDHC_NISEN_TRSCMP) &&
>>>> (s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY) {
>>>> s->norintsts |= SDHC_NIS_TRSCMP;
>>>> }
>>>> @@ -1191,6 +1192,8 @@ static void sdhci_initfn(SDHCIState *s)
>>>>
>>>> s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s);
>>>> s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s);
>>>> +
>>>> + s->io_ops = &sdhci_mmio_ops;
>>>> }
>>>>
>>>> static void sdhci_uninitfn(SDHCIState *s)
>>>> @@ -1347,7 +1350,7 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
>>>> s->buf_maxsz = sdhci_get_fifolen(s);
>>>> s->fifo_buffer = g_malloc0(s->buf_maxsz);
>>>> sysbus_init_irq(sbd, &s->irq);
>>>> - memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
>>>> + memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci",
>>>> SDHC_REGISTERS_MAP_SIZE);
>>>> sysbus_init_mmio(sbd, &s->iomem);
>>>> }
>>>> @@ -1386,11 +1389,232 @@ static const TypeInfo sdhci_bus_info = {
>>>> .class_init = sdhci_bus_class_init,
>>>> };
>>>>
>>>> +static uint64_t usdhc_read(void *opaque, hwaddr offset, unsigned size)
>>>> +{
>>>> + SDHCIState *s = SYSBUS_SDHCI(opaque);
>>>> + uint32_t ret;
>>>> + uint16_t hostctl;
>>>> +
>>>> + switch (offset) {
>>>> + default:
>>>> + return sdhci_read(opaque, offset, size);
>>>> +
>>>> + case SDHC_HOSTCTL:
>>>> + /*
>>>> + * For a detailed explanation on the following bit
>>>> + * manipulation code see comments in a similar part of
>>>> + * usdhc_write()
>>>> + */
>>>> + hostctl = SDHC_DMA_TYPE(s->hostctl) << (8 - 3);
>>>> +
>>>> + if (s->hostctl & SDHC_CTRL_8BITBUS) {
>>>> + hostctl |= ESDHC_CTRL_8BITBUS;
>>>> + }
>>>> +
>>>> + if (s->hostctl & SDHC_CTRL_4BITBUS) {
>>>> + hostctl |= ESDHC_CTRL_4BITBUS;
>>>> + }
>>>> +
>>>> + ret = hostctl;
>>>> + ret |= (uint32_t)s->blkgap << 16;
>>>> + ret |= (uint32_t)s->wakcon << 24;
>>>> +
>>>> + break;
>>>> +
>>>> + case ESDHC_DLL_CTRL:
>>>> + case ESDHC_TUNE_CTRL_STATUS:
>>>> + case 0x6c:
>>>> + case ESDHC_TUNING_CTRL:
>>>> + case ESDHC_VENDOR_SPEC:
>>>> + case ESDHC_MIX_CTRL:
>>>> + case ESDHC_WTMK_LVL:
>>>> + ret = 0;
>>>> + break;
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static void
>>>> +usdhc_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>>>> +{
>>>> + SDHCIState *s = SYSBUS_SDHCI(opaque);
>>>> + uint8_t hostctl;
>>>> + uint32_t value = (uint32_t)val;
>>>> +
>>>> + switch (offset) {
>>>> + case ESDHC_DLL_CTRL:
>>>> + case ESDHC_TUNE_CTRL_STATUS:
>>>> + case 0x6c:
>>>> + case ESDHC_TUNING_CTRL:
>>>> + case ESDHC_WTMK_LVL:
>>>> + case ESDHC_VENDOR_SPEC:
>>>> + break;
>>>> +
>>>> + case SDHC_HOSTCTL:
>>>> + /*
>>>> + * Here's What ESDHCI has at offset 0x28 (SDHC_HOSTCTL)
>>>> + *
>>>> + * 7 6 5 4 3 2 1 0
>>>> + * |-----------+--------+--------+-----------+----------+---------|
>>>> + * | Card | Card | Endian | DATA3 | Data | Led |
>>>> + * | Detect | Detect | Mode | as Card | Transfer | Control |
>>>> + * | Signal | Test | | Detection | Width | |
>>>> + * | Selection | Level | | Pin | | |
>>>> + * |-----------+--------+--------+-----------+----------+---------|
>>>> + *
>>>> + * and 0x29
>>>> + *
>>>> + * 15 10 9 8
>>>> + * |----------+------|
>>>> + * | Reserved | DMA |
>>>> + * | | Sel. |
>>>> + * | | |
>>>> + * |----------+------|
>>>> + *
>>>> + * and here's what SDCHI spec expects those offsets to be:
>>>> + *
>>>> + * 0x28 (Host Control Register)
>>>> + *
>>>> + * 7 6 5 4 3 2 1 0
>>>> + * |--------+--------+----------+------+--------+----------+---------|
>>>> + * | Card | Card | Extended | DMA | High | Data | LED |
>>>> + * | Detect | Detect | Data | Sel. | Speed | Transfer | Control |
>>>> + * | Signal | Test | Transfer | | Enable | Width | |
>>>> + * | Sel. | Level | Width | | | | |
>>>> + * |--------+--------+----------+------+--------+----------+---------|
>>>> + *
>>>> + * and 0x29 (Power Control Register)
>>>> + *
>>>> + * |----------------------------------|
>>>> + * | Power Control Register |
>>>> + * | |
>>>> + * | Description omitted, |
>>>> + * | since it has no analog in ESDHCI |
>>>> + * | |
>>>> + * |----------------------------------|
>>>> + *
>>>> + * Since offsets 0x2A and 0x2B should be compatible between
>>>> + * both IP specs we only need to reconcile least 16-bit of the
>>>> + * word we've been given.
>>>> + */
>>>> +
>>>> + /*
>>>> + * First, save bits 7 6 and 0 since they are identical
>>>> + */
>>>> + hostctl = value & (SDHC_CTRL_LED |
>>>> + SDHC_CTRL_CDTEST_INS |
>>>> + SDHC_CTRL_CDTEST_EN);
>>>> + /*
>>>> + * Second, split "Data Transfer Width" from bits 2 and 1 in to
>>>> + * bits 5 and 1
>>>> + */
>>>> + if (value & ESDHC_CTRL_8BITBUS) {
>>>> + hostctl |= SDHC_CTRL_8BITBUS;
>>>> + }
>>>> +
>>>> + if (value & ESDHC_CTRL_4BITBUS) {
>>>> + hostctl |= ESDHC_CTRL_4BITBUS;
>>>> + }
>>>> +
>>>> + /*
>>>> + * Third, move DMA select from bits 9 and 8 to bits 4 and 3
>>>> + */
>>>> + hostctl |= SDHC_DMA_TYPE(value >> (8 - 3));
>>>> +
>>>> + /*
>>>> + * Now place the corrected value into low 16-bit of the value
>>>> + * we are going to give standard SDHCI write function
>>>> + *
>>>> + * NOTE: This transformation should be the inverse of what can
>>>> + * be found in drivers/mmc/host/sdhci-esdhc-imx.c in Linux
>>>> + * kernel
>>>> + */
>>>> + value &= ~UINT16_MAX;
>>>> + value |= hostctl;
>>>> + value |= (uint16_t)s->pwrcon << 8;
>>>> +
>>>> + sdhci_write(opaque, offset, value, size);
>>>> + break;
>>>> +
>>>> + case ESDHC_MIX_CTRL:
>>>> + /*
>>>> + * So, when SD/MMC stack in Linux tries to write to "Transfer
>>>> + * Mode Register", ESDHC i.MX quirk code will translate it
>>>> + * into a write to ESDHC_MIX_CTRL, so we do the opposite in
>>>> + * order to get where we started
>>>> + *
>>>> + * Note that Auto CMD23 Enable bit is located in a wrong place
>>>> + * on i.MX, but since it is not used by QEMU we do not care.
>>>> + *
>>>> + * We don't want to call sdhci_write(.., SDHC_TRNMOD, ...)
>>>> + * here becuase it will result in a call to
>>>> + * sdhci_send_command(s) which we don't want.
>>>> + *
>>>> + */
>>>> + s->trnmod = value & UINT16_MAX;
>>>> + break;
>>>> + case SDHC_TRNMOD:
>>>> + /*
>>>> + * Similar to above, but this time a write to "Command
>>>> + * Register" will be translated into a 4-byte write to
>>>> + * "Transfer Mode register" where lower 16-bit of value would
>>>> + * be set to zero. So what we do is fill those bits with
>>>> + * cached value from s->trnmod and let the SDHCI
>>>> + * infrastructure handle the rest
>>>> + */
>>>> + sdhci_write(opaque, offset, val | s->trnmod, size);
>>>> + break;
>>>> + case SDHC_BLKSIZE:
>>>> + /*
>>>> + * ESDHCI does not implement "Host SDMA Buffer Boundary", and
>>>> + * Linux driver will try to zero this field out which will
>>>> + * break the rest of SDHCI emulation.
>>>> + *
>>>> + * Linux defaults to maximum possible setting (512K boundary)
>>>> + * and it seems to be the only option that i.MX IP implements,
>>>> + * so we artificially set it to that value.
>>>> + */
>>>> + val |= 0x7 << 12;
>>>> + /* FALLTHROUGH */
>>>> + default:
>>>> + sdhci_write(opaque, offset, val, size);
>>>> + break;
>>>> + }
>>>> +}
>>>> +
>>>> +
>>>> +static const MemoryRegionOps usdhc_mmio_ops = {
>>>> + .read = usdhc_read,
>>>> + .write = usdhc_write,
>>>> + .valid = {
>>>> + .min_access_size = 1,
>>>> + .max_access_size = 4,
>>>> + .unaligned = false
>>>> + },
>>>> + .endianness = DEVICE_LITTLE_ENDIAN,
>>>> +};
>>>
>>> I think the usdhc_mmio_ops can be avoided.
>>>
>>
>> OK, care to actually elaborate on this?
>
> We can avoid to keep it in this Arasan SDHCI IP file, see below.
>
>>>> +
>>>> +static void imx_usdhc_init(Object *obj)
>>>> +{
>>>> + SDHCIState *s = SYSBUS_SDHCI(obj);
>>>> +
>>>> + s->io_ops = &usdhc_mmio_ops;
>>>> + s->quirks = SDHCI_QUIRK_NO_BUSY_IRQ;
>>>> +}
>>>> +
>>>> +static const TypeInfo imx_usdhc_info = {
>>>> + .name = TYPE_IMX_USDHC,
>>>> + .parent = TYPE_SYSBUS_SDHCI,
>>>> + .instance_init = imx_usdhc_init,
>>>> +};
>>>
>>> there is no need to add yet another device. You can achieve the same
>>> using a property bit (like the "pending-insert-quirk" one).
>>>
>>> I think the correct way is to add a DEFINE_PROP_BIT64("quirks*").
>>>
>>
>> And how's that better? Now, every i.MX user instead of instantiating a
>> uSDCH IP block, which matches what i.MX RM say, would have to instead
>> create SDHCI (confusing) as well as to set an quirk, which means they
>> have to know what SDHCI_QUIRK_NO_BUSY_IRQ means. What's the point of
>> spilling those internal implementation guts all of the place just to
>> avoid re-defining "yet another device"?
>
> I see your point, you are correct, this is easier to model it this way.
> I was more thinking move the uSDCH core in his own file (i.e.
> imx_usdch.c), and keep this file with the Arasan IP implementation only.
>
> So the imx_usdch.c only contains the specific USDHC registers/defines
> and the TYPE_IMX_USDHC device would keep the SDHCI link hidden from the
> user.
> Using other words, the IMX_USDHC device would 'contains' a SDHCI block
> instanciated with the SDHCI_QUIRK_NO_BUSY_IRQ property bit set.
>
First, question is due to my ignorance, what's "Arasan"/"Arasan IP"
you keep referring to? I am not familiar with this term and can't seem
to find any references to it latest QEMU master.
Second, I'd love to move i.MX code into a separate file, but I wasn't
able to because a number of uSDHC registers are shadowing similar
registers in SDHCI, so I needed to "intercept" those writes first,
readjust the data to be laid out properly, and then pass it on to the
original SDCHI implementation. I couldn't figure out a way how to do
that better than just calling sdhc_write()/sdhc_read() directly, do
you have better ideas/suggestions?
>
> Again, I appreciate the quality of the patches you send, and did not
> intend to annoy you this way. I'll try to make some efforts to improve
> my english communication. Please accept my apology.
>
No worries. It was a simple misunderstanding. Let's get back to
talking about code :-)
Thanks,
Andrey Smirnov
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 05/15] imx_fec: Use ENET_FTRL to determine truncation length
2017-12-15 10:15 ` Philippe Mathieu-Daudé
@ 2017-12-17 0:41 ` Andrey Smirnov
0 siblings, 0 replies; 25+ messages in thread
From: Andrey Smirnov @ 2017-12-17 0:41 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: open list:ARM, Peter Maydell, Jason Wang, QEMU Developers,
Andrey Yurovsky
On Fri, Dec 15, 2017 at 2:15 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Andrey,
>
> On 12/14/2017 11:52 AM, Andrey Smirnov wrote:
>> Frame truncation length, TRUNC_FL, is determined by the contents of
>> ENET_FTRL register, so convert the code to use it instead of a
>> hardcoded constant.
>>
>> To avoid the case where TRUNC_FL is greater that ENET_MAX_FRAME_SIZE,
>> increase the value of the latter to its theoretical maximum of 16K.
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Cc: qemu-devel@nongnu.org
>> Cc: qemu-arm@nongnu.org
>> Cc: yurovsky@gmail.com
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>> hw/net/imx_fec.c | 4 ++--
>> include/hw/net/imx_fec.h | 3 ++-
>> 2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
>> index 56cb72273c..50da91bf9e 100644
>> --- a/hw/net/imx_fec.c
>> +++ b/hw/net/imx_fec.c
>> @@ -1052,8 +1052,8 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
>> crc_ptr = (uint8_t *) &crc;
>>
>> /* Huge frames are truncted. */
>> - if (size > ENET_MAX_FRAME_SIZE) {
>> - size = ENET_MAX_FRAME_SIZE;
>> + if (size > s->regs[ENET_FTRL]) {
>
> Shouldn't this be:
>
> if (size > s->regs[ENET_FTRL] + 1) {
>
>> + size = s->regs[ENET_FTRL];
>
> and:
>
> size = s->regs[ENET_FTRL] + 1;
>
I haven't tried to reproduce this in real HW and verify the behavior,
so I can't say for a fact, but the datasheet seems to be pretty clear
about "ENETx_FTRL":
"Frame Truncation Length
Indicates the value a receive frame is truncated, if it is greater
than this value. Must be greater than or
equal to RCR[MAX_FL].
NOTE: Truncation happens at TRUNC_FL. However, when truncation occurs,
the application (FIFO) may
receive less data, guaranteeing that it never receives more than the set limit."
so, having nothing better to go on than above description, I think the
original code is OK.
>> flags |= ENET_BD_TR | ENET_BD_LG;
>> }
>>
>> diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
>> index 67993870a2..a390d704a6 100644
>> --- a/include/hw/net/imx_fec.h
>> +++ b/include/hw/net/imx_fec.h
>> @@ -86,7 +86,6 @@
>> #define ENET_TCCR3 393
>> #define ENET_MAX 400
>>
>> -#define ENET_MAX_FRAME_SIZE 2032
>>
>> /* EIR and EIMR */
>> #define ENET_INT_HB (1 << 31)
>> @@ -155,6 +154,8 @@
>> #define ENET_RCR_NLC (1 << 30)
>> #define ENET_RCR_GRS (1 << 31)
>>
>> +#define ENET_MAX_FRAME_SIZE (1 << ENET_RCR_MAX_FL_LENGTH)
>> +
>> /* TCR */
>> #define ENET_TCR_GTS (1 << 0)
>> #define ENET_TCR_FDEN (1 << 2)
>>
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2017-12-17 0:41 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-14 14:52 [Qemu-devel] [PATCH v2 00/15] i.MX FEC and SD changes Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 01/15] imx_fec: Do not link to netdev Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 02/15] imx_fec: Refactor imx_eth_enable_rx() Andrey Smirnov
2017-12-15 10:06 ` Philippe Mathieu-Daudé
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 03/15] imx_fec: Change queue flushing heuristics Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 04/15] imx_fec: Move Tx frame buffer away from the stack Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 05/15] imx_fec: Use ENET_FTRL to determine truncation length Andrey Smirnov
2017-12-15 10:15 ` Philippe Mathieu-Daudé
2017-12-17 0:41 ` Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 06/15] imx_fec: Use MIN instead of explicit ternary operator Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 07/15] imx_fec: Emulate SHIFT16 in ENETx_RACC Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 08/15] imx_fec: Add support for multiple Tx DMA rings Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 09/15] imx_fec: Use correct length for packet size Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 10/15] imx_fec: Fix a typo in imx_enet_receive() Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 11/15] imx_fec: Reserve full FSL_IMX25_FEC_SIZE page for the register file Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 12/15] sdhci: Add i.MX specific subtype of SDHCI Andrey Smirnov
2017-12-15 2:31 ` Philippe Mathieu-Daudé
2017-12-15 18:51 ` Andrey Smirnov
2017-12-15 20:15 ` Philippe Mathieu-Daudé
2017-12-17 0:21 ` Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 13/15] hw: i.MX: Convert i.MX6 to use TYPE_IMX_USDHC Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 14/15] sd: Check for READ_MULTIPLE_BLOCK size limit violation first Andrey Smirnov
2017-12-14 16:45 ` Andrey Smirnov
2017-12-14 14:52 ` [Qemu-devel] [PATCH v2 15/15] sdhci: Implement write method of ACMD12ERRSTS register Andrey Smirnov
2017-12-15 2:18 ` Philippe Mathieu-Daudé
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).