How to add support to different reg offset definition to share the same driver code?

How to add support to different reg offset definition to share the same driver code?

[Jun Nie]

Hi,

I am trying to add support to different reg offset and bit offset in
PL011 UART. It seems impossible to add macro in platform.dsc to enable
undef/redef in the header file with "#ifdef ZX_PL011_FLAG". Is there
any proper way to control the reg/bit offset definition? Or we have to
adopt the Linux driver method with a structure to hold different
offset value and wrap register access function as below? If so,
another Pcd is needed to specify the offset structure index for the
platforms.


static u16 pl011_st_offsets[REG_ARRAY_SIZE] = {
[REG_DR] = UART01x_DR,
[REG_ST_DMAWM] = ST_UART011_DMAWM,
[REG_ST_TIMEOUT] = ST_UART011_TIMEOUT,
        ...
}

static unsigned int pl011_read(const struct uart_amba_port *uap,
unsigned int reg)
{
void __iomem *addr = uap->port.membase + uap->reg_offset[reg];

return (uap->port.iotype == UPIO_MEM32) ?
readl_relaxed(addr) : readw_relaxed(addr);
}

Jun

Re: How to add support to different reg offset definition to share the same driver code?

[Leif Lindholm]

Hi Jun,

I think there is more than one benefit in mimicing the Linux driver,
so I would lean towards the Pcd option. But as Ard points out to me,
it needs to use a FixedPcd (using FixedPcdGet()) - this can only ever
have a buildtime resolution.

Regards,

Leif (technically on holiday, so no patch review until Monday)

On Fri, Jun 30, 2017 at 11:35:26AM +0800, Jun Nie wrote:
> Hi,
> 
> I am trying to add support to different reg offset and bit offset in
> PL011 UART. It seems impossible to add macro in platform.dsc to enable
> undef/redef in the header file with "#ifdef ZX_PL011_FLAG". Is there
> any proper way to control the reg/bit offset definition? Or we have to
> adopt the Linux driver method with a structure to hold different
> offset value and wrap register access function as below? If so,
> another Pcd is needed to specify the offset structure index for the
> platforms.
> 
> 
> static u16 pl011_st_offsets[REG_ARRAY_SIZE] = {
> [REG_DR] = UART01x_DR,
> [REG_ST_DMAWM] = ST_UART011_DMAWM,
> [REG_ST_TIMEOUT] = ST_UART011_TIMEOUT,
>         ...
> }
> 
> static unsigned int pl011_read(const struct uart_amba_port *uap,
> unsigned int reg)
> {
> void __iomem *addr = uap->port.membase + uap->reg_offset[reg];
> 
> return (uap->port.iotype == UPIO_MEM32) ?
> readl_relaxed(addr) : readw_relaxed(addr);
> }
> 
> Jun

Re: How to add support to different reg offset definition to share the same driver code?

[Gao, Liming]

Jun:
 You can add C MACRO in [BuildOptions] of Platform.dsc, then use DSC flag to control it. 

For example: Platform.dsc
[Defines]
DEFINE ZX_PL011_FLAG = FALSE

[BuildOptions]
!if $(ZX_PL011_FLAG) == TRUE
*_*_*_CC_FLAGS = -D ZX_PL011_FLAG
!endif

Thanks
Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jun Nie
> Sent: Friday, June 30, 2017 11:35 AM
> To: Leif Lindholm <leif.lindholm@linaro.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org;
> linaro-uefi@lists.linaro.org; Alexei.Fedorov@arm.com; evan.lloyd@arm.com
> Subject: [edk2] How to add support to different reg offset definition to share the same driver code?
> 
> Hi,
> 
> I am trying to add support to different reg offset and bit offset in
> PL011 UART. It seems impossible to add macro in platform.dsc to enable
> undef/redef in the header file with "#ifdef ZX_PL011_FLAG". Is there
> any proper way to control the reg/bit offset definition? Or we have to
> adopt the Linux driver method with a structure to hold different
> offset value and wrap register access function as below? If so,
> another Pcd is needed to specify the offset structure index for the
> platforms.
> 
> 
> static u16 pl011_st_offsets[REG_ARRAY_SIZE] = {
> [REG_DR] = UART01x_DR,
> [REG_ST_DMAWM] = ST_UART011_DMAWM,
> [REG_ST_TIMEOUT] = ST_UART011_TIMEOUT,
>         ...
> }
> 
> static unsigned int pl011_read(const struct uart_amba_port *uap,
> unsigned int reg)
> {
> void __iomem *addr = uap->port.membase + uap->reg_offset[reg];
> 
> return (uap->port.iotype == UPIO_MEM32) ?
> readl_relaxed(addr) : readw_relaxed(addr);
> }
> 
> Jun
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: How to add support to different reg offset definition to share the same driver code?

