[PATCH 0/3] gpio: brcmstb: Misc changes

[PATCH 0/3] gpio: brcmstb: Misc changes

[Florian Fainelli]

Hi Linus, Alexandre,

This patch series updates the brcmstb GPIO driver to support the following:

- move the initialization to subsys_initcall to allow using this driver with
  GPIO-driven regulators
- properly configure the driver for big-endian MIPS operation
- enable the driver for MIPS-based brcmstb chips

Thank you!

Florian Fainelli (2):
  gpio: brcmstb: Set endian flags for big-endian MIPS
  gpio: brcmstb: Allow building driver for BMIPS_GENERIC

Jim Quinlan (1):
  gpio: brcmstb: have driver register during subsys_initcall()

 drivers/gpio/Kconfig        |  4 ++--
 drivers/gpio/gpio-brcmstb.c | 28 ++++++++++++++++++++++++++--
 2 files changed, 28 insertions(+), 4 deletions(-)

-- 
2.1.0

[PATCH 1/3] gpio: brcmstb: have driver register during subsys_initcall()

[Florian Fainelli]

From: Jim Quinlan <jim2101024@gmail.com>

Because regulators are started with subsys_initcall(), and gpio references may
be contained in the regulators, it makes sense to start the brcmstb-gpio's with
a subsys_initcall(). The order within the drivers/Makefile ensures that the
gpio initialization happens prior to the regulator's initialization.

We need to unroll module_platform_driver() now to allow this and have custom
exit and init module functions to control the initialization level.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/gpio/gpio-brcmstb.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index dc3f0395693b..3618b9fd0cba 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -535,7 +535,18 @@ static struct platform_driver brcmstb_gpio_driver = {
 	.probe = brcmstb_gpio_probe,
 	.remove = brcmstb_gpio_remove,
 };
-module_platform_driver(brcmstb_gpio_driver);
+
+static int __init brcmstb_gpio_init(void)
+{
+	return platform_driver_register(&brcmstb_gpio_driver);
+}
+subsys_initcall(brcmstb_gpio_init);
+
+static void __exit brcmstb_gpio_exit(void)
+{
+	platform_driver_unregister(&brcmstb_gpio_driver);
+}
+module_exit(brcmstb_gpio_exit);
 
 MODULE_AUTHOR("Gregory Fong");
 MODULE_DESCRIPTION("Driver for Broadcom BRCMSTB SoC UPG GPIO");
-- 
2.1.0

Re: [PATCH 1/3] gpio: brcmstb: have driver register during subsys_initcall()

[Gregory Fong]

Hello Florian and Jim,

On Wed, Jan 6, 2016 at 10:55 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> From: Jim Quinlan <jim2101024@gmail.com>
>
> Because regulators are started with subsys_initcall(), and gpio references may
> be contained in the regulators, it makes sense to start the brcmstb-gpio's with
> a subsys_initcall(). The order within the drivers/Makefile ensures that the
> gpio initialization happens prior to the regulator's initialization.
>
> We need to unroll module_platform_driver() now to allow this and have custom
> exit and init module functions to control the initialization level.

If gpio pins are needed for a regulator to come up, wouldn't it be
better to handle this using deferred probe instead of initcall-based
initialization?  Deferred probe has its problems, but I was under the
impression that it's the encouraged way to resolve these sort of
initialization order issues.

Best regards,
Gregory

Re: [PATCH 1/3] gpio: brcmstb: have driver register during subsys_initcall()

[Florian Fainelli]

On 06/01/16 22:05, Gregory Fong wrote:
> Hello Florian and Jim,
> 
> On Wed, Jan 6, 2016 at 10:55 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> From: Jim Quinlan <jim2101024@gmail.com>
>>
>> Because regulators are started with subsys_initcall(), and gpio references may
>> be contained in the regulators, it makes sense to start the brcmstb-gpio's with
>> a subsys_initcall(). The order within the drivers/Makefile ensures that the
>> gpio initialization happens prior to the regulator's initialization.
>>
>> We need to unroll module_platform_driver() now to allow this and have custom
>> exit and init module functions to control the initialization level.
> 
> If gpio pins are needed for a regulator to come up, wouldn't it be
> better to handle this using deferred probe instead of initcall-based
> initialization?  Deferred probe has its problems, but I was under the
> impression that it's the encouraged way to resolve these sort of
> initialization order issues.

