From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934261AbcIVPBs (ORCPT ); Thu, 22 Sep 2016 11:01:48 -0400 Received: from mout.kundenserver.de ([212.227.126.133]:60413 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934129AbcIVPBn (ORCPT ); Thu, 22 Sep 2016 11:01:43 -0400 From: Arnd Bergmann To: Gabriele Paoloni Cc: zhichang , "linux-arm-kernel@lists.infradead.org" , "devicetree@vger.kernel.org" , "lorenzo.pieralisi@arm.com" , "minyard@acm.org" , "linux-pci@vger.kernel.org" , "gregkh@linuxfoundation.org" , John Garry , "will.deacon@arm.com" , "linux-kernel@vger.kernel.org" , Yuanzhichang , Linuxarm , "xuwei (O)" , "linux-serial@vger.kernel.org" , "benh@kernel.crashing.org" , "zourongrong@gmail.com" , "liviu.dudau@arm.com" , "kantyzc@163.com" Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Date: Thu, 22 Sep 2016 16:59:29 +0200 Message-ID: <1669038.JevuM4F83e@wuerfel> User-Agent: KMail/5.1.3 (Linux/4.4.0-34-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: References: <1473855354-150093-1-git-send-email-yuanzhichang@hisilicon.com> <9178320.n4yHmfyPA3@wuerfel> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:B5MOrU5F/RYxo6tM5/S0dR3Z4I3y54vlD1mlSENTb9Ia+kmwYy7 YYDB/RX9ckDIhDH4+GnqHMVPqV5MxerXjNf3RAN1Y47xZ/LP3wG/pt9wWzCN8D2q99uf81X P6ZTGsZMyBDrCYg9dzUtSJBbTlrYRoSOi6jH/ZWi9CfkAmp6dxpuGVohq0U8JQqe+xPA0qG GkgtnE7d/wfgFHfn/51sg== X-UI-Out-Filterresults: notjunk:1;V01:K0:YrQeU4fxv24=:Nnh2qSxPX+iWOh3awDTdPN B+GDcTZaIbAibPhnQkTHQp3iGHxu2GhiPFk7HBjMJ8xVLllPWLtIXVENHYWBKjsFnokZl9AZ0 8B8q57lEOCjg+umdF6v9tzxvu9LrtGQvjHsFFNCGXtV1mjXBAV9EW7jwALvP5GoUvEks1mSgH gPbBJkA94z+6bUtWQh3oJ6bazj77tQQw5DLAJuf0BmiYlRCdPNH0VGhFgKbK+NJLX2HvETgKe yzTbzBZgAaGJSEEWS/QNsN5B9wMBcgWVa+iuaVvMX9pRpRP5IjNpWOMglJeC1FPx74aBCYHg0 XFeR5edK6Xhm8fNy44J6lkW6HzheVXbw4CTwY4pEr1it13cTb88Zy03eSKTAnyy56cp4yMEIl fBIm0FADgfqwImyheFF31l7vzdeQSR6xfczqyzESa+l8zYxY0ti4FRnkcHAd6EQSRBReQcSDY o1jWGiewz8Rs4XOR/Q/0PTUsz2R7SCDETfuF0uGWhF4E6XRL2FHVwFk0SJQiLSsTyxG9wn400 hv5Ek178j7BBslwVuCrjSZftMwRE7bHYEfP00zU/Ugc5ltYxd8yj+6R3BvFC1Zc8WGWC1lCgw N2jQILC63aI12F/YYEkTOdrwLnqVC+HASf7FBZFNiaGcM5TMUJD3BXijdgW28QrGJymiPrX/n w1214mtECDBe67OFHpJBY2ph5YA9mTIhnpQIBxFuVg0Q4ExJHCqijyWiGwXO9ijb+wtVMsEDS rvxTHE5CZYuA0GBG Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thursday, September 22, 2016 2:47:14 PM CEST Gabriele Paoloni wrote: > > > static int of_empty_ranges_quirk(struct device_node *np) > > > { > > > if (IS_ENABLED(CONFIG_PPC)) { > > > @@ -503,7 +512,7 @@ static int of_translate_one(struct device_node > > *parent, struct of_bus *bus, > > > * This code is only enabled on powerpc. --gcl > > > */ > > > ranges = of_get_property(parent, rprop, &rlen); > > > - if (ranges == NULL && !of_empty_ranges_quirk(parent)) { > > > + if (ranges == NULL && !of_empty_ranges_quirk(parent) && > > !of_isa_indirect_io(parent)) { > > > pr_debug("OF: no ranges; cannot translate\n"); > > > return 1; > > > } > > > > I don't see what effect that would have. What do you want to > > achieve with this? > > If I read the code correctly adding the function above would end > up in a 1:1 mapping: > http://lxr.free-electrons.com/source/drivers/of/address.c#L513 > > so taddr will be assigned with the cpu address space specified > in the children nodes of LPC and we are not using a quirk function > (we are just checking that we have the indirect io assigned and > that we are on a ISA bus). Now probably there is a nit in my > code sketch where of_isa_indirect_io should be probably an architecture > specific function... But the point is that it would then return an incorrect address, which in the worst case could be the same as another I/O space if that happens to be at CPU address zero. > > I think all we need from this function is to return '1' if > > we hit an ISA I/O window, and that should happen for the two > > interesting cases, either no 'ranges' at all, or no translation > > for the range in question, so that __of_translate_address can > > return OF_BAD_ADDR, and we can enter the special case > > handling in the caller, that handles it like > > > > I don't think this is very right as you may fail for different > reasons other than a missing range property, e.g: > http://lxr.free-electrons.com/source/drivers/of/address.c#L575 > > And even if the only failure case was a missing range if in the > future __of_translate_address had to be reworked we would again > make a wrong assumption...you get my point? The newly introduced function would clearly have to make some sanity checks. The idea is that treat the case of not being able to translate a bus specific I/O address into a CPU address literally and fall back to another method of translating that address. This matches my mental model of how we find the resource: - start with the bus address - try to translate that into a CPU address - if we arrive at a CPU physical address for IORESOURCE_MEM, use that - if we arrive at a CPU physical address for IORESOURCE_IO, translate that into a Linux IORESOURCE_IO token - if there is no valid CPU physical address, try to translate the address into an IORESOURCE_IO using the ISA accessor - if that fails too, give up. If you try to fake a CPU physical address inbetween, it just gets more confusing. Arnd