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

[Philippe Mathieu-Daudé]

On 7/9/20 2:23 AM, Havard Skinnemoen wrote:
> On Wed, Jul 8, 2020 at 10:56 AM Philippe Mathieu-Daudé <f4bug@amsat.org> 
> wrote:
>>
>> 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?
> 
> I went ahead and added them all since they are all defined in that
> public file. It does make the -d unimp output a lot more helpful.

Great news!

> I'll send v5 tonight. Not sure if I got the DRAM stuff 100% right.
> Please let me know what you think.

I am seeing this now and v5 is already posted, so I'll review it
directly instead.

> 
> Havard
>