From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755445AbcKJLhy (ORCPT ); Thu, 10 Nov 2016 06:37:54 -0500 Received: from mout.kundenserver.de ([217.72.192.74]:56219 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754704AbcKJLhw (ORCPT ); Thu, 10 Nov 2016 06:37:52 -0500 From: Arnd Bergmann To: Geert Uytterhoeven Cc: "linuxppc-dev@lists.ozlabs.org" , Geert Uytterhoeven , Greg Kroah-Hartman , Yangbo Lu , Simon Horman , Magnus Damm , Rob Herring , Mark Rutland , "devicetree@vger.kernel.org" , Dirk Behme , "linux-kernel@vger.kernel.org" , Linux-Renesas , "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH v2 7/7] soc: renesas: Identify SoC and register with the SoC bus Date: Thu, 10 Nov 2016 12:37:01 +0100 Message-ID: <25057157.YlGi2v6RrE@wuerfel> User-Agent: KMail/5.1.3 (Linux/4.4.0-34-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: References: <1477913455-5314-1-git-send-email-geert+renesas@glider.be> <3693445.TSdZGaiT9S@wuerfel> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:6il1aUVGDA4NDlQmm0zd89caWoeV8+MTL6MYBUGhZcacG3M3IiJ EjRU6oroAcOO9/shfhRzfva7oEtq7kIR5lo5HRq5Hp3QlLiYi+WXxbmDpK30norN6vTIgRV IckjDpYvt+kKJ/vW/oUupq306t8jrC75Ge54rfhr2wK0I6NzLcpwP6vjZ1AQNpPqZxjtJcv O6+retPxBdBs75mBXAVQg== X-UI-Out-Filterresults: notjunk:1;V01:K0:FE3J0Cg86Kg=:kULi9HqJy1RcYZQnW6pCBO RAsNWnXiX0vMY8tCOcShT/tQDJFQvc1v4hVC3cuZAmmmcdJQv+f8DdrwPoRHJ7wzNSx4GOakH lvBcBwrhuXvbdh6nkNK9Hc89t/R/Hzl/8Zj+fFRhhJJUTH6YBYB3w9f4O1qkzMdbaJ/GKSCf5 4FYZma40W1B6mpdMbILmJB7uwtDujb/ShbgPy18ScwsOncI3KKzJIDdcdJeafbSceXrsImVtV WUgzO9PWKa5ePc4qn2z6XjiWRPxqOvUfO36siBFz15tHLUGnRenXWL+6g/LnOjfAXUBqds/lE TmFmEHNzhfT0E5MSopT28z2f/ExrMYCmQWb/0J0VF7BgLLqXaGDAlYpTHkHCQefXskRnHqSMd xouQie14NgBDwh/aCXkFU0fy55wQ486r6iGAS9wtBgk0CSexXS5tMTsWQymy5aTrOCFlad1k7 ByQBYlOR8EzBkwosYVjPri91tcDqA4PHVT60KcIKeAjqGucIDoH4dxTp2fabloTM9yxxslbXQ wz9xR+vqOmOtd54g5s4hYQ+B/6EIszKABuuOFFIORykfK7xeM4VbkouETCAv7AE2sS2j3Idf1 EFgPr82LPAldF383XOM3SDF0tpyd0Bfk3lanENQzx3ftOqe2gekqLOA07DuBgJjKjF1Cli0TY gUA7SOsKAZ/T0Smw+YfVvSc29GFVWhyfBU58xDUNmSGwJCCSGFMLGAQ7vjNZc+40KBLqFSRNt Dt1HQ3+1hPPK1yUx Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thursday, November 10, 2016 11:19:20 AM CET Geert Uytterhoeven wrote: > Hi Arnd, > > Thanks for your comments! > > On Wed, Nov 9, 2016 at 5:55 PM, Arnd Bergmann wrote: > > On Monday, October 31, 2016 12:30:55 PM CET Geert Uytterhoeven wrote: > >> v2: > >> - Drop SoC families and family names; use fixed "Renesas" instead, > > > > I think I'd rather have seen the family names left in there, but it's > > not important, so up to you. > > They're not useful for matching, as family names may change anytime, and don't > always say much about the hardware capabilities. > E.g. SH-Mobile -> R-Mobile -> R-Car | RZ/A | RZ/G > Some SH-Mobile (even some R-Car) parts are SuperH only, others have ARM and > SuperH. > > At least the SoC part numbers are stable (hmm, sh73a0 == r8a73a0). I think the marketing names are much more useful for humans looking at the sysfs files than the kernel doing matching on, but both use cases are important. > >> - Use "renesas,prr" and "renesas,cccr" device nodes in DT if > >> available, else fall back to hardcoded addresses for compatibility > >> with existing DTBs, > > > > I only see patches 2, 3, 5, and 7 in my inbox, so I'm lacking the DT > > binding for these, among other things. > > I understand you've received them in the mean time? Yes, I found them in my inbox later, not sure why I didn't see them at first. > > It does seem wrong to have a device node for a specific register though. > > Shouldn't the node be for the block of registers that these are inside > > of? > > On R-Mobile APE6, R-Car Gen2 and Gen3, PRR is a lone register. > On R-Car Gen1, it's not even documented (and doesn't exist on all parts). It just seems odd to have it at address 0xff000044 when all the other devices are at page-aligned addresses. Do you mean that accessing 0xff000040 or 0xff000048 will result in a bus-level exception for a missing register and just 0xff000044 is actually valid for access, or is it just the only thing that is documented? > On SH-Mobile/R-Mobile, CCCR may be part of the HPB/APB register block, which > we further don't touch at all. > On R-Car Gen2, it's not documented, but does exist. This is where the family names would come in handy ;-) I now have no idea which chip(s) you are referring to. If you know the name of the register block, just put it into DT with that name. The driver can trivially add the right offset. > >> - Don't register the SoC bus if the chip ID register is missing, > > > > Why? My objection was to hardcoding the register, not to registering > > the device? I think I'd rather see the device registered with an > > empty revision string. > > If there's no chip ID register, there's no reason to use soc_device_match(), > as we can always look at a compatible value. All SoCs listed in this driver > have a chip ID register. But you may still have user space tools looking into sysfs, e.g. to figure out how to install a kernel that the boot loader can find, or which hardware specific distro packages to install. > if you want me to register the soc_bus for those SoCs regardless, I want to > re-add r7s72100 (RZ/A) and r8a7778 (R-Car M1A), who don't have chip ID > registers ;-) Right. Just don't encode too much knowledge about the SoCs into the driver, so we are prepared for adding new ones: We should still look for the registers in DT on all chips. > >> +#define CCCR 0xe600101c /* Common Chip Code Register */ > >> +#define PRR 0xff000044 /* Product Register */ > >> +#define PRR3 0xfff00044 /* Product Register on R-Car Gen3 */ > >> + > >> +static const struct of_device_id renesas_socs[] __initconst = { > >> +#ifdef CONFIG_ARCH_R8A73A4 > >> + { .compatible = "renesas,r8a73a4", .data = (void *)PRR, }, > >> +#endif > >> +#ifdef CONFIG_ARCH_R8A7740 > >> + { .compatible = "renesas,r8a7740", .data = (void *)CCCR, }, > >> +#endif > > > > My preference here would be to list the register address only for > > SoCs that are known to need them, while also having .dtb files that > > don't have the nodes. > > Even if drivers don't have to handle differences, there's been a long > outstanding request to print SoC revision information during bootup > (E.g. "Does it still work on ES1.0?"). Hence that covers all SoCs. Ok, fair enough. > >> +static int __init renesas_soc_init(void) > >> +{ > >> + struct soc_device_attribute *soc_dev_attr; > >> + const struct of_device_id *match; > >> + void __iomem *chipid = NULL; > >> + struct soc_device *soc_dev; > >> + struct device_node *np; > >> + unsigned int product; > >> + > >> + np = of_find_matching_node_and_match(NULL, renesas_socs, &match); > >> + if (!np) > >> + return -ENODEV; > >> + > >> + of_node_put(np); > >> + > >> + /* Try PRR first, then CCCR, then hardcoded fallback */ > >> + np = of_find_compatible_node(NULL, NULL, "renesas,prr"); > >> + if (!np) > >> + np = of_find_compatible_node(NULL, NULL, "renesas,cccr"); > >> + if (np) { > >> + chipid = of_iomap(np, 0); > >> + of_node_put(np); > >> + } else if (match->data) { > >> + chipid = ioremap((uintptr_t)match->data, 4); > >> + } > >> + if (!chipid) > >> > > > > Here, I'd turn the order around and look for the DT nodes of the > > devices first. Only if they are not found, look at the compatible > > string of the root node. No need to search for a node though, > > you know which one it is when you look for a compatible = > > "renesas,r8a73a4". > > "renesas,r8a73a4" is the root node, not the device, so it does not have the > "reg" property for reading the chip ID? I mean replace of_find_matching_node_and_match() with of_match_node(renesas_socs, of_root). It does the same thing, just more efficiently. > There is no SoC part number in the "renesas,prr" and "renesas,cccr" nodes. > Hence I always need to look at the root nodes. Not sure what that would protect you from. Could you have a renesas,cccr Arnd