[PATCH for-5.2 00/19] aspeed: mostly cleanups and some extensions

[PATCH for-5.2 00/19] aspeed: mostly cleanups and some extensions

[Cédric Le Goater]

Hello,

Various fixes improving the support of Aspeed machines.

Thanks,

C.

Cédric Le Goater (16):
  m25p80: Return the JEDEC ID twice for mx25l25635e
  m25p80: Add support for mx25l25635f
  m25p80: Add support for n25q512ax3
  aspeed/scu: Fix valid access size on AST2400
  aspeed/smc: Fix MemoryRegionOps definition
  aspeed/smc: Fix max_slaves of the legacy SMC device
  aspeed/sdhci: Fix reset sequence
  ftgmac100: Fix registers that can be read
  ftgmac100: Fix interrupt status "Packet transmitted on ethernet"
  ftgmac100: Fix interrupt status "Packet moved to RX FIFO"
  ftgmac100: Change interrupt status when a DMA error occurs
  ftgmac100: Check for invalid len and address before doing a DMA
    transfer
  ftgmac100: Fix integer overflow in ftgmac100_do_tx()
  ftgmac100: Improve software reset
  aspeed/sdmc: Simplify calculation of RAM bits
  aspeed/smc: Open AHB window of the second chip of the AST2600 FMC
    controller

Joel Stanley (2):
  aspeed/sdmc: Perform memory training
  aspeed/sdmc: Allow writes to unprotected registers

erik-smit (1):
  hw/arm/aspeed: Add board model for Supermicro X11 BMC

 include/hw/misc/aspeed_sdmc.h |  13 +++-
 hw/arm/aspeed.c               |  35 ++++++++++
 hw/block/m25p80.c             |   4 +-
 hw/misc/aspeed_scu.c          |   9 +--
 hw/misc/aspeed_sdmc.c         | 125 +++++++++++++++++++---------------
 hw/net/ftgmac100.c            |  45 ++++++++----
 hw/sd/aspeed_sdhci.c          |  10 ++-
 hw/ssi/aspeed_smc.c           |   6 +-
 8 files changed, 167 insertions(+), 80 deletions(-)

-- 
2.25.4

[PATCH for-5.2 01/19] m25p80: Return the JEDEC ID twice for mx25l25635e

[Cédric Le Goater]

The mx25l25635e returns the JEDEC ID twice when issuing a RDID command :

  [    2.512027] aspeed-smc 1e630000.spi: reading JEDEC ID C2:20:19:C2:20:19

This can break some firmware testing for this condition on the
supermicrox11-bmc machine.