To give you some more context associated with this change, we now have
some boards which have GPIO-connected regulators to turn on/off PCIe
endpoint devices. In the downstream kernel, and with lack of a better
solution for now, we ended-up having the PCIE Root Complex driver to
claim these regulator, and flip them on shortly before attempting a bus
scan.

If we used deferred probing, I am assuming the sequence of events could
go like this:

- PCIe driver gets initialized, looks for regulators, cannot get a
handle on them, gets EPROBE_DEFER (arch_initcall right now)
- regulator subsystem gets initialized, does not have a valid GPIO
provider driver yet, returns EPROBE_DEFER (subsys_initcall)
- GPIO provider (gpio-brcmstb) finally gets probed and registered,
regulator get registered and available, PCIe RC driver can now use them
and power on the PCIE end point (module_initcall)

I suppose this might be working actually, let me go back to the white
board and look at this with Jim.
-- 
Florian

Re: [PATCH 1/3] gpio: brcmstb: have driver register during subsys_initcall()

[Jim Quinlan]

Hi, sorry I am late to this thead.  Our brcmstb PCIe code is using
platform_driver_probe() which does not tolerate EPROBE_DEFER. Also, I
do not see any "cost" associated with having gpio-brcmstb.c using
subsys_initcall(); in fact, I see 33 gpio-*.c files that use
subsys_initcall().  Thanks, Jim

On Thu, Jan 7, 2016 at 1:12 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 06/01/16 22:05, Gregory Fong wrote:
>> Hello Florian and Jim,
>>
>> On Wed, Jan 6, 2016 at 10:55 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> From: Jim Quinlan <jim2101024@gmail.com>
>>>
>>> Because regulators are started with subsys_initcall(), and gpio references may
>>> be contained in the regulators, it makes sense to start the brcmstb-gpio's with
>>> a subsys_initcall(). The order within the drivers/Makefile ensures that the
>>> gpio initialization happens prior to the regulator's initialization.
>>>
>>> We need to unroll module_platform_driver() now to allow this and have custom
>>> exit and init module functions to control the initialization level.
>>
>> If gpio pins are needed for a regulator to come up, wouldn't it be
>> better to handle this using deferred probe instead of initcall-based
>> initialization?  Deferred probe has its problems, but I was under the
>> impression that it's the encouraged way to resolve these sort of
>> initialization order issues.
>
> To give you some more context associated with this change, we now have
> some boards which have GPIO-connected regulators to turn on/off PCIe
> endpoint devices. In the downstream kernel, and with lack of a better
> solution for now, we ended-up having the PCIE Root Complex driver to
> claim these regulator, and flip them on shortly before attempting a bus
> scan.
>
> If we used deferred probing, I am assuming the sequence of events could
> go like this:
>
> - PCIe driver gets initialized, looks for regulators, cannot get a
> handle on them, gets EPROBE_DEFER (arch_initcall right now)
> - regulator subsystem gets initialized, does not have a valid GPIO
> provider driver yet, returns EPROBE_DEFER (subsys_initcall)
> - GPIO provider (gpio-brcmstb) finally gets probed and registered,
> regulator get registered and available, PCIe RC driver can now use them
> and power on the PCIE end point (module_initcall)
>
> I suppose this might be working actually, let me go back to the white
> board and look at this with Jim.
> --
> Florian

Re: [PATCH 1/3] gpio: brcmstb: have driver register during subsys_initcall()

[Gregory Fong]

Hi Jim,

On Tue, Jan 19, 2016 at 1:18 PM, Jim Quinlan <jim2101024@gmail.com> wrote:
> Hi, sorry I am late to this thead.  Our brcmstb PCIe code is using
> platform_driver_probe() which does not tolerate EPROBE_DEFER.

Can't that be changed?  For instance, see this changeset which was
merged, particularly patches 3 and 4:
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-August/191612.html

> Also, I
> do not see any "cost" associated with having gpio-brcmstb.c using
> subsys_initcall(); in fact, I see 33 gpio-*.c files that use
> subsys_initcall().

I've been seeing some recent patches merged that remove uses of
subsys_initcall() in favor of deferred probe.  But I acknowledge it
may have some problems:
- may slow boot time because it just hacks around the dependency
problem, not really taking the required order into account
- may waste memory for non-hotpluggable devices (like PCIe root
complex) by requiring that the initialization functions be kept in
memory for the case where probe must be deferred

