From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Fri, 08 Jan 2016 13:01:50 +0100 Subject: [U-Boot] [PATCH] fdt: fix address cell count checking in fdt_translate_address() In-Reply-To: References: <1446043077-21005-1-git-send-email-p.marczak@samsung.com> <5638850D.2080205@samsung.com> <563C4CEE.1060706@denx.de> Message-ID: <568FA52E.2030001@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Simon, On 01/07/2016 08:24 PM, Simon Glass wrote: > +Stephen > > On 4 January 2016 at 17:59, Simon Glass wrote: >> Hi Przemyslaw, >> >> On 5 November 2015 at 23:47, Stefan Roese wrote: >>> On 06.11.2015 04:16, Simon Glass wrote: >>>> >>>> Hi, >>>> >>>> On 3 November 2015 at 02:57, Przemyslaw Marczak >>>> wrote: >>>>> >>>>> Hello All, >>>>> >>>>> >>>>> On 10/29/2015 06:15 PM, Simon Glass wrote: >>>>>> >>>>>> >>>>>> Hi Stefan, >>>>>> >>>>>> On 28 October 2015 at 08:37, Przemyslaw Marczak >>>>>> wrote: >>>>>>> >>>>>>> >>>>>>> Commit: dm: core: Enable optional use of fdt_translate_address() >>>>>>> >>>>>>> Enables use of this function as default, but after this it's not >>>>>>> possible to get dev address for the case in which: '#size-cells == 0' >>>>>>> >>>>>>> This causes errors when getting address for some GPIOs, for which >>>>>>> the '#size-cells' is set to 0. >>>>>>> >>>>>>> Example error: >>>>>>> '__of_translate_address: Bad cell count for gpx0' >>>>>>> >>>>>>> Allowing for that case by modifying the macro 'OF_CHECK_COUNTS', >>>>>>> (called from )__of_translate_address(), fixes the issue. >>>>>>> >>>>>>> Now, this macro doesn't check, that '#size-cells' is greater than 0. >>>>>>> >>>>>>> This is possible from the specification point of view, but I'm not sure >>>>>>> that it doesn't introduce a regression for other configs. >>>>>>> >>>>>>> Please test and share the results. >>>>>>> >>>>>>> Tested-on: Odroid U3, Odroid X2, Odroid XU3, Sandbox. >>>>>>> >>>>>>> Signed-off-by: Przemyslaw Marczak >>>>>>> Cc: Masahiro Yamada >>>>>>> Cc: Lukasz Majewski >>>>>>> Cc: Jaehoon Chung >>>>>>> Cc: Stefan Roese >>>>>>> Cc: Simon Glass >>>>>>> Cc: Bin Meng >>>>>>> Cc: Marek Vasut >>>>>>> --- >>>>>>> common/fdt_support.c | 7 +++---- >>>>>>> 1 file changed, 3 insertions(+), 4 deletions(-) >>>>>>> >>>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c >>>>>>> index f86365e..5f808cc 100644 >>>>>>> --- a/common/fdt_support.c >>>>>>> +++ b/common/fdt_support.c >>>>>>> @@ -946,8 +946,7 @@ void fdt_del_node_and_alias(void *blob, const char >>>>>>> *alias) >>>>>>> /* Max address size we deal with */ >>>>>>> #define OF_MAX_ADDR_CELLS 4 >>>>>>> #define OF_BAD_ADDR ((u64)-1) >>>>>>> -#define OF_CHECK_COUNTS(na, ns) ((na) > 0 && (na) <= >>>>>>> OF_MAX_ADDR_CELLS && \ >>>>>>> - (ns) > 0) >>>>>>> +#define OF_CHECK_COUNTS(na) ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS) >>>>>>> >>>>>>> /* Debug utility */ >>>>>>> #ifdef DEBUG >>>>>>> @@ -1115,7 +1114,7 @@ static u64 __of_translate_address(void *blob, int >>>>>>> node_offset, const fdt32_t *in >>>>>>> >>>>>>> /* Cound address cells & copy address locally */ >>>>>>> bus->count_cells(blob, parent, &na, &ns); >>>>>>> - if (!OF_CHECK_COUNTS(na, ns)) { >>>>>>> + if (!OF_CHECK_COUNTS(na)) { >>>>>> >>>>>> >>>>>> >>>>>> This seems to conflict with the comment at the top of this function: >>>>>> >>>>>> * Note: We consider that crossing any level with #size-cells == 0 to >>>>>> mean >>>>>> * that translation is impossible (that is we are not dealing with a >>>>>> value >>>>>> * that can be mapped to a cpu physical address). This is not really >>>>>> specified >>>>>> * that way, but this is traditionally the way IBM at least do things >>>>>> >>>>>> What should we do here? >>>>>> >>>>> >>>>> Is that commit acceptable? I would like send V2 with removing the above >>>>> comment. >>>> >>>> >>>> That's what I am worried about. Presumably the comment is accurate >>>> today and this check has some value. I was hoping Stefan might know. >>> >>> >>> Unfortunately no. I just stumbled over this problem with the >>> translation of the "complex" ranges on the MVEBU platform. And >>> noticed that we already have this functionality to translate >>> the addresses the "right way". >>> >>> I'm wondering how this problem with those GPIOs is handled in >>> the kernel? I assume that it is working correctly there, right? >>> Przemyslaw, could you perhaps check this and see, why its >>> working there? And change / fix it in U-Boot accordingly? >> >> Let's pick up this patch for now as a bug-fix. We can deal with this >> problem after the release. > > Applied to u-boot-dm/master. > > I'll post a revert after the release. It seems like you and Stephen > are making good progress. > > - Simon > > Why so fast with this one? I think, that more proper for a temporary fix is my latest patch with #size-cells count checking only if ranges found in the parent node. I will continue the discussion with Stephen. Best regards, -- Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com