Reported-by: erik-smit <erik.lucas.smit@gmail.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/block/m25p80.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 82270884416e..605ff55c6756 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -217,7 +217,7 @@ static const FlashPartInfo known_devices[] = {
     { INFO("mx25l6405d",  0xc22017,      0,  64 << 10, 128, 0) },
     { INFO("mx25l12805d", 0xc22018,      0,  64 << 10, 256, 0) },
     { INFO("mx25l12855e", 0xc22618,      0,  64 << 10, 256, 0) },
-    { INFO("mx25l25635e", 0xc22019,      0,  64 << 10, 512, 0) },
+    { INFO6("mx25l25635e", 0xc22019,     0xc22019,  64 << 10, 512, 0) },
     { INFO("mx25l25655e", 0xc22619,      0,  64 << 10, 512, 0) },
     { INFO("mx66u51235f", 0xc2253a,      0,  64 << 10, 1024, ER_4K | ER_32K) },
     { INFO("mx66u1g45g",  0xc2253b,      0,  64 << 10, 2048, ER_4K | ER_32K) },
-- 
2.25.4

[PATCH for-5.2 02/19] m25p80: Add support for mx25l25635f

[Cédric Le Goater]

The mx25l25635f is an extenstion of the mx25l25635e. It includes QPI
support, 4-Byte Address Command Set and faster transfers. See this
document for more details :

https://www.macronix.com/Lists/ApplicationNote/Attachments/1892/AN0200V1_MGRT_MX25L25635E_25735E%20to%20MX25L25635F_25735F.pdf

Both devices have the same 3bytes JEDEC ID: 0xc22019. They can be
distinguished with the QPIID command which is only available on
mx25l25635f. The mx25l25635f also has a longer JEDEC ID that we can
use for the model.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/block/m25p80.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 605ff55c6756..1696ab1f7821 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -218,6 +218,7 @@ static const FlashPartInfo known_devices[] = {
     { INFO("mx25l12805d", 0xc22018,      0,  64 << 10, 256, 0) },
     { INFO("mx25l12855e", 0xc22618,      0,  64 << 10, 256, 0) },
     { INFO6("mx25l25635e", 0xc22019,     0xc22019,  64 << 10, 512, 0) },
+    { INFO("mx25l25635f", 0xc22019,      0xc200,  64 << 10, 512, 0) },
     { INFO("mx25l25655e", 0xc22619,      0,  64 << 10, 512, 0) },
     { INFO("mx66u51235f", 0xc2253a,      0,  64 << 10, 1024, ER_4K | ER_32K) },
     { INFO("mx66u1g45g",  0xc2253b,      0,  64 << 10, 2048, ER_4K | ER_32K) },
-- 
2.25.4

Re: [PATCH for-5.2 02/19] m25p80: Add support for mx25l25635f

[Joel Stanley]

On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>
> The mx25l25635f is an extenstion of the mx25l25635e. It includes QPI
> support, 4-Byte Address Command Set and faster transfers. See this
> document for more details :
>
> https://www.macronix.com/Lists/ApplicationNote/Attachments/1892/AN0200V1_MGRT_MX25L25635E_25735E%20to%20MX25L25635F_25735F.pdf
>
> Both devices have the same 3bytes JEDEC ID: 0xc22019. They can be
> distinguished with the QPIID command which is only available on
> mx25l25635f. The mx25l25635f also has a longer JEDEC ID that we can
> use for the model.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

We don't have support for this one in upstream Linux. It's the one
that Alexander tried to get merged by renaming the mysterious
mx66l51235l.


> ---
>  hw/block/m25p80.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 605ff55c6756..1696ab1f7821 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -218,6 +218,7 @@ static const FlashPartInfo known_devices[] = {
>      { INFO("mx25l12805d", 0xc22018,      0,  64 << 10, 256, 0) },
>      { INFO("mx25l12855e", 0xc22618,      0,  64 << 10, 256, 0) },
>      { INFO6("mx25l25635e", 0xc22019,     0xc22019,  64 << 10, 512, 0) },
> +    { INFO("mx25l25635f", 0xc22019,      0xc200,  64 << 10, 512, 0) },
>      { INFO("mx25l25655e", 0xc22619,      0,  64 << 10, 512, 0) },
>      { INFO("mx66u51235f", 0xc2253a,      0,  64 << 10, 1024, ER_4K | ER_32K) },
>      { INFO("mx66u1g45g",  0xc2253b,      0,  64 << 10, 2048, ER_4K | ER_32K) },
> --
> 2.25.4
>

Re: [PATCH for-5.2 02/19] m25p80: Add support for mx25l25635f

[Cédric Le Goater]

On 8/7/20 12:55 AM, Joel Stanley wrote:
> On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> The mx25l25635f is an extenstion of the mx25l25635e. It includes QPI
>> support, 4-Byte Address Command Set and faster transfers. See this
>> document for more details :
>>
>> https://www.macronix.com/Lists/ApplicationNote/Attachments/1892/AN0200V1_MGRT_MX25L25635E_25735E%20to%20MX25L25635F_25735F.pdf
>>
>> Both devices have the same 3bytes JEDEC ID: 0xc22019. They can be
>> distinguished with the QPIID command which is only available on
>> mx25l25635f. The mx25l25635f also has a longer JEDEC ID that we can
>> use for the model.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> 
> We don't have support for this one in upstream Linux.

It's detected by testing the 4BYTE operation capability in the SFDP table
in mx25l25635_post_bfpt_fixups()

The only system I have access to is a SuperMicro P9 Boston but I can not
upload a new kernel. However, the FW reports : 

  BMC flash ID: 0xc21920c2
  jedec_id: 0xc21920c2
  flash type: MX25L25635F
  ReadClk=0x32, WriteClk=0x85, EraseClk=0x85
  [smcfw_spi] cpuclk: 198000000 MHz, RefCLK: 24000000 MHz, AXI-AHB ratio: 2:1
  platform_flash: MX25L25635F (32768 Kbytes)


> It's the one
> that Alexander tried to get merged by renaming the mysterious
> mx66l51235l.

Yes. That one behaves a bit more like the mx25l25635e.

Difficult to find a detection pattern. A model for SFDP would help. 

C.

> 
>> ---
>>  hw/block/m25p80.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 605ff55c6756..1696ab1f7821 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -218,6 +218,7 @@ static const FlashPartInfo known_devices[] = {
>>      { INFO("mx25l12805d", 0xc22018,      0,  64 << 10, 256, 0) },
>>      { INFO("mx25l12855e", 0xc22618,      0,  64 << 10, 256, 0) },
>>      { INFO6("mx25l25635e", 0xc22019,     0xc22019,  64 << 10, 512, 0) },
>> +    { INFO("mx25l25635f", 0xc22019,      0xc200,  64 << 10, 512, 0) },
>>      { INFO("mx25l25655e", 0xc22619,      0,  64 << 10, 512, 0) },
>>      { INFO("mx66u51235f", 0xc2253a,      0,  64 << 10, 1024, ER_4K | ER_32K) },
>>      { INFO("mx66u1g45g",  0xc2253b,      0,  64 << 10, 2048, ER_4K | ER_32K) },
>> --
>> 2.25.4
>>

[PATCH for-5.2 03/19] m25p80: Add support for n25q512ax3

[Cédric Le Goater]

Datasheet available here :

https://www.micron.com/-/media/client/global/Documents/Products/Data%20Sheet/NOR%20Flash/Serial%20NOR/N25Q/n25q_512mb_1ce_3v_65nm.pdf

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/block/m25p80.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 1696ab1f7821..8a3fd959e218 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -238,6 +238,7 @@ static const FlashPartInfo known_devices[] = {
     { INFO("n25q128",     0x20ba18,      0,  64 << 10, 256, 0) },
     { INFO("n25q256a",    0x20ba19,      0,  64 << 10, 512, ER_4K) },
     { INFO("n25q512a",    0x20ba20,      0,  64 << 10, 1024, ER_4K) },
+    { INFO("n25q512ax3",  0x20ba20,  0x1000,  64 << 10, 1024, ER_4K) },
     { INFO_STACKED("n25q00",    0x20ba21, 0x1000, 64 << 10, 2048, ER_4K, 4) },
     { INFO_STACKED("n25q00a",   0x20bb21, 0x1000, 64 << 10, 2048, ER_4K, 4) },
     { INFO_STACKED("mt25ql01g", 0x20ba21, 0x1040, 64 << 10, 2048, ER_4K, 2) },
-- 
2.25.4

Re: [PATCH for-5.2 03/19] m25p80: Add support for n25q512ax3

[Joel Stanley]

On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>
> Datasheet available here :
>
> https://www.micron.com/-/media/client/global/Documents/Products/Data%20Sheet/NOR%20Flash/Serial%20NOR/N25Q/n25q_512mb_1ce_3v_65nm.pdf
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  hw/block/m25p80.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 1696ab1f7821..8a3fd959e218 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -238,6 +238,7 @@ static const FlashPartInfo known_devices[] = {
>      { INFO("n25q128",     0x20ba18,      0,  64 << 10, 256, 0) },
>      { INFO("n25q256a",    0x20ba19,      0,  64 << 10, 512, ER_4K) },
>      { INFO("n25q512a",    0x20ba20,      0,  64 << 10, 1024, ER_4K) },
> +    { INFO("n25q512ax3",  0x20ba20,  0x1000,  64 << 10, 1024, ER_4K) },
>      { INFO_STACKED("n25q00",    0x20ba21, 0x1000, 64 << 10, 2048, ER_4K, 4) },
>      { INFO_STACKED("n25q00a",   0x20bb21, 0x1000, 64 << 10, 2048, ER_4K, 4) },
>      { INFO_STACKED("mt25ql01g", 0x20ba21, 0x1040, 64 << 10, 2048, ER_4K, 2) },
> --
> 2.25.4
>

[PATCH for-5.2 04/19] aspeed/scu: Fix valid access size on AST2400

[Cédric Le Goater]

The read access size of the SCU registers can be 1/2/4 bytes and write
is 4 bytes. Set the min access size to 1 byte to cover both read and
write operations on the AST2400 but keep the min access size of the
other SoCs to 4 bytes as this is an unusual access size.

This fixes support for some old firmware doing 2 bytes reads on the
AST2400 SoC.

Reported-by: erik-smit <erik.lucas.smit@gmail.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/misc/aspeed_scu.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index ec4fef900e27..764222404bef 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -328,9 +328,10 @@ static const MemoryRegionOps aspeed_ast2400_scu_ops = {
     .read = aspeed_scu_read,
     .write = aspeed_ast2400_scu_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
-    .valid.min_access_size = 4,
-    .valid.max_access_size = 4,
-    .valid.unaligned = false,
+    .valid = {
+        .min_access_size = 1,
+        .max_access_size = 4,
+    },
 };
 
 static const MemoryRegionOps aspeed_ast2500_scu_ops = {
-- 
2.25.4

Re: [PATCH for-5.2 04/19] aspeed/scu: Fix valid access size on AST2400

[Philippe Mathieu-Daudé]

On 8/6/20 3:20 PM, Cédric Le Goater wrote:
> The read access size of the SCU registers can be 1/2/4 bytes and write
> is 4 bytes. Set the min access size to 1 byte to cover both read and
> write operations on the AST2400 but keep the min access size of the
> other SoCs to 4 bytes as this is an unusual access size.

From your description it seems you need to implement .valid.accepts().

> 
> This fixes support for some old firmware doing 2 bytes reads on the
> AST2400 SoC.
> 
> Reported-by: erik-smit <erik.lucas.smit@gmail.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/misc/aspeed_scu.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index ec4fef900e27..764222404bef 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -328,9 +328,10 @@ static const MemoryRegionOps aspeed_ast2400_scu_ops = {
>      .read = aspeed_scu_read,
>      .write = aspeed_ast2400_scu_write,
>      .endianness = DEVICE_LITTLE_ENDIAN,
> -    .valid.min_access_size = 4,
> -    .valid.max_access_size = 4,
> -    .valid.unaligned = false,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 4,
> +    },
>  };
>  
>  static const MemoryRegionOps aspeed_ast2500_scu_ops = {
> 

Re: [PATCH for-5.2 04/19] aspeed/scu: Fix valid access size on AST2400

[Cédric Le Goater]

On 8/6/20 3:32 PM, Philippe Mathieu-Daudé wrote:
> On 8/6/20 3:20 PM, Cédric Le Goater wrote:
>> The read access size of the SCU registers can be 1/2/4 bytes and write
>> is 4 bytes. Set the min access size to 1 byte to cover both read and
>> write operations on the AST2400 but keep the min access size of the
>> other SoCs to 4 bytes as this is an unusual access size.
> 
> From your description it seems you need to implement .valid.accepts().

Ah yes. 

Can this come as a follow up ? because this patch is enabling
support for the Supermicro X11 BMC machine.

Thanks,

C. 


> 
>>
>> This fixes support for some old firmware doing 2 bytes reads on the
>> AST2400 SoC.
>>
>> Reported-by: erik-smit <erik.lucas.smit@gmail.com>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/misc/aspeed_scu.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
>> index ec4fef900e27..764222404bef 100644
>> --- a/hw/misc/aspeed_scu.c
>> +++ b/hw/misc/aspeed_scu.c
>> @@ -328,9 +328,10 @@ static const MemoryRegionOps aspeed_ast2400_scu_ops = {
>>      .read = aspeed_scu_read,
>>      .write = aspeed_ast2400_scu_write,
>>      .endianness = DEVICE_LITTLE_ENDIAN,
>> -    .valid.min_access_size = 4,
>> -    .valid.max_access_size = 4,
>> -    .valid.unaligned = false,
>> +    .valid = {
>> +        .min_access_size = 1,
>> +        .max_access_size = 4,
>> +    },
>>  };
>>  
>>  static const MemoryRegionOps aspeed_ast2500_scu_ops = {
>>
> 

Re: [PATCH for-5.2 04/19] aspeed/scu: Fix valid access size on AST2400

[Philippe Mathieu-Daudé]

On 8/6/20 3:49 PM, Cédric Le Goater wrote:
> On 8/6/20 3:32 PM, Philippe Mathieu-Daudé wrote:
>> On 8/6/20 3:20 PM, Cédric Le Goater wrote:
>>> The read access size of the SCU registers can be 1/2/4 bytes and write
>>> is 4 bytes. Set the min access size to 1 byte to cover both read and
>>> write operations on the AST2400 but keep the min access size of the
>>> other SoCs to 4 bytes as this is an unusual access size.
>>
>> From your description it seems you need to implement .valid.accepts().
> 
> Ah yes. 
> 
> Can this come as a follow up ? because this patch is enabling
> support for the Supermicro X11 BMC machine.

This is certainly not a blocker, so up to you :)

> 
> Thanks,
> 
> C. 
> 
> 
>>
>>>
>>> This fixes support for some old firmware doing 2 bytes reads on the
>>> AST2400 SoC.
>>>
>>> Reported-by: erik-smit <erik.lucas.smit@gmail.com>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>  hw/misc/aspeed_scu.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
>>> index ec4fef900e27..764222404bef 100644
>>> --- a/hw/misc/aspeed_scu.c
>>> +++ b/hw/misc/aspeed_scu.c
>>> @@ -328,9 +328,10 @@ static const MemoryRegionOps aspeed_ast2400_scu_ops = {
>>>      .read = aspeed_scu_read,
>>>      .write = aspeed_ast2400_scu_write,
>>>      .endianness = DEVICE_LITTLE_ENDIAN,
>>> -    .valid.min_access_size = 4,
>>> -    .valid.max_access_size = 4,
>>> -    .valid.unaligned = false,
>>> +    .valid = {
>>> +        .min_access_size = 1,
>>> +        .max_access_size = 4,
>>> +    },
>>>  };
>>>  
>>>  static const MemoryRegionOps aspeed_ast2500_scu_ops = {
>>>
>>
> 
> 

Re: [PATCH for-5.2 04/19] aspeed/scu: Fix valid access size on AST2400

[Joel Stanley]

On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>
> The read access size of the SCU registers can be 1/2/4 bytes and write
> is 4 bytes. Set the min access size to 1 byte to cover both read and
> write operations on the AST2400 but keep the min access size of the
> other SoCs to 4 bytes as this is an unusual access size.
>
> This fixes support for some old firmware doing 2 bytes reads on the
> AST2400 SoC.
>
> Reported-by: erik-smit <erik.lucas.smit@gmail.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  hw/misc/aspeed_scu.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index ec4fef900e27..764222404bef 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -328,9 +328,10 @@ static const MemoryRegionOps aspeed_ast2400_scu_ops = {
>      .read = aspeed_scu_read,
>      .write = aspeed_ast2400_scu_write,
>      .endianness = DEVICE_LITTLE_ENDIAN,
> -    .valid.min_access_size = 4,
> -    .valid.max_access_size = 4,
> -    .valid.unaligned = false,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 4,
> +    },
>  };
>
>  static const MemoryRegionOps aspeed_ast2500_scu_ops = {
> --
> 2.25.4
>

[PATCH for-5.2 05/19] hw/arm/aspeed: Add board model for Supermicro X11 BMC

[Cédric Le Goater]

From: erik-smit <erik.lucas.smit@gmail.com>

The BMC Firmware can be downloaded from :

  https://www.supermicro.com/en/products/motherboard/X11SSL-F

Signed-off-by: erik-smit <erik.lucas.smit@gmail.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
[ clg: Modified commit log ]
Message-Id: <20200715173418.186-1-erik.lucas.smit@gmail.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index fcb1a7cd8729..d17a4885a03c 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -57,6 +57,20 @@ struct AspeedMachineState {
         SCU_HW_STRAP_VGA_SIZE_SET(VGA_16M_DRAM) |                       \
         SCU_AST2400_HW_STRAP_BOOT_MODE(AST2400_SPI_BOOT))
 
+/* TODO: Find the actual hardware value */
+#define SUPERMICROX11_BMC_HW_STRAP1 (                                   \
+        SCU_AST2400_HW_STRAP_DRAM_SIZE(DRAM_SIZE_128MB) |               \
+        SCU_AST2400_HW_STRAP_DRAM_CONFIG(2) |                           \
+        SCU_AST2400_HW_STRAP_ACPI_DIS |                                 \
+        SCU_AST2400_HW_STRAP_SET_CLK_SOURCE(AST2400_CLK_48M_IN) |       \
+        SCU_HW_STRAP_VGA_CLASS_CODE |                                   \
+        SCU_HW_STRAP_LPC_RESET_PIN |                                    \
+        SCU_HW_STRAP_SPI_MODE(SCU_HW_STRAP_SPI_M_S_EN) |                \
+        SCU_AST2400_HW_STRAP_SET_CPU_AHB_RATIO(AST2400_CPU_AHB_RATIO_2_1) | \
+        SCU_HW_STRAP_SPI_WIDTH |                                        \
+        SCU_HW_STRAP_VGA_SIZE_SET(VGA_16M_DRAM) |                       \
+        SCU_AST2400_HW_STRAP_BOOT_MODE(AST2400_SPI_BOOT))
+
 /* AST2500 evb hardware value: 0xF100C2E6 */
 #define AST2500_EVB_HW_STRAP1 ((                                        \
         AST2500_HW_STRAP1_DEFAULTS |                                    \
@@ -603,6 +617,23 @@ static void aspeed_machine_palmetto_class_init(ObjectClass *oc, void *data)
         aspeed_soc_num_cpus(amc->soc_name);
 };
 
+static void aspeed_machine_supermicrox11_bmc_class_init(ObjectClass *oc,
+                                                        void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+    AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
+
+    mc->desc       = "Supermicro X11 BMC (ARM926EJ-S)";
+    amc->soc_name  = "ast2400-a1";
+    amc->hw_strap1 = SUPERMICROX11_BMC_HW_STRAP1;
+    amc->fmc_model = "mx25l25635e";
+    amc->spi_model = "mx25l25635e";
+    amc->num_cs    = 1;
+    amc->macs_mask = ASPEED_MAC0_ON | ASPEED_MAC1_ON;
+    amc->i2c_init  = palmetto_bmc_i2c_init;
+    mc->default_ram_size = 256 * MiB;
+}
+
 static void aspeed_machine_ast2500_evb_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -731,6 +762,10 @@ static const TypeInfo aspeed_machine_types[] = {
         .name          = MACHINE_TYPE_NAME("palmetto-bmc"),
         .parent        = TYPE_ASPEED_MACHINE,
         .class_init    = aspeed_machine_palmetto_class_init,
+    }, {
+        .name          = MACHINE_TYPE_NAME("supermicrox11-bmc"),
+        .parent        = TYPE_ASPEED_MACHINE,
+        .class_init    = aspeed_machine_supermicrox11_bmc_class_init,
     }, {
         .name          = MACHINE_TYPE_NAME("ast2500-evb"),
         .parent        = TYPE_ASPEED_MACHINE,
-- 
2.25.4

Re: [PATCH for-5.2 05/19] hw/arm/aspeed: Add board model for Supermicro X11 BMC

[Joel Stanley]

On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>
> From: erik-smit <erik.lucas.smit@gmail.com>
>
> The BMC Firmware can be downloaded from :
>
>   https://www.supermicro.com/en/products/motherboard/X11SSL-F
>
> Signed-off-by: erik-smit <erik.lucas.smit@gmail.com>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> [ clg: Modified commit log ]
> Message-Id: <20200715173418.186-1-erik.lucas.smit@gmail.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  hw/arm/aspeed.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index fcb1a7cd8729..d17a4885a03c 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -57,6 +57,20 @@ struct AspeedMachineState {
>          SCU_HW_STRAP_VGA_SIZE_SET(VGA_16M_DRAM) |                       \
>          SCU_AST2400_HW_STRAP_BOOT_MODE(AST2400_SPI_BOOT))
>
> +/* TODO: Find the actual hardware value */

I've asked some bmc people if they could provide this.

> +#define SUPERMICROX11_BMC_HW_STRAP1 (                                   \
> +        SCU_AST2400_HW_STRAP_DRAM_SIZE(DRAM_SIZE_128MB) |               \
> +        SCU_AST2400_HW_STRAP_DRAM_CONFIG(2) |                           \
> +        SCU_AST2400_HW_STRAP_ACPI_DIS |                                 \
> +        SCU_AST2400_HW_STRAP_SET_CLK_SOURCE(AST2400_CLK_48M_IN) |       \
> +        SCU_HW_STRAP_VGA_CLASS_CODE |                                   \
> +        SCU_HW_STRAP_LPC_RESET_PIN |                                    \
> +        SCU_HW_STRAP_SPI_MODE(SCU_HW_STRAP_SPI_M_S_EN) |                \
> +        SCU_AST2400_HW_STRAP_SET_CPU_AHB_RATIO(AST2400_CPU_AHB_RATIO_2_1) | \
> +        SCU_HW_STRAP_SPI_WIDTH |                                        \
> +        SCU_HW_STRAP_VGA_SIZE_SET(VGA_16M_DRAM) |                       \
> +        SCU_AST2400_HW_STRAP_BOOT_MODE(AST2400_SPI_BOOT))
> +
>  /* AST2500 evb hardware value: 0xF100C2E6 */
>  #define AST2500_EVB_HW_STRAP1 ((                                        \
>          AST2500_HW_STRAP1_DEFAULTS |                                    \
> @@ -603,6 +617,23 @@ static void aspeed_machine_palmetto_class_init(ObjectClass *oc, void *data)
>          aspeed_soc_num_cpus(amc->soc_name);
>  };
>
> +static void aspeed_machine_supermicrox11_bmc_class_init(ObjectClass *oc,
> +                                                        void *data)
> +{
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +    AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
> +
> +    mc->desc       = "Supermicro X11 BMC (ARM926EJ-S)";
> +    amc->soc_name  = "ast2400-a1";
> +    amc->hw_strap1 = SUPERMICROX11_BMC_HW_STRAP1;
> +    amc->fmc_model = "mx25l25635e";
> +    amc->spi_model = "mx25l25635e";
> +    amc->num_cs    = 1;
> +    amc->macs_mask = ASPEED_MAC0_ON | ASPEED_MAC1_ON;
> +    amc->i2c_init  = palmetto_bmc_i2c_init;
> +    mc->default_ram_size = 256 * MiB;
> +}
> +
>  static void aspeed_machine_ast2500_evb_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -731,6 +762,10 @@ static const TypeInfo aspeed_machine_types[] = {
>          .name          = MACHINE_TYPE_NAME("palmetto-bmc"),
>          .parent        = TYPE_ASPEED_MACHINE,
>          .class_init    = aspeed_machine_palmetto_class_init,
> +    }, {
> +        .name          = MACHINE_TYPE_NAME("supermicrox11-bmc"),
> +        .parent        = TYPE_ASPEED_MACHINE,
> +        .class_init    = aspeed_machine_supermicrox11_bmc_class_init,
>      }, {
>          .name          = MACHINE_TYPE_NAME("ast2500-evb"),
>          .parent        = TYPE_ASPEED_MACHINE,
> --
> 2.25.4
>

[PATCH for-5.2 06/19] aspeed/smc: Fix MemoryRegionOps definition

[Cédric Le Goater]

Unaligned access support is a leftover from the initial commit. There
is no such need on this device register mapping. Remove it.

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ssi/aspeed_smc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 4fab1f5f855e..0646e0dca72e 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -1299,10 +1299,8 @@ static const MemoryRegionOps aspeed_smc_ops = {
     .read = aspeed_smc_read,
     .write = aspeed_smc_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
-    .valid.unaligned = true,
 };
 
-
 /*
  * Initialize the custom address spaces for DMAs
  */
-- 
2.25.4

Re: [PATCH for-5.2 06/19] aspeed/smc: Fix MemoryRegionOps definition

[Joel Stanley]

On Thu, 6 Aug 2020 at 13:23, Cédric Le Goater <clg@kaod.org> wrote:
>
> Unaligned access support is a leftover from the initial commit. There
> is no such need on this device register mapping. Remove it.
>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  hw/ssi/aspeed_smc.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 4fab1f5f855e..0646e0dca72e 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -1299,10 +1299,8 @@ static const MemoryRegionOps aspeed_smc_ops = {
>      .read = aspeed_smc_read,
>      .write = aspeed_smc_write,
>      .endianness = DEVICE_LITTLE_ENDIAN,
> -    .valid.unaligned = true,
>  };
>
> -
>  /*
>   * Initialize the custom address spaces for DMAs
>   */
> --
> 2.25.4
>

[PATCH for-5.2 07/19] aspeed/smc: Fix max_slaves of the legacy SMC device

[Cédric Le Goater]

The legacy controller only has one slave.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ssi/aspeed_smc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 0646e0dca72e..8c79a5552f93 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -259,7 +259,7 @@ static const AspeedSMCController controllers[] = {
         .r_timings         = R_TIMINGS,
         .nregs_timings     = 1,
         .conf_enable_w0    = CONF_ENABLE_W0,
-        .max_slaves        = 5,
+        .max_slaves        = 1,
         .segments          = aspeed_segments_legacy,
         .flash_window_base = ASPEED_SOC_SMC_FLASH_BASE,
         .flash_window_size = 0x6000000,
-- 
2.25.4

Re: [PATCH for-5.2 07/19] aspeed/smc: Fix max_slaves of the legacy SMC device

[Joel Stanley]

On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>
> The legacy controller only has one slave.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  hw/ssi/aspeed_smc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 0646e0dca72e..8c79a5552f93 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -259,7 +259,7 @@ static const AspeedSMCController controllers[] = {
>          .r_timings         = R_TIMINGS,
>          .nregs_timings     = 1,
>          .conf_enable_w0    = CONF_ENABLE_W0,
> -        .max_slaves        = 5,
> +        .max_slaves        = 1,
>          .segments          = aspeed_segments_legacy,
>          .flash_window_base = ASPEED_SOC_SMC_FLASH_BASE,
>          .flash_window_size = 0x6000000,
> --
> 2.25.4
>

[PATCH for-5.2 08/19] aspeed/sdhci: Fix reset sequence

[Cédric Le Goater]

BIT(0) of the ASPEED_SDHCI_INFO register is set by SW and polled until
the bit is cleared by HW. Add definitions for the default value of
this register and fix the reset sequence by clearing the RESET bit.

Cc: Eddie James <eajames@linux.ibm.com>
Fixes: 2bea128c3d0b ("hw/sd/aspeed_sdhci: New device")
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/sd/aspeed_sdhci.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/sd/aspeed_sdhci.c b/hw/sd/aspeed_sdhci.c
index 22cafce0fbdc..c51cda932463 100644
--- a/hw/sd/aspeed_sdhci.c
+++ b/hw/sd/aspeed_sdhci.c
@@ -16,7 +16,9 @@
 #include "hw/qdev-properties.h"
 
 #define ASPEED_SDHCI_INFO            0x00
-#define  ASPEED_SDHCI_INFO_RESET     0x00030000
+#define  ASPEED_SDHCI_INFO_SLOT1     (1 << 17)
+#define  ASPEED_SDHCI_INFO_SLOT0     (1 << 16)
+#define  ASPEED_SDHCI_INFO_RESET     (1 << 0)
 #define ASPEED_SDHCI_DEBOUNCE        0x04
 #define  ASPEED_SDHCI_DEBOUNCE_RESET 0x00000005
 #define ASPEED_SDHCI_BUS             0x08
@@ -67,6 +69,9 @@ static void aspeed_sdhci_write(void *opaque, hwaddr addr, uint64_t val,
     AspeedSDHCIState *sdhci = opaque;
 
     switch (addr) {
+    case ASPEED_SDHCI_INFO:
+        sdhci->regs[TO_REG(addr)] = (uint32_t)val & ~ASPEED_SDHCI_INFO_RESET;
+
     case ASPEED_SDHCI_SDIO_140:
         sdhci->slots[0].capareg = (uint64_t)(uint32_t)val;
         break;
@@ -155,7 +160,8 @@ static void aspeed_sdhci_reset(DeviceState *dev)
     AspeedSDHCIState *sdhci = ASPEED_SDHCI(dev);
 
     memset(sdhci->regs, 0, ASPEED_SDHCI_REG_SIZE);
-    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] = ASPEED_SDHCI_INFO_RESET;
+    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] =
+        ASPEED_SDHCI_INFO_SLOT1 | ASPEED_SDHCI_INFO_SLOT0;
     sdhci->regs[TO_REG(ASPEED_SDHCI_DEBOUNCE)] = ASPEED_SDHCI_DEBOUNCE_RESET;
 }
 
-- 
2.25.4

Re: [PATCH for-5.2 08/19] aspeed/sdhci: Fix reset sequence

[Joel Stanley]

On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>
> BIT(0) of the ASPEED_SDHCI_INFO register is set by SW and polled until
> the bit is cleared by HW. Add definitions for the default value of
> this register and fix the reset sequence by clearing the RESET bit.

This is mentioned in the datasheet but I couldn't find if software
depends on the behaviour. Were you just trying to make the model more
accurate?

>  #define ASPEED_SDHCI_INFO            0x00
> -#define  ASPEED_SDHCI_INFO_RESET     0x00030000
> +#define  ASPEED_SDHCI_INFO_SLOT1     (1 << 17)
> +#define  ASPEED_SDHCI_INFO_SLOT0     (1 << 16)
> +#define  ASPEED_SDHCI_INFO_RESET     (1 << 0)
>  #define ASPEED_SDHCI_DEBOUNCE        0x04
>  #define  ASPEED_SDHCI_DEBOUNCE_RESET 0x00000005
>  #define ASPEED_SDHCI_BUS             0x08
> @@ -67,6 +69,9 @@ static void aspeed_sdhci_write(void *opaque, hwaddr addr, uint64_t val,
>      AspeedSDHCIState *sdhci = opaque;
>
>      switch (addr) {
> +    case ASPEED_SDHCI_INFO:
> +        sdhci->regs[TO_REG(addr)] = (uint32_t)val & ~ASPEED_SDHCI_INFO_RESET;

I think bits 24 and 25 should be writable too?

        sdhci->regs[TO_REG(addr)] = (uint32_t)val &
~(ASPEED_SDHCI_INFO_RESET | ASPEED_SDHCI_INFO_SLOT10 |
ASPEED_SDHCI_INFO_SLOT1);

> +
>      case ASPEED_SDHCI_SDIO_140:
>          sdhci->slots[0].capareg = (uint64_t)(uint32_t)val;
>          break;
> @@ -155,7 +160,8 @@ static void aspeed_sdhci_reset(DeviceState *dev)
>      AspeedSDHCIState *sdhci = ASPEED_SDHCI(dev);
>
>      memset(sdhci->regs, 0, ASPEED_SDHCI_REG_SIZE);
> -    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] = ASPEED_SDHCI_INFO_RESET;
> +    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] =
> +        ASPEED_SDHCI_INFO_SLOT1 | ASPEED_SDHCI_INFO_SLOT0;

If we want to be super strict this is true for the "sd" devices, but
the "emmc" device in the ast2600 only sets slot0. I don't think this
distinction is important to model though.

>      sdhci->regs[TO_REG(ASPEED_SDHCI_DEBOUNCE)] = ASPEED_SDHCI_DEBOUNCE_RESET;
>  }
>
> --
> 2.25.4
>

Re: [PATCH for-5.2 08/19] aspeed/sdhci: Fix reset sequence

[Cédric Le Goater]

On 8/7/20 1:42 AM, Joel Stanley wrote:
> On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> BIT(0) of the ASPEED_SDHCI_INFO register is set by SW and polled until
>> the bit is cleared by HW. Add definitions for the default value of
>> this register and fix the reset sequence by clearing the RESET bit.
> 
> This is mentioned in the datasheet but I couldn't find if software
> depends on the behaviour. Were you just trying to make the model more
> accurate?

Yes. The AMI FW for the palmetto is requiring this : 


U-Boot 1.1.6 (Oct 27 2016 - 10:46:29)

DRAM:  446 MiB
Found SPI Chip Numonyx n25q256 
Flash: 32 MiB
MMC:   ast_sd: 0
Environment Read 1 time
Net:   ast_eth0
DRAM ECC disabled
Hit Esc key to stop autoboot:  0 
Image to be booted is 1
conf @ /dev/mtdblock1 Address 20050000
Found Root File System @ /dev/mtdblock2
root @ /dev/mtdblock2 Address 20120000
extlog @ /dev/mtdblock3 Address 20cd0000
www @ /dev/mtdblock4 Address 20d90000
Un-Protect Flash Bank # 1
Booting from Primary side
Booting from MODULE_PIMAGE ...
Bootargs = [root=/dev/mtdblock2 ro ip=none console=ttyS4,38400 rootfstype=cramfs bigphysarea=8192 imagebooted=1]
## Booting image at 20ae0040 ...
   Image Name:   Linux-2.6.28.10-ami
   Image Type:   ARM Linux Kernel Image (uncompressed)
   Data Size:    1943652 Bytes = 1.9 MiB
   Load Address: 40008000
   Entry Point:  40008000
   Verifying Checksum ... OK
OK

Starting kernel ...

Uncompressing Linux............................................................................................................................. done, booting the kernel.
Linux version 2.6.28.10-ami (root@viswa-desk) (gcc version 4.5.1 (Sourcery G++ Lite 2010.09-50) ) #1 Thu Oct 27 10:45:59 EDT 2016
CPU: ARM926EJ-S [41069265] revision 5 (ARMv5TEJ), cr=00093177
CPU: VIVT data cache, VIVT instruction cache
Machine: AST2400EVB



> 
>>  #define ASPEED_SDHCI_INFO            0x00
>> -#define  ASPEED_SDHCI_INFO_RESET     0x00030000
>> +#define  ASPEED_SDHCI_INFO_SLOT1     (1 << 17)
>> +#define  ASPEED_SDHCI_INFO_SLOT0     (1 << 16)
>> +#define  ASPEED_SDHCI_INFO_RESET     (1 << 0)
>>  #define ASPEED_SDHCI_DEBOUNCE        0x04
>>  #define  ASPEED_SDHCI_DEBOUNCE_RESET 0x00000005
>>  #define ASPEED_SDHCI_BUS             0x08
>> @@ -67,6 +69,9 @@ static void aspeed_sdhci_write(void *opaque, hwaddr addr, uint64_t val,
>>      AspeedSDHCIState *sdhci = opaque;
>>
>>      switch (addr) {
>> +    case ASPEED_SDHCI_INFO:
>> +        sdhci->regs[TO_REG(addr)] = (uint32_t)val & ~ASPEED_SDHCI_INFO_RESET;
> 
> I think bits 24 and 25 should be writable too?

ok. I will take a look.
> 
>         sdhci->regs[TO_REG(addr)] = (uint32_t)val &
> ~(ASPEED_SDHCI_INFO_RESET | ASPEED_SDHCI_INFO_SLOT10 |
> ASPEED_SDHCI_INFO_SLOT1);
> 
>> +
>>      case ASPEED_SDHCI_SDIO_140:
>>          sdhci->slots[0].capareg = (uint64_t)(uint32_t)val;
>>          break;
>> @@ -155,7 +160,8 @@ static void aspeed_sdhci_reset(DeviceState *dev)
>>      AspeedSDHCIState *sdhci = ASPEED_SDHCI(dev);
>>
>>      memset(sdhci->regs, 0, ASPEED_SDHCI_REG_SIZE);
>> -    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] = ASPEED_SDHCI_INFO_RESET;
>> +    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] =
>> +        ASPEED_SDHCI_INFO_SLOT1 | ASPEED_SDHCI_INFO_SLOT0;
> 
> If we want to be super strict this is true for the "sd" devices, but
> the "emmc" device in the ast2600 only sets slot0. I don't think this
> distinction is important to model though.

OK. we can add different reset arrays depending on the SoC. 

C.

>>      sdhci->regs[TO_REG(ASPEED_SDHCI_DEBOUNCE)] = ASPEED_SDHCI_DEBOUNCE_RESET;
>>  }
>>
>> --
>> 2.25.4
>>

Re: [PATCH for-5.2 08/19] aspeed/sdhci: Fix reset sequence

[Cédric Le Goater]

On 8/7/20 1:42 AM, Joel Stanley wrote:
> On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> BIT(0) of the ASPEED_SDHCI_INFO register is set by SW and polled until
>> the bit is cleared by HW. Add definitions for the default value of
>> this register and fix the reset sequence by clearing the RESET bit.
> 
> This is mentioned in the datasheet but I couldn't find if software
> depends on the behaviour. Were you just trying to make the model more
> accurate?
> 
>>  #define ASPEED_SDHCI_INFO            0x00
>> -#define  ASPEED_SDHCI_INFO_RESET     0x00030000
>> +#define  ASPEED_SDHCI_INFO_SLOT1     (1 << 17)
>> +#define  ASPEED_SDHCI_INFO_SLOT0     (1 << 16)
>> +#define  ASPEED_SDHCI_INFO_RESET     (1 << 0)
>>  #define ASPEED_SDHCI_DEBOUNCE        0x04
>>  #define  ASPEED_SDHCI_DEBOUNCE_RESET 0x00000005
>>  #define ASPEED_SDHCI_BUS             0x08
>> @@ -67,6 +69,9 @@ static void aspeed_sdhci_write(void *opaque, hwaddr addr, uint64_t val,
>>      AspeedSDHCIState *sdhci = opaque;
>>
>>      switch (addr) {
>> +    case ASPEED_SDHCI_INFO:
>> +        sdhci->regs[TO_REG(addr)] = (uint32_t)val & ~ASPEED_SDHCI_INFO_RESET;
> 
> I think bits 24 and 25 should be writable too?
> 
>         sdhci->regs[TO_REG(addr)] = (uint32_t)val &
> ~(ASPEED_SDHCI_INFO_RESET | ASPEED_SDHCI_INFO_SLOT10 |
> ASPEED_SDHCI_INFO_SLOT1);
> 
>> +
>>      case ASPEED_SDHCI_SDIO_140:
>>          sdhci->slots[0].capareg = (uint64_t)(uint32_t)val;
>>          break;
>> @@ -155,7 +160,8 @@ static void aspeed_sdhci_reset(DeviceState *dev)
>>      AspeedSDHCIState *sdhci = ASPEED_SDHCI(dev);
>>
>>      memset(sdhci->regs, 0, ASPEED_SDHCI_REG_SIZE);
>> -    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] = ASPEED_SDHCI_INFO_RESET;
>> +    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] =
>> +        ASPEED_SDHCI_INFO_SLOT1 | ASPEED_SDHCI_INFO_SLOT0;
> 
> If we want to be super strict this is true for the "sd" devices, but
> the "emmc" device in the ast2600 only sets slot0. I don't think this
> distinction is important to model though.

Both slots seems to be activated on all three SoCs. Am I looking at the 
wrong controller ? 

Thanks,

C. 
 
> 
>>      sdhci->regs[TO_REG(ASPEED_SDHCI_DEBOUNCE)] = ASPEED_SDHCI_DEBOUNCE_RESET;
>>  }
>>
>> --
>> 2.25.4
>>

Re: [PATCH for-5.2 08/19] aspeed/sdhci: Fix reset sequence

[Joel Stanley]

On Mon, 10 Aug 2020 at 17:16, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 8/7/20 1:42 AM, Joel Stanley wrote:
> > On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
> >>
> >> BIT(0) of the ASPEED_SDHCI_INFO register is set by SW and polled until
> >> the bit is cleared by HW. Add definitions for the default value of
> >> this register and fix the reset sequence by clearing the RESET bit.
> >
> > This is mentioned in the datasheet but I couldn't find if software
> > depends on the behaviour. Were you just trying to make the model more
> > accurate?
> >
> >>  #define ASPEED_SDHCI_INFO            0x00
> >> -#define  ASPEED_SDHCI_INFO_RESET     0x00030000
> >> +#define  ASPEED_SDHCI_INFO_SLOT1     (1 << 17)
> >> +#define  ASPEED_SDHCI_INFO_SLOT0     (1 << 16)
> >> +#define  ASPEED_SDHCI_INFO_RESET     (1 << 0)
> >>  #define ASPEED_SDHCI_DEBOUNCE        0x04
> >>  #define  ASPEED_SDHCI_DEBOUNCE_RESET 0x00000005
> >>  #define ASPEED_SDHCI_BUS             0x08
> >> @@ -67,6 +69,9 @@ static void aspeed_sdhci_write(void *opaque, hwaddr addr, uint64_t val,
> >>      AspeedSDHCIState *sdhci = opaque;
> >>
> >>      switch (addr) {
> >> +    case ASPEED_SDHCI_INFO:
> >> +        sdhci->regs[TO_REG(addr)] = (uint32_t)val & ~ASPEED_SDHCI_INFO_RESET;
> >
> > I think bits 24 and 25 should be writable too?
> >
> >         sdhci->regs[TO_REG(addr)] = (uint32_t)val &
> > ~(ASPEED_SDHCI_INFO_RESET | ASPEED_SDHCI_INFO_SLOT10 |
> > ASPEED_SDHCI_INFO_SLOT1);
> >
> >> +
> >>      case ASPEED_SDHCI_SDIO_140:
> >>          sdhci->slots[0].capareg = (uint64_t)(uint32_t)val;
> >>          break;
> >> @@ -155,7 +160,8 @@ static void aspeed_sdhci_reset(DeviceState *dev)
> >>      AspeedSDHCIState *sdhci = ASPEED_SDHCI(dev);
> >>
> >>      memset(sdhci->regs, 0, ASPEED_SDHCI_REG_SIZE);
> >> -    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] = ASPEED_SDHCI_INFO_RESET;
> >> +    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] =
> >> +        ASPEED_SDHCI_INFO_SLOT1 | ASPEED_SDHCI_INFO_SLOT0;
> >
> > If we want to be super strict this is true for the "sd" devices, but
> > the "emmc" device in the ast2600 only sets slot0. I don't think this
> > distinction is important to model though.
>
> Both slots seems to be activated on all three SoCs. Am I looking at the
> wrong controller ?

Yes. the "SD/SDIO Host Controller" have both slots. The "eMMC
controller" at 0x1E750000 on the ast2600 has just the one slot.

We have a property for the number of slots, so we could do something like this:

--- a/hw/sd/aspeed_sdhci.c
+++ b/hw/sd/aspeed_sdhci.c
@@ -159,12 +159,15 @@ static void aspeed_sdhci_realize(DeviceState
*dev, Error **errp)
 static void aspeed_sdhci_reset(DeviceState *dev)
 {
     AspeedSDHCIState *sdhci = ASPEED_SDHCI(dev);
+    uint32_t slots = ASPEED_SDHCI_INFO_SLOT0;

     memset(sdhci->regs, 0, ASPEED_SDHCI_REG_SIZE);

+    if (sdhci->num_slots == 2)
+        slots |= ASPEED_SDHCI_INFO_SLOT1;
+
     /* Same default value on AST2400, AST2500 and AST2600 SoCs */
-    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] =
-        ASPEED_SDHCI_INFO_SLOT1 | ASPEED_SDHCI_INFO_SLOT0;
+    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] = slots;
     sdhci->regs[TO_REG(ASPEED_SDHCI_DEBOUNCE)] = ASPEED_SDHCI_DEBOUNCE_RESET;
 }

Re: [PATCH for-5.2 08/19] aspeed/sdhci: Fix reset sequence

[Cédric Le Goater]

On 8/11/20 1:20 AM, Joel Stanley wrote:
> On Mon, 10 Aug 2020 at 17:16, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> On 8/7/20 1:42 AM, Joel Stanley wrote:
>>> On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>>>>
>>>> BIT(0) of the ASPEED_SDHCI_INFO register is set by SW and polled until
>>>> the bit is cleared by HW. Add definitions for the default value of
>>>> this register and fix the reset sequence by clearing the RESET bit.
>>>
>>> This is mentioned in the datasheet but I couldn't find if software
>>> depends on the behaviour. Were you just trying to make the model more
>>> accurate?
>>>
>>>>  #define ASPEED_SDHCI_INFO            0x00
>>>> -#define  ASPEED_SDHCI_INFO_RESET     0x00030000
>>>> +#define  ASPEED_SDHCI_INFO_SLOT1     (1 << 17)
>>>> +#define  ASPEED_SDHCI_INFO_SLOT0     (1 << 16)
>>>> +#define  ASPEED_SDHCI_INFO_RESET     (1 << 0)
>>>>  #define ASPEED_SDHCI_DEBOUNCE        0x04
>>>>  #define  ASPEED_SDHCI_DEBOUNCE_RESET 0x00000005
>>>>  #define ASPEED_SDHCI_BUS             0x08
>>>> @@ -67,6 +69,9 @@ static void aspeed_sdhci_write(void *opaque, hwaddr addr, uint64_t val,
>>>>      AspeedSDHCIState *sdhci = opaque;
>>>>
>>>>      switch (addr) {
>>>> +    case ASPEED_SDHCI_INFO:
>>>> +        sdhci->regs[TO_REG(addr)] = (uint32_t)val & ~ASPEED_SDHCI_INFO_RESET;
>>>
>>> I think bits 24 and 25 should be writable too?
>>>
>>>         sdhci->regs[TO_REG(addr)] = (uint32_t)val &
>>> ~(ASPEED_SDHCI_INFO_RESET | ASPEED_SDHCI_INFO_SLOT10 |
>>> ASPEED_SDHCI_INFO_SLOT1);
>>>
>>>> +
>>>>      case ASPEED_SDHCI_SDIO_140:
>>>>          sdhci->slots[0].capareg = (uint64_t)(uint32_t)val;
>>>>          break;
>>>> @@ -155,7 +160,8 @@ static void aspeed_sdhci_reset(DeviceState *dev)
>>>>      AspeedSDHCIState *sdhci = ASPEED_SDHCI(dev);
>>>>
>>>>      memset(sdhci->regs, 0, ASPEED_SDHCI_REG_SIZE);
>>>> -    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] = ASPEED_SDHCI_INFO_RESET;
>>>> +    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] =
>>>> +        ASPEED_SDHCI_INFO_SLOT1 | ASPEED_SDHCI_INFO_SLOT0;
>>>
>>> If we want to be super strict this is true for the "sd" devices, but
>>> the "emmc" device in the ast2600 only sets slot0. I don't think this
>>> distinction is important to model though.
>>
>> Both slots seems to be activated on all three SoCs. Am I looking at the
>> wrong controller ?
> 
> Yes. the "SD/SDIO Host Controller" have both slots. The "eMMC
> controller" at 0x1E750000 on the ast2600 has just the one slot.

I forgot that one.

> We have a property for the number of slots, so we could do something like this:
> 
> --- a/hw/sd/aspeed_sdhci.c
> +++ b/hw/sd/aspeed_sdhci.c
> @@ -159,12 +159,15 @@ static void aspeed_sdhci_realize(DeviceState
> *dev, Error **errp)
>  static void aspeed_sdhci_reset(DeviceState *dev)
>  {
>      AspeedSDHCIState *sdhci = ASPEED_SDHCI(dev);
> +    uint32_t slots = ASPEED_SDHCI_INFO_SLOT0;
> 
>      memset(sdhci->regs, 0, ASPEED_SDHCI_REG_SIZE);
> 
> +    if (sdhci->num_slots == 2)
> +        slots |= ASPEED_SDHCI_INFO_SLOT1;
> +

I think this is fine. The alternative would be an object class but it
would be a bit overkill. 

Thanks,

C. 
  
>      /* Same default value on AST2400, AST2500 and AST2600 SoCs */
> -    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] =
> -        ASPEED_SDHCI_INFO_SLOT1 | ASPEED_SDHCI_INFO_SLOT0;
> +    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] = slots;
>      sdhci->regs[TO_REG(ASPEED_SDHCI_DEBOUNCE)] = ASPEED_SDHCI_DEBOUNCE_RESET;
>  }
> 

[PATCH for-5.2 09/19] ftgmac100: Fix registers that can be read

[Cédric Le Goater]

Receive Ring Base Address Register (RXR_BADR) and the Normal Priority
Transmit Receive Ring Base Address Register (NPTXR_BADR) can als be
read.

Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/net/ftgmac100.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 5f4b26fc5f3c..0348fcf45676 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -669,6 +669,10 @@ static uint64_t ftgmac100_read(void *opaque, hwaddr addr, unsigned size)
         return s->math[0];
     case FTGMAC100_MATH1:
         return s->math[1];
+    case FTGMAC100_RXR_BADR:
+        return s->rx_ring;
+    case FTGMAC100_NPTXR_BADR:
+        return s->tx_ring;
     case FTGMAC100_ITC:
         return s->itc;
     case FTGMAC100_DBLAC:
-- 
2.25.4

Re: [PATCH for-5.2 09/19] ftgmac100: Fix registers that can be read

[Philippe Mathieu-Daudé]

On 8/6/20 3:20 PM, Cédric Le Goater wrote:
> Receive Ring Base Address Register (RXR_BADR) and the Normal Priority
> Transmit Receive Ring Base Address Register (NPTXR_BADR) can als be

typo "also"

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> read.
> 
> Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/net/ftgmac100.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 5f4b26fc5f3c..0348fcf45676 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -669,6 +669,10 @@ static uint64_t ftgmac100_read(void *opaque, hwaddr addr, unsigned size)
>          return s->math[0];
>      case FTGMAC100_MATH1:
>          return s->math[1];
> +    case FTGMAC100_RXR_BADR:
> +        return s->rx_ring;
> +    case FTGMAC100_NPTXR_BADR:
> +        return s->tx_ring;
>      case FTGMAC100_ITC:
>          return s->itc;
>      case FTGMAC100_DBLAC:
> 