[Jun Nie]

2017-06-30 21:29 GMT+08:00 Gao, Liming <liming.gao@intel.com>:
> Jun:
>  You can add C MACRO in [BuildOptions] of Platform.dsc, then use DSC flag to control it.
>
> For example: Platform.dsc
> [Defines]
> DEFINE ZX_PL011_FLAG = FALSE
>
> [BuildOptions]
> !if $(ZX_PL011_FLAG) == TRUE
> *_*_*_CC_FLAGS = -D ZX_PL011_FLAG
> !endif
>
> Thanks
> Liming

Thanks for your demo code. It help a lot to a new comer.

Jun
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jun Nie
>> Sent: Friday, June 30, 2017 11:35 AM
>> To: Leif Lindholm <leif.lindholm@linaro.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org;
>> linaro-uefi@lists.linaro.org; Alexei.Fedorov@arm.com; evan.lloyd@arm.com
>> Subject: [edk2] How to add support to different reg offset definition to share the same driver code?
>>
>> Hi,
>>
>> I am trying to add support to different reg offset and bit offset in
>> PL011 UART. It seems impossible to add macro in platform.dsc to enable
>> undef/redef in the header file with "#ifdef ZX_PL011_FLAG". Is there
>> any proper way to control the reg/bit offset definition? Or we have to
>> adopt the Linux driver method with a structure to hold different
>> offset value and wrap register access function as below? If so,
>> another Pcd is needed to specify the offset structure index for the
>> platforms.
>>
>>
>> static u16 pl011_st_offsets[REG_ARRAY_SIZE] = {
>> [REG_DR] = UART01x_DR,
>> [REG_ST_DMAWM] = ST_UART011_DMAWM,
>> [REG_ST_TIMEOUT] = ST_UART011_TIMEOUT,
>>         ...
>> }
>>
>> static unsigned int pl011_read(const struct uart_amba_port *uap,
>> unsigned int reg)
>> {
>> void __iomem *addr = uap->port.membase + uap->reg_offset[reg];
>>
>> return (uap->port.iotype == UPIO_MEM32) ?
>> readl_relaxed(addr) : readw_relaxed(addr);
>> }
>>
>> Jun
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: How to add support to different reg offset definition to share the same driver code?

[Jun Nie]

2017-06-30 19:01 GMT+08:00 Leif Lindholm <leif.lindholm@linaro.org>:
> Hi Jun,
>
> I think there is more than one benefit in mimicing the Linux driver,
> so I would lean towards the Pcd option. But as Ard points out to me,
> it needs to use a FixedPcd (using FixedPcdGet()) - this can only ever
> have a buildtime resolution.
>
> Regards,
>
> Leif (technically on holiday, so no patch review until Monday)


Just learn from Liming that a MACRO is OK to control the register
offset definition in header file as we can change build option in
platform.dsc file. This should be most clear method to minimize impact
to other platform.

Jun

>
> On Fri, Jun 30, 2017 at 11:35:26AM +0800, Jun Nie wrote:
>> Hi,
>>
>> I am trying to add support to different reg offset and bit offset in
>> PL011 UART. It seems impossible to add macro in platform.dsc to enable
>> undef/redef in the header file with "#ifdef ZX_PL011_FLAG". Is there
>> any proper way to control the reg/bit offset definition? Or we have to
>> adopt the Linux driver method with a structure to hold different
>> offset value and wrap register access function as below? If so,
>> another Pcd is needed to specify the offset structure index for the
>> platforms.
>>
>>
>> static u16 pl011_st_offsets[REG_ARRAY_SIZE] = {
>> [REG_DR] = UART01x_DR,
>> [REG_ST_DMAWM] = ST_UART011_DMAWM,
>> [REG_ST_TIMEOUT] = ST_UART011_TIMEOUT,
>>         ...
>> }
>>
>> static unsigned int pl011_read(const struct uart_amba_port *uap,
>> unsigned int reg)
>> {
>> void __iomem *addr = uap->port.membase + uap->reg_offset[reg];
>>
>> return (uap->port.iotype == UPIO_MEM32) ?
>> readl_relaxed(addr) : readw_relaxed(addr);
>> }
>>
>> Jun