linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] ARM: bcm: nsp: gpio improvements (hopefully)
@ 2019-10-25  4:00 Chris Packham
  2019-10-25  4:00 ` [PATCH 1/2] pinctrl: bcm: nsp: use gpiolib infrastructure for interrupts Chris Packham
  2019-10-25  4:00 ` [PATCH 2/2] ARM: dts: NSP: avoid unnecessary probe deferrals Chris Packham
  0 siblings, 2 replies; 6+ messages in thread
From: Chris Packham @ 2019-10-25  4:00 UTC (permalink / raw)
  To: robh+dt, mark.rutland, rjui, sbranden, bcm-kernel-feedback-list,
	linus.walleij
  Cc: devicetree, Chris Packham, linux-kernel, linux-arm-kernel, linux-gpio

I'm working on a platform using the BCM 58525 SoC. I noticed that some of
the peripherals were being deferred (not that that's a problem) and debugfs
was complaining "File ':axi@18000000:gpio@20' in directory 'domains' already
present!" which is more of a sign that things were not right.

The debugfs error was because the manually created irq domain was not
cleaned up on failure (or deferral). The deferral was happening because the
pinctrl node had not been probed.

These two patches take care of these problems.

Chris Packham (2):
  pinctrl: bcm: nsp: use gpiolib infrastructure for interrupts
  ARM: dts: NSP: avoid unnecessary probe deferrals

 arch/arm/boot/dts/bcm-nsp.dtsi         |  14 ++--
 drivers/pinctrl/bcm/pinctrl-nsp-gpio.c | 105 ++++++++++---------------
 2 files changed, 49 insertions(+), 70 deletions(-)

-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] pinctrl: bcm: nsp: use gpiolib infrastructure for interrupts
  2019-10-25  4:00 [PATCH 0/2] ARM: bcm: nsp: gpio improvements (hopefully) Chris Packham
@ 2019-10-25  4:00 ` Chris Packham
  2019-10-25  4:00 ` [PATCH 2/2] ARM: dts: NSP: avoid unnecessary probe deferrals Chris Packham
  1 sibling, 0 replies; 6+ messages in thread
From: Chris Packham @ 2019-10-25  4:00 UTC (permalink / raw)
  To: robh+dt, mark.rutland, rjui, sbranden, bcm-kernel-feedback-list,
	linus.walleij
  Cc: devicetree, Chris Packham, linux-kernel, linux-arm-kernel, linux-gpio

Use more of the gpiolib infrastructure for handling interrupts. The
root interrupt still needs to be handled manually as it is shared with
other peripherals on the SoC.

This will allow multiple instances of this driver to be supported and
will clean up gracefully on failure thanks to the device managed APIs.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 drivers/pinctrl/bcm/pinctrl-nsp-gpio.c | 105 ++++++++++---------------
 1 file changed, 42 insertions(+), 63 deletions(-)

diff --git a/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c b/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
index e67ae52023ad..cf77c6fe9f9c 100644
--- a/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
+++ b/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
@@ -64,17 +64,16 @@
  * @gc: GPIO chip
  * @pctl: pointer to pinctrl_dev
  * @pctldesc: pinctrl descriptor
- * @irq_domain: pointer to irq domain
  * @lock: lock to protect access to I/O registers
  */
 struct nsp_gpio {
 	struct device *dev;
 	void __iomem *base;
 	void __iomem *io_ctrl;
+	struct irq_chip irqchip;
 	struct gpio_chip gc;
 	struct pinctrl_dev *pctl;
 	struct pinctrl_desc pctldesc;
-	struct irq_domain *irq_domain;
 	raw_spinlock_t lock;
 };
 
