Re: [PATCH 2/9] ARM: dts: uniphier: rework UniPhier System Bus nodes

From: Olof Johansson
Date: Thu Feb 25 2016 - 02:20:38 EST

On Wed, Feb 24, 2016 at 6:22 PM, Masahiro Yamada
<yamada.masahiro@xxxxxxxxxxxxx> wrote:
> Hi Olof,
> 2016-02-25 9:26 GMT+09:00 Olof Johansson <olof@xxxxxxxxx>:
>> Hi,
>> On Tue, Feb 16, 2016 at 11:15:04AM +0900, Masahiro Yamada wrote:
>>> diff --git a/arch/arm/mach-uniphier/platsmp.c b/arch/arm/mach-uniphier/platsmp.c
>>> index e1cfc1d..b53a8d9 100644
>>> --- a/arch/arm/mach-uniphier/platsmp.c
>>> +++ b/arch/arm/mach-uniphier/platsmp.c
>>> @@ -30,7 +30,7 @@
>>> * The secondary CPUs check this register from the boot ROM for the jump
>>> * destination. After that, it can be reused as a scratch register.
>>> */
>>> -#define UNIPHIER_SBC_ROM_BOOT_RSV2 0x1208
>>> static void __iomem *uniphier_smp_rom_boot_rsv2;
>>> static unsigned int uniphier_smp_max_cpus;
>>> @@ -98,15 +98,14 @@ static int __init uniphier_smp_prepare_trampoline(unsigned int max_cpus)
>>> phys_addr_t rom_rsv2_phys;
>>> int ret;
>>> - np = of_find_compatible_node(NULL, NULL,
>>> - "socionext,uniphier-system-bus-controller");
>>> - ret = of_address_to_resource(np, 1, &res);
>>> + np = of_find_compatible_node(NULL, NULL, "socionext,uniphier-smpctrl");
>>> + ret = of_address_to_resource(np, 0, &res);
>>> if (ret) {
>>> - pr_err("failed to get resource of system-bus-controller\n");
>>> + pr_err("failed to get resource of uniphier-smpctrl\n");
>>> return ret;
>>> }
>>> - rom_rsv2_phys = res.start + UNIPHIER_SBC_ROM_BOOT_RSV2;
>>> + rom_rsv2_phys = res.start + UNIPHIER_SMPCTRL_ROM_BOOT_RSV2;
>>> ret = uniphier_smp_copy_trampoline(rom_rsv2_phys);
>>> if (ret)
>> The previous binding has already been released. You can update, but your driver
>> should be able to handle the previous binding.
>> So, you still need to keep the old code around.
>> This has the benefit of breaking the dependency between the code change and the
>> DT change, so you no longer have to change your platform code at the same time
>> as the DT to avoid regressions.
>> Please adjust and resend. I'll hold off applying the series until then, so we
>> don't have a partially applied series.
> How long do I have to keep the support for the old binding?

You know your platform best -- how many users do you think you have
out there that might have built DTS files based on the old binding?

If there's a good chance there are none, or if you're in good contact
with them and can ask them to update, then you can be more flexible.

> [1]
> Everyone makes mistakes.
> The constraint for the DT-binding is really really painful.
> This is how it happened.
> At first, I implemented uniphier-system-bus.c based on the old binding.
> Then, during the review, Mark suggested me to change the driver design:
> I followed his suggestion, but I needed to changed the DT-binding as well.
> Before that time, the DT and other support code for UniPhier had been
> partially merged
> in the mainline. So, in the current tree exist two bindings that are
> not compatible to
> each other. This situation is really a mess.
> I want to clean up the code as soon as possible.

Yeah, I understand that it's hard to come up with perfect bindings
from day one, and that's why we sometimes have to play by ear.

It's not a bad idea to get practice on how to solve it -- in this case
it wouldn't really bad that bad. If you use variables to hold the base
addresses, and get them from either binding, you'll only special-case
during probe and not anywhere else in the driver.

The general idea of decoupling DT changes from code changes is also a
good habit.

> [2]
> For now, DT is mostly developed in the kernel tree in practice,
> while DT is not theoretically only for Linux.
> Everybody (at least every user of UniPhier SoCs) uses the combination
> of a DTB and a kernel image
> generated from the same Linux tree.
> I see no reason to use a new DTB + an old kernel image, or vice versa.

We're not aiming to support new DTB + old kernel image. The main
problem is if someone has a product DTB that's not yet merged, and you
change the binding, then their DTB might no longer work. It's not a
huge deal, and for most changes it's fairly harmless, but the general
principle still applies.

As I said earlier, you know the users of your platform the best (I
hope :), so you'll have the best feel for whether this is a breakage
they will hurt from or not.

> [3]
> This binding is UniPhier-specific. No impact on other SoC vendors.
> Everything is under my control.
> For now, I will prepare the logic to support the old binding,
> but for the reasons above, please let me drop the support for the old
> one some time later.

Yeah, I'm perfectly fine with not keeping it for a long time. For
example, feel free to remove it next release if you think that will
work for your downstream users.