[U-Boot] [PATCH 3/5] efi_loader: complete implementation of GetTime()

[Alexander Graf]

On 04.07.18 21:26, Heinrich Schuchardt wrote:
> On 07/04/2018 05:46 PM, Alexander Graf wrote:
>> On 06/30/2018 04:52 AM, Heinrich Schuchardt wrote:
>>> Implement the missing parts of the GetTime() runtime service.
>>>
>>> Support CONFIG_DM_RTC=n.
>>> Fill seconds.
>>> Fill daylight saving time flag correctly.
>>> Provide dummy values for capabilities.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>> ---
>>>   lib/efi_loader/efi_runtime.c | 101 +++++++++++++++++++++++++++++------
>>>   1 file changed, 86 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
>>> index 5ec17867fb..20eb3f373d 100644
>>> --- a/lib/efi_loader/efi_runtime.c
>>> +++ b/lib/efi_loader/efi_runtime.c
>>> @@ -10,6 +10,7 @@
>>>   #include <dm.h>
>>>   #include <elf.h>
>>>   #include <efi_loader.h>
>>> +#include <i2c.h>
>>>   #include <rtc.h>
>>>     /* For manual relocation support */
>>> @@ -117,24 +118,86 @@ static void EFIAPI efi_reset_system_boottime(
>>>       while (1) { }
>>>   }
>>>   +int __weak rtc_get(struct rtc_time *tm)
>>> +{
>>> +    return 1;
>>> +}
>>> +
>>> +#if defined(CONFIG_SYS_RTC_BUS_NUM) && !defined(CONFIG_DM_RTC)
>>> +/**
>>> + * efi_set_rtc_i2c_bus - select I2C bus for real time clock
>>> + *
>>> + * @bus:        bus to select, -1 for default
>>> + * Return Value:    previously selected bus
>>> + */
>>> +static int efi_set_rtc_i2c_bus(int bus)
>>> +{
>>> +    int old_bus;
>>> +
>>> +    if (bus < 0)
>>> +        bus = CONFIG_SYS_RTC_BUS_NUM;
>>> +
>>> +#ifdef CONFIG_SYS_I2C
>>> +    old_bus = i2c_get_bus_num();
>>> +    i2c_set_bus_num(bus);
>>> +#else
>>> +    old_bus = I2C_GET_BUS();
>>> +    I2C_SET_BUS(bus);
>>> +#endif
>>> +    return old_bus;
>>> +}
>>> +#endif /* CONFIG_SYS_RTC_BUS_NUM && !CONFIG_DM_RTC */
>>> +
>>> +/**
>>> + * efi_get_time_boottime - get current time
>>> + *
>>> + * This function implements the GetTime runtime service.
>>> + * See the Unified Extensible Firmware Interface (UEFI) specification
>>> + * for details.
>>> + *
>>> + * @time:        pointer to structure to receive current time
>>> + * @capabilities:    pointer to structure to receive RTC properties
>>> + * Return Value:    status code
>>> + */
>>>   static efi_status_t EFIAPI efi_get_time_boottime(
>>>               struct efi_time *time,
>>>               struct efi_time_cap *capabilities)
>>>   {
>>> -#if defined(CONFIG_CMD_DATE) && defined(CONFIG_DM_RTC)
>>> -    struct rtc_time tm;
>>> +    efi_status_t ret = EFI_SUCCESS;
>>>       int r;
>>> -    struct udevice *dev;
>>> +    struct rtc_time tm;
>>>         EFI_ENTRY("%p %p", time, capabilities);
>>>   -    r = uclass_get_device(UCLASS_RTC, 0, &dev);
>>> -    if (r)
>>> -        return EFI_EXIT(EFI_DEVICE_ERROR);
>>> +    if (!time) {
>>> +        ret = EFI_INVALID_PARAMETER;
>>> +        goto out;
>>> +    }
>>>   -    r = dm_rtc_get(dev, &tm);
>>> -    if (r)
>>> -        return EFI_EXIT(EFI_DEVICE_ERROR);
>>> +#ifdef CONFIG_DM_RTC
>>> +    {
>>> +        struct udevice *dev;
>>> +
>>> +        r = uclass_get_device(UCLASS_RTC, 0, &dev);
>>> +        if (!r)
>>> +            r = dm_rtc_get(dev, &tm);
>>> +    }
>>> +#else
>>> +    {
>>> +#ifdef CONFIG_SYS_RTC_BUS_NUM
>>> +        int oldbus = efi_set_rtc_i2c_bus(-1);
>>
>> Please make up your mind whether you want an #ifdef in this code path or
>> not :). So IMHO you should either do the bus setting with ifdefs, but
>> then explicitly pass CONFIG_SYS_RTC_BUS_NUM as parameter or do it all
>> without ifdefs and just #ifdef out the body of efi_set_rtc_i2c_bus().
> 
> The first thing to decide is if we want to support non-DM RTC at all.
> What is your oppinion? - I put in the support for non-DM because I did
> not see that QEMU was using a device tree. But now Takahiro has set up
> the QEMU RTC driver as DM driver.

I think the RTC is a leaf enough use case to make DM mandatory on it.

> If CONFIG_SYS_RTC_BUS_NUM is defined or not is independent of
> CONFIG_DM_RTC, so we cannot unconditionally pass CONFIG_SYS_RTC_BUS_NUM
> here.

Yes, what I was trying to say is that this should either be:

	{
#ifdef CONFIG_SYS_RTC_BUS_NUM
		int oldbus = efi_set_rtc_i2c_bus(CONFIG_SYS_RTC_BUS_NUM);

#endif
		r = rtc_get(&tm);
#ifdef CONFIG_SYS_RTC_BUS_NUM
		efi_set_rtc_i2c_bus(oldbus);
#endif
	}


or

	{
		int oldbus = efi_set_rtc_i2c_bus(-1);

		r = rtc_get(&tm);
		efi_set_rtc_i2c_bus(oldbus);
	}

but passing in -1 when you are already in an #ifdef path that only
exists when CONFIG_SYS_RTC_BUS_NUM is available feels ... weird.

> Yes I can move the ifdefs to the body efi_set_rtc_i2c_bus() at the
> expense of some superfluous code being generated in case the function is
> not needed.

It will get optimized away :).

> But please, answer the above question first.

I don't mind it being DM only, as indicated above.


Alex