* [Qemu-devel] [PATCH 01/17] imx_fec: Do not link to netdev
2017-09-18 19:50 [Qemu-devel] [PATCH 00/17] Initial i.MX7 support Andrey Smirnov
@ 2017-09-18 19:50 ` Andrey Smirnov
2017-10-06 13:46 ` Peter Maydell
2017-09-18 19:50 ` [Qemu-devel] [PATCH 02/17] imx_fec: Do not calculate FEC Andrey Smirnov
` (17 subsequent siblings)
18 siblings, 1 reply; 51+ messages in thread
From: Andrey Smirnov @ 2017-09-18 19:50 UTC (permalink / raw)
To: qemu-arm; +Cc: Andrey Smirnov, Peter Maydell, Jason Wang, 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: 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 | 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.13.5
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 01/17] imx_fec: Do not link to netdev
2017-09-18 19:50 ` [Qemu-devel] [PATCH 01/17] imx_fec: Do not link to netdev Andrey Smirnov
@ 2017-10-06 13:46 ` Peter Maydell
0 siblings, 0 replies; 51+ messages in thread
From: Peter Maydell @ 2017-10-06 13:46 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-arm, Jason Wang, QEMU Developers, Andrey Yurovsky
On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> 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: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 51+ messages in thread
* [Qemu-devel] [PATCH 02/17] imx_fec: Do not calculate FEC
2017-09-18 19:50 [Qemu-devel] [PATCH 00/17] Initial i.MX7 support Andrey Smirnov
2017-09-18 19:50 ` [Qemu-devel] [PATCH 01/17] imx_fec: Do not link to netdev Andrey Smirnov
@ 2017-09-18 19:50 ` Andrey Smirnov
2017-10-06 13:48 ` Peter Maydell
2017-09-18 19:50 ` [Qemu-devel] [PATCH 03/17] imx_fec: Refactor imx_eth_enable_rx() Andrey Smirnov
` (16 subsequent siblings)
18 siblings, 1 reply; 51+ messages in thread
From: Andrey Smirnov @ 2017-09-18 19:50 UTC (permalink / raw)
To: qemu-arm; +Cc: Andrey Smirnov, Peter Maydell, Jason Wang, qemu-devel, yurovsky
Save some computation time and avoid calculating CRC's frame
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
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 | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 88b4b049d7..75822344fc 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -1032,9 +1032,7 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
IMXENETBufDesc bd;
uint32_t flags = 0;
uint32_t addr;
- uint32_t crc;
uint32_t buf_addr;
- uint8_t *crc_ptr;
unsigned int buf_len;
size_t size = len;
@@ -1048,8 +1046,6 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
/* 4 bytes for the CRC. */
size += 4;
- crc = cpu_to_be32(crc32(~0, buf, size));
- crc_ptr = (uint8_t *) &crc;
/* Huge frames are truncted. */
if (size > ENET_MAX_FRAME_SIZE) {
@@ -1090,9 +1086,10 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
dma_memory_write(&address_space_memory, buf_addr, buf, buf_len);
buf += buf_len;
if (size < 4) {
+ const uint8_t zeros[4] = { 0 };
+
dma_memory_write(&address_space_memory, buf_addr + buf_len,
- crc_ptr, 4 - size);
- crc_ptr += 4 - size;
+ zeros, 4 - size);
}
bd.flags &= ~ENET_BD_E;
if (size == 0) {
--
2.13.5
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 02/17] imx_fec: Do not calculate FEC
2017-09-18 19:50 ` [Qemu-devel] [PATCH 02/17] imx_fec: Do not calculate FEC Andrey Smirnov
@ 2017-10-06 13:48 ` Peter Maydell
2017-10-09 14:47 ` Andrey Smirnov
0 siblings, 1 reply; 51+ messages in thread
From: Peter Maydell @ 2017-10-06 13:48 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-arm, Jason Wang, QEMU Developers, Andrey Yurovsky
On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> Save some computation time and avoid calculating CRC's frame
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> 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 | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index 88b4b049d7..75822344fc 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -1032,9 +1032,7 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
> IMXENETBufDesc bd;
> uint32_t flags = 0;
> uint32_t addr;
> - uint32_t crc;
> uint32_t buf_addr;
> - uint8_t *crc_ptr;
> unsigned int buf_len;
> size_t size = len;
>
> @@ -1048,8 +1046,6 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
>
> /* 4 bytes for the CRC. */
> size += 4;
> - crc = cpu_to_be32(crc32(~0, buf, size));
> - crc_ptr = (uint8_t *) &crc;
>
> /* Huge frames are truncted. */
> if (size > ENET_MAX_FRAME_SIZE) {
> @@ -1090,9 +1086,10 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
> dma_memory_write(&address_space_memory, buf_addr, buf, buf_len);
> buf += buf_len;
> if (size < 4) {
> + const uint8_t zeros[4] = { 0 };
> +
> dma_memory_write(&address_space_memory, buf_addr + buf_len,
> - crc_ptr, 4 - size);
> - crc_ptr += 4 - size;
> + zeros, 4 - size);
> }
> bd.flags &= ~ENET_BD_E;
> if (size == 0) {
This looks a bit odd. Doesn't the hardware calculate the CRC here?
thanks
-- PMM
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 02/17] imx_fec: Do not calculate FEC
2017-10-06 13:48 ` Peter Maydell
@ 2017-10-09 14:47 ` Andrey Smirnov
2017-10-09 17:03 ` Peter Maydell
0 siblings, 1 reply; 51+ messages in thread
From: Andrey Smirnov @ 2017-10-09 14:47 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-arm, Jason Wang, QEMU Developers, Andrey Yurovsky
On Fri, Oct 6, 2017 at 6:48 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>> Save some computation time and avoid calculating CRC's frame
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Jason Wang <jasowang@redhat.com>
>> 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 | 9 +++------
>> 1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
>> index 88b4b049d7..75822344fc 100644
>> --- a/hw/net/imx_fec.c
>> +++ b/hw/net/imx_fec.c
>> @@ -1032,9 +1032,7 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
>> IMXENETBufDesc bd;
>> uint32_t flags = 0;
>> uint32_t addr;
>> - uint32_t crc;
>> uint32_t buf_addr;
>> - uint8_t *crc_ptr;
>> unsigned int buf_len;
>> size_t size = len;
>>
>> @@ -1048,8 +1046,6 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
>>
>> /* 4 bytes for the CRC. */
>> size += 4;
>> - crc = cpu_to_be32(crc32(~0, buf, size));
>> - crc_ptr = (uint8_t *) &crc;
>>
>> /* Huge frames are truncted. */
>> if (size > ENET_MAX_FRAME_SIZE) {
>> @@ -1090,9 +1086,10 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
>> dma_memory_write(&address_space_memory, buf_addr, buf, buf_len);
>> buf += buf_len;
>> if (size < 4) {
>> + const uint8_t zeros[4] = { 0 };
>> +
>> dma_memory_write(&address_space_memory, buf_addr + buf_len,
>> - crc_ptr, 4 - size);
>> - crc_ptr += 4 - size;
>> + zeros, 4 - size);
>> }
>> bd.flags &= ~ENET_BD_E;
>> if (size == 0) {
>
> This looks a bit odd. Doesn't the hardware calculate the CRC here?
>
It does, it just seemed to me that since the hardware also has a "CRC
error" bit in its status register, there would be few if any users of
the actual calculated CRC value. Given how eTSEC emulation layer gets
away without calculating CRC I thought that it might be possible to
have this optimization here as well.
I can drop this patch if this seems risky.
Thanks,
Andrey Smirnov
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 02/17] imx_fec: Do not calculate FEC
2017-10-09 14:47 ` Andrey Smirnov
@ 2017-10-09 17:03 ` Peter Maydell
0 siblings, 0 replies; 51+ messages in thread
From: Peter Maydell @ 2017-10-09 17:03 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-arm, Jason Wang, QEMU Developers, Andrey Yurovsky
On 9 October 2017 at 15:47, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> On Fri, Oct 6, 2017 at 6:48 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> This looks a bit odd. Doesn't the hardware calculate the CRC here?
>>
>
> It does, it just seemed to me that since the hardware also has a "CRC
> error" bit in its status register, there would be few if any users of
> the actual calculated CRC value. Given how eTSEC emulation layer gets
> away without calculating CRC I thought that it might be possible to
> have this optimization here as well.
As a general rule QEMU should just emulate what the hardware does
(even if the usual guest software doesn't happen to care about
that corner of the specification).
thanks
-- PMM
^ permalink raw reply [flat|nested] 51+ messages in thread
* [Qemu-devel] [PATCH 03/17] imx_fec: Refactor imx_eth_enable_rx()
2017-09-18 19:50 [Qemu-devel] [PATCH 00/17] Initial i.MX7 support Andrey Smirnov
2017-09-18 19:50 ` [Qemu-devel] [PATCH 01/17] imx_fec: Do not link to netdev Andrey Smirnov
2017-09-18 19:50 ` [Qemu-devel] [PATCH 02/17] imx_fec: Do not calculate FEC Andrey Smirnov
@ 2017-09-18 19:50 ` Andrey Smirnov
2017-10-06 13:49 ` Peter Maydell
2017-09-18 19:50 ` [Qemu-devel] [PATCH 04/17] imx_fec: Change queue flushing heuristics Andrey Smirnov
` (15 subsequent siblings)
18 siblings, 1 reply; 51+ messages in thread
From: Andrey Smirnov @ 2017-09-18 19:50 UTC (permalink / raw)
To: qemu-arm; +Cc: Andrey Smirnov, Peter Maydell, Jason Wang, 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: 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 | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 75822344fc..84085afe09 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.13.5
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 03/17] imx_fec: Refactor imx_eth_enable_rx()
2017-09-18 19:50 ` [Qemu-devel] [PATCH 03/17] imx_fec: Refactor imx_eth_enable_rx() Andrey Smirnov
@ 2017-10-06 13:49 ` Peter Maydell
0 siblings, 0 replies; 51+ messages in thread
From: Peter Maydell @ 2017-10-06 13:49 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-arm, Jason Wang, QEMU Developers, Andrey Yurovsky
On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> 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: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 51+ messages in thread
* [Qemu-devel] [PATCH 04/17] imx_fec: Change queue flushing heuristics
2017-09-18 19:50 [Qemu-devel] [PATCH 00/17] Initial i.MX7 support Andrey Smirnov
` (2 preceding siblings ...)
2017-09-18 19:50 ` [Qemu-devel] [PATCH 03/17] imx_fec: Refactor imx_eth_enable_rx() Andrey Smirnov
@ 2017-09-18 19:50 ` Andrey Smirnov
2017-09-22 7:27 ` Jason Wang
2017-10-06 13:56 ` Peter Maydell
2017-09-18 19:50 ` [Qemu-devel] [PATCH 05/17] imx_fec: Use ENET_FTRL to determine truncation length Andrey Smirnov
` (14 subsequent siblings)
18 siblings, 2 replies; 51+ messages in thread
From: Andrey Smirnov @ 2017-09-18 19:50 UTC (permalink / raw)
To: qemu-arm; +Cc: Andrey Smirnov, Peter Maydell, Jason Wang, 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.
Change the code to remember the fact that can_receive callback was
called before Rx ring was ready and use it to make a decision if
receive queue needs to be flushed.
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
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 | 6 ++++--
include/hw/net/imx_fec.h | 1 +
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 84085afe09..767402909d 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -544,8 +544,9 @@ 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 (s->needs_flush) {
qemu_flush_queued_packets(qemu_get_queue(s->nic));
+ s->needs_flush = false;
}
s->regs[ENET_RDAR] = rx_ring_full ? 0 : ENET_RDAR_RDAR;
@@ -930,7 +931,8 @@ static int imx_eth_can_receive(NetClientState *nc)
FEC_PRINTF("\n");
- return s->regs[ENET_RDAR] ? 1 : 0;
+ s->needs_flush = !s->regs[ENET_RDAR];
+ return !!s->regs[ENET_RDAR];
}
static ssize_t imx_fec_receive(NetClientState *nc, const uint8_t *buf,
diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
index 62ad473b05..4bc8f03ec2 100644
--- a/include/hw/net/imx_fec.h
+++ b/include/hw/net/imx_fec.h
@@ -252,6 +252,7 @@ typedef struct IMXFECState {
uint32_t phy_int_mask;
bool is_fec;
+ bool needs_flush;
} IMXFECState;
#endif
--
2.13.5
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 04/17] imx_fec: Change queue flushing heuristics
2017-09-18 19:50 ` [Qemu-devel] [PATCH 04/17] imx_fec: Change queue flushing heuristics Andrey Smirnov
@ 2017-09-22 7:27 ` Jason Wang
2017-09-25 18:10 ` Andrey Smirnov
2017-10-06 13:56 ` Peter Maydell
1 sibling, 1 reply; 51+ messages in thread
From: Jason Wang @ 2017-09-22 7:27 UTC (permalink / raw)
To: Andrey Smirnov, qemu-arm; +Cc: Peter Maydell, qemu-devel, yurovsky
On 2017年09月19日 03:50, Andrey Smirnov wrote:
> 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));
> }
Not familiar with FEC, but if you are tracking 0->1 transition, why not
simply introduce a parameter of imx_eth_enable_rx() to force the flushing?
Thanks
>
> results in Rx queue never being flushed and corresponding
> I/O event beign disabled.
>
> Change the code to remember the fact that can_receive callback was
> called before Rx ring was ready and use it to make a decision if
> receive queue needs to be flushed.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> 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 | 6 ++++--
> include/hw/net/imx_fec.h | 1 +
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index 84085afe09..767402909d 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -544,8 +544,9 @@ 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 (s->needs_flush) {
> qemu_flush_queued_packets(qemu_get_queue(s->nic));
> + s->needs_flush = false;
> }
>
> s->regs[ENET_RDAR] = rx_ring_full ? 0 : ENET_RDAR_RDAR;
> @@ -930,7 +931,8 @@ static int imx_eth_can_receive(NetClientState *nc)
>
> FEC_PRINTF("\n");
>
> - return s->regs[ENET_RDAR] ? 1 : 0;
> + s->needs_flush = !s->regs[ENET_RDAR];
> + return !!s->regs[ENET_RDAR];
> }
>
> static ssize_t imx_fec_receive(NetClientState *nc, const uint8_t *buf,
> diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
> index 62ad473b05..4bc8f03ec2 100644
> --- a/include/hw/net/imx_fec.h
> +++ b/include/hw/net/imx_fec.h
> @@ -252,6 +252,7 @@ typedef struct IMXFECState {
> uint32_t phy_int_mask;
>
> bool is_fec;
> + bool needs_flush;
> } IMXFECState;
>
> #endif
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 04/17] imx_fec: Change queue flushing heuristics
2017-09-22 7:27 ` Jason Wang
@ 2017-09-25 18:10 ` Andrey Smirnov
0 siblings, 0 replies; 51+ messages in thread
From: Andrey Smirnov @ 2017-09-25 18:10 UTC (permalink / raw)
To: Jason Wang; +Cc: open list:ARM, Peter Maydell, QEMU Developers, Andrey Yurovsky
On Fri, Sep 22, 2017 at 12:27 AM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2017年09月19日 03:50, Andrey Smirnov wrote:
>>
>> 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));
>> }
>
>
> Not familiar with FEC, but if you are tracking 0->1 transition, why not
> simply introduce a parameter of imx_eth_enable_rx() to force the flushing?
>
Not sure I fully understand you, are you proposing I get rid of
"needs_flush" parameter in the device state, converting it to be a
parameter to imx_eth_enable_rx(), and then force flushing every time
imx_eth_enable_rx() is called in imx_eth_write()?
That should work, but it'll end up making the emulator code to flush
corresponding NIC queue every time the driver is done processing RX
ring. If that is not a big problem I am more than happy to make that
change.
Thanks,
Andrey Smirnov
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [PATCH 04/17] imx_fec: Change queue flushing heuristics
2017-09-18 19:50 ` [Qemu-devel] [PATCH 04/17] imx_fec: Change queue flushing heuristics Andrey Smirnov
2017-09-22 7:27 ` Jason Wang
@ 2017-10-06 13:56 ` Peter Maydell
2017-10-09 14:57 ` Andrey Smirnov
1 sibling, 1 reply; 51+ messages in thread
From: Peter Maydell @ 2017-10-06 13:56 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-arm, Jason Wang, QEMU Developers, Andrey Yurovsky
On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> 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.
>
> Change the code to remember the fact that can_receive callback was
> called before Rx ring was ready and use it to make a decision if
> receive queue needs to be flushed.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> 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 | 6 ++++--
> include/hw/net/imx_fec.h | 1 +
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index 84085afe09..767402909d 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -544,8 +544,9 @@ 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 (s->needs_flush) {
> qemu_flush_queued_packets(qemu_get_queue(s->nic));
> + s->needs_flush = false;
> }
>
> s->regs[ENET_RDAR] = rx_ring_full ? 0 : ENET_RDAR_RDAR;
> @@ -930,7 +931,8 @@ static int imx_eth_can_receive(NetClientState *nc)
>
> FEC_PRINTF("\n");
>
> - return s->regs[ENET_RDAR] ? 1 : 0;
> + s->needs_flush = !s->regs[ENET_RDAR];
> + return !!s->regs[ENET_RDAR];
> }
>
> static ssize_t imx_fec_receive(NetClientState *nc, const uint8_t *buf,
> diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
> index 62ad473b05..4bc8f03ec2 100644
> --- a/include/hw/net/imx_fec.h
> +++ b/include/hw/net/imx_fec.h
> @@ -252,6 +252,7 @@ typedef struct IMXFECState {
> uint32_t phy_int_mask;
>
> bool is_fec;
> + bool needs_flush;
> } IMXFECState;
This looks odd -- I don't think you should need extra
state here. Conceptually what you want is:
* in the can_receive callback, test some function of
various bits of device state to decide whether you can
take data
* in the rest of the device, whenever the device state
changes such that you were previously not able to take
data but now you can, call qemu_flush_queued_packets().
You shouldn't need any extra state to do this, you just
need to fix the bug where you have a code path that
flips ENET_RDAR from 0 to 1 without calling flush
(you might for instance have a helper function for
"set ENET_RDAR" that encapsulates setting the state
and arranging that flush is called).
thanks
-- PMM
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 04/17] imx_fec: Change queue flushing heuristics
2017-10-06 13:56 ` Peter Maydell
@ 2017-10-09 14:57 ` Andrey Smirnov
0 siblings, 0 replies; 51+ messages in thread
From: Andrey Smirnov @ 2017-10-09 14:57 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-arm, Jason Wang, QEMU Developers, Andrey Yurovsky
On Fri, Oct 6, 2017 at 6:56 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>> 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.
>>
>> Change the code to remember the fact that can_receive callback was
>> called before Rx ring was ready and use it to make a decision if
>> receive queue needs to be flushed.
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Jason Wang <jasowang@redhat.com>
>> 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 | 6 ++++--
>> include/hw/net/imx_fec.h | 1 +
>> 2 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
>> index 84085afe09..767402909d 100644
>> --- a/hw/net/imx_fec.c
>> +++ b/hw/net/imx_fec.c
>> @@ -544,8 +544,9 @@ 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 (s->needs_flush) {
>> qemu_flush_queued_packets(qemu_get_queue(s->nic));
>> + s->needs_flush = false;
>> }
>>
>> s->regs[ENET_RDAR] = rx_ring_full ? 0 : ENET_RDAR_RDAR;
>> @@ -930,7 +931,8 @@ static int imx_eth_can_receive(NetClientState *nc)
>>
>> FEC_PRINTF("\n");
>>
>> - return s->regs[ENET_RDAR] ? 1 : 0;
>> + s->needs_flush = !s->regs[ENET_RDAR];
>> + return !!s->regs[ENET_RDAR];
>> }
>>
>> static ssize_t imx_fec_receive(NetClientState *nc, const uint8_t *buf,
>> diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
>> index 62ad473b05..4bc8f03ec2 100644
>> --- a/include/hw/net/imx_fec.h
>> +++ b/include/hw/net/imx_fec.h
>> @@ -252,6 +252,7 @@ typedef struct IMXFECState {
>> uint32_t phy_int_mask;
>>
>> bool is_fec;
>> + bool needs_flush;
>> } IMXFECState;
>
> This looks odd -- I don't think you should need extra
> state here. Conceptually what you want is:
>
> * in the can_receive callback, test some function of
> various bits of device state to decide whether you can
> take data
> * in the rest of the device, whenever the device state
> changes such that you were previously not able to take
> data but now you can, call qemu_flush_queued_packets().
>
> You shouldn't need any extra state to do this, you just
> need to fix the bug where you have a code path that
> flips ENET_RDAR from 0 to 1 without calling flush
> (you might for instance have a helper function for
> "set ENET_RDAR" that encapsulates setting the state
> and arranging that flush is called).
>
I don't know if you've seen my response to Jason Wang, but I think he
was proposing something similar, and, as I said, that should work fine
and the only reason I didn't do it that way was to avoid doing a flush
every time that host driver drains full RX-ring and gives it back to
the IP block.
I'll give this a try in v2.
Thanks,
Andrey Smirnov
^ permalink raw reply [flat|nested] 51+ messages in thread
* [Qemu-devel] [PATCH 05/17] imx_fec: Use ENET_FTRL to determine truncation length
2017-09-18 19:50 [Qemu-devel] [PATCH 00/17] Initial i.MX7 support Andrey Smirnov
` (3 preceding siblings ...)
2017-09-18 19:50 ` [Qemu-devel] [PATCH 04/17] imx_fec: Change queue flushing heuristics Andrey Smirnov
@ 2017-09-18 19:50 ` Andrey Smirnov
2017-09-30 0:17 ` Philippe Mathieu-Daudé
2017-10-06 14:03 ` Peter Maydell
2017-09-18 19:50 ` [Qemu-devel] [PATCH 06/17] imx_fec: Use MIN instead of explicit ternary operator Andrey Smirnov
` (13 subsequent siblings)
18 siblings, 2 replies; 51+ messages in thread
From: Andrey Smirnov @ 2017-09-18 19:50 UTC (permalink / raw)
To: qemu-arm; +Cc: Andrey Smirnov, Peter Maydell, Jason Wang, 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.
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
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 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 767402909d..989c11be5f 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -1050,8 +1050,8 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
size += 4;
/* 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;
}
--
2.13.5
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 05/17] imx_fec: Use ENET_FTRL to determine truncation length
2017-09-18 19:50 ` [Qemu-devel] [PATCH 05/17] imx_fec: Use ENET_FTRL to determine truncation length Andrey Smirnov
@ 2017-09-30 0:17 ` Philippe Mathieu-Daudé
2017-10-06 14:00 ` Peter Maydell
2017-10-06 14:03 ` Peter Maydell
1 sibling, 1 reply; 51+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-09-30 0:17 UTC (permalink / raw)
To: Andrey Smirnov, qemu-arm; +Cc: Peter Maydell, Jason Wang, qemu-devel, yurovsky
Hi Andrey,
On 09/18/2017 04:50 PM, 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.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> 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 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index 767402909d..989c11be5f 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -1050,8 +1050,8 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
> size += 4;
>
> /* 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;
> }
for this to be ok you need to update imx_enet_write(), such:
case ENET_FTRL:
- s->regs[index] = value & 0x00003fff;
+ value &= 0x00003fff;
+ if (value > ENET_MAX_FRAME_SIZE) {
+ warn_report("%s: guest requested bigger "
+ "frame size than QEMU supports "
+ "(%u > %u)", value,
+ ENET_MAX_FRAME_SIZE);
+ value = ENET_MAX_FRAME_SIZE;
+ }
+ s->regs[index] = value;
break;
Regards,
Phil.
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 05/17] imx_fec: Use ENET_FTRL to determine truncation length
2017-09-30 0:17 ` Philippe Mathieu-Daudé
@ 2017-10-06 14:00 ` Peter Maydell
2017-10-09 15:19 ` Andrey Smirnov
0 siblings, 1 reply; 51+ messages in thread
From: Peter Maydell @ 2017-10-06 14:00 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Andrey Smirnov, qemu-arm, Jason Wang, QEMU Developers, Andrey Yurovsky
On 30 September 2017 at 01:17, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Andrey,
>
> On 09/18/2017 04:50 PM, 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.
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Jason Wang <jasowang@redhat.com>
>> 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 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
>> index 767402909d..989c11be5f 100644
>> --- a/hw/net/imx_fec.c
>> +++ b/hw/net/imx_fec.c
>> @@ -1050,8 +1050,8 @@ static ssize_t imx_enet_receive(NetClientState *nc,
>> const uint8_t *buf,
>> size += 4;
>> /* 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;
>> }
>
>
> for this to be ok you need to update imx_enet_write(), such:
>
> case ENET_FTRL:
> - s->regs[index] = value & 0x00003fff;
> + value &= 0x00003fff;
> + if (value > ENET_MAX_FRAME_SIZE) {
> + warn_report("%s: guest requested bigger "
> + "frame size than QEMU supports "
> + "(%u > %u)", value,
> + ENET_MAX_FRAME_SIZE);
> + value = ENET_MAX_FRAME_SIZE;
> + }
> + s->regs[index] = value;
> break;
Yes, and also an incoming-migration post_load callback that
fails migration if the value is too large.
It might be simpler to truncate to the smaller of the ENET_FTRL
register value and ENET_MAX_FRAME_SIZE.
(PS: what is the hardware behaviour if ENET_FTRL is set to
a larger value than ENET_MAX_FRAME_SIZE ?)
thanks
-- PMM
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 05/17] imx_fec: Use ENET_FTRL to determine truncation length
2017-10-06 14:00 ` Peter Maydell
@ 2017-10-09 15:19 ` Andrey Smirnov
0 siblings, 0 replies; 51+ messages in thread
From: Andrey Smirnov @ 2017-10-09 15:19 UTC (permalink / raw)
To: Peter Maydell
Cc: Philippe Mathieu-Daudé,
qemu-arm, Jason Wang, QEMU Developers, Andrey Yurovsky
On Fri, Oct 6, 2017 at 7:00 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 September 2017 at 01:17, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Hi Andrey,
>>
>> On 09/18/2017 04:50 PM, 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.
>>>
>>> Cc: Peter Maydell <peter.maydell@linaro.org>
>>> Cc: Jason Wang <jasowang@redhat.com>
>>> 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 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
>>> index 767402909d..989c11be5f 100644
>>> --- a/hw/net/imx_fec.c
>>> +++ b/hw/net/imx_fec.c
>>> @@ -1050,8 +1050,8 @@ static ssize_t imx_enet_receive(NetClientState *nc,
>>> const uint8_t *buf,
>>> size += 4;
>>> /* 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;
>>> }
>>
>>
>> for this to be ok you need to update imx_enet_write(), such:
>>
>> case ENET_FTRL:
>> - s->regs[index] = value & 0x00003fff;
>> + value &= 0x00003fff;
>> + if (value > ENET_MAX_FRAME_SIZE) {
>> + warn_report("%s: guest requested bigger "
>> + "frame size than QEMU supports "
>> + "(%u > %u)", value,
>> + ENET_MAX_FRAME_SIZE);
>> + value = ENET_MAX_FRAME_SIZE;
>> + }
>> + s->regs[index] = value;
>> break;
>
> Yes, and also an incoming-migration post_load callback that
> fails migration if the value is too large.
>
> It might be simpler to truncate to the smaller of the ENET_FTRL
> register value and ENET_MAX_FRAME_SIZE.
>
> (PS: what is the hardware behaviour if ENET_FTRL is set to
> a larger value than ENET_MAX_FRAME_SIZE ?)
>
I haven't tested it in practice, but I assume that hardware should be
capable of receiving packets of up to ENET_RCR[MAX_FL] bytes big (both
RCR[MAX_FL] and FTRL are 14-bits), since that would allow supporting
networks with jumbo frames.
What if ENET_MAX_FRAME_SIZE is increased to 16K instead? Would that
make all of that additional error checking code unnecessary?
Thanks,
Andrey Smirnov
P.S: And sure, I'll fix the typo in v2.
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [PATCH 05/17] imx_fec: Use ENET_FTRL to determine truncation length
2017-09-18 19:50 ` [Qemu-devel] [PATCH 05/17] imx_fec: Use ENET_FTRL to determine truncation length Andrey Smirnov
2017-09-30 0:17 ` Philippe Mathieu-Daudé
@ 2017-10-06 14:03 ` Peter Maydell
1 sibling, 0 replies; 51+ messages in thread
From: Peter Maydell @ 2017-10-06 14:03 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-arm, Jason Wang, QEMU Developers, Andrey Yurovsky
On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> 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.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> 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 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index 767402909d..989c11be5f 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -1050,8 +1050,8 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
> size += 4;
>
> /* 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;
> }
PS: since you're editing this bit of code anyway, can you fix
the typo in the comment in passing: should be "truncated".
thanks
-- PMM
^ permalink raw reply [flat|nested] 51+ messages in thread
* [Qemu-devel] [PATCH 06/17] imx_fec: Use MIN instead of explicit ternary operator
2017-09-18 19:50 [Qemu-devel] [PATCH 00/17] Initial i.MX7 support Andrey Smirnov
` (4 preceding siblings ...)
2017-09-18 19:50 ` [Qemu-devel] [PATCH 05/17] imx_fec: Use ENET_FTRL to determine truncation length Andrey Smirnov
@ 2017-09-18 19:50 ` Andrey Smirnov
2017-09-30 0:19 ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé
2017-09-18 19:50 ` [Qemu-devel] [PATCH 07/17] imx_fec: Emulate SHIFT16 in ENETx_RACC Andrey Smirnov
` (12 subsequent siblings)
18 siblings, 1 reply; 51+ messages in thread
From: Andrey Smirnov @ 2017-09-18 19:50 UTC (permalink / raw)
To: qemu-arm; +Cc: Andrey Smirnov, Peter Maydell, Jason Wang, qemu-devel, yurovsky
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
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 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 989c11be5f..8a77136d38 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -1074,7 +1074,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.13.5
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH 06/17] imx_fec: Use MIN instead of explicit ternary operator
2017-09-18 19:50 ` [Qemu-devel] [PATCH 06/17] imx_fec: Use MIN instead of explicit ternary operator Andrey Smirnov
@ 2017-09-30 0:19 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 51+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-09-30 0:19 UTC (permalink / raw)
To: Andrey Smirnov, qemu-arm; +Cc: Peter Maydell, Jason Wang, qemu-devel, yurovsky
On 09/18/2017 04:50 PM, Andrey Smirnov wrote:
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> 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 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index 989c11be5f..8a77136d38 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -1074,7 +1074,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]);
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> bd.length = buf_len;
> size -= buf_len;
>
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* [Qemu-devel] [PATCH 07/17] imx_fec: Emulate SHIFT16 in ENETx_RACC
2017-09-18 19:50 [Qemu-devel] [PATCH 00/17] Initial i.MX7 support Andrey Smirnov
` (5 preceding siblings ...)
2017-09-18 19:50 ` [Qemu-devel] [PATCH 06/17] imx_fec: Use MIN instead of explicit ternary operator Andrey Smirnov
@ 2017-09-18 19:50 ` Andrey Smirnov
2017-10-06 14:02 ` Peter Maydell
2017-09-18 19:50 ` [Qemu-devel] [PATCH 08/17] imx_fec: Add support for multiple Tx DMA rings Andrey Smirnov
` (11 subsequent siblings)
18 siblings, 1 reply; 51+ messages in thread
From: Andrey Smirnov @ 2017-09-18 19:50 UTC (permalink / raw)
To: qemu-arm; +Cc: Andrey Smirnov, Peter Maydell, Jason Wang, 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: 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 | 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 8a77136d38..bd62d7a75f 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,
uint32_t buf_addr;
unsigned int buf_len;
size_t size = len;
+ bool shift16 = s->regs[ENET_RACC] & ENET_RACC_SHIFT16;
FEC_PRINTF("len %d\n", (int)size);
@@ -1049,6 +1050,10 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
/* 4 bytes for the CRC. */
size += 4;
+ if (shift16) {
+ size += 2;
+ }
+
/* Huge frames are truncted. */
if (size > s->regs[ENET_FTRL]) {
size = s->regs[ENET_FTRL];
@@ -1085,6 +1090,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);
+
+ shift16 = false; /* We only do this once per Ethernet
+ * frame */
+ }
+
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 4bc8f03ec2..20a6aa98b4 100644
--- a/include/hw/net/imx_fec.h
+++ b/include/hw/net/imx_fec.h
@@ -169,6 +169,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.13.5
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 07/17] imx_fec: Emulate SHIFT16 in ENETx_RACC
2017-09-18 19:50 ` [Qemu-devel] [PATCH 07/17] imx_fec: Emulate SHIFT16 in ENETx_RACC Andrey Smirnov
@ 2017-10-06 14:02 ` Peter Maydell
2017-10-09 15:22 ` Andrey Smirnov
0 siblings, 1 reply; 51+ messages in thread
From: Peter Maydell @ 2017-10-06 14:02 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-arm, Jason Wang, QEMU Developers, Andrey Yurovsky
On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> 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: 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 | 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 8a77136d38..bd62d7a75f 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,
> uint32_t buf_addr;
> unsigned int buf_len;
> size_t size = len;
> + bool shift16 = s->regs[ENET_RACC] & ENET_RACC_SHIFT16;
>
> FEC_PRINTF("len %d\n", (int)size);
>
> @@ -1049,6 +1050,10 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
> /* 4 bytes for the CRC. */
> size += 4;
>
> + if (shift16) {
> + size += 2;
> + }
> +
> /* Huge frames are truncted. */
> if (size > s->regs[ENET_FTRL]) {
> size = s->regs[ENET_FTRL];
> @@ -1085,6 +1090,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);
> +
> + shift16 = false; /* We only do this once per Ethernet
> + * frame */
> + }
> +
Can you avoid having an end-of-source-line comment that wraps
to multiple lines, please? (put it on a line of its own, or
edit down to fit.) Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 07/17] imx_fec: Emulate SHIFT16 in ENETx_RACC
2017-10-06 14:02 ` Peter Maydell
@ 2017-10-09 15:22 ` Andrey Smirnov
0 siblings, 0 replies; 51+ messages in thread
From: Andrey Smirnov @ 2017-10-09 15:22 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-arm, Jason Wang, QEMU Developers, Andrey Yurovsky
On Fri, Oct 6, 2017 at 7:02 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>> 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: 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 | 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 8a77136d38..bd62d7a75f 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,
>> uint32_t buf_addr;
>> unsigned int buf_len;
>> size_t size = len;
>> + bool shift16 = s->regs[ENET_RACC] & ENET_RACC_SHIFT16;
>>
>> FEC_PRINTF("len %d\n", (int)size);
>>
>> @@ -1049,6 +1050,10 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
>> /* 4 bytes for the CRC. */
>> size += 4;
>>
>> + if (shift16) {
>> + size += 2;
>> + }
>> +
>> /* Huge frames are truncted. */
>> if (size > s->regs[ENET_FTRL]) {
>> size = s->regs[ENET_FTRL];
>> @@ -1085,6 +1090,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);
>> +
>> + shift16 = false; /* We only do this once per Ethernet
>> + * frame */
>> + }
>> +
>
> Can you avoid having an end-of-source-line comment that wraps
> to multiple lines, please? (put it on a line of its own, or
> edit down to fit.) Otherwise
>
Sure, will fix in v2.
Thanks,
Andrey Smirnov
^ permalink raw reply [flat|nested] 51+ messages in thread
* [Qemu-devel] [PATCH 08/17] imx_fec: Add support for multiple Tx DMA rings
2017-09-18 19:50 [Qemu-devel] [PATCH 00/17] Initial i.MX7 support Andrey Smirnov
` (6 preceding siblings ...)
2017-09-18 19:50 ` [Qemu-devel] [PATCH 07/17] imx_fec: Emulate SHIFT16 in ENETx_RACC Andrey Smirnov
@ 2017-09-18 19:50 ` Andrey Smirnov
2017-09-22 7:33 ` Jason Wang
2017-10-06 14:10 ` Peter Maydell
2017-09-18 19:50 ` [Qemu-devel] [PATCH 09/17] imx_fec: Use correct length for packet size Andrey Smirnov
` (10 subsequent siblings)
18 siblings, 2 replies; 51+ messages in thread
From: Andrey Smirnov @ 2017-09-18 19:50 UTC (permalink / raw)
To: qemu-arm; +Cc: Andrey Smirnov, Peter Maydell, Jason Wang, 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: 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 | 97 +++++++++++++++++++++++++++++++++++++++---------
include/hw/net/imx_fec.h | 23 +++++++++++-
2 files changed, 101 insertions(+), 19 deletions(-)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index bd62d7a75f..6045ffe673 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -196,6 +196,17 @@ static const char *imx_eth_reg_name(IMXFECState *s, uint32_t index)
}
}
+static const VMStateDescription vmstate_imx_eth_tx_ring = {
+ .name = "fec-tx-ring",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(descriptor, IMXFECTxRing),
+ VMSTATE_UINT32(tdsr, IMXFECTxRing),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
static const VMStateDescription vmstate_imx_eth = {
.name = TYPE_IMX_FEC,
.version_id = 2,
@@ -203,8 +214,10 @@ 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_STRUCT_ARRAY(tx_ring, IMXFECState,
+ ENET_TX_RING_NUM,
+ 1, vmstate_imx_eth_tx_ring,
+ IMXFECTxRing),
VMSTATE_UINT32(phy_status, IMXFECState),
VMSTATE_UINT32(phy_control, IMXFECState),
VMSTATE_UINT32(phy_advertise, IMXFECState),
@@ -407,7 +420,7 @@ 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;
- uint32_t addr = s->tx_descriptor;
+ uint32_t addr = s->tx_ring[0].descriptor;
while (descnt++ < IMX_MAX_DESC) {
IMXFECBufDesc bd;
@@ -448,17 +461,38 @@ static void imx_fec_do_tx(IMXFECState *s)
}
}
- s->tx_descriptor = addr;
+ s->tx_ring[0].descriptor = 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 frame[ENET_MAX_FRAME_SIZE];
uint8_t *ptr = frame;
- uint32_t addr = s->tx_descriptor;
+ IMXFECTxRing *ring;
+ uint32_t addr;
+
+ switch (index) {
+ case ENET_TDAR:
+ ring = &s->tx_ring[0];
+ break;
+ case ENET_TDAR1:
+ ring = &s->tx_ring[1];
+ break;
+ case ENET_TDAR2:
+ ring = &s->tx_ring[2];
+ break;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: bogus value for index %x\n",
+ __func__, index);
+ abort();
+ break;
+ }
+
+ addr = ring->descriptor;
while (descnt++ < IMX_MAX_DESC) {
IMXENETBufDesc bd;
@@ -502,32 +536,32 @@ static void imx_enet_do_tx(IMXFECState *s)
ptr = frame;
frame_size = 0;
if (bd.option & ENET_BD_TX_INT) {
- s->regs[ENET_EIR] |= ENET_INT_TXF;
+ s->regs[ENET_EIR] |= ring->int_txf;
}
}
if (bd.option & ENET_BD_TX_INT) {
- s->regs[ENET_EIR] |= ENET_INT_TXB;
+ s->regs[ENET_EIR] |= ring->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[ring->tdsr];
} else {
addr += sizeof(bd);
}
}
- s->tx_descriptor = addr;
+ ring->descriptor = 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);
}
@@ -586,7 +620,22 @@ static void imx_eth_reset(DeviceState *d)
}
s->rx_descriptor = 0;
- s->tx_descriptor = 0;
+
+ s->tx_ring[0].tdsr = ENET_TDSR;
+ s->tx_ring[1].tdsr = ENET_TDSR1;
+ s->tx_ring[2].tdsr = ENET_TDSR2;
+
+ s->tx_ring[0].int_txf = ENET_INT_TXF;
+ s->tx_ring[1].int_txf = ENET_INT_TXF1;
+ s->tx_ring[2].int_txf = ENET_INT_TXF2;
+
+ s->tx_ring[0].int_txb = ENET_INT_TXB;
+ s->tx_ring[1].int_txb = ENET_INT_TXB1;
+ s->tx_ring[2].int_txb = ENET_INT_TXB2;
+
+ s->tx_ring[0].descriptor = 0;
+ s->tx_ring[1].descriptor = 0;
+ s->tx_ring[2].descriptor = 0;
/* We also reset the PHY */
phy_reset(s);
@@ -814,10 +863,12 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
s->regs[index] = 0;
}
break;
+ case ENET_TDAR1: /* FALLTHROUGH */
+ case ENET_TDAR2: /* FALLTHROUGH */
case ENET_TDAR:
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;
@@ -829,8 +880,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_ring[0].descriptor = s->regs[ENET_TDSR];
+ s->tx_ring[1].descriptor = s->regs[ENET_TDSR1];
+ s->tx_ring[2].descriptor = s->regs[ENET_TDSR2];
}
break;
case ENET_MMFR:
@@ -908,7 +963,15 @@ 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_ring[0].descriptor = s->regs[index];
+ break;
+ case ENET_TDSR1:
+ s->regs[index] = value & ~7;
+ s->tx_ring[1].descriptor = s->regs[index];
+ break;
+ case ENET_TDSR2:
+ s->regs[index] = value & ~7;
+ s->tx_ring[2].descriptor = s->regs[index];
break;
case ENET_MRBR:
s->regs[index] = value & 0x00003ff0;
diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
index 20a6aa98b4..40bd29771f 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
@@ -106,13 +110,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)
@@ -233,6 +242,15 @@ typedef struct {
#define ENET_BD_BDU (1 << 31)
+#define ENET_TX_RING_NUM 3
+
+typedef struct IMXFECTxRing {
+ uint32_t descriptor;
+ uint32_t tdsr;
+ uint32_t int_txf;
+ uint32_t int_txb;
+} IMXFECTxRing;
+
typedef struct IMXFECState {
/*< private >*/
SysBusDevice parent_obj;
@@ -245,7 +263,8 @@ typedef struct IMXFECState {
uint32_t regs[ENET_MAX];
uint32_t rx_descriptor;
- uint32_t tx_descriptor;
+
+ IMXFECTxRing tx_ring[ENET_TX_RING_NUM];
uint32_t phy_status;
uint32_t phy_control;
--
2.13.5
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 08/17] imx_fec: Add support for multiple Tx DMA rings
2017-09-18 19:50 ` [Qemu-devel] [PATCH 08/17] imx_fec: Add support for multiple Tx DMA rings Andrey Smirnov
@ 2017-09-22 7:33 ` Jason Wang
2017-09-25 18:23 ` Andrey Smirnov
2017-10-06 14:10 ` Peter Maydell
1 sibling, 1 reply; 51+ messages in thread
From: Jason Wang @ 2017-09-22 7:33 UTC (permalink / raw)
To: Andrey Smirnov, qemu-arm; +Cc: Peter Maydell, qemu-devel, yurovsky
On 2017年09月19日 03:50, Andrey Smirnov wrote:
> 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: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 | 97 +++++++++++++++++++++++++++++++++++++++---------
> include/hw/net/imx_fec.h | 23 +++++++++++-
> 2 files changed, 101 insertions(+), 19 deletions(-)
Is there a register for driver to know about the version? (Looks like I
didn't find it).
Thanks
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [PATCH 08/17] imx_fec: Add support for multiple Tx DMA rings
2017-09-22 7:33 ` Jason Wang
@ 2017-09-25 18:23 ` Andrey Smirnov
0 siblings, 0 replies; 51+ messages in thread
From: Andrey Smirnov @ 2017-09-25 18:23 UTC (permalink / raw)
To: Jason Wang; +Cc: open list:ARM, Peter Maydell, QEMU Developers, Andrey Yurovsky
On Fri, Sep 22, 2017 at 12:33 AM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2017年09月19日 03:50, Andrey Smirnov wrote:
>>
>> 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: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 | 97
>> +++++++++++++++++++++++++++++++++++++++---------
>> include/hw/net/imx_fec.h | 23 +++++++++++-
>> 2 files changed, 101 insertions(+), 19 deletions(-)
>
>
> Is there a register for driver to know about the version? (Looks like I
> didn't find it).
I haven't seen anything of the sort in the datasheet and, since Linux
driver relies on DT for that information, I am inclined to think
there's none.
Thanks,
Andrey Smirnov
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [PATCH 08/17] imx_fec: Add support for multiple Tx DMA rings
2017-09-18 19:50 ` [Qemu-devel] [PATCH 08/17] imx_fec: Add support for multiple Tx DMA rings Andrey Smirnov
2017-09-22 7:33 ` Jason Wang
@ 2017-10-06 14:10 ` Peter Maydell
2017-10-09 15:38 ` Andrey Smirnov
1 sibling, 1 reply; 51+ messages in thread
From: Peter Maydell @ 2017-10-06 14:10 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-arm, Jason Wang, QEMU Developers, Andrey Yurovsky
On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> 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: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
I'm surprised this doesn't mean "we have a property on the device
to indicate what IP version it is, which the boards set". Or are
all our current boards mismodelling their ethernet devices with the
wrong number of TX rings ?
> ---
> hw/net/imx_fec.c | 97 +++++++++++++++++++++++++++++++++++++++---------
> include/hw/net/imx_fec.h | 23 +++++++++++-
> 2 files changed, 101 insertions(+), 19 deletions(-)
>
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index bd62d7a75f..6045ffe673 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -196,6 +196,17 @@ static const char *imx_eth_reg_name(IMXFECState *s, uint32_t index)
> }
> }
>
> +static const VMStateDescription vmstate_imx_eth_tx_ring = {
> + .name = "fec-tx-ring",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT32(descriptor, IMXFECTxRing),
> + VMSTATE_UINT32(tdsr, IMXFECTxRing),
> + VMSTATE_END_OF_LIST()
> + },
> +};
> +
> static const VMStateDescription vmstate_imx_eth = {
> .name = TYPE_IMX_FEC,
> .version_id = 2,
> @@ -203,8 +214,10 @@ 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_STRUCT_ARRAY(tx_ring, IMXFECState,
> + ENET_TX_RING_NUM,
> + 1, vmstate_imx_eth_tx_ring,
> + IMXFECTxRing),
> VMSTATE_UINT32(phy_status, IMXFECState),
> VMSTATE_UINT32(phy_control, IMXFECState),
> VMSTATE_UINT32(phy_advertise, IMXFECState),
This breaks migration compatibility, so you need to figure out
how you want to handle that. The simple solution is to increment
the version_id and minimum_version_id fields in vmstate_imx_eth;
that will mean migration from an old QEMU to a new one doesn't
work but it fails in a clean way. The complex way is to put the
new state into a migration subsection with a .needed function
to determine whether to use it or not. For imx I think we can
get away with the simple solution.
> @@ -407,7 +420,7 @@ 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;
> - uint32_t addr = s->tx_descriptor;
> + uint32_t addr = s->tx_ring[0].descriptor;
>
> while (descnt++ < IMX_MAX_DESC) {
> IMXFECBufDesc bd;
> @@ -448,17 +461,38 @@ static void imx_fec_do_tx(IMXFECState *s)
> }
> }
>
> - s->tx_descriptor = addr;
> + s->tx_ring[0].descriptor = 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 frame[ENET_MAX_FRAME_SIZE];
> uint8_t *ptr = frame;
> - uint32_t addr = s->tx_descriptor;
> + IMXFECTxRing *ring;
> + uint32_t addr;
> +
> + switch (index) {
> + case ENET_TDAR:
> + ring = &s->tx_ring[0];
> + break;
> + case ENET_TDAR1:
> + ring = &s->tx_ring[1];
> + break;
> + case ENET_TDAR2:
> + ring = &s->tx_ring[2];
> + break;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: bogus value for index %x\n",
> + __func__, index);
> + abort();
> + break;
> + }
> +
> + addr = ring->descriptor;
>
> while (descnt++ < IMX_MAX_DESC) {
> IMXENETBufDesc bd;
> @@ -502,32 +536,32 @@ static void imx_enet_do_tx(IMXFECState *s)
> ptr = frame;
> frame_size = 0;
> if (bd.option & ENET_BD_TX_INT) {
> - s->regs[ENET_EIR] |= ENET_INT_TXF;
> + s->regs[ENET_EIR] |= ring->int_txf;
> }
> }
> if (bd.option & ENET_BD_TX_INT) {
> - s->regs[ENET_EIR] |= ENET_INT_TXB;
> + s->regs[ENET_EIR] |= ring->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[ring->tdsr];
> } else {
> addr += sizeof(bd);
> }
> }
>
> - s->tx_descriptor = addr;
> + ring->descriptor = 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);
> }
> @@ -586,7 +620,22 @@ static void imx_eth_reset(DeviceState *d)
> }
>
> s->rx_descriptor = 0;
> - s->tx_descriptor = 0;
> +
> + s->tx_ring[0].tdsr = ENET_TDSR;
> + s->tx_ring[1].tdsr = ENET_TDSR1;
> + s->tx_ring[2].tdsr = ENET_TDSR2;
> +
> + s->tx_ring[0].int_txf = ENET_INT_TXF;
> + s->tx_ring[1].int_txf = ENET_INT_TXF1;
> + s->tx_ring[2].int_txf = ENET_INT_TXF2;
> +
> + s->tx_ring[0].int_txb = ENET_INT_TXB;
> + s->tx_ring[1].int_txb = ENET_INT_TXB1;
> + s->tx_ring[2].int_txb = ENET_INT_TXB2;
> +
> + s->tx_ring[0].descriptor = 0;
> + s->tx_ring[1].descriptor = 0;
> + s->tx_ring[2].descriptor = 0;
>
> /* We also reset the PHY */
> phy_reset(s);
> @@ -814,10 +863,12 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
> s->regs[index] = 0;
> }
> break;
> + case ENET_TDAR1: /* FALLTHROUGH */
> + case ENET_TDAR2: /* FALLTHROUGH */
> case ENET_TDAR:
> 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;
> @@ -829,8 +880,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_ring[0].descriptor = s->regs[ENET_TDSR];
> + s->tx_ring[1].descriptor = s->regs[ENET_TDSR1];
> + s->tx_ring[2].descriptor = s->regs[ENET_TDSR2];
> }
> break;
> case ENET_MMFR:
> @@ -908,7 +963,15 @@ 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_ring[0].descriptor = s->regs[index];
> + break;
> + case ENET_TDSR1:
> + s->regs[index] = value & ~7;
> + s->tx_ring[1].descriptor = s->regs[index];
> + break;
> + case ENET_TDSR2:
> + s->regs[index] = value & ~7;
> + s->tx_ring[2].descriptor = s->regs[index];
> break;
> case ENET_MRBR:
> s->regs[index] = value & 0x00003ff0;
> diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
> index 20a6aa98b4..40bd29771f 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
> @@ -106,13 +110,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)
> @@ -233,6 +242,15 @@ typedef struct {
>
> #define ENET_BD_BDU (1 << 31)
>
> +#define ENET_TX_RING_NUM 3
> +
> +typedef struct IMXFECTxRing {
> + uint32_t descriptor;
> + uint32_t tdsr;
> + uint32_t int_txf;
> + uint32_t int_txb;
Why aren't the int_txf and int_txb fields in the migration state?
What are they for? It looks from the code like they're just constants
indicating the tx interrupt bits to use for this tx ring, in which
case they'd be better implemented using a utility function which
returns the tx bit when given a ring number, I think.
> +} IMXFECTxRing;
> +
> typedef struct IMXFECState {
> /*< private >*/
> SysBusDevice parent_obj;
> @@ -245,7 +263,8 @@ typedef struct IMXFECState {
>
> uint32_t regs[ENET_MAX];
> uint32_t rx_descriptor;
> - uint32_t tx_descriptor;
> +
> + IMXFECTxRing tx_ring[ENET_TX_RING_NUM];
>
> uint32_t phy_status;
> uint32_t phy_control;
> --
> 2.13.5
thanks
-- PMM
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 08/17] imx_fec: Add support for multiple Tx DMA rings
2017-10-06 14:10 ` Peter Maydell
@ 2017-10-09 15:38 ` Andrey Smirnov
2017-10-09 17:06 ` Peter Maydell
0 siblings, 1 reply; 51+ messages in thread
From: Andrey Smirnov @ 2017-10-09 15:38 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-arm, Jason Wang, QEMU Developers, Andrey Yurovsky
On Fri, Oct 6, 2017 at 7:10 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>> 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: qemu-devel@nongnu.org
>> Cc: qemu-arm@nongnu.org
>> Cc: yurovsky@gmail.com
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>
> I'm surprised this doesn't mean "we have a property on the device
> to indicate what IP version it is, which the boards set". Or are
> all our current boards mismodelling their ethernet devices with the
> wrong number of TX rings ?
>
As far as I know the only already emulated SoC that this affects is
i.MX6, and, no, it doesn't mismodel its Ethernet device since it has
version of the IP block with only one Tx ring. I didn't add any notion
of versioning because it didn't seem necessary, since 3-ring IP block
should be backwards compatible with 1-ring version and host drivers
written for the latter will end up using only ring #1 of the 3-ring
block.
>> ---
>> hw/net/imx_fec.c | 97 +++++++++++++++++++++++++++++++++++++++---------
>> include/hw/net/imx_fec.h | 23 +++++++++++-
>> 2 files changed, 101 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
>> index bd62d7a75f..6045ffe673 100644
>> --- a/hw/net/imx_fec.c
>> +++ b/hw/net/imx_fec.c
>> @@ -196,6 +196,17 @@ static const char *imx_eth_reg_name(IMXFECState *s, uint32_t index)
>> }
>> }
>>
>> +static const VMStateDescription vmstate_imx_eth_tx_ring = {
>> + .name = "fec-tx-ring",
>> + .version_id = 1,
>> + .minimum_version_id = 1,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_UINT32(descriptor, IMXFECTxRing),
>> + VMSTATE_UINT32(tdsr, IMXFECTxRing),
>> + VMSTATE_END_OF_LIST()
>> + },
>> +};
>> +
>> static const VMStateDescription vmstate_imx_eth = {
>> .name = TYPE_IMX_FEC,
>> .version_id = 2,
>> @@ -203,8 +214,10 @@ 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_STRUCT_ARRAY(tx_ring, IMXFECState,
>> + ENET_TX_RING_NUM,
>> + 1, vmstate_imx_eth_tx_ring,
>> + IMXFECTxRing),
>> VMSTATE_UINT32(phy_status, IMXFECState),
>> VMSTATE_UINT32(phy_control, IMXFECState),
>> VMSTATE_UINT32(phy_advertise, IMXFECState),
>
> This breaks migration compatibility, so you need to figure out
> how you want to handle that. The simple solution is to increment
> the version_id and minimum_version_id fields in vmstate_imx_eth;
> that will mean migration from an old QEMU to a new one doesn't
> work but it fails in a clean way. The complex way is to put the
> new state into a migration subsection with a .needed function
> to determine whether to use it or not. For imx I think we can
> get away with the simple solution.
>
OK, will do in v2.
>> @@ -407,7 +420,7 @@ 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;
>> - uint32_t addr = s->tx_descriptor;
>> + uint32_t addr = s->tx_ring[0].descriptor;
>>
>> while (descnt++ < IMX_MAX_DESC) {
>> IMXFECBufDesc bd;
>> @@ -448,17 +461,38 @@ static void imx_fec_do_tx(IMXFECState *s)
>> }
>> }
>>
>> - s->tx_descriptor = addr;
>> + s->tx_ring[0].descriptor = 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 frame[ENET_MAX_FRAME_SIZE];
>> uint8_t *ptr = frame;
>> - uint32_t addr = s->tx_descriptor;
>> + IMXFECTxRing *ring;
>> + uint32_t addr;
>> +
>> + switch (index) {
>> + case ENET_TDAR:
>> + ring = &s->tx_ring[0];
>> + break;
>> + case ENET_TDAR1:
>> + ring = &s->tx_ring[1];
>> + break;
>> + case ENET_TDAR2:
>> + ring = &s->tx_ring[2];
>> + break;
>> + default:
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "%s: bogus value for index %x\n",
>> + __func__, index);
>> + abort();
>> + break;
>> + }
>> +
>> + addr = ring->descriptor;
>>
>> while (descnt++ < IMX_MAX_DESC) {
>> IMXENETBufDesc bd;
>> @@ -502,32 +536,32 @@ static void imx_enet_do_tx(IMXFECState *s)
>> ptr = frame;
>> frame_size = 0;
>> if (bd.option & ENET_BD_TX_INT) {
>> - s->regs[ENET_EIR] |= ENET_INT_TXF;
>> + s->regs[ENET_EIR] |= ring->int_txf;
>> }
>> }
>> if (bd.option & ENET_BD_TX_INT) {
>> - s->regs[ENET_EIR] |= ENET_INT_TXB;
>> + s->regs[ENET_EIR] |= ring->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[ring->tdsr];
>> } else {
>> addr += sizeof(bd);
>> }
>> }
>>
>> - s->tx_descriptor = addr;
>> + ring->descriptor = 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);
>> }
>> @@ -586,7 +620,22 @@ static void imx_eth_reset(DeviceState *d)
>> }
>>
>> s->rx_descriptor = 0;
>> - s->tx_descriptor = 0;
>> +
>> + s->tx_ring[0].tdsr = ENET_TDSR;
>> + s->tx_ring[1].tdsr = ENET_TDSR1;
>> + s->tx_ring[2].tdsr = ENET_TDSR2;
>> +
>> + s->tx_ring[0].int_txf = ENET_INT_TXF;
>> + s->tx_ring[1].int_txf = ENET_INT_TXF1;
>> + s->tx_ring[2].int_txf = ENET_INT_TXF2;
>> +
>> + s->tx_ring[0].int_txb = ENET_INT_TXB;
>> + s->tx_ring[1].int_txb = ENET_INT_TXB1;
>> + s->tx_ring[2].int_txb = ENET_INT_TXB2;
>> +
>> + s->tx_ring[0].descriptor = 0;
>> + s->tx_ring[1].descriptor = 0;
>> + s->tx_ring[2].descriptor = 0;
>>
>> /* We also reset the PHY */
>> phy_reset(s);
>> @@ -814,10 +863,12 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
>> s->regs[index] = 0;
>> }
>> break;
>> + case ENET_TDAR1: /* FALLTHROUGH */
>> + case ENET_TDAR2: /* FALLTHROUGH */
>> case ENET_TDAR:
>> 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;
>> @@ -829,8 +880,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_ring[0].descriptor = s->regs[ENET_TDSR];
>> + s->tx_ring[1].descriptor = s->regs[ENET_TDSR1];
>> + s->tx_ring[2].descriptor = s->regs[ENET_TDSR2];
>> }
>> break;
>> case ENET_MMFR:
>> @@ -908,7 +963,15 @@ 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_ring[0].descriptor = s->regs[index];
>> + break;
>> + case ENET_TDSR1:
>> + s->regs[index] = value & ~7;
>> + s->tx_ring[1].descriptor = s->regs[index];
>> + break;
>> + case ENET_TDSR2:
>> + s->regs[index] = value & ~7;
>> + s->tx_ring[2].descriptor = s->regs[index];
>> break;
>> case ENET_MRBR:
>> s->regs[index] = value & 0x00003ff0;
>> diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
>> index 20a6aa98b4..40bd29771f 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
>> @@ -106,13 +110,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)
>> @@ -233,6 +242,15 @@ typedef struct {
>>
>> #define ENET_BD_BDU (1 << 31)
>>
>> +#define ENET_TX_RING_NUM 3
>> +
>> +typedef struct IMXFECTxRing {
>> + uint32_t descriptor;
>> + uint32_t tdsr;
>> + uint32_t int_txf;
>> + uint32_t int_txb;
>
> Why aren't the int_txf and int_txb fields in the migration state?
> What are they for? It looks from the code like they're just constants
> indicating the tx interrupt bits to use for this tx ring, in which
> case they'd be better implemented using a utility function which
> returns the tx bit when given a ring number, I think.
>
I think I just forgot to add those fields to migration data. Any you
are right about their purpose. I'll change the patch to use functions
in v2.
Thanks,
Andrey Smirnov
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 08/17] imx_fec: Add support for multiple Tx DMA rings
2017-10-09 15:38 ` Andrey Smirnov
@ 2017-10-09 17:06 ` Peter Maydell
0 siblings, 0 replies; 51+ messages in thread
From: Peter Maydell @ 2017-10-09 17:06 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-arm, Jason Wang, QEMU Developers, Andrey Yurovsky
On 9 October 2017 at 16:38, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> On Fri, Oct 6, 2017 at 7:10 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>>> 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: qemu-devel@nongnu.org
>>> Cc: qemu-arm@nongnu.org
>>> Cc: yurovsky@gmail.com
>>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>>
>> I'm surprised this doesn't mean "we have a property on the device
>> to indicate what IP version it is, which the boards set". Or are
>> all our current boards mismodelling their ethernet devices with the
>> wrong number of TX rings ?
>>
>
> As far as I know the only already emulated SoC that this affects is
> i.MX6, and, no, it doesn't mismodel its Ethernet device since it has
> version of the IP block with only one Tx ring. I didn't add any notion
> of versioning because it didn't seem necessary, since 3-ring IP block
> should be backwards compatible with 1-ring version and host drivers
> written for the latter will end up using only ring #1 of the 3-ring
> block.
It is guest visible if the guest looks for it, though, so I think
we should have a device property to set the number of Tx rings
(or to set the version number of the IP if that's in a guest
visible register and a more useful way to model it) so we can
get it right for both boards.
thanks
-- PMM
^ permalink raw reply [flat|nested] 51+ messages in thread
* [Qemu-devel] [PATCH 09/17] imx_fec: Use correct length for packet size
2017-09-18 19:50 [Qemu-devel] [PATCH 00/17] Initial i.MX7 support Andrey Smirnov
` (7 preceding siblings ...)
2017-09-18 19:50 ` [Qemu-devel] [PATCH 08/17] imx_fec: Add support for multiple Tx DMA rings Andrey Smirnov
@ 2017-09-18 19:50 ` Andrey Smirnov
2017-10-06 14:12 ` Peter Maydell
2017-09-18 19:50 ` [Qemu-devel] [PATCH 10/17] sdhci: Add i.MX specific subtype of SDHCI Andrey Smirnov
` (9 subsequent siblings)
18 siblings, 1 reply; 51+ messages in thread
From: Andrey Smirnov @ 2017-09-18 19:50 UTC (permalink / raw)
To: qemu-arm; +Cc: Andrey Smirnov, Peter Maydell, Jason Wang, 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: 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 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 6045ffe673..c45b9648d9 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -532,7 +532,7 @@ static void imx_enet_do_tx(IMXFECState *s, uint32_t index)
}
}
/* Last buffer in frame. */
- qemu_send_packet(qemu_get_queue(s->nic), frame, len);
+ qemu_send_packet(qemu_get_queue(s->nic), frame, frame_size);
ptr = frame;
frame_size = 0;
if (bd.option & ENET_BD_TX_INT) {
--
2.13.5
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 09/17] imx_fec: Use correct length for packet size
2017-09-18 19:50 ` [Qemu-devel] [PATCH 09/17] imx_fec: Use correct length for packet size Andrey Smirnov
@ 2017-10-06 14:12 ` Peter Maydell
0 siblings, 0 replies; 51+ messages in thread
From: Peter Maydell @ 2017-10-06 14:12 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-arm, Jason Wang, QEMU Developers, Andrey Yurovsky
On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> 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: 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 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index 6045ffe673..c45b9648d9 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -532,7 +532,7 @@ static void imx_enet_do_tx(IMXFECState *s, uint32_t index)
> }
> }
> /* Last buffer in frame. */
> - qemu_send_packet(qemu_get_queue(s->nic), frame, len);
> + qemu_send_packet(qemu_get_queue(s->nic), frame, frame_size);
> ptr = frame;
> frame_size = 0;
> if (bd.option & ENET_BD_TX_INT) {
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 51+ messages in thread
* [Qemu-devel] [PATCH 10/17] sdhci: Add i.MX specific subtype of SDHCI
2017-09-18 19:50 [Qemu-devel] [PATCH 00/17] Initial i.MX7 support Andrey Smirnov
` (8 preceding siblings ...)
2017-09-18 19:50 ` [Qemu-devel] [PATCH 09/17] imx_fec: Use correct length for packet size Andrey Smirnov
@ 2017-09-18 19:50 ` Andrey Smirnov
2017-09-18 19:50 ` [Qemu-devel] [PATCH 11/17] sdhci: Implement write method of ACMD12ERRSTS register Andrey Smirnov
` (8 subsequent siblings)
18 siblings, 0 replies; 51+ messages in thread
From: Andrey Smirnov @ 2017-09-18 19:50 UTC (permalink / raw)
To: qemu-arm; +Cc: Andrey Smirnov, Peter Maydell, 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: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
hw/sd/sdhci-internal.h | 15 ++++++
hw/sd/sdhci.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++-
include/hw/sd/sdhci.h | 8 ++++
3 files changed, 144 insertions(+), 2 deletions(-)
diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index 161177cf39..7b9ed06c36 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -91,6 +91,8 @@
#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
/* R/W Power Control Register 0x0 */
#define SDHC_PWRCON 0x29
@@ -229,4 +231,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..73e7910ba9 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,127 @@ 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:
+ hostctl = SDHC_DMA_TYPE(s->hostctl) << 5;
+
+ if (s->hostctl & SDHC_CTRL_8BITBUS)
+ hostctl |= ESDHC_CTRL_8BITBUS;
+
+ if (s->hostctl & SDHC_CTRL_4BITBUS)
+ hostctl |= ESDHC_CTRL_4BITBUS;
+
+ ret = hostctl | (s->blkgap << 16) |
+ (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 = 0;
+ 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:
+ if (value & ESDHC_CTRL_8BITBUS)
+ hostctl |= SDHC_CTRL_8BITBUS;
+
+ if (value & ESDHC_CTRL_4BITBUS)
+ hostctl |= ESDHC_CTRL_4BITBUS;
+
+ hostctl |= SDHC_DMA_TYPE(value >> 5);
+
+ value &= ~0xFE;
+ value |= hostctl;
+ value &= ~0xFF00;
+ value |= s->pwrcon;
+
+ sdhci_write(opaque, offset, value, size);
+ break;
+
+ case ESDHC_MIX_CTRL:
+ /*
+ * The layout of the register is slightly different, but we
+ * don't care about those bits
+ */
+ s->trnmod = value & 0xFFFF;
+ break;
+ case SDHC_TRNMOD:
+ sdhci_write(opaque, offset, val | s->trnmod, size);
+ break;
+ case SDHC_BLKSIZE:
+ val |= 0x7 << 12;
+ default: /* FALLTHROUGH */
+ 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..903472c901 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,13 @@ 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 */
+
+ unsigned long quirks;
} SDHCIState;
+/* Controller does not provide transfer-complete interrupt when not busy */
+#define SDHCI_QUIRK_NO_BUSY_IRQ (1<<14)
+
#define TYPE_PCI_SDHCI "sdhci-pci"
#define PCI_SDHCI(obj) OBJECT_CHECK(SDHCIState, (obj), TYPE_PCI_SDHCI)
@@ -92,4 +98,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.13.5
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PATCH 11/17] sdhci: Implement write method of ACMD12ERRSTS register
2017-09-18 19:50 [Qemu-devel] [PATCH 00/17] Initial i.MX7 support Andrey Smirnov
` (9 preceding siblings ...)
2017-09-18 19:50 ` [Qemu-devel] [PATCH 10/17] sdhci: Add i.MX specific subtype of SDHCI Andrey Smirnov
@ 2017-09-18 19:50 ` Andrey Smirnov
2017-09-18 19:50 ` [Qemu-devel] [PATCH 12/17] i.MX: Add i.MX7 CCM, PMU and ANALOG device Andrey Smirnov
` (7 subsequent siblings)
18 siblings, 0 replies; 51+ messages in thread
From: Andrey Smirnov @ 2017-09-18 19:50 UTC (permalink / raw)
To: qemu-arm; +Cc: Andrey Smirnov, Peter Maydell, qemu-devel, yurovsky
Cc: Peter Maydell <peter.maydell@linaro.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/sdhci.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 73e7910ba9..9249471957 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.13.5
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PATCH 12/17] i.MX: Add i.MX7 CCM, PMU and ANALOG device
2017-09-18 19:50 [Qemu-devel] [PATCH 00/17] Initial i.MX7 support Andrey Smirnov
` (10 preceding siblings ...)
2017-09-18 19:50 ` [Qemu-devel] [PATCH 11/17] sdhci: Implement write method of ACMD12ERRSTS register Andrey Smirnov
@ 2017-09-18 19:50 ` Andrey Smirnov
2017-09-18 19:50 ` [Qemu-devel] [PATCH 13/17] i.MX: Add code to emulate i.MX2 watchdog IP block Andrey Smirnov
` (6 subsequent siblings)
18 siblings, 0 replies; 51+ messages in thread
From: Andrey Smirnov @ 2017-09-18 19:50 UTC (permalink / raw)
To: qemu-arm; +Cc: Andrey Smirnov, Peter Maydell, qemu-devel, yurovsky
Cc: Peter Maydell <peter.maydell@linaro.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/misc/Makefile.objs | 1 +
hw/misc/imx7_ccm.c | 201 +++++++++++++++++++++++++++++++++++++++++++++
include/hw/misc/imx7_ccm.h | 76 +++++++++++++++++
3 files changed, 278 insertions(+)
create mode 100644 hw/misc/imx7_ccm.c
create mode 100644 include/hw/misc/imx7_ccm.h
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 29fb922cef..ac1be05a03 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -34,6 +34,7 @@ obj-$(CONFIG_IMX) += imx31_ccm.o
obj-$(CONFIG_IMX) += imx25_ccm.o
obj-$(CONFIG_IMX) += imx6_ccm.o
obj-$(CONFIG_IMX) += imx6_src.o
+obj-$(CONFIG_IMX) += imx7_ccm.o
obj-$(CONFIG_MILKYMIST) += milkymist-hpdmc.o
obj-$(CONFIG_MILKYMIST) += milkymist-pfpu.o
obj-$(CONFIG_MAINSTONE) += mst_fpga.o
diff --git a/hw/misc/imx7_ccm.c b/hw/misc/imx7_ccm.c
new file mode 100644
index 0000000000..418aafe7cc
--- /dev/null
+++ b/hw/misc/imx7_ccm.c
@@ -0,0 +1,201 @@
+/*
+ * Copyright (c) 2017, Impinj, Inc.
+ *
+ * i.MX7 CCM, PMU and ANALOG IP blocks emulation code
+ *
+ * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/sizes.h"
+#include "hw/misc/imx7_ccm.h"
+#include "qemu/log.h"
+
+static uint32_t imx7_ccm_get_clock_frequency(IMXCCMState *dev, IMXClk clock)
+{
+ /*
+ * This function is "consumed" by GPT emulation code, however on
+ * i.MX7 each GPT block can have their own clock root. This means
+ * that this functions needs somehow to know requester's identity
+ * and the way to pass it: be it via additional IMXClk constants
+ * or by adding another argument to this method needs to be
+ * figured out
+ */
+ qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Not implemented\n",
+ TYPE_IMX7_CCM, __func__);
+ return 0;
+}
+
+static void imx7_ccm_reset(DeviceState *dev)
+{
+ IMX7CCMState *s = IMX7_CCM(dev);
+
+ s->analog[CCM_ANALOG_PLL_ARM] = 0x00002042;
+ s->analog[CCM_ANALOG_PLL_DDR] = 0x0060302c;
+ s->analog[CCM_ANALOG_PLL_DDR_SS] = 0x00000000;
+ s->analog[CCM_ANALOG_PLL_DDR_NUM] = 0x06aaac4d;
+ s->analog[CCM_ANALOG_PLL_DDR_DENOM] = 0x100003ec;
+ s->analog[CCM_ANALOG_PLL_480] = 0x00002000;
+ s->analog[CCM_ANALOG_PLL_480A] = 0x52605a56;
+ s->analog[CCM_ANALOG_PLL_480B] = 0x52525216;
+ s->analog[CCM_ANALOG_PLL_ENET] = 0x00001fc0;
+ s->analog[CCM_ANALOG_PLL_AUDIO] = 0x0001301b;
+ s->analog[CCM_ANALOG_PLL_AUDIO_SS] = 0x00000000;
+ s->analog[CCM_ANALOG_PLL_AUDIO_NUM] = 0x05f5e100;
+ s->analog[CCM_ANALOG_PLL_AUDIO_DENOM] = 0x2964619c;
+ s->analog[CCM_ANALOG_PLL_VIDEO] = 0x0008201b;
+ s->analog[CCM_ANALOG_PLL_VIDEO_SS] = 0x00000000;
+ s->analog[CCM_ANALOG_PLL_VIDEO_NUM] = 0x0000f699;
+ s->analog[CCM_ANALOG_PLL_VIDEO_DENOM] = 0x000f4240;
+ s->analog[CCM_ANALOG_PLL_MISC0] = 0x00000000;
+
+ /* all PLLs need to be locked */
+ s->analog[CCM_ANALOG_PLL_ARM] |= CCM_ANALOG_PLL_LOCK;
+ s->analog[CCM_ANALOG_PLL_DDR] |= CCM_ANALOG_PLL_LOCK;
+ s->analog[CCM_ANALOG_PLL_480] |= CCM_ANALOG_PLL_LOCK;
+ s->analog[CCM_ANALOG_PLL_480A] |= CCM_ANALOG_PLL_LOCK;
+ s->analog[CCM_ANALOG_PLL_480B] |= CCM_ANALOG_PLL_LOCK;
+ s->analog[CCM_ANALOG_PLL_ENET] |= CCM_ANALOG_PLL_LOCK;
+ s->analog[CCM_ANALOG_PLL_AUDIO] |= CCM_ANALOG_PLL_LOCK;
+ s->analog[CCM_ANALOG_PLL_VIDEO] |= CCM_ANALOG_PLL_LOCK;
+ s->analog[CCM_ANALOG_PLL_MISC0] |= CCM_ANALOG_PLL_LOCK;
+}
+
+#define CCM_INDEX(offset) (((offset) & ~(hwaddr)0xF) / sizeof(uint32_t))
+#define CCM_BITOP(offset) ((offset) & (hwaddr)0xF)
+
+enum {
+ CCM_BITOP_NONE = 0x00,
+ CCM_BITOP_SET = 0x04,
+ CCM_BITOP_CLR = 0x08,
+ CCM_BITOP_TOG = 0x0C,
+};
+
+static uint64_t imx7_set_clr_tog_read(void *opaque, hwaddr offset,
+ unsigned size)
+{
+ const uint32_t *mmio = opaque;
+
+ return (uint64_t)mmio[CCM_INDEX(offset)];
+}
+
+static void imx7_set_clr_tog_write(void *opaque, hwaddr offset,
+ uint64_t value, unsigned size)
+{
+ const uint8_t bitop = CCM_BITOP(offset);
+ const uint32_t index = CCM_INDEX(offset);
+ uint32_t *mmio = opaque;
+
+ switch (bitop) {
+ case CCM_BITOP_NONE:
+ mmio[index] = value;
+ break;
+ case CCM_BITOP_SET:
+ mmio[index] |= value;
+ break;
+ case CCM_BITOP_CLR:
+ mmio[index] &= ~value;
+ break;
+ case CCM_BITOP_TOG:
+ mmio[index] ^= value;
+ break;
+ };
+}
+
+static const struct MemoryRegionOps imx7_set_clr_tog_ops = {
+ .read = imx7_set_clr_tog_read,
+ .write = imx7_set_clr_tog_write,
+ .endianness = DEVICE_NATIVE_ENDIAN,
+ .valid = {
+ /*
+ * Our device would not work correctly if the guest was doing
+ * unaligned access. This might not be a limitation on the real
+ * device but in practice there is no reason for a guest to access
+ * this device unaligned.
+ */
+ .min_access_size = 4,
+ .max_access_size = 4,
+ .unaligned = false,
+ },
+};
+
+static void imx7_ccm_init(Object *obj)
+{
+ SysBusDevice *sd = SYS_BUS_DEVICE(obj);
+ IMX7CCMState *s = IMX7_CCM(obj);
+
+ memory_region_init(&s->mmio.container, obj, TYPE_IMX7_CCM,
+ 0x30000);
+
+ memory_region_init_io(&s->mmio.analog,
+ obj,
+ &imx7_set_clr_tog_ops,
+ s->analog,
+ TYPE_IMX7_CCM ".analog",
+ sizeof(s->analog));
+
+ memory_region_add_subregion(&s->mmio.container,
+ 0x60, &s->mmio.analog);
+
+ memory_region_init_io(&s->mmio.pmu,
+ obj,
+ &imx7_set_clr_tog_ops,
+ s->pmu,
+ TYPE_IMX7_CCM ".pmu",
+ sizeof(s->pmu));
+
+ memory_region_add_subregion(&s->mmio.container,
+ 0x200, &s->mmio.pmu);
+
+ memory_region_init_io(&s->mmio.ccm,
+ obj,
+ &imx7_set_clr_tog_ops,
+ s->ccm,
+ TYPE_IMX7_CCM ".ccm",
+ sizeof(s->ccm));
+
+ memory_region_add_subregion(&s->mmio.container,
+ 0x20000, &s->mmio.ccm);
+
+ sysbus_init_mmio(sd, &s->mmio.container);
+}
+
+static const VMStateDescription vmstate_imx7_ccm = {
+ .name = TYPE_IMX7_CCM,
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32_ARRAY(ccm, IMX7CCMState, CCM_MAX),
+ VMSTATE_UINT32_ARRAY(analog, IMX7CCMState, CCM_ANALOG_MAX),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
+static void imx7_ccm_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ IMXCCMClass *ccm = IMX_CCM_CLASS(klass);
+
+ dc->reset = imx7_ccm_reset;
+ dc->vmsd = &vmstate_imx7_ccm;
+ dc->desc = "i.MX7 Clock Control Module";
+
+ ccm->get_clock_frequency = imx7_ccm_get_clock_frequency;
+}
+
+static const TypeInfo imx7_ccm_info = {
+ .name = TYPE_IMX7_CCM,
+ .parent = TYPE_IMX_CCM,
+ .instance_size = sizeof(IMX7CCMState),
+ .instance_init = imx7_ccm_init,
+ .class_init = imx7_ccm_class_init,
+};
+
+static void imx7_ccm_register_type(void)
+{
+ type_register_static(&imx7_ccm_info);
+}
+type_init(imx7_ccm_register_type)
diff --git a/include/hw/misc/imx7_ccm.h b/include/hw/misc/imx7_ccm.h
new file mode 100644
index 0000000000..72ca10a04a
--- /dev/null
+++ b/include/hw/misc/imx7_ccm.h
@@ -0,0 +1,76 @@
+/*
+ * Copyright (c) 2017, Impinj, Inc.
+ *
+ * i.MX7 CCM, PMU and ANALOG IP blocks emulation code
+ *
+ * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef IMX7_CCM_H
+#define IMX7_CCM_H
+
+#include "hw/misc/imx_ccm.h"
+#include "qemu/bitops.h"
+
+#define REG_SET_CLR_TOG(name) name, name##_SET, name##_CLR, name##_TOG
+
+enum IMX7AnalogRegisters {
+ REG_SET_CLR_TOG(CCM_ANALOG_PLL_ARM),
+ REG_SET_CLR_TOG(CCM_ANALOG_PLL_DDR),
+ REG_SET_CLR_TOG(CCM_ANALOG_PLL_DDR_SS),
+ REG_SET_CLR_TOG(CCM_ANALOG_PLL_DDR_NUM),
+ REG_SET_CLR_TOG(CCM_ANALOG_PLL_DDR_DENOM),
+ REG_SET_CLR_TOG(CCM_ANALOG_PLL_480),
+ REG_SET_CLR_TOG(CCM_ANALOG_PLL_480A),
+ REG_SET_CLR_TOG(CCM_ANALOG_PLL_480B),
+ REG_SET_CLR_TOG(CCM_ANALOG_PLL_ENET),
+ REG_SET_CLR_TOG(CCM_ANALOG_PLL_AUDIO),
+ REG_SET_CLR_TOG(CCM_ANALOG_PLL_AUDIO_SS),
+ REG_SET_CLR_TOG(CCM_ANALOG_PLL_AUDIO_NUM),
+ REG_SET_CLR_TOG(CCM_ANALOG_PLL_AUDIO_DENOM),
+ REG_SET_CLR_TOG(CCM_ANALOG_PLL_VIDEO),
+ REG_SET_CLR_TOG(CCM_ANALOG_PLL_VIDEO_SS),
+ REG_SET_CLR_TOG(CCM_ANALOG_PLL_VIDEO_NUM),
+ REG_SET_CLR_TOG(CCM_ANALOG_PLL_VIDEO_DENOM),
+ REG_SET_CLR_TOG(CCM_ANALOG_PLL_MISC0),
+
+ CCM_ANALOG_MAX,
+
+ CCM_ANALOG_PLL_LOCK = BIT(31)
+};
+
+enum IMX7CCMRegisters {
+ CCM_MAX = 0xBC80 / sizeof(uint32_t),
+};
+
+enum IMX7PMURegisters {
+ PMU_MAX = 0x140 / sizeof(uint32_t),
+};
+
+#undef REG_SET_CLR_TOG
+
+#define TYPE_IMX7_CCM "imx7.ccm"
+#define IMX7_CCM(obj) OBJECT_CHECK(IMX7CCMState, (obj), TYPE_IMX7_CCM)
+
+typedef struct IMX6CCMState {
+ /* <private> */
+ IMXCCMState parent_obj;
+
+ /* <public> */
+ struct {
+ MemoryRegion container;
+ MemoryRegion ccm;
+ MemoryRegion pmu;
+ MemoryRegion analog;
+ } mmio;
+
+ uint32_t ccm[CCM_MAX];
+ uint32_t pmu[PMU_MAX];
+ uint32_t analog[CCM_ANALOG_MAX];
+
+} IMX7CCMState;
+
+#endif /* IMX7_CCM_H */
--
2.13.5
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PATCH 13/17] i.MX: Add code to emulate i.MX2 watchdog IP block
2017-09-18 19:50 [Qemu-devel] [PATCH 00/17] Initial i.MX7 support Andrey Smirnov
` (11 preceding siblings ...)
2017-09-18 19:50 ` [Qemu-devel] [PATCH 12/17] i.MX: Add i.MX7 CCM, PMU and ANALOG device Andrey Smirnov
@ 2017-09-18 19:50 ` Andrey Smirnov
2017-10-06 14:22 ` Peter Maydell
2017-09-18 19:50 ` [Qemu-devel] [PATCH 14/17] i.MX7: Add code to emulate SNVS IP-block Andrey Smirnov
` (5 subsequent siblings)
18 siblings, 1 reply; 51+ messages in thread
From: Andrey Smirnov @ 2017-09-18 19:50 UTC (permalink / raw)
To: qemu-arm; +Cc: Andrey Smirnov, Peter Maydell, qemu-devel, yurovsky
Add enough code to emulate i.MX2 watchdog IP block so it would be
possible to reboot the machine running Linux Guest.
Cc: Peter Maydell <peter.maydell@linaro.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/misc/Makefile.objs | 1 +
hw/misc/imx2_wdt.c | 117 +++++++++++++++++++++++++++++++++++++++++++++
include/hw/misc/imx2_wdt.h | 36 ++++++++++++++
3 files changed, 154 insertions(+)
create mode 100644 hw/misc/imx2_wdt.c
create mode 100644 include/hw/misc/imx2_wdt.h
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index ac1be05a03..c393a93456 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -35,6 +35,7 @@ obj-$(CONFIG_IMX) += imx25_ccm.o
obj-$(CONFIG_IMX) += imx6_ccm.o
obj-$(CONFIG_IMX) += imx6_src.o
obj-$(CONFIG_IMX) += imx7_ccm.o
+obj-$(CONFIG_IMX) += imx2_wdt.o
obj-$(CONFIG_MILKYMIST) += milkymist-hpdmc.o
obj-$(CONFIG_MILKYMIST) += milkymist-pfpu.o
obj-$(CONFIG_MAINSTONE) += mst_fpga.o
diff --git a/hw/misc/imx2_wdt.c b/hw/misc/imx2_wdt.c
new file mode 100644
index 0000000000..9d97a19511
--- /dev/null
+++ b/hw/misc/imx2_wdt.c
@@ -0,0 +1,117 @@
+/*
+ * Copyright (c) 2017, Impinj, Inc.
+ *
+ * i.MX2 Watchdog IP block
+ *
+ * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu-common.h"
+#include "hw/hw.h"
+#include "exec/memory.h"
+#include "exec/address-spaces.h"
+#include "hw/sysbus.h"
+#include "qemu/log.h"
+#include "qemu/timer.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/watchdog.h"
+#include "qemu/error-report.h"
+#include "qemu/sizes.h"
+
+#include "hw/misc/imx2_wdt.h"
+
+
+#define IMX2_WDT_WCR_WDA BIT(5) /* -> External Reset WDOG_B */
+#define IMX2_WDT_WCR_SRS BIT(4) /* -> Software Reset Signal */
+
+static uint64_t imx2_wdt_read(void *opaque, hwaddr addr,
+ unsigned int size)
+{
+ IMX2WdtState *s = opaque;
+ const size_t index = addr / sizeof(s->reg[0]);
+
+ if (index < ARRAY_SIZE(s->reg))
+ return s->reg[index];
+ else
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
+
+ return 0xDEADBEEF;
+}
+
+static void imx2_wdt_write(void *opaque, hwaddr addr,
+ uint64_t val64, unsigned int size)
+{
+ uint16_t value = val64;
+ IMX2WdtState *s = opaque;
+ const size_t index = addr / sizeof(s->reg[0]);
+
+ switch (index) {
+ case IMX2_WDT_WCR:
+ if (value & (IMX2_WDT_WCR_WDA | IMX2_WDT_WCR_SRS))
+ watchdog_perform_action();
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
+ }
+}
+
+static const MemoryRegionOps imx2_wdt_ops = {
+ .read = imx2_wdt_read,
+ .write = imx2_wdt_write,
+ .endianness = DEVICE_NATIVE_ENDIAN,
+ .valid = {
+ /*
+ * Our device would not work correctly if the guest was doing
+ * unaligned access. This might not be a limitation on the real
+ * device but in practice there is no reason for a guest to access
+ * this device unaligned.
+ */
+ .min_access_size = 4,
+ .max_access_size = 4,
+ .unaligned = false,
+ },
+};
+
+static void imx2_wdt_realize(DeviceState *dev, Error **errp)
+{
+ IMX2WdtState *s = IMX2_WDT(dev);
+
+ memory_region_init_io(&s->mmio, OBJECT(dev),
+ &imx2_wdt_ops, s,
+ TYPE_IMX2_WDT".mmio", SZ_64K);
+ sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
+}
+
+static void imx2_wdt_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->realize = imx2_wdt_realize;
+
+ set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+}
+
+static const TypeInfo imx2_wdt_info = {
+ .name = TYPE_IMX2_WDT,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(IMX2WdtState),
+ .class_init = imx2_wdt_class_init,
+};
+
+static WatchdogTimerModel model = {
+ .wdt_name = "imx2-watchdog",
+ .wdt_description = "i.MX2 Watchdog",
+};
+
+static void imx2_wdt_register_type(void)
+{
+ watchdog_add_model(&model);
+ type_register_static(&imx2_wdt_info);
+}
+type_init(imx2_wdt_register_type)
diff --git a/include/hw/misc/imx2_wdt.h b/include/hw/misc/imx2_wdt.h
new file mode 100644
index 0000000000..3a30ed1ef8
--- /dev/null
+++ b/include/hw/misc/imx2_wdt.h
@@ -0,0 +1,36 @@
+/*
+ * Copyright (c) 2017, Impinj, Inc.
+ *
+ * i.MX2 Watchdog IP block
+ *
+ * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef IMX2_WDT_H
+#define IMX2_WDT_H
+
+#include "qemu/bitops.h"
+#include "hw/sysbus.h"
+
+#define TYPE_IMX2_WDT "imx2.wdt"
+#define IMX2_WDT(obj) OBJECT_CHECK(IMX2WdtState, (obj), TYPE_IMX2_WDT)
+
+enum IMX2WdtRegisters {
+ IMX2_WDT_WCR,
+ IMX2_WDT_REG_NUM
+};
+
+
+typedef struct IMX2WdtState {
+ /* <private> */
+ SysBusDevice parent_obj;
+
+ MemoryRegion mmio;
+
+ uint16_t reg[IMX2_WDT_REG_NUM];
+} IMX2WdtState;
+
+#endif /* IMX7_SNVS_H */
--
2.13.5
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 13/17] i.MX: Add code to emulate i.MX2 watchdog IP block
2017-09-18 19:50 ` [Qemu-devel] [PATCH 13/17] i.MX: Add code to emulate i.MX2 watchdog IP block Andrey Smirnov
@ 2017-10-06 14:22 ` Peter Maydell
2017-10-09 15:54 ` Andrey Smirnov
0 siblings, 1 reply; 51+ messages in thread
From: Peter Maydell @ 2017-10-06 14:22 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-arm, QEMU Developers, Andrey Yurovsky
On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> Add enough code to emulate i.MX2 watchdog IP block so it would be
> possible to reboot the machine running Linux Guest.
>
> Cc: Peter Maydell <peter.maydell@linaro.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/misc/Makefile.objs | 1 +
> hw/misc/imx2_wdt.c | 117 +++++++++++++++++++++++++++++++++++++++++++++
> include/hw/misc/imx2_wdt.h | 36 ++++++++++++++
> 3 files changed, 154 insertions(+)
> create mode 100644 hw/misc/imx2_wdt.c
> create mode 100644 include/hw/misc/imx2_wdt.h
>
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index ac1be05a03..c393a93456 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -35,6 +35,7 @@ obj-$(CONFIG_IMX) += imx25_ccm.o
> obj-$(CONFIG_IMX) += imx6_ccm.o
> obj-$(CONFIG_IMX) += imx6_src.o
> obj-$(CONFIG_IMX) += imx7_ccm.o
> +obj-$(CONFIG_IMX) += imx2_wdt.o
> obj-$(CONFIG_MILKYMIST) += milkymist-hpdmc.o
> obj-$(CONFIG_MILKYMIST) += milkymist-pfpu.o
> obj-$(CONFIG_MAINSTONE) += mst_fpga.o
> diff --git a/hw/misc/imx2_wdt.c b/hw/misc/imx2_wdt.c
> new file mode 100644
> index 0000000000..9d97a19511
> --- /dev/null
> +++ b/hw/misc/imx2_wdt.c
> @@ -0,0 +1,117 @@
> +/*
> + * Copyright (c) 2017, Impinj, Inc.
> + *
> + * i.MX2 Watchdog IP block
> + *
> + * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu-common.h"
> +#include "hw/hw.h"
> +#include "exec/memory.h"
> +#include "exec/address-spaces.h"
> +#include "hw/sysbus.h"
> +#include "qemu/log.h"
> +#include "qemu/timer.h"
> +#include "sysemu/sysemu.h"
> +#include "sysemu/watchdog.h"
> +#include "qemu/error-report.h"
> +#include "qemu/sizes.h"
That's an awful lot of includes for a very simple device. Are
you sure they're all needed?
> +
> +#include "hw/misc/imx2_wdt.h"
> +
> +
> +#define IMX2_WDT_WCR_WDA BIT(5) /* -> External Reset WDOG_B */
> +#define IMX2_WDT_WCR_SRS BIT(4) /* -> Software Reset Signal */
> +
> +static uint64_t imx2_wdt_read(void *opaque, hwaddr addr,
> + unsigned int size)
> +{
> + IMX2WdtState *s = opaque;
> + const size_t index = addr / sizeof(s->reg[0]);
> +
> + if (index < ARRAY_SIZE(s->reg))
> + return s->reg[index];
> + else
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
> +
> + return 0xDEADBEEF;
> +}
> +
> +static void imx2_wdt_write(void *opaque, hwaddr addr,
> + uint64_t val64, unsigned int size)
> +{
> + uint16_t value = val64;
> + IMX2WdtState *s = opaque;
> + const size_t index = addr / sizeof(s->reg[0]);
> +
> + switch (index) {
> + case IMX2_WDT_WCR:
> + if (value & (IMX2_WDT_WCR_WDA | IMX2_WDT_WCR_SRS))
> + watchdog_perform_action();
Missing "break"?
Also checkpatch should tell you you need more braces.
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
> + }
> +}
> +
> +static const MemoryRegionOps imx2_wdt_ops = {
> + .read = imx2_wdt_read,
> + .write = imx2_wdt_write,
> + .endianness = DEVICE_NATIVE_ENDIAN,
> + .valid = {
> + /*
> + * Our device would not work correctly if the guest was doing
> + * unaligned access. This might not be a limitation on the real
> + * device but in practice there is no reason for a guest to access
> + * this device unaligned.
> + */
This sounds like it's perhaps a job for .impl.unaligned
rather than .valid.unaligned ?
> + .min_access_size = 4,
> + .max_access_size = 4,
> + .unaligned = false,
> + },
> +};
> +
> +static void imx2_wdt_realize(DeviceState *dev, Error **errp)
> +{
> + IMX2WdtState *s = IMX2_WDT(dev);
> +
> + memory_region_init_io(&s->mmio, OBJECT(dev),
> + &imx2_wdt_ops, s,
> + TYPE_IMX2_WDT".mmio", SZ_64K);
> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
> +}
> +
> +static void imx2_wdt_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->realize = imx2_wdt_realize;
> +
> + set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +}
> +
> +static const TypeInfo imx2_wdt_info = {
> + .name = TYPE_IMX2_WDT,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(IMX2WdtState),
> + .class_init = imx2_wdt_class_init,
> +};
This device needs a reset function and vmstate description.
> +
> +static WatchdogTimerModel model = {
> + .wdt_name = "imx2-watchdog",
> + .wdt_description = "i.MX2 Watchdog",
> +};
> +
> +static void imx2_wdt_register_type(void)
> +{
> + watchdog_add_model(&model);
> + type_register_static(&imx2_wdt_info);
> +}
thanks
-- PMM
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 13/17] i.MX: Add code to emulate i.MX2 watchdog IP block
2017-10-06 14:22 ` Peter Maydell
@ 2017-10-09 15:54 ` Andrey Smirnov
0 siblings, 0 replies; 51+ messages in thread
From: Andrey Smirnov @ 2017-10-09 15:54 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-arm, QEMU Developers, Andrey Yurovsky
On Fri, Oct 6, 2017 at 7:22 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>> Add enough code to emulate i.MX2 watchdog IP block so it would be
>> possible to reboot the machine running Linux Guest.
>>
>> Cc: Peter Maydell <peter.maydell@linaro.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/misc/Makefile.objs | 1 +
>> hw/misc/imx2_wdt.c | 117 +++++++++++++++++++++++++++++++++++++++++++++
>> include/hw/misc/imx2_wdt.h | 36 ++++++++++++++
>> 3 files changed, 154 insertions(+)
>> create mode 100644 hw/misc/imx2_wdt.c
>> create mode 100644 include/hw/misc/imx2_wdt.h
>>
>> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
>> index ac1be05a03..c393a93456 100644
>> --- a/hw/misc/Makefile.objs
>> +++ b/hw/misc/Makefile.objs
>> @@ -35,6 +35,7 @@ obj-$(CONFIG_IMX) += imx25_ccm.o
>> obj-$(CONFIG_IMX) += imx6_ccm.o
>> obj-$(CONFIG_IMX) += imx6_src.o
>> obj-$(CONFIG_IMX) += imx7_ccm.o
>> +obj-$(CONFIG_IMX) += imx2_wdt.o
>> obj-$(CONFIG_MILKYMIST) += milkymist-hpdmc.o
>> obj-$(CONFIG_MILKYMIST) += milkymist-pfpu.o
>> obj-$(CONFIG_MAINSTONE) += mst_fpga.o
>> diff --git a/hw/misc/imx2_wdt.c b/hw/misc/imx2_wdt.c
>> new file mode 100644
>> index 0000000000..9d97a19511
>> --- /dev/null
>> +++ b/hw/misc/imx2_wdt.c
>> @@ -0,0 +1,117 @@
>> +/*
>> + * Copyright (c) 2017, Impinj, Inc.
>> + *
>> + * i.MX2 Watchdog IP block
>> + *
>> + * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "qemu-common.h"
>> +#include "hw/hw.h"
>> +#include "exec/memory.h"
>> +#include "exec/address-spaces.h"
>> +#include "hw/sysbus.h"
>> +#include "qemu/log.h"
>> +#include "qemu/timer.h"
>> +#include "sysemu/sysemu.h"
>> +#include "sysemu/watchdog.h"
>> +#include "qemu/error-report.h"
>> +#include "qemu/sizes.h"
>
> That's an awful lot of includes for a very simple device. Are
> you sure they're all needed?
No, I am not sure. I'll double-check.
>
>> +
>> +#include "hw/misc/imx2_wdt.h"
>> +
>> +
>> +#define IMX2_WDT_WCR_WDA BIT(5) /* -> External Reset WDOG_B */
>> +#define IMX2_WDT_WCR_SRS BIT(4) /* -> Software Reset Signal */
>> +
>> +static uint64_t imx2_wdt_read(void *opaque, hwaddr addr,
>> + unsigned int size)
>> +{
>> + IMX2WdtState *s = opaque;
>> + const size_t index = addr / sizeof(s->reg[0]);
>> +
>> + if (index < ARRAY_SIZE(s->reg))
>> + return s->reg[index];
>> + else
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
>> +
>> + return 0xDEADBEEF;
>> +}
>> +
>> +static void imx2_wdt_write(void *opaque, hwaddr addr,
>> + uint64_t val64, unsigned int size)
>> +{
>> + uint16_t value = val64;
>> + IMX2WdtState *s = opaque;
>> + const size_t index = addr / sizeof(s->reg[0]);
>> +
>> + switch (index) {
>> + case IMX2_WDT_WCR:
>> + if (value & (IMX2_WDT_WCR_WDA | IMX2_WDT_WCR_SRS))
>> + watchdog_perform_action();
>
> Missing "break"?
>
Oops, good catch! Will fix in v2.
> Also checkpatch should tell you you need more braces.
>
Yeah, I definitely forgot to run this through checkpatch, sorry.
>> + default:
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
>> + }
>> +}
>> +
>> +static const MemoryRegionOps imx2_wdt_ops = {
>> + .read = imx2_wdt_read,
>> + .write = imx2_wdt_write,
>> + .endianness = DEVICE_NATIVE_ENDIAN,
>> + .valid = {
>> + /*
>> + * Our device would not work correctly if the guest was doing
>> + * unaligned access. This might not be a limitation on the real
>> + * device but in practice there is no reason for a guest to access
>> + * this device unaligned.
>> + */
>
> This sounds like it's perhaps a job for .impl.unaligned
> rather than .valid.unaligned ?
>
This was a snippet I copied from some already existing emulation
block, so I didn't pay too much attention. But yeah, now that you
suggested it, I think I should've used impl.valid as you suggest. Will
fix in v2.
>> + .min_access_size = 4,
>> + .max_access_size = 4,
>> + .unaligned = false,
>> + },
>> +};
>> +
>> +static void imx2_wdt_realize(DeviceState *dev, Error **errp)
>> +{
>> + IMX2WdtState *s = IMX2_WDT(dev);
>> +
>> + memory_region_init_io(&s->mmio, OBJECT(dev),
>> + &imx2_wdt_ops, s,
>> + TYPE_IMX2_WDT".mmio", SZ_64K);
>> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
>> +}
>> +
>> +static void imx2_wdt_class_init(ObjectClass *klass, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> + dc->realize = imx2_wdt_realize;
>> +
>> + set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>> +}
>> +
>> +static const TypeInfo imx2_wdt_info = {
>> + .name = TYPE_IMX2_WDT,
>> + .parent = TYPE_SYS_BUS_DEVICE,
>> + .instance_size = sizeof(IMX2WdtState),
>> + .class_init = imx2_wdt_class_init,
>> +};
>
> This device needs a reset function and vmstate description.
>
OK, will fix in v2.
Thanks
Andrey Smirnov
^ permalink raw reply [flat|nested] 51+ messages in thread
* [Qemu-devel] [PATCH 14/17] i.MX7: Add code to emulate SNVS IP-block
2017-09-18 19:50 [Qemu-devel] [PATCH 00/17] Initial i.MX7 support Andrey Smirnov
` (12 preceding siblings ...)
2017-09-18 19:50 ` [Qemu-devel] [PATCH 13/17] i.MX: Add code to emulate i.MX2 watchdog IP block Andrey Smirnov
@ 2017-09-18 19:50 ` Andrey Smirnov
2017-09-18 19:50 ` [Qemu-devel] [PATCH 15/17] include/qemu: Add sizes.h from Linux Andrey Smirnov
` (4 subsequent siblings)
18 siblings, 0 replies; 51+ messages in thread
From: Andrey Smirnov @ 2017-09-18 19:50 UTC (permalink / raw)
To: qemu-arm; +Cc: Andrey Smirnov, Peter Maydell, qemu-devel, yurovsky
Add code to emulate SNVS IP-block. Currently only the bits needed to
be able to emulate machine shutdown are implemented.
Cc: Peter Maydell <peter.maydell@linaro.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/misc/Makefile.objs | 1 +
hw/misc/imx7_snvs.c | 84 +++++++++++++++++++++++++++++++++++++++++++++
include/hw/misc/imx7_snvs.h | 35 +++++++++++++++++++
3 files changed, 120 insertions(+)
create mode 100644 hw/misc/imx7_snvs.c
create mode 100644 include/hw/misc/imx7_snvs.h
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index c393a93456..16cee88e0f 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -36,6 +36,7 @@ obj-$(CONFIG_IMX) += imx6_ccm.o
obj-$(CONFIG_IMX) += imx6_src.o
obj-$(CONFIG_IMX) += imx7_ccm.o
obj-$(CONFIG_IMX) += imx2_wdt.o
+obj-$(CONFIG_IMX) += imx7_snvs.o
obj-$(CONFIG_MILKYMIST) += milkymist-hpdmc.o
obj-$(CONFIG_MILKYMIST) += milkymist-pfpu.o
obj-$(CONFIG_MAINSTONE) += mst_fpga.o
diff --git a/hw/misc/imx7_snvs.c b/hw/misc/imx7_snvs.c
new file mode 100644
index 0000000000..efce0a760f
--- /dev/null
+++ b/hw/misc/imx7_snvs.c
@@ -0,0 +1,84 @@
+/*
+ * IMX7 Secure Non-Volatile Storage
+ *
+ * Copyright (c) 2017, Impinj, Inc.
+ *
+ * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ * Bare minimum emulation code needed to support being able to shut
+ * down linux guest gracefully.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/sizes.h"
+#include "hw/misc/imx7_snvs.h"
+#include "qemu/log.h"
+#include "sysemu/sysemu.h"
+
+static uint64_t imx7_snvs_read(void *opaque, hwaddr offset, unsigned size)
+{
+ return 0;
+}
+
+static void imx7_snvs_write(void *opaque, hwaddr offset,
+ uint64_t v, unsigned size)
+{
+ const uint32_t value = v;
+ const uint32_t mask = SNVS_LPCR_TOP | SNVS_LPCR_DP_EN;
+
+ if (offset == SNVS_LPCR && ((value & mask) == mask)) {
+ qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+ }
+}
+
+static const struct MemoryRegionOps imx7_snvs_ops = {
+ .read = imx7_snvs_read,
+ .write = imx7_snvs_write,
+ .endianness = DEVICE_NATIVE_ENDIAN,
+ .valid = {
+ /*
+ * Our device would not work correctly if the guest was doing
+ * unaligned access. This might not be a limitation on the real
+ * device but in practice there is no reason for a guest to access
+ * this device unaligned.
+ */
+ .min_access_size = 4,
+ .max_access_size = 4,
+ .unaligned = false,
+ },
+};
+
+static void imx7_snvs_init(Object *obj)
+{
+ SysBusDevice *sd = SYS_BUS_DEVICE(obj);
+ IMX7SNVSState *s = IMX7_SNVS(obj);
+
+ memory_region_init_io(&s->mmio, obj, &imx7_snvs_ops, s,
+ TYPE_IMX7_SNVS, 0x1000);
+
+ sysbus_init_mmio(sd, &s->mmio);
+}
+
+static void imx7_snvs_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->desc = "i.MX7 Secure Non-Volatile Storage Module";
+}
+
+static const TypeInfo imx7_snvs_info = {
+ .name = TYPE_IMX7_SNVS,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(IMX7SNVSState),
+ .instance_init = imx7_snvs_init,
+ .class_init = imx7_snvs_class_init,
+};
+
+static void imx7_snvs_register_type(void)
+{
+ type_register_static(&imx7_snvs_info);
+}
+type_init(imx7_snvs_register_type)
diff --git a/include/hw/misc/imx7_snvs.h b/include/hw/misc/imx7_snvs.h
new file mode 100644
index 0000000000..255f8f26f9
--- /dev/null
+++ b/include/hw/misc/imx7_snvs.h
@@ -0,0 +1,35 @@
+/*
+ * Copyright (c) 2017, Impinj, Inc.
+ *
+ * i.MX7 SNVS block emulation code
+ *
+ * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef IMX7_SNVS_H
+#define IMX7_SNVS_H
+
+#include "qemu/bitops.h"
+#include "hw/sysbus.h"
+
+
+enum IMX7SNVSRegisters {
+ SNVS_LPCR = 0x38,
+ SNVS_LPCR_TOP = BIT(6),
+ SNVS_LPCR_DP_EN = BIT(5)
+};
+
+#define TYPE_IMX7_SNVS "imx7.snvs"
+#define IMX7_SNVS(obj) OBJECT_CHECK(IMX7SNVSState, (obj), TYPE_IMX7_SNVS)
+
+typedef struct IMX7SNVSState {
+ /* <private> */
+ SysBusDevice parent_obj;
+
+ MemoryRegion mmio;
+} IMX7SNVSState;
+
+#endif /* IMX7_SNVS_H */
--
2.13.5
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PATCH 15/17] include/qemu: Add sizes.h from Linux
2017-09-18 19:50 [Qemu-devel] [PATCH 00/17] Initial i.MX7 support Andrey Smirnov
` (13 preceding siblings ...)
2017-09-18 19:50 ` [Qemu-devel] [PATCH 14/17] i.MX7: Add code to emulate SNVS IP-block Andrey Smirnov
@ 2017-09-18 19:50 ` Andrey Smirnov
2017-10-06 14:13 ` Peter Maydell
2017-09-18 19:50 ` [Qemu-devel] [PATCH 16/17] i.MX: Add i.MX7 SOC implementation Andrey Smirnov
` (3 subsequent siblings)
18 siblings, 1 reply; 51+ messages in thread
From: Andrey Smirnov @ 2017-09-18 19:50 UTC (permalink / raw)
To: qemu-arm; +Cc: Andrey Smirnov, Peter Maydell, qemu-devel, yurovsky
Add sizes.h from Linux to have a more readable way of specifying
MemoryRegion sizes.
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
include/qemu/sizes.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 47 insertions(+)
create mode 100644 include/qemu/sizes.h
diff --git a/include/qemu/sizes.h b/include/qemu/sizes.h
new file mode 100644
index 0000000000..9aedb9f8f6
--- /dev/null
+++ b/include/qemu/sizes.h
@@ -0,0 +1,47 @@
+/*
+ * Copy of include/linux/sizes.h
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef SIZES_H
+#define SIZES_H
+
+#define SZ_1 0x00000001
+#define SZ_2 0x00000002
+#define SZ_4 0x00000004
+#define SZ_8 0x00000008
+#define SZ_16 0x00000010
+#define SZ_32 0x00000020
+#define SZ_64 0x00000040
+#define SZ_128 0x00000080
+#define SZ_256 0x00000100
+#define SZ_512 0x00000200
+
+#define SZ_1K 0x00000400
+#define SZ_2K 0x00000800
+#define SZ_4K 0x00001000
+#define SZ_8K 0x00002000
+#define SZ_16K 0x00004000
+#define SZ_32K 0x00008000
+#define SZ_64K 0x00010000
+#define SZ_128K 0x00020000
+#define SZ_256K 0x00040000
+#define SZ_512K 0x00080000
+
+#define SZ_1M 0x00100000
+#define SZ_2M 0x00200000
+#define SZ_4M 0x00400000
+#define SZ_8M 0x00800000
+#define SZ_16M 0x01000000
+#define SZ_32M 0x02000000
+#define SZ_64M 0x04000000
+#define SZ_128M 0x08000000
+#define SZ_256M 0x10000000
+#define SZ_512M 0x20000000
+
+#define SZ_1G 0x40000000
+#define SZ_2G 0x80000000
+
+#endif /* SIZES_H */
--
2.13.5
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 15/17] include/qemu: Add sizes.h from Linux
2017-09-18 19:50 ` [Qemu-devel] [PATCH 15/17] include/qemu: Add sizes.h from Linux Andrey Smirnov
@ 2017-10-06 14:13 ` Peter Maydell
2017-10-09 15:55 ` Andrey Smirnov
0 siblings, 1 reply; 51+ messages in thread
From: Peter Maydell @ 2017-10-06 14:13 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-arm, QEMU Developers, Andrey Yurovsky
On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> Add sizes.h from Linux to have a more readable way of specifying
> MemoryRegion sizes.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
> include/qemu/sizes.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)
> create mode 100644 include/qemu/sizes.h
>
> diff --git a/include/qemu/sizes.h b/include/qemu/sizes.h
> new file mode 100644
> index 0000000000..9aedb9f8f6
> --- /dev/null
> +++ b/include/qemu/sizes.h
> @@ -0,0 +1,47 @@
> +/*
> + * Copy of include/linux/sizes.h
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
Not really convinced of the utility, and it's gpl-2-only, so no thanks.
-- PMM
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [PATCH 15/17] include/qemu: Add sizes.h from Linux
2017-10-06 14:13 ` Peter Maydell
@ 2017-10-09 15:55 ` Andrey Smirnov
0 siblings, 0 replies; 51+ messages in thread
From: Andrey Smirnov @ 2017-10-09 15:55 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-arm, QEMU Developers, Andrey Yurovsky
On Fri, Oct 6, 2017 at 7:13 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>> Add sizes.h from Linux to have a more readable way of specifying
>> MemoryRegion sizes.
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: qemu-devel@nongnu.org
>> Cc: qemu-arm@nongnu.org
>> Cc: yurovsky@gmail.com
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>> include/qemu/sizes.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 47 insertions(+)
>> create mode 100644 include/qemu/sizes.h
>>
>> diff --git a/include/qemu/sizes.h b/include/qemu/sizes.h
>> new file mode 100644
>> index 0000000000..9aedb9f8f6
>> --- /dev/null
>> +++ b/include/qemu/sizes.h
>> @@ -0,0 +1,47 @@
>> +/*
>> + * Copy of include/linux/sizes.h
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>
> Not really convinced of the utility, and it's gpl-2-only, so no thanks.
>
OK, will drop in v2.
Thanks,
Andrey Smironv
^ permalink raw reply [flat|nested] 51+ messages in thread
* [Qemu-devel] [PATCH 16/17] i.MX: Add i.MX7 SOC implementation.
2017-09-18 19:50 [Qemu-devel] [PATCH 00/17] Initial i.MX7 support Andrey Smirnov
` (14 preceding siblings ...)
2017-09-18 19:50 ` [Qemu-devel] [PATCH 15/17] include/qemu: Add sizes.h from Linux Andrey Smirnov
@ 2017-09-18 19:50 ` Andrey Smirnov
2017-10-06 14:38 ` Peter Maydell
2017-09-18 19:51 ` [Qemu-devel] [PATCH 17/17] Implement support for i.MX7 Sabre board Andrey Smirnov
` (2 subsequent siblings)
18 siblings, 1 reply; 51+ messages in thread
From: Andrey Smirnov @ 2017-09-18 19:50 UTC (permalink / raw)
To: qemu-arm; +Cc: Andrey Smirnov, Peter Maydell, qemu-devel, yurovsky
For now we only support the following devices:
* up to 2 Cortex A9 cores (SMP works with PSCI)
* A7 MPCORE (identical to A15 MPCORE)
* 7 i.MX UARTs
* 1 CCM device
* 2 Ethernet controllers (FEC)
* 3 SD controllers (USDHC)
* 1 SNVS device
* 1 WDT device
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
default-configs/arm-softmmu.mak | 1 +
hw/arm/Makefile.objs | 2 +
hw/arm/fsl-imx7.c | 327 ++++++++++++++++++++++++++++++++++++++++
include/hw/arm/fsl-imx7.h | 114 ++++++++++++++
4 files changed, 444 insertions(+)
create mode 100644 hw/arm/fsl-imx7.c
create mode 100644 include/hw/arm/fsl-imx7.h
diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index bbdd3c1d8b..98396a3ad2 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -118,6 +118,7 @@ CONFIG_ALLWINNER_A10=y
CONFIG_FSL_IMX6=y
CONFIG_FSL_IMX31=y
CONFIG_FSL_IMX25=y
+CONFIG_FSL_IMX7=y
CONFIG_IMX_I2C=y
diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index a2e56ecaae..33f6051ae3 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -19,3 +19,5 @@ obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
obj-$(CONFIG_FSL_IMX6) += fsl-imx6.o sabrelite.o
obj-$(CONFIG_ASPEED_SOC) += aspeed_soc.o aspeed.o
obj-$(CONFIG_MPS2) += mps2.o
+obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o
+
diff --git a/hw/arm/fsl-imx7.c b/hw/arm/fsl-imx7.c
new file mode 100644
index 0000000000..bd01bb7f59
--- /dev/null
+++ b/hw/arm/fsl-imx7.c
@@ -0,0 +1,327 @@
+/*
+ * Copyright (c) 2017, Impinj, Inc.
+ *
+ * i.MX7 SoC definitions
+ *
+ * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
+ *
+ * Based on hw/arm/fsl-imx6.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu-common.h"
+#include "hw/arm/fsl-imx7.h"
+#include "sysemu/sysemu.h"
+#include "qemu/error-report.h"
+
+#define NAME_SIZE 20
+
+#define for_each_cpu(i) for (i = 0; i < smp_cpus; i++)
+
+static void fsl_imx7_init(Object *obj)
+{
+ BusState *sysbus = sysbus_get_default();
+ FslIMX7State *s = FSL_IMX7(obj);
+ char name[NAME_SIZE];
+ int i;
+
+ if (smp_cpus > FSL_IMX7_NUM_CPUS) {
+ error_report("%s: Only %d CPUs are supported (%d requested)",
+ TYPE_FSL_IMX7, FSL_IMX7_NUM_CPUS, smp_cpus);
+ exit(1);
+ }
+
+ for_each_cpu(i) {
+ object_initialize(&s->cpu[i], sizeof(s->cpu[i]),
+ "cortex-a7-" TYPE_ARM_CPU);
+ snprintf(name, NAME_SIZE, "cpu%d", i);
+ object_property_add_child(obj, name, OBJECT(&s->cpu[i]),
+ &error_fatal);
+ }
+
+ /*
+ * A7MPCORE
+ */
+ object_initialize(&s->a7mpcore, sizeof(s->a7mpcore), TYPE_A15MPCORE_PRIV);
+ qdev_set_parent_bus(DEVICE(&s->a7mpcore), sysbus);
+ object_property_add_child(obj, "a7mpcore",
+ OBJECT(&s->a7mpcore), &error_fatal);
+
+ /*
+ * CCM
+ */
+ object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX7_CCM);
+ qdev_set_parent_bus(DEVICE(&s->ccm), sysbus);
+ object_property_add_child(obj, "ccm", OBJECT(&s->ccm), &error_fatal);
+
+ /*
+ * UART
+ */
+ for (i = 0; i < FSL_IMX7_NUM_UARTS; i++) {
+ object_initialize(&s->uart[i], sizeof(s->uart[i]), TYPE_IMX_SERIAL);
+ qdev_set_parent_bus(DEVICE(&s->uart[i]), sysbus);
+ snprintf(name, NAME_SIZE, "uart%d", i);
+ object_property_add_child(obj, name, OBJECT(&s->uart[i]),
+ &error_fatal);
+ }
+
+ /*
+ * Ethernet
+ */
+ for (i = 0; i < FSL_IMX7_NUM_ETHS; i++) {
+ object_initialize(&s->eth[i], sizeof(s->eth[i]), TYPE_IMX_ENET);
+ qdev_set_parent_bus(DEVICE(&s->eth[i]), sysbus);
+ snprintf(name, NAME_SIZE, "eth%d", i);
+ object_property_add_child(obj, name, OBJECT(&s->eth[i]),
+ &error_fatal);
+ }
+
+ /*
+ * SDHCI
+ */
+ for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
+ object_initialize(&s->usdhc[i], sizeof(s->usdhc[i]),
+ TYPE_IMX_USDHC);
+ qdev_set_parent_bus(DEVICE(&s->usdhc[i]), sysbus);
+ snprintf(name, NAME_SIZE, "usdhc%d", i);
+ object_property_add_child(obj, name, OBJECT(&s->usdhc[i]),
+ &error_fatal);
+ }
+
+ /*
+ * SNVS
+ */
+ object_initialize(&s->snvs, sizeof(s->snvs), TYPE_IMX7_SNVS);
+ qdev_set_parent_bus(DEVICE(&s->snvs), sysbus);
+ object_property_add_child(obj, "snvs", OBJECT(&s->snvs), &error_fatal);
+
+ /*
+ * Watchdog
+ */
+ for (i = 0; i < FSL_IMX7_NUM_WDTS; i++) {
+ object_initialize(&s->wdt[i], sizeof(s->wdt[i]), TYPE_IMX2_WDT);
+ qdev_set_parent_bus(DEVICE(&s->wdt[i]), sysbus);
+ snprintf(name, NAME_SIZE, "wdt%d", i);
+ object_property_add_child(obj, name, OBJECT(&s->wdt[i]),
+ &error_fatal);
+ }
+}
+
+static void fsl_imx7_realize(DeviceState *dev, Error **errp)
+{
+ FslIMX7State *s = FSL_IMX7(dev);
+ Object *o;
+ int i;
+ qemu_irq irq;
+
+ for_each_cpu(i) {
+ o = OBJECT(&s->cpu[i]);
+
+ object_property_set_int(o, QEMU_PSCI_CONDUIT_SMC,
+ "psci-conduit", &error_abort);
+
+ object_property_set_bool(o, false, "has_el3", &error_abort);
+
+ /* On uniprocessor, the CBAR is set to 0 */
+ if (smp_cpus > 1) {
+ object_property_set_int(o, FSL_IMX7_A7MPCORE_ADDR,
+ "reset-cbar", &error_abort);
+ }
+
+ if (i) {
+ /* Secondary CPUs start in PSCI powered-down state */
+ object_property_set_bool(o, true,
+ "start-powered-off", &error_abort);
+ }
+
+ object_property_set_bool(o, true, "realized", &error_abort);
+ }
+
+ /*
+ * A7MPCORE
+ */
+ object_property_set_int(OBJECT(&s->a7mpcore), smp_cpus, "num-cpu",
+ &error_abort);
+ object_property_set_int(OBJECT(&s->a7mpcore),
+ FSL_IMX7_MAX_IRQ + GIC_INTERNAL,
+ "num-irq", &error_abort);
+
+ object_property_set_bool(OBJECT(&s->a7mpcore), true, "realized",
+ &error_abort);
+
+ sysbus_mmio_map(SYS_BUS_DEVICE(&s->a7mpcore), 0, FSL_IMX7_A7MPCORE_ADDR);
+
+ for_each_cpu(i) {
+ SysBusDevice *sbd = SYS_BUS_DEVICE(&s->a7mpcore);
+ DeviceState *d = DEVICE(qemu_get_cpu(i));
+
+ irq = qdev_get_gpio_in(d, ARM_CPU_IRQ);
+ sysbus_connect_irq(sbd, i, irq);
+ irq = qdev_get_gpio_in(d, ARM_CPU_FIQ);
+ sysbus_connect_irq(sbd, i + smp_cpus, irq);
+ }
+
+ /*
+ * CCM
+ */
+ object_property_set_bool(OBJECT(&s->ccm), true, "realized", &error_abort);
+
+ sysbus_mmio_map(SYS_BUS_DEVICE(&s->ccm), 0, FSL_IMX7_CCM_ADDR);
+
+ /*
+ * UART
+ */
+ for (i = 0; i < FSL_IMX7_NUM_UARTS; i++) {
+ static const hwaddr FSL_IMX7_UARTn_ADDR[FSL_IMX7_NUM_UARTS] = {
+ FSL_IMX7_UART1_ADDR,
+ FSL_IMX7_UART2_ADDR,
+ FSL_IMX7_UART3_ADDR,
+ FSL_IMX7_UART4_ADDR,
+ FSL_IMX7_UART5_ADDR,
+ FSL_IMX7_UART6_ADDR,
+ FSL_IMX7_UART7_ADDR,
+ };
+
+ static const int FSL_IMX7_UARTn_IRQ[FSL_IMX7_NUM_UARTS] = {
+ FSL_IMX7_UART1_IRQ,
+ FSL_IMX7_UART2_IRQ,
+ FSL_IMX7_UART3_IRQ,
+ FSL_IMX7_UART4_IRQ,
+ FSL_IMX7_UART5_IRQ,
+ FSL_IMX7_UART6_IRQ,
+ FSL_IMX7_UART7_IRQ,
+ };
+
+
+ if (i < MAX_SERIAL_PORTS) {
+ Chardev *chr;
+ chr = serial_hds[i];
+
+ if (!chr) {
+ char *label = g_strdup_printf("imx7.uart%d", i + 1);
+ chr = qemu_chr_new(label, "null");
+ g_free(label);
+ serial_hds[i] = chr;
+ }
+
+ qdev_prop_set_chr(DEVICE(&s->uart[i]), "chardev", chr);
+ }
+
+ object_property_set_bool(OBJECT(&s->uart[i]), true, "realized",
+ &error_abort);
+
+ sysbus_mmio_map(SYS_BUS_DEVICE(&s->uart[i]), 0, FSL_IMX7_UARTn_ADDR[i]);
+
+ irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_UARTn_IRQ[i]);
+ sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart[i]), 0, irq);
+ }
+
+ /*
+ * Etherenet
+ */
+ for (i = 0; i < FSL_IMX7_NUM_ETHS; i++) {
+ static const hwaddr FSL_IMX7_ENETn_ADDR[FSL_IMX7_NUM_ETHS] = {
+ FSL_IMX7_ENET1_ADDR,
+ FSL_IMX7_ENET2_ADDR,
+ };
+
+ qdev_set_nic_properties(DEVICE(&s->eth[i]), &nd_table[i]);
+ object_property_set_bool(OBJECT(&s->eth[i]), true, "realized",
+ &error_abort);
+
+ sysbus_mmio_map(SYS_BUS_DEVICE(&s->eth[i]), 0, FSL_IMX7_ENETn_ADDR[i]);
+
+ irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_ENET_IRQ(i, 0));
+ sysbus_connect_irq(SYS_BUS_DEVICE(&s->eth[i]), 0, irq);
+ irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_ENET_IRQ(i, 3));
+ sysbus_connect_irq(SYS_BUS_DEVICE(&s->eth[i]), 1, irq);
+ }
+
+ /*
+ * USDHC
+ */
+ for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
+ static const hwaddr FSL_IMX7_USDHCn_ADDR[FSL_IMX7_NUM_USDHCS] = {
+ FSL_IMX7_USDHC1_ADDR,
+ FSL_IMX7_USDHC2_ADDR,
+ FSL_IMX7_USDHC3_ADDR,
+ };
+
+ static const int FSL_IMX7_USDHCn_IRQ[FSL_IMX7_NUM_USDHCS] = {
+ FSL_IMX7_USDHC1_IRQ,
+ FSL_IMX7_USDHC2_IRQ,
+ FSL_IMX7_USDHC3_IRQ,
+ };
+
+ object_property_set_bool(OBJECT(&s->usdhc[i]), true, "realized",
+ &error_abort);
+
+ sysbus_mmio_map(SYS_BUS_DEVICE(&s->usdhc[i]), 0,
+ FSL_IMX7_USDHCn_ADDR[i]);
+
+ irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_USDHCn_IRQ[i]);
+ sysbus_connect_irq(SYS_BUS_DEVICE(&s->usdhc[i]), 0, irq);
+ }
+
+ /*
+ * SNVS
+ */
+ object_property_set_bool(OBJECT(&s->snvs), true, "realized", &error_abort);
+
+ sysbus_mmio_map(SYS_BUS_DEVICE(&s->snvs), 0, FSL_IMX7_SNVS_ADDR);
+
+ /*
+ * Watchdog
+ */
+ for (i = 0; i < FSL_IMX7_NUM_WDTS; i++) {
+ static const hwaddr FSL_IMX7_WDOGn_ADDR[FSL_IMX7_NUM_WDTS] = {
+ FSL_IMX7_WDOG1_ADDR,
+ FSL_IMX7_WDOG2_ADDR,
+ FSL_IMX7_WDOG3_ADDR,
+ FSL_IMX7_WDOG4_ADDR,
+ };
+
+ object_property_set_bool(OBJECT(&s->wdt[i]), true, "realized",
+ &error_abort);
+
+ sysbus_mmio_map(SYS_BUS_DEVICE(&s->wdt[i]), 0, FSL_IMX7_WDOGn_ADDR[i]);
+ }
+}
+
+static void fsl_imx7_class_init(ObjectClass *oc, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(oc);
+
+ dc->realize = fsl_imx7_realize;
+
+ /*
+ * Reason: creates an ARM CPU, thus use after free(), see
+ * arm_cpu_class_init()
+ */
+ dc->user_creatable = false;
+ dc->desc = "i.MX7 SOC";
+}
+
+static const TypeInfo fsl_imx7_type_info = {
+ .name = TYPE_FSL_IMX7,
+ .parent = TYPE_DEVICE,
+ .instance_size = sizeof(FslIMX7State),
+ .instance_init = fsl_imx7_init,
+ .class_init = fsl_imx7_class_init,
+};
+
+static void fsl_imx7_register_types(void)
+{
+ type_register_static(&fsl_imx7_type_info);
+}
+type_init(fsl_imx7_register_types)
diff --git a/include/hw/arm/fsl-imx7.h b/include/hw/arm/fsl-imx7.h
new file mode 100644
index 0000000000..0f3d43e379
--- /dev/null
+++ b/include/hw/arm/fsl-imx7.h
@@ -0,0 +1,114 @@
+/*
+ * Copyright (c) 2017, Impinj, Inc.
+ *
+ * i.MX7 SoC definitions
+ *
+ * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef FSL_IMX7_H
+#define FSL_IMX7_H
+
+#include "hw/arm/arm.h"
+#include "hw/cpu/a15mpcore.h"
+#include "hw/misc/imx7_ccm.h"
+#include "hw/misc/imx7_snvs.h"
+#include "hw/misc/imx6_src.h"
+#include "hw/misc/imx2_wdt.h"
+#include "hw/char/imx_serial.h"
+#include "hw/timer/imx_gpt.h"
+#include "hw/timer/imx_epit.h"
+#include "hw/i2c/imx_i2c.h"
+#include "hw/gpio/imx_gpio.h"
+#include "hw/sd/sdhci.h"
+#include "hw/ssi/imx_spi.h"
+#include "hw/net/imx_fec.h"
+#include "exec/memory.h"
+#include "cpu.h"
+#include "qemu/sizes.h"
+
+#define TYPE_FSL_IMX7 "fsl,imx7"
+#define FSL_IMX7(obj) OBJECT_CHECK(FslIMX7State, (obj), TYPE_FSL_IMX7)
+
+enum FslIMX7Configuration {
+ FSL_IMX7_NUM_CPUS = 2,
+ FSL_IMX7_NUM_UARTS = 7,
+ FSL_IMX7_NUM_ETHS = 2,
+ FSL_IMX7_NUM_USDHCS = 3,
+ FSL_IMX7_NUM_WDTS = 4,
+};
+
+typedef struct FslIMX7State {
+ /*< private >*/
+ DeviceState parent_obj;
+
+ /*< public >*/
+ ARMCPU cpu[FSL_IMX7_NUM_CPUS];
+ A15MPPrivState a7mpcore;
+ IMX7CCMState ccm;
+ IMX7SNVSState snvs;
+ IMXSerialState uart[FSL_IMX7_NUM_UARTS];
+ IMXFECState eth[FSL_IMX7_NUM_ETHS];
+ SDHCIState usdhc[FSL_IMX7_NUM_USDHCS];
+ IMX2WdtState wdt[FSL_IMX7_NUM_WDTS];
+} FslIMX7State;
+
+enum FslIMX7MemoryMap {
+ FSL_IMX7_MMDC_ADDR = 0x80000000,
+ FSL_IMX7_MMDC_SIZE = SZ_2G,
+
+ FSL_IMX7_WDOG1_ADDR = 0x30280000,
+ FSL_IMX7_WDOG2_ADDR = 0x30290000,
+ FSL_IMX7_WDOG3_ADDR = 0x302A0000,
+ FSL_IMX7_WDOG4_ADDR = 0x302B0000,
+
+ FSL_IMX7_CCM_ADDR = 0x30360000,
+ FSL_IMX7_SNVS_ADDR = 0x30370000,
+
+ FSL_IMX7_UART1_ADDR = 0x30860000,
+ FSL_IMX7_UART2_ADDR = 0x30870000,
+ FSL_IMX7_UART3_ADDR = 0x30880000,
+ FSL_IMX7_UART4_ADDR = 0x30A60000,
+ FSL_IMX7_UART5_ADDR = 0x30A70000,
+ FSL_IMX7_UART6_ADDR = 0x30A80000,
+ FSL_IMX7_UART7_ADDR = 0x30A90000,
+
+ FSL_IMX7_ENET1_ADDR = 0x30BE0000,
+ FSL_IMX7_ENET2_ADDR = 0x30BF0000,
+
+ FSL_IMX7_USDHC1_ADDR = 0x30B40000,
+ FSL_IMX7_USDHC2_ADDR = 0x30B50000,
+ FSL_IMX7_USDHC3_ADDR = 0x30B60000,
+
+ FSL_IMX7_A7MPCORE_ADDR = 0x31000000,
+};
+
+enum FslIMX7IRQs {
+
+ FSL_IMX7_USDHC1_IRQ = 22,
+ FSL_IMX7_USDHC2_IRQ = 23,
+ FSL_IMX7_USDHC3_IRQ = 24,
+
+ FSL_IMX7_UART1_IRQ = 26,
+ FSL_IMX7_UART2_IRQ = 27,
+ FSL_IMX7_UART3_IRQ = 28,
+ FSL_IMX7_UART4_IRQ = 29,
+ FSL_IMX7_UART5_IRQ = 30,
+ FSL_IMX7_UART6_IRQ = 16,
+ FSL_IMX7_UART7_IRQ = 126,
+
+#define FSL_IMX7_ENET_IRQ(i, n) ((n) + ((i) ? 100 : 118))
+
+ FSL_IMX7_MAX_IRQ = 128,
+};
+
+#endif /* FSL_IMX7_H */
--
2.13.5
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 16/17] i.MX: Add i.MX7 SOC implementation.
2017-09-18 19:50 ` [Qemu-devel] [PATCH 16/17] i.MX: Add i.MX7 SOC implementation Andrey Smirnov
@ 2017-10-06 14:38 ` Peter Maydell
2017-10-09 16:18 ` Andrey Smirnov
0 siblings, 1 reply; 51+ messages in thread
From: Peter Maydell @ 2017-10-06 14:38 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-arm, QEMU Developers, Andrey Yurovsky
On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> For now we only support the following devices:
> * up to 2 Cortex A9 cores (SMP works with PSCI)
> * A7 MPCORE (identical to A15 MPCORE)
> * 7 i.MX UARTs
> * 1 CCM device
> * 2 Ethernet controllers (FEC)
> * 3 SD controllers (USDHC)
> * 1 SNVS device
> * 1 WDT device
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
> default-configs/arm-softmmu.mak | 1 +
> hw/arm/Makefile.objs | 2 +
> hw/arm/fsl-imx7.c | 327 ++++++++++++++++++++++++++++++++++++++++
> include/hw/arm/fsl-imx7.h | 114 ++++++++++++++
> 4 files changed, 444 insertions(+)
> create mode 100644 hw/arm/fsl-imx7.c
> create mode 100644 include/hw/arm/fsl-imx7.h
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index bbdd3c1d8b..98396a3ad2 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -118,6 +118,7 @@ CONFIG_ALLWINNER_A10=y
> CONFIG_FSL_IMX6=y
> CONFIG_FSL_IMX31=y
> CONFIG_FSL_IMX25=y
> +CONFIG_FSL_IMX7=y
>
> CONFIG_IMX_I2C=y
>
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index a2e56ecaae..33f6051ae3 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -19,3 +19,5 @@ obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
> obj-$(CONFIG_FSL_IMX6) += fsl-imx6.o sabrelite.o
> obj-$(CONFIG_ASPEED_SOC) += aspeed_soc.o aspeed.o
> obj-$(CONFIG_MPS2) += mps2.o
> +obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o
> +
> diff --git a/hw/arm/fsl-imx7.c b/hw/arm/fsl-imx7.c
> new file mode 100644
> index 0000000000..bd01bb7f59
> --- /dev/null
> +++ b/hw/arm/fsl-imx7.c
> @@ -0,0 +1,327 @@
> +/*
> + * Copyright (c) 2017, Impinj, Inc.
> + *
> + * i.MX7 SoC definitions
> + *
> + * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
> + *
> + * Based on hw/arm/fsl-imx6.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
We pretty strongly prefer GPL-2-or-later for new code, not 2-only.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu-common.h"
> +#include "hw/arm/fsl-imx7.h"
> +#include "sysemu/sysemu.h"
> +#include "qemu/error-report.h"
> +
> +#define NAME_SIZE 20
> +
> +#define for_each_cpu(i) for (i = 0; i < smp_cpus; i++)
Just open-code this, please.
> +
> +static void fsl_imx7_init(Object *obj)
> +{
> + BusState *sysbus = sysbus_get_default();
> + FslIMX7State *s = FSL_IMX7(obj);
> + char name[NAME_SIZE];
> + int i;
> +
> + if (smp_cpus > FSL_IMX7_NUM_CPUS) {
> + error_report("%s: Only %d CPUs are supported (%d requested)",
> + TYPE_FSL_IMX7, FSL_IMX7_NUM_CPUS, smp_cpus);
> + exit(1);
> + }
> +
> + for_each_cpu(i) {
> + object_initialize(&s->cpu[i], sizeof(s->cpu[i]),
> + "cortex-a7-" TYPE_ARM_CPU);
> + snprintf(name, NAME_SIZE, "cpu%d", i);
> + object_property_add_child(obj, name, OBJECT(&s->cpu[i]),
> + &error_fatal);
> + }
> +
> + /*
> + * A7MPCORE
> + */
> + object_initialize(&s->a7mpcore, sizeof(s->a7mpcore), TYPE_A15MPCORE_PRIV);
Stealing the A15MPCORE device for a7 is kind of ugly, but I can't
decide what would be better instead...
> + qdev_set_parent_bus(DEVICE(&s->a7mpcore), sysbus);
> + object_property_add_child(obj, "a7mpcore",
> + OBJECT(&s->a7mpcore), &error_fatal);
> +
> + /*
> + * CCM
> + */
> + object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX7_CCM);
> + qdev_set_parent_bus(DEVICE(&s->ccm), sysbus);
> + object_property_add_child(obj, "ccm", OBJECT(&s->ccm), &error_fatal);
> +
> + /*
> + * UART
> + */
> + for (i = 0; i < FSL_IMX7_NUM_UARTS; i++) {
> + object_initialize(&s->uart[i], sizeof(s->uart[i]), TYPE_IMX_SERIAL);
> + qdev_set_parent_bus(DEVICE(&s->uart[i]), sysbus);
> + snprintf(name, NAME_SIZE, "uart%d", i);
> + object_property_add_child(obj, name, OBJECT(&s->uart[i]),
> + &error_fatal);
> + }
> +
> + /*
> + * Ethernet
> + */
> + for (i = 0; i < FSL_IMX7_NUM_ETHS; i++) {
> + object_initialize(&s->eth[i], sizeof(s->eth[i]), TYPE_IMX_ENET);
> + qdev_set_parent_bus(DEVICE(&s->eth[i]), sysbus);
> + snprintf(name, NAME_SIZE, "eth%d", i);
> + object_property_add_child(obj, name, OBJECT(&s->eth[i]),
> + &error_fatal);
> + }
> +
> + /*
> + * SDHCI
> + */
> + for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
> + object_initialize(&s->usdhc[i], sizeof(s->usdhc[i]),
> + TYPE_IMX_USDHC);
> + qdev_set_parent_bus(DEVICE(&s->usdhc[i]), sysbus);
> + snprintf(name, NAME_SIZE, "usdhc%d", i);
> + object_property_add_child(obj, name, OBJECT(&s->usdhc[i]),
> + &error_fatal);
> + }
> +
> + /*
> + * SNVS
> + */
> + object_initialize(&s->snvs, sizeof(s->snvs), TYPE_IMX7_SNVS);
> + qdev_set_parent_bus(DEVICE(&s->snvs), sysbus);
> + object_property_add_child(obj, "snvs", OBJECT(&s->snvs), &error_fatal);
> +
> + /*
> + * Watchdog
> + */
> + for (i = 0; i < FSL_IMX7_NUM_WDTS; i++) {
> + object_initialize(&s->wdt[i], sizeof(s->wdt[i]), TYPE_IMX2_WDT);
> + qdev_set_parent_bus(DEVICE(&s->wdt[i]), sysbus);
> + snprintf(name, NAME_SIZE, "wdt%d", i);
> + object_property_add_child(obj, name, OBJECT(&s->wdt[i]),
> + &error_fatal);
> + }
> +}
> +
> +static void fsl_imx7_realize(DeviceState *dev, Error **errp)
> +{
> + FslIMX7State *s = FSL_IMX7(dev);
> + Object *o;
> + int i;
> + qemu_irq irq;
> +
> + for_each_cpu(i) {
> + o = OBJECT(&s->cpu[i]);
> +
> + object_property_set_int(o, QEMU_PSCI_CONDUIT_SMC,
> + "psci-conduit", &error_abort);
> +
> + object_property_set_bool(o, false, "has_el3", &error_abort);
Does this SoC's CPU really not have EL3?
> +
> + /* On uniprocessor, the CBAR is set to 0 */
> + if (smp_cpus > 1) {
> + object_property_set_int(o, FSL_IMX7_A7MPCORE_ADDR,
> + "reset-cbar", &error_abort);
> + }
> +
> + if (i) {
> + /* Secondary CPUs start in PSCI powered-down state */
> + object_property_set_bool(o, true,
> + "start-powered-off", &error_abort);
> + }
> +
> + object_property_set_bool(o, true, "realized", &error_abort);
> + }
> +
> + /*
> + * A7MPCORE
> + */
> + object_property_set_int(OBJECT(&s->a7mpcore), smp_cpus, "num-cpu",
> + &error_abort);
> + object_property_set_int(OBJECT(&s->a7mpcore),
> + FSL_IMX7_MAX_IRQ + GIC_INTERNAL,
> + "num-irq", &error_abort);
> +
> + object_property_set_bool(OBJECT(&s->a7mpcore), true, "realized",
> + &error_abort);
> +
> + sysbus_mmio_map(SYS_BUS_DEVICE(&s->a7mpcore), 0, FSL_IMX7_A7MPCORE_ADDR);
> +
> + for_each_cpu(i) {
> + SysBusDevice *sbd = SYS_BUS_DEVICE(&s->a7mpcore);
> + DeviceState *d = DEVICE(qemu_get_cpu(i));
> +
> + irq = qdev_get_gpio_in(d, ARM_CPU_IRQ);
> + sysbus_connect_irq(sbd, i, irq);
> + irq = qdev_get_gpio_in(d, ARM_CPU_FIQ);
> + sysbus_connect_irq(sbd, i + smp_cpus, irq);
> + }
> +
> + /*
> + * CCM
> + */
> + object_property_set_bool(OBJECT(&s->ccm), true, "realized", &error_abort);
> +
> + sysbus_mmio_map(SYS_BUS_DEVICE(&s->ccm), 0, FSL_IMX7_CCM_ADDR);
> +
> + /*
> + * UART
> + */
> + for (i = 0; i < FSL_IMX7_NUM_UARTS; i++) {
> + static const hwaddr FSL_IMX7_UARTn_ADDR[FSL_IMX7_NUM_UARTS] = {
> + FSL_IMX7_UART1_ADDR,
> + FSL_IMX7_UART2_ADDR,
> + FSL_IMX7_UART3_ADDR,
> + FSL_IMX7_UART4_ADDR,
> + FSL_IMX7_UART5_ADDR,
> + FSL_IMX7_UART6_ADDR,
> + FSL_IMX7_UART7_ADDR,
> + };
> +
> + static const int FSL_IMX7_UARTn_IRQ[FSL_IMX7_NUM_UARTS] = {
> + FSL_IMX7_UART1_IRQ,
> + FSL_IMX7_UART2_IRQ,
> + FSL_IMX7_UART3_IRQ,
> + FSL_IMX7_UART4_IRQ,
> + FSL_IMX7_UART5_IRQ,
> + FSL_IMX7_UART6_IRQ,
> + FSL_IMX7_UART7_IRQ,
> + };
> +
> +
> + if (i < MAX_SERIAL_PORTS) {
> + Chardev *chr;
> + chr = serial_hds[i];
> +
> + if (!chr) {
> + char *label = g_strdup_printf("imx7.uart%d", i + 1);
> + chr = qemu_chr_new(label, "null");
> + g_free(label);
> + serial_hds[i] = chr;
> + }
I think the conclusion we've come to about chardevs is that the
UART should handle being passed a NULL chardev, rather than the
board code having to create a dummy chardev to pass to it.
> +
> + qdev_prop_set_chr(DEVICE(&s->uart[i]), "chardev", chr);
> + }
> +
> + object_property_set_bool(OBJECT(&s->uart[i]), true, "realized",
> + &error_abort);
> +
> + sysbus_mmio_map(SYS_BUS_DEVICE(&s->uart[i]), 0, FSL_IMX7_UARTn_ADDR[i]);
> +
> + irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_UARTn_IRQ[i]);
> + sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart[i]), 0, irq);
> + }
> +
> + /*
> + * Etherenet
Typo: "Ethernet"
> + */
> + for (i = 0; i < FSL_IMX7_NUM_ETHS; i++) {
> + static const hwaddr FSL_IMX7_ENETn_ADDR[FSL_IMX7_NUM_ETHS] = {
> + FSL_IMX7_ENET1_ADDR,
> + FSL_IMX7_ENET2_ADDR,
> + };
> +
> + qdev_set_nic_properties(DEVICE(&s->eth[i]), &nd_table[i]);
> + object_property_set_bool(OBJECT(&s->eth[i]), true, "realized",
> + &error_abort);
> +
> + sysbus_mmio_map(SYS_BUS_DEVICE(&s->eth[i]), 0, FSL_IMX7_ENETn_ADDR[i]);
> +
> + irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_ENET_IRQ(i, 0));
> + sysbus_connect_irq(SYS_BUS_DEVICE(&s->eth[i]), 0, irq);
> + irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_ENET_IRQ(i, 3));
> + sysbus_connect_irq(SYS_BUS_DEVICE(&s->eth[i]), 1, irq);
> + }
> +
> + /*
> + * USDHC
> + */
> + for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
> + static const hwaddr FSL_IMX7_USDHCn_ADDR[FSL_IMX7_NUM_USDHCS] = {
> + FSL_IMX7_USDHC1_ADDR,
> + FSL_IMX7_USDHC2_ADDR,
> + FSL_IMX7_USDHC3_ADDR,
> + };
> +
> + static const int FSL_IMX7_USDHCn_IRQ[FSL_IMX7_NUM_USDHCS] = {
> + FSL_IMX7_USDHC1_IRQ,
> + FSL_IMX7_USDHC2_IRQ,
> + FSL_IMX7_USDHC3_IRQ,
> + };
> +
> + object_property_set_bool(OBJECT(&s->usdhc[i]), true, "realized",
> + &error_abort);
> +
> + sysbus_mmio_map(SYS_BUS_DEVICE(&s->usdhc[i]), 0,
> + FSL_IMX7_USDHCn_ADDR[i]);
> +
> + irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_USDHCn_IRQ[i]);
> + sysbus_connect_irq(SYS_BUS_DEVICE(&s->usdhc[i]), 0, irq);
> + }
> +
> + /*
> + * SNVS
> + */
> + object_property_set_bool(OBJECT(&s->snvs), true, "realized", &error_abort);
> +
> + sysbus_mmio_map(SYS_BUS_DEVICE(&s->snvs), 0, FSL_IMX7_SNVS_ADDR);
> +
> + /*
> + * Watchdog
> + */
> + for (i = 0; i < FSL_IMX7_NUM_WDTS; i++) {
> + static const hwaddr FSL_IMX7_WDOGn_ADDR[FSL_IMX7_NUM_WDTS] = {
> + FSL_IMX7_WDOG1_ADDR,
> + FSL_IMX7_WDOG2_ADDR,
> + FSL_IMX7_WDOG3_ADDR,
> + FSL_IMX7_WDOG4_ADDR,
> + };
> +
> + object_property_set_bool(OBJECT(&s->wdt[i]), true, "realized",
> + &error_abort);
> +
> + sysbus_mmio_map(SYS_BUS_DEVICE(&s->wdt[i]), 0, FSL_IMX7_WDOGn_ADDR[i]);
> + }
> +}
> +
> +static void fsl_imx7_class_init(ObjectClass *oc, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(oc);
> +
> + dc->realize = fsl_imx7_realize;
> +
> + /*
> + * Reason: creates an ARM CPU, thus use after free(), see
> + * arm_cpu_class_init()
> + */
I think this is the wrong reason (we've fixed that problem, IIRC).
You do want this not to be user-creatable, but the reason is
/* Reason: uses serial_hds and nd_table in realize() */
(compare the reason text in the aspeed/allwinner/digic/xlnx-zynqmp
SoC device objects).
> + dc->user_creatable = false;
> + dc->desc = "i.MX7 SOC";
> +}
> +
> +static const TypeInfo fsl_imx7_type_info = {
> + .name = TYPE_FSL_IMX7,
> + .parent = TYPE_DEVICE,
> + .instance_size = sizeof(FslIMX7State),
> + .instance_init = fsl_imx7_init,
> + .class_init = fsl_imx7_class_init,
> +};
> +
> +static void fsl_imx7_register_types(void)
> +{
> + type_register_static(&fsl_imx7_type_info);
> +}
> +type_init(fsl_imx7_register_types)
thanks
-- PMM
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 16/17] i.MX: Add i.MX7 SOC implementation.
2017-10-06 14:38 ` Peter Maydell
@ 2017-10-09 16:18 ` Andrey Smirnov
2017-10-09 17:09 ` Peter Maydell
0 siblings, 1 reply; 51+ messages in thread
From: Andrey Smirnov @ 2017-10-09 16:18 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-arm, QEMU Developers, Andrey Yurovsky
On Fri, Oct 6, 2017 at 7:38 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>> For now we only support the following devices:
>> * up to 2 Cortex A9 cores (SMP works with PSCI)
>> * A7 MPCORE (identical to A15 MPCORE)
>> * 7 i.MX UARTs
>> * 1 CCM device
>> * 2 Ethernet controllers (FEC)
>> * 3 SD controllers (USDHC)
>> * 1 SNVS device
>> * 1 WDT device
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: qemu-devel@nongnu.org
>> Cc: qemu-arm@nongnu.org
>> Cc: yurovsky@gmail.com
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>> default-configs/arm-softmmu.mak | 1 +
>> hw/arm/Makefile.objs | 2 +
>> hw/arm/fsl-imx7.c | 327 ++++++++++++++++++++++++++++++++++++++++
>> include/hw/arm/fsl-imx7.h | 114 ++++++++++++++
>> 4 files changed, 444 insertions(+)
>> create mode 100644 hw/arm/fsl-imx7.c
>> create mode 100644 include/hw/arm/fsl-imx7.h
>>
>> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
>> index bbdd3c1d8b..98396a3ad2 100644
>> --- a/default-configs/arm-softmmu.mak
>> +++ b/default-configs/arm-softmmu.mak
>> @@ -118,6 +118,7 @@ CONFIG_ALLWINNER_A10=y
>> CONFIG_FSL_IMX6=y
>> CONFIG_FSL_IMX31=y
>> CONFIG_FSL_IMX25=y
>> +CONFIG_FSL_IMX7=y
>>
>> CONFIG_IMX_I2C=y
>>
>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>> index a2e56ecaae..33f6051ae3 100644
>> --- a/hw/arm/Makefile.objs
>> +++ b/hw/arm/Makefile.objs
>> @@ -19,3 +19,5 @@ obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
>> obj-$(CONFIG_FSL_IMX6) += fsl-imx6.o sabrelite.o
>> obj-$(CONFIG_ASPEED_SOC) += aspeed_soc.o aspeed.o
>> obj-$(CONFIG_MPS2) += mps2.o
>> +obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o
>> +
>> diff --git a/hw/arm/fsl-imx7.c b/hw/arm/fsl-imx7.c
>> new file mode 100644
>> index 0000000000..bd01bb7f59
>> --- /dev/null
>> +++ b/hw/arm/fsl-imx7.c
>> @@ -0,0 +1,327 @@
>> +/*
>> + * Copyright (c) 2017, Impinj, Inc.
>> + *
>> + * i.MX7 SoC definitions
>> + *
>> + * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
>> + *
>> + * Based on hw/arm/fsl-imx6.c
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; version 2 of the License.
>
> We pretty strongly prefer GPL-2-or-later for new code, not 2-only.
>
OK, will change in v2.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "qemu-common.h"
>> +#include "hw/arm/fsl-imx7.h"
>> +#include "sysemu/sysemu.h"
>> +#include "qemu/error-report.h"
>> +
>> +#define NAME_SIZE 20
>> +
>> +#define for_each_cpu(i) for (i = 0; i < smp_cpus; i++)
>
> Just open-code this, please.
Sure, will do in v2.
>
>> +
>> +static void fsl_imx7_init(Object *obj)
>> +{
>> + BusState *sysbus = sysbus_get_default();
>> + FslIMX7State *s = FSL_IMX7(obj);
>> + char name[NAME_SIZE];
>> + int i;
>> +
>> + if (smp_cpus > FSL_IMX7_NUM_CPUS) {
>> + error_report("%s: Only %d CPUs are supported (%d requested)",
>> + TYPE_FSL_IMX7, FSL_IMX7_NUM_CPUS, smp_cpus);
>> + exit(1);
>> + }
>> +
>> + for_each_cpu(i) {
>> + object_initialize(&s->cpu[i], sizeof(s->cpu[i]),
>> + "cortex-a7-" TYPE_ARM_CPU);
>> + snprintf(name, NAME_SIZE, "cpu%d", i);
>> + object_property_add_child(obj, name, OBJECT(&s->cpu[i]),
>> + &error_fatal);
>> + }
>> +
>> + /*
>> + * A7MPCORE
>> + */
>> + object_initialize(&s->a7mpcore, sizeof(s->a7mpcore), TYPE_A15MPCORE_PRIV);
>
> Stealing the A15MPCORE device for a7 is kind of ugly, but I can't
> decide what would be better instead...
>
I agree, but I looked through the RMs for both and didn't find any
software-visible differences that would warrant creating a standalone
A7MPCORE type.
>> + qdev_set_parent_bus(DEVICE(&s->a7mpcore), sysbus);
>> + object_property_add_child(obj, "a7mpcore",
>> + OBJECT(&s->a7mpcore), &error_fatal);
>> +
>> + /*
>> + * CCM
>> + */
>> + object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX7_CCM);
>> + qdev_set_parent_bus(DEVICE(&s->ccm), sysbus);
>> + object_property_add_child(obj, "ccm", OBJECT(&s->ccm), &error_fatal);
>> +
>> + /*
>> + * UART
>> + */
>> + for (i = 0; i < FSL_IMX7_NUM_UARTS; i++) {
>> + object_initialize(&s->uart[i], sizeof(s->uart[i]), TYPE_IMX_SERIAL);
>> + qdev_set_parent_bus(DEVICE(&s->uart[i]), sysbus);
>> + snprintf(name, NAME_SIZE, "uart%d", i);
>> + object_property_add_child(obj, name, OBJECT(&s->uart[i]),
>> + &error_fatal);
>> + }
>> +
>> + /*
>> + * Ethernet
>> + */
>> + for (i = 0; i < FSL_IMX7_NUM_ETHS; i++) {
>> + object_initialize(&s->eth[i], sizeof(s->eth[i]), TYPE_IMX_ENET);
>> + qdev_set_parent_bus(DEVICE(&s->eth[i]), sysbus);
>> + snprintf(name, NAME_SIZE, "eth%d", i);
>> + object_property_add_child(obj, name, OBJECT(&s->eth[i]),
>> + &error_fatal);
>> + }
>> +
>> + /*
>> + * SDHCI
>> + */
>> + for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
>> + object_initialize(&s->usdhc[i], sizeof(s->usdhc[i]),
>> + TYPE_IMX_USDHC);
>> + qdev_set_parent_bus(DEVICE(&s->usdhc[i]), sysbus);
>> + snprintf(name, NAME_SIZE, "usdhc%d", i);
>> + object_property_add_child(obj, name, OBJECT(&s->usdhc[i]),
>> + &error_fatal);
>> + }
>> +
>> + /*
>> + * SNVS
>> + */
>> + object_initialize(&s->snvs, sizeof(s->snvs), TYPE_IMX7_SNVS);
>> + qdev_set_parent_bus(DEVICE(&s->snvs), sysbus);
>> + object_property_add_child(obj, "snvs", OBJECT(&s->snvs), &error_fatal);
>> +
>> + /*
>> + * Watchdog
>> + */
>> + for (i = 0; i < FSL_IMX7_NUM_WDTS; i++) {
>> + object_initialize(&s->wdt[i], sizeof(s->wdt[i]), TYPE_IMX2_WDT);
>> + qdev_set_parent_bus(DEVICE(&s->wdt[i]), sysbus);
>> + snprintf(name, NAME_SIZE, "wdt%d", i);
>> + object_property_add_child(obj, name, OBJECT(&s->wdt[i]),
>> + &error_fatal);
>> + }
>> +}
>> +
>> +static void fsl_imx7_realize(DeviceState *dev, Error **errp)
>> +{
>> + FslIMX7State *s = FSL_IMX7(dev);
>> + Object *o;
>> + int i;
>> + qemu_irq irq;
>> +
>> + for_each_cpu(i) {
>> + o = OBJECT(&s->cpu[i]);
>> +
>> + object_property_set_int(o, QEMU_PSCI_CONDUIT_SMC,
>> + "psci-conduit", &error_abort);
>> +
>> + object_property_set_bool(o, false, "has_el3", &error_abort);
>
> Does this SoC's CPU really not have EL3?
>
The SoC support TrustZone, so I thing it does have EL3. This setting
however was breaking SMP support(I can't remember more details) and
that's why I added the code setting it to "false".
>> +
>> + /* On uniprocessor, the CBAR is set to 0 */
>> + if (smp_cpus > 1) {
>> + object_property_set_int(o, FSL_IMX7_A7MPCORE_ADDR,
>> + "reset-cbar", &error_abort);
>> + }
>> +
>> + if (i) {
>> + /* Secondary CPUs start in PSCI powered-down state */
>> + object_property_set_bool(o, true,
>> + "start-powered-off", &error_abort);
>> + }
>> +
>> + object_property_set_bool(o, true, "realized", &error_abort);
>> + }
>> +
>> + /*
>> + * A7MPCORE
>> + */
>> + object_property_set_int(OBJECT(&s->a7mpcore), smp_cpus, "num-cpu",
>> + &error_abort);
>> + object_property_set_int(OBJECT(&s->a7mpcore),
>> + FSL_IMX7_MAX_IRQ + GIC_INTERNAL,
>> + "num-irq", &error_abort);
>> +
>> + object_property_set_bool(OBJECT(&s->a7mpcore), true, "realized",
>> + &error_abort);
>> +
>> + sysbus_mmio_map(SYS_BUS_DEVICE(&s->a7mpcore), 0, FSL_IMX7_A7MPCORE_ADDR);
>> +
>> + for_each_cpu(i) {
>> + SysBusDevice *sbd = SYS_BUS_DEVICE(&s->a7mpcore);
>> + DeviceState *d = DEVICE(qemu_get_cpu(i));
>> +
>> + irq = qdev_get_gpio_in(d, ARM_CPU_IRQ);
>> + sysbus_connect_irq(sbd, i, irq);
>> + irq = qdev_get_gpio_in(d, ARM_CPU_FIQ);
>> + sysbus_connect_irq(sbd, i + smp_cpus, irq);
>> + }
>> +
>> + /*
>> + * CCM
>> + */
>> + object_property_set_bool(OBJECT(&s->ccm), true, "realized", &error_abort);
>> +
>> + sysbus_mmio_map(SYS_BUS_DEVICE(&s->ccm), 0, FSL_IMX7_CCM_ADDR);
>> +
>> + /*
>> + * UART
>> + */
>> + for (i = 0; i < FSL_IMX7_NUM_UARTS; i++) {
>> + static const hwaddr FSL_IMX7_UARTn_ADDR[FSL_IMX7_NUM_UARTS] = {
>> + FSL_IMX7_UART1_ADDR,
>> + FSL_IMX7_UART2_ADDR,
>> + FSL_IMX7_UART3_ADDR,
>> + FSL_IMX7_UART4_ADDR,
>> + FSL_IMX7_UART5_ADDR,
>> + FSL_IMX7_UART6_ADDR,
>> + FSL_IMX7_UART7_ADDR,
>> + };
>> +
>> + static const int FSL_IMX7_UARTn_IRQ[FSL_IMX7_NUM_UARTS] = {
>> + FSL_IMX7_UART1_IRQ,
>> + FSL_IMX7_UART2_IRQ,
>> + FSL_IMX7_UART3_IRQ,
>> + FSL_IMX7_UART4_IRQ,
>> + FSL_IMX7_UART5_IRQ,
>> + FSL_IMX7_UART6_IRQ,
>> + FSL_IMX7_UART7_IRQ,
>> + };
>> +
>> +
>> + if (i < MAX_SERIAL_PORTS) {
>> + Chardev *chr;
>> + chr = serial_hds[i];
>> +
>> + if (!chr) {
>> + char *label = g_strdup_printf("imx7.uart%d", i + 1);
>> + chr = qemu_chr_new(label, "null");
>> + g_free(label);
>> + serial_hds[i] = chr;
>> + }
>
> I think the conclusion we've come to about chardevs is that the
> UART should handle being passed a NULL chardev, rather than the
> board code having to create a dummy chardev to pass to it.
>
OK. Will fix in v2.
>> +
>> + qdev_prop_set_chr(DEVICE(&s->uart[i]), "chardev", chr);
>> + }
>> +
>> + object_property_set_bool(OBJECT(&s->uart[i]), true, "realized",
>> + &error_abort);
>> +
>> + sysbus_mmio_map(SYS_BUS_DEVICE(&s->uart[i]), 0, FSL_IMX7_UARTn_ADDR[i]);
>> +
>> + irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_UARTn_IRQ[i]);
>> + sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart[i]), 0, irq);
>> + }
>> +
>> + /*
>> + * Etherenet
>
> Typo: "Ethernet"
>
Will fix in v2.
>> + */
>> + for (i = 0; i < FSL_IMX7_NUM_ETHS; i++) {
>> + static const hwaddr FSL_IMX7_ENETn_ADDR[FSL_IMX7_NUM_ETHS] = {
>> + FSL_IMX7_ENET1_ADDR,
>> + FSL_IMX7_ENET2_ADDR,
>> + };
>> +
>> + qdev_set_nic_properties(DEVICE(&s->eth[i]), &nd_table[i]);
>> + object_property_set_bool(OBJECT(&s->eth[i]), true, "realized",
>> + &error_abort);
>> +
>> + sysbus_mmio_map(SYS_BUS_DEVICE(&s->eth[i]), 0, FSL_IMX7_ENETn_ADDR[i]);
>> +
>> + irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_ENET_IRQ(i, 0));
>> + sysbus_connect_irq(SYS_BUS_DEVICE(&s->eth[i]), 0, irq);
>> + irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_ENET_IRQ(i, 3));
>> + sysbus_connect_irq(SYS_BUS_DEVICE(&s->eth[i]), 1, irq);
>> + }
>> +
>> + /*
>> + * USDHC
>> + */
>> + for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
>> + static const hwaddr FSL_IMX7_USDHCn_ADDR[FSL_IMX7_NUM_USDHCS] = {
>> + FSL_IMX7_USDHC1_ADDR,
>> + FSL_IMX7_USDHC2_ADDR,
>> + FSL_IMX7_USDHC3_ADDR,
>> + };
>> +
>> + static const int FSL_IMX7_USDHCn_IRQ[FSL_IMX7_NUM_USDHCS] = {
>> + FSL_IMX7_USDHC1_IRQ,
>> + FSL_IMX7_USDHC2_IRQ,
>> + FSL_IMX7_USDHC3_IRQ,
>> + };
>> +
>> + object_property_set_bool(OBJECT(&s->usdhc[i]), true, "realized",
>> + &error_abort);
>> +
>> + sysbus_mmio_map(SYS_BUS_DEVICE(&s->usdhc[i]), 0,
>> + FSL_IMX7_USDHCn_ADDR[i]);
>> +
>> + irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_USDHCn_IRQ[i]);
>> + sysbus_connect_irq(SYS_BUS_DEVICE(&s->usdhc[i]), 0, irq);
>> + }
>> +
>> + /*
>> + * SNVS
>> + */
>> + object_property_set_bool(OBJECT(&s->snvs), true, "realized", &error_abort);
>> +
>> + sysbus_mmio_map(SYS_BUS_DEVICE(&s->snvs), 0, FSL_IMX7_SNVS_ADDR);
>> +
>> + /*
>> + * Watchdog
>> + */
>> + for (i = 0; i < FSL_IMX7_NUM_WDTS; i++) {
>> + static const hwaddr FSL_IMX7_WDOGn_ADDR[FSL_IMX7_NUM_WDTS] = {
>> + FSL_IMX7_WDOG1_ADDR,
>> + FSL_IMX7_WDOG2_ADDR,
>> + FSL_IMX7_WDOG3_ADDR,
>> + FSL_IMX7_WDOG4_ADDR,
>> + };
>> +
>> + object_property_set_bool(OBJECT(&s->wdt[i]), true, "realized",
>> + &error_abort);
>> +
>> + sysbus_mmio_map(SYS_BUS_DEVICE(&s->wdt[i]), 0, FSL_IMX7_WDOGn_ADDR[i]);
>> + }
>> +}
>> +
>> +static void fsl_imx7_class_init(ObjectClass *oc, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(oc);
>> +
>> + dc->realize = fsl_imx7_realize;
>> +
>> + /*
>> + * Reason: creates an ARM CPU, thus use after free(), see
>> + * arm_cpu_class_init()
>> + */
>
> I think this is the wrong reason (we've fixed that problem, IIRC).
> You do want this not to be user-creatable, but the reason is
> /* Reason: uses serial_hds and nd_table in realize() */
> (compare the reason text in the aspeed/allwinner/digic/xlnx-zynqmp
> SoC device objects).
>
That's another copy-pasted snippet, so I am not surprised to learn it
is wrong. I'll take a look at Zynq emulation and update/fix this code
in v2.
Thanks,
Andrey Smirnov
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 16/17] i.MX: Add i.MX7 SOC implementation.
2017-10-09 16:18 ` Andrey Smirnov
@ 2017-10-09 17:09 ` Peter Maydell
0 siblings, 0 replies; 51+ messages in thread
From: Peter Maydell @ 2017-10-09 17:09 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-arm, QEMU Developers, Andrey Yurovsky
On 9 October 2017 at 17:18, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> On Fri, Oct 6, 2017 at 7:38 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>>> +static void fsl_imx7_realize(DeviceState *dev, Error **errp)
>>> +{
>>> + FslIMX7State *s = FSL_IMX7(dev);
>>> + Object *o;
>>> + int i;
>>> + qemu_irq irq;
>>> +
>>> + for_each_cpu(i) {
>>> + o = OBJECT(&s->cpu[i]);
>>> +
>>> + object_property_set_int(o, QEMU_PSCI_CONDUIT_SMC,
>>> + "psci-conduit", &error_abort);
>>> +
>>> + object_property_set_bool(o, false, "has_el3", &error_abort);
>>
>> Does this SoC's CPU really not have EL3?
>>
>
> The SoC support TrustZone, so I thing it does have EL3. This setting
> however was breaking SMP support(I can't remember more details) and
> that's why I added the code setting it to "false".
The idea is that for new boards (where we start from "no guest
code works") we try to model the hardware correctly from the
start, ie figure out why SMP doesn't work if you say you have
EL3 (probably because either you need to provide boot code
for the other CPUs or implement a power controller, depending
on how SMP secondary CPU start works on this hardware).
Existing board models that were added before we got EL3 working
tend to set has_el3 to false even though the h/w we're modelling
has EL3, because they're maintaining backwards compatibility with
previous released versions of QEMU where we didn't implement EL3.
New boards don't have that back-compat requirement, so if it's
not too hard to get it working from the start that's preferable.
thanks
-- PMM
^ permalink raw reply [flat|nested] 51+ messages in thread
* [Qemu-devel] [PATCH 17/17] Implement support for i.MX7 Sabre board
2017-09-18 19:50 [Qemu-devel] [PATCH 00/17] Initial i.MX7 support Andrey Smirnov
` (15 preceding siblings ...)
2017-09-18 19:50 ` [Qemu-devel] [PATCH 16/17] i.MX: Add i.MX7 SOC implementation Andrey Smirnov
@ 2017-09-18 19:51 ` Andrey Smirnov
2017-10-06 14:42 ` Peter Maydell
2017-09-18 21:00 ` [Qemu-devel] [PATCH 00/17] Initial i.MX7 support no-reply
2017-10-06 14:46 ` Peter Maydell
18 siblings, 1 reply; 51+ messages in thread
From: Andrey Smirnov @ 2017-09-18 19:51 UTC (permalink / raw)
To: qemu-arm; +Cc: Andrey Smirnov, Peter Maydell, qemu-devel, yurovsky
Cc: Peter Maydell <peter.maydell@linaro.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/Makefile.objs | 2 +-
hw/arm/mcimx7d-sabre.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 101 insertions(+), 1 deletion(-)
create mode 100644 hw/arm/mcimx7d-sabre.c
diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index 33f6051ae3..fc4a963de8 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -19,5 +19,5 @@ obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
obj-$(CONFIG_FSL_IMX6) += fsl-imx6.o sabrelite.o
obj-$(CONFIG_ASPEED_SOC) += aspeed_soc.o aspeed.o
obj-$(CONFIG_MPS2) += mps2.o
-obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o
+obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o mcimx7d-sabre.o
diff --git a/hw/arm/mcimx7d-sabre.c b/hw/arm/mcimx7d-sabre.c
new file mode 100644
index 0000000000..34e3933db8
--- /dev/null
+++ b/hw/arm/mcimx7d-sabre.c
@@ -0,0 +1,100 @@
+/*
+ * Copyright (c) 2017, Impinj, Inc.
+ *
+ * MCIMX7D_SABRE Board System emulation.
+ *
+ * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
+ *
+ * This code is licensed under the GPL, version 2 or later.
+ * See the file `COPYING' in the top level directory.
+ *
+ * It (partially) emulates a mcimx7d_sabre board, with a Freescale
+ * i.MX7 SoC
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu-common.h"
+#include "hw/arm/fsl-imx7.h"
+#include "hw/boards.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/device_tree.h"
+#include "qemu/error-report.h"
+#include "sysemu/qtest.h"
+#include "net/net.h"
+
+typedef struct {
+ FslIMX7State soc;
+ MemoryRegion ram;
+} MCIMX7Sabre;
+
+static void mcimx7d_add_psci_node(const struct arm_boot_info *boot_info, void *fdt)
+{
+ const char comp[] = "arm,psci-0.2\0arm,psci";
+
+ qemu_fdt_add_subnode(fdt, "/psci");
+ qemu_fdt_setprop(fdt, "/psci", "compatible", comp, sizeof(comp));
+ qemu_fdt_setprop_string(fdt, "/psci", "method", "smc");
+}
+
+static void mcimx7d_sabre_init(MachineState *machine)
+{
+ static struct arm_boot_info boot_info;
+ MCIMX7Sabre *s = g_new0(MCIMX7Sabre, 1);
+ Object *soc;
+ int i;
+
+ if (machine->ram_size > FSL_IMX7_MMDC_SIZE) {
+ error_report("RAM size " RAM_ADDR_FMT " above max supported (%08x)",
+ machine->ram_size, FSL_IMX7_MMDC_SIZE);
+ exit(1);
+ }
+
+ boot_info = (struct arm_boot_info) {
+ .loader_start = FSL_IMX7_MMDC_ADDR,
+ .board_id = -1,
+ .ram_size = machine->ram_size,
+ .kernel_filename = machine->kernel_filename,
+ .kernel_cmdline = machine->kernel_cmdline,
+ .initrd_filename = machine->initrd_filename,
+ .nb_cpus = smp_cpus,
+ .modify_dtb = mcimx7d_add_psci_node,
+ };
+
+ object_initialize(&s->soc, sizeof(s->soc), TYPE_FSL_IMX7);
+ soc = OBJECT(&s->soc);
+ object_property_add_child(OBJECT(machine), "soc", soc, &error_fatal);
+ object_property_set_bool(soc, true, "realized", &error_fatal);
+
+ memory_region_allocate_system_memory(&s->ram, NULL, "mcimx7d-sabre.ram",
+ machine->ram_size);
+ memory_region_add_subregion(get_system_memory(),
+ FSL_IMX7_MMDC_ADDR, &s->ram);
+
+ for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
+ BusState *bus;
+ DeviceState *carddev;
+ DriveInfo *di;
+ BlockBackend *blk;
+
+ di = drive_get_next(IF_SD);
+ blk = di ? blk_by_legacy_dinfo(di) : NULL;
+ bus = qdev_get_child_bus(DEVICE(&s->soc.usdhc[i]), "sd-bus");
+ carddev = qdev_create(bus, TYPE_SD_CARD);
+ qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
+ object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);
+ }
+
+ if (!qtest_enabled()) {
+ arm_load_kernel(&s->soc.cpu[0], &boot_info);
+ }
+}
+
+static void mcimx7d_sabre_machine_init(MachineClass *mc)
+{
+ mc->desc = "Freescale i.MX7 DUAL SABRE (Cortex A7)";
+ mc->init = mcimx7d_sabre_init;
+ mc->max_cpus = FSL_IMX7_NUM_CPUS;
+ mc->ignore_memory_transaction_failures = true;
+}
+DEFINE_MACHINE("mcimx7d-sabre", mcimx7d_sabre_machine_init)
--
2.13.5
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 17/17] Implement support for i.MX7 Sabre board
2017-09-18 19:51 ` [Qemu-devel] [PATCH 17/17] Implement support for i.MX7 Sabre board Andrey Smirnov
@ 2017-10-06 14:42 ` Peter Maydell
2017-10-09 16:30 ` Andrey Smirnov
0 siblings, 1 reply; 51+ messages in thread
From: Peter Maydell @ 2017-10-06 14:42 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-arm, QEMU Developers, Andrey Yurovsky
On 18 September 2017 at 20:51, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> Cc: Peter Maydell <peter.maydell@linaro.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/Makefile.objs | 2 +-
> hw/arm/mcimx7d-sabre.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 101 insertions(+), 1 deletion(-)
> create mode 100644 hw/arm/mcimx7d-sabre.c
>
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index 33f6051ae3..fc4a963de8 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -19,5 +19,5 @@ obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
> obj-$(CONFIG_FSL_IMX6) += fsl-imx6.o sabrelite.o
> obj-$(CONFIG_ASPEED_SOC) += aspeed_soc.o aspeed.o
> obj-$(CONFIG_MPS2) += mps2.o
> -obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o
> +obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o mcimx7d-sabre.o
>
> diff --git a/hw/arm/mcimx7d-sabre.c b/hw/arm/mcimx7d-sabre.c
> new file mode 100644
> index 0000000000..34e3933db8
> --- /dev/null
> +++ b/hw/arm/mcimx7d-sabre.c
> @@ -0,0 +1,100 @@
> +/*
> + * Copyright (c) 2017, Impinj, Inc.
> + *
> + * MCIMX7D_SABRE Board System emulation.
> + *
> + * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
> + *
> + * This code is licensed under the GPL, version 2 or later.
> + * See the file `COPYING' in the top level directory.
> + *
> + * It (partially) emulates a mcimx7d_sabre board, with a Freescale
> + * i.MX7 SoC
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu-common.h"
> +#include "hw/arm/fsl-imx7.h"
> +#include "hw/boards.h"
> +#include "sysemu/sysemu.h"
> +#include "sysemu/device_tree.h"
> +#include "qemu/error-report.h"
> +#include "sysemu/qtest.h"
> +#include "net/net.h"
> +
> +typedef struct {
> + FslIMX7State soc;
> + MemoryRegion ram;
> +} MCIMX7Sabre;
> +
> +static void mcimx7d_add_psci_node(const struct arm_boot_info *boot_info, void *fdt)
> +{
> + const char comp[] = "arm,psci-0.2\0arm,psci";
> +
> + qemu_fdt_add_subnode(fdt, "/psci");
> + qemu_fdt_setprop(fdt, "/psci", "compatible", comp, sizeof(comp));
> + qemu_fdt_setprop_string(fdt, "/psci", "method", "smc");
> +}
You shouldn't need this -- we should just be able to work with whatever
the real hardware's device tree is. ("virt" is a special case because
we create our own device tree there.)
> +
> +static void mcimx7d_sabre_init(MachineState *machine)
> +{
> + static struct arm_boot_info boot_info;
> + MCIMX7Sabre *s = g_new0(MCIMX7Sabre, 1);
> + Object *soc;
> + int i;
> +
> + if (machine->ram_size > FSL_IMX7_MMDC_SIZE) {
> + error_report("RAM size " RAM_ADDR_FMT " above max supported (%08x)",
> + machine->ram_size, FSL_IMX7_MMDC_SIZE);
> + exit(1);
> + }
> +
> + boot_info = (struct arm_boot_info) {
> + .loader_start = FSL_IMX7_MMDC_ADDR,
> + .board_id = -1,
> + .ram_size = machine->ram_size,
> + .kernel_filename = machine->kernel_filename,
> + .kernel_cmdline = machine->kernel_cmdline,
> + .initrd_filename = machine->initrd_filename,
> + .nb_cpus = smp_cpus,
> + .modify_dtb = mcimx7d_add_psci_node,
> + };
> +
> + object_initialize(&s->soc, sizeof(s->soc), TYPE_FSL_IMX7);
> + soc = OBJECT(&s->soc);
> + object_property_add_child(OBJECT(machine), "soc", soc, &error_fatal);
> + object_property_set_bool(soc, true, "realized", &error_fatal);
> +
> + memory_region_allocate_system_memory(&s->ram, NULL, "mcimx7d-sabre.ram",
> + machine->ram_size);
> + memory_region_add_subregion(get_system_memory(),
> + FSL_IMX7_MMDC_ADDR, &s->ram);
> +
> + for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
> + BusState *bus;
> + DeviceState *carddev;
> + DriveInfo *di;
> + BlockBackend *blk;
> +
> + di = drive_get_next(IF_SD);
> + blk = di ? blk_by_legacy_dinfo(di) : NULL;
> + bus = qdev_get_child_bus(DEVICE(&s->soc.usdhc[i]), "sd-bus");
> + carddev = qdev_create(bus, TYPE_SD_CARD);
> + qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
> + object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);
> + }
> +
> + if (!qtest_enabled()) {
> + arm_load_kernel(&s->soc.cpu[0], &boot_info);
> + }
> +}
> +
> +static void mcimx7d_sabre_machine_init(MachineClass *mc)
> +{
> + mc->desc = "Freescale i.MX7 DUAL SABRE (Cortex A7)";
> + mc->init = mcimx7d_sabre_init;
> + mc->max_cpus = FSL_IMX7_NUM_CPUS;
> + mc->ignore_memory_transaction_failures = true;
Please don't set this flag on new board models -- it is only
for legacy existing board models (where we don't know what
running guest code might be broken by turning bad accesses
into guest aborts. For a new board model, you should leave
the flag unset and instead stub out any devices that you
need to using create_unimplemented_device() so that your
guest code boots.
thanks
-- PMM
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 17/17] Implement support for i.MX7 Sabre board
2017-10-06 14:42 ` Peter Maydell
@ 2017-10-09 16:30 ` Andrey Smirnov
0 siblings, 0 replies; 51+ messages in thread
From: Andrey Smirnov @ 2017-10-09 16:30 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-arm, QEMU Developers, Andrey Yurovsky
On Fri, Oct 6, 2017 at 7:42 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 September 2017 at 20:51, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>> Cc: Peter Maydell <peter.maydell@linaro.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/Makefile.objs | 2 +-
>> hw/arm/mcimx7d-sabre.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 101 insertions(+), 1 deletion(-)
>> create mode 100644 hw/arm/mcimx7d-sabre.c
>>
>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>> index 33f6051ae3..fc4a963de8 100644
>> --- a/hw/arm/Makefile.objs
>> +++ b/hw/arm/Makefile.objs
>> @@ -19,5 +19,5 @@ obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
>> obj-$(CONFIG_FSL_IMX6) += fsl-imx6.o sabrelite.o
>> obj-$(CONFIG_ASPEED_SOC) += aspeed_soc.o aspeed.o
>> obj-$(CONFIG_MPS2) += mps2.o
>> -obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o
>> +obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o mcimx7d-sabre.o
>>
>> diff --git a/hw/arm/mcimx7d-sabre.c b/hw/arm/mcimx7d-sabre.c
>> new file mode 100644
>> index 0000000000..34e3933db8
>> --- /dev/null
>> +++ b/hw/arm/mcimx7d-sabre.c
>> @@ -0,0 +1,100 @@
>> +/*
>> + * Copyright (c) 2017, Impinj, Inc.
>> + *
>> + * MCIMX7D_SABRE Board System emulation.
>> + *
>> + * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
>> + *
>> + * This code is licensed under the GPL, version 2 or later.
>> + * See the file `COPYING' in the top level directory.
>> + *
>> + * It (partially) emulates a mcimx7d_sabre board, with a Freescale
>> + * i.MX7 SoC
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "qemu-common.h"
>> +#include "hw/arm/fsl-imx7.h"
>> +#include "hw/boards.h"
>> +#include "sysemu/sysemu.h"
>> +#include "sysemu/device_tree.h"
>> +#include "qemu/error-report.h"
>> +#include "sysemu/qtest.h"
>> +#include "net/net.h"
>> +
>> +typedef struct {
>> + FslIMX7State soc;
>> + MemoryRegion ram;
>> +} MCIMX7Sabre;
>> +
>> +static void mcimx7d_add_psci_node(const struct arm_boot_info *boot_info, void *fdt)
>> +{
>> + const char comp[] = "arm,psci-0.2\0arm,psci";
>> +
>> + qemu_fdt_add_subnode(fdt, "/psci");
>> + qemu_fdt_setprop(fdt, "/psci", "compatible", comp, sizeof(comp));
>> + qemu_fdt_setprop_string(fdt, "/psci", "method", "smc");
>> +}
>
> You shouldn't need this -- we should just be able to work with whatever
> the real hardware's device tree is. ("virt" is a special case because
> we create our own device tree there.)
>
Upstream kernel for i.MX7 relies on PSCI to support SMP and this is a
kind of DT fixup that would normally be done by a PSCI-compatible
bootloader. I can remove this code, but it would effectively disable
SMP on vanilla DT/kernel combos.
>> +
>> +static void mcimx7d_sabre_init(MachineState *machine)
>> +{
>> + static struct arm_boot_info boot_info;
>> + MCIMX7Sabre *s = g_new0(MCIMX7Sabre, 1);
>> + Object *soc;
>> + int i;
>> +
>> + if (machine->ram_size > FSL_IMX7_MMDC_SIZE) {
>> + error_report("RAM size " RAM_ADDR_FMT " above max supported (%08x)",
>> + machine->ram_size, FSL_IMX7_MMDC_SIZE);
>> + exit(1);
>> + }
>> +
>> + boot_info = (struct arm_boot_info) {
>> + .loader_start = FSL_IMX7_MMDC_ADDR,
>> + .board_id = -1,
>> + .ram_size = machine->ram_size,
>> + .kernel_filename = machine->kernel_filename,
>> + .kernel_cmdline = machine->kernel_cmdline,
>> + .initrd_filename = machine->initrd_filename,
>> + .nb_cpus = smp_cpus,
>> + .modify_dtb = mcimx7d_add_psci_node,
>> + };
>> +
>> + object_initialize(&s->soc, sizeof(s->soc), TYPE_FSL_IMX7);
>> + soc = OBJECT(&s->soc);
>> + object_property_add_child(OBJECT(machine), "soc", soc, &error_fatal);
>> + object_property_set_bool(soc, true, "realized", &error_fatal);
>> +
>> + memory_region_allocate_system_memory(&s->ram, NULL, "mcimx7d-sabre.ram",
>> + machine->ram_size);
>> + memory_region_add_subregion(get_system_memory(),
>> + FSL_IMX7_MMDC_ADDR, &s->ram);
>> +
>> + for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
>> + BusState *bus;
>> + DeviceState *carddev;
>> + DriveInfo *di;
>> + BlockBackend *blk;
>> +
>> + di = drive_get_next(IF_SD);
>> + blk = di ? blk_by_legacy_dinfo(di) : NULL;
>> + bus = qdev_get_child_bus(DEVICE(&s->soc.usdhc[i]), "sd-bus");
>> + carddev = qdev_create(bus, TYPE_SD_CARD);
>> + qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
>> + object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);
>> + }
>> +
>> + if (!qtest_enabled()) {
>> + arm_load_kernel(&s->soc.cpu[0], &boot_info);
>> + }
>> +}
>> +
>> +static void mcimx7d_sabre_machine_init(MachineClass *mc)
>> +{
>> + mc->desc = "Freescale i.MX7 DUAL SABRE (Cortex A7)";
>> + mc->init = mcimx7d_sabre_init;
>> + mc->max_cpus = FSL_IMX7_NUM_CPUS;
>> + mc->ignore_memory_transaction_failures = true;
>
> Please don't set this flag on new board models -- it is only
> for legacy existing board models (where we don't know what
> running guest code might be broken by turning bad accesses
> into guest aborts. For a new board model, you should leave
> the flag unset and instead stub out any devices that you
> need to using create_unimplemented_device() so that your
> guest code boots.
That flag didn't exist when I started porting this board and I only
discovered it a couple of hours before I prepared v1 of the patchset,
since I didn't want to submit the code that wouldn't work but didn't
have time to implements all of the missing blocks. I'll update v2, to
get rid of it.
Thanks,
Andrey Smirnov
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [PATCH 00/17] Initial i.MX7 support
2017-09-18 19:50 [Qemu-devel] [PATCH 00/17] Initial i.MX7 support Andrey Smirnov
` (16 preceding siblings ...)
2017-09-18 19:51 ` [Qemu-devel] [PATCH 17/17] Implement support for i.MX7 Sabre board Andrey Smirnov
@ 2017-09-18 21:00 ` no-reply
2017-10-06 14:46 ` Peter Maydell
18 siblings, 0 replies; 51+ messages in thread
From: no-reply @ 2017-09-18 21:00 UTC (permalink / raw)
To: andrew.smirnov; +Cc: famz, qemu-arm, peter.maydell, qemu-devel, yurovsky
Hi,
This series seems to have some coding style problems. See output below for
more information:
Subject: [Qemu-devel] [PATCH 00/17] Initial i.MX7 support
Message-id: 20170918195100.17593-1-andrew.smirnov@gmail.com
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
t [tag update] patchew/20170918202529.28379-1-el13635@mail.ntua.gr -> patchew/20170918202529.28379-1-el13635@mail.ntua.gr
Switched to a new branch 'test'
274d7165b1 Implement support for i.MX7 Sabre board
d24ef9c251 i.MX: Add i.MX7 SOC implementation.
74a0b2d896 include/qemu: Add sizes.h from Linux
920e37dac2 i.MX7: Add code to emulate SNVS IP-block
0d876ce354 i.MX: Add code to emulate i.MX2 watchdog IP block
e21f310ca0 i.MX: Add i.MX7 CCM, PMU and ANALOG device
e5d8486d8c sdhci: Implement write method of ACMD12ERRSTS register
c00331bbb7 sdhci: Add i.MX specific subtype of SDHCI
9b01e35230 imx_fec: Use correct length for packet size
47351ad73c imx_fec: Add support for multiple Tx DMA rings
f23e9656f5 imx_fec: Emulate SHIFT16 in ENETx_RACC
abc29e9048 imx_fec: Use MIN instead of explicit ternary operator
456a2681ea imx_fec: Use ENET_FTRL to determine truncation length
f756d1961b imx_fec: Change queue flushing heuristics
e3f400de53 imx_fec: Refactor imx_eth_enable_rx()
38f4f4a46c imx_fec: Do not calculate FEC
e07552c97b imx_fec: Do not link to netdev
=== OUTPUT BEGIN ===
Checking PATCH 1/17: imx_fec: Do not link to netdev...
Checking PATCH 2/17: imx_fec: Do not calculate FEC...
Checking PATCH 3/17: imx_fec: Refactor imx_eth_enable_rx()...
Checking PATCH 4/17: imx_fec: Change queue flushing heuristics...
Checking PATCH 5/17: imx_fec: Use ENET_FTRL to determine truncation length...
Checking PATCH 6/17: imx_fec: Use MIN instead of explicit ternary operator...
Checking PATCH 7/17: imx_fec: Emulate SHIFT16 in ENETx_RACC...
ERROR: code indent should never use tabs
#73: FILE: include/hw/net/imx_fec.h:172:
+#define ENET_RACC_SHIFT16^IBIT(7)$
total: 1 errors, 0 warnings, 49 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 8/17: imx_fec: Add support for multiple Tx DMA rings...
WARNING: line over 80 characters
#249: FILE: include/hw/net/imx_fec.h:123:
+ ENET_INT_TS_AVAIL | ENET_INT_TXF1 | ENET_INT_TXB1 |\
total: 0 errors, 1 warnings, 241 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 9/17: imx_fec: Use correct length for packet size...
Checking PATCH 10/17: sdhci: Add i.MX specific subtype of SDHCI...
ERROR: code indent should never use tabs
#37: FILE: hw/sd/sdhci-internal.h:235:
+#define ESDHC_MIX_CTRL^I^I^I0x48$
ERROR: code indent should never use tabs
#38: FILE: hw/sd/sdhci-internal.h:236:
+#define ESDHC_VENDOR_SPEC^I^I0xc0$
ERROR: code indent should never use tabs
#39: FILE: hw/sd/sdhci-internal.h:237:
+#define ESDHC_DLL_CTRL^I^I^I0x60$
ERROR: code indent should never use tabs
#41: FILE: hw/sd/sdhci-internal.h:239:
+#define ESDHC_TUNING_CTRL^I^I0xcc$
ERROR: code indent should never use tabs
#42: FILE: hw/sd/sdhci-internal.h:240:
+#define ESDHC_TUNE_CTRL_STATUS^I^I0x68$
ERROR: code indent should never use tabs
#43: FILE: hw/sd/sdhci-internal.h:241:
+#define ESDHC_WTMK_LVL^I^I^I0x44$
ERROR: code indent should never use tabs
#45: FILE: hw/sd/sdhci-internal.h:243:
+#define ESDHC_CTRL_4BITBUS^I^I(0x1 << 1)$
ERROR: code indent should never use tabs
#46: FILE: hw/sd/sdhci-internal.h:244:
+#define ESDHC_CTRL_8BITBUS^I^I(0x2 << 1)$
ERROR: braces {} are necessary for all arms of this statement
#99: FILE: hw/sd/sdhci.c:1405:
+ if (s->hostctl & SDHC_CTRL_8BITBUS)
[...]
ERROR: braces {} are necessary for all arms of this statement
#102: FILE: hw/sd/sdhci.c:1408:
+ if (s->hostctl & SDHC_CTRL_4BITBUS)
[...]
ERROR: braces {} are necessary for all arms of this statement
#141: FILE: hw/sd/sdhci.c:1447:
+ if (value & ESDHC_CTRL_8BITBUS)
[...]
ERROR: braces {} are necessary for all arms of this statement
#144: FILE: hw/sd/sdhci.c:1450:
+ if (value & ESDHC_CTRL_4BITBUS)
[...]
ERROR: code indent should never use tabs
#231: FILE: include/hw/sd/sdhci.h:92:
+#define SDHCI_QUIRK_NO_BUSY_IRQ^I^I^I^I(1<<14)$
ERROR: spaces required around that '<<' (ctx:VxV)
#231: FILE: include/hw/sd/sdhci.h:92:
+#define SDHCI_QUIRK_NO_BUSY_IRQ (1<<14)
^
total: 14 errors, 0 warnings, 203 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 11/17: sdhci: Implement write method of ACMD12ERRSTS register...
Checking PATCH 12/17: i.MX: Add i.MX7 CCM, PMU and ANALOG device...
ERROR: trailing whitespace
#71: FILE: hw/misc/imx7_ccm.c:41:
+ s->analog[CCM_ANALOG_PLL_480] = 0x00002000; $
ERROR: code indent should never use tabs
#97: FILE: hw/misc/imx7_ccm.c:67:
+#define CCM_INDEX(offset)^I(((offset) & ~(hwaddr)0xF) / sizeof(uint32_t))$
ERROR: code indent should never use tabs
#98: FILE: hw/misc/imx7_ccm.c:68:
+#define CCM_BITOP(offset)^I((offset) & (hwaddr)0xF)$
ERROR: trailing whitespace
#182: FILE: hw/misc/imx7_ccm.c:152:
+ $
ERROR: Macros with complex values should be enclosed in parenthesis
#255: FILE: include/hw/misc/imx7_ccm.h:18:
+#define REG_SET_CLR_TOG(name) name, name##_SET, name##_CLR, name##_TOG
total: 5 errors, 0 warnings, 284 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 13/17: i.MX: Add code to emulate i.MX2 watchdog IP block...
ERROR: code indent should never use tabs
#62: FILE: hw/misc/imx2_wdt.c:29:
+#define IMX2_WDT_WCR_WDA^IBIT(5)^I^I/* -> External Reset WDOG_B */$
ERROR: code indent should never use tabs
#63: FILE: hw/misc/imx2_wdt.c:30:
+#define IMX2_WDT_WCR_SRS^IBIT(4)^I^I/* -> Software Reset Signal */$
ERROR: braces {} are necessary for all arms of this statement
#71: FILE: hw/misc/imx2_wdt.c:38:
+ if (index < ARRAY_SIZE(s->reg))
[...]
+ else
[...]
ERROR: braces {} are necessary for all arms of this statement
#89: FILE: hw/misc/imx2_wdt.c:56:
+ if (value & (IMX2_WDT_WCR_WDA | IMX2_WDT_WCR_SRS))
[...]
total: 4 errors, 0 warnings, 160 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 14/17: i.MX7: Add code to emulate SNVS IP-block...
Checking PATCH 15/17: include/qemu: Add sizes.h from Linux...
ERROR: code indent should never use tabs
#32: FILE: include/qemu/sizes.h:11:
+#define SZ_1^I^I^I^I0x00000001$
ERROR: code indent should never use tabs
#33: FILE: include/qemu/sizes.h:12:
+#define SZ_2^I^I^I^I0x00000002$
ERROR: code indent should never use tabs
#34: FILE: include/qemu/sizes.h:13:
+#define SZ_4^I^I^I^I0x00000004$
ERROR: code indent should never use tabs
#35: FILE: include/qemu/sizes.h:14:
+#define SZ_8^I^I^I^I0x00000008$
ERROR: code indent should never use tabs
#36: FILE: include/qemu/sizes.h:15:
+#define SZ_16^I^I^I^I0x00000010$
ERROR: code indent should never use tabs
#37: FILE: include/qemu/sizes.h:16:
+#define SZ_32^I^I^I^I0x00000020$
ERROR: code indent should never use tabs
#38: FILE: include/qemu/sizes.h:17:
+#define SZ_64^I^I^I^I0x00000040$
ERROR: code indent should never use tabs
#39: FILE: include/qemu/sizes.h:18:
+#define SZ_128^I^I^I^I0x00000080$
ERROR: code indent should never use tabs
#40: FILE: include/qemu/sizes.h:19:
+#define SZ_256^I^I^I^I0x00000100$
ERROR: code indent should never use tabs
#41: FILE: include/qemu/sizes.h:20:
+#define SZ_512^I^I^I^I0x00000200$
ERROR: code indent should never use tabs
#43: FILE: include/qemu/sizes.h:22:
+#define SZ_1K^I^I^I^I0x00000400$
ERROR: code indent should never use tabs
#44: FILE: include/qemu/sizes.h:23:
+#define SZ_2K^I^I^I^I0x00000800$
ERROR: code indent should never use tabs
#45: FILE: include/qemu/sizes.h:24:
+#define SZ_4K^I^I^I^I0x00001000$
ERROR: code indent should never use tabs
#46: FILE: include/qemu/sizes.h:25:
+#define SZ_8K^I^I^I^I0x00002000$
ERROR: code indent should never use tabs
#47: FILE: include/qemu/sizes.h:26:
+#define SZ_16K^I^I^I^I0x00004000$
ERROR: code indent should never use tabs
#48: FILE: include/qemu/sizes.h:27:
+#define SZ_32K^I^I^I^I0x00008000$
ERROR: code indent should never use tabs
#49: FILE: include/qemu/sizes.h:28:
+#define SZ_64K^I^I^I^I0x00010000$
ERROR: code indent should never use tabs
#50: FILE: include/qemu/sizes.h:29:
+#define SZ_128K^I^I^I^I0x00020000$
ERROR: code indent should never use tabs
#51: FILE: include/qemu/sizes.h:30:
+#define SZ_256K^I^I^I^I0x00040000$
ERROR: code indent should never use tabs
#52: FILE: include/qemu/sizes.h:31:
+#define SZ_512K^I^I^I^I0x00080000$
ERROR: code indent should never use tabs
#54: FILE: include/qemu/sizes.h:33:
+#define SZ_1M^I^I^I^I0x00100000$
ERROR: code indent should never use tabs
#55: FILE: include/qemu/sizes.h:34:
+#define SZ_2M^I^I^I^I0x00200000$
ERROR: code indent should never use tabs
#56: FILE: include/qemu/sizes.h:35:
+#define SZ_4M^I^I^I^I0x00400000$
ERROR: code indent should never use tabs
#57: FILE: include/qemu/sizes.h:36:
+#define SZ_8M^I^I^I^I0x00800000$
ERROR: code indent should never use tabs
#58: FILE: include/qemu/sizes.h:37:
+#define SZ_16M^I^I^I^I0x01000000$
ERROR: code indent should never use tabs
#59: FILE: include/qemu/sizes.h:38:
+#define SZ_32M^I^I^I^I0x02000000$
ERROR: code indent should never use tabs
#60: FILE: include/qemu/sizes.h:39:
+#define SZ_64M^I^I^I^I0x04000000$
ERROR: code indent should never use tabs
#61: FILE: include/qemu/sizes.h:40:
+#define SZ_128M^I^I^I^I0x08000000$
ERROR: code indent should never use tabs
#62: FILE: include/qemu/sizes.h:41:
+#define SZ_256M^I^I^I^I0x10000000$
ERROR: code indent should never use tabs
#63: FILE: include/qemu/sizes.h:42:
+#define SZ_512M^I^I^I^I0x20000000$
ERROR: code indent should never use tabs
#65: FILE: include/qemu/sizes.h:44:
+#define SZ_1G^I^I^I^I0x40000000$
ERROR: code indent should never use tabs
#66: FILE: include/qemu/sizes.h:45:
+#define SZ_2G^I^I^I^I0x80000000$
total: 32 errors, 0 warnings, 47 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 16/17: i.MX: Add i.MX7 SOC implementation....
ERROR: code indent should never use tabs
#178: FILE: hw/arm/fsl-imx7.c:128:
+^Io = OBJECT(&s->cpu[i]);$
ERROR: code indent should never use tabs
#458: FILE: include/hw/arm/fsl-imx7.h:75:
+ FSL_IMX7_SNVS_ADDR^I = 0x30370000,$
total: 2 errors, 0 warnings, 453 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 17/17: Implement support for i.MX7 Sabre board...
WARNING: line over 80 characters
#60: FILE: hw/arm/mcimx7d-sabre.c:31:
+static void mcimx7d_add_psci_node(const struct arm_boot_info *boot_info, void *fdt)
WARNING: line over 80 characters
#114: FILE: hw/arm/mcimx7d-sabre.c:85:
+ object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);
total: 0 errors, 2 warnings, 106 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH 00/17] Initial i.MX7 support
2017-09-18 19:50 [Qemu-devel] [PATCH 00/17] Initial i.MX7 support Andrey Smirnov
` (17 preceding siblings ...)
2017-09-18 21:00 ` [Qemu-devel] [PATCH 00/17] Initial i.MX7 support no-reply
@ 2017-10-06 14:46 ` Peter Maydell
18 siblings, 0 replies; 51+ messages in thread
From: Peter Maydell @ 2017-10-06 14:46 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: qemu-arm, QEMU Developers, Andrey Yurovsky
On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> Hi everyone,
>
> This patch series contains the work that I've done in order to enable
> support for i.MX7 emulation in QEMU. Majority of the set are just odd
> fixes and small features implementation that I had to do to already
> exisitng code but last 5 commits contain new emulation code.
>
> As the one before last commit in the series states the supported i.MX7
> features are:
>
> * up to 2 Cortex A9 cores (SMP works with PSCI)
> * A7 MPCORE (identical to A15 MPCORE)
> * 7 i.MX UARTs
> * 1 CCM device
> * 2 Ethernet controllers (FEC)
> * 3 SD controllers (USDHC)
> * 1 SNVS device
> * 1 WDT device
>
> I also have a follow up series that implements bit needes for PCIe
> emulation support (DesignWare IP emulation + supporting code) which
> I'll be submitting after this series is accepted.
Hi; thanks for these patches. I've now reviewed most of them. Hopefully
somebody else can get to the sdhci patches, as sdhci is not an
area I know much about and I don't think I have the specs.
Please also run your patches through scripts/checkpatch.pl and fix
those issues as necessary (the script does occasionally have false
positives so you need not follow it blindly if it suggests something
ridiculous).
thanks
-- PMM
^ permalink raw reply [flat|nested] 51+ messages in thread