|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [XEN/ARM PATCH v1 1/1] Unbreak Arndale XEN boot
On Fri, Sep 12, 2014 at 11:58 AM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hi Suriyan,
>
> A couple of remarks about the Samsung secure firmware and your patch.
>
> When secure firmware is present, the CPU bring up is done by SMC (I don't
> see this code in your patch).
>
Yes you are right. A call to call_firmware_op(cpu_boot, phys_cpu)
translating to exynos_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0) is missing
right before the call to waking up the cpu.
This was intentional on my part as the trustzone blob that comes with
the odroid xu does not have code for that smc call and just returns
-1.
I am guessing as we want this as generic as posisble, I should add
that part in.
> Also, for now, the secure firmware MMIO range is mapped to DOM0. I'm not
> sure we should do that because it contains way to idle CPU...
> I think we should blacklist it for now. And see how DOM0 behave without.
>
I shall try this on the Odroid XU.
Thanks!
- Suriyan
> Regards,
>
>
> On 11/09/14 14:01, Suriyan Ramasami wrote:
>>
>> On Thu, Sep 11, 2014 at 11:49 AM, Julien Grall <julien.grall@xxxxxxxxxx>
>> wrote:
>>>
>>> Hello Suriyan,
>>>
>>> My email address is @linaro.org not @linaro.com. I nearly miss this patch
>>> because of this.
>>>
>> Sorry about that. A slip on my part. Apologies.
>>
>>> Depending if Ian apply his patch (see the conversation on
>>> "Odroid-XU..."), I
>>> would update the title and the message.
>>>
>>>
>> Would the title change back to the previous XU patch - "Add Odroid-XU
>> board ..etc" with patch version 7 and modified message?
>>
>>> On 11/09/14 10:25, Suriyan Ramasami wrote:
>>>>
>>>>
>>>> diff --git a/xen/arch/arm/platforms/exynos5.c
>>>> b/xen/arch/arm/platforms/exynos5.c
>>>> index bc9ae15..334650c 100644
>>>> --- a/xen/arch/arm/platforms/exynos5.c
>>>> +++ b/xen/arch/arm/platforms/exynos5.c
>>>> @@ -28,6 +28,9 @@
>>>> #include <asm/platform.h>
>>>> #include <asm/io.h>
>>>>
>>>> +/* This corresponds to CONFIG_NR_CPUS in linux config */
>>>> +#define EXYNOS_CONFIG_NR_CPUS 0x08
>>>> +
>>>> #define EXYNOS_ARM_CORE0_CONFIG 0x2000
>>>> #define EXYNOS_ARM_CORE_CONFIG(_nr) (0x80 * (_nr))
>>>> #define EXYNOS_ARM_CORE_STATUS(_nr) (EXYNOS_ARM_CORE_CONFIG(_nr) +
>>>> 0x4)
>>>> @@ -42,8 +45,6 @@ static int exynos5_init_time(void)
>>>> u64 mct_base_addr;
>>>> u64 size;
>>>>
>>>> - BUILD_BUG_ON(EXYNOS5_MCT_G_TCON >= PAGE_SIZE);
>>>> -
>>>> node = dt_find_compatible_node(NULL, NULL,
>>>> "samsung,exynos4210-mct");
>>>> if ( !node )
>>>> {
>>>> @@ -61,7 +62,14 @@ static int exynos5_init_time(void)
>>>> dprintk(XENLOG_INFO, "mct_base_addr: %016llx size: %016llx\n",
>>>> mct_base_addr, size);
>>>>
>>>> - mct = ioremap_attr(mct_base_addr, PAGE_SIZE,
>>>> PAGE_HYPERVISOR_NOCACHE);
>>>> + if ( size <= EXYNOS5_MCT_G_TCON )
>>>
>>>
>>>
>>> Honestly, I don't think this check is very useful. The device tree should
>>> always contains valid size.
>>>
>> No doubt. But, wouldn't one have a wrong value for EXYNOS5_MCT_G_TCON?
>> I thought we are trying to catch wrong values of the offsets that we
>> use, to poke at the registers.
>>
>>>> + {
>>>> + dprintk(XENLOG_ERR, "EXYNOS5_MCT_G_TCON: %d is >= size\n",
>>>> + EXYNOS5_MCT_G_TCON);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + mct = ioremap_attr(mct_base_addr, size, PAGE_HYPERVISOR_NOCACHE);
>>>> if ( !mct )
>>>> {
>>>> dprintk(XENLOG_ERR, "Unable to map MCT\n");
>>>> @@ -91,14 +99,36 @@ static int exynos5250_specific_mapping(struct domain
>>>> *d)
>>>> return 0;
>>>> }
>>>>
>>>> -static int __init exynos5_smp_init(void)
>>>> +static int exynos_smp_init_getbaseandoffset(u64 *sysram_addr,
>>>> + u64 *sysram_offset)
>>>> {
>>>
>>>
>>>
>>> This function contains nearly twice the same code. Except the compatible
>>> string and the offset which differs, everything is the same.
>>>
>>> I would do smth like:
>>>
>>> node = dt_find_compatible_node(NULL, NULL, "samsung,secure-firmare");
>>> if ( !node )
>>> {
>>> compatible = "samsung,exynos4210-sysram";
>>> *sysram_offset = 0;
>>> }
>>> else
>>> {
>>> compatible "samsung,exynos4210-sysram-ns";
>>> *sysram_offset = 0x1c;
>>> }
>>>
>>> node = dt_find_compatible_node(NULL, NULL, compatible);
>>> if ( !node )
>>> ....
>>>
>>> [..]
>>>
>> I shall make that change.
>>
>>>> +static int __init exynos5_smp_init(void)
>>>> +{
>>>> + void __iomem *sysram;
>>>> + u64 sysram_addr;
>>>> + u64 sysram_offset;
>>>> + int rc;
>>>> +
>>>> + rc = exynos_smp_init_getbaseandoffset(&sysram_addr,
>>>> &sysram_offset);
>>>> + if ( rc )
>>>> + return rc;
>>>>
>>>> - sysram = ioremap_nocache(sysram_ns_base_addr, PAGE_SIZE);
>>>> + dprintk(XENLOG_INFO, "sysram_addr: %016llx offset: %016llx\n",
>>>> + sysram_addr, sysram_offset);
>>>> +
>>>> + if ( sysram_offset >= PAGE_SIZE )
>>>> + {
>>>> + dprintk(XENLOG_ERR, "sysram_offset is >= PAGE_SIZE\n");
>>>> + return -EINVAL;
>>>> + }
>>>
>>>
>>>
>>> As the previous check for the MCT. I don't think it's useful. Also why
>>> don't
>>> do get the size from the device tree?
>>>
>> In this case as we are poking only offset 0 or 0x1c which is always
>> less than PAGE_SIZE, I didn't bother with the size. I could if you
>> insist.
>> I can get rid of this check as sysram_offset is always less that
>> PAGE_SIZE.
>>
>>>> +
>>>> + sysram = ioremap_nocache(sysram_addr, PAGE_SIZE);
>>>> if ( !sysram )
>>>> {
>>>> dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n");
>>>> @@ -125,7 +176,7 @@ static int __init exynos5_smp_init(void)
>>>>
>>>> printk("Set SYSRAM to %"PRIpaddr" (%p)\n",
>>>> __pa(init_secondary), init_secondary);
>>>> - writel(__pa(init_secondary), sysram + 0x1c);
>>>> + writel(__pa(init_secondary), sysram + sysram_offset);
>>>>
>>>> iounmap(sysram);
>>>>
>>>> @@ -134,14 +185,14 @@ static int __init exynos5_smp_init(void)
>>>>
>>>> static int exynos_cpu_power_state(void __iomem *power, int cpu)
>>>> {
>>>> - return __raw_readl(power + EXYNOS_ARM_CORE_STATUS(cpu)) &
>>>> - S5P_CORE_LOCAL_PWR_EN;
>>>> + return __raw_readl(power + EXYNOS_ARM_CORE0_CONFIG +
>>>> + EXYNOS_ARM_CORE_STATUS(cpu)) &
>>>> S5P_CORE_LOCAL_PWR_EN;
>>>
>>>
>>>
>>> Linux has the EXYNOS_ARM_CORE0_CONFIG included in the macro
>>> EXYNOS_CORE_CONFIGURATION. I would do the same here.
>>>
>> OK, I will make that change.
>>
>>> [..]
>>>
>>>> - power = ioremap_nocache(power_base_addr +
>>>> - EXYNOS_ARM_CORE0_CONFIG, PAGE_SIZE);
>>>> + if ( size <= EXYNOS_ARM_CORE0_CONFIG +
>>>> + EXYNOS_ARM_CORE_STATUS(EXYNOS_CONFIG_NR_CPUS) )
>>>> + {
>>>> + dprintk(XENLOG_ERR, "CORE registers fall outside of pmu
>>>> range.\n");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>
>>>
>>>
>>> Same remark as before for the check.
>>>
>> My same response, in the sense that the size from DT will be correct,
>> but don't we have to make sure that the offsets that we calculate with
>> the #defines, fall within size?
>>
>>> [..]
>>>
>>>> - pmu = ioremap_nocache(power_base_addr, PAGE_SIZE);
>>>> + if ( size <= EXYNOS5_SWRESET )
>>>> + {
>>>> + dprintk(XENLOG_ERR, "EXYNOS5_SWRESET: %d is >= size\n",
>>>> + EXYNOS5_SWRESET);
>>>> + return;
>>>> + }
>>>> +
>>>
>>>
>>>
>>> Same here too.
>>
>> My same response as before, that the EXYNOS5_SWRESET offset might be
>> miscalculated or quoted for a newer platform which falls beyond size
>> offset.
>>
>> Please let me know if these checks are needed or not. I believe they
>> are good to keep, but if you insist I could leave them out.
>>
>> Also, the next version of this patch, do I still maintain the same
>> subject and comments, or should this come up as something else?
>>
>> Thanks as always
>> - Suriyan
>>
>>>
>>> Regards,
>>>
>>> --
>>> Julien Grall
>
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |