From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Tue, 12 Jan 2016 11:49:14 +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> Message-ID: <5694DA2A.1050907@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/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. Best regards, -- Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com