[PATCH 0/2] gpio: sch: Interrupt support

[PATCH 0/2] gpio: sch: Interrupt support

[Jan Kiszka]

See patches for details.

Two things I'm not 100% sure about:
 - Should we enable interrupts for all sch variants the driver support,
   or only the Quark where I was able to validate it (and where I found
   documentation for)?

 - Is there a better way to hook into the SCI? Yes, GPIO events will
   also set a bit in GPE0_STS, but there is no ACPI description for that
   event on our IOT2000 platform and, thus, also the original Galileo
   Gen2.

Jan

Jan Kiszka (2):
  gpio: sch: Remove write-only core_base
  gpio: sch: Add interrupt support

 drivers/gpio/gpio-sch.c | 147 ++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 135 insertions(+), 12 deletions(-)

-- 
2.16.4

[PATCH 1/2] gpio: sch: Remove write-only core_base

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/gpio/gpio-sch.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index c333046d02b8..fb143f28c386 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -23,7 +23,6 @@ struct sch_gpio {
 	struct gpio_chip chip;
 	spinlock_t lock;
 	unsigned short iobase;
-	unsigned short core_base;
 	unsigned short resume_base;
 };
 
@@ -166,7 +165,6 @@ static int sch_gpio_probe(struct platform_device *pdev)
 
 	switch (pdev->id) {
 	case PCI_DEVICE_ID_INTEL_SCH_LPC:
-		sch->core_base = 0;
 		sch->resume_base = 10;
 		sch->chip.ngpio = 14;
 
@@ -185,19 +183,16 @@ static int sch_gpio_probe(struct platform_device *pdev)
 		break;
 
 	case PCI_DEVICE_ID_INTEL_ITC_LPC:
-		sch->core_base = 0;
 		sch->resume_base = 5;
 		sch->chip.ngpio = 14;
 		break;
 
 	case PCI_DEVICE_ID_INTEL_CENTERTON_ILB:
-		sch->core_base = 0;
 		sch->resume_base = 21;
 		sch->chip.ngpio = 30;
 		break;
 
 	case PCI_DEVICE_ID_INTEL_QUARK_X1000_ILB:
-		sch->core_base = 0;
 		sch->resume_base = 2;
 		sch->chip.ngpio = 8;
 		break;
-- 
2.16.4

[PATCH 2/2] gpio: sch: Add interrupt support

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Validated on the Quark platform, this adds interrupt support on rising
and/or falling edges.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/gpio/gpio-sch.c | 142 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 135 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index fb143f28c386..9459f1920996 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -18,12 +18,17 @@
 #define GEN	0x00
 #define GIO	0x04
 #define GLV	0x08
+#define GTPE	0x0c
+#define GTNE	0x10
+#define GGPE	0x14
+#define GTS	0x1c
 
 struct sch_gpio {
 	struct gpio_chip chip;
 	spinlock_t lock;
 	unsigned short iobase;
 	unsigned short resume_base;
+	int irq_base;
 };
 
 static unsigned sch_gpio_offset(struct sch_gpio *sch, unsigned gpio,
@@ -79,10 +84,11 @@ static void sch_gpio_reg_set(struct sch_gpio *sch, unsigned gpio, unsigned reg,
 static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
 {
 	struct sch_gpio *sch = gpiochip_get_data(gc);
+	unsigned long flags;
 
-	spin_lock(&sch->lock);
+	spin_lock_irqsave(&sch->lock, flags);
 	sch_gpio_reg_set(sch, gpio_num, GIO, 1);
-	spin_unlock(&sch->lock);
+	spin_unlock_irqrestore(&sch->lock, flags);
 	return 0;
 }
 
@@ -95,20 +101,22 @@ static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num)
 static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
 {
 	struct sch_gpio *sch = gpiochip_get_data(gc);
+	unsigned long flags;
 
-	spin_lock(&sch->lock);
+	spin_lock_irqsave(&sch->lock, flags);
 	sch_gpio_reg_set(sch, gpio_num, GLV, val);
-	spin_unlock(&sch->lock);
+	spin_unlock_irqrestore(&sch->lock, flags);
 }
 
 static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
 				  int val)
 {
 	struct sch_gpio *sch = gpiochip_get_data(gc);
+	unsigned long flags;
 
-	spin_lock(&sch->lock);
+	spin_lock_irqsave(&sch->lock, flags);
 	sch_gpio_reg_set(sch, gpio_num, GIO, 0);
-	spin_unlock(&sch->lock);
+	spin_unlock_irqrestore(&sch->lock, flags);
 
 	/*
 	 * according to the datasheet, writing to the level register has no
@@ -130,6 +138,12 @@ static int sch_gpio_get_direction(struct gpio_chip *gc, unsigned gpio_num)
 	return sch_gpio_reg_get(sch, gpio_num, GIO);
 }
 
+static int sch_gpio_to_irq(struct gpio_chip *gpio, unsigned int offset)
+{
+	struct sch_gpio *sch = gpiochip_get_data(gpio);
+	return sch->irq_base + offset;
+}
+
 static const struct gpio_chip sch_gpio_chip = {
 	.label			= "sch_gpio",
 	.owner			= THIS_MODULE,
@@ -138,12 +152,94 @@ static const struct gpio_chip sch_gpio_chip = {
 	.direction_output	= sch_gpio_direction_out,
 	.set			= sch_gpio_set,
 	.get_direction		= sch_gpio_get_direction,
+	.to_irq			= sch_gpio_to_irq,
 };
 
+static u32 sch_sci_handler(void *context)
+{
+	struct sch_gpio *sch = context;
+	unsigned long core_status, resume_status;
+	unsigned int resume_gpios, offset;
+
+	core_status = inl(sch->iobase + GTS);
+	resume_status = inl(sch->iobase + GTS + 0x20);
+
+	if (core_status == 0 && resume_status == 0)
+		return ACPI_INTERRUPT_NOT_HANDLED;
+
+	for_each_set_bit(offset, &core_status, sch->resume_base)
+		generic_handle_irq(sch->irq_base + offset);
+
+	resume_gpios = sch->chip.ngpio - sch->resume_base;
+	for_each_set_bit(offset, &resume_status, resume_gpios)
+		generic_handle_irq(sch->irq_base + sch->resume_base + offset);
+
+	outl(core_status, sch->iobase + GTS);
+	outl(resume_status, sch->iobase + GTS + 0x20);
+
+	return ACPI_INTERRUPT_HANDLED;
+}
+
+static int sch_irq_type(struct irq_data *d, unsigned int type)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct sch_gpio *sch = gc->private;
+	unsigned int gpio_num = d->irq - sch->irq_base;
+	int rising = 0;
+	int falling = 0;
+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_RISING:
+		rising = 1;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		falling = 1;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		rising = 1;
+		falling = 1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	spin_lock(&sch->lock);
+	sch_gpio_reg_set(sch, gpio_num, GTPE, rising);
+	sch_gpio_reg_set(sch, gpio_num, GTNE, falling);
+	spin_unlock(&sch->lock);
+
+	return 0;
+}
+
+static void sch_irq_set_enable(struct irq_data *d, int val)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct sch_gpio *sch = gc->private;
+	unsigned int gpio_num = d->irq - sch->irq_base;
+
+	spin_lock(&sch->lock);
+	sch_gpio_reg_set(sch, gpio_num, GGPE, val);
+	spin_unlock(&sch->lock);
+}
+
+static void sch_irq_mask(struct irq_data *d)
+{
+	sch_irq_set_enable(d, 0);
+}
+
+static void sch_irq_unmask(struct irq_data *d)
+{
+	sch_irq_set_enable(d, 1);
+}
+
 static int sch_gpio_probe(struct platform_device *pdev)
 {
+	struct irq_chip_generic *gc;
+	struct irq_chip_type *ct;
 	struct sch_gpio *sch;
 	struct resource *res;
+	acpi_status status;
+	int irq_base, ret;
 
 	sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL);
 	if (!sch)
@@ -203,7 +299,39 @@ static int sch_gpio_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, sch);
 
-	return devm_gpiochip_add_data(&pdev->dev, &sch->chip, sch);
+	ret = devm_gpiochip_add_data(&pdev->dev, &sch->chip, sch);
+	if (ret)
+		return ret;
+
+	irq_base = devm_irq_alloc_descs(&pdev->dev, -1, 0, sch->chip.ngpio,
+					NUMA_NO_NODE);
+	if (irq_base < 0)
+		return irq_base;
+	sch->irq_base = irq_base;
+
+	gc = devm_irq_alloc_generic_chip(&pdev->dev, "sch_gpio", 1, irq_base,
+					 NULL, handle_simple_irq);
+	if (!gc)
+		return -ENOMEM;
+
+	gc->private = sch;
+	ct = gc->chip_types;
+
+	ct->chip.irq_mask = sch_irq_mask;
+	ct->chip.irq_unmask = sch_irq_unmask;
+	ct->chip.irq_set_type = sch_irq_type;
+
+	ret = devm_irq_setup_generic_chip(&pdev->dev, gc,
+					  IRQ_MSK(sch->chip.ngpio),
+					  0, IRQ_NOREQUEST | IRQ_NOPROBE, 0);
+	if (ret)
+		return ret;
+
+	status = acpi_install_sci_handler(sch_sci_handler, sch);
+	if (ACPI_FAILURE(status))
+		return -EINVAL;
+
+	return 0;
 }
 
 static struct platform_driver sch_gpio_driver = {
-- 
2.16.4

Re: [PATCH 1/2] gpio: sch: Remove write-only core_base

[Linus Walleij]

On Thu, Apr 18, 2019 at 11:23 AM Jan Kiszka <jan.kiszka@siemens.com> wrote:

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Dead code I see.
Patch applied.

Yours,
Linus Walleij

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Linus Walleij]

On Thu, Apr 18, 2019 at 11:23 AM Jan Kiszka <jan.kiszka@siemens.com> wrote:

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Validated on the Quark platform, this adds interrupt support on rising
> and/or falling edges.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Since this utilizes ACPI I'd like Andy and/or Mika to say something about
it before I merge it. I don't know how ACPI irqchips are supposed to
be instantiated.

Yours,
Linus Walleij

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Mika Westerberg]

+Rafael and linux-acpi.

On Thu, Apr 18, 2019 at 11:23:49AM +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Validated on the Quark platform, this adds interrupt support on rising
> and/or falling edges.

The irqchip parts look good to me but but the ACPI SCI handling seems
weird. This is typically handled by ACPI core based on the values read
from FADT ACPI table. What does it contain on this Quark platform?

> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  drivers/gpio/gpio-sch.c | 142 +++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 135 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
> index fb143f28c386..9459f1920996 100644
> --- a/drivers/gpio/gpio-sch.c
> +++ b/drivers/gpio/gpio-sch.c
> @@ -18,12 +18,17 @@
>  #define GEN	0x00
>  #define GIO	0x04
>  #define GLV	0x08
> +#define GTPE	0x0c
> +#define GTNE	0x10
> +#define GGPE	0x14
> +#define GTS	0x1c
>  
>  struct sch_gpio {
>  	struct gpio_chip chip;
>  	spinlock_t lock;
>  	unsigned short iobase;
>  	unsigned short resume_base;
> +	int irq_base;
>  };
>  
>  static unsigned sch_gpio_offset(struct sch_gpio *sch, unsigned gpio,
> @@ -79,10 +84,11 @@ static void sch_gpio_reg_set(struct sch_gpio *sch, unsigned gpio, unsigned reg,
>  static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
>  {
>  	struct sch_gpio *sch = gpiochip_get_data(gc);
> +	unsigned long flags;
>  
> -	spin_lock(&sch->lock);
> +	spin_lock_irqsave(&sch->lock, flags);
>  	sch_gpio_reg_set(sch, gpio_num, GIO, 1);
> -	spin_unlock(&sch->lock);
> +	spin_unlock_irqrestore(&sch->lock, flags);
>  	return 0;
>  }
>  
> @@ -95,20 +101,22 @@ static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num)
>  static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
>  {
>  	struct sch_gpio *sch = gpiochip_get_data(gc);
> +	unsigned long flags;
>  
> -	spin_lock(&sch->lock);
> +	spin_lock_irqsave(&sch->lock, flags);
>  	sch_gpio_reg_set(sch, gpio_num, GLV, val);
> -	spin_unlock(&sch->lock);
> +	spin_unlock_irqrestore(&sch->lock, flags);
>  }
>  
>  static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
>  				  int val)
>  {
>  	struct sch_gpio *sch = gpiochip_get_data(gc);
> +	unsigned long flags;
>  
> -	spin_lock(&sch->lock);
> +	spin_lock_irqsave(&sch->lock, flags);
>  	sch_gpio_reg_set(sch, gpio_num, GIO, 0);
> -	spin_unlock(&sch->lock);
> +	spin_unlock_irqrestore(&sch->lock, flags);
>  
>  	/*
>  	 * according to the datasheet, writing to the level register has no
> @@ -130,6 +138,12 @@ static int sch_gpio_get_direction(struct gpio_chip *gc, unsigned gpio_num)
>  	return sch_gpio_reg_get(sch, gpio_num, GIO);
>  }
>  
> +static int sch_gpio_to_irq(struct gpio_chip *gpio, unsigned int offset)
> +{
> +	struct sch_gpio *sch = gpiochip_get_data(gpio);
> +	return sch->irq_base + offset;
> +}
> +
>  static const struct gpio_chip sch_gpio_chip = {
>  	.label			= "sch_gpio",
>  	.owner			= THIS_MODULE,
> @@ -138,12 +152,94 @@ static const struct gpio_chip sch_gpio_chip = {
>  	.direction_output	= sch_gpio_direction_out,
>  	.set			= sch_gpio_set,
>  	.get_direction		= sch_gpio_get_direction,
> +	.to_irq			= sch_gpio_to_irq,
>  };
>  
> +static u32 sch_sci_handler(void *context)
> +{
> +	struct sch_gpio *sch = context;
> +	unsigned long core_status, resume_status;
> +	unsigned int resume_gpios, offset;
> +
> +	core_status = inl(sch->iobase + GTS);
> +	resume_status = inl(sch->iobase + GTS + 0x20);
> +
> +	if (core_status == 0 && resume_status == 0)
> +		return ACPI_INTERRUPT_NOT_HANDLED;
> +
> +	for_each_set_bit(offset, &core_status, sch->resume_base)
> +		generic_handle_irq(sch->irq_base + offset);
> +
> +	resume_gpios = sch->chip.ngpio - sch->resume_base;
> +	for_each_set_bit(offset, &resume_status, resume_gpios)
> +		generic_handle_irq(sch->irq_base + sch->resume_base + offset);
> +
> +	outl(core_status, sch->iobase + GTS);
> +	outl(resume_status, sch->iobase + GTS + 0x20);
> +
> +	return ACPI_INTERRUPT_HANDLED;
> +}
> +
> +static int sch_irq_type(struct irq_data *d, unsigned int type)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct sch_gpio *sch = gc->private;
> +	unsigned int gpio_num = d->irq - sch->irq_base;
> +	int rising = 0;
> +	int falling = 0;
> +
> +	switch (type & IRQ_TYPE_SENSE_MASK) {
> +	case IRQ_TYPE_EDGE_RISING:
> +		rising = 1;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		falling = 1;
> +		break;
> +	case IRQ_TYPE_EDGE_BOTH:
> +		rising = 1;
> +		falling = 1;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	spin_lock(&sch->lock);
> +	sch_gpio_reg_set(sch, gpio_num, GTPE, rising);
> +	sch_gpio_reg_set(sch, gpio_num, GTNE, falling);
> +	spin_unlock(&sch->lock);
> +
> +	return 0;
> +}
> +
> +static void sch_irq_set_enable(struct irq_data *d, int val)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct sch_gpio *sch = gc->private;
> +	unsigned int gpio_num = d->irq - sch->irq_base;
> +
> +	spin_lock(&sch->lock);
> +	sch_gpio_reg_set(sch, gpio_num, GGPE, val);
> +	spin_unlock(&sch->lock);
> +}
> +
> +static void sch_irq_mask(struct irq_data *d)
> +{
> +	sch_irq_set_enable(d, 0);
> +}
> +
> +static void sch_irq_unmask(struct irq_data *d)
> +{
> +	sch_irq_set_enable(d, 1);
> +}
> +
>  static int sch_gpio_probe(struct platform_device *pdev)
>  {
> +	struct irq_chip_generic *gc;
> +	struct irq_chip_type *ct;
>  	struct sch_gpio *sch;
>  	struct resource *res;
> +	acpi_status status;
> +	int irq_base, ret;
>  
>  	sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL);
>  	if (!sch)
> @@ -203,7 +299,39 @@ static int sch_gpio_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, sch);
>  
> -	return devm_gpiochip_add_data(&pdev->dev, &sch->chip, sch);
> +	ret = devm_gpiochip_add_data(&pdev->dev, &sch->chip, sch);
> +	if (ret)
> +		return ret;
> +
> +	irq_base = devm_irq_alloc_descs(&pdev->dev, -1, 0, sch->chip.ngpio,
> +					NUMA_NO_NODE);
> +	if (irq_base < 0)
> +		return irq_base;
> +	sch->irq_base = irq_base;
> +
> +	gc = devm_irq_alloc_generic_chip(&pdev->dev, "sch_gpio", 1, irq_base,
> +					 NULL, handle_simple_irq);
> +	if (!gc)
> +		return -ENOMEM;
> +
> +	gc->private = sch;
> +	ct = gc->chip_types;
> +
> +	ct->chip.irq_mask = sch_irq_mask;
> +	ct->chip.irq_unmask = sch_irq_unmask;
> +	ct->chip.irq_set_type = sch_irq_type;
> +
> +	ret = devm_irq_setup_generic_chip(&pdev->dev, gc,
> +					  IRQ_MSK(sch->chip.ngpio),
> +					  0, IRQ_NOREQUEST | IRQ_NOPROBE, 0);
> +	if (ret)
> +		return ret;
> +
> +	status = acpi_install_sci_handler(sch_sci_handler, sch);
> +	if (ACPI_FAILURE(status))
> +		return -EINVAL;
> +
> +	return 0;
>  }
>  
>  static struct platform_driver sch_gpio_driver = {
> -- 
> 2.16.4

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Jan Kiszka]

On 24.04.19 09:58, Mika Westerberg wrote:
> +Rafael and linux-acpi.
> 
> On Thu, Apr 18, 2019 at 11:23:49AM +0200, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Validated on the Quark platform, this adds interrupt support on rising
>> and/or falling edges.
> 
> The irqchip parts look good to me but but the ACPI SCI handling seems
> weird. This is typically handled by ACPI core based on the values read
> from FADT ACPI table. What does it contain on this Quark platform?

There is no FADT on the original Quark firmware, nor did we add one. As we are 
talking about existing devices, possibly not only Quarks, I was going down the 
ACPI-independent way to hook into the interrupt. But I'm open to learn about 
better alternatives.

Jan

> 
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>   drivers/gpio/gpio-sch.c | 142 +++++++++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 135 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
>> index fb143f28c386..9459f1920996 100644
>> --- a/drivers/gpio/gpio-sch.c
>> +++ b/drivers/gpio/gpio-sch.c
>> @@ -18,12 +18,17 @@
>>   #define GEN	0x00
>>   #define GIO	0x04
>>   #define GLV	0x08
>> +#define GTPE	0x0c
>> +#define GTNE	0x10
>> +#define GGPE	0x14
>> +#define GTS	0x1c
>>   
>>   struct sch_gpio {
>>   	struct gpio_chip chip;
>>   	spinlock_t lock;
>>   	unsigned short iobase;
>>   	unsigned short resume_base;
>> +	int irq_base;
>>   };
>>   
>>   static unsigned sch_gpio_offset(struct sch_gpio *sch, unsigned gpio,
>> @@ -79,10 +84,11 @@ static void sch_gpio_reg_set(struct sch_gpio *sch, unsigned gpio, unsigned reg,
>>   static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
>>   {
>>   	struct sch_gpio *sch = gpiochip_get_data(gc);
>> +	unsigned long flags;
>>   
>> -	spin_lock(&sch->lock);
>> +	spin_lock_irqsave(&sch->lock, flags);
>>   	sch_gpio_reg_set(sch, gpio_num, GIO, 1);
>> -	spin_unlock(&sch->lock);
>> +	spin_unlock_irqrestore(&sch->lock, flags);
>>   	return 0;
>>   }
>>   
>> @@ -95,20 +101,22 @@ static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num)
>>   static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
>>   {
>>   	struct sch_gpio *sch = gpiochip_get_data(gc);
>> +	unsigned long flags;
>>   
>> -	spin_lock(&sch->lock);
>> +	spin_lock_irqsave(&sch->lock, flags);
>>   	sch_gpio_reg_set(sch, gpio_num, GLV, val);
>> -	spin_unlock(&sch->lock);
>> +	spin_unlock_irqrestore(&sch->lock, flags);
>>   }
>>   
>>   static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
>>   				  int val)
>>   {
>>   	struct sch_gpio *sch = gpiochip_get_data(gc);
>> +	unsigned long flags;
>>   
>> -	spin_lock(&sch->lock);
>> +	spin_lock_irqsave(&sch->lock, flags);
>>   	sch_gpio_reg_set(sch, gpio_num, GIO, 0);
>> -	spin_unlock(&sch->lock);
>> +	spin_unlock_irqrestore(&sch->lock, flags);
>>   
>>   	/*
>>   	 * according to the datasheet, writing to the level register has no
>> @@ -130,6 +138,12 @@ static int sch_gpio_get_direction(struct gpio_chip *gc, unsigned gpio_num)
>>   	return sch_gpio_reg_get(sch, gpio_num, GIO);
>>   }
>>   
>> +static int sch_gpio_to_irq(struct gpio_chip *gpio, unsigned int offset)
>> +{
>> +	struct sch_gpio *sch = gpiochip_get_data(gpio);
>> +	return sch->irq_base + offset;
>> +}
>> +
>>   static const struct gpio_chip sch_gpio_chip = {
>>   	.label			= "sch_gpio",
>>   	.owner			= THIS_MODULE,
>> @@ -138,12 +152,94 @@ static const struct gpio_chip sch_gpio_chip = {
>>   	.direction_output	= sch_gpio_direction_out,
>>   	.set			= sch_gpio_set,
>>   	.get_direction		= sch_gpio_get_direction,
>> +	.to_irq			= sch_gpio_to_irq,
>>   };
>>   
>> +static u32 sch_sci_handler(void *context)
>> +{
>> +	struct sch_gpio *sch = context;
>> +	unsigned long core_status, resume_status;
>> +	unsigned int resume_gpios, offset;
>> +
>> +	core_status = inl(sch->iobase + GTS);
>> +	resume_status = inl(sch->iobase + GTS + 0x20);
>> +
>> +	if (core_status == 0 && resume_status == 0)
>> +		return ACPI_INTERRUPT_NOT_HANDLED;
>> +
>> +	for_each_set_bit(offset, &core_status, sch->resume_base)
>> +		generic_handle_irq(sch->irq_base + offset);
>> +
>> +	resume_gpios = sch->chip.ngpio - sch->resume_base;
>> +	for_each_set_bit(offset, &resume_status, resume_gpios)
>> +		generic_handle_irq(sch->irq_base + sch->resume_base + offset);
>> +
>> +	outl(core_status, sch->iobase + GTS);
>> +	outl(resume_status, sch->iobase + GTS + 0x20);
>> +
>> +	return ACPI_INTERRUPT_HANDLED;
>> +}
>> +
>> +static int sch_irq_type(struct irq_data *d, unsigned int type)
>> +{
>> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>> +	struct sch_gpio *sch = gc->private;
>> +	unsigned int gpio_num = d->irq - sch->irq_base;
>> +	int rising = 0;
>> +	int falling = 0;
>> +
>> +	switch (type & IRQ_TYPE_SENSE_MASK) {
>> +	case IRQ_TYPE_EDGE_RISING:
>> +		rising = 1;
>> +		break;
>> +	case IRQ_TYPE_EDGE_FALLING:
>> +		falling = 1;
>> +		break;
>> +	case IRQ_TYPE_EDGE_BOTH:
>> +		rising = 1;
>> +		falling = 1;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	spin_lock(&sch->lock);
>> +	sch_gpio_reg_set(sch, gpio_num, GTPE, rising);
>> +	sch_gpio_reg_set(sch, gpio_num, GTNE, falling);
>> +	spin_unlock(&sch->lock);
>> +
>> +	return 0;
>> +}
>> +
>> +static void sch_irq_set_enable(struct irq_data *d, int val)
>> +{
>> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>> +	struct sch_gpio *sch = gc->private;
>> +	unsigned int gpio_num = d->irq - sch->irq_base;
>> +
>> +	spin_lock(&sch->lock);
>> +	sch_gpio_reg_set(sch, gpio_num, GGPE, val);
>> +	spin_unlock(&sch->lock);
>> +}
>> +
>> +static void sch_irq_mask(struct irq_data *d)
>> +{
>> +	sch_irq_set_enable(d, 0);
>> +}
>> +
>> +static void sch_irq_unmask(struct irq_data *d)
>> +{
>> +	sch_irq_set_enable(d, 1);
>> +}
>> +
>>   static int sch_gpio_probe(struct platform_device *pdev)
>>   {
>> +	struct irq_chip_generic *gc;
>> +	struct irq_chip_type *ct;
>>   	struct sch_gpio *sch;
>>   	struct resource *res;
>> +	acpi_status status;
>> +	int irq_base, ret;
>>   
>>   	sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL);
>>   	if (!sch)
>> @@ -203,7 +299,39 @@ static int sch_gpio_probe(struct platform_device *pdev)
>>   
>>   	platform_set_drvdata(pdev, sch);
>>   
>> -	return devm_gpiochip_add_data(&pdev->dev, &sch->chip, sch);
>> +	ret = devm_gpiochip_add_data(&pdev->dev, &sch->chip, sch);
>> +	if (ret)
>> +		return ret;
>> +
>> +	irq_base = devm_irq_alloc_descs(&pdev->dev, -1, 0, sch->chip.ngpio,
>> +					NUMA_NO_NODE);
>> +	if (irq_base < 0)
>> +		return irq_base;
>> +	sch->irq_base = irq_base;
>> +
>> +	gc = devm_irq_alloc_generic_chip(&pdev->dev, "sch_gpio", 1, irq_base,
>> +					 NULL, handle_simple_irq);
>> +	if (!gc)
>> +		return -ENOMEM;
>> +
>> +	gc->private = sch;
>> +	ct = gc->chip_types;
>> +
>> +	ct->chip.irq_mask = sch_irq_mask;
>> +	ct->chip.irq_unmask = sch_irq_unmask;
>> +	ct->chip.irq_set_type = sch_irq_type;
>> +
>> +	ret = devm_irq_setup_generic_chip(&pdev->dev, gc,
>> +					  IRQ_MSK(sch->chip.ngpio),
>> +					  0, IRQ_NOREQUEST | IRQ_NOPROBE, 0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	status = acpi_install_sci_handler(sch_sci_handler, sch);
>> +	if (ACPI_FAILURE(status))
>> +		return -EINVAL;
>> +
>> +	return 0;
>>   }
>>   
>>   static struct platform_driver sch_gpio_driver = {
>> -- 
>> 2.16.4


-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Mika Westerberg]

On Wed, Apr 24, 2019 at 10:12:42AM +0200, Jan Kiszka wrote:
> On 24.04.19 09:58, Mika Westerberg wrote:
> > +Rafael and linux-acpi.
> > 
> > On Thu, Apr 18, 2019 at 11:23:49AM +0200, Jan Kiszka wrote:
> > > From: Jan Kiszka <jan.kiszka@siemens.com>
> > > 
> > > Validated on the Quark platform, this adds interrupt support on rising
> > > and/or falling edges.
> > 
> > The irqchip parts look good to me but but the ACPI SCI handling seems
> > weird. This is typically handled by ACPI core based on the values read
> > from FADT ACPI table. What does it contain on this Quark platform?
> 
> There is no FADT on the original Quark firmware, nor did we add one. As we
> are talking about existing devices, possibly not only Quarks, I was going
> down the ACPI-independent way to hook into the interrupt. But I'm open to
> learn about better alternatives.

Hmm, if it does not have FADT table why would you need SCI then? Is this
implementing some real use case?

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Jan Kiszka]

On 24.04.19 10:18, Mika Westerberg wrote:
> On Wed, Apr 24, 2019 at 10:12:42AM +0200, Jan Kiszka wrote:
>> On 24.04.19 09:58, Mika Westerberg wrote:
>>> +Rafael and linux-acpi.
>>>
>>> On Thu, Apr 18, 2019 at 11:23:49AM +0200, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> Validated on the Quark platform, this adds interrupt support on rising
>>>> and/or falling edges.
>>>
>>> The irqchip parts look good to me but but the ACPI SCI handling seems
>>> weird. This is typically handled by ACPI core based on the values read
>>> from FADT ACPI table. What does it contain on this Quark platform?
>>
>> There is no FADT on the original Quark firmware, nor did we add one. As we
>> are talking about existing devices, possibly not only Quarks, I was going
>> down the ACPI-independent way to hook into the interrupt. But I'm open to
>> learn about better alternatives.
> 
> Hmm, if it does not have FADT table why would you need SCI then? Is this
> implementing some real use case?
> 

Maybe I'm looking at the wrong place: Where would I find it? There is definitely 
no separate entry under /sys/firmware/acpi/tables - but that's also true for my 
workstation.

In any case: The hardware defines that the GPIO events are sent via SCIs. That 
fact is probably not expressed in ACPI language.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Mika Westerberg]

On Wed, Apr 24, 2019 at 10:25:40AM +0200, Jan Kiszka wrote:
> On 24.04.19 10:18, Mika Westerberg wrote:
> > On Wed, Apr 24, 2019 at 10:12:42AM +0200, Jan Kiszka wrote:
> > > On 24.04.19 09:58, Mika Westerberg wrote:
> > > > +Rafael and linux-acpi.
> > > > 
> > > > On Thu, Apr 18, 2019 at 11:23:49AM +0200, Jan Kiszka wrote:
> > > > > From: Jan Kiszka <jan.kiszka@siemens.com>
> > > > > 
> > > > > Validated on the Quark platform, this adds interrupt support on rising
> > > > > and/or falling edges.
> > > > 
> > > > The irqchip parts look good to me but but the ACPI SCI handling seems
> > > > weird. This is typically handled by ACPI core based on the values read
> > > > from FADT ACPI table. What does it contain on this Quark platform?
> > > 
> > > There is no FADT on the original Quark firmware, nor did we add one. As we
> > > are talking about existing devices, possibly not only Quarks, I was going
> > > down the ACPI-independent way to hook into the interrupt. But I'm open to
> > > learn about better alternatives.
> > 
> > Hmm, if it does not have FADT table why would you need SCI then? Is this
> > implementing some real use case?
> > 
> 
> Maybe I'm looking at the wrong place: Where would I find it? There is
> definitely no separate entry under /sys/firmware/acpi/tables - but that's
> also true for my workstation.

The table signature and name is FACP for historical reasons.

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Jan Kiszka]

On 24.04.19 10:42, Mika Westerberg wrote:
> On Wed, Apr 24, 2019 at 10:25:40AM +0200, Jan Kiszka wrote:
>> On 24.04.19 10:18, Mika Westerberg wrote:
>>> On Wed, Apr 24, 2019 at 10:12:42AM +0200, Jan Kiszka wrote:
>>>> On 24.04.19 09:58, Mika Westerberg wrote:
>>>>> +Rafael and linux-acpi.
>>>>>
>>>>> On Thu, Apr 18, 2019 at 11:23:49AM +0200, Jan Kiszka wrote:
>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>
>>>>>> Validated on the Quark platform, this adds interrupt support on rising
>>>>>> and/or falling edges.
>>>>>
>>>>> The irqchip parts look good to me but but the ACPI SCI handling seems
>>>>> weird. This is typically handled by ACPI core based on the values read
>>>>> from FADT ACPI table. What does it contain on this Quark platform?
>>>>
>>>> There is no FADT on the original Quark firmware, nor did we add one. As we
>>>> are talking about existing devices, possibly not only Quarks, I was going
>>>> down the ACPI-independent way to hook into the interrupt. But I'm open to
>>>> learn about better alternatives.
>>>
>>> Hmm, if it does not have FADT table why would you need SCI then? Is this
>>> implementing some real use case?
>>>
>>
>> Maybe I'm looking at the wrong place: Where would I find it? There is
>> definitely no separate entry under /sys/firmware/acpi/tables - but that's
>> also true for my workstation.
> 
> The table signature and name is FACP for historical reasons.
> 

OK, there is that table, but what is it supposed to tell me about the
event and where to hook into it better?

[000h 0000   4]                    Signature : "FACP"    [Fixed ACPI Description Table (FADT)]
[004h 0004   4]                 Table Length : 000000F4
[008h 0008   1]                     Revision : 03
[009h 0009   1]                     Checksum : CB
[00Ah 0010   6]                       Oem ID : "INTEL "
[010h 0016   8]                 Oem Table ID : "TIANO   "
[018h 0024   4]                 Oem Revision : 00000004
[01Ch 0028   4]              Asl Compiler ID : "INTL"
[020h 0032   4]        Asl Compiler Revision : 0100000D

[024h 0036   4]                 FACS Address : 371B7000
[028h 0040   4]                 DSDT Address : 37002000
[02Ch 0044   1]                        Model : 00
[02Dh 0045   1]                   PM Profile : 01 [Desktop]
[02Eh 0046   2]                SCI Interrupt : 0009
[030h 0048   4]             SMI Command Port : 000000B2
[034h 0052   1]            ACPI Enable Value : A0
[035h 0053   1]           ACPI Disable Value : A1
[036h 0054   1]               S4BIOS Command : 00
[037h 0055   1]              P-State Control : 00
[038h 0056   4]     PM1A Event Block Address : 00001000
[03Ch 0060   4]     PM1B Event Block Address : 00000000
[040h 0064   4]   PM1A Control Block Address : 00001004
[044h 0068   4]   PM1B Control Block Address : 00000000
[048h 0072   4]    PM2 Control Block Address : 00000000
[04Ch 0076   4]       PM Timer Block Address : 00001008
[050h 0080   4]           GPE0 Block Address : 00001100
[054h 0084   4]           GPE1 Block Address : 00000000
[058h 0088   1]       PM1 Event Block Length : 04
[059h 0089   1]     PM1 Control Block Length : 02
[05Ah 0090   1]     PM2 Control Block Length : 00
[05Bh 0091   1]        PM Timer Block Length : 04
[05Ch 0092   1]            GPE0 Block Length : 08
[05Dh 0093   1]            GPE1 Block Length : 00
[05Eh 0094   1]             GPE1 Base Offset : 00
[05Fh 0095   1]                 _CST Support : 00
[060h 0096   2]                   C2 Latency : 0065
[062h 0098   2]                   C3 Latency : 03E9
[064h 0100   2]               CPU Cache Size : 0400
[066h 0102   2]           Cache Flush Stride : 0010
[068h 0104   1]            Duty Cycle Offset : 01
[069h 0105   1]             Duty Cycle Width : 03
[06Ah 0106   1]          RTC Day Alarm Index : 00
[06Bh 0107   1]        RTC Month Alarm Index : 00
[06Ch 0108   1]            RTC Century Index : 00
[06Dh 0109   2]   Boot Flags (decoded below) : 0001
               Legacy Devices Supported (V2) : 1
            8042 Present on ports 60/64 (V2) : 0
                        VGA Not Present (V4) : 0
                      MSI Not Supported (V4) : 0
                PCIe ASPM Not Supported (V4) : 0
                   CMOS RTC Not Present (V5) : 0
[06Fh 0111   1]                     Reserved : 00
[070h 0112   4]        Flags (decoded below) : 000084B5
      WBINVD instruction is operational (V1) : 1
              WBINVD flushes all caches (V1) : 0
                    All CPUs support C1 (V1) : 1
                  C2 works on MP system (V1) : 0
            Control Method Power Button (V1) : 1
            Control Method Sleep Button (V1) : 1
        RTC wake not in fixed reg space (V1) : 0
            RTC can wake system from S4 (V1) : 1
                        32-bit PM Timer (V1) : 0
                      Docking Supported (V1) : 0
               Reset Register Supported (V2) : 1
                            Sealed Case (V3) : 0
                    Headless - No Video (V3) : 0
        Use native instr after SLP_TYPx (V3) : 0
              PCIEXP_WAK Bits Supported (V4) : 0
                     Use Platform Timer (V4) : 1
               RTC_STS valid on S4 wake (V4) : 0
                Remote Power-on capable (V4) : 0
                 Use APIC Cluster Model (V4) : 0
     Use APIC Physical Destination Mode (V4) : 0
                       Hardware Reduced (V5) : 0
                      Low Power S0 Idle (V5) : 0

[074h 0116  12]               Reset Register : [Generic Address Structure]
[074h 0116   1]                     Space ID : 01 [SystemIO]
[075h 0117   1]                    Bit Width : 08
[076h 0118   1]                   Bit Offset : 00
[077h 0119   1]         Encoded Access Width : 00 [Undefined/Legacy]
[078h 0120   8]                      Address : 0000000000000CF9

[080h 0128   1]         Value to cause reset : 02
[081h 0129   2]    ARM Flags (decoded below) : 0000
                              PSCI Compliant : 0
                       Must use HVC for PSCI : 0

[083h 0131   1]          FADT Minor Revision : 00
[084h 0132   8]                 FACS Address : 0000000000000000
[08Ch 0140   8]                 DSDT Address : 0000000037002000
[094h 0148  12]             PM1A Event Block : [Generic Address Structure]
[094h 0148   1]                     Space ID : 01 [SystemIO]
[095h 0149   1]                    Bit Width : 20
[096h 0150   1]                   Bit Offset : 00
[097h 0151   1]         Encoded Access Width : 00 [Undefined/Legacy]
[098h 0152   8]                      Address : 0000000000001000

[0A0h 0160  12]             PM1B Event Block : [Generic Address Structure]
[0A0h 0160   1]                     Space ID : 01 [SystemIO]
[0A1h 0161   1]                    Bit Width : 00
[0A2h 0162   1]                   Bit Offset : 00
[0A3h 0163   1]         Encoded Access Width : 00 [Undefined/Legacy]
[0A4h 0164   8]                      Address : 0000000000000000

[0ACh 0172  12]           PM1A Control Block : [Generic Address Structure]
[0ACh 0172   1]                     Space ID : 01 [SystemIO]
[0ADh 0173   1]                    Bit Width : 10
[0AEh 0174   1]                   Bit Offset : 00
[0AFh 0175   1]         Encoded Access Width : 00 [Undefined/Legacy]
[0B0h 0176   8]                      Address : 0000000000001004

[0B8h 0184  12]           PM1B Control Block : [Generic Address Structure]
[0B8h 0184   1]                     Space ID : 01 [SystemIO]
[0B9h 0185   1]                    Bit Width : 00
[0BAh 0186   1]                   Bit Offset : 00
[0BBh 0187   1]         Encoded Access Width : 00 [Undefined/Legacy]
[0BCh 0188   8]                      Address : 0000000000000000

[0C4h 0196  12]            PM2 Control Block : [Generic Address Structure]
[0C4h 0196   1]                     Space ID : 01 [SystemIO]
[0C5h 0197   1]                    Bit Width : 00
[0C6h 0198   1]                   Bit Offset : 00
[0C7h 0199   1]         Encoded Access Width : 00 [Undefined/Legacy]
[0C8h 0200   8]                      Address : 0000000000000000

[0D0h 0208  12]               PM Timer Block : [Generic Address Structure]
[0D0h 0208   1]                     Space ID : 01 [SystemIO]
[0D1h 0209   1]                    Bit Width : 20
[0D2h 0210   1]                   Bit Offset : 00
[0D3h 0211   1]         Encoded Access Width : 00 [Undefined/Legacy]
[0D4h 0212   8]                      Address : 0000000000001008

[0DCh 0220  12]                   GPE0 Block : [Generic Address Structure]
[0DCh 0220   1]                     Space ID : 01 [SystemIO]
[0DDh 0221   1]                    Bit Width : 40
[0DEh 0222   1]                   Bit Offset : 00
[0DFh 0223   1]         Encoded Access Width : 00 [Undefined/Legacy]
[0E0h 0224   8]                      Address : 0000000000001100

[0E8h 0232  12]                   GPE1 Block : [Generic Address Structure]
[0E8h 0232   1]                     Space ID : 01 [SystemIO]
[0E9h 0233   1]                    Bit Width : 00
[0EAh 0234   1]                   Bit Offset : 00
[0EBh 0235   1]         Encoded Access Width : 00 [Undefined/Legacy]
[0ECh 0236   8]                      Address : 0000000000000000


Raw Table Data: Length 244 (0xF4)

  0000: 46 41 43 50 F4 00 00 00 03 CB 49 4E 54 45 4C 20  // FACP......INTEL 
  0010: 54 49 41 4E 4F 20 20 20 04 00 00 00 49 4E 54 4C  // TIANO   ....INTL
  0020: 0D 00 00 01 00 70 1B 37 00 20 00 37 00 01 09 00  // .....p.7. .7....
  0030: B2 00 00 00 A0 A1 00 00 00 10 00 00 00 00 00 00  // ................
  0040: 04 10 00 00 00 00 00 00 00 00 00 00 08 10 00 00  // ................
  0050: 00 11 00 00 00 00 00 00 04 02 00 04 08 00 00 00  // ................
  0060: 65 00 E9 03 00 04 10 00 01 03 00 00 00 01 00 00  // e...............
  0070: B5 84 00 00 01 08 00 00 F9 0C 00 00 00 00 00 00  // ................
  0080: 02 00 00 00 00 00 00 00 00 00 00 00 00 20 00 37  // ............. .7
  0090: 00 00 00 00 01 20 00 00 00 10 00 00 00 00 00 00  // ..... ..........
  00A0: 01 00 00 00 00 00 00 00 00 00 00 00 01 10 00 00  // ................
  00B0: 04 10 00 00 00 00 00 00 01 00 00 00 00 00 00 00  // ................
  00C0: 00 00 00 00 01 00 00 00 00 00 00 00 00 00 00 00  // ................
  00D0: 01 20 00 00 08 10 00 00 00 00 00 00 01 40 00 00  // . ...........@..
  00E0: 00 11 00 00 00 00 00 00 01 00 00 00 00 00 00 00  // ................
  00F0: 00 00 00 00                                      // ....

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Mika Westerberg]

On Wed, Apr 24, 2019 at 11:36:58AM +0200, Jan Kiszka wrote:
> OK, there is that table, but what is it supposed to tell me about the
> event and where to hook into it better?
...
> [02Eh 0046   2]                SCI Interrupt : 0009

This is the SCI interrupt GSI number. IIRC it maps 1:1 to Linux
interrupt number so you should see it in /proc/interrupts. When a GPE
event is triggered it should be handled in the ACPI core.

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Jan Kiszka]

On 24.04.19 11:45, Mika Westerberg wrote:
> On Wed, Apr 24, 2019 at 11:36:58AM +0200, Jan Kiszka wrote:
>> OK, there is that table, but what is it supposed to tell me about the
>> event and where to hook into it better?
> ...
>> [02Eh 0046   2]                SCI Interrupt : 0009
> 
> This is the SCI interrupt GSI number. IIRC it maps 1:1 to Linux
> interrupt number so you should see it in /proc/interrupts. When a GPE
> event is triggered it should be handled in the ACPI core.
> 

Yes, clear, all this happens already. But I need to link the core with the sch 
gpio handler so that the gpio event is filtered out and the right virtual gpio 
interrupt is triggered. So, other than acpi_install_sci_handler, how should I 
establish that link?

Jan


-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Mika Westerberg]

On Wed, Apr 24, 2019 at 11:48:09AM +0200, Jan Kiszka wrote:
> On 24.04.19 11:45, Mika Westerberg wrote:
> > On Wed, Apr 24, 2019 at 11:36:58AM +0200, Jan Kiszka wrote:
> > > OK, there is that table, but what is it supposed to tell me about the
> > > event and where to hook into it better?
> > ...
> > > [02Eh 0046   2]                SCI Interrupt : 0009
> > 
> > This is the SCI interrupt GSI number. IIRC it maps 1:1 to Linux
> > interrupt number so you should see it in /proc/interrupts. When a GPE
> > event is triggered it should be handled in the ACPI core.
> > 
> 
> Yes, clear, all this happens already. But I need to link the core with the
> sch gpio handler so that the gpio event is filtered out and the right
> virtual gpio interrupt is triggered. So, other than
> acpi_install_sci_handler, how should I establish that link?

I think what you want is "GPIO signaled ACPI event". It works so that
you declare _AEI method below the GPIO controller listing the GPIOs you
want to trigger events for and then either _Lxx, _Exx or _EVT method for
each of them under the same controller. GPIO core then handles it
automatically when you register the GPIO chip. See also
acpi_gpiochip_request_interrupts().

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Jan Kiszka]

On 24.04.19 12:01, Mika Westerberg wrote:
> On Wed, Apr 24, 2019 at 11:48:09AM +0200, Jan Kiszka wrote:
>> On 24.04.19 11:45, Mika Westerberg wrote:
>>> On Wed, Apr 24, 2019 at 11:36:58AM +0200, Jan Kiszka wrote:
>>>> OK, there is that table, but what is it supposed to tell me about the
>>>> event and where to hook into it better?
>>> ...
>>>> [02Eh 0046   2]                SCI Interrupt : 0009
>>>
>>> This is the SCI interrupt GSI number. IIRC it maps 1:1 to Linux
>>> interrupt number so you should see it in /proc/interrupts. When a GPE
>>> event is triggered it should be handled in the ACPI core.
>>>
>>
>> Yes, clear, all this happens already. But I need to link the core with the
>> sch gpio handler so that the gpio event is filtered out and the right
>> virtual gpio interrupt is triggered. So, other than
>> acpi_install_sci_handler, how should I establish that link?
> 
> I think what you want is "GPIO signaled ACPI event". It works so that
> you declare _AEI method below the GPIO controller listing the GPIOs you
> want to trigger events for and then either _Lxx, _Exx or _EVT method for
> each of them under the same controller. GPIO core then handles it
> automatically when you register the GPIO chip. See also
> acpi_gpiochip_request_interrupts().
> 

Right, that is was I read as well. Let's assume I would be able to patch the 
tables: Would I describe all the logic of this patch in ACPI terms? Where to 
enable interrupts, how to dispatch the SCI event, how to acknowledge it etc.? 
Will it also take care of locking? (BTW, my locking seems to have some remaining 
inconsistency, on second look.)

And even if that were possible, we would be back to the square of existing 
devices without those definitions. If this were a recent chipset, I would say, 
"go, fix future firmware versions". But this one is legacy.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Mika Westerberg]

On Wed, Apr 24, 2019 at 12:19:02PM +0200, Jan Kiszka wrote:
> > I think what you want is "GPIO signaled ACPI event". It works so that
> > you declare _AEI method below the GPIO controller listing the GPIOs you
> > want to trigger events for and then either _Lxx, _Exx or _EVT method for
> > each of them under the same controller. GPIO core then handles it
> > automatically when you register the GPIO chip. See also
> > acpi_gpiochip_request_interrupts().
> 
> Right, that is was I read as well. Let's assume I would be able to patch the
> tables: Would I describe all the logic of this patch in ACPI terms? Where to
> enable interrupts, how to dispatch the SCI event, how to acknowledge it
> etc.? Will it also take care of locking? (BTW, my locking seems to have some
> remaining inconsistency, on second look.)

The GPIO core would then take care of it by requesting the GPIO in
question and dispatching to the correct event handler. In this patch you
just leave out the SCI part and only implement the irqchip like you did
already.

> And even if that were possible, we would be back to the square of existing
> devices without those definitions. If this were a recent chipset, I would
> say, "go, fix future firmware versions". But this one is legacy.

Is it fixing some real issue with these legacy platforms? I mean without
the patch some GPE event is not handled properly? It was not clear to me
from the commit message.

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Jan Kiszka]

On 24.04.19 12:33, Mika Westerberg wrote:
> On Wed, Apr 24, 2019 at 12:19:02PM +0200, Jan Kiszka wrote:
>>> I think what you want is "GPIO signaled ACPI event". It works so that
>>> you declare _AEI method below the GPIO controller listing the GPIOs you
>>> want to trigger events for and then either _Lxx, _Exx or _EVT method for
>>> each of them under the same controller. GPIO core then handles it
>>> automatically when you register the GPIO chip. See also
>>> acpi_gpiochip_request_interrupts().
>>
>> Right, that is was I read as well. Let's assume I would be able to patch the
>> tables: Would I describe all the logic of this patch in ACPI terms? Where to
>> enable interrupts, how to dispatch the SCI event, how to acknowledge it
>> etc.? Will it also take care of locking? (BTW, my locking seems to have some
>> remaining inconsistency, on second look.)
> 
> The GPIO core would then take care of it by requesting the GPIO in
> question and dispatching to the correct event handler. In this patch you
> just leave out the SCI part and only implement the irqchip like you did
> already.

Could you point me to a gpio driver that works like that already? Would be 
easier to learn that from an example. That infrastructure with all its different 
modes is seriously complex and not very well documented.

> 
>> And even if that were possible, we would be back to the square of existing
>> devices without those definitions. If this were a recent chipset, I would
>> say, "go, fix future firmware versions". But this one is legacy.
> 
> Is it fixing some real issue with these legacy platforms? I mean without
> the patch some GPE event is not handled properly? It was not clear to me
> from the commit message.
> 

Without that patch, you are forced to poll for event changes in your 
application, timer-driven. There are application that cannot process these GPIOs 
because they lack such logic (mraa with node-red-node-intel-gpio is a public 
example).

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Mika Westerberg]

On Wed, Apr 24, 2019 at 12:39:35PM +0200, Jan Kiszka wrote:
> On 24.04.19 12:33, Mika Westerberg wrote:
> > On Wed, Apr 24, 2019 at 12:19:02PM +0200, Jan Kiszka wrote:
> > > > I think what you want is "GPIO signaled ACPI event". It works so that
> > > > you declare _AEI method below the GPIO controller listing the GPIOs you
> > > > want to trigger events for and then either _Lxx, _Exx or _EVT method for
> > > > each of them under the same controller. GPIO core then handles it
> > > > automatically when you register the GPIO chip. See also
> > > > acpi_gpiochip_request_interrupts().
> > > 
> > > Right, that is was I read as well. Let's assume I would be able to patch the
> > > tables: Would I describe all the logic of this patch in ACPI terms? Where to
> > > enable interrupts, how to dispatch the SCI event, how to acknowledge it
> > > etc.? Will it also take care of locking? (BTW, my locking seems to have some
> > > remaining inconsistency, on second look.)
> > 
> > The GPIO core would then take care of it by requesting the GPIO in
> > question and dispatching to the correct event handler. In this patch you
> > just leave out the SCI part and only implement the irqchip like you did
> > already.
> 
> Could you point me to a gpio driver that works like that already? Would be
> easier to learn that from an example. That infrastructure with all its
> different modes is seriously complex and not very well documented.

Pretty much all drivers under drivers/pinctrl/intel.

> > > And even if that were possible, we would be back to the square of existing
> > > devices without those definitions. If this were a recent chipset, I would
> > > say, "go, fix future firmware versions". But this one is legacy.
> > 
> > Is it fixing some real issue with these legacy platforms? I mean without
> > the patch some GPE event is not handled properly? It was not clear to me
> > from the commit message.
> > 
> Without that patch, you are forced to poll for event changes in your
> application, timer-driven. There are application that cannot process these
> GPIOs because they lack such logic (mraa with node-red-node-intel-gpio is a
> public example).

But those are using the GPIOs via sysfs or the char device which should
work without the SCI handling part of your patch, no?

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Jan Kiszka]

On 24.04.19 12:46, Mika Westerberg wrote:
> On Wed, Apr 24, 2019 at 12:39:35PM +0200, Jan Kiszka wrote:
>> On 24.04.19 12:33, Mika Westerberg wrote:
>>> On Wed, Apr 24, 2019 at 12:19:02PM +0200, Jan Kiszka wrote:
>>>>> I think what you want is "GPIO signaled ACPI event". It works so that
>>>>> you declare _AEI method below the GPIO controller listing the GPIOs you
>>>>> want to trigger events for and then either _Lxx, _Exx or _EVT method for
>>>>> each of them under the same controller. GPIO core then handles it
>>>>> automatically when you register the GPIO chip. See also
>>>>> acpi_gpiochip_request_interrupts().
>>>>
>>>> Right, that is was I read as well. Let's assume I would be able to patch the
>>>> tables: Would I describe all the logic of this patch in ACPI terms? Where to
>>>> enable interrupts, how to dispatch the SCI event, how to acknowledge it
>>>> etc.? Will it also take care of locking? (BTW, my locking seems to have some
>>>> remaining inconsistency, on second look.)
>>>
>>> The GPIO core would then take care of it by requesting the GPIO in
>>> question and dispatching to the correct event handler. In this patch you
>>> just leave out the SCI part and only implement the irqchip like you did
>>> already.
>>
>> Could you point me to a gpio driver that works like that already? Would be
>> easier to learn that from an example. That infrastructure with all its
>> different modes is seriously complex and not very well documented.
> 
> Pretty much all drivers under drivers/pinctrl/intel.

OK... that's a purely descriptive way. So, provided we had such ACPI table 
entries, that plus some corresponding pinctrl driver would obsolete gpio-sch.c? 
Or are there other reason than historical ones for having gpio-*ch.c drivers around?

> 
>>>> And even if that were possible, we would be back to the square of existing
>>>> devices without those definitions. If this were a recent chipset, I would
>>>> say, "go, fix future firmware versions". But this one is legacy.
>>>
>>> Is it fixing some real issue with these legacy platforms? I mean without
>>> the patch some GPE event is not handled properly? It was not clear to me
>>> from the commit message.
>>>
>> Without that patch, you are forced to poll for event changes in your
>> application, timer-driven. There are application that cannot process these
>> GPIOs because they lack such logic (mraa with node-red-node-intel-gpio is a
>> public example).
> 
> But those are using the GPIOs via sysfs or the char device which should
> work without the SCI handling part of your patch, no?

They work via sysfs. How would the char dev compensate the missing interrupt 
support? By doing polling itself when the user wants an event? But I don't find 
any traces or timers.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Mika Westerberg]

On Wed, Apr 24, 2019 at 02:41:02PM +0200, Jan Kiszka wrote:
> On 24.04.19 12:46, Mika Westerberg wrote:
> > On Wed, Apr 24, 2019 at 12:39:35PM +0200, Jan Kiszka wrote:
> > > On 24.04.19 12:33, Mika Westerberg wrote:
> > > > On Wed, Apr 24, 2019 at 12:19:02PM +0200, Jan Kiszka wrote:
> > > > > > I think what you want is "GPIO signaled ACPI event". It works so that
> > > > > > you declare _AEI method below the GPIO controller listing the GPIOs you
> > > > > > want to trigger events for and then either _Lxx, _Exx or _EVT method for
> > > > > > each of them under the same controller. GPIO core then handles it
> > > > > > automatically when you register the GPIO chip. See also
> > > > > > acpi_gpiochip_request_interrupts().
> > > > > 
> > > > > Right, that is was I read as well. Let's assume I would be able to patch the
> > > > > tables: Would I describe all the logic of this patch in ACPI terms? Where to
> > > > > enable interrupts, how to dispatch the SCI event, how to acknowledge it
> > > > > etc.? Will it also take care of locking? (BTW, my locking seems to have some
> > > > > remaining inconsistency, on second look.)
> > > > 
> > > > The GPIO core would then take care of it by requesting the GPIO in
> > > > question and dispatching to the correct event handler. In this patch you
> > > > just leave out the SCI part and only implement the irqchip like you did
> > > > already.
> > > 
> > > Could you point me to a gpio driver that works like that already? Would be
> > > easier to learn that from an example. That infrastructure with all its
> > > different modes is seriously complex and not very well documented.
> > 
> > Pretty much all drivers under drivers/pinctrl/intel.
> 
> OK... that's a purely descriptive way. So, provided we had such ACPI table
> entries, that plus some corresponding pinctrl driver would obsolete
> gpio-sch.c? Or are there other reason than historical ones for having
> gpio-*ch.c drivers around?

No they are for different hardware. The GPIO core will parse necessary
ACPI entires when any GPIO driver (with ACPI description) calls
gpiochip_add_data() or any of the wrappers.

> > > > > And even if that were possible, we would be back to the square of existing
> > > > > devices without those definitions. If this were a recent chipset, I would
> > > > > say, "go, fix future firmware versions". But this one is legacy.
> > > > 
> > > > Is it fixing some real issue with these legacy platforms? I mean without
> > > > the patch some GPE event is not handled properly? It was not clear to me
> > > > from the commit message.
> > > > 
> > > Without that patch, you are forced to poll for event changes in your
> > > application, timer-driven. There are application that cannot process these
> > > GPIOs because they lack such logic (mraa with node-red-node-intel-gpio is a
> > > public example).
> > 
> > But those are using the GPIOs via sysfs or the char device which should
> > work without the SCI handling part of your patch, no?
> 
> They work via sysfs. How would the char dev compensate the missing interrupt
> support?

I'm trying to say that for the sysfs access (well or char dev) you
should not need the sch_sci_handler() thing that is in your current
patch.

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Jan Kiszka]

On 24.04.19 15:13, Mika Westerberg wrote:
> On Wed, Apr 24, 2019 at 02:41:02PM +0200, Jan Kiszka wrote:
>> On 24.04.19 12:46, Mika Westerberg wrote:
>>> On Wed, Apr 24, 2019 at 12:39:35PM +0200, Jan Kiszka wrote:
>>>> On 24.04.19 12:33, Mika Westerberg wrote:
>>>>> On Wed, Apr 24, 2019 at 12:19:02PM +0200, Jan Kiszka wrote:
>>>>>>> I think what you want is "GPIO signaled ACPI event". It works so that
>>>>>>> you declare _AEI method below the GPIO controller listing the GPIOs you
>>>>>>> want to trigger events for and then either _Lxx, _Exx or _EVT method for
>>>>>>> each of them under the same controller. GPIO core then handles it
>>>>>>> automatically when you register the GPIO chip. See also
>>>>>>> acpi_gpiochip_request_interrupts().
>>>>>>
>>>>>> Right, that is was I read as well. Let's assume I would be able to patch the
>>>>>> tables: Would I describe all the logic of this patch in ACPI terms? Where to
>>>>>> enable interrupts, how to dispatch the SCI event, how to acknowledge it
>>>>>> etc.? Will it also take care of locking? (BTW, my locking seems to have some
>>>>>> remaining inconsistency, on second look.)
>>>>>
>>>>> The GPIO core would then take care of it by requesting the GPIO in
>>>>> question and dispatching to the correct event handler. In this patch you
>>>>> just leave out the SCI part and only implement the irqchip like you did
>>>>> already.
>>>>
>>>> Could you point me to a gpio driver that works like that already? Would be
>>>> easier to learn that from an example. That infrastructure with all its
>>>> different modes is seriously complex and not very well documented.
>>>
>>> Pretty much all drivers under drivers/pinctrl/intel.
>>
>> OK... that's a purely descriptive way. So, provided we had such ACPI table
>> entries, that plus some corresponding pinctrl driver would obsolete
>> gpio-sch.c? Or are there other reason than historical ones for having
>> gpio-*ch.c drivers around?
> 
> No they are for different hardware. The GPIO core will parse necessary
> ACPI entires when any GPIO driver (with ACPI description) calls
> gpiochip_add_data() or any of the wrappers.
> 
>>>>>> And even if that were possible, we would be back to the square of existing
>>>>>> devices without those definitions. If this were a recent chipset, I would
>>>>>> say, "go, fix future firmware versions". But this one is legacy.
>>>>>
>>>>> Is it fixing some real issue with these legacy platforms? I mean without
>>>>> the patch some GPE event is not handled properly? It was not clear to me
>>>>> from the commit message.
>>>>>
>>>> Without that patch, you are forced to poll for event changes in your
>>>> application, timer-driven. There are application that cannot process these
>>>> GPIOs because they lack such logic (mraa with node-red-node-intel-gpio is a
>>>> public example).
>>>
>>> But those are using the GPIOs via sysfs or the char device which should
>>> work without the SCI handling part of your patch, no?
>>
>> They work via sysfs. How would the char dev compensate the missing interrupt
>> support?
> 
> I'm trying to say that for the sysfs access (well or char dev) you
> should not need the sch_sci_handler() thing that is in your current
> patch.

Then I'm still missing the black magic where - in my case - CGTS or RGTS are 
read, evaluated and written back.

And we would still need the gpio-sch driver to handle GGPE, GTNE, GTPE when edge 
events are requested? Is the a reference for /such/ a case? The newer Intels 
must be different then.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Mika Westerberg]

On Wed, Apr 24, 2019 at 04:24:16PM +0200, Jan Kiszka wrote:
> > I'm trying to say that for the sysfs access (well or char dev) you
> > should not need the sch_sci_handler() thing that is in your current
> > patch.
> 
> Then I'm still missing the black magic where - in my case - CGTS or RGTS are
> read, evaluated and written back.
> 
> And we would still need the gpio-sch driver to handle GGPE, GTNE, GTPE when
> edge events are requested? Is the a reference for /such/ a case? The newer
> Intels must be different then.

I realized the patch does not get an IRQ resource for the device so the
sch_sci_handler() is used to circumvent that. Yeah, if you don't have
IRQ resource available from the device ACPI description then I guess
hooking into ACPI SCI might be sensible thing to do after all...

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Andy Shevchenko]

On Wed, Apr 24, 2019 at 12:39:35PM +0200, Jan Kiszka wrote:
> On 24.04.19 12:33, Mika Westerberg wrote:
> > On Wed, Apr 24, 2019 at 12:19:02PM +0200, Jan Kiszka wrote:

> > > And even if that were possible, we would be back to the square of existing
> > > devices without those definitions. If this were a recent chipset, I would
> > > say, "go, fix future firmware versions". But this one is legacy.
> > 
> > Is it fixing some real issue with these legacy platforms? I mean without
> > the patch some GPE event is not handled properly? It was not clear to me
> > from the commit message.
> > 
> 
> Without that patch, you are forced to poll for event changes in your
> application, timer-driven. There are application that cannot process these
> GPIOs because they lack such logic (mraa with node-red-node-intel-gpio is a
> public example).

Just a side note: MRAA is a hack itself. It abuses almost all interfaces Linux
kernel provides.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Jan Kiszka]

On 26.04.19 15:06, Andy Shevchenko wrote:
> On Wed, Apr 24, 2019 at 12:39:35PM +0200, Jan Kiszka wrote:
>> On 24.04.19 12:33, Mika Westerberg wrote:
>>> On Wed, Apr 24, 2019 at 12:19:02PM +0200, Jan Kiszka wrote:
> 
>>>> And even if that were possible, we would be back to the square of existing
>>>> devices without those definitions. If this were a recent chipset, I would
>>>> say, "go, fix future firmware versions". But this one is legacy.
>>>
>>> Is it fixing some real issue with these legacy platforms? I mean without
>>> the patch some GPE event is not handled properly? It was not clear to me
>>> from the commit message.
>>>
>>
>> Without that patch, you are forced to poll for event changes in your
>> application, timer-driven. There are application that cannot process these
>> GPIOs because they lack such logic (mraa with node-red-node-intel-gpio is a
>> public example).
> 
> Just a side note: MRAA is a hack itself. It abuses almost all interfaces Linux
> kernel provides.
> 

Yes, very well aware of that. I have some patches that started to clean up at 
least parts of it, but that effort stalled over to many other fires. At the same 
time, there are no real alternatives - to my knowledge - for the value it brings 
(various bindings) to simply switch the engine.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Linus Walleij]

On Fri, Apr 26, 2019 at 3:06 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Wed, Apr 24, 2019 at 12:39:35PM +0200, Jan Kiszka wrote:
> > On 24.04.19 12:33, Mika Westerberg wrote:
> > > On Wed, Apr 24, 2019 at 12:19:02PM +0200, Jan Kiszka wrote:
>
> > > > And even if that were possible, we would be back to the square of existing
> > > > devices without those definitions. If this were a recent chipset, I would
> > > > say, "go, fix future firmware versions". But this one is legacy.
> > >
> > > Is it fixing some real issue with these legacy platforms? I mean without
> > > the patch some GPE event is not handled properly? It was not clear to me
> > > from the commit message.
> > >
> >
> > Without that patch, you are forced to poll for event changes in your
> > application, timer-driven. There are application that cannot process these
> > GPIOs because they lack such logic (mraa with node-red-node-intel-gpio is a
> > public example).
>
> Just a side note: MRAA is a hack itself. It abuses almost all interfaces Linux
> kernel provides.

I think it's pretty clean for GPIOs these days. My colleague Manivannan
was part of cleaning it up a while back and since then it is doing
what userspace should be doing if userspace absolutely cannot
abstain from using GPIOs directly (i.e. uses the character device).
https://github.com/intel-iot-devkit/mraa/blob/master/src/gpio/gpio_chardev.c

I don't know about other resources than GPIOs though.

Yours,
Linus Walleij

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Enrico Weigelt, metux IT consult]

On 26.04.19 15:36, Jan Kiszka wrote:

> At the same time, there are no real alternatives - to my> knowledge - for the value it brings (various bindings) to simply
switch> the engine.
Which value exactly does that collection of crude wrappers and broken
attempts to buypass the kernel (driving gpios via /dev/mem *facepalm*)
provide ?

mraa belongs to the category of software, I would never put onto any
production system. (yes, I already had a client who asked me to repair
his mraa-based software. finally, I've replaced mraa w/ a few LoC ...)


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Manivannan Sadhasivam]

On Fri, Apr 26, 2019 at 04:42:35PM +0200, Enrico Weigelt, metux IT consult wrote:
> On 26.04.19 15:36, Jan Kiszka wrote:
> 
> > At the same time, there are no real alternatives - to my> knowledge - for the value it brings (various bindings) to simply
> switch> the engine.
> Which value exactly does that collection of crude wrappers and broken
> attempts to buypass the kernel (driving gpios via /dev/mem *facepalm*)
> provide ?
> 

/dev/mem access to gpio's were done during the sysfs days (yes to bypass
the kernel interface) but now mraa will use chardev if the kernel is
chardev supported. Still, it just behaves the way you want (/dev/mem was
also optional back then) and I couldn't find a better alternative for playing
in the userspace with multiple languages.

-Mani

> mraa belongs to the category of software, I would never put onto any
> production system. (yes, I already had a client who asked me to repair
> his mraa-based software. finally, I've replaced mraa w/ a few LoC ...)
> 
> 
> --mtx
> 
> -- 
> Enrico Weigelt, metux IT consult
> Free software and Linux embedded engineering
> info@metux.net -- +49-151-27565287

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Jan Kiszka]

On 26.04.19 16:41, Linus Walleij wrote:
> On Fri, Apr 26, 2019 at 3:06 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>> On Wed, Apr 24, 2019 at 12:39:35PM +0200, Jan Kiszka wrote:
>>> On 24.04.19 12:33, Mika Westerberg wrote:
>>>> On Wed, Apr 24, 2019 at 12:19:02PM +0200, Jan Kiszka wrote:
>>
>>>>> And even if that were possible, we would be back to the square of existing
>>>>> devices without those definitions. If this were a recent chipset, I would
>>>>> say, "go, fix future firmware versions". But this one is legacy.
>>>>
>>>> Is it fixing some real issue with these legacy platforms? I mean without
>>>> the patch some GPE event is not handled properly? It was not clear to me
>>>> from the commit message.
>>>>
>>>
>>> Without that patch, you are forced to poll for event changes in your
>>> application, timer-driven. There are application that cannot process these
>>> GPIOs because they lack such logic (mraa with node-red-node-intel-gpio is a
>>> public example).
>>
>> Just a side note: MRAA is a hack itself. It abuses almost all interfaces Linux
>> kernel provides.
> 
> I think it's pretty clean for GPIOs these days. My colleague Manivannan
> was part of cleaning it up a while back and since then it is doing
> what userspace should be doing if userspace absolutely cannot
> abstain from using GPIOs directly (i.e. uses the character device).
> https://github.com/intel-iot-devkit/mraa/blob/master/src/gpio/gpio_chardev.c
> 
> I don't know about other resources than GPIOs though.

That's valuable progress!

OTOH, there still seem to be the broken pattern to address pins via hard-coded 
GPIO numbers. This broke our neck, e.g., when trying to replace the hacky BSP 
kernel with upstream. I started to create better infrastructure but never fished 
that.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Jan Kiszka]

On 26.04.19 16:42, Enrico Weigelt, metux IT consult wrote:
> On 26.04.19 15:36, Jan Kiszka wrote:
> 
>> At the same time, there are no real alternatives - to my> knowledge - for the value it brings (various bindings) to simply
> switch> the engine.
> Which value exactly does that collection of crude wrappers and broken
> attempts to buypass the kernel (driving gpios via /dev/mem *facepalm*)
> provide ?

Leaving that blunt hack aside:

import mraa

pin = mraa.Gpio(13)
pin.dir(mraa.DIR_OUT)
pin.write(1)

And the same goes for nodejs, java and c++.

Moreover, this allows you to abstract away where "Pin 13" actually came from on 
that board if the kernel changes (BSP -> upstream...) or the extension board or 
... We will exploit that when moving to a completely different base hardware 
with the next revision or our IOT2000.

> 
> mraa belongs to the category of software, I would never put onto any
> production system. (yes, I already had a client who asked me to repair
> his mraa-based software. finally, I've replaced mraa w/ a few LoC ...)

You also have to keep the class of "cool, I've just created my first Node.RED 
node!" IoT newbies in mind. These higher abstraction help to keep them away from 
the wrong lower APIs - or enable them to do something at all ("Cool, I've just 
connected my first two nodes!"). By far not all of them have consultants at hand.

And while we only ship our IOT2000 image with mraa and all the fun as reference 
image, it's way more for quite a few users. Even if you do not want to look 
behind the curtains of certain software components (that we primarily inherited 
and then started to clean up), they are working, or we would have heard.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Andy Shevchenko]

On Fri, Apr 26, 2019 at 7:05 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 26.04.19 16:42, Enrico Weigelt, metux IT consult wrote:
> > On 26.04.19 15:36, Jan Kiszka wrote:
> >
> >> At the same time, there are no real alternatives - to my> knowledge - for the value it brings (various bindings) to simply
> > switch> the engine.
> > Which value exactly does that collection of crude wrappers and broken
> > attempts to buypass the kernel (driving gpios via /dev/mem *facepalm*)
> > provide ?
>
> Leaving that blunt hack aside:
>
> import mraa
>
> pin = mraa.Gpio(13)
> pin.dir(mraa.DIR_OUT)
> pin.write(1)
>
> And the same goes for nodejs, java and c++.
>
> Moreover, this allows you to abstract away where "Pin 13" actually came from on
> that board if the kernel changes (BSP -> upstream...) or the extension board or
> ...

The problem here is opaque number. This has to be chip + *relative* pin number/
See this:
https://stackoverflow.com/questions/55532410/how-do-linux-gpio-numbers-get-their-values/55579640#55579640

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Manivannan Sadhasivam]

On Fri, Apr 26, 2019 at 08:20:19PM +0300, Andy Shevchenko wrote:
> On Fri, Apr 26, 2019 at 7:05 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >
> > On 26.04.19 16:42, Enrico Weigelt, metux IT consult wrote:
> > > On 26.04.19 15:36, Jan Kiszka wrote:
> > >
> > >> At the same time, there are no real alternatives - to my> knowledge - for the value it brings (various bindings) to simply
> > > switch> the engine.
> > > Which value exactly does that collection of crude wrappers and broken
> > > attempts to buypass the kernel (driving gpios via /dev/mem *facepalm*)
> > > provide ?
> >
> > Leaving that blunt hack aside:
> >
> > import mraa
> >
> > pin = mraa.Gpio(13)
> > pin.dir(mraa.DIR_OUT)
> > pin.write(1)
> >
> > And the same goes for nodejs, java and c++.
> >
> > Moreover, this allows you to abstract away where "Pin 13" actually came from on
> > that board if the kernel changes (BSP -> upstream...) or the extension board or
> > ...
> 
> The problem here is opaque number. This has to be chip + *relative* pin number/
> See this:
> https://stackoverflow.com/questions/55532410/how-do-linux-gpio-numbers-get-their-values/55579640#55579640
> 

But for platform like 96Boards we don't need controller specific lookup, these
are all handled by the platform code [1] so that the users can use the standard
pinout number to access GPIOs. For instance, pin 23 on the Low Speed expansion
header is the GPIO for all 96Boards platform, so the user can access that pin
using 23 itself in the application and it will run across all supported
96Boards.

That's one of the reason why we prefer MRAA.

Thanks,
Mani

[1] https://github.com/intel-iot-devkit/mraa/blob/master/src/arm/96boards.c#L75

> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Jan Kiszka]

On 26.04.19 19:20, Andy Shevchenko wrote:
> On Fri, Apr 26, 2019 at 7:05 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>
>> On 26.04.19 16:42, Enrico Weigelt, metux IT consult wrote:
>>> On 26.04.19 15:36, Jan Kiszka wrote:
>>>
>>>> At the same time, there are no real alternatives - to my> knowledge - for the value it brings (various bindings) to simply
>>> switch> the engine.
>>> Which value exactly does that collection of crude wrappers and broken
>>> attempts to buypass the kernel (driving gpios via /dev/mem *facepalm*)
>>> provide ?
>>
>> Leaving that blunt hack aside:
>>
>> import mraa
>>
>> pin = mraa.Gpio(13)
>> pin.dir(mraa.DIR_OUT)
>> pin.write(1)
>>
>> And the same goes for nodejs, java and c++.
>>
>> Moreover, this allows you to abstract away where "Pin 13" actually came from on
>> that board if the kernel changes (BSP -> upstream...) or the extension board or
>> ...
> 
> The problem here is opaque number. This has to be chip + *relative* pin number/
> See this:
> https://stackoverflow.com/questions/55532410/how-do-linux-gpio-numbers-get-their-values/55579640#55579640
> 

...and device path based discovery of the base:

https://github.com/siemens/mraa/commits/jan/queue

And I think that makes good use cases for application-level abstractions like in 
mraa. You don't want all user having to learn those nasty details, for 
devicetree, ACPI, and even worse stuff.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Jan Kiszka]

On 26.04.19 19:33, Manivannan Sadhasivam wrote:
> On Fri, Apr 26, 2019 at 08:20:19PM +0300, Andy Shevchenko wrote:
>> On Fri, Apr 26, 2019 at 7:05 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>
>>> On 26.04.19 16:42, Enrico Weigelt, metux IT consult wrote:
>>>> On 26.04.19 15:36, Jan Kiszka wrote:
>>>>
>>>>> At the same time, there are no real alternatives - to my> knowledge - for the value it brings (various bindings) to simply
>>>> switch> the engine.
>>>> Which value exactly does that collection of crude wrappers and broken
>>>> attempts to buypass the kernel (driving gpios via /dev/mem *facepalm*)
>>>> provide ?
>>>
>>> Leaving that blunt hack aside:
>>>
>>> import mraa
>>>
>>> pin = mraa.Gpio(13)
>>> pin.dir(mraa.DIR_OUT)
>>> pin.write(1)
>>>
>>> And the same goes for nodejs, java and c++.
>>>
>>> Moreover, this allows you to abstract away where "Pin 13" actually came from on
>>> that board if the kernel changes (BSP -> upstream...) or the extension board or
>>> ...
>>
>> The problem here is opaque number. This has to be chip + *relative* pin number/
>> See this:
>> https://stackoverflow.com/questions/55532410/how-do-linux-gpio-numbers-get-their-values/55579640#55579640
>>
> 
> But for platform like 96Boards we don't need controller specific lookup, these
> are all handled by the platform code [1] so that the users can use the standard
> pinout number to access GPIOs. For instance, pin 23 on the Low Speed expansion
> header is the GPIO for all 96Boards platform, so the user can access that pin
> using 23 itself in the application and it will run across all supported
> 96Boards.

Can you ensure stable numbering when probing order changes, e.g. due to adding 
an extension board?

Jan

> 
> That's one of the reason why we prefer MRAA.
> 
> Thanks,
> Mani
> 
> [1] https://github.com/intel-iot-devkit/mraa/blob/master/src/arm/96boards.c#L75
> 
>> -- 
>> With Best Regards,
>> Andy Shevchenko

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Andy Shevchenko]

On Fri, Apr 26, 2019 at 8:33 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
> On Fri, Apr 26, 2019 at 08:20:19PM +0300, Andy Shevchenko wrote:
> > On Fri, Apr 26, 2019 at 7:05 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> > > On 26.04.19 16:42, Enrico Weigelt, metux IT consult wrote:
> > > > On 26.04.19 15:36, Jan Kiszka wrote:

> > The problem here is opaque number. This has to be chip + *relative* pin number/
> > See this:
> > https://stackoverflow.com/questions/55532410/how-do-linux-gpio-numbers-get-their-values/55579640#55579640
> >
>
> But for platform like 96Boards we don't need controller specific lookup, these
> are all handled by the platform code [1] so that the users can use the standard
> pinout number to access GPIOs.

This is a complete mistake.

There is *no* global GPIO numbers anymore in Linux. (I don't count
very old legacy platforms)
Read above, it applies to DT or whatever resource provider.

> For instance, pin 23 on the Low Speed expansion
> header is the GPIO for all 96Boards platform, so the user can access that pin
> using 23 itself in the application and it will run across all supported
> 96Boards.
>
> That's one of the reason why we prefer MRAA.

> [1] https://github.com/intel-iot-devkit/mraa/blob/master/src/arm/96boards.c#L75

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Manivannan Sadhasivam]

On Fri, Apr 26, 2019 at 07:39:56PM +0200, Jan Kiszka wrote:
> On 26.04.19 19:33, Manivannan Sadhasivam wrote:
> > On Fri, Apr 26, 2019 at 08:20:19PM +0300, Andy Shevchenko wrote:
> > > On Fri, Apr 26, 2019 at 7:05 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> > > > 
> > > > On 26.04.19 16:42, Enrico Weigelt, metux IT consult wrote:
> > > > > On 26.04.19 15:36, Jan Kiszka wrote:
> > > > > 
> > > > > > At the same time, there are no real alternatives - to my> knowledge - for the value it brings (various bindings) to simply
> > > > > switch> the engine.
> > > > > Which value exactly does that collection of crude wrappers and broken
> > > > > attempts to buypass the kernel (driving gpios via /dev/mem *facepalm*)
> > > > > provide ?
> > > > 
> > > > Leaving that blunt hack aside:
> > > > 
> > > > import mraa
> > > > 
> > > > pin = mraa.Gpio(13)
> > > > pin.dir(mraa.DIR_OUT)
> > > > pin.write(1)
> > > > 
> > > > And the same goes for nodejs, java and c++.
> > > > 
> > > > Moreover, this allows you to abstract away where "Pin 13" actually came from on
> > > > that board if the kernel changes (BSP -> upstream...) or the extension board or
> > > > ...
> > > 
> > > The problem here is opaque number. This has to be chip + *relative* pin number/
> > > See this:
> > > https://stackoverflow.com/questions/55532410/how-do-linux-gpio-numbers-get-their-values/55579640#55579640
> > > 
> > 
> > But for platform like 96Boards we don't need controller specific lookup, these
> > are all handled by the platform code [1] so that the users can use the standard
> > pinout number to access GPIOs. For instance, pin 23 on the Low Speed expansion
> > header is the GPIO for all 96Boards platform, so the user can access that pin
> > using 23 itself in the application and it will run across all supported
> > 96Boards.
> 
> Can you ensure stable numbering when probing order changes, e.g. due to
> adding an extension board?
> 

Good point! For tackling this, I'm planning to introduce an API for accessing
the GPIO by its line name. It will be tricky to implement but once done, it
will serve.

Regards,
Mani

> Jan
> 
> > 
> > That's one of the reason why we prefer MRAA.
> > 
> > Thanks,
> > Mani
> > 
> > [1] https://github.com/intel-iot-devkit/mraa/blob/master/src/arm/96boards.c#L75
> > 
> > > -- 
> > > With Best Regards,
> > > Andy Shevchenko
> 
> -- 
> Siemens AG, Corporate Technology, CT RDA IOT SES-DE
> Corporate Competence Center Embedded Linux

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Manivannan Sadhasivam]

On Fri, Apr 26, 2019 at 08:44:36PM +0300, Andy Shevchenko wrote:
> On Fri, Apr 26, 2019 at 8:33 PM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> > On Fri, Apr 26, 2019 at 08:20:19PM +0300, Andy Shevchenko wrote:
> > > On Fri, Apr 26, 2019 at 7:05 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> > > > On 26.04.19 16:42, Enrico Weigelt, metux IT consult wrote:
> > > > > On 26.04.19 15:36, Jan Kiszka wrote:
> 
> > > The problem here is opaque number. This has to be chip + *relative* pin number/
> > > See this:
> > > https://stackoverflow.com/questions/55532410/how-do-linux-gpio-numbers-get-their-values/55579640#55579640
> > >
> >
> > But for platform like 96Boards we don't need controller specific lookup, these
> > are all handled by the platform code [1] so that the users can use the standard
> > pinout number to access GPIOs.
> 
> This is a complete mistake.
> 
> There is *no* global GPIO numbers anymore in Linux. (I don't count
> very old legacy platforms)
> Read above, it applies to DT or whatever resource provider.
> 

I think you misunderstood what I said. I referred the standard 96Boards
pinout and in the MRAA platform code, individual boards just map their
GPIO chip and line number based on that. I didn't mean the deprecated
global linux numbering.

https://github.com/intel-iot-devkit/mraa/blob/master/src/arm/96boards.c#L109

And of couse as Jan mentioned, the chip number will change when some
other external GPIO controller got probed before but so far we haven't
got to it!

Regards,
Mani

> > For instance, pin 23 on the Low Speed expansion
> > header is the GPIO for all 96Boards platform, so the user can access that pin
> > using 23 itself in the application and it will run across all supported
> > 96Boards.
> >
> > That's one of the reason why we prefer MRAA.
> 
> > [1] https://github.com/intel-iot-devkit/mraa/blob/master/src/arm/96boards.c#L75
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Jan Kiszka]

On 26.04.19 19:46, Manivannan Sadhasivam wrote:
> On Fri, Apr 26, 2019 at 07:39:56PM +0200, Jan Kiszka wrote:
>> On 26.04.19 19:33, Manivannan Sadhasivam wrote:
>>> On Fri, Apr 26, 2019 at 08:20:19PM +0300, Andy Shevchenko wrote:
>>>> On Fri, Apr 26, 2019 at 7:05 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>
>>>>> On 26.04.19 16:42, Enrico Weigelt, metux IT consult wrote:
>>>>>> On 26.04.19 15:36, Jan Kiszka wrote:
>>>>>>
>>>>>>> At the same time, there are no real alternatives - to my> knowledge - for the value it brings (various bindings) to simply
>>>>>> switch> the engine.
>>>>>> Which value exactly does that collection of crude wrappers and broken
>>>>>> attempts to buypass the kernel (driving gpios via /dev/mem *facepalm*)
>>>>>> provide ?
>>>>>
>>>>> Leaving that blunt hack aside:
>>>>>
>>>>> import mraa
>>>>>
>>>>> pin = mraa.Gpio(13)
>>>>> pin.dir(mraa.DIR_OUT)
>>>>> pin.write(1)
>>>>>
>>>>> And the same goes for nodejs, java and c++.
>>>>>
>>>>> Moreover, this allows you to abstract away where "Pin 13" actually came from on
>>>>> that board if the kernel changes (BSP -> upstream...) or the extension board or
>>>>> ...
>>>>
>>>> The problem here is opaque number. This has to be chip + *relative* pin number/
>>>> See this:
>>>> https://stackoverflow.com/questions/55532410/how-do-linux-gpio-numbers-get-their-values/55579640#55579640
>>>>
>>>
>>> But for platform like 96Boards we don't need controller specific lookup, these
>>> are all handled by the platform code [1] so that the users can use the standard
>>> pinout number to access GPIOs. For instance, pin 23 on the Low Speed expansion
>>> header is the GPIO for all 96Boards platform, so the user can access that pin
>>> using 23 itself in the application and it will run across all supported
>>> 96Boards.
>>
>> Can you ensure stable numbering when probing order changes, e.g. due to
>> adding an extension board?
>>
> 
> Good point! For tackling this, I'm planning to introduce an API for accessing
> the GPIO by its line name. It will be tricky to implement but once done, it
> will serve.

Whatever is stable and handy. As cited in the other thread, I played with the 
device path as well, see [1][2]. Feel free to pick what what is useful, I will 
likely not have time to work on that soon again.

Jan

[1] https://github.com/siemens/mraa/commit/034d787eea1a5b201ea77a6549ffc0bdbc3b776d
[2] https://github.com/siemens/mraa/commit/6fd8a3c27764718c1e62a4938a6dc88819c3f65f

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Manivannan Sadhasivam]

On Fri, Apr 26, 2019 at 07:52:48PM +0200, Jan Kiszka wrote:
> On 26.04.19 19:46, Manivannan Sadhasivam wrote:
> > On Fri, Apr 26, 2019 at 07:39:56PM +0200, Jan Kiszka wrote:
> > > On 26.04.19 19:33, Manivannan Sadhasivam wrote:
> > > > On Fri, Apr 26, 2019 at 08:20:19PM +0300, Andy Shevchenko wrote:
> > > > > On Fri, Apr 26, 2019 at 7:05 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> > > > > > 
> > > > > > On 26.04.19 16:42, Enrico Weigelt, metux IT consult wrote:
> > > > > > > On 26.04.19 15:36, Jan Kiszka wrote:
> > > > > > > 
> > > > > > > > At the same time, there are no real alternatives - to my> knowledge - for the value it brings (various bindings) to simply
> > > > > > > switch> the engine.
> > > > > > > Which value exactly does that collection of crude wrappers and broken
> > > > > > > attempts to buypass the kernel (driving gpios via /dev/mem *facepalm*)
> > > > > > > provide ?
> > > > > > 
> > > > > > Leaving that blunt hack aside:
> > > > > > 
> > > > > > import mraa
> > > > > > 
> > > > > > pin = mraa.Gpio(13)
> > > > > > pin.dir(mraa.DIR_OUT)
> > > > > > pin.write(1)
> > > > > > 
> > > > > > And the same goes for nodejs, java and c++.
> > > > > > 
> > > > > > Moreover, this allows you to abstract away where "Pin 13" actually came from on
> > > > > > that board if the kernel changes (BSP -> upstream...) or the extension board or
> > > > > > ...
> > > > > 
> > > > > The problem here is opaque number. This has to be chip + *relative* pin number/
> > > > > See this:
> > > > > https://stackoverflow.com/questions/55532410/how-do-linux-gpio-numbers-get-their-values/55579640#55579640
> > > > > 
> > > > 
> > > > But for platform like 96Boards we don't need controller specific lookup, these
> > > > are all handled by the platform code [1] so that the users can use the standard
> > > > pinout number to access GPIOs. For instance, pin 23 on the Low Speed expansion
> > > > header is the GPIO for all 96Boards platform, so the user can access that pin
> > > > using 23 itself in the application and it will run across all supported
> > > > 96Boards.
> > > 
> > > Can you ensure stable numbering when probing order changes, e.g. due to
> > > adding an extension board?
> > > 
> > 
> > Good point! For tackling this, I'm planning to introduce an API for accessing
> > the GPIO by its line name. It will be tricky to implement but once done, it
> > will serve.
> 
> Whatever is stable and handy. As cited in the other thread, I played with
> the device path as well, see [1][2]. Feel free to pick what what is useful,
> I will likely not have time to work on that soon again.
> 

Cool, thanks for sharing :-)

Regards,
Mani

> Jan
> 
> [1] https://github.com/siemens/mraa/commit/034d787eea1a5b201ea77a6549ffc0bdbc3b776d
> [2] https://github.com/siemens/mraa/commit/6fd8a3c27764718c1e62a4938a6dc88819c3f65f
> 
> -- 
> Siemens AG, Corporate Technology, CT RDA IOT SES-DE
> Corporate Competence Center Embedded Linux

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Andy Shevchenko]

On Fri, Apr 26, 2019 at 8:52 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
> On Fri, Apr 26, 2019 at 08:44:36PM +0300, Andy Shevchenko wrote:
> > On Fri, Apr 26, 2019 at 8:33 PM Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> wrote:
> > > On Fri, Apr 26, 2019 at 08:20:19PM +0300, Andy Shevchenko wrote:
> > > > On Fri, Apr 26, 2019 at 7:05 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> > > > > On 26.04.19 16:42, Enrico Weigelt, metux IT consult wrote:
> > > > > > On 26.04.19 15:36, Jan Kiszka wrote:
> >
> > > > The problem here is opaque number. This has to be chip + *relative* pin number/
> > > > See this:
> > > > https://stackoverflow.com/questions/55532410/how-do-linux-gpio-numbers-get-their-values/55579640#55579640
> > > >
> > >
> > > But for platform like 96Boards we don't need controller specific lookup, these
> > > are all handled by the platform code [1] so that the users can use the standard
> > > pinout number to access GPIOs.
> >
> > This is a complete mistake.
> >
> > There is *no* global GPIO numbers anymore in Linux. (I don't count
> > very old legacy platforms)
> > Read above, it applies to DT or whatever resource provider.
> >
>
> I think you misunderstood what I said.

I think you misunderstood what I said. :)

>  I referred the standard 96Boards
> pinout and in the MRAA platform code, individual boards just map their
> GPIO chip and line number based on that. I didn't mean the deprecated
> global linux numbering.

It can be easily broken by shuffling DT, kernel cnfiguration and
adding some GPIO expanders. Note, no C-code mangling is involved.

>
> https://github.com/intel-iot-devkit/mraa/blob/master/src/arm/96boards.c#L109
>
> And of couse as Jan mentioned, the chip number will change when some
> other external GPIO controller got probed before but so far we haven't
> got to it!


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Enrico Weigelt, metux IT consult]

On 26.04.19 18:03, Jan Kiszka wrote:

> Leaving that blunt hack aside:
> 
> import mraa
> 
> pin = mraa.Gpio(13)
> pin.dir(mraa.DIR_OUT)
> pin.write(1)
> 
> And the same goes for nodejs, java and c++.

Such trivial wrappers are easy write in the coffe break.
For those usecases the good old sysfs interface is really sufficient.

(I'm frequently replacing those kind of "abstraction layers" of sevaral
10kLoC in various client projects by just a few lines of really tiny
shim ...)

And relying on numeric gpio ids is generally a bad idea.
(exactly one of the kind of problems why certain clients call me in
great despair, when it broke again :p)

> Moreover, this allows you to abstract away where "Pin 13" actually came
> from on that board if the kernel changes (BSP -> upstream...) or the
> extension board or ... 

As said: numeric pin IDs are a *bad* idea.
If it's a *name* (=string), then that seems to be a usecase for labels.
For convenience, you could just pupulate the fs w/ proper symlinks that
are named by the pin names from the schematics.

> We will exploit that when moving to a completely
> different base hardware with the next revision or our IOT2000.

When doing that, you could also kick out Intel and move to ARM, as quite
everybody else does ;-)

<snip>

> You also have to keep the class of "cool, I've just created my first
> Node.RED node!" IoT newbies in mind.

As said above: writing a small js wrapper for that is just a coffee
break job.

> These higher abstraction help to
> keep them away from the wrong lower APIs - or enable them to do
> something at all ("Cool, I've just connected my first two nodes!"). 

gpio's are already very low-level. Highlevel would be things like keys,
leds, etc.

That's one of the things I frequently have to teach my clients: first
make yourself clear about what you're *actually* doing with these pins
and then pick the right drivers.

Okay, we could talk about creating some nice oftree-overlay generators,
to make it easier for newbies. Or create some small DSL, perhaps w/ some
tiny GUI editor. But don't let them play w/ raw gpio's - this quickly
goes horribly wrong (I've seen that in the field, many times).

It's nice, that you folks (Siemens) are now doing your own controller,
so you can finally throw out the ugly AMX+co crap. But please don't
repeat their mistakes. We don't need yet another raspi for building
automation - we need a different programming model than the old PLC-
style spaghetti configuration :p

(oh, and if you add ATEX and railways certification, I might have a new
customer for you ;-))

Okay, that's getting far OT, so maybe we should continue off-list ;-)

> By far not all of them have consultants at hand.

They better should :p

> And while we only ship our IOT2000 image with mraa and all the fun as
> reference image, it's way more for quite a few users. Even if you do not
> want to look behind the curtains of certain software components (that we
> primarily inherited and then started to clean up), they are working, or
> we would have heard.

Maybe because some field techs rather grab some beers and go to some
friend who happens to be a linux hacker, instead of burning their time
on the official channels those big corporate structures ;-)


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH 2/2] gpio: sch: Add interrupt support

[Linus Walleij]

Hi folks,

this became an interesting thread and Manivannan made some nice
changes to MRAA as a result.

It appears the Raspberry Pi has become a victim of its own success
and went from hobbyist toy system to a foundation for random industrial
control hacks. I'm a bit scared about that for electronic reasons but when
it comes to the software that is a board that get things right after
we added line-names to everything on its connectors.

What I can add is that when designing the character device I tried to get some
input from inductrial control (PLC) vendors and asked on the mailing list,
and mailed directly to ABB at one point, I am sorry for not finding the right
contact at Siemens (would have helped for sure).

I have tried to talk to Liebherr in related matters but can't seem to find the
right contact.

We really want to do things right with industrial control boards because
these seem to have very long life cycles and stay around forever in the
kernel for that reason.

Yours,
Linus Walleij