From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Tue, 12 Jan 2016 15:25:32 +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> <568FA52E.2030001@samsung.com> <5694DA2A.1050907@samsung.com> Message-ID: <56950CDC.80003@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/12/2016 02:59 PM, Simon Glass wrote: > Hi Przemyslaw, > > On 12 January 2016 at 03:49, Przemyslaw Marczak wrote: >> Hello Simon, >> >> >> On 01/11/2016 05:59 PM, Simon Glass wrote: >>> >>> Hi Przemyslaw, >>> >>> On 8 January 2016 at 05:01, Przemyslaw Marczak >>> wrote: >>>> >>>> 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. >>> >>> >>> The release is scheduled for today, so we had to do something to fix >>> the breakage. >>> >>> Once you have a full solution figured out we can revert this patch and >>> apply what you come up with. >>> >>> Regards, >>> Simon >>> >>> >> >> Ok. It's hard to convince Stephen to accept such change, so I will send a >> patch with another solution - just bring back fdtdec_get_addr() for Exynos >> GPIO driver. And will revert this one within the patchset. > > Please hold off on that. I'll accept your other patch but let's see if > Stephen wants to write something first. Using fdtdec_get_addr() > doesn't make sense although I fully understand your frustration. Let's > give it a week. > > Regards, > Simon > > Ah, too late:) I have prepared patch, it's simple and doesn't touch the code - only device-tree. And this would be a proper approach, according to Stephen's comments. Best regards, -- Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com