This isn't advising against using subsys_initcall() in the brcmstb
downstream tree as a stopgap.  But I'd rather we don't contribute to
the manual initcall mess without a very good reason, and since
1. it sounds like there's a way to write the pcie root complex driver
to use deferred probe,
2. I'm not fully convinced that the cost of writing in a way to handle
deferred probe is that high vs platform_driver_probe(), and
3. the one driver that would benefit from this isn't being submitted
upstream at this time,
I don't think this patch should be applied right now.

Aside: It looks like there were some good ideas thrown around after
the 2015 Kernel Summit last November for better handling of device
dependencies: https://lwn.net/Articles/662820/ .  Not sure what's
developed since then.

Best regards,
Gregory

>
> On Thu, Jan 7, 2016 at 1:12 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 06/01/16 22:05, Gregory Fong wrote:
>>> Hello Florian and Jim,
>>>
>>> On Wed, Jan 6, 2016 at 10:55 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>> From: Jim Quinlan <jim2101024@gmail.com>
>>>>
>>>> Because regulators are started with subsys_initcall(), and gpio references may
>>>> be contained in the regulators, it makes sense to start the brcmstb-gpio's with
>>>> a subsys_initcall(). The order within the drivers/Makefile ensures that the
>>>> gpio initialization happens prior to the regulator's initialization.
>>>>
>>>> We need to unroll module_platform_driver() now to allow this and have custom
>>>> exit and init module functions to control the initialization level.
>>>
>>> If gpio pins are needed for a regulator to come up, wouldn't it be
>>> better to handle this using deferred probe instead of initcall-based
>>> initialization?  Deferred probe has its problems, but I was under the
>>> impression that it's the encouraged way to resolve these sort of
>>> initialization order issues.
>>
>> To give you some more context associated with this change, we now have
>> some boards which have GPIO-connected regulators to turn on/off PCIe
>> endpoint devices. In the downstream kernel, and with lack of a better
>> solution for now, we ended-up having the PCIE Root Complex driver to
>> claim these regulator, and flip them on shortly before attempting a bus
>> scan.
>>
>> If we used deferred probing, I am assuming the sequence of events could
>> go like this:
>>
>> - PCIe driver gets initialized, looks for regulators, cannot get a
>> handle on them, gets EPROBE_DEFER (arch_initcall right now)
>> - regulator subsystem gets initialized, does not have a valid GPIO
>> provider driver yet, returns EPROBE_DEFER (subsys_initcall)
>> - GPIO provider (gpio-brcmstb) finally gets probed and registered,
>> regulator get registered and available, PCIe RC driver can now use them
>> and power on the PCIE end point (module_initcall)
>>
>> I suppose this might be working actually, let me go back to the white
>> board and look at this with Jim.
>> --
>> Florian

Re: [PATCH 1/3] gpio: brcmstb: have driver register during subsys_initcall()

[Jim Quinlan]

Hi Gregory,
I don't want to use deferred probes as I think I can move the PCIe RC
driver to use module_platform_driver().  It originally used
arch_initcall(), but that was for legacy reasons; in it's previous
life it was a MIPs PCIe driver, and all of the MIPs PCIe drivers use
arch/postcore_initcall() because of the way the Linux MIPs PCIe
infrastructure works.  So, assuming I can do this, we do not need this
patch.
Thanks, Jim

