From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt0-f194.google.com ([209.85.216.194]:36015 "EHLO mail-qt0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933445AbcKVXan (ORCPT ); Tue, 22 Nov 2016 18:30:43 -0500 Received: by mail-qt0-f194.google.com with SMTP id n34so4683535qtb.3 for ; Tue, 22 Nov 2016 15:30:12 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: <20161119012836.GA2543@localhost> From: Olof Johansson Date: Tue, 22 Nov 2016 15:30:10 -0800 Message-ID: Subject: Re: [GIT PULL] Second Round of Renesas ARM Based SoC Updates for v4.10 To: Geert Uytterhoeven Cc: Simon Horman , "arm@kernel.org" , Linux-Renesas , Kevin Hilman , Arnd Bergmann , "linux-arm-kernel@lists.infradead.org" , Magnus Damm Content-Type: text/plain; charset=UTF-8 Sender: linux-renesas-soc-owner@vger.kernel.org List-ID: On Tue, Nov 22, 2016 at 1:56 AM, Geert Uytterhoeven wrote: > Hi Olof, > > On Mon, Nov 21, 2016 at 5:35 PM, Olof Johansson wrote: >> On Mon, Nov 21, 2016 at 8:27 AM, Geert Uytterhoeven >> wrote: >>> On Mon, Nov 21, 2016 at 5:19 PM, Olof Johansson wrote: >>>> On Mon, Nov 21, 2016 at 1:31 AM, Geert Uytterhoeven >>>> wrote: >>>>> On Sat, Nov 19, 2016 at 2:28 AM, Olof Johansson wrote: >>>>>> On Thu, Nov 17, 2016 at 02:34:25PM +0100, Simon Horman wrote: >>>>>>> Please consider these second round of Renesas ARM based SoC updates for v4.10. >>>>> >>>>>>> * Basic support for r8a7745 SoC >>>>>>> >>>>>>> ---------------------------------------------------------------- >>>>>>> Sergei Shtylyov (2): >>>>>>> ARM: shmobile: r8a7745: basic SoC support >>>>>>> ARM: shmobile: document SK-RZG1E board >>>>>> >>>>>> Is there a reason you're adding a config option per SoC? >>>>>> >>>>>> I think you'd be better off not adding these config options, and just adding >>>>>> support for the SoCs through compatibles (and adding the drivers to defconfigs, >>>>>> etc). >>>>> >>>>> Yes there is a reason: kernel size. >>>>> The main offenders are the pinctrl tables, which add ca. 20-50 KiB per >>>>> supported SoC. >>>> >>>> So don't turn on that pinctrl driver unless you have that SoC? >>> >>> The enablement of the pinctrl driver (and the clock driver, FWIW) is controlled >>> by the SoC Kconfig symbol. If you want support for the SoC, you want the >>> pinctrl driver, too. >> >> Oh, that's trivial to fix! Do as almost all other SoCs do, and don't >> use silent options. > > What does that gain us? The ability to enable support for an SoC, without > enabling the accompanying pinctrl driver, leading to a non-booting system? It doesn't enable anything new, it just makes it less awkward for you to add new SoCs in the future without creating dependencies on new Kconfig symbols in the arch directory. > As soon as you have any pinctrl properties in the DT, you need the pinctrl > driver. Unless you disable CONFIG_PINCTRL (it's selected, and not > user-controlled), and rely on fragile reset state/boot loader. > > Pinctrl (and clock and irqchip) on-SoC drivers are special: if you fail to > include them, the system won't boot. This isn't about booting without a pinctrl driver. It's about avoiding adding new config symbols when they're not needed. I started out comparing your way of using config options with, for example, Exynos that has a bunch of different SoCs enabled. Having dependencies described clearly has some value, since it can be hard to know if you can turn off a driver and still have a bootable system with some of the other platforms. That being said, I think you should look at changing how you use your config options to make it less messy to add new ones: Looking around a bit, I noticed CLK_RENESAS_CPG_MSTP. This config option is used awkwardly -- what you should have is a (silent) config option that it depends on (such as {NEEDS|HAS}_CLK_CPG_MSTP or similar), that each platform needing this can select. Then CLK_RENESAS_CPG_MSTP only depends on that. No more need to touch this config entry for every new SoC that is enabled. Also, right now drivers/clk/renesas/Makefile is all keyed off of the SoC config option, when instead you should consider a CLK_* option to avoid duplication like you have today. For the shared platforms you can do it just as above with HAS_.* config entries. Again, for some of these you won't have to touch the clk/ directory at all any more for new SoCs, the way the rcar2 ones have been made! pinctrl config seems like it's a bit heavy on boiler plate as well. Most of the entries do the same things. Many other platforms instead select the pinctrl driver from the SoC Kconfig entry, maybe this is a good approach here too, and avoids more boilerplate in the driver directory. Of course, legacy SH platforms can probably be left alone as they are though. -Olof From mboxrd@z Thu Jan 1 00:00:00 1970 From: olof@lixom.net (Olof Johansson) Date: Tue, 22 Nov 2016 15:30:10 -0800 Subject: [GIT PULL] Second Round of Renesas ARM Based SoC Updates for v4.10 In-Reply-To: References: <20161119012836.GA2543@localhost> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, Nov 22, 2016 at 1:56 AM, Geert Uytterhoeven wrote: > Hi Olof, > > On Mon, Nov 21, 2016 at 5:35 PM, Olof Johansson wrote: >> On Mon, Nov 21, 2016 at 8:27 AM, Geert Uytterhoeven >> wrote: >>> On Mon, Nov 21, 2016 at 5:19 PM, Olof Johansson wrote: >>>> On Mon, Nov 21, 2016 at 1:31 AM, Geert Uytterhoeven >>>> wrote: >>>>> On Sat, Nov 19, 2016 at 2:28 AM, Olof Johansson wrote: >>>>>> On Thu, Nov 17, 2016 at 02:34:25PM +0100, Simon Horman wrote: >>>>>>> Please consider these second round of Renesas ARM based SoC updates for v4.10. >>>>> >>>>>>> * Basic support for r8a7745 SoC >>>>>>> >>>>>>> ---------------------------------------------------------------- >>>>>>> Sergei Shtylyov (2): >>>>>>> ARM: shmobile: r8a7745: basic SoC support >>>>>>> ARM: shmobile: document SK-RZG1E board >>>>>> >>>>>> Is there a reason you're adding a config option per SoC? >>>>>> >>>>>> I think you'd be better off not adding these config options, and just adding >>>>>> support for the SoCs through compatibles (and adding the drivers to defconfigs, >>>>>> etc). >>>>> >>>>> Yes there is a reason: kernel size. >>>>> The main offenders are the pinctrl tables, which add ca. 20-50 KiB per >>>>> supported SoC. >>>> >>>> So don't turn on that pinctrl driver unless you have that SoC? >>> >>> The enablement of the pinctrl driver (and the clock driver, FWIW) is controlled >>> by the SoC Kconfig symbol. If you want support for the SoC, you want the >>> pinctrl driver, too. >> >> Oh, that's trivial to fix! Do as almost all other SoCs do, and don't >> use silent options. > > What does that gain us? The ability to enable support for an SoC, without > enabling the accompanying pinctrl driver, leading to a non-booting system? It doesn't enable anything new, it just makes it less awkward for you to add new SoCs in the future without creating dependencies on new Kconfig symbols in the arch directory. > As soon as you have any pinctrl properties in the DT, you need the pinctrl > driver. Unless you disable CONFIG_PINCTRL (it's selected, and not > user-controlled), and rely on fragile reset state/boot loader. > > Pinctrl (and clock and irqchip) on-SoC drivers are special: if you fail to > include them, the system won't boot. This isn't about booting without a pinctrl driver. It's about avoiding adding new config symbols when they're not needed. I started out comparing your way of using config options with, for example, Exynos that has a bunch of different SoCs enabled. Having dependencies described clearly has some value, since it can be hard to know if you can turn off a driver and still have a bootable system with some of the other platforms. That being said, I think you should look at changing how you use your config options to make it less messy to add new ones: Looking around a bit, I noticed CLK_RENESAS_CPG_MSTP. This config option is used awkwardly -- what you should have is a (silent) config option that it depends on (such as {NEEDS|HAS}_CLK_CPG_MSTP or similar), that each platform needing this can select. Then CLK_RENESAS_CPG_MSTP only depends on that. No more need to touch this config entry for every new SoC that is enabled. Also, right now drivers/clk/renesas/Makefile is all keyed off of the SoC config option, when instead you should consider a CLK_* option to avoid duplication like you have today. For the shared platforms you can do it just as above with HAS_.* config entries. Again, for some of these you won't have to touch the clk/ directory at all any more for new SoCs, the way the rcar2 ones have been made! pinctrl config seems like it's a bit heavy on boiler plate as well. Most of the entries do the same things. Many other platforms instead select the pinctrl driver from the SoC Kconfig entry, maybe this is a good approach here too, and avoids more boilerplate in the driver directory. Of course, legacy SH platforms can probably be left alone as they are though. -Olof