From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Thu, 7 Jan 2016 11:22:42 -0700 Subject: [U-Boot] [PATCH] fdt: Allow fdt_translate_address() to work with buses In-Reply-To: <568E4F4C.6070909@samsung.com> References: <1451862280-15245-1-git-send-email-sjg@chromium.org> <568AD2F9.2050903@wwwdotorg.org> <568BE59D.5040806@samsung.com> <568BFCBF.2080705@wwwdotorg.org> <568E4F4C.6070909@samsung.com> Message-ID: <568EACF2.8010809@wwwdotorg.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 01/07/2016 04:43 AM, Przemyslaw Marczak wrote: > Hello, > > On 01/05/2016 06:26 PM, Stephen Warren wrote: >> On 01/05/2016 08:47 AM, Przemyslaw Marczak wrote: >>> Hello, >>> >>> On 01/05/2016 02:00 AM, Simon Glass wrote: >>>> Hi Stephen, >>>> >>>> On 4 January 2016 at 13:15, Stephen Warren >>>> wrote: >>>>> On 01/03/2016 04:04 PM, Simon Glass wrote: >>>>>> >>>>>> It is common for I2C and SPI buses to have a single-cell address >>>>>> and a >>>>>> size >>>>>> of 0. These produce a warning at present. For example on snow: >>>>>> >>>>>> __of_translate_address: Bad cell count for gpc4 >>>>>> __of_translate_address: Bad cell count for gpx0 >>>>>> __of_translate_address: Bad cell count for gpv2 >>>>>> __of_translate_address: Bad cell count for gpv4 >>>>>> >>>>>> One of the nodes in question looks like this in part: >>>>>> >>>>>> pinctrl_2: pinctrl at 10d10000 { >>>>>> #address-cells = <1>; >>>>>> #size-cells = <0>; >>>>>> gpv2: gpv2 { >>>>>> reg = <0x060>; >>>>>> }; >>>>>> gpv4: gpv4 { >>>>>> reg = <0xc0>; >>>>>> }; >>>>>> }; >>>>>> >>>>>> This is clearly valid so it looks like the conversion to use >>>>>> fdt_translate_address() in dev_get_addr() is not currently a good >>>>>> move. >>>>> >>>>> >>>>> To disable that, why not simply turn off CONFIG_OF_TRANSLATE on the >>>>> affected >>>>> platforms? That's precisely why that config option was introduced >>>>> when the >>>>> call to fdt_translate_address() was added to dev_get_addr()? >>>>> >>>>> That would prevent this patch from affecting platforms that don't >>>>> trigger >>>>> this issue, this leaving the valid check in place. >>>> >>>> But since this breaks normal behaviour we don't know what platforms >>>> are affected. We have made CONFIG_OF_TRANSLATE the default. So this >>>> approach doesn't seem (in effect) any better than Przemyslaw's newer >>>> series, below. >>>> >>>>> >>>>>> Przemyslaw Marczak sent three patches to resolve this for exynos >>>>>> boards: >>>>>> >>>>>> https://patchwork.ozlabs.org/patch/557008/ >>>>>> https://patchwork.ozlabs.org/patch/557010/ >>>>>> https://patchwork.ozlabs.org/patch/557009/ >>>>>> >>>>>> But this involves creating a new function, and everyone will need to >>>>>> know >>>>>> when to use which one. Also the problem may affect other boards. >>>>> >>>>> >>>>> I suggest adding an extra parameter to dev_get_addr() (or whatever >>>>> calls it) >>>>> that indicates the root of the address space. The check on #size-cells >>>>> should be skipped for that one node (or level of translation) but >>>>> enabled >>>>> for all other levels. This way, there would be no need for anyone to >>>>> choose >>>>> between functions; there'd only be one. Most cases (i.e. translation >>>>> of MMIO >>>>> addresses) would simply pass 0 as the extra parameter (for the root >>>>> node), >>>>> but in special cases where it's known translation is not expected to >>>>> reach >>>>> the root MMIO space (e.g. I2C, SPI controllers), the controller node >>>>> would >>>>> be passed in. >>>> >>>> How would the caller know this root? It sounds plausible, but I do >>>> want to avoid complex rules. I think you are saying that buses that >>>> use their own address mechanism (i.e. not MMIO) must do something >>>> special. The current dev_get_addr() is really simple. >>> >>> Simon, Stephen >>> >>> As a summary of the issue, please tell me your opinion about this: >>> >>> Since the __of_translate_address() is called always with the "ranges" as >>> an argument, it looks reasonable to check it at the function beginning, >>> that the "ranges" property exists in the parent node. >>> >>> If not exists - then don't check the size-cells count and this should >>> fix the problem with additional argument. >>> This is simple and correct from specification point of view - which says >>> ranges and #size-cells property's - are not required [ePAPR v1.1]. >> >> It /might/ indeed be reasonable to skip the check on #size-cells if >> there is no ranges property; a missing ranges property is already >> treated as a 1:1 translation as described by the comment at the top of >> of_translate_one(). Still, there are likely some edge-cases w.r.t. the >> "length" fields in the ranges property that would need to be thought >> through in more detail before I'd say for certain that this change does >> absolutely make sense. >> >> Either way though, making that change wouldn't solve the problem at all. >> >> It's simply not possible to translate an I2C device address beyond the >> root of the I2C address space; there is no equivalent of an I2C device's >> address in MMIO address space. The root of the problem is that the code >> is attempting to perform an I2C->MMIO address translation. If we prevent >> that (by capping any such translation at the root of the I2C address >> space for example), the problem is solved at that point. Once that's >> solved, the issue of translating across a boundary with #size-cells=0 >> will never appear, and hence does not need to be solved. >> >> > > The problem with I2C and I2C->MMIO address translation is solved by my > new patch. The specification is clear for such case, if reg can't be > mapped to it's parent address space, then don't use 'ranges' and this > can be easy solved by additional check. Logic dictates that such a case (I2C->MMIO) doesn't make sense, so the specification doesn't cover it at all; see my upcoming comments on that patch.