[PATCH 2/2] serial: a37xx: Do not call get_ref_clk() in _debug_uart_init()

[Stefan Roese]

On 16.08.21 10:33, Pali Rohár wrote:
> On Monday 16 August 2021 10:04:39 Stefan Roese wrote:
>> Hi Pali,
>>
>> On 11.08.21 20:57, Pali Rohár wrote:
>>> On Monday 26 July 2021 17:24:26 Marek Behun wrote:
>>>> On Mon, 26 Jul 2021 16:58:04 +0200
>>>> Pali Rohár <pali at kernel.org> wrote:
>>>>
>>>>> On Monday 26 July 2021 16:55:22 Marek Behun wrote:
>>>>>> On Mon, 26 Jul 2021 14:58:59 +0200
>>>>>> Pali Rohár <pali at kernel.org> wrote:
>>>>>>> Static inline function _debug_uart_init() should avoid calling external
>>>>>>> (non-inline) functions.
>>>>>>
>>>>>> Why?
>>>>>
>>>>> Function is called in stage when stack is not fully initialized and
>>>>> documentation suggest to avoid stack usage and other functions.
>>>>
>>>> OK, but maybe we should use the macro names for register constants.
>>>>
>>>> Could you move (in a separate patch) the corresponding macros from
>>>> arch/arm/mach-mvebu/armada3700/cpu.c to
>>>> arch/arm/mach-mvebu/include/mach/soc.h and then in this patch
>>>> include <asm/arch/soc.h> in the serial driver and use the constant
>>>> names?
>>>
>>> Implemented in separate patch as Stefan wanted:
>>> http://patchwork.ozlabs.org/project/uboot/patch/20210811185330.15414-2-pali@kernel.org/
>>
>> Did I really want this? You're removing the macro names with this patch,
>> and I would prefer using the defines/names instead of register numbers
>> like here:
>>
>> +#elif defined(CONFIG_ARMADA_3700)
>> +/* SAR values for Armada 3700 */
>> +#define CONFIG_SYS_REF_CLK	((readl(MVEBU_REGISTER(0x13808)) & BIT(9)) ? \
>> +				 40000000 : 25000000)
>>   #endif
>>
>> Or did I miss something?
>>
>> Thanks,
>> Stefan
> 
> It is global include header file, so I though it would be better to not
> export lot of other macros which are relevant only for CONFIG_SYS_REF_CLK
> 
> But if you want to see names, I can change it e.g. to:
> 
> +#elif defined(CONFIG_ARMADA_3700)
> +/* SAR values for Armada 3700 */
> +#define MVEBU_TEST_PIN_LATCH_N	MVEBU_REGISTER(0x13808)
> +#define MVEBU_XTAL_MODE_MASK	BIT(9)
> +#define CONFIG_SYS_REF_CLK	((readl(MVEBU_TEST_PIN_LATCH_N) & MVEBU_XTAL_MODE_MASK) ? \
> +				 40000000 : 25000000)
> 

IMHO, this version is a bit better, as the names (hopefully) reflect
the register description in the datasheet.

Thanks,
Stefan