On Wed, Jan 20, 2016 at 4:40 AM, Gregory Fong <gregory.0xf0@gmail.com> wrote:
> Hi Jim,
>
> On Tue, Jan 19, 2016 at 1:18 PM, Jim Quinlan <jim2101024@gmail.com> wrote:
>> Hi, sorry I am late to this thead.  Our brcmstb PCIe code is using
>> platform_driver_probe() which does not tolerate EPROBE_DEFER.
>
> Can't that be changed?  For instance, see this changeset which was
> merged, particularly patches 3 and 4:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-August/191612.html
>
>> Also, I
>> do not see any "cost" associated with having gpio-brcmstb.c using
>> subsys_initcall(); in fact, I see 33 gpio-*.c files that use
>> subsys_initcall().
>
> I've been seeing some recent patches merged that remove uses of
> subsys_initcall() in favor of deferred probe.  But I acknowledge it
> may have some problems:
> - may slow boot time because it just hacks around the dependency
> problem, not really taking the required order into account
> - may waste memory for non-hotpluggable devices (like PCIe root
> complex) by requiring that the initialization functions be kept in
> memory for the case where probe must be deferred
>
> This isn't advising against using subsys_initcall() in the brcmstb
> downstream tree as a stopgap.  But I'd rather we don't contribute to
> the manual initcall mess without a very good reason, and since
> 1. it sounds like there's a way to write the pcie root complex driver
> to use deferred probe,
> 2. I'm not fully convinced that the cost of writing in a way to handle
> deferred probe is that high vs platform_driver_probe(), and
> 3. the one driver that would benefit from this isn't being submitted
> upstream at this time,
> I don't think this patch should be applied right now.
>
> Aside: It looks like there were some good ideas thrown around after
> the 2015 Kernel Summit last November for better handling of device
> dependencies: https://lwn.net/Articles/662820/ .  Not sure what's
> developed since then.
>
> Best regards,
> Gregory
>
>>
>> On Thu, Jan 7, 2016 at 1:12 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> On 06/01/16 22:05, Gregory Fong wrote:
>>>> Hello Florian and Jim,
>>>>
>>>> On Wed, Jan 6, 2016 at 10:55 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>> From: Jim Quinlan <jim2101024@gmail.com>
>>>>>
>>>>> Because regulators are started with subsys_initcall(), and gpio references may
>>>>> be contained in the regulators, it makes sense to start the brcmstb-gpio's with
>>>>> a subsys_initcall(). The order within the drivers/Makefile ensures that the
>>>>> gpio initialization happens prior to the regulator's initialization.
>>>>>
>>>>> We need to unroll module_platform_driver() now to allow this and have custom
>>>>> exit and init module functions to control the initialization level.
>>>>
>>>> If gpio pins are needed for a regulator to come up, wouldn't it be
>>>> better to handle this using deferred probe instead of initcall-based
>>>> initialization?  Deferred probe has its problems, but I was under the
>>>> impression that it's the encouraged way to resolve these sort of
>>>> initialization order issues.
>>>
>>> To give you some more context associated with this change, we now have
>>> some boards which have GPIO-connected regulators to turn on/off PCIe
>>> endpoint devices. In the downstream kernel, and with lack of a better
>>> solution for now, we ended-up having the PCIE Root Complex driver to
>>> claim these regulator, and flip them on shortly before attempting a bus
>>> scan.
>>>
>>> If we used deferred probing, I am assuming the sequence of events could
>>> go like this:
>>>
>>> - PCIe driver gets initialized, looks for regulators, cannot get a
>>> handle on them, gets EPROBE_DEFER (arch_initcall right now)
>>> - regulator subsystem gets initialized, does not have a valid GPIO
>>> provider driver yet, returns EPROBE_DEFER (subsys_initcall)
>>> - GPIO provider (gpio-brcmstb) finally gets probed and registered,
>>> regulator get registered and available, PCIe RC driver can now use them
>>> and power on the PCIE end point (module_initcall)
>>>
>>> I suppose this might be working actually, let me go back to the white
>>> board and look at this with Jim.
>>> --
>>> Florian

Re: [PATCH 1/3] gpio: brcmstb: have driver register during subsys_initcall()

[Linus Walleij]

On Wed, Jan 6, 2016 at 7:55 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:

> From: Jim Quinlan <jim2101024@gmail.com>
>
> Because regulators are started with subsys_initcall(), and gpio references may
> be contained in the regulators, it makes sense to start the brcmstb-gpio's with
> a subsys_initcall(). The order within the drivers/Makefile ensures that the
> gpio initialization happens prior to the regulator's initialization.
>
> We need to unroll module_platform_driver() now to allow this and have custom
> exit and init module functions to control the initialization level.
>
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

I'm holding this back until the initcall ordering discussion is
resolved, the other two patches are applied.

Yours,
Linus Walleij

[PATCH 2/3] gpio: brcmstb: Set endian flags for big-endian MIPS

[Florian Fainelli]

Broadcom MIPS-based STB chips endianness is configured by boot strap,
which also reverses all bus endianness (i.e., big-endian CPU + big
endian bus ==> native endian I/O).

Other architectures (e.g., ARM) either do not support big endian, or
else leave I/O in little endian mode.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/gpio/gpio-brcmstb.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index 3618b9fd0cba..8e8ddc76a56f 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -409,6 +409,7 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 	int num_banks = 0;
 	int err;
 	static int gpio_base;