Re: [PATCH for-5.2 09/19] ftgmac100: Fix registers that can be read

[Joel Stanley]

On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>
> Receive Ring Base Address Register (RXR_BADR) and the Normal Priority
> Transmit Receive Ring Base Address Register (NPTXR_BADR) can als be
> read.
>
> Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  hw/net/ftgmac100.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 5f4b26fc5f3c..0348fcf45676 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -669,6 +669,10 @@ static uint64_t ftgmac100_read(void *opaque, hwaddr addr, unsigned size)
>          return s->math[0];
>      case FTGMAC100_MATH1:
>          return s->math[1];
> +    case FTGMAC100_RXR_BADR:
> +        return s->rx_ring;
> +    case FTGMAC100_NPTXR_BADR:
> +        return s->tx_ring;
>      case FTGMAC100_ITC:
>          return s->itc;
>      case FTGMAC100_DBLAC:
> --
> 2.25.4
>

[PATCH for-5.2 10/19] ftgmac100: Fix interrupt status "Packet transmitted on ethernet"

[Cédric Le Goater]

The second field of the TX descriptor has a set of flags to choose
when the transmit interrupt is raised : after the packet has been sent
on the ethernet or after it has been moved into the TX FIFO. But we
don't model that today.

Simply raise the "Packet transmitted on ethernet" the interrupt status
bit as soon as the packet is sent by QEMU.

Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/net/ftgmac100.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 0348fcf45676..aa3c05ef9882 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -547,9 +547,7 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
             qemu_send_packet(qemu_get_queue(s->nic), s->frame, frame_size);
             ptr = s->frame;
             frame_size = 0;
-            if (flags & FTGMAC100_TXDES1_TXIC) {
-                s->isr |= FTGMAC100_INT_XPKT_ETH;
-            }
+            s->isr |= FTGMAC100_INT_XPKT_ETH;
         }
 
         if (flags & FTGMAC100_TXDES1_TX2FIC) {
-- 
2.25.4

Re: [PATCH for-5.2 10/19] ftgmac100: Fix interrupt status "Packet transmitted on ethernet"

[Joel Stanley]

On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>
> The second field of the TX descriptor has a set of flags to choose
> when the transmit interrupt is raised : after the packet has been sent
> on the ethernet or after it has been moved into the TX FIFO. But we
> don't model that today.

Does any software depend on this behaviour? Perhaps mention it in the
commit message so we remember why we changed it.

>
> Simply raise the "Packet transmitted on ethernet" the interrupt status
> bit as soon as the packet is sent by QEMU.

delete the second 'the'?

Reviewed-by: Joel Stanley <joel@jms.id.au>

>
> Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/net/ftgmac100.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 0348fcf45676..aa3c05ef9882 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -547,9 +547,7 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
>              qemu_send_packet(qemu_get_queue(s->nic), s->frame, frame_size);
>              ptr = s->frame;
>              frame_size = 0;
> -            if (flags & FTGMAC100_TXDES1_TXIC) {
> -                s->isr |= FTGMAC100_INT_XPKT_ETH;
> -            }
> +            s->isr |= FTGMAC100_INT_XPKT_ETH;
>          }
>
>          if (flags & FTGMAC100_TXDES1_TX2FIC) {
> --
> 2.25.4
>

Re: [PATCH for-5.2 10/19] ftgmac100: Fix interrupt status "Packet transmitted on ethernet"

[Cédric Le Goater]

On 8/7/20 1:47 AM, Joel Stanley wrote:
> On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> The second field of the TX descriptor has a set of flags to choose
>> when the transmit interrupt is raised : after the packet has been sent
>> on the ethernet or after it has been moved into the TX FIFO. But we
>> don't model that today.
> 
> Does any software depend on this behaviour? 

No. I compared with HW with extra logging.

> Perhaps mention it in the
> commit message so we remember why we changed it.

OK.
 
>> Simply raise the "Packet transmitted on ethernet" the interrupt status
>> bit as soon as the packet is sent by QEMU.
> 
> delete the second 'the'?

sure :)

Thanks,

C. 

> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> 
>>
>> Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/net/ftgmac100.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
>> index 0348fcf45676..aa3c05ef9882 100644
>> --- a/hw/net/ftgmac100.c
>> +++ b/hw/net/ftgmac100.c
>> @@ -547,9 +547,7 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
>>              qemu_send_packet(qemu_get_queue(s->nic), s->frame, frame_size);
>>              ptr = s->frame;
>>              frame_size = 0;
>> -            if (flags & FTGMAC100_TXDES1_TXIC) {
>> -                s->isr |= FTGMAC100_INT_XPKT_ETH;
>> -            }
>> +            s->isr |= FTGMAC100_INT_XPKT_ETH;
>>          }
>>
>>          if (flags & FTGMAC100_TXDES1_TX2FIC) {
>> --
>> 2.25.4
>>

[PATCH for-5.2 11/19] ftgmac100: Fix interrupt status "Packet moved to RX FIFO"

[Cédric Le Goater]

As we don't model the RX or TX FIFO, raise the "Packet moved to RX
FIFO" interrupt status bit as soon as we are handling a RX packet.

Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/net/ftgmac100.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index aa3c05ef9882..5c0fe2d8cb75 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -950,6 +950,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
         break;
     }
 
