Re: [PATCH v4 05/12] hw/arm: Add NPCM730 and NPCM750 SoC models

[Philippe Mathieu-Daudé]

On 7/8/20 7:31 PM, Philippe Mathieu-Daudé wrote:
> Hi Havard,
> 
> On 7/7/20 8:47 PM, Havard Skinnemoen wrote:
>> The Nuvoton NPCM7xx SoC family are used to implement Baseboard
>> Management Controllers in servers. While the family includes four SoCs,
>> this patch implements limited support for two of them: NPCM730 (targeted
>> for Data Center applications) and NPCM750 (targeted for Enterprise
>> applications).
>>
>> This patch includes little more than the bare minimum needed to boot a
>> Linux kernel built with NPCM7xx support in direct-kernel mode:
>>
>>   - Two Cortex-A9 CPU cores with built-in periperhals.
>>   - Global Configuration Registers.
>>   - Clock Management.
>>   - 3 Timer Modules with 5 timers each.
>>   - 4 serial ports.
>>
>> The chips themselves have a lot more features, some of which will be
>> added to the model at a later stage.
[...]

>> +static void npcm7xx_realize(DeviceState *dev, Error **errp)
>> +{
>> +    NPCM7xxState *s = NPCM7XX(dev);
>> +    NPCM7xxClass *nc = NPCM7XX_GET_CLASS(s);
>> +    Error *err = NULL;
>> +    int i;
>> +
>> +    /* I/O space -- unimplemented unless overridden below. */
>> +    create_unimplemented_device("npcm7xx.io", NPCM7XX_MMIO_BA, 
>> NPCM7XX_MMIO_SZ);
> 
> I still insist this is not the best, but as "The data sheet for these
> SoCs is not generally available" there is not much I can suggest to
> improve.

>From your other comment I found:

https://github.com/Nuvoton-Israel/bootblock/blob/master/SWC_HAL/Chips/npcm750/npcm750.h

In particular:

#define AHB1_BASE_ADDR                  0xF0000000      /* AHB1
allocation (Including APB allocations)  */
#define AHB18_BASE_ADDR                 0x80000000      /* AHB18
allocation  */
#define AHB3_BASE_ADDR                  0xA0000000      /* AHB3
allocation  */
#define XBUSR_BASE_ADDR                 0xC0002000      /* XBUS
registers  */
#define AHB14_BASE_ADDR                 0xE0000000      /* AHB14
Allocation  */
#define APB14_BASE_ADDR                 0xE0000000      /* APB14
Allocation  */
#define VDMX_BASE_ADDR                  0xE0800000      /* VDMX  */

XBUS doesn't seem important.

If SPI flashes aren't connected, returning bus transaction sounds
correct:

#define SPI0CS0_BASE_ADDR               0x80000000      /* SPI0 direct
access CS0  */
#define SPI0CS1_BASE_ADDR               0x88000000      /* SPI0 direct
access CS1  */
#define SPI0CS2_BASE_ADDR               0x90000000      /* SPI0 direct
access CS2  */
#define SPI0CS3_BASE_ADDR               0x98000000      /* SPI0 direct
access CS3  */

#define SPI3CS0_BASE_ADDR               0xA0000000      /* SPI3 direct
access CS0  */
#define SPI3CS1_BASE_ADDR               0xA8000000      /* SPI3 direct
access CS1  */
#define SPI3CS2_BASE_ADDR               0xB0000000      /* SPI3 direct
access CS2  */
#define SPI3CS3_BASE_ADDR               0xB8000000      /* SPI3 direct
access CS3  */

So I'd prefer you use:

  create_unimplemented_device("npcm7xx.AHB1",  0xf0000000, 256 * MiB);

Maybe for the PCI root complex:

  create_unimplemented_device("npcm7xx.AHB14", 0xe0000000, 256 * MiB);

What do you think?

Regards,

Phil.