[PATCH qemu 11/38] aspeed/smc: get the number of flash modules from hw strapping

[Cédric Le Goater]

On 11/23/2016 02:22 AM, Andrew Jeffery wrote:
> On Tue, 2016-11-22 at 08:21 +0100, Cédric Le Goater wrote:
>>>> So we should set or unset these SCU regs when the SoC is created
>>>> and 
>>>> derive the wdt2 enablement from them. I suppose that the helper
>>>> routine 
>>>> to check for the conditions would be under SCU. 
>>>>
>>>> or do we start a simple pinctrl model and slowly add the
>>>> enablement
>>>> of the pins we need ? 
>>>>
>>>
>>> From the discussion on IRC last night between Cédric, Joel and
>>> myself
>>> we decided to start on a pinmux backend to help manage these
>>> issues.
>>
>> Do you have a rough idea of an API that the SoC could use ? 
>>
>> 	enum AspeedAST2500Function {
>> 		FWSPICS0,
>> 		FWSPICS1,
>> 		FWSPICS2,
>> 	};
>>
>> 	enum AspeedAST2400Function {
>> 		ROMCS1,
>> 		ROMCS2,
>> 		ROMCS3,
>> 		ROMCS4,
>> 	};
>>
>> 	bool aspeed_pin_is_enabled(AspeedPinmux *p, int func);
> 
> Largely this looks good.
> 
> But bikeshedding a bit, but I think the function name should focus on
> functions, not pins. The function maps to some pin in the physical
> case, but for qemu's purposes we don't care about that just yet. Even
> if we do care in the future we can internally map functions to pins
> behind a pinmux abstraction. As such I think we can get away with a
> small tweak:
> 
> bool aspeed_pinmux_is_enabled(AspeedPinmux *p, int func);

ok Looks good to me. I will fake this in the patchset to ease the 
change in the future.

> Did you have any thought about how we hoist ourselves from a
> AspeedPinmux to say an AspeedAST2500Pinmux with this interface? 

There are different solutions. We could introduce a class like

#define TYPE_ASPEED_PINMUX "aspeed-pinmux"
#define ASPEED_PINMUX(obj)
     OBJECT_CHECK(AspeedPinmux, (obj), TYPE_ASPEED_PINMUX)
#define ASPEED_PINMUX_CLASS(klass) \
     OBJECT_CLASS_CHECK(AspeedPinmuxClass, (klass), TYPE_ASPEED_PINMUX)
#define ASPEED_PINMUX_GET_CLASS(obj) \
     OBJECT_GET_CLASS(AspeedPinmuxClass, (obj), TYPE_ASPEED_PINMUX)

typedef struct AspeedPinmux {
	...
} AspeedPinmux;

typedef struct AspeedPinmuxClass {
	...
	AspeedPinmuxType pinmux_type;
        ... 

or
	... 
        bool (*is_enabled)(AspeedPinmux *pmux, int func);
	....
} AspeedPinmuxClass;

#define TYPE_ASPEED_PINMUX_POWER8E TYPE_ASPEED_PINMUX "-ast2500"
#define ASPEED_PINMUX_AST2500(obj) \
    OBJECT_CHECK(AspeedPinmux, (obj), TYPE_ASPEED_PINMUX_AST2500)

#define TYPE_ASPEED_PINMUX_POWER8E TYPE_ASPEED_PINMUX "-ast2400"
#define ASPEED_PINMUX_AST2400(obj) \
    OBJECT_CHECK(AspeedPinmux, (obj), TYPE_ASPEED_PINMUX_AST2400)


and use 'pinmux_type' to dispatch to specific handlers or directly
include the handlers we need under the class. 

If we only have handlers and no state, we could define a Interface
and tie it to the soc object.

typedef struct AspeedPinmuxInterface {
    Object parent;
} AspeedPinmuxInterface;

#define TYPE_ASPEED_PINMUX_INTERFACE "aspeed-pinmux-interface"
#define ASPEED_PINMUX_INTERFACE(obj) \
     OBJECT_CHECK(AspeedPinmuxInterface, (obj), TYPE_ASPEED_PINMUX_INTERFACE)
#define ASPEED_PINMUX_INTERFACE_CLASS(klass)                \
    OBJECT_CLASS_CHECK(AspeedPinmuxInterfaceClass, (klass), \
                       TYPE_ASPEED_PINMUX_INTERFACE)
#define ASPEED_PINMUX_INTERFACE_GET_CLASS(obj) \
     OBJECT_GET_CLASS(AspeedPinmuxInterfaceClass, (obj), TYPE_ASPEED_PINMUX_INTERFACE)

typedef struct AspeedPinmuxInterfaceClass {
    InterfaceClass parent;
    bool (*is_enabled)(AspeedPinmuxInterface *dev, int func);
    ...
} AspeedPinmuxInterfaceClass;


Conceptually, the SoC object would have a set of routines to handle
muxing. that seems ok to me as pinmux is a rather invasive component 
needing access to all controllers to be able to set the functions. 

C.

> As it
> stands (without specifying parts of AspeedPinmux) we don't have the
> context to correctly interpret the func arg. Already with these few
> functions we have to test different bits depending on the SoC at hand,
> and the 2500 has the extra requirements of talking to the GFX and LPC
> controllers in other cases.
> 
> I expect having a tag field internal to p is enough. I'll have a think
> about it.
> 
> Andrew
> 
>>
>> Thanks,
>>
>> C.