+    s->isr |= FTGMAC100_INT_RPKT_FIFO;
     addr = s->rx_descriptor;
     while (size > 0) {
         if (!ftgmac100_can_receive(nc)) {
@@ -1001,8 +1002,6 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
             /* Last buffer in frame.  */
             bd.des0 |= flags | FTGMAC100_RXDES0_LRS;
             s->isr |= FTGMAC100_INT_RPKT_BUF;
-        } else {
-            s->isr |= FTGMAC100_INT_RPKT_FIFO;
         }
         ftgmac100_write_bd(&bd, addr);
         if (bd.des0 & s->rxdes0_edorr) {
-- 
2.25.4

Re: [PATCH for-5.2 11/19] ftgmac100: Fix interrupt status "Packet moved to RX FIFO"

[Joel Stanley]

On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>
> As we don't model the RX or TX FIFO, raise the "Packet moved to RX
> FIFO" interrupt status bit as soon as we are handling a RX packet.
>
> Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  hw/net/ftgmac100.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index aa3c05ef9882..5c0fe2d8cb75 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -950,6 +950,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
>          break;
>      }
>
> +    s->isr |= FTGMAC100_INT_RPKT_FIFO;
>      addr = s->rx_descriptor;
>      while (size > 0) {
>          if (!ftgmac100_can_receive(nc)) {
> @@ -1001,8 +1002,6 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
>              /* Last buffer in frame.  */
>              bd.des0 |= flags | FTGMAC100_RXDES0_LRS;
>              s->isr |= FTGMAC100_INT_RPKT_BUF;
> -        } else {
> -            s->isr |= FTGMAC100_INT_RPKT_FIFO;
>          }
>          ftgmac100_write_bd(&bd, addr);
>          if (bd.des0 & s->rxdes0_edorr) {
> --
> 2.25.4
>

[PATCH for-5.2 12/19] ftgmac100: Change interrupt status when a DMA error occurs

[Cédric Le Goater]

The model uses today the "No transmit buffer unavailable" interrupt
status which it is not appropriate. According to the Aspeed specs, no
interrupts are raised in that case. An "AHB error" status seems like a
better modeling choice for all implementations since it is covered by
the Linux kernel.

Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/net/ftgmac100.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 5c0fe2d8cb75..014980d30aca 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -517,7 +517,7 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
         if (dma_memory_read(&address_space_memory, bd.des3, ptr, len)) {
             qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to read packet @ 0x%x\n",
                           __func__, bd.des3);
-            s->isr |= FTGMAC100_INT_NO_NPTXBUF;
+            s->isr |= FTGMAC100_INT_AHB_ERR;
             break;
         }
 
-- 
2.25.4

Re: [PATCH for-5.2 12/19] ftgmac100: Change interrupt status when a DMA error occurs

[Joel Stanley]

On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>
> The model uses today the "No transmit buffer unavailable" interrupt
> status which it is not appropriate. According to the Aspeed specs, no
> interrupts are raised in that case. An "AHB error" status seems like a
> better modeling choice for all implementations since it is covered by
> the Linux kernel.

The datasheet calls it this:

 NPTXBUF UNAVA: Normal priority transmit buffer unavailable

Perhaps we should say this:

The model uses today the "Normal priority transmit buffer unavailable"
interrupt status which is not appropriate.

Reviewed-by: Joel Stanley <joel@jms.id.au>

>
> Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/net/ftgmac100.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 5c0fe2d8cb75..014980d30aca 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -517,7 +517,7 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
>          if (dma_memory_read(&address_space_memory, bd.des3, ptr, len)) {
>              qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to read packet @ 0x%x\n",
>                            __func__, bd.des3);
> -            s->isr |= FTGMAC100_INT_NO_NPTXBUF;
> +            s->isr |= FTGMAC100_INT_AHB_ERR;
>              break;
>          }
>
> --
> 2.25.4
>

Re: [PATCH for-5.2 12/19] ftgmac100: Change interrupt status when a DMA error occurs

[Cédric Le Goater]

On 8/7/20 1:51 AM, Joel Stanley wrote:
> On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> The model uses today the "No transmit buffer unavailable" interrupt
>> status which it is not appropriate. According to the Aspeed specs, no
>> interrupts are raised in that case. An "AHB error" status seems like a
>> better modeling choice for all implementations since it is covered by
>> the Linux kernel.
> 
> The datasheet calls it this:
> 
>  NPTXBUF UNAVA: Normal priority transmit buffer unavailable
> 
> Perhaps we should say this:
> 
> The model uses today the "Normal priority transmit buffer unavailable"
> interrupt status which is not appropriate.

done.

> Reviewed-by: Joel Stanley <joel@jms.id.au>

Thanks,

C.
 
>>
>> Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/net/ftgmac100.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
>> index 5c0fe2d8cb75..014980d30aca 100644
>> --- a/hw/net/ftgmac100.c
>> +++ b/hw/net/ftgmac100.c
>> @@ -517,7 +517,7 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
>>          if (dma_memory_read(&address_space_memory, bd.des3, ptr, len)) {
>>              qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to read packet @ 0x%x\n",
>>                            __func__, bd.des3);
>> -            s->isr |= FTGMAC100_INT_NO_NPTXBUF;
>> +            s->isr |= FTGMAC100_INT_AHB_ERR;
>>              break;
>>          }
>>
>> --
>> 2.25.4
>>

[PATCH for-5.2 13/19] ftgmac100: Check for invalid len and address before doing a DMA transfer

[Cédric Le Goater]

According to the Aspeed specs, no interrupts are raised in that case
but a "Tx-packets lost" status seems like a good modeling choice for
all implementations. It is covered by the Linux kernel.

Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/net/ftgmac100.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 014980d30aca..280aa3d3a1e2 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -507,6 +507,15 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
         }
 
         len = FTGMAC100_TXDES0_TXBUF_SIZE(bd.des0);
+        if (!len) {
+            /*
+             * 0 is an invalid size, however the HW does not raise any
+             * interrupt. Flag an error because the guest is buggy.
+             */
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid segment size\n",
+                          __func__);
+        }
+
         if (frame_size + len > sizeof(s->frame)) {
             qemu_log_mask(LOG_GUEST_ERROR, "%s: frame too big : %d bytes\n",
                           __func__, len);
-- 
2.25.4

Re: [PATCH for-5.2 13/19] ftgmac100: Check for invalid len and address before doing a DMA transfer

[Joel Stanley]

On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>
> According to the Aspeed specs, no interrupts are raised in that case
> but a "Tx-packets lost" status seems like a good modeling choice for
> all implementations. It is covered by the Linux kernel.
>
> Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  hw/net/ftgmac100.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 014980d30aca..280aa3d3a1e2 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -507,6 +507,15 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
>          }
>
>          len = FTGMAC100_TXDES0_TXBUF_SIZE(bd.des0);
> +        if (!len) {
> +            /*
> +             * 0 is an invalid size, however the HW does not raise any
> +             * interrupt. Flag an error because the guest is buggy.
> +             */
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid segment size\n",
> +                          __func__);
> +        }
> +
>          if (frame_size + len > sizeof(s->frame)) {
>              qemu_log_mask(LOG_GUEST_ERROR, "%s: frame too big : %d bytes\n",
>                            __func__, len);
> --
> 2.25.4
>

[PATCH for-5.2 14/19] ftgmac100: Fix integer overflow in ftgmac100_do_tx()

[Cédric Le Goater]

When inserting the VLAN tag in packets, memmove() can generate an
integer overflow for packets whose length is less than 12 bytes.

Check length against the size of the ethernet header (14 bytes) to
avoid the crash and return FTGMAC100_INT_XPKT_LOST status. This seems
like a good modeling choice even if Aspeed does not specify anything
in that case.

Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
Cc: Mauro Matteo Cascella <mcascell@redhat.com>
Reported-by: Ziming Zhang <ezrakiez@gmail.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/net/ftgmac100.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 280aa3d3a1e2..987b843fabc4 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -540,10 +540,21 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
                 s->isr |= FTGMAC100_INT_XPKT_LOST;
                 len =  sizeof(s->frame) - frame_size - 4;
             }
-            memmove(ptr + 16, ptr + 12, len - 12);
-            stw_be_p(ptr + 12, ETH_P_VLAN);
-            stw_be_p(ptr + 14, bd.des1);
-            len += 4;
+
+            if (len < sizeof(struct eth_header)) {
+                qemu_log_mask(LOG_GUEST_ERROR,
+                         "%s: frame too small for VLAN insertion : %d bytes\n",
+                         __func__, len);
+                s->isr |= FTGMAC100_INT_XPKT_LOST;
+            } else {
+                uint8_t *vlan_hdr = ptr + (ETH_ALEN * 2);
+                uint8_t *payload = vlan_hdr + sizeof(struct vlan_header);
+
+                memmove(payload, vlan_hdr, len - (ETH_ALEN * 2));
+                stw_be_p(vlan_hdr, ETH_P_VLAN);
+                stw_be_p(vlan_hdr + 2, FTGMAC100_TXDES1_VLANTAG_CI(bd.des1));
+                len += sizeof(struct vlan_header);
+            }
         }
 
         ptr += len;
-- 
2.25.4

Re: [PATCH for-5.2 14/19] ftgmac100: Fix integer overflow in ftgmac100_do_tx()

[Joel Stanley]

On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>
> When inserting the VLAN tag in packets, memmove() can generate an
> integer overflow for packets whose length is less than 12 bytes.
>
> Check length against the size of the ethernet header (14 bytes) to
> avoid the crash and return FTGMAC100_INT_XPKT_LOST status. This seems
> like a good modeling choice even if Aspeed does not specify anything
> in that case.
>
> Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
> Cc: Mauro Matteo Cascella <mcascell@redhat.com>
> Reported-by: Ziming Zhang <ezrakiez@gmail.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  hw/net/ftgmac100.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 280aa3d3a1e2..987b843fabc4 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -540,10 +540,21 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
>                  s->isr |= FTGMAC100_INT_XPKT_LOST;
>                  len =  sizeof(s->frame) - frame_size - 4;
>              }
> -            memmove(ptr + 16, ptr + 12, len - 12);
> -            stw_be_p(ptr + 12, ETH_P_VLAN);
> -            stw_be_p(ptr + 14, bd.des1);
> -            len += 4;
> +
> +            if (len < sizeof(struct eth_header)) {
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                         "%s: frame too small for VLAN insertion : %d bytes\n",
> +                         __func__, len);
> +                s->isr |= FTGMAC100_INT_XPKT_LOST;
> +            } else {
> +                uint8_t *vlan_hdr = ptr + (ETH_ALEN * 2);
> +                uint8_t *payload = vlan_hdr + sizeof(struct vlan_header);
> +
> +                memmove(payload, vlan_hdr, len - (ETH_ALEN * 2));
> +                stw_be_p(vlan_hdr, ETH_P_VLAN);
> +                stw_be_p(vlan_hdr + 2, FTGMAC100_TXDES1_VLANTAG_CI(bd.des1));
> +                len += sizeof(struct vlan_header);
> +            }
>          }
>
>          ptr += len;
> --
> 2.25.4
>

Re: [PATCH for-5.2 14/19] ftgmac100: Fix integer overflow in ftgmac100_do_tx()

[Mauro Matteo Cascella]

On Thu, Aug 6, 2020 at 3:21 PM Cédric Le Goater <clg@kaod.org> wrote:
>
> When inserting the VLAN tag in packets, memmove() can generate an
> integer overflow for packets whose length is less than 12 bytes.
>
> Check length against the size of the ethernet header (14 bytes) to
> avoid the crash and return FTGMAC100_INT_XPKT_LOST status. This seems
> like a good modeling choice even if Aspeed does not specify anything
> in that case.
>
> Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
> Cc: Mauro Matteo Cascella <mcascell@redhat.com>
> Reported-by: Ziming Zhang <ezrakiez@gmail.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/net/ftgmac100.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 280aa3d3a1e2..987b843fabc4 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -540,10 +540,21 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
>                  s->isr |= FTGMAC100_INT_XPKT_LOST;
>                  len =  sizeof(s->frame) - frame_size - 4;
>              }
> -            memmove(ptr + 16, ptr + 12, len - 12);
> -            stw_be_p(ptr + 12, ETH_P_VLAN);
> -            stw_be_p(ptr + 14, bd.des1);
> -            len += 4;
> +
> +            if (len < sizeof(struct eth_header)) {
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                         "%s: frame too small for VLAN insertion : %d bytes\n",
> +                         __func__, len);
> +                s->isr |= FTGMAC100_INT_XPKT_LOST;
> +            } else {
> +                uint8_t *vlan_hdr = ptr + (ETH_ALEN * 2);
> +                uint8_t *payload = vlan_hdr + sizeof(struct vlan_header);
> +
> +                memmove(payload, vlan_hdr, len - (ETH_ALEN * 2));
> +                stw_be_p(vlan_hdr, ETH_P_VLAN);
> +                stw_be_p(vlan_hdr + 2, FTGMAC100_TXDES1_VLANTAG_CI(bd.des1));
> +                len += sizeof(struct vlan_header);
> +            }
>          }
>
>          ptr += len;
> --
> 2.25.4
>

I can confirm that I can't reproduce the issue with the above patch. Thank you.

-- 
Mauro Matteo Cascella, Red Hat Product Security
6F78 E20B 5935 928C F0A8  1A9D 4E55 23B8 BB34 10B0

Re: [PATCH for-5.2 14/19] ftgmac100: Fix integer overflow in ftgmac100_do_tx()

[Cédric Le Goater]

On 8/10/20 3:43 PM, Mauro Matteo Cascella wrote:
> On Thu, Aug 6, 2020 at 3:21 PM Cédric Le Goater <clg@kaod.org> wrote:
>>
>> When inserting the VLAN tag in packets, memmove() can generate an
>> integer overflow for packets whose length is less than 12 bytes.
>>
>> Check length against the size of the ethernet header (14 bytes) to
>> avoid the crash and return FTGMAC100_INT_XPKT_LOST status. This seems
>> like a good modeling choice even if Aspeed does not specify anything
>> in that case.
>>
>> Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
>> Cc: Mauro Matteo Cascella <mcascell@redhat.com>
>> Reported-by: Ziming Zhang <ezrakiez@gmail.com>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/net/ftgmac100.c | 19 +++++++++++++++----
>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
>> index 280aa3d3a1e2..987b843fabc4 100644
>> --- a/hw/net/ftgmac100.c
>> +++ b/hw/net/ftgmac100.c
>> @@ -540,10 +540,21 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
>>                  s->isr |= FTGMAC100_INT_XPKT_LOST;
>>                  len =  sizeof(s->frame) - frame_size - 4;
>>              }
>> -            memmove(ptr + 16, ptr + 12, len - 12);
>> -            stw_be_p(ptr + 12, ETH_P_VLAN);
>> -            stw_be_p(ptr + 14, bd.des1);
>> -            len += 4;
>> +
>> +            if (len < sizeof(struct eth_header)) {
>> +                qemu_log_mask(LOG_GUEST_ERROR,
>> +                         "%s: frame too small for VLAN insertion : %d bytes\n",
>> +                         __func__, len);
>> +                s->isr |= FTGMAC100_INT_XPKT_LOST;
>> +            } else {
>> +                uint8_t *vlan_hdr = ptr + (ETH_ALEN * 2);
>> +                uint8_t *payload = vlan_hdr + sizeof(struct vlan_header);
>> +
>> +                memmove(payload, vlan_hdr, len - (ETH_ALEN * 2));
>> +                stw_be_p(vlan_hdr, ETH_P_VLAN);
>> +                stw_be_p(vlan_hdr + 2, FTGMAC100_TXDES1_VLANTAG_CI(bd.des1));
>> +                len += sizeof(struct vlan_header);
>> +            }
>>          }
>>
>>          ptr += len;
>> --
>> 2.25.4
>>
> 
> I can confirm that I can't reproduce the issue with the above patch. Thank you.
> 