@@ -136,8 +135,8 @@ static inline bool nsp_get_bit(struct nsp_gpio *chip, enum base_type address,
 
 static irqreturn_t nsp_gpio_irq_handler(int irq, void *data)
 {
-	struct nsp_gpio *chip = (struct nsp_gpio *)data;
-	struct gpio_chip gc = chip->gc;
+	struct gpio_chip *gc = (struct gpio_chip *)data;
+	struct nsp_gpio *chip = gpiochip_get_data(gc);
 	int bit;
 	unsigned long int_bits = 0;
 	u32 int_status;
@@ -155,14 +154,14 @@ static irqreturn_t nsp_gpio_irq_handler(int irq, void *data)
 		level &= readl(chip->base + NSP_GPIO_INT_MASK);
 		int_bits = level | event;
 
-		for_each_set_bit(bit, &int_bits, gc.ngpio) {
+		for_each_set_bit(bit, &int_bits, gc->ngpio) {
 			/*
 			 * Clear the interrupt before invoking the
 			 * handler, so we do not leave any window
 			 */
 			writel(BIT(bit), chip->base + NSP_GPIO_EVENT);
 			generic_handle_irq(
-				irq_linear_revmap(chip->irq_domain, bit));
+				irq_linear_revmap(gc->irq.domain, bit));
 		}
 	}
 
@@ -171,7 +170,8 @@ static irqreturn_t nsp_gpio_irq_handler(int irq, void *data)
 
 static void nsp_gpio_irq_ack(struct irq_data *d)
 {
-	struct nsp_gpio *chip = irq_data_get_irq_chip_data(d);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct nsp_gpio *chip = gpiochip_get_data(gc);
 	unsigned gpio = d->hwirq;
 	u32 val = BIT(gpio);
 	u32 trigger_type;
@@ -189,7 +189,8 @@ static void nsp_gpio_irq_ack(struct irq_data *d)
  */
 static void nsp_gpio_irq_set_mask(struct irq_data *d, bool unmask)
 {
-	struct nsp_gpio *chip = irq_data_get_irq_chip_data(d);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct nsp_gpio *chip = gpiochip_get_data(gc);
 	unsigned gpio = d->hwirq;
 	u32 trigger_type;
 
@@ -202,7 +203,8 @@ static void nsp_gpio_irq_set_mask(struct irq_data *d, bool unmask)
 
 static void nsp_gpio_irq_mask(struct irq_data *d)
 {
-	struct nsp_gpio *chip = irq_data_get_irq_chip_data(d);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct nsp_gpio *chip = gpiochip_get_data(gc);
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&chip->lock, flags);
@@ -212,7 +214,8 @@ static void nsp_gpio_irq_mask(struct irq_data *d)
 
 static void nsp_gpio_irq_unmask(struct irq_data *d)
 {
-	struct nsp_gpio *chip = irq_data_get_irq_chip_data(d);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct nsp_gpio *chip = gpiochip_get_data(gc);
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&chip->lock, flags);
@@ -222,7 +225,8 @@ static void nsp_gpio_irq_unmask(struct irq_data *d)
 
 static int nsp_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 {
-	struct nsp_gpio *chip = irq_data_get_irq_chip_data(d);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct nsp_gpio *chip = gpiochip_get_data(gc);
 	unsigned gpio = d->hwirq;
 	bool level_low;
 	bool falling;
@@ -265,16 +269,6 @@ static int nsp_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	return 0;
 }
 
-static struct irq_chip nsp_gpio_irq_chip = {
-	.name = "gpio-a",
-	.irq_enable = nsp_gpio_irq_unmask,
-	.irq_disable = nsp_gpio_irq_mask,
-	.irq_ack = nsp_gpio_irq_ack,
-	.irq_mask = nsp_gpio_irq_mask,
-	.irq_unmask = nsp_gpio_irq_unmask,
-	.irq_set_type = nsp_gpio_irq_set_type,
-};
-
 static int nsp_gpio_direction_input(struct gpio_chip *gc, unsigned gpio)
 {
 	struct nsp_gpio *chip = gpiochip_get_data(gc);
@@ -322,13 +316,6 @@ static int nsp_gpio_get(struct gpio_chip *gc, unsigned gpio)
 	return !!(readl(chip->base + NSP_GPIO_DATA_IN) & BIT(gpio));
 }
 
-static int nsp_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
-{
-	struct nsp_gpio *chip = gpiochip_get_data(gc);
-
-	return irq_linear_revmap(chip->irq_domain, offset);
-}
-
 static int nsp_get_groups_count(struct pinctrl_dev *pctldev)
 {
 	return 1;
@@ -613,10 +600,9 @@ static const struct of_device_id nsp_gpio_of_match[] = {
 static int nsp_gpio_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct resource *res;
 	struct nsp_gpio *chip;
 	struct gpio_chip *gc;
-	u32 val, count;
+	u32 val;
 	int irq, ret;
 
 	if (of_property_read_u32(pdev->dev.of_node, "ngpios", &val)) {
@@ -631,15 +617,13 @@ static int nsp_gpio_probe(struct platform_device *pdev)
 	chip->dev = dev;
 	platform_set_drvdata(pdev, chip);
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	chip->base = devm_ioremap_resource(dev, res);
+	chip->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(chip->base)) {
 		dev_err(dev, "unable to map I/O memory\n");
 		return PTR_ERR(chip->base);
 	}
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	chip->io_ctrl = devm_ioremap_resource(dev, res);
+	chip->io_ctrl = devm_platform_ioremap_resource(pdev, 1);
 	if (IS_ERR(chip->io_ctrl)) {
 		dev_err(dev, "unable to map I/O memory\n");
 		return PTR_ERR(chip->io_ctrl);
@@ -659,44 +643,44 @@ static int nsp_gpio_probe(struct platform_device *pdev)
 	gc->direction_output = nsp_gpio_direction_output;
 	gc->set = nsp_gpio_set;
 	gc->get = nsp_gpio_get;
-	gc->to_irq = nsp_gpio_to_irq;
 
 	/* optional GPIO interrupt support */
 	irq = platform_get_irq(pdev, 0);
 	if (irq > 0) {
-		/* Create irq domain so that each pin can be assigned an IRQ.*/
-		chip->irq_domain = irq_domain_add_linear(gc->of_node, gc->ngpio,
-							 &irq_domain_simple_ops,
-							 chip);
-		if (!chip->irq_domain) {
-			dev_err(&pdev->dev, "Couldn't allocate IRQ domain\n");
-			return -ENXIO;
-		}
+		struct gpio_irq_chip *girq;
+		struct irq_chip *irqc;
 
-		/* Map each gpio to an IRQ and set the handler for gpiolib. */
-		for (count = 0; count < gc->ngpio; count++) {
-			int irq = irq_create_mapping(chip->irq_domain, count);
+		irqc = &chip->irqchip;
+		irqc->name = dev_name(dev);
+		irqc->irq_ack = nsp_gpio_irq_ack;
+		irqc->irq_mask = nsp_gpio_irq_mask;
+		irqc->irq_unmask = nsp_gpio_irq_unmask;
+		irqc->irq_set_type = nsp_gpio_irq_set_type;
 
-			irq_set_chip_and_handler(irq, &nsp_gpio_irq_chip,
-						 handle_simple_irq);
-			irq_set_chip_data(irq, chip);
-		}
+		val = readl(chip->base + NSP_CHIP_A_INT_MASK);
+		val = val | NSP_CHIP_A_GPIO_INT_BIT;
+		writel(val, (chip->base + NSP_CHIP_A_INT_MASK));
 
 		/* Install ISR for this GPIO controller. */
-		ret = devm_request_irq(&pdev->dev, irq, nsp_gpio_irq_handler,
-				       IRQF_SHARED, "gpio-a", chip);
+		ret = devm_request_irq(dev, irq, nsp_gpio_irq_handler,
+				       IRQF_SHARED, "gpio-a", &chip->gc);
 		if (ret) {
 			dev_err(&pdev->dev, "Unable to request IRQ%d: %d\n",
 				irq, ret);
-			goto err_rm_gpiochip;
+			return ret;
 		}
 
-		val = readl(chip->base + NSP_CHIP_A_INT_MASK);
-		val = val | NSP_CHIP_A_GPIO_INT_BIT;
-		writel(val, (chip->base + NSP_CHIP_A_INT_MASK));
+		girq = &chip->gc.irq;
+		girq->chip = irqc;
+		/* This will let us handle the parent IRQ in the driver */
+		girq->parent_handler = NULL;
+		girq->num_parents = 0;
+		girq->parents = NULL;
+		girq->default_type = IRQ_TYPE_NONE;
+		girq->handler = handle_simple_irq;
 	}
 
-	ret = gpiochip_add_data(gc, chip);
+	ret = devm_gpiochip_add_data(dev, gc, chip);
 	if (ret < 0) {
 		dev_err(dev, "unable to add GPIO chip\n");
 		return ret;
@@ -705,15 +689,10 @@ static int nsp_gpio_probe(struct platform_device *pdev)
 	ret = nsp_gpio_register_pinconf(chip);
 	if (ret) {
 		dev_err(dev, "unable to register pinconf\n");
-		goto err_rm_gpiochip;
+		return ret;
 	}
 
 	return 0;
-
-err_rm_gpiochip:
-	gpiochip_remove(gc);
-
-	return ret;
 }
 
 static struct platform_driver nsp_gpio_driver = {
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] ARM: dts: NSP: avoid unnecessary probe deferrals
  2019-10-25  4:00 [PATCH 0/2] ARM: bcm: nsp: gpio improvements (hopefully) Chris Packham
  2019-10-25  4:00 ` [PATCH 1/2] pinctrl: bcm: nsp: use gpiolib infrastructure for interrupts Chris Packham
@ 2019-10-25  4:00 ` Chris Packham
  2019-10-25 17:26   ` Florian Fainelli
  1 sibling, 1 reply; 6+ messages in thread
From: Chris Packham @ 2019-10-25  4:00 UTC (permalink / raw)
  To: robh+dt, mark.rutland, rjui, sbranden, bcm-kernel-feedback-list,
	linus.walleij
  Cc: devicetree, Chris Packham, linux-kernel, linux-arm-kernel, linux-gpio

The pinctrl node is used by the gpioa node. Which may have more
descendants at a board level. If the pinctrl node isn't probed first the
gpio is deferred and anything that needs a gpio pin on that chip is also
deferred.

Normally we and nodes in the device tree to be listed in their natural
memory mapped address order but putting the pinctrl node first avoids
the deferral of numerous devices so make an exception in this case.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 arch/arm/boot/dts/bcm-nsp.dtsi | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/arm/boot/dts/bcm-nsp.dtsi b/arch/arm/boot/dts/bcm-nsp.dtsi
index da6d70f09ef1..dd7a65743c08 100644
--- a/arch/arm/boot/dts/bcm-nsp.dtsi
+++ b/arch/arm/boot/dts/bcm-nsp.dtsi
@@ -172,6 +172,13 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 
+		pinctrl: pinctrl@3f1c0 {
+			compatible = "brcm,nsp-pinmux";
+			reg = <0x3f1c0 0x04>,
+			      <0x30028 0x04>,
+			      <0x3f408 0x04>;
+		};
+
 		gpioa: gpio@20 {
 			compatible = "brcm,nsp-gpio-a";
 			reg = <0x0020 0x70>,
@@ -458,13 +465,6 @@
 					     "sata2";
 		};
 
-		pinctrl: pinctrl@3f1c0 {
-			compatible = "brcm,nsp-pinmux";
-			reg = <0x3f1c0 0x04>,
-			      <0x30028 0x04>,
-			      <0x3f408 0x04>;
-		};
-
 		thermal: thermal@3f2c0 {
 			compatible = "brcm,ns-thermal";
 			reg = <0x3f2c0 0x10>;
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] ARM: dts: NSP: avoid unnecessary probe deferrals
  2019-10-25  4:00 ` [PATCH 2/2] ARM: dts: NSP: avoid unnecessary probe deferrals Chris Packham
@ 2019-10-25 17:26   ` Florian Fainelli
  2019-10-28 20:21     ` Chris Packham
  0 siblings, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2019-10-25 17:26 UTC (permalink / raw)
  To: Chris Packham, robh+dt, mark.rutland, rjui, sbranden,
	bcm-kernel-feedback-list, linus.walleij
  Cc: devicetree, linux-kernel, linux-arm-kernel, linux-gpio

On 10/24/19 9:00 PM, Chris Packham wrote:
> The pinctrl node is used by the gpioa node. Which may have more
> descendants at a board level. If the pinctrl node isn't probed first the
> gpio is deferred and anything that needs a gpio pin on that chip is also
> deferred.

If what you care is to optimize your boot flow such that no re-probing
occurs, maybe another solution to look at is to re-order the order in
which subsystems are initialized or built (_initcall changes or
drivers/Makefile changes), because changing Device Tree certainly does
not scale over platforms and I recall Rob indicating that he wanted to
introduce randomized platform_device creation from
of_platform_bus_populate() at one point or another.

> 
> Normally we and nodes in the device tree to be listed in their natural
> memory mapped address order but putting the pinctrl node first avoids
> the deferral of numerous devices so make an exception in this case.

That is a workaround more than a real solution, though I understand why
you would to do that. One downside is that the entries are no longer in
incrementing register address order and that is visually disturbing and
who knows, maybe a drive by contributor whose pet project will be to
order the Device Tree entries by incrementing addresses will change that
in the future...

> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>  arch/arm/boot/dts/bcm-nsp.dtsi | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/bcm-nsp.dtsi b/arch/arm/boot/dts/bcm-nsp.dtsi
> index da6d70f09ef1..dd7a65743c08 100644
> --- a/arch/arm/boot/dts/bcm-nsp.dtsi
> +++ b/arch/arm/boot/dts/bcm-nsp.dtsi
> @@ -172,6 +172,13 @@
>  		#address-cells = <1>;
>  		#size-cells = <1>;
>  
> +		pinctrl: pinctrl@3f1c0 {
> +			compatible = "brcm,nsp-pinmux";
> +			reg = <0x3f1c0 0x04>,
> +			      <0x30028 0x04>,
> +			      <0x3f408 0x04>;
> +		};
> +
>  		gpioa: gpio@20 {
>  			compatible = "brcm,nsp-gpio-a";
>  			reg = <0x0020 0x70>,
> @@ -458,13 +465,6 @@
>  					     "sata2";
>  		};
>  
> -		pinctrl: pinctrl@3f1c0 {
> -			compatible = "brcm,nsp-pinmux";
> -			reg = <0x3f1c0 0x04>,
> -			      <0x30028 0x04>,
> -			      <0x3f408 0x04>;
> -		};
> -
>  		thermal: thermal@3f2c0 {
>  			compatible = "brcm,ns-thermal";
>  			reg = <0x3f2c0 0x10>;
> 


-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] ARM: dts: NSP: avoid unnecessary probe deferrals
  2019-10-25 17:26   ` Florian Fainelli
@ 2019-10-28 20:21     ` Chris Packham
  2019-10-28 21:44       ` Chris Packham
  0 siblings, 1 reply; 6+ messages in thread
From: Chris Packham @ 2019-10-28 20:21 UTC (permalink / raw)
  To: sbranden, mark.rutland, bcm-kernel-feedback-list, f.fainelli,
	robh+dt, rjui, linus.walleij
  Cc: linux-gpio, linux-kernel, linux-arm-kernel, devicetree

On Fri, 2019-10-25 at 10:26 -0700, Florian Fainelli wrote:
> On 10/24/19 9:00 PM, Chris Packham wrote:
> > The pinctrl node is used by the gpioa node. Which may have more
> > descendants at a board level. If the pinctrl node isn't probed first the
> > gpio is deferred and anything that needs a gpio pin on that chip is also
> > deferred.
> 
> If what you care is to optimize your boot flow such that no re-probing
> occurs, maybe another solution to look at is to re-order the order in
> which subsystems are initialized or built (_initcall changes or
> drivers/Makefile changes), because changing Device Tree certainly does
> not scale over platforms and I recall Rob indicating that he wanted to
> introduce randomized platform_device creation from
> of_platform_bus_populate() at one point or another.
> 

Hmm. I might be missing something. pinctrl-nsp-gpio.c uses
arch_initcall_sync() and pinctrl-nsp-mux.c uses arch_initcall() so in
theory they are already in the right order.

> > 
> > Normally we and nodes in the device tree to be listed in their natural
> > memory mapped address order but putting the pinctrl node first avoids
> > the deferral of numerous devices so make an exception in this case.
> 
> That is a workaround more than a real solution, though I understand why
> you would to do that. One downside is that the entries are no longer in
> incrementing register address order and that is visually disturbing and
> who knows, maybe a drive by contributor whose pet project will be to
> order the Device Tree entries by incrementing addresses will change that
> in the future...
> 

I guess really what's needed is something that understands phandles and
tries to produce a dependency tree based on that.

> > 
> > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > ---
> >  arch/arm/boot/dts/bcm-nsp.dtsi | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/bcm-nsp.dtsi b/arch/arm/boot/dts/bcm-nsp.dtsi
> > index da6d70f09ef1..dd7a65743c08 100644
> > --- a/arch/arm/boot/dts/bcm-nsp.dtsi
> > +++ b/arch/arm/boot/dts/bcm-nsp.dtsi
> > @@ -172,6 +172,13 @@
> >  		#address-cells = <1>;
> >  		#size-cells = <1>;
> >  
> > +		pinctrl: pinctrl@3f1c0 {
> > +			compatible = "brcm,nsp-pinmux";
> > +			reg = <0x3f1c0 0x04>,
> > +			      <0x30028 0x04>,
> > +			      <0x3f408 0x04>;
> > +		};
> > +
> >  		gpioa: gpio@20 {
> >  			compatible = "brcm,nsp-gpio-a";
> >  			reg = <0x0020 0x70>,
> > @@ -458,13 +465,6 @@
> >  					     "sata2";
> >  		};
> >  
> > -		pinctrl: pinctrl@3f1c0 {
> > -			compatible = "brcm,nsp-pinmux";
> > -			reg = <0x3f1c0 0x04>,
> > -			      <0x30028 0x04>,
> > -			      <0x3f408 0x04>;
> > -		};
> > -
> >  		thermal: thermal@3f2c0 {
> >  			compatible = "brcm,ns-thermal";
> >  			reg = <0x3f2c0 0x10>;
> > 
> 
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] ARM: dts: NSP: avoid unnecessary probe deferrals
  2019-10-28 20:21     ` Chris Packham
@ 2019-10-28 21:44       ` Chris Packham
  0 siblings, 0 replies; 6+ messages in thread
From: Chris Packham @ 2019-10-28 21:44 UTC (permalink / raw)
  To: sbranden, mark.rutland, bcm-kernel-feedback-list, f.fainelli,
	robh+dt, rjui, linus.walleij
  Cc: linux-gpio, linux-kernel, linux-arm-kernel, devicetree

On Tue, 2019-10-29 at 09:21 +1300, Chris Packham wrote:
> On Fri, 2019-10-25 at 10:26 -0700, Florian Fainelli wrote:
> > On 10/24/19 9:00 PM, Chris Packham wrote:
> > > The pinctrl node is used by the gpioa node. Which may have more
> > > descendants at a board level. If the pinctrl node isn't probed first the
> > > gpio is deferred and anything that needs a gpio pin on that chip is also
> > > deferred.
> > 
> > If what you care is to optimize your boot flow such that no re-probing
> > occurs, maybe another solution to look at is to re-order the order in
> > which subsystems are initialized or built (_initcall changes or
> > drivers/Makefile changes), because changing Device Tree certainly does
> > not scale over platforms and I recall Rob indicating that he wanted to
> > introduce randomized platform_device creation from
> > of_platform_bus_populate() at one point or another.
> > 
> 
> Hmm. I might be missing something. pinctrl-nsp-gpio.c uses
> arch_initcall_sync() and pinctrl-nsp-mux.c uses arch_initcall() so in
> theory they are already in the right order.
> 

Actually the init calls are made in the required order w.r.t each
other. But they are both before of_platform_populate, so it's back to
the device tree being the determining factor for when the probe()
functions are run.

With the current kernel I get

nsp_pinmux_init:
nsp_gpio_init:
OF: of_platform_populate:
OF: of_platform_bus_create: /axi@18000000/gpio@20
nsp_gpio_probe:
gpiochip_add_data_with_key: GPIOs 480..511 (18000020.gpio) failed to
register, -517
nsp-gpio-a 18000020.gpio: unable to add GPIO chip
OF: of_platform_bus_create: /axi@18000000/pinctrl@3f1c0
nsp_pinmux_probe:
... much later ...
nsp_gpio_probe:

Would it be acceptable to change the init calls to device_initcall()
and device_initcall_sync()? pinctrl-nsp-mux.c could even be converted
to (builtin_)platform_driver.

> > > 
> > > Normally we and nodes in the device tree to be listed in their natural
> > > memory mapped address order but putting the pinctrl node first avoids
> > > the deferral of numerous devices so make an exception in this case.
> > 
> > That is a workaround more than a real solution, though I understand why
> > you would to do that. One downside is that the entries are no longer in
> > incrementing register address order and that is visually disturbing and
> > who knows, maybe a drive by contributor whose pet project will be to
> > order the Device Tree entries by incrementing addresses will change that
> > in the future...
> > 
> 
> I guess really what's needed is something that understands phandles and
> tries to produce a dependency tree based on that.
> 
> > > 
> > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > > ---
> > >  arch/arm/boot/dts/bcm-nsp.dtsi | 14 +++++++-------
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/arch/arm/boot/dts/bcm-nsp.dtsi b/arch/arm/boot/dts/bcm-nsp.dtsi
> > > index da6d70f09ef1..dd7a65743c08 100644
> > > --- a/arch/arm/boot/dts/bcm-nsp.dtsi
> > > +++ b/arch/arm/boot/dts/bcm-nsp.dtsi
> > > @@ -172,6 +172,13 @@
> > >  		#address-cells = <1>;
> > >  		#size-cells = <1>;
> > >  
> > > +		pinctrl: pinctrl@3f1c0 {
> > > +			compatible = "brcm,nsp-pinmux";
> > > +			reg = <0x3f1c0 0x04>,
> > > +			      <0x30028 0x04>,
> > > +			      <0x3f408 0x04>;
> > > +		};
> > > +
> > >  		gpioa: gpio@20 {
> > >  			compatible = "brcm,nsp-gpio-a";
> > >  			reg = <0x0020 0x70>,
> > > @@ -458,13 +465,6 @@
> > >  					     "sata2";
> > >  		};
> > >  
> > > -		pinctrl: pinctrl@3f1c0 {
> > > -			compatible = "brcm,nsp-pinmux";
> > > -			reg = <0x3f1c0 0x04>,
> > > -			      <0x30028 0x04>,
> > > -			      <0x3f408 0x04>;
> > > -		};
> > > -
> > >  		thermal: thermal@3f2c0 {
> > >  			compatible = "brcm,ns-thermal";
> > >  			reg = <0x3f2c0 0x10>;
> > > 
> > 
> > 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-10-28 21:44 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-25  4:00 [PATCH 0/2] ARM: bcm: nsp: gpio improvements (hopefully) Chris Packham
2019-10-25  4:00 ` [PATCH 1/2] pinctrl: bcm: nsp: use gpiolib infrastructure for interrupts Chris Packham
2019-10-25  4:00 ` [PATCH 2/2] ARM: dts: NSP: avoid unnecessary probe deferrals Chris Packham
2019-10-25 17:26   ` Florian Fainelli
2019-10-28 20:21     ` Chris Packham
2019-10-28 21:44       ` Chris Packham

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).