+	unsigned long flags = 0;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -438,6 +439,18 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 	if (brcmstb_gpio_sanity_check_banks(dev, np, res))
 		return -EINVAL;
 
+	/*
+	 * MIPS endianness is configured by boot strap, which also reverses all
+	 * bus endianness (i.e., big-endian CPU + big endian bus ==> native
+	 * endian I/O).
+	 *
+	 * Other architectures (e.g., ARM) either do not support big endian, or
+	 * else leave I/O in little endian mode.
+	 */
+#if defined(CONFIG_MIPS) && defined(__BIG_ENDIAN)
+	flags = BGPIOF_BIG_ENDIAN_BYTE_ORDER;
+#endif
+
 	of_property_for_each_u32(np, "brcm,gpio-bank-widths", prop, p,
 			bank_width) {
 		struct brcmstb_gpio_bank *bank;
@@ -466,7 +479,7 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 		err = bgpio_init(gc, dev, 4,
 				reg_base + GIO_DATA(bank->id),
 				NULL, NULL, NULL,
-				reg_base + GIO_IODIR(bank->id), 0);
+				reg_base + GIO_IODIR(bank->id), flags);
 		if (err) {
 			dev_err(dev, "bgpio_init() failed\n");
 			goto fail;
-- 
2.1.0

Re: [PATCH 2/3] gpio: brcmstb: Set endian flags for big-endian MIPS

[Gregory Fong]

On Wed, Jan 6, 2016 at 10:55 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> Broadcom MIPS-based STB chips endianness is configured by boot strap,
> which also reverses all bus endianness (i.e., big-endian CPU + big
> endian bus ==> native endian I/O).
>
> Other architectures (e.g., ARM) either do not support big endian, or
> else leave I/O in little endian mode.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Acked-by: Gregory Fong <gregory.0xf0@gmail.com>

Re: [PATCH 2/3] gpio: brcmstb: Set endian flags for big-endian MIPS

[Linus Walleij]

On Wed, Jan 6, 2016 at 7:55 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:

> Broadcom MIPS-based STB chips endianness is configured by boot strap,
> which also reverses all bus endianness (i.e., big-endian CPU + big
> endian bus ==> native endian I/O).
>
> Other architectures (e.g., ARM) either do not support big endian, or
> else leave I/O in little endian mode.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Patch applied with Gregory's ACK.

Yours,
Linus Walleij

[PATCH 3/3] gpio: brcmstb: Allow building driver for BMIPS_GENERIC

[Florian Fainelli]

BMIPS_GENERIC (arch/mips/bmips) is the Kconfig symbol associated with
Broadcom MIPS-based STB chips. Since this driver is perfectly usable on
these platforms as well, allow using it.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/gpio/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b60f40a423f3..cb212ebb39ff 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -134,8 +134,8 @@ config GPIO_BCM_KONA
 
 config GPIO_BRCMSTB
 	tristate "BRCMSTB GPIO support"
-	default y if ARCH_BRCMSTB
-	depends on OF_GPIO && (ARCH_BRCMSTB || COMPILE_TEST)
+	default y if (ARCH_BRCMSTB || BMIPS_GENERIC)
+	depends on OF_GPIO && (ARCH_BRCMSTB || BMIPS_GENERIC || COMPILE_TEST)
 	select GPIO_GENERIC
 	select GPIOLIB_IRQCHIP
 	help
-- 
2.1.0

Re: [PATCH 3/3] gpio: brcmstb: Allow building driver for BMIPS_GENERIC

[Gregory Fong]

On Wed, Jan 6, 2016 at 10:55 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> BMIPS_GENERIC (arch/mips/bmips) is the Kconfig symbol associated with
> Broadcom MIPS-based STB chips. Since this driver is perfectly usable on
> these platforms as well, allow using it.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Acked-by: Gregory Fong <gregory.0xf0@gmail.com>

I never did get to test this out myself on big-endian bmips.  Glad it
worked after the bus endianness fix in patch 2/3.

Re: [PATCH 3/3] gpio: brcmstb: Allow building driver for BMIPS_GENERIC

[Linus Walleij]

On Wed, Jan 6, 2016 at 7:55 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:

> BMIPS_GENERIC (arch/mips/bmips) is the Kconfig symbol associated with
> Broadcom MIPS-based STB chips. Since this driver is perfectly usable on
> these platforms as well, allow using it.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Patch applied with Gregory's ACK.

Yours,
Linus Walleij