Can I count that as a Tested-by ? 

Thanks,

C. 

Re: [PATCH for-5.2 14/19] ftgmac100: Fix integer overflow in ftgmac100_do_tx()

[Mauro Matteo Cascella]

On Mon, Aug 10, 2020 at 7:14 PM Cédric Le Goater <clg@kaod.org> wrote:
>
> On 8/10/20 3:43 PM, Mauro Matteo Cascella wrote:
> > On Thu, Aug 6, 2020 at 3:21 PM Cédric Le Goater <clg@kaod.org> wrote:
> >>
> >> When inserting the VLAN tag in packets, memmove() can generate an
> >> integer overflow for packets whose length is less than 12 bytes.
> >>
> >> Check length against the size of the ethernet header (14 bytes) to
> >> avoid the crash and return FTGMAC100_INT_XPKT_LOST status. This seems
> >> like a good modeling choice even if Aspeed does not specify anything
> >> in that case.
> >>
> >> Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
> >> Cc: Mauro Matteo Cascella <mcascell@redhat.com>
> >> Reported-by: Ziming Zhang <ezrakiez@gmail.com>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>  hw/net/ftgmac100.c | 19 +++++++++++++++----
> >>  1 file changed, 15 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> >> index 280aa3d3a1e2..987b843fabc4 100644
> >> --- a/hw/net/ftgmac100.c
> >> +++ b/hw/net/ftgmac100.c
> >> @@ -540,10 +540,21 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
> >>                  s->isr |= FTGMAC100_INT_XPKT_LOST;
> >>                  len =  sizeof(s->frame) - frame_size - 4;
> >>              }
> >> -            memmove(ptr + 16, ptr + 12, len - 12);
> >> -            stw_be_p(ptr + 12, ETH_P_VLAN);
> >> -            stw_be_p(ptr + 14, bd.des1);
> >> -            len += 4;
> >> +
> >> +            if (len < sizeof(struct eth_header)) {
> >> +                qemu_log_mask(LOG_GUEST_ERROR,
> >> +                         "%s: frame too small for VLAN insertion : %d bytes\n",
> >> +                         __func__, len);
> >> +                s->isr |= FTGMAC100_INT_XPKT_LOST;
> >> +            } else {
> >> +                uint8_t *vlan_hdr = ptr + (ETH_ALEN * 2);
> >> +                uint8_t *payload = vlan_hdr + sizeof(struct vlan_header);
> >> +
> >> +                memmove(payload, vlan_hdr, len - (ETH_ALEN * 2));
> >> +                stw_be_p(vlan_hdr, ETH_P_VLAN);
> >> +                stw_be_p(vlan_hdr + 2, FTGMAC100_TXDES1_VLANTAG_CI(bd.des1));
> >> +                len += sizeof(struct vlan_header);
> >> +            }
> >>          }
> >>
> >>          ptr += len;
> >> --
> >> 2.25.4
> >>
> >
> > I can confirm that I can't reproduce the issue with the above patch. Thank you.
> >
>
> Can I count that as a Tested-by ?
>
> Thanks,
>
> C.
>
>

Sure. I wonder whether we should make 'len' unsigned, though I think
it doesn't really matter due to FTGMAC100_TXDES0_TXBUF_SIZE. What do
you think?

Tested-by: Mauro Matteo Cascella <mcascell@redhat.com>


--
Mauro Matteo Cascella, Red Hat Product Security
6F78 E20B 5935 928C F0A8  1A9D 4E55 23B8 BB34 10B0

Re: [PATCH for-5.2 14/19] ftgmac100: Fix integer overflow in ftgmac100_do_tx()

[Peter Maydell]

On Thu, 6 Aug 2020 at 14:21, Cédric Le Goater <clg@kaod.org> wrote:
>
> When inserting the VLAN tag in packets, memmove() can generate an
> integer overflow for packets whose length is less than 12 bytes.
>
> Check length against the size of the ethernet header (14 bytes) to
> avoid the crash and return FTGMAC100_INT_XPKT_LOST status. This seems
> like a good modeling choice even if Aspeed does not specify anything
> in that case.
>
> Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
> Cc: Mauro Matteo Cascella <mcascell@redhat.com>
> Reported-by: Ziming Zhang <ezrakiez@gmail.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/net/ftgmac100.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 280aa3d3a1e2..987b843fabc4 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -540,10 +540,21 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
>                  s->isr |= FTGMAC100_INT_XPKT_LOST;
>                  len =  sizeof(s->frame) - frame_size - 4;
>              }
> -            memmove(ptr + 16, ptr + 12, len - 12);
> -            stw_be_p(ptr + 12, ETH_P_VLAN);
> -            stw_be_p(ptr + 14, bd.des1);
> -            len += 4;
> +
> +            if (len < sizeof(struct eth_header)) {
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                         "%s: frame too small for VLAN insertion : %d bytes\n",
> +                         __func__, len);
> +                s->isr |= FTGMAC100_INT_XPKT_LOST;
> +            } else {
> +                uint8_t *vlan_hdr = ptr + (ETH_ALEN * 2);
> +                uint8_t *payload = vlan_hdr + sizeof(struct vlan_header);
> +
> +                memmove(payload, vlan_hdr, len - (ETH_ALEN * 2));
> +                stw_be_p(vlan_hdr, ETH_P_VLAN);
> +                stw_be_p(vlan_hdr + 2, FTGMAC100_TXDES1_VLANTAG_CI(bd.des1));
> +                len += sizeof(struct vlan_header);
> +            }
>          }

If you want to be picky, this will unnecessarily fail for the case of
a packet that is big enough for the vlan header but which has been
split up into multiple tx descriptors such that the first one is
smaller than the size of the eth_header. You could fix that by
doing the insertion of the vlan tag when you process the TXDES0_LTS
descriptor rather than when you process the TXDES0_FTS one. (We
already save the des1 info where the INS_VLANTAG flag is in the
'flags' variable, so we have that available for the LTS descriptor code.)

thanks
-- PMM

Re: [PATCH for-5.2 14/19] ftgmac100: Fix integer overflow in ftgmac100_do_tx()

[Cédric Le Goater]

On 8/11/20 2:39 PM, Peter Maydell wrote:
> On Thu, 6 Aug 2020 at 14:21, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> When inserting the VLAN tag in packets, memmove() can generate an
>> integer overflow for packets whose length is less than 12 bytes.
>>
>> Check length against the size of the ethernet header (14 bytes) to
>> avoid the crash and return FTGMAC100_INT_XPKT_LOST status. This seems
>> like a good modeling choice even if Aspeed does not specify anything
>> in that case.
>>
>> Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
>> Cc: Mauro Matteo Cascella <mcascell@redhat.com>
>> Reported-by: Ziming Zhang <ezrakiez@gmail.com>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/net/ftgmac100.c | 19 +++++++++++++++----
>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
>> index 280aa3d3a1e2..987b843fabc4 100644
>> --- a/hw/net/ftgmac100.c
>> +++ b/hw/net/ftgmac100.c
>> @@ -540,10 +540,21 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
>>                  s->isr |= FTGMAC100_INT_XPKT_LOST;
>>                  len =  sizeof(s->frame) - frame_size - 4;
>>              }
>> -            memmove(ptr + 16, ptr + 12, len - 12);
>> -            stw_be_p(ptr + 12, ETH_P_VLAN);
>> -            stw_be_p(ptr + 14, bd.des1);
>> -            len += 4;
>> +
>> +            if (len < sizeof(struct eth_header)) {
>> +                qemu_log_mask(LOG_GUEST_ERROR,
>> +                         "%s: frame too small for VLAN insertion : %d bytes\n",
>> +                         __func__, len);
>> +                s->isr |= FTGMAC100_INT_XPKT_LOST;
>> +            } else {
>> +                uint8_t *vlan_hdr = ptr + (ETH_ALEN * 2);
>> +                uint8_t *payload = vlan_hdr + sizeof(struct vlan_header);
>> +
>> +                memmove(payload, vlan_hdr, len - (ETH_ALEN * 2));
>> +                stw_be_p(vlan_hdr, ETH_P_VLAN);
>> +                stw_be_p(vlan_hdr + 2, FTGMAC100_TXDES1_VLANTAG_CI(bd.des1));
>> +                len += sizeof(struct vlan_header);
>> +            }
>>          }
> 
> If you want to be picky, this will unnecessarily fail for the case of
> a packet that is big enough for the vlan header but which has been
> split up into multiple tx descriptors such that the first one is
> smaller than the size of the eth_header. You could fix that by
> doing the insertion of the vlan tag when you process the TXDES0_LTS
> descriptor rather than when you process the TXDES0_FTS one. (We
> already save the des1 info where the INS_VLANTAG flag is in the
> 'flags' variable, so we have that available for the LTS descriptor code.)

yes. Good idea. The code is cleaner and the driver can even survive 
a bogus frame.

I will send a new version, without the Tested and Reviewed tags.

To reproduce, I have created a little kernel module tester based 
on the POC proposed by Ziming, which was for another MAC.

	https://github.com/legoater/ftgmac100-test

Thanks,

C. 
 

[PATCH for-5.2 15/19] ftgmac100: Improve software reset

[Cédric Le Goater]

The software reset of the MAC needs a finer granularity. Not all
registers are reseted and some setting in MACCR are kept.

Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
Fixes: bd44300d1afc ("net: add FTGMAC100 support")
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/net/ftgmac100.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 987b843fabc4..0740049c5268 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -655,11 +655,10 @@ static void ftgmac100_reset(DeviceState *d)
     s->itc = 0;
     s->aptcr = 1;
     s->dblac = 0x00022f00;
-    s->revr = 0;
     s->fear1 = 0;
     s->tpafcr = 0xf1;
 
-    s->maccr = 0;
+    s->maccr &= FTGMAC100_MACCR_GIGA_MODE | FTGMAC100_MACCR_FAST_MODE;
     s->phycr = 0;
     s->phydata = 0;
     s->fcr = 0x400;
@@ -812,6 +811,7 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
     case FTGMAC100_MACCR: /* MAC Device control */
         s->maccr = value;
         if (value & FTGMAC100_MACCR_SW_RST) {
+            /* TODO: rework software reset to have a finer granularity */
             ftgmac100_reset(DEVICE(s));
         }
 
-- 
2.25.4

Re: [PATCH for-5.2 15/19] ftgmac100: Improve software reset

[Philippe Mathieu-Daudé]

On 8/6/20 3:21 PM, Cédric Le Goater wrote:
> The software reset of the MAC needs a finer granularity. Not all
> registers are reseted and some setting in MACCR are kept.

typo in "reseted".

> 
> Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
> Fixes: bd44300d1afc ("net: add FTGMAC100 support")
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/net/ftgmac100.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 987b843fabc4..0740049c5268 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -655,11 +655,10 @@ static void ftgmac100_reset(DeviceState *d)
>      s->itc = 0;
>      s->aptcr = 1;
>      s->dblac = 0x00022f00;
> -    s->revr = 0;
>      s->fear1 = 0;
>      s->tpafcr = 0xf1;
>  
> -    s->maccr = 0;
> +    s->maccr &= FTGMAC100_MACCR_GIGA_MODE | FTGMAC100_MACCR_FAST_MODE;
>      s->phycr = 0;
>      s->phydata = 0;
>      s->fcr = 0x400;
> @@ -812,6 +811,7 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
>      case FTGMAC100_MACCR: /* MAC Device control */
>          s->maccr = value;
>          if (value & FTGMAC100_MACCR_SW_RST) {
> +            /* TODO: rework software reset to have a finer granularity */
>              ftgmac100_reset(DEVICE(s));
>          }
>  
> 

Re: [PATCH for-5.2 15/19] ftgmac100: Improve software reset

[Joel Stanley]

On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>
> The software reset of the MAC needs a finer granularity. Not all
> registers are reseted and some setting in MACCR are kept.

'settings'

This makes the software reset incorrect, but the power on reset values
correct. Was that your goal?

If so, perhaps put that in the commit message.

>
> Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
> Fixes: bd44300d1afc ("net: add FTGMAC100 support")
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/net/ftgmac100.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 987b843fabc4..0740049c5268 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -655,11 +655,10 @@ static void ftgmac100_reset(DeviceState *d)
>      s->itc = 0;
>      s->aptcr = 1;
>      s->dblac = 0x00022f00;
> -    s->revr = 0;
>      s->fear1 = 0;
>      s->tpafcr = 0xf1;
>
> -    s->maccr = 0;
> +    s->maccr &= FTGMAC100_MACCR_GIGA_MODE | FTGMAC100_MACCR_FAST_MODE;
>      s->phycr = 0;
>      s->phydata = 0;
>      s->fcr = 0x400;
> @@ -812,6 +811,7 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
>      case FTGMAC100_MACCR: /* MAC Device control */
>          s->maccr = value;
>          if (value & FTGMAC100_MACCR_SW_RST) {
> +            /* TODO: rework software reset to have a finer granularity */
>              ftgmac100_reset(DEVICE(s));
>          }
>
> --
> 2.25.4
>

Re: [PATCH for-5.2 15/19] ftgmac100: Improve software reset

[Cédric Le Goater]

On 8/7/20 2:03 AM, Joel Stanley wrote:
> On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> The software reset of the MAC needs a finer granularity. Not all
>> registers are reseted and some setting in MACCR are kept.
> 
> 'settings'
> 
> This makes the software reset incorrect, but the power on reset values
> correct. Was that your goal?

You are right. I should address the TODO below.

Thanks,

C. 

> 
> If so, perhaps put that in the commit message.
> 
>>
>> Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
>> Fixes: bd44300d1afc ("net: add FTGMAC100 support")
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/net/ftgmac100.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
>> index 987b843fabc4..0740049c5268 100644
>> --- a/hw/net/ftgmac100.c
>> +++ b/hw/net/ftgmac100.c
>> @@ -655,11 +655,10 @@ static void ftgmac100_reset(DeviceState *d)
>>      s->itc = 0;
>>      s->aptcr = 1;
>>      s->dblac = 0x00022f00;
>> -    s->revr = 0;
>>      s->fear1 = 0;
>>      s->tpafcr = 0xf1;
>>
>> -    s->maccr = 0;
>> +    s->maccr &= FTGMAC100_MACCR_GIGA_MODE | FTGMAC100_MACCR_FAST_MODE;
>>      s->phycr = 0;
>>      s->phydata = 0;
>>      s->fcr = 0x400;
>> @@ -812,6 +811,7 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
>>      case FTGMAC100_MACCR: /* MAC Device control */
>>          s->maccr = value;
>>          if (value & FTGMAC100_MACCR_SW_RST) {
>> +            /* TODO: rework software reset to have a finer granularity */
>>              ftgmac100_reset(DEVICE(s));
>>          }
>>
>> --
>> 2.25.4
>>

[PATCH for-5.2 16/19] aspeed/sdmc: Perform memory training

[Cédric Le Goater]

From: Joel Stanley <joel@jms.id.au>

This allows qemu to run the "normal" power on reset boot path through
u-boot, where the DDR is trained.

An enhancement would be to have the SCU bit stick across qemu reboots,
but be unset on initial boot.

Proper modelling would be to discard all writes to the phy setting regs
at offset 0x100 - 0x400 and to model the phy status regs at offset
0x400.

The status regs model would only need to account for offets 0x00,
0x50, 0x68 and 0x7c.

Signed-off-by: Joel Stanley <joel@jms.id.au>
[ clg: checkpatch fixes ]
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/misc/aspeed_sdmc.h | 13 ++++++++++++-
 hw/misc/aspeed_scu.c          |  2 +-
 hw/misc/aspeed_sdmc.c         | 19 +++++++++++++++++--
 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
index cea1e67fe365..c6226957dd3d 100644
--- a/include/hw/misc/aspeed_sdmc.h
+++ b/include/hw/misc/aspeed_sdmc.h
@@ -17,7 +17,18 @@
 #define TYPE_ASPEED_2500_SDMC TYPE_ASPEED_SDMC "-ast2500"
 #define TYPE_ASPEED_2600_SDMC TYPE_ASPEED_SDMC "-ast2600"
 
-#define ASPEED_SDMC_NR_REGS (0x174 >> 2)
+/*
+ * SDMC has 174 documented registers. In addition the u-boot device tree
+ * describes the following regions:
+ *  - PHY status regs at offset 0x400, length 0x200
+ *  - PHY setting regs at offset 0x100, length 0x300
+ *
+ * There are two sets of MRS (Mode Registers) configuration in ast2600 memory
+ * system: one is in the SDRAM MC (memory controller) which is used in run
+ * time, and the other is in the DDR-PHY IP which is used during DDR-PHY
+ * training.
+ */
+#define ASPEED_SDMC_NR_REGS (0x500 >> 2)
 
 typedef struct AspeedSDMCState {
     /*< private >*/
diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index 764222404bef..dc6dd87c22f4 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -656,7 +656,7 @@ static const uint32_t ast2600_a1_resets[ASPEED_AST2600_SCU_NR_REGS] = {
     [AST2600_SYS_RST_CTRL2]     = 0xFFFFFFFC,
     [AST2600_CLK_STOP_CTRL]     = 0xFFFF7F8A,
     [AST2600_CLK_STOP_CTRL2]    = 0xFFF0FFF0,
-    [AST2600_SDRAM_HANDSHAKE]   = 0x00000040,  /* SoC completed DRAM init */
+    [AST2600_SDRAM_HANDSHAKE]   = 0x00000000,
     [AST2600_HPLL_PARAM]        = 0x1000405F,
     [AST2600_CHIP_ID0]          = 0x1234ABCD,
     [AST2600_CHIP_ID1]          = 0x88884444,
diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index 855848b7d23a..ff2809a09965 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -113,7 +113,7 @@ static uint64_t aspeed_sdmc_read(void *opaque, hwaddr addr, unsigned size)
     if (addr >= ARRAY_SIZE(s->regs)) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx "\n",
-                      __func__, addr);
+                      __func__, addr * 4);
         return 0;
     }
 
@@ -206,6 +206,19 @@ static void aspeed_sdmc_reset(DeviceState *dev)
 
     /* Set ram size bit and defaults values */
     s->regs[R_CONF] = asc->compute_conf(s, 0);
+
+    /*
+     * PHY status:
+     *  - set phy status ok (set bit 1)
+     *  - initial PVT calibration ok (clear bit 3)
+     *  - runtime calibration ok (clear bit 5)
+     */
+    s->regs[0x100] = BIT(1);
+
+    /* PHY eye window: set all as passing */
+    s->regs[0x100 | (0x68 / 4)] = 0xff;
+    s->regs[0x100 | (0x7c / 4)] = 0xff;
+    s->regs[0x100 | (0x50 / 4)] = 0xfffffff;
 }
 
 static void aspeed_sdmc_get_ram_size(Object *obj, Visitor *v, const char *name,
@@ -443,7 +456,9 @@ static void aspeed_2600_sdmc_write(AspeedSDMCState *s, uint32_t reg,
     }
 
     if (reg != R_PROT && s->regs[R_PROT] == PROT_SOFTLOCKED) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: SDMC is locked!\n", __func__);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: SDMC is locked! (write to MCR%02x blocked)\n",
+                      __func__, reg * 4);
         return;
     }
 
-- 
2.25.4

Re: [PATCH for-5.2 16/19] aspeed/sdmc: Perform memory training

[Philippe Mathieu-Daudé]

On 8/6/20 3:21 PM, Cédric Le Goater wrote:
> From: Joel Stanley <joel@jms.id.au>
> 
> This allows qemu to run the "normal" power on reset boot path through
> u-boot, where the DDR is trained.
> 
> An enhancement would be to have the SCU bit stick across qemu reboots,
> but be unset on initial boot.
> 
> Proper modelling would be to discard all writes to the phy setting regs
> at offset 0x100 - 0x400 and to model the phy status regs at offset
> 0x400.
> 
> The status regs model would only need to account for offets 0x00,
> 0x50, 0x68 and 0x7c.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> [ clg: checkpatch fixes ]
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/misc/aspeed_sdmc.h | 13 ++++++++++++-
>  hw/misc/aspeed_scu.c          |  2 +-
>  hw/misc/aspeed_sdmc.c         | 19 +++++++++++++++++--
>  3 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
> index cea1e67fe365..c6226957dd3d 100644
> --- a/include/hw/misc/aspeed_sdmc.h
> +++ b/include/hw/misc/aspeed_sdmc.h
> @@ -17,7 +17,18 @@
>  #define TYPE_ASPEED_2500_SDMC TYPE_ASPEED_SDMC "-ast2500"
>  #define TYPE_ASPEED_2600_SDMC TYPE_ASPEED_SDMC "-ast2600"
>  
> -#define ASPEED_SDMC_NR_REGS (0x174 >> 2)
> +/*
> + * SDMC has 174 documented registers. In addition the u-boot device tree
> + * describes the following regions:
> + *  - PHY status regs at offset 0x400, length 0x200
> + *  - PHY setting regs at offset 0x100, length 0x300
> + *
> + * There are two sets of MRS (Mode Registers) configuration in ast2600 memory
> + * system: one is in the SDRAM MC (memory controller) which is used in run
> + * time, and the other is in the DDR-PHY IP which is used during DDR-PHY
> + * training.
> + */
> +#define ASPEED_SDMC_NR_REGS (0x500 >> 2)
>  
>  typedef struct AspeedSDMCState {
>      /*< private >*/
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index 764222404bef..dc6dd87c22f4 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -656,7 +656,7 @@ static const uint32_t ast2600_a1_resets[ASPEED_AST2600_SCU_NR_REGS] = {
>      [AST2600_SYS_RST_CTRL2]     = 0xFFFFFFFC,
>      [AST2600_CLK_STOP_CTRL]     = 0xFFFF7F8A,
>      [AST2600_CLK_STOP_CTRL2]    = 0xFFF0FFF0,
> -    [AST2600_SDRAM_HANDSHAKE]   = 0x00000040,  /* SoC completed DRAM init */
> +    [AST2600_SDRAM_HANDSHAKE]   = 0x00000000,
>      [AST2600_HPLL_PARAM]        = 0x1000405F,
>      [AST2600_CHIP_ID0]          = 0x1234ABCD,
>      [AST2600_CHIP_ID1]          = 0x88884444,
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index 855848b7d23a..ff2809a09965 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -113,7 +113,7 @@ static uint64_t aspeed_sdmc_read(void *opaque, hwaddr addr, unsigned size)
>      if (addr >= ARRAY_SIZE(s->regs)) {
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx "\n",
> -                      __func__, addr);
> +                      __func__, addr * 4);
>          return 0;
>      }
>  
> @@ -206,6 +206,19 @@ static void aspeed_sdmc_reset(DeviceState *dev)
>  
>      /* Set ram size bit and defaults values */
>      s->regs[R_CONF] = asc->compute_conf(s, 0);
> +
> +    /*
> +     * PHY status:
> +     *  - set phy status ok (set bit 1)
> +     *  - initial PVT calibration ok (clear bit 3)
> +     *  - runtime calibration ok (clear bit 5)
> +     */
> +    s->regs[0x100] = BIT(1);

This is usually implemented with a one-shot timer, see
sd_ocr_powerup() in hw/sd/sd.c (migration is handled).

> +
> +    /* PHY eye window: set all as passing */
> +    s->regs[0x100 | (0x68 / 4)] = 0xff;
> +    s->regs[0x100 | (0x7c / 4)] = 0xff;
> +    s->regs[0x100 | (0x50 / 4)] = 0xfffffff;
>  }
>  
>  static void aspeed_sdmc_get_ram_size(Object *obj, Visitor *v, const char *name,
> @@ -443,7 +456,9 @@ static void aspeed_2600_sdmc_write(AspeedSDMCState *s, uint32_t reg,
>      }
>  
>      if (reg != R_PROT && s->regs[R_PROT] == PROT_SOFTLOCKED) {
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: SDMC is locked!\n", __func__);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: SDMC is locked! (write to MCR%02x blocked)\n",
> +                      __func__, reg * 4);
>          return;
>      }
>  
> 

Re: [PATCH for-5.2 16/19] aspeed/sdmc: Perform memory training

[Joel Stanley]

On Thu, 6 Aug 2020 at 13:38, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> > @@ -206,6 +206,19 @@ static void aspeed_sdmc_reset(DeviceState *dev)
> >
> >      /* Set ram size bit and defaults values */
> >      s->regs[R_CONF] = asc->compute_conf(s, 0);
> > +
> > +    /*
> > +     * PHY status:
> > +     *  - set phy status ok (set bit 1)
> > +     *  - initial PVT calibration ok (clear bit 3)
> > +     *  - runtime calibration ok (clear bit 5)
> > +     */
> > +    s->regs[0x100] = BIT(1);
>
> This is usually implemented with a one-shot timer, see
> sd_ocr_powerup() in hw/sd/sd.c (migration is handled).

You mean we could have the calibration done bits become set at a later time?

It would be hard to work out what to do. We have no documentation for
the register, I modelled it based on the code in u-boot doing this:

         /* make sure DDR-PHY is ready before access */
        do {
                reg = readl(priv->phy_status) & BIT(1);
        } while(reg == 0);

So I think there would be limited value in modelling it.

Thanks for the suggestion though, I'll keep the one shot timer idea in
mind for future models.

Cheers,

Joel

[PATCH for-5.2 17/19] aspeed/sdmc: Allow writes to unprotected registers

[Cédric Le Goater]

From: Joel Stanley <joel@jms.id.au>

A subset of registers are not protected by the lock behaviour, so allow
unconditionally writing to those.

Signed-off-by: Joel Stanley <joel@jms.id.au>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/misc/aspeed_sdmc.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index ff2809a09965..81c73450ab5d 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -33,15 +33,28 @@
 /* Configuration Register */
 #define R_CONF            (0x04 / 4)
 
+/* Interrupt control/status */
+#define R_ISR             (0x50 / 4)
+
 /* Control/Status Register #1 (ast2500) */
 #define R_STATUS1         (0x60 / 4)
 #define   PHY_BUSY_STATE      BIT(0)
 #define   PHY_PLL_LOCK_STATUS BIT(4)
 
+/* Reserved */
+#define R_MCR6C           (0x6c / 4)
+
 #define R_ECC_TEST_CTRL   (0x70 / 4)
 #define   ECC_TEST_FINISHED   BIT(12)
 #define   ECC_TEST_FAIL       BIT(13)
 
+#define R_TEST_START_LEN  (0x74 / 4)
+#define R_TEST_FAIL_DQ    (0x78 / 4)
+#define R_TEST_INIT_VAL   (0x7c / 4)
+#define R_DRAM_SW         (0x88 / 4)
+#define R_DRAM_TIME       (0x8c / 4)
+#define R_ECC_ERR_INJECT  (0xb4 / 4)
+
 /*
  * Configuration register Ox4 (for Aspeed AST2400 SOC)
  *
@@ -449,6 +462,20 @@ static uint32_t aspeed_2600_sdmc_compute_conf(AspeedSDMCState *s, uint32_t data)
 static void aspeed_2600_sdmc_write(AspeedSDMCState *s, uint32_t reg,
                                    uint32_t data)
 {
+    /* Unprotected registers */
+    switch (reg) {
+    case R_ISR:
+    case R_MCR6C:
+    case R_TEST_START_LEN:
+    case R_TEST_FAIL_DQ:
+    case R_TEST_INIT_VAL:
+    case R_DRAM_SW:
+    case R_DRAM_TIME:
+    case R_ECC_ERR_INJECT:
+        s->regs[reg] = data;
+        return;
+    }
+
     if (s->regs[R_PROT] == PROT_HARDLOCKED) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: SDMC is locked until system reset!\n",
                 __func__);
-- 
2.25.4

[PATCH for-5.2 18/19] aspeed/sdmc: Simplify calculation of RAM bits

[Cédric Le Goater]

Changes in commit 533eb415df2e ("arm/aspeed: actually check RAM size")
introduced a 'valid_ram_sizes' array which can be used to compute the
associated bit field value encoding the RAM size. The field is simply
the index of the array.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/misc/aspeed_sdmc.c | 79 ++++++++++++++-----------------------------
 1 file changed, 25 insertions(+), 54 deletions(-)

diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index 81c73450ab5d..08f856cbda7e 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -159,57 +159,6 @@ static const MemoryRegionOps aspeed_sdmc_ops = {
     .valid.max_access_size = 4,
 };
 
