Re: [Qemu-devel] [PATCH v3 20/20] arm: add Faraday FTSPI020 SPI flash controller support

[Kuo-Jung Su]

2013/2/8 Peter Crosthwaite <address@hidden>:
> On Thu, Feb 7, 2013 at 10:13 PM, Andreas Färber <address@hidden> wrote:
>> Am 07.02.2013 13:04, schrieb Peter Maydell:
>>> On 7 February 2013 11:50, Andreas Färber <address@hidden> wrote:
>>>> Am 07.02.2013 07:59, schrieb Kuo-Jung Su:
>>>>>>> +/******************************************************************************
>>>>>>> + * FTSPI020 registers
>>>>>>> + 
>>>>>>> *****************************************************************************/
>>>>>>> +#define REG_CMD0            0x00    /* Flash address */
>>>>>>> +#define REG_CMD1            0x04
>>>>>>> +#define REG_CMD2            0x08
>>>>>>> +#define REG_CMD3            0x0c
>>>>>>> +#define REG_CTRL            0x10    /* Control Register */
>>>>>>> +#define REG_ACT             0x14    /* AC Timing Register */
>>>>>>> +#define REG_STR             0x18    /* Status Register */
>>>>>>> +#define REG_ICR             0x20    /* Interrupt Control Register */
>>>>>>> +#define REG_ISR             0x24    /* Interrupt Status Register */
>>>>>>> +#define REG_RDST            0x28    /* Read Status Register */
>>>>>>> +#define REG_REV             0x50    /* Revision Register */
>>>>>>> +#define REG_FEA             0x54    /* Feature Register */
>>>>>>> +#define REG_DATA            0x100    /* Data Register */
>>>>>>> +
>>>>>>
>>>>>> These can just live in the C file - they are private to the 
>>>>>> implementation.
>>>>
>>>> No. Having them in a header means they can be reused by qtests.
>>>
>>> Do we really want to change the public/private boundaries of
>>> our source code just for the benefit of the test framework? Ugh.
>>
>> The truth is (and that applies to the hw/ refactoring series as well)
>> that we don't have a clear distinction between "public" and "private"
>> headers for devices.
>>
>
> I guess a logical conclusion here is that defines that relate to the
> programmers model (eg #define REG_ISR 0x0C/4) and parameterization
> (e.g. #define NUM_CHIP_SELECTS 4) need to be public while everything
> else (implementation stuff) is private. In theory however, the only
> client of a programmers model is unit testing. So perhaps there should
> be some sort of poisoning system like with the cpu headers? Your only
> allowed to include a header for a concrete class (such as this) if you
> own it, or if your a test.
>

Got it, thanks.
I'll try to update the header files and source files to meet the
conclusion here.

>>>> To embed the device into a SoC object, the state struct and TYPE_
>>>> constant should be in the header. That applies to each device part of
>>>> the SoC rather than on the board.
>>>
>

Did you mean to create a new DeviceState instance for SoC (e.g.
faraday_a369_soc)
and a new QEMUMachine instance for Target Board (e.g. faraday_a369_evb)

If I'm correct, in this way, it would be easier to get the instance of
parent object thru
'obj->parent' without breaking the QOM rules. The document I've found
for QOM is a only abstract concept (http://wiki.qemu.org/Features/QOM)
and since my english sucks, it's
too difficult to me to understanding the concrete QOM implementation
rules without example codes.

BTW emulaing Faraday AHB remap function, it requires the items bellow
to be accessible to
these chips: SoC, DDR controller, AHB controller.

1. RAM size (Board parameter)
2. RAM base & window size (SoC parameter)
3. ROM base & window size (SoC parameter)
4. AHB address remap for ROM/RAM (SoC parameter)

> I really think this is a step backwards. Every little lightweight
> device model needs its own header just for
>
> #define TYPE_FOO "foo"
>
> Regards,
> Peter
>
>>> I would like it if we could define what we consider to be best
>>> practice for public/private header files for QOM devices
>>> (in particular whether we need a public header for every device
>>> with the TYPE_ constants &c).
>>
>> gtk-doc for instance needs to scan one directory, so having all headers
>> in include/ would be beneficial long-term - today we do not get
>> generated documentation for qdev-core.h.
>>
>> The separation between PCI internal helpers and
>> structs/typedefs-to-be-used is made by including specific headers
>> depending on what you need.
>>
>> In the refcatoring thread I suggested that if we want to have all SPI
>> devices in hw/spi/ then each device that needs a public function
>> declaration or TYPE_ or struct should export its own header. If we group
>> them into a common directory such as hw/arm/ that could be shared across
>> SoC devices. Therefore I was suggesting not to split up too much but
>> seem to be overruled. ;) There's downsides to everything.
>>
>> Andreas
>>
>> --
>> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>>



--
Best wishes,
Kuo-Jung Su