Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[pankaj.dubey]

Hi Russell,

On Monday 14 November 2016 08:21 PM, Russell King - ARM Linux wrote:
> On Mon, Nov 14, 2016 at 03:37:44PM +0100, Arnd Bergmann wrote:
>> On Monday, November 14, 2016 1:50:18 PM CET Russell King - ARM Linux wrote:
>>> On Mon, Nov 14, 2016 at 01:03:09PM +0100, Arnd Bergmann wrote:
>>>> On Monday, November 14, 2016 2:10:16 PM CET pankaj.dubey wrote:
>>>>>>> +    scu_base = of_iomap(np, 0);
>>>>>>> +    of_node_put(np);
>>>>>>> +    if (!scu_base) {
>>>>>>> +            pr_err("%s failed to map scu_base via DT\n", __func__);
>>>>>>
>>>>>> For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
>>>>>> what does it mean, but it may confuse normal users. In current version,
>>>>>> berlin doesn't complain like this for non-ca9 SoCs
>>>>>>
>>>>>
>>>>> OK, let me see other reviewer's comment on this. Then we will decide if
>>>>> this error message is required or can be omitted.
>>>>
>>>> We need to look at all callers here, to see if the function ever gets
>>>> called for a CPU that doesn't have an SCU. I'd say we should warn if
>>>> we know there is an SCU but we cannot map it, but never warn on
>>>> any of the CPU cores that don't support an SCU.
>>>
>>> Maybe there should be two helpers:
>>>
>>> of_scu_enable() which _only_ looks up the SCU address in DT and enables
>>> it if it finds it, otherwise returning failure.
>>>
>>> a9_scu_enable() which tries to use the A9 provided SCU address and
>>> enables it if it finds it, otherwise returning failure.
>>>

OK, In that case I can see need for following four helpers as:

1: of_scu_enable() which will __only__ lookup the SCU address in DT and
enables it if it finds, otherwise return -ENOMEM failure.
This helper APIs is required and sufficient for most of platforms such
as exynos, berlin, realview, socfpga, STi, ux500, vexpress, rockchip and
mvebu

2: a9_scu_enable(), which will __only__ use A9 provided SCU address and
enables it, if address mapped successfully, otherwise returning failure.
This helper APIs is required and sufficient for two ARM platforms as of
now tegra and hisi.

3: of_scu_get_base() which will lookup the SCU address in DT and if node
found maps address and returns ioremapped address to caller.
This helper APIs is required for three ARM plaforms rockchip, mvebu and
ux500, along with scu_enable() API to enable and find number_of_cores.

4: s9_scu_iomap_base() which will internally use s9_scu_get_base() and
do ioremap of scu address and returns ioremapped address to the caller
along with ownership (caller has responsibility to unmap it).
This helper APIs is required to simplify SCU enable and related code in
two ARM plaforms BCM ans ZX.

For remaining two ARM platforms (IMX and ZYNQ), none of these helpers
are useful for the time-being, as they need SCU mapping very early of
boot, where we can't use iomap APIs. So I will drop patches related to
these platforms in v2 version.

Please let me know if any concern in this approach.


>>> Then callers can decide which of these to call, and what error messages
>>> to print on their failures.
>>
>> Splitting the function in two is probably simpler overall, but
>> we may still have to look at all the callers: Any platform that
>> currently tries to map it on any CPU and doesn't warn about the
>> absence of the device node (or about scu_a9_has_base() == false)
>> should really continue not to warn about that.
> 
> Did you miss the bit where none of of_scu_enable() or a9_scu_enable()
> should produce any warnings or errors to be printed.  It's up to the
> caller to report the failure, otherwise doing this doesn't make sense:
> 
> 	if (of_scu_enable() < 0 && a9_scu_enable() < 0)
> 		pr_err("Failed to map and enable the SCU\n");
> 
> because if of_scu_enable() prints a warning/error, then it's patently
> misleading.
> 

I will move out error message out of these helpers and let caller
(platform specific code) handle and print error if required.


Thanks,
Pankaj Dubey