-static int ast2400_rambits(AspeedSDMCState *s)
-{
-    switch (s->ram_size >> 20) {
-    case 64:
-        return ASPEED_SDMC_DRAM_64MB;
-    case 128:
-        return ASPEED_SDMC_DRAM_128MB;
-    case 256:
-        return ASPEED_SDMC_DRAM_256MB;
-    case 512:
-        return ASPEED_SDMC_DRAM_512MB;
-    default:
-        g_assert_not_reached();
-        break;
-    }
-}
-
-static int ast2500_rambits(AspeedSDMCState *s)
-{
-    switch (s->ram_size >> 20) {
-    case 128:
-        return ASPEED_SDMC_AST2500_128MB;
-    case 256:
-        return ASPEED_SDMC_AST2500_256MB;
-    case 512:
-        return ASPEED_SDMC_AST2500_512MB;
-    case 1024:
-        return ASPEED_SDMC_AST2500_1024MB;
-    default:
-        g_assert_not_reached();
-        break;
-    }
-}
-
-static int ast2600_rambits(AspeedSDMCState *s)
-{
-    switch (s->ram_size >> 20) {
-    case 256:
-        return ASPEED_SDMC_AST2600_256MB;
-    case 512:
-        return ASPEED_SDMC_AST2600_512MB;
-    case 1024:
-        return ASPEED_SDMC_AST2600_1024MB;
-    case 2048:
-        return ASPEED_SDMC_AST2600_2048MB;
-    default:
-        g_assert_not_reached();
-        break;
-    }
-}
-
 static void aspeed_sdmc_reset(DeviceState *dev)
 {
     AspeedSDMCState *s = ASPEED_SDMC(dev);
@@ -324,10 +273,32 @@ static const TypeInfo aspeed_sdmc_info = {
     .abstract   = true,
 };
 
+static int aspeed_sdmc_get_ram_bits(AspeedSDMCState *s)
+{
+    AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s);
+    int i;
+
+    /*
+     * The bitfield value encoding the RAM size is the index of the
+     * possible RAM size array
+     */
+    for (i = 0; asc->valid_ram_sizes[i]; i++) {
+        if (s->ram_size == asc->valid_ram_sizes[i]) {
+            return i;
+        }
+    }
+
+    /*
+     * Invalid RAM sizes should have been excluded when setting the
+     * SoC RAM size.
+     */
+    g_assert_not_reached();
+}
+
 static uint32_t aspeed_2400_sdmc_compute_conf(AspeedSDMCState *s, uint32_t data)
 {
     uint32_t fixed_conf = ASPEED_SDMC_VGA_COMPAT |
-        ASPEED_SDMC_DRAM_SIZE(ast2400_rambits(s));
+        ASPEED_SDMC_DRAM_SIZE(aspeed_sdmc_get_ram_bits(s));
 
     /* Make sure readonly bits are kept */
     data &= ~ASPEED_SDMC_READONLY_MASK;
@@ -385,7 +356,7 @@ static uint32_t aspeed_2500_sdmc_compute_conf(AspeedSDMCState *s, uint32_t data)
     uint32_t fixed_conf = ASPEED_SDMC_HW_VERSION(1) |
         ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
         ASPEED_SDMC_CACHE_INITIAL_DONE |
-        ASPEED_SDMC_DRAM_SIZE(ast2500_rambits(s));
+        ASPEED_SDMC_DRAM_SIZE(aspeed_sdmc_get_ram_bits(s));
 
     /* Make sure readonly bits are kept */
     data &= ~ASPEED_SDMC_AST2500_READONLY_MASK;
@@ -451,7 +422,7 @@ static uint32_t aspeed_2600_sdmc_compute_conf(AspeedSDMCState *s, uint32_t data)
 {
     uint32_t fixed_conf = ASPEED_SDMC_HW_VERSION(3) |
         ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
-        ASPEED_SDMC_DRAM_SIZE(ast2600_rambits(s));
+        ASPEED_SDMC_DRAM_SIZE(aspeed_sdmc_get_ram_bits(s));
 
     /* Make sure readonly bits are kept (use ast2500 mask) */
     data &= ~ASPEED_SDMC_AST2500_READONLY_MASK;
-- 
2.25.4

Re: [PATCH for-5.2 18/19] aspeed/sdmc: Simplify calculation of RAM bits

[Joel Stanley]

On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>
> Changes in commit 533eb415df2e ("arm/aspeed: actually check RAM size")
> introduced a 'valid_ram_sizes' array which can be used to compute the
> associated bit field value encoding the RAM size. The field is simply
> the index of the array.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  hw/misc/aspeed_sdmc.c | 79 ++++++++++++++-----------------------------
>  1 file changed, 25 insertions(+), 54 deletions(-)
>
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index 81c73450ab5d..08f856cbda7e 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -159,57 +159,6 @@ static const MemoryRegionOps aspeed_sdmc_ops = {
>      .valid.max_access_size = 4,
>  };
>
> -static int ast2400_rambits(AspeedSDMCState *s)
> -{
> -    switch (s->ram_size >> 20) {
> -    case 64:
> -        return ASPEED_SDMC_DRAM_64MB;
> -    case 128:
> -        return ASPEED_SDMC_DRAM_128MB;
> -    case 256:
> -        return ASPEED_SDMC_DRAM_256MB;
> -    case 512:
> -        return ASPEED_SDMC_DRAM_512MB;
> -    default:
> -        g_assert_not_reached();
> -        break;
> -    }
> -}
> -
> -static int ast2500_rambits(AspeedSDMCState *s)
> -{
> -    switch (s->ram_size >> 20) {
> -    case 128:
> -        return ASPEED_SDMC_AST2500_128MB;
> -    case 256:
> -        return ASPEED_SDMC_AST2500_256MB;
> -    case 512:
> -        return ASPEED_SDMC_AST2500_512MB;
> -    case 1024:
> -        return ASPEED_SDMC_AST2500_1024MB;
> -    default:
> -        g_assert_not_reached();
> -        break;
> -    }
> -}
> -
> -static int ast2600_rambits(AspeedSDMCState *s)
> -{
> -    switch (s->ram_size >> 20) {
> -    case 256:
> -        return ASPEED_SDMC_AST2600_256MB;
> -    case 512:
> -        return ASPEED_SDMC_AST2600_512MB;
> -    case 1024:
> -        return ASPEED_SDMC_AST2600_1024MB;
> -    case 2048:
> -        return ASPEED_SDMC_AST2600_2048MB;
> -    default:
> -        g_assert_not_reached();
> -        break;
> -    }
> -}
> -
>  static void aspeed_sdmc_reset(DeviceState *dev)
>  {
>      AspeedSDMCState *s = ASPEED_SDMC(dev);
> @@ -324,10 +273,32 @@ static const TypeInfo aspeed_sdmc_info = {
>      .abstract   = true,
>  };
>
> +static int aspeed_sdmc_get_ram_bits(AspeedSDMCState *s)
> +{
> +    AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s);
> +    int i;
> +
> +    /*
> +     * The bitfield value encoding the RAM size is the index of the
> +     * possible RAM size array
> +     */
> +    for (i = 0; asc->valid_ram_sizes[i]; i++) {
> +        if (s->ram_size == asc->valid_ram_sizes[i]) {
> +            return i;
> +        }
> +    }
> +
> +    /*
> +     * Invalid RAM sizes should have been excluded when setting the
> +     * SoC RAM size.
> +     */
> +    g_assert_not_reached();
> +}
> +
>  static uint32_t aspeed_2400_sdmc_compute_conf(AspeedSDMCState *s, uint32_t data)
>  {
>      uint32_t fixed_conf = ASPEED_SDMC_VGA_COMPAT |
> -        ASPEED_SDMC_DRAM_SIZE(ast2400_rambits(s));
> +        ASPEED_SDMC_DRAM_SIZE(aspeed_sdmc_get_ram_bits(s));
>
>      /* Make sure readonly bits are kept */
>      data &= ~ASPEED_SDMC_READONLY_MASK;
> @@ -385,7 +356,7 @@ static uint32_t aspeed_2500_sdmc_compute_conf(AspeedSDMCState *s, uint32_t data)
>      uint32_t fixed_conf = ASPEED_SDMC_HW_VERSION(1) |
>          ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
>          ASPEED_SDMC_CACHE_INITIAL_DONE |
> -        ASPEED_SDMC_DRAM_SIZE(ast2500_rambits(s));
> +        ASPEED_SDMC_DRAM_SIZE(aspeed_sdmc_get_ram_bits(s));
>
>      /* Make sure readonly bits are kept */
>      data &= ~ASPEED_SDMC_AST2500_READONLY_MASK;
> @@ -451,7 +422,7 @@ static uint32_t aspeed_2600_sdmc_compute_conf(AspeedSDMCState *s, uint32_t data)
>  {
>      uint32_t fixed_conf = ASPEED_SDMC_HW_VERSION(3) |
>          ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
> -        ASPEED_SDMC_DRAM_SIZE(ast2600_rambits(s));
> +        ASPEED_SDMC_DRAM_SIZE(aspeed_sdmc_get_ram_bits(s));
>
>      /* Make sure readonly bits are kept (use ast2500 mask) */
>      data &= ~ASPEED_SDMC_AST2500_READONLY_MASK;
> --
> 2.25.4
>

[PATCH for-5.2 19/19] aspeed/smc: Open AHB window of the second chip of the AST2600 FMC controller

[Cédric Le Goater]

This change works around the HW default values to be able to test the
tacoma board with -kernel command line option.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ssi/aspeed_smc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 8c79a5552f93..795784e5f364 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -230,7 +230,7 @@ static void aspeed_smc_reg_to_segment(const AspeedSMCState *s, uint32_t reg,
 
 static const AspeedSegments aspeed_segments_ast2600_fmc[] = {
     { 0x0, 128 * MiB }, /* start address is readonly */
-    { 0x0, 0 }, /* disabled */
+    { 128 * MiB, 128 * MiB }, /* default is disabled but needed for -kernel */
     { 0x0, 0 }, /* disabled */
 };
 
-- 
2.25.4

Re: [PATCH for-5.2 19/19] aspeed/smc: Open AHB window of the second chip of the AST2600 FMC controller

[Joel Stanley]

On Thu, 6 Aug 2020 at 13:21, Cédric Le Goater <clg@kaod.org> wrote:
>
> This change works around the HW default values to be able to test the
> tacoma board with -kernel command line option.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Good fix. This was required for Tacoma when we had both flash chips
enabled in the device tree, otherwise Linux would fail to probe the
entire controller leaving it with no rootfs.

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  hw/ssi/aspeed_smc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 8c79a5552f93..795784e5f364 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -230,7 +230,7 @@ static void aspeed_smc_reg_to_segment(const AspeedSMCState *s, uint32_t reg,
>
>  static const AspeedSegments aspeed_segments_ast2600_fmc[] = {
>      { 0x0, 128 * MiB }, /* start address is readonly */
> -    { 0x0, 0 }, /* disabled */
> +    { 128 * MiB, 128 * MiB }, /* default is disabled but needed for -kernel */
>      { 0x0, 0 }, /* disabled */
>  };
>
> --
> 2.25.4
>

Re: [PATCH for-5.2 00/19] aspeed: mostly cleanups and some extensions

[Cédric Le Goater]

On 8/6/20 3:20 PM, Cédric Le Goater wrote:
> Hello,
> 
> Various fixes improving the support of Aspeed machines.
> 
> Thanks,
> 
> C.
> 
> Cédric Le Goater (16):
>   m25p80: Return the JEDEC ID twice for mx25l25635e
>   m25p80: Add support for mx25l25635f
>   m25p80: Add support for n25q512ax3
>   aspeed/scu: Fix valid access size on AST2400
>   aspeed/smc: Fix MemoryRegionOps definition
>   aspeed/smc: Fix max_slaves of the legacy SMC device
>   aspeed/sdhci: Fix reset sequence
>   ftgmac100: Fix registers that can be read
>   ftgmac100: Fix interrupt status "Packet transmitted on ethernet"
>   ftgmac100: Fix interrupt status "Packet moved to RX FIFO"
>   ftgmac100: Change interrupt status when a DMA error occurs
>   ftgmac100: Check for invalid len and address before doing a DMA
>     transfer
>   ftgmac100: Fix integer overflow in ftgmac100_do_tx()
>   ftgmac100: Improve software reset
>   aspeed/sdmc: Simplify calculation of RAM bits
>   aspeed/smc: Open AHB window of the second chip of the AST2600 FMC
>     controller
> 
> Joel Stanley (2):
>   aspeed/sdmc: Perform memory training
>   aspeed/sdmc: Allow writes to unprotected registers
> 
> erik-smit (1):
>   hw/arm/aspeed: Add board model for Supermicro X11 BMC

Peter,

I saw that you just merged that one. I did some minor changes in 
the commit log. Nothing very important.

Thanks,

C.

> 
>  include/hw/misc/aspeed_sdmc.h |  13 +++-
>  hw/arm/aspeed.c               |  35 ++++++++++
>  hw/block/m25p80.c             |   4 +-
>  hw/misc/aspeed_scu.c          |   9 +--
>  hw/misc/aspeed_sdmc.c         | 125 +++++++++++++++++++---------------
>  hw/net/ftgmac100.c            |  45 ++++++++----
>  hw/sd/aspeed_sdhci.c          |  10 ++-
>  hw/ssi/aspeed_smc.c           |   6 +-
>  8 files changed, 167 insertions(+), 80 deletions(-)
> 

Re: [PATCH for-5.2 00/19] aspeed: mostly cleanups and some extensions

[Peter Maydell]

On Thu, 6 Aug 2020 at 14:24, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 8/6/20 3:20 PM, Cédric Le Goater wrote:
> > Hello,
> >
> > Various fixes improving the support of Aspeed machines.
> >
> > Thanks,
> >
> > C.
> >
> > Cédric Le Goater (16):
> >   m25p80: Return the JEDEC ID twice for mx25l25635e
> >   m25p80: Add support for mx25l25635f
> >   m25p80: Add support for n25q512ax3
> >   aspeed/scu: Fix valid access size on AST2400
> >   aspeed/smc: Fix MemoryRegionOps definition
> >   aspeed/smc: Fix max_slaves of the legacy SMC device
> >   aspeed/sdhci: Fix reset sequence
> >   ftgmac100: Fix registers that can be read
> >   ftgmac100: Fix interrupt status "Packet transmitted on ethernet"
> >   ftgmac100: Fix interrupt status "Packet moved to RX FIFO"
> >   ftgmac100: Change interrupt status when a DMA error occurs
> >   ftgmac100: Check for invalid len and address before doing a DMA
> >     transfer
> >   ftgmac100: Fix integer overflow in ftgmac100_do_tx()
> >   ftgmac100: Improve software reset
> >   aspeed/sdmc: Simplify calculation of RAM bits
> >   aspeed/smc: Open AHB window of the second chip of the AST2600 FMC
> >     controller
> >
> > Joel Stanley (2):
> >   aspeed/sdmc: Perform memory training
> >   aspeed/sdmc: Allow writes to unprotected registers
> >
> > erik-smit (1):
> >   hw/arm/aspeed: Add board model for Supermicro X11 BMC
>
> Peter,
>
> I saw that you just merged that one. I did some minor changes in
> the commit log. Nothing very important.

OK, I'll drop the old version from target-arm.next.

I'm wondering whether it would be simpler to let you just
submit pullreqs for aspeed-specific patches, since we seem
to have quite a lot of them and I'm generally letting you do
the review work anyway. What do you think ? I'm happy
to continue to take them in via target-arm.next if you prefer.

thanks
-- PMM