Re: acpiec on acer aspire S7 with CURRENT

2016-10-24 Thread Philip Guenther 
On Mon, Oct 24, 2016 at 3:01 PM, Mark Kettenis  wrote:
>> Date: Mon, 24 Oct 2016 23:01:22 +0300
>> From: Paul Irofti 
>>
>> On Mon, Oct 24, 2016 at 10:50:00PM +0300, Paul Irofti wrote:
>> > On Mon, Oct 24, 2016 at 07:46:33PM +0200, Mark Kettenis wrote:
>> > > > From: Paul Irofti 
>> > > > Date: Mon, 24 Oct 2016 17:12:01 +0300
>> > > >
>> > > > Any thoughts on this?
>> > >
>> > > Sorry, yes.  Adding the crs "index" as the last argument of the
>> > > callback function seems a bit non-intuitive to me.  I'd say the void *
>> > > argument should remain the last argument, and the crs "number" should
>> > > be the first, although I could live with it being the second.
>> > >
>> > > I feel a bit bad though for not suggesting that earlier.
>> >
>> > Sure, makes sense. I thought about doing that too, but I did not know
>> > how much breakage I could do to the original function.
>> >
>> > What about crsno, do you prefer it to be called crsidx? That might be
>> > a better name...
>
> I agree!
>
>> Like this?
>
> Yes, ok kettenis@

I agree as well.  ok guenther@

The other diff is looking good to me as well, cleaning things up and
eliminating the duplicate parsing better than I expected.  Thanks for
doing this work!


Philip

Re: acpiec on acer aspire S7 with CURRENT

2016-10-24 Thread Mark Kettenis 
> Date: Mon, 24 Oct 2016 23:01:22 +0300
> From: Paul Irofti 
> 
> On Mon, Oct 24, 2016 at 10:50:00PM +0300, Paul Irofti wrote:
> > On Mon, Oct 24, 2016 at 07:46:33PM +0200, Mark Kettenis wrote:
> > > > From: Paul Irofti 
> > > > Date: Mon, 24 Oct 2016 17:12:01 +0300
> > > > 
> > > > Any thoughts on this?
> > > 
> > > Sorry, yes.  Adding the crs "index" as the last argument of the
> > > callback function seems a bit non-intuitive to me.  I'd say the void *
> > > argument should remain the last argument, and the crs "number" should
> > > be the first, although I could live with it being the second.
> > > 
> > > I feel a bit bad though for not suggesting that earlier.
> > 
> > Sure, makes sense. I thought about doing that too, but I did not know
> > how much breakage I could do to the original function.
> > 
> > What about crsno, do you prefer it to be called crsidx? That might be
> > a better name...

I agree!

> Like this?

Yes, ok kettenis@


> Index: acpi.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpi.c,v
> retrieving revision 1.316
> diff -u -p -u -p -r1.316 acpi.c
> --- acpi.c18 Sep 2016 23:56:45 -  1.316
> +++ acpi.c24 Oct 2016 20:00:30 -
> @@ -513,10 +513,10 @@ TAILQ_HEAD(, acpi_pci) acpi_pcirootdevs 
>  TAILQ_HEAD_INITIALIZER(acpi_pcirootdevs);
>  
>  int acpi_getpci(struct aml_node *node, void *arg);
> -int acpi_getminbus(union acpi_resource *crs, void *arg);
> +int acpi_getminbus(int crsidx, union acpi_resource *crs, void *arg);
>  
>  int
> -acpi_getminbus(union acpi_resource *crs, void *arg)
> +acpi_getminbus(int crsidx, union acpi_resource *crs, void *arg)
>  {
>   int *bbn = arg;
>   int typ = AML_CRSTYPE(crs);
> Index: acpiprt.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpiprt.c,v
> retrieving revision 1.47
> diff -u -p -u -p -r1.47 acpiprt.c
> --- acpiprt.c 14 Mar 2015 03:38:46 -  1.47
> +++ acpiprt.c 24 Oct 2016 20:00:31 -
> @@ -60,8 +60,8 @@ SIMPLEQ_HEAD(, acpiprt_map) acpiprt_map_
>  
>  int  acpiprt_match(struct device *, void *, void *);
>  void acpiprt_attach(struct device *, struct device *, void *);
> -int  acpiprt_getirq(union acpi_resource *crs, void *arg);
> -int  acpiprt_chooseirq(union acpi_resource *, void *);
> +int  acpiprt_getirq(int, union acpi_resource *, void *);
> +int  acpiprt_chooseirq(int, union acpi_resource *, void *);
>  
>  struct acpiprt_softc {
>   struct device   sc_dev;
> @@ -137,7 +137,7 @@ acpiprt_attach(struct device *parent, st
>  }
>  
>  int
> -acpiprt_getirq(union acpi_resource *crs, void *arg)
> +acpiprt_getirq(int crsidx, union acpi_resource *crs, void *arg)
>  {
>   struct acpiprt_irq *irq = arg;
>   int typ, len;
> @@ -190,7 +190,7 @@ acpiprt_pri[16] = {
>  };
>  
>  int
> -acpiprt_chooseirq(union acpi_resource *crs, void *arg)
> +acpiprt_chooseirq(int crsidx, union acpi_resource *crs, void *arg)
>  {
>   struct acpiprt_irq *irq = arg;
>   int typ, len, i, pri = -1;
> Index: bytgpio.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/bytgpio.c,v
> retrieving revision 1.11
> diff -u -p -u -p -r1.11 bytgpio.c
> --- bytgpio.c 8 May 2016 11:08:01 -   1.11
> +++ bytgpio.c 24 Oct 2016 20:00:31 -
> @@ -104,7 +104,7 @@ const int byt_sus_pins[] = {
>   56, 54, 49, 55, 48, 57, 50, 58, 52, 53, 59, 40
>  };
>  
> -int  bytgpio_parse_resources(union acpi_resource *, void *);
> +int  bytgpio_parse_resources(int, union acpi_resource *, void *);
>  int  bytgpio_read_pin(void *, int);
>  void bytgpio_write_pin(void *, int, int);
>  void bytgpio_intr_establish(void *, int, int, int (*)(), void *);
> @@ -238,7 +238,7 @@ free:
>  }
>  
>  int
> -bytgpio_parse_resources(union acpi_resource *crs, void *arg)
> +bytgpio_parse_resources(int crsidx, union acpi_resource *crs, void *arg)
>  {
>   struct bytgpio_softc *sc = arg;
>   int type = AML_CRSTYPE(crs);
> Index: chvgpio.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/chvgpio.c,v
> retrieving revision 1.5
> diff -u -p -u -p -r1.5 chvgpio.c
> --- chvgpio.c 8 May 2016 18:18:42 -   1.5
> +++ chvgpio.c 24 Oct 2016 20:00:31 -
> @@ -143,7 +143,7 @@ const int chv_southeast_pins[] = {
>   8, 12, 6, 8, 10, 11, -1
>  };
>  
> -int  chvgpio_parse_resources(union acpi_resource *, void *);
> +int  chvgpio_parse_resources(int, union acpi_resource *, void *);
>  int  chvgpio_check_pin(struct chvgpio_softc *, int);
>  int  chvgpio_read_pin(void *, int);
>  void chvgpio_write_pin(void *, int, int);
> @@ -264,7 +264,7 @@ unmap:
>  }
>  
>  int
> -chvgpio_parse_resources(union acpi_resource *crs, void *arg)
> +chvgpio_parse_resources(int crsidx, union acpi_resource *crs, void *arg)
>  {
>   struct chvgpio_softc *sc = arg;
>   int type = AML_CRSTYPE(crs);

Re: acpiec on acer aspire S7 with CURRENT

2016-10-24 Thread Paul Irofti 
On Mon, Oct 24, 2016 at 10:50:00PM +0300, Paul Irofti wrote:
> On Mon, Oct 24, 2016 at 07:46:33PM +0200, Mark Kettenis wrote:
> > > From: Paul Irofti 
> > > Date: Mon, 24 Oct 2016 17:12:01 +0300
> > > 
> > > Any thoughts on this?
> > 
> > Sorry, yes.  Adding the crs "index" as the last argument of the
> > callback function seems a bit non-intuitive to me.  I'd say the void *
> > argument should remain the last argument, and the crs "number" should
> > be the first, although I could live with it being the second.
> > 
> > I feel a bit bad though for not suggesting that earlier.
> 
> Sure, makes sense. I thought about doing that too, but I did not know
> how much breakage I could do to the original function.
> 
> What about crsno, do you prefer it to be called crsidx? That might be
> a better name...

Like this?


Index: acpi.c
===
RCS file: /cvs/src/sys/dev/acpi/acpi.c,v
retrieving revision 1.316
diff -u -p -u -p -r1.316 acpi.c
--- acpi.c  18 Sep 2016 23:56:45 -  1.316
+++ acpi.c  24 Oct 2016 20:00:30 -
@@ -513,10 +513,10 @@ TAILQ_HEAD(, acpi_pci) acpi_pcirootdevs 
 TAILQ_HEAD_INITIALIZER(acpi_pcirootdevs);
 
 int acpi_getpci(struct aml_node *node, void *arg);
-int acpi_getminbus(union acpi_resource *crs, void *arg);
+int acpi_getminbus(int crsidx, union acpi_resource *crs, void *arg);
 
 int
-acpi_getminbus(union acpi_resource *crs, void *arg)
+acpi_getminbus(int crsidx, union acpi_resource *crs, void *arg)
 {
int *bbn = arg;
int typ = AML_CRSTYPE(crs);
Index: acpiprt.c
===
RCS file: /cvs/src/sys/dev/acpi/acpiprt.c,v
retrieving revision 1.47
diff -u -p -u -p -r1.47 acpiprt.c
--- acpiprt.c   14 Mar 2015 03:38:46 -  1.47
+++ acpiprt.c   24 Oct 2016 20:00:31 -
@@ -60,8 +60,8 @@ SIMPLEQ_HEAD(, acpiprt_map) acpiprt_map_
 
 intacpiprt_match(struct device *, void *, void *);
 void   acpiprt_attach(struct device *, struct device *, void *);
-intacpiprt_getirq(union acpi_resource *crs, void *arg);
-intacpiprt_chooseirq(union acpi_resource *, void *);
+intacpiprt_getirq(int, union acpi_resource *, void *);
+intacpiprt_chooseirq(int, union acpi_resource *, void *);
 
 struct acpiprt_softc {
struct device   sc_dev;
@@ -137,7 +137,7 @@ acpiprt_attach(struct device *parent, st
 }
 
 int
-acpiprt_getirq(union acpi_resource *crs, void *arg)
+acpiprt_getirq(int crsidx, union acpi_resource *crs, void *arg)
 {
struct acpiprt_irq *irq = arg;
int typ, len;
@@ -190,7 +190,7 @@ acpiprt_pri[16] = {
 };
 
 int
-acpiprt_chooseirq(union acpi_resource *crs, void *arg)
+acpiprt_chooseirq(int crsidx, union acpi_resource *crs, void *arg)
 {
struct acpiprt_irq *irq = arg;
int typ, len, i, pri = -1;
Index: bytgpio.c
===
RCS file: /cvs/src/sys/dev/acpi/bytgpio.c,v
retrieving revision 1.11
diff -u -p -u -p -r1.11 bytgpio.c
--- bytgpio.c   8 May 2016 11:08:01 -   1.11
+++ bytgpio.c   24 Oct 2016 20:00:31 -
@@ -104,7 +104,7 @@ const int byt_sus_pins[] = {
56, 54, 49, 55, 48, 57, 50, 58, 52, 53, 59, 40
 };
 
-intbytgpio_parse_resources(union acpi_resource *, void *);
+intbytgpio_parse_resources(int, union acpi_resource *, void *);
 intbytgpio_read_pin(void *, int);
 void   bytgpio_write_pin(void *, int, int);
 void   bytgpio_intr_establish(void *, int, int, int (*)(), void *);
@@ -238,7 +238,7 @@ free:
 }
 
 int
-bytgpio_parse_resources(union acpi_resource *crs, void *arg)
+bytgpio_parse_resources(int crsidx, union acpi_resource *crs, void *arg)
 {
struct bytgpio_softc *sc = arg;
int type = AML_CRSTYPE(crs);
Index: chvgpio.c
===
RCS file: /cvs/src/sys/dev/acpi/chvgpio.c,v
retrieving revision 1.5
diff -u -p -u -p -r1.5 chvgpio.c
--- chvgpio.c   8 May 2016 18:18:42 -   1.5
+++ chvgpio.c   24 Oct 2016 20:00:31 -
@@ -143,7 +143,7 @@ const int chv_southeast_pins[] = {
8, 12, 6, 8, 10, 11, -1
 };
 
-intchvgpio_parse_resources(union acpi_resource *, void *);
+intchvgpio_parse_resources(int, union acpi_resource *, void *);
 intchvgpio_check_pin(struct chvgpio_softc *, int);
 intchvgpio_read_pin(void *, int);
 void   chvgpio_write_pin(void *, int, int);
@@ -264,7 +264,7 @@ unmap:
 }
 
 int
-chvgpio_parse_resources(union acpi_resource *crs, void *arg)
+chvgpio_parse_resources(int crsidx, union acpi_resource *crs, void *arg)
 {
struct chvgpio_softc *sc = arg;
int type = AML_CRSTYPE(crs);
Index: dsdt.c
===
RCS file: /cvs/src/sys/dev/acpi/dsdt.c,v
retrieving revision 1.225
diff -u -p -u -p -r1.225 dsdt.c
--- dsdt.c  27 Sep 2016 10:04:19 -  1.225
+++ dsdt.c  24 Oct 2016 20:00:31 -
@@

Re: acpiec on acer aspire S7 with CURRENT

2016-10-24 Thread Paul Irofti 
On Mon, Oct 24, 2016 at 07:46:33PM +0200, Mark Kettenis wrote:
> > From: Paul Irofti 
> > Date: Mon, 24 Oct 2016 17:12:01 +0300
> > 
> > Any thoughts on this?
> 
> Sorry, yes.  Adding the crs "index" as the last argument of the
> callback function seems a bit non-intuitive to me.  I'd say the void *
> argument should remain the last argument, and the crs "number" should
> be the first, although I could live with it being the second.
> 
> I feel a bit bad though for not suggesting that earlier.

Sure, makes sense. I thought about doing that too, but I did not know
how much breakage I could do to the original function.

What about crsno, do you prefer it to be called crsidx? That might be
a better name...

Re: acpiec on acer aspire S7 with CURRENT

2016-10-24 Thread Mark Kettenis 
> From: Paul Irofti <p...@irofti.net>
> Date: Mon, 24 Oct 2016 17:12:01 +0300
> 
> Any thoughts on this?

Sorry, yes.  Adding the crs "index" as the last argument of the
callback function seems a bit non-intuitive to me.  I'd say the void *
argument should remain the last argument, and the crs "number" should
be the first, although I could live with it being the second.

I feel a bit bad though for not suggesting that earlier.

> -Mesaj original-
> De la: "Paul Irofti" <p...@irofti.net>
> Trimis: ‎22.‎10.‎2016 15:34
> Către: "Mark Kettenis" <mark.kette...@xs4all.nl>
> Cc: "guent...@gmail.com" <guent...@gmail.com>; "tech@openbsd.org" 
> <tech@openbsd.org>
> Subiect: Re: acpiec on acer aspire S7 with CURRENT
> 
> > I'm not really happy about the use of the static variable in the
> > resource parsing function.  It feels like the aml_parse_resource API
> > should be changed to pass the resource number to the callback
> > function.
> 
> I hated doing that as well, but I was afraid it wouldn't be justified to
> change the prototype and all the callers for this particular use case.
> Glad you suggested it. Here is a diff to add support for that in the
> current code.
> 
> 
> Index: acpi.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpi.c,v
> retrieving revision 1.316
> diff -u -p -u -p -r1.316 acpi.c
> --- acpi.c18 Sep 2016 23:56:45 -  1.316
> +++ acpi.c22 Oct 2016 12:29:11 -
> @@ -513,10 +513,10 @@ TAILQ_HEAD(, acpi_pci) acpi_pcirootdevs 
>  TAILQ_HEAD_INITIALIZER(acpi_pcirootdevs);
>  
>  int acpi_getpci(struct aml_node *node, void *arg);
> -int acpi_getminbus(union acpi_resource *crs, void *arg);
> +int acpi_getminbus(union acpi_resource *crs, void *arg, int crsno);
>  
>  int
> -acpi_getminbus(union acpi_resource *crs, void *arg)
> +acpi_getminbus(union acpi_resource *crs, void *arg, int crsno)
>  {
>   int *bbn = arg;
>   int typ = AML_CRSTYPE(crs);
> Index: acpiprt.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpiprt.c,v
> retrieving revision 1.47
> diff -u -p -u -p -r1.47 acpiprt.c
> --- acpiprt.c 14 Mar 2015 03:38:46 -  1.47
> +++ acpiprt.c 22 Oct 2016 12:29:11 -
> @@ -60,8 +60,8 @@ SIMPLEQ_HEAD(, acpiprt_map) acpiprt_map_
>  
>  int  acpiprt_match(struct device *, void *, void *);
>  void acpiprt_attach(struct device *, struct device *, void *);
> -int  acpiprt_getirq(union acpi_resource *crs, void *arg);
> -int  acpiprt_chooseirq(union acpi_resource *, void *);
> +int  acpiprt_getirq(union acpi_resource *crs, void *arg, int crsno);
> +int  acpiprt_chooseirq(union acpi_resource *, void *, int);
>  
>  struct acpiprt_softc {
>   struct device   sc_dev;
> @@ -137,7 +137,7 @@ acpiprt_attach(struct device *parent, st
>  }
>  
>  int
> -acpiprt_getirq(union acpi_resource *crs, void *arg)
> +acpiprt_getirq(union acpi_resource *crs, void *arg, int crsno)
>  {
>   struct acpiprt_irq *irq = arg;
>   int typ, len;
> @@ -190,7 +190,7 @@ acpiprt_pri[16] = {
>  };
>  
>  int
> -acpiprt_chooseirq(union acpi_resource *crs, void *arg)
> +acpiprt_chooseirq(union acpi_resource *crs, void *arg, int crsno)
>  {
>   struct acpiprt_irq *irq = arg;
>   int typ, len, i, pri = -1;
> Index: bytgpio.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/bytgpio.c,v
> retrieving revision 1.11
> diff -u -p -u -p -r1.11 bytgpio.c
> --- bytgpio.c 8 May 2016 11:08:01 -   1.11
> +++ bytgpio.c 22 Oct 2016 12:29:11 -
> @@ -104,7 +104,7 @@ const int byt_sus_pins[] = {
>   56, 54, 49, 55, 48, 57, 50, 58, 52, 53, 59, 40
>  };
>  
> -int  bytgpio_parse_resources(union acpi_resource *, void *);
> +int  bytgpio_parse_resources(union acpi_resource *, void *, int crsno);
>  int  bytgpio_read_pin(void *, int);
>  void bytgpio_write_pin(void *, int, int);
>  void bytgpio_intr_establish(void *, int, int, int (*)(), void *);
> @@ -238,7 +238,7 @@ free:
>  }
>  
>  int
> -bytgpio_parse_resources(union acpi_resource *crs, void *arg)
> +bytgpio_parse_resources(union acpi_resource *crs, void *arg, int crsno)
>  {
>   struct bytgpio_softc *sc = arg;
>   int type = AML_CRSTYPE(crs);
> Index: chvgpio.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/chvgpio.c,v
> retrieving revision 1.5
> diff -u -p -u -p -r1.5 chvgpio.c
> --- chvgpio.c 8 May 2016 18:18:42 -   1.5
> +++ chvgpio.c 22 Oct 2

Re: acpiec on acer aspire S7 with CURRENT

2016-10-24 Thread Paul Irofti 
Any thoughts on this?

-Mesaj original-
De la: "Paul Irofti" <p...@irofti.net>
Trimis: ‎22.‎10.‎2016 15:34
Către: "Mark Kettenis" <mark.kette...@xs4all.nl>
Cc: "guent...@gmail.com" <guent...@gmail.com>; "tech@openbsd.org" 
<tech@openbsd.org>
Subiect: Re: acpiec on acer aspire S7 with CURRENT

> I'm not really happy about the use of the static variable in the
> resource parsing function.  It feels like the aml_parse_resource API
> should be changed to pass the resource number to the callback
> function.

I hated doing that as well, but I was afraid it wouldn't be justified to
change the prototype and all the callers for this particular use case.
Glad you suggested it. Here is a diff to add support for that in the
current code.


Index: acpi.c
===
RCS file: /cvs/src/sys/dev/acpi/acpi.c,v
retrieving revision 1.316
diff -u -p -u -p -r1.316 acpi.c
--- acpi.c  18 Sep 2016 23:56:45 -  1.316
+++ acpi.c  22 Oct 2016 12:29:11 -
@@ -513,10 +513,10 @@ TAILQ_HEAD(, acpi_pci) acpi_pcirootdevs 
 TAILQ_HEAD_INITIALIZER(acpi_pcirootdevs);
 
 int acpi_getpci(struct aml_node *node, void *arg);
-int acpi_getminbus(union acpi_resource *crs, void *arg);
+int acpi_getminbus(union acpi_resource *crs, void *arg, int crsno);
 
 int
-acpi_getminbus(union acpi_resource *crs, void *arg)
+acpi_getminbus(union acpi_resource *crs, void *arg, int crsno)
 {
int *bbn = arg;
int typ = AML_CRSTYPE(crs);
Index: acpiprt.c
===
RCS file: /cvs/src/sys/dev/acpi/acpiprt.c,v
retrieving revision 1.47
diff -u -p -u -p -r1.47 acpiprt.c
--- acpiprt.c   14 Mar 2015 03:38:46 -  1.47
+++ acpiprt.c   22 Oct 2016 12:29:11 -
@@ -60,8 +60,8 @@ SIMPLEQ_HEAD(, acpiprt_map) acpiprt_map_
 
 intacpiprt_match(struct device *, void *, void *);
 void   acpiprt_attach(struct device *, struct device *, void *);
-intacpiprt_getirq(union acpi_resource *crs, void *arg);
-intacpiprt_chooseirq(union acpi_resource *, void *);
+intacpiprt_getirq(union acpi_resource *crs, void *arg, int crsno);
+intacpiprt_chooseirq(union acpi_resource *, void *, int);
 
 struct acpiprt_softc {
struct device   sc_dev;
@@ -137,7 +137,7 @@ acpiprt_attach(struct device *parent, st
 }
 
 int
-acpiprt_getirq(union acpi_resource *crs, void *arg)
+acpiprt_getirq(union acpi_resource *crs, void *arg, int crsno)
 {
struct acpiprt_irq *irq = arg;
int typ, len;
@@ -190,7 +190,7 @@ acpiprt_pri[16] = {
 };
 
 int
-acpiprt_chooseirq(union acpi_resource *crs, void *arg)
+acpiprt_chooseirq(union acpi_resource *crs, void *arg, int crsno)
 {
struct acpiprt_irq *irq = arg;
int typ, len, i, pri = -1;
Index: bytgpio.c
===
RCS file: /cvs/src/sys/dev/acpi/bytgpio.c,v
retrieving revision 1.11
diff -u -p -u -p -r1.11 bytgpio.c
--- bytgpio.c   8 May 2016 11:08:01 -   1.11
+++ bytgpio.c   22 Oct 2016 12:29:11 -
@@ -104,7 +104,7 @@ const int byt_sus_pins[] = {
56, 54, 49, 55, 48, 57, 50, 58, 52, 53, 59, 40
 };
 
-intbytgpio_parse_resources(union acpi_resource *, void *);
+intbytgpio_parse_resources(union acpi_resource *, void *, int crsno);
 intbytgpio_read_pin(void *, int);
 void   bytgpio_write_pin(void *, int, int);
 void   bytgpio_intr_establish(void *, int, int, int (*)(), void *);
@@ -238,7 +238,7 @@ free:
 }
 
 int
-bytgpio_parse_resources(union acpi_resource *crs, void *arg)
+bytgpio_parse_resources(union acpi_resource *crs, void *arg, int crsno)
 {
struct bytgpio_softc *sc = arg;
int type = AML_CRSTYPE(crs);
Index: chvgpio.c
===
RCS file: /cvs/src/sys/dev/acpi/chvgpio.c,v
retrieving revision 1.5
diff -u -p -u -p -r1.5 chvgpio.c
--- chvgpio.c   8 May 2016 18:18:42 -   1.5
+++ chvgpio.c   22 Oct 2016 12:29:11 -
@@ -143,7 +143,7 @@ const int chv_southeast_pins[] = {
8, 12, 6, 8, 10, 11, -1
 };
 
-intchvgpio_parse_resources(union acpi_resource *, void *);
+intchvgpio_parse_resources(union acpi_resource *, void *, int);
 intchvgpio_check_pin(struct chvgpio_softc *, int);
 intchvgpio_read_pin(void *, int);
 void   chvgpio_write_pin(void *, int, int);
@@ -264,7 +264,7 @@ unmap:
 }
 
 int
-chvgpio_parse_resources(union acpi_resource *crs, void *arg)
+chvgpio_parse_resources(union acpi_resource *crs, void *arg, int crsno)
 {
struct chvgpio_softc *sc = arg;
int type = AML_CRSTYPE(crs);
Index: dsdt.c
===
RCS file: /cvs/src/sys/dev/acpi/dsdt.c,v
retrieving revision 1.225
diff -u -p -u -p -r1.225 dsdt.c
--- dsdt.c  27 Sep 2016 10:04:19 -  1.225
+++ dsdt.c  22 Oct 2016 12:29:12 -
@@ -1621,14 +16

Re: acpiec on acer aspire S7 with CURRENT

2016-10-22 Thread Paul Irofti 
> I'm not really happy about the use of the static variable in the
> resource parsing function.  It feels like the aml_parse_resource API
> should be changed to pass the resource number to the callback
> function.

I hated doing that as well, but I was afraid it wouldn't be justified to
change the prototype and all the callers for this particular use case.
Glad you suggested it. Here is a diff to add support for that in the
current code.


Index: acpi.c
===
RCS file: /cvs/src/sys/dev/acpi/acpi.c,v
retrieving revision 1.316
diff -u -p -u -p -r1.316 acpi.c
--- acpi.c  18 Sep 2016 23:56:45 -  1.316
+++ acpi.c  22 Oct 2016 12:29:11 -
@@ -513,10 +513,10 @@ TAILQ_HEAD(, acpi_pci) acpi_pcirootdevs 
 TAILQ_HEAD_INITIALIZER(acpi_pcirootdevs);
 
 int acpi_getpci(struct aml_node *node, void *arg);
-int acpi_getminbus(union acpi_resource *crs, void *arg);
+int acpi_getminbus(union acpi_resource *crs, void *arg, int crsno);
 
 int
-acpi_getminbus(union acpi_resource *crs, void *arg)
+acpi_getminbus(union acpi_resource *crs, void *arg, int crsno)
 {
int *bbn = arg;
int typ = AML_CRSTYPE(crs);
Index: acpiprt.c
===
RCS file: /cvs/src/sys/dev/acpi/acpiprt.c,v
retrieving revision 1.47
diff -u -p -u -p -r1.47 acpiprt.c
--- acpiprt.c   14 Mar 2015 03:38:46 -  1.47
+++ acpiprt.c   22 Oct 2016 12:29:11 -
@@ -60,8 +60,8 @@ SIMPLEQ_HEAD(, acpiprt_map) acpiprt_map_
 
 intacpiprt_match(struct device *, void *, void *);
 void   acpiprt_attach(struct device *, struct device *, void *);
-intacpiprt_getirq(union acpi_resource *crs, void *arg);
-intacpiprt_chooseirq(union acpi_resource *, void *);
+intacpiprt_getirq(union acpi_resource *crs, void *arg, int crsno);
+intacpiprt_chooseirq(union acpi_resource *, void *, int);
 
 struct acpiprt_softc {
struct device   sc_dev;
@@ -137,7 +137,7 @@ acpiprt_attach(struct device *parent, st
 }
 
 int
-acpiprt_getirq(union acpi_resource *crs, void *arg)
+acpiprt_getirq(union acpi_resource *crs, void *arg, int crsno)
 {
struct acpiprt_irq *irq = arg;
int typ, len;
@@ -190,7 +190,7 @@ acpiprt_pri[16] = {
 };
 
 int
-acpiprt_chooseirq(union acpi_resource *crs, void *arg)
+acpiprt_chooseirq(union acpi_resource *crs, void *arg, int crsno)
 {
struct acpiprt_irq *irq = arg;
int typ, len, i, pri = -1;
Index: bytgpio.c
===
RCS file: /cvs/src/sys/dev/acpi/bytgpio.c,v
retrieving revision 1.11
diff -u -p -u -p -r1.11 bytgpio.c
--- bytgpio.c   8 May 2016 11:08:01 -   1.11
+++ bytgpio.c   22 Oct 2016 12:29:11 -
@@ -104,7 +104,7 @@ const int byt_sus_pins[] = {
56, 54, 49, 55, 48, 57, 50, 58, 52, 53, 59, 40
 };
 
-intbytgpio_parse_resources(union acpi_resource *, void *);
+intbytgpio_parse_resources(union acpi_resource *, void *, int crsno);
 intbytgpio_read_pin(void *, int);
 void   bytgpio_write_pin(void *, int, int);
 void   bytgpio_intr_establish(void *, int, int, int (*)(), void *);
@@ -238,7 +238,7 @@ free:
 }
 
 int
-bytgpio_parse_resources(union acpi_resource *crs, void *arg)
+bytgpio_parse_resources(union acpi_resource *crs, void *arg, int crsno)
 {
struct bytgpio_softc *sc = arg;
int type = AML_CRSTYPE(crs);
Index: chvgpio.c
===
RCS file: /cvs/src/sys/dev/acpi/chvgpio.c,v
retrieving revision 1.5
diff -u -p -u -p -r1.5 chvgpio.c
--- chvgpio.c   8 May 2016 18:18:42 -   1.5
+++ chvgpio.c   22 Oct 2016 12:29:11 -
@@ -143,7 +143,7 @@ const int chv_southeast_pins[] = {
8, 12, 6, 8, 10, 11, -1
 };
 
-intchvgpio_parse_resources(union acpi_resource *, void *);
+intchvgpio_parse_resources(union acpi_resource *, void *, int);
 intchvgpio_check_pin(struct chvgpio_softc *, int);
 intchvgpio_read_pin(void *, int);
 void   chvgpio_write_pin(void *, int, int);
@@ -264,7 +264,7 @@ unmap:
 }
 
 int
-chvgpio_parse_resources(union acpi_resource *crs, void *arg)
+chvgpio_parse_resources(union acpi_resource *crs, void *arg, int crsno)
 {
struct chvgpio_softc *sc = arg;
int type = AML_CRSTYPE(crs);
Index: dsdt.c
===
RCS file: /cvs/src/sys/dev/acpi/dsdt.c,v
retrieving revision 1.225
diff -u -p -u -p -r1.225 dsdt.c
--- dsdt.c  27 Sep 2016 10:04:19 -  1.225
+++ dsdt.c  22 Oct 2016 12:29:12 -
@@ -1621,14 +1621,14 @@ aml_mapresource(union acpi_resource *crs
 
 int
 aml_parse_resource(struct aml_value *res,
-int (*crs_enum)(union acpi_resource *, void *), void *arg)
+int (*crs_enum)(union acpi_resource *, void *, int), void *arg)
 {
-   int off, rlen;
+   int off, rlen, crsno;
union acpi_resource *crs;
 
if (res->type != AML_OBJTYPE_BUFFER ||

Re: acpiec on acer aspire S7 with CURRENT

2016-10-21 Thread Mark Kettenis 
> Date: Fri, 21 Oct 2016 10:37:08 +0300
> From: Paul Irofti 
> 
> On Tue, Oct 18, 2016 at 09:01:59PM -0700, Ilya Kaliman wrote:
> > Thanks! Now everything seems to work. Minor tweak - maybe need extra
> > newline after "acpiec0 at acpi0"
> 
> New diff that fixes the printfs, removes dead code (suggested by
> guenther@) and takes care of some small style(9) nits. OK?

I'm not really happy about the use of the static variable in the
resource parsing function.  It feels like the aml_parse_resource API
should be changed to pass the resource number to the callback
function.

> Index: acpidev.h
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpidev.h,v
> retrieving revision 1.38
> diff -u -p -u -p -r1.38 acpidev.h
> --- acpidev.h 12 Aug 2015 05:59:54 -  1.38
> +++ acpidev.h 21 Oct 2016 07:35:22 -
> @@ -323,10 +323,12 @@ struct acpiec_softc {
>   int sc_ecbusy;
>  
>   /* command/status register */
> + bus_size_t  sc_ec_sc;
>   bus_space_tag_t sc_cmd_bt;
>   bus_space_handle_t  sc_cmd_bh;
>  
>   /* data register */
> + bus_size_t  sc_ec_data;
>   bus_space_tag_t sc_data_bt;
>   bus_space_handle_t  sc_data_bh;
>  
> Index: acpiec.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpiec.c,v
> retrieving revision 1.54
> diff -u -p -u -p -r1.54 acpiec.c
> --- acpiec.c  23 Aug 2016 18:26:21 -  1.54
> +++ acpiec.c  21 Oct 2016 07:35:22 -
> @@ -48,7 +48,7 @@ voidacpiec_write(struct acpiec_softc *
>  
>  int  acpiec_getcrs(struct acpiec_softc *,
>   struct acpi_attach_args *);
> -int  acpiec_getregister(const u_int8_t *, int, int *, bus_size_t *);
> +int  acpiec_parse_resources(union acpi_resource *, void *);
>  
>  void acpiec_wait(struct acpiec_softc *, u_int8_t, u_int8_t);
>  void acpiec_sci_event(struct acpiec_softc *);
> @@ -285,15 +285,16 @@ acpiec_attach(struct device *parent, str
>   return;
>   }
>  
> + printf("\n");
>   if (acpiec_getcrs(sc, aa)) {
> - printf(": Failed to read resource settings\n");
> + printf("%s: Failed to read resource settings\n", DEVNAME(sc));
>   return;
>   }
>  
>   sc->sc_acpi->sc_ec = sc;
>  
>   if (acpiec_reg(sc)) {
> - printf(": Failed to register address space\n");
> + printf("%s: Failed to register address space\n", DEVNAME(sc));
>   return;
>   }
>  
> @@ -305,15 +306,13 @@ acpiec_attach(struct device *parent, str
>   acpi_set_gpehandler(sc->sc_acpi, sc->sc_gpe, acpiec_gpehandler,
>   sc, 1);
>  #endif
> - 
> +
>   if (aml_evalname(sc->sc_acpi, sc->sc_devnode, "_GLK", 0, NULL, ))
>   sc->sc_glk = 0;
>   else if (res.type != AML_OBJTYPE_INTEGER)
>   sc->sc_glk = 0;
>   else
>   sc->sc_glk = res.v_integer ? 1 : 0;
> -
> - printf("\n");
>  }
>  
>  void
> @@ -366,68 +365,75 @@ acpiec_gpehandler(struct acpi_softc *acp
>   return (0);
>  }
>  
> -/* parse the resource buffer to get a 'register' value */
>  int
> -acpiec_getregister(const u_int8_t *buf, int size, int *type, bus_size_t 
> *addr)
> +acpiec_parse_resources(union acpi_resource *crs, void *arg)
>  {
> - int len, hlen;
> + struct acpiec_softc *sc = arg;
> + int type = AML_CRSTYPE(crs);
>  
> -#define RES_TYPE_MASK 0x80
> -#define RES_LENGTH_MASK 0x07
> -#define RES_TYPE_IOPORT  0x47
> -#define RES_TYPE_ENDTAG  0x79
> + static int argno = 0;
>  
> - if (size <= 0)
> - return (0);
> -
> - if (*buf & RES_TYPE_MASK) {
> - /* large resource */
> - if (size < 3)
> - return (1);
> - len = (int)buf[1] + 256 * (int)buf[2];
> - hlen = 3;
> - } else {
> - /* small resource */
> - len = buf[0] & RES_LENGTH_MASK;
> - hlen = 1;
> + switch (argno) {
> + case 0:
> + if (type != SR_IOPORT) {
> + printf("%s: Unexpected resource #%d type %d\n",
> + DEVNAME(sc), argno, type);
> + break;
> + }
> + sc->sc_data_bt = sc->sc_acpi->sc_iot;
> + sc->sc_ec_data = crs->sr_ioport._max;
> + break;
> + case 1:
> + if (type != SR_IOPORT) {
> + printf("%s: Unexpected resource #%d type %d\n",
> + DEVNAME(sc), argno, type);
> + break;
> + }
> + sc->sc_cmd_bt = sc->sc_acpi->sc_iot;
> + sc->sc_ec_sc = crs->sr_ioport._max;
> + break;
> + case 2:
> + if (!sc->sc_acpi->sc_hw_reduced) {
> +

Re: acpiec on acer aspire S7 with CURRENT

2016-10-21 Thread Ilya Kaliman 
Works for me. Ok.

acpiprt11 at acpi0: bus -1 (PEG1)
acpiprt12 at acpi0: bus -1 (PEG2)
acpiec0 at acpi0
acpiec0: Not running on HW-Reduced ACPI type 8
acpiec0: invalid resource #3 type 134
acpicpu0 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33),
C1(1000@1 mwait.1), PSS
acpicpu1 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33),
C1(1000@1 mwait.1), PSS

Thanks,
Ilya

On Fri, Oct 21, 2016 at 12:37 AM, Paul Irofti  wrote:
> On Tue, Oct 18, 2016 at 09:01:59PM -0700, Ilya Kaliman wrote:
>> Thanks! Now everything seems to work. Minor tweak - maybe need extra
>> newline after "acpiec0 at acpi0"
>
> New diff that fixes the printfs, removes dead code (suggested by
> guenther@) and takes care of some small style(9) nits. OK?
>
>
> Index: acpidev.h
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpidev.h,v
> retrieving revision 1.38
> diff -u -p -u -p -r1.38 acpidev.h
> --- acpidev.h   12 Aug 2015 05:59:54 -  1.38
> +++ acpidev.h   21 Oct 2016 07:35:22 -
> @@ -323,10 +323,12 @@ struct acpiec_softc {
> int sc_ecbusy;
>
> /* command/status register */
> +   bus_size_t  sc_ec_sc;
> bus_space_tag_t sc_cmd_bt;
> bus_space_handle_t  sc_cmd_bh;
>
> /* data register */
> +   bus_size_t  sc_ec_data;
> bus_space_tag_t sc_data_bt;
> bus_space_handle_t  sc_data_bh;
>
> Index: acpiec.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpiec.c,v
> retrieving revision 1.54
> diff -u -p -u -p -r1.54 acpiec.c
> --- acpiec.c23 Aug 2016 18:26:21 -  1.54
> +++ acpiec.c21 Oct 2016 07:35:22 -
> @@ -48,7 +48,7 @@ void  acpiec_write(struct acpiec_softc *
>
>  intacpiec_getcrs(struct acpiec_softc *,
> struct acpi_attach_args *);
> -intacpiec_getregister(const u_int8_t *, int, int *, bus_size_t 
> *);
> +intacpiec_parse_resources(union acpi_resource *, void *);
>
>  void   acpiec_wait(struct acpiec_softc *, u_int8_t, u_int8_t);
>  void   acpiec_sci_event(struct acpiec_softc *);
> @@ -285,15 +285,16 @@ acpiec_attach(struct device *parent, str
> return;
> }
>
> +   printf("\n");
> if (acpiec_getcrs(sc, aa)) {
> -   printf(": Failed to read resource settings\n");
> +   printf("%s: Failed to read resource settings\n", DEVNAME(sc));
> return;
> }
>
> sc->sc_acpi->sc_ec = sc;
>
> if (acpiec_reg(sc)) {
> -   printf(": Failed to register address space\n");
> +   printf("%s: Failed to register address space\n", DEVNAME(sc));
> return;
> }
>
> @@ -305,15 +306,13 @@ acpiec_attach(struct device *parent, str
> acpi_set_gpehandler(sc->sc_acpi, sc->sc_gpe, acpiec_gpehandler,
> sc, 1);
>  #endif
> -
> +
> if (aml_evalname(sc->sc_acpi, sc->sc_devnode, "_GLK", 0, NULL, ))
> sc->sc_glk = 0;
> else if (res.type != AML_OBJTYPE_INTEGER)
> sc->sc_glk = 0;
> else
> sc->sc_glk = res.v_integer ? 1 : 0;
> -
> -   printf("\n");
>  }
>
>  void
> @@ -366,68 +365,75 @@ acpiec_gpehandler(struct acpi_softc *acp
> return (0);
>  }
>
> -/* parse the resource buffer to get a 'register' value */
>  int
> -acpiec_getregister(const u_int8_t *buf, int size, int *type, bus_size_t 
> *addr)
> +acpiec_parse_resources(union acpi_resource *crs, void *arg)
>  {
> -   int len, hlen;
> +   struct acpiec_softc *sc = arg;
> +   int type = AML_CRSTYPE(crs);
>
> -#define RES_TYPE_MASK 0x80
> -#define RES_LENGTH_MASK 0x07
> -#define RES_TYPE_IOPORT0x47
> -#define RES_TYPE_ENDTAG0x79
> +   static int argno = 0;
>
> -   if (size <= 0)
> -   return (0);
> -
> -   if (*buf & RES_TYPE_MASK) {
> -   /* large resource */
> -   if (size < 3)
> -   return (1);
> -   len = (int)buf[1] + 256 * (int)buf[2];
> -   hlen = 3;
> -   } else {
> -   /* small resource */
> -   len = buf[0] & RES_LENGTH_MASK;
> -   hlen = 1;
> +   switch (argno) {
> +   case 0:
> +   if (type != SR_IOPORT) {
> +   printf("%s: Unexpected resource #%d type %d\n",
> +   DEVNAME(sc), argno, type);
> +   break;
> +   }
> +   sc->sc_data_bt = sc->sc_acpi->sc_iot;
> +   sc->sc_ec_data = crs->sr_ioport._max;
> +   break;
> +   case 1:
> +   if (type != SR_IOPORT) {
> +   printf("%s: Unexpected resource #%d type %d\n",
> +

Re: acpiec on acer aspire S7 with CURRENT

2016-10-21 Thread Paul Irofti 
On Tue, Oct 18, 2016 at 09:01:59PM -0700, Ilya Kaliman wrote:
> Thanks! Now everything seems to work. Minor tweak - maybe need extra
> newline after "acpiec0 at acpi0"

New diff that fixes the printfs, removes dead code (suggested by
guenther@) and takes care of some small style(9) nits. OK?


Index: acpidev.h
===
RCS file: /cvs/src/sys/dev/acpi/acpidev.h,v
retrieving revision 1.38
diff -u -p -u -p -r1.38 acpidev.h
--- acpidev.h   12 Aug 2015 05:59:54 -  1.38
+++ acpidev.h   21 Oct 2016 07:35:22 -
@@ -323,10 +323,12 @@ struct acpiec_softc {
int sc_ecbusy;
 
/* command/status register */
+   bus_size_t  sc_ec_sc;
bus_space_tag_t sc_cmd_bt;
bus_space_handle_t  sc_cmd_bh;
 
/* data register */
+   bus_size_t  sc_ec_data;
bus_space_tag_t sc_data_bt;
bus_space_handle_t  sc_data_bh;
 
Index: acpiec.c
===
RCS file: /cvs/src/sys/dev/acpi/acpiec.c,v
retrieving revision 1.54
diff -u -p -u -p -r1.54 acpiec.c
--- acpiec.c23 Aug 2016 18:26:21 -  1.54
+++ acpiec.c21 Oct 2016 07:35:22 -
@@ -48,7 +48,7 @@ void  acpiec_write(struct acpiec_softc *
 
 intacpiec_getcrs(struct acpiec_softc *,
struct acpi_attach_args *);
-intacpiec_getregister(const u_int8_t *, int, int *, bus_size_t *);
+intacpiec_parse_resources(union acpi_resource *, void *);
 
 void   acpiec_wait(struct acpiec_softc *, u_int8_t, u_int8_t);
 void   acpiec_sci_event(struct acpiec_softc *);
@@ -285,15 +285,16 @@ acpiec_attach(struct device *parent, str
return;
}
 
+   printf("\n");
if (acpiec_getcrs(sc, aa)) {
-   printf(": Failed to read resource settings\n");
+   printf("%s: Failed to read resource settings\n", DEVNAME(sc));
return;
}
 
sc->sc_acpi->sc_ec = sc;
 
if (acpiec_reg(sc)) {
-   printf(": Failed to register address space\n");
+   printf("%s: Failed to register address space\n", DEVNAME(sc));
return;
}
 
@@ -305,15 +306,13 @@ acpiec_attach(struct device *parent, str
acpi_set_gpehandler(sc->sc_acpi, sc->sc_gpe, acpiec_gpehandler,
sc, 1);
 #endif
-   
+
if (aml_evalname(sc->sc_acpi, sc->sc_devnode, "_GLK", 0, NULL, ))
sc->sc_glk = 0;
else if (res.type != AML_OBJTYPE_INTEGER)
sc->sc_glk = 0;
else
sc->sc_glk = res.v_integer ? 1 : 0;
-
-   printf("\n");
 }
 
 void
@@ -366,68 +365,75 @@ acpiec_gpehandler(struct acpi_softc *acp
return (0);
 }
 
-/* parse the resource buffer to get a 'register' value */
 int
-acpiec_getregister(const u_int8_t *buf, int size, int *type, bus_size_t *addr)
+acpiec_parse_resources(union acpi_resource *crs, void *arg)
 {
-   int len, hlen;
+   struct acpiec_softc *sc = arg;
+   int type = AML_CRSTYPE(crs);
 
-#define RES_TYPE_MASK 0x80
-#define RES_LENGTH_MASK 0x07
-#define RES_TYPE_IOPORT0x47
-#define RES_TYPE_ENDTAG0x79
+   static int argno = 0;
 
-   if (size <= 0)
-   return (0);
-
-   if (*buf & RES_TYPE_MASK) {
-   /* large resource */
-   if (size < 3)
-   return (1);
-   len = (int)buf[1] + 256 * (int)buf[2];
-   hlen = 3;
-   } else {
-   /* small resource */
-   len = buf[0] & RES_LENGTH_MASK;
-   hlen = 1;
+   switch (argno) {
+   case 0:
+   if (type != SR_IOPORT) {
+   printf("%s: Unexpected resource #%d type %d\n",
+   DEVNAME(sc), argno, type);
+   break;
+   }
+   sc->sc_data_bt = sc->sc_acpi->sc_iot;
+   sc->sc_ec_data = crs->sr_ioport._max;
+   break;
+   case 1:
+   if (type != SR_IOPORT) {
+   printf("%s: Unexpected resource #%d type %d\n",
+   DEVNAME(sc), argno, type);
+   break;
+   }
+   sc->sc_cmd_bt = sc->sc_acpi->sc_iot;
+   sc->sc_ec_sc = crs->sr_ioport._max;
+   break;
+   case 2:
+   if (!sc->sc_acpi->sc_hw_reduced) {
+   printf("%s: Not running on HW-Reduced ACPI type %d\n",
+   DEVNAME(sc), type);
+   break;
+   }
+   /* XXX: handle SCI GPIO  */
+   break;
+   default:
+   printf("%s: invalid resource #%d type %d\n",
+   DEVNAME(sc), argno, type);
}
 
-   /* XXX todo: decode other types */
-

Re: acpiec on acer aspire S7 with CURRENT

2016-10-18 Thread Ilya Kaliman 
Thanks! Now everything seems to work. Minor tweak - maybe need extra
newline after "acpiec0 at acpi0"

acpiprt9 at acpi0: bus -1 (RP08)
acpiprt10 at acpi0: bus -1 (PEG0)
acpiprt11 at acpi0: bus -1 (PEG1)
acpiprt12 at acpi0: bus -1 (PEG2)
acpiec0 at acpi0acpiec0: Not running on HW-Reduced ACPI type 8
acpiec0: invalid resource #3 type 134

acpicpu0 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33), C1(1000@1
 mwait.1), PSS
acpicpu1 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33), C1(1000@1
 mwait.1), PSS

On Tue, Oct 18, 2016 at 11:31 AM, Paul Irofti  wrote:
> On Tue, Oct 18, 2016 at 10:22:06AM -0700, Ilya Kaliman wrote:
>> Hi!
>>
>> The patch seems to work partially - acpiec now gets initialized. The
>> suspend-on-lid-close does not work anymore, though (it works with my
>> initial patch).
>
> Right, that's because I had an off-by-one in the switch case. I wonder
> how my laptop managed to function with that bug...
>
> Please try again with this diff.
>
>
> Index: acpidev.h
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpidev.h,v
> retrieving revision 1.38
> diff -u -p -u -p -r1.38 acpidev.h
> --- acpidev.h   12 Aug 2015 05:59:54 -  1.38
> +++ acpidev.h   18 Oct 2016 18:30:24 -
> @@ -323,10 +323,12 @@ struct acpiec_softc {
> int sc_ecbusy;
>
> /* command/status register */
> +   bus_size_t  sc_ec_sc;
> bus_space_tag_t sc_cmd_bt;
> bus_space_handle_t  sc_cmd_bh;
>
> /* data register */
> +   bus_size_t  sc_ec_data;
> bus_space_tag_t sc_data_bt;
> bus_space_handle_t  sc_data_bh;
>
> Index: acpiec.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpiec.c,v
> retrieving revision 1.54
> diff -u -p -u -p -r1.54 acpiec.c
> --- acpiec.c23 Aug 2016 18:26:21 -  1.54
> +++ acpiec.c18 Oct 2016 18:30:25 -
> @@ -407,13 +407,53 @@ acpiec_getregister(const u_int8_t *buf,
>  }
>
>  int
> +acpiec_parse_resources(union acpi_resource *crs, void *arg)
> +{
> +   struct acpiec_softc *sc = arg;
> +   int type = AML_CRSTYPE(crs);
> +
> +   static int argno = 0;
> +
> +   switch (argno) {
> +   case 0:
> +   if (type != SR_IOPORT) {
> +   printf("%s: Unexpected resource #%d type %d\n",
> +   DEVNAME(sc), argno, type);
> +   break;
> +   }
> +   sc->sc_data_bt = sc->sc_acpi->sc_iot;
> +   sc->sc_ec_data = crs->sr_ioport._max;
> +   break;
> +   case 1:
> +   if (type != SR_IOPORT) {
> +   printf("%s: Unexpected resource #%d type %d\n",
> +   DEVNAME(sc), argno, type);
> +   break;
> +   }
> +   sc->sc_cmd_bt = sc->sc_acpi->sc_iot;
> +   sc->sc_ec_sc = crs->sr_ioport._max;
> +   break;
> +   case 2:
> +   if (!sc->sc_acpi->sc_hw_reduced) {
> +   printf("%s: Not running on HW-Reduced ACPI type %d\n",
> +   DEVNAME(sc), type);
> +   break;
> +   }
> +   /* XXX: handle SCI GPIO  */
> +   break;
> +   default:
> +   printf("%s: invalid resource #%d type %d\n",
> +   DEVNAME(sc), argno, type);
> +   }
> +
> +   argno++;
> +   return 0;
> +}
> +
> +int
>  acpiec_getcrs(struct acpiec_softc *sc, struct acpi_attach_args *aa)
>  {
> struct aml_valueres;
> -   bus_size_t  ec_sc, ec_data;
> -   int dtype, ctype;
> -   char*buf;
> -   int size, ret;
> int64_t gpe;
> struct acpi_ecdt*ecdt = aa->aaa_table;
> extern struct aml_node  aml_root;
> @@ -423,11 +463,17 @@ acpiec_getcrs(struct acpiec_softc *sc, s
> /* Get GPE, Data and Control segments */
> sc->sc_gpe = ecdt->gpe_bit;
>
> -   ctype = ecdt->ec_control.address_space_id;
> -   ec_sc = ecdt->ec_control.address;
> -
> -   dtype = ecdt->ec_data.address_space_id;
> -   ec_data = ecdt->ec_data.address;
> +   if (ecdt->ec_control.address_space_id == GAS_SYSTEM_IOSPACE)
> +   sc->sc_cmd_bt = sc->sc_acpi->sc_iot;
> +   else
> +   sc->sc_cmd_bt = sc->sc_acpi->sc_memt;
> +   sc->sc_ec_sc = ecdt->ec_control.address;
> +
> +   if (ecdt->ec_data.address_space_id == GAS_SYSTEM_IOSPACE)
> +   sc->sc_data_bt = sc->sc_acpi->sc_iot;
> +   else
> +   sc->sc_data_bt = sc->sc_acpi->sc_memt;
> +

Re: acpiec on acer aspire S7 with CURRENT

2016-10-18 Thread Paul Irofti 
On Tue, Oct 18, 2016 at 10:22:06AM -0700, Ilya Kaliman wrote:
> Hi!
> 
> The patch seems to work partially - acpiec now gets initialized. The
> suspend-on-lid-close does not work anymore, though (it works with my
> initial patch).

Right, that's because I had an off-by-one in the switch case. I wonder
how my laptop managed to function with that bug...

Please try again with this diff.


Index: acpidev.h
===
RCS file: /cvs/src/sys/dev/acpi/acpidev.h,v
retrieving revision 1.38
diff -u -p -u -p -r1.38 acpidev.h
--- acpidev.h   12 Aug 2015 05:59:54 -  1.38
+++ acpidev.h   18 Oct 2016 18:30:24 -
@@ -323,10 +323,12 @@ struct acpiec_softc {
int sc_ecbusy;
 
/* command/status register */
+   bus_size_t  sc_ec_sc;
bus_space_tag_t sc_cmd_bt;
bus_space_handle_t  sc_cmd_bh;
 
/* data register */
+   bus_size_t  sc_ec_data;
bus_space_tag_t sc_data_bt;
bus_space_handle_t  sc_data_bh;
 
Index: acpiec.c
===
RCS file: /cvs/src/sys/dev/acpi/acpiec.c,v
retrieving revision 1.54
diff -u -p -u -p -r1.54 acpiec.c
--- acpiec.c23 Aug 2016 18:26:21 -  1.54
+++ acpiec.c18 Oct 2016 18:30:25 -
@@ -407,13 +407,53 @@ acpiec_getregister(const u_int8_t *buf, 
 }
 
 int
+acpiec_parse_resources(union acpi_resource *crs, void *arg)
+{
+   struct acpiec_softc *sc = arg;
+   int type = AML_CRSTYPE(crs);
+
+   static int argno = 0;
+
+   switch (argno) {
+   case 0:
+   if (type != SR_IOPORT) {
+   printf("%s: Unexpected resource #%d type %d\n",
+   DEVNAME(sc), argno, type);
+   break;
+   }
+   sc->sc_data_bt = sc->sc_acpi->sc_iot;
+   sc->sc_ec_data = crs->sr_ioport._max;
+   break;
+   case 1:
+   if (type != SR_IOPORT) {
+   printf("%s: Unexpected resource #%d type %d\n",
+   DEVNAME(sc), argno, type);
+   break;
+   }
+   sc->sc_cmd_bt = sc->sc_acpi->sc_iot;
+   sc->sc_ec_sc = crs->sr_ioport._max;
+   break;
+   case 2:
+   if (!sc->sc_acpi->sc_hw_reduced) {
+   printf("%s: Not running on HW-Reduced ACPI type %d\n",
+   DEVNAME(sc), type);
+   break;
+   }
+   /* XXX: handle SCI GPIO  */
+   break;
+   default:
+   printf("%s: invalid resource #%d type %d\n",
+   DEVNAME(sc), argno, type);
+   }
+
+   argno++;
+   return 0;
+}
+
+int
 acpiec_getcrs(struct acpiec_softc *sc, struct acpi_attach_args *aa)
 {
struct aml_valueres;
-   bus_size_t  ec_sc, ec_data;
-   int dtype, ctype;
-   char*buf;
-   int size, ret;
int64_t gpe;
struct acpi_ecdt*ecdt = aa->aaa_table;
extern struct aml_node  aml_root;
@@ -423,11 +463,17 @@ acpiec_getcrs(struct acpiec_softc *sc, s
/* Get GPE, Data and Control segments */
sc->sc_gpe = ecdt->gpe_bit;
 
-   ctype = ecdt->ec_control.address_space_id;
-   ec_sc = ecdt->ec_control.address;
-
-   dtype = ecdt->ec_data.address_space_id;
-   ec_data = ecdt->ec_data.address;
+   if (ecdt->ec_control.address_space_id == GAS_SYSTEM_IOSPACE)
+   sc->sc_cmd_bt = sc->sc_acpi->sc_iot;
+   else
+   sc->sc_cmd_bt = sc->sc_acpi->sc_memt;
+   sc->sc_ec_sc = ecdt->ec_control.address;
+
+   if (ecdt->ec_data.address_space_id == GAS_SYSTEM_IOSPACE)
+   sc->sc_data_bt = sc->sc_acpi->sc_iot;
+   else
+   sc->sc_data_bt = sc->sc_acpi->sc_memt;
+   sc->sc_ec_data = ecdt->ec_data.address;
 
/* Get devnode from header */
sc->sc_devnode = aml_searchname(_root, ecdt->ec_id);
@@ -456,60 +502,24 @@ acpiec_getcrs(struct acpiec_softc *sc, s
return (1);
}
 
-   size = res.length;
-   buf = res.v_buffer;
-
-   ret = acpiec_getregister(buf, size, , _data);
-   if (ret <= 0) {
-   dnprintf(10, "%s: failed to read DATA from _CRS\n",
-   DEVNAME(sc));
-   aml_freevalue();
-   return (1);
-   }
-
-   buf += ret;
-   size -= ret;
-
-   ret = acpiec_getregister(buf, size, , _sc);
-   if (ret <= 0) {
-   dnprintf(10, "%s: failed to read S/C from _CRS\n",
-   DEVNAME(sc));
-

Re: acpiec on acer aspire S7 with CURRENT

2016-10-18 Thread Ilya Kaliman 
Hi!

The patch seems to work partially - acpiec now gets initialized. The
suspend-on-lid-close does not work anymore, though (it works with my
initial patch).

acpiprt10 at acpi0: bus -1 (PEG0)
acpiprt11 at acpi0: bus -1 (PEG1)
acpiprt12 at acpi0: bus -1 (PEG2)
acpiec0 at acpi0acpiec0: invalid resource #0 type 8
acpiec0: Not running on HW-Reduced ACPI type 134

acpicpu0 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33), C1(1000@1
 mwait.1), PSS
acpicpu1 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33), C1(1000@1
 mwait.1), PSS
acpicpu2 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33), C1(1000@1
 mwait.1), PSS
acpicpu3 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33), C1(1000@1
 mwait.1), PSS
acpitz0 at acpi0: critical temperature is 99 degC
acpitz1 at acpi0: critical temperature is 98 degC
acpials0 at acpi0: ALSD
acpiac0 at acpi0: AC unit online
acpibat0 at acpi0: BAT0 model "AP13F3N" serial  2358 type LION oem "4f594e4153"
acpibtn0 at acpi0: PWRB
"10250759" at acpi0 not configured
"SYN1B78" at acpi0 not configured
"PNP0C14" at acpi0 not configured
dwiic0 at acpi0: I2C1 addr 0xfe105000/0x1000 irq 7
iic0 at dwiic0
"BCM2E4E" at acpi0 not configured
acpibtn1 at acpi0: LID0
acpibtn2 at acpi0: SLPB
"PNP0C14" at acpi0 not configured
"INT340E" at acpi0 not configured
"INT33A0" at acpi0 not configured

On Tue, Oct 18, 2016 at 3:25 AM, Paul Irofti  wrote:
>> Perhaps acpiec should use aml_parse_resource() instead of the hand-crafted
>> parsing it does right now?  That would solve the second problem, at
>> least.
>
> Like this?
>
> To the OP can you please test this diff on your system and let me know
> if it fixes things?
>
> I will move on to adding support for hardware reduced machines once this
> diff is in and I see an actual ACPI dump that has GPIO AML code.
>
>
> Index: acpidev.h
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpidev.h,v
> retrieving revision 1.38
> diff -u -p -u -p -r1.38 acpidev.h
> --- acpidev.h   12 Aug 2015 05:59:54 -  1.38
> +++ acpidev.h   18 Oct 2016 10:22:41 -
> @@ -323,10 +323,12 @@ struct acpiec_softc {
> int sc_ecbusy;
>
> /* command/status register */
> +   bus_size_t  sc_ec_sc;
> bus_space_tag_t sc_cmd_bt;
> bus_space_handle_t  sc_cmd_bh;
>
> /* data register */
> +   bus_size_t  sc_ec_data;
> bus_space_tag_t sc_data_bt;
> bus_space_handle_t  sc_data_bh;
>
> Index: acpiec.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpiec.c,v
> retrieving revision 1.54
> diff -u -p -u -p -r1.54 acpiec.c
> --- acpiec.c23 Aug 2016 18:26:21 -  1.54
> +++ acpiec.c18 Oct 2016 10:22:41 -
> @@ -407,13 +407,53 @@ acpiec_getregister(const u_int8_t *buf,
>  }
>
>  int
> +acpiec_parse_resources(union acpi_resource *crs, void *arg)
> +{
> +   struct acpiec_softc *sc = arg;
> +   int type = AML_CRSTYPE(crs);
> +
> +   static int argno = 0;
> +
> +   switch (argno) {
> +   case 1:
> +   if (type != SR_IOPORT) {
> +   printf("%s: Unexpected resource #%d type %d\n",
> +   DEVNAME(sc), argno, type);
> +   break;
> +   }
> +   sc->sc_data_bt = sc->sc_acpi->sc_iot;
> +   sc->sc_ec_data = crs->sr_ioport._max;
> +   break;
> +   case 2:
> +   if (type != SR_IOPORT) {
> +   printf("%s: Unexpected resource #%d type %d\n",
> +   DEVNAME(sc), argno, type);
> +   break;
> +   }
> +   sc->sc_cmd_bt = sc->sc_acpi->sc_iot;
> +   sc->sc_ec_sc = crs->sr_ioport._max;
> +   break;
> +   case 3:
> +   if (!sc->sc_acpi->sc_hw_reduced) {
> +   printf("%s: Not running on HW-Reduced ACPI type %d\n",
> +   DEVNAME(sc), type);
> +   break;
> +   }
> +   /* XXX: handle SCI GPIO  */
> +   break;
> +   default:
> +   printf("%s: invalid resource #%d type %d\n",
> +   DEVNAME(sc), argno, type);
> +   }
> +
> +   argno++;
> +   return 0;
> +}
> +
> +int
>  acpiec_getcrs(struct acpiec_softc *sc, struct acpi_attach_args *aa)
>  {
> struct aml_valueres;
> -   bus_size_t  ec_sc, ec_data;
> -   int dtype, ctype;
> -   char*buf;
> -   int size, ret;
> int64_t gpe;
> struct acpi_ecdt*ecdt = aa->aaa_table;
> extern struct aml_node  aml_root;
> @@ -423,11 +463,17 @@ acpiec_getcrs(struct acpiec_softc *sc, s
> /* Get GPE,

Re: acpiec on acer aspire S7 with CURRENT

2016-10-18 Thread Paul Irofti 
> Perhaps acpiec should use aml_parse_resource() instead of the hand-crafted 
> parsing it does right now?  That would solve the second problem, at 
> least.

Like this?

To the OP can you please test this diff on your system and let me know
if it fixes things?

I will move on to adding support for hardware reduced machines once this
diff is in and I see an actual ACPI dump that has GPIO AML code.


Index: acpidev.h
===
RCS file: /cvs/src/sys/dev/acpi/acpidev.h,v
retrieving revision 1.38
diff -u -p -u -p -r1.38 acpidev.h
--- acpidev.h   12 Aug 2015 05:59:54 -  1.38
+++ acpidev.h   18 Oct 2016 10:22:41 -
@@ -323,10 +323,12 @@ struct acpiec_softc {
int sc_ecbusy;
 
/* command/status register */
+   bus_size_t  sc_ec_sc;
bus_space_tag_t sc_cmd_bt;
bus_space_handle_t  sc_cmd_bh;
 
/* data register */
+   bus_size_t  sc_ec_data;
bus_space_tag_t sc_data_bt;
bus_space_handle_t  sc_data_bh;
 
Index: acpiec.c
===
RCS file: /cvs/src/sys/dev/acpi/acpiec.c,v
retrieving revision 1.54
diff -u -p -u -p -r1.54 acpiec.c
--- acpiec.c23 Aug 2016 18:26:21 -  1.54
+++ acpiec.c18 Oct 2016 10:22:41 -
@@ -407,13 +407,53 @@ acpiec_getregister(const u_int8_t *buf, 
 }
 
 int
+acpiec_parse_resources(union acpi_resource *crs, void *arg)
+{
+   struct acpiec_softc *sc = arg;
+   int type = AML_CRSTYPE(crs);
+
+   static int argno = 0;
+
+   switch (argno) {
+   case 1:
+   if (type != SR_IOPORT) {
+   printf("%s: Unexpected resource #%d type %d\n",
+   DEVNAME(sc), argno, type);
+   break;
+   }
+   sc->sc_data_bt = sc->sc_acpi->sc_iot;
+   sc->sc_ec_data = crs->sr_ioport._max;
+   break;
+   case 2:
+   if (type != SR_IOPORT) {
+   printf("%s: Unexpected resource #%d type %d\n",
+   DEVNAME(sc), argno, type);
+   break;
+   }
+   sc->sc_cmd_bt = sc->sc_acpi->sc_iot;
+   sc->sc_ec_sc = crs->sr_ioport._max;
+   break;
+   case 3:
+   if (!sc->sc_acpi->sc_hw_reduced) {
+   printf("%s: Not running on HW-Reduced ACPI type %d\n",
+   DEVNAME(sc), type);
+   break;
+   }
+   /* XXX: handle SCI GPIO  */
+   break;
+   default:
+   printf("%s: invalid resource #%d type %d\n",
+   DEVNAME(sc), argno, type);
+   }
+
+   argno++;
+   return 0;
+}
+
+int
 acpiec_getcrs(struct acpiec_softc *sc, struct acpi_attach_args *aa)
 {
struct aml_valueres;
-   bus_size_t  ec_sc, ec_data;
-   int dtype, ctype;
-   char*buf;
-   int size, ret;
int64_t gpe;
struct acpi_ecdt*ecdt = aa->aaa_table;
extern struct aml_node  aml_root;
@@ -423,11 +463,17 @@ acpiec_getcrs(struct acpiec_softc *sc, s
/* Get GPE, Data and Control segments */
sc->sc_gpe = ecdt->gpe_bit;
 
-   ctype = ecdt->ec_control.address_space_id;
-   ec_sc = ecdt->ec_control.address;
-
-   dtype = ecdt->ec_data.address_space_id;
-   ec_data = ecdt->ec_data.address;
+   if (ecdt->ec_control.address_space_id == GAS_SYSTEM_IOSPACE)
+   sc->sc_cmd_bt = sc->sc_acpi->sc_iot;
+   else
+   sc->sc_cmd_bt = sc->sc_acpi->sc_memt;
+   sc->sc_ec_sc = ecdt->ec_control.address;
+
+   if (ecdt->ec_data.address_space_id == GAS_SYSTEM_IOSPACE)
+   sc->sc_data_bt = sc->sc_acpi->sc_iot;
+   else
+   sc->sc_data_bt = sc->sc_acpi->sc_memt;
+   sc->sc_ec_data = ecdt->ec_data.address;
 
/* Get devnode from header */
sc->sc_devnode = aml_searchname(_root, ecdt->ec_id);
@@ -456,60 +502,24 @@ acpiec_getcrs(struct acpiec_softc *sc, s
return (1);
}
 
-   size = res.length;
-   buf = res.v_buffer;
-
-   ret = acpiec_getregister(buf, size, , _data);
-   if (ret <= 0) {
-   dnprintf(10, "%s: failed to read DATA from _CRS\n",
-   DEVNAME(sc));
-   aml_freevalue();
-   return (1);
-   }
-
-   buf += ret;
-   size -= ret;
-
-   ret = acpiec_getregister(buf, size, , _sc);
-   if (ret <= 0) {
-   dnprintf(10, "%s: failed to read S/C from _CRS\n",
-   DEVNAME(sc));
-

Re: acpiec on acer aspire S7 with CURRENT

2016-10-14 Thread Philip Guenther 
On Fri, 14 Oct 2016, Ilya Kaliman wrote:
> dmesg:
> OpenBSD 6.0 (GENERIC.MP) #1: Thu Sep  8 16:32:34 PDT 2016
> i...@puffy.my.domain:/usr/src/sys/arch/amd64/compile/GENERIC.MP
..
> acpiec0 at acpi0: Failed to read resource settings
...
> acpidump:

I've taken a brief look at this.  The definition of the resources for the 
embedded controller is odd:

Scope (_SB.PCI0.LPCB)
{
Device (EC0)
{
Name (_HID, EisaId ("PNP0C09") /* Embedded Controller Device */)  
// _HID: Hardware ID
...
Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
{
IO (Decode16,
0x0062, // Range Minimum
0x0062, // Range Maximum
0x00,   // Alignment
0x01,   // Length
)
IO (Decode16,
0x0066, // Range Minimum
0x0066, // Range Maximum
0x00,   // Alignment
0x01,   // Length
)
IO (Decode16,
0x0068, // Range Minimum
0x0068, // Range Maximum
0x01,   // Alignment
0x08,   // Length
)
Memory32Fixed (ReadWrite,
0xFE800E00, // Address Base
0x0001, // Address Length
)
})


Note that there are *four* resources...but even revision 6.1 of the ACPI 
standard only gives meaning for the first *three*, and the third resource 
is the GPIO for the SCI interrupt...but is only for hardware-reduced ACPI 
platforms, which this machine is not:
...
 Use APIC Physical Destination Mode (V4) : 0
   Hardware Reduced (V5) : 0
  Low Power S0 Idle (V5) : 0
...

So I guess there's two defects in acpicec.c right now:
 - we don't use a third resource for the SCI interrupt when hw-reduced
 - we barf on extra resources despite vendors deciding to throwing them 
   in for the hell of it


Perhaps acpiec should use aml_parse_resource() instead of the hand-crafted 
parsing it does right now?  That would solve the second problem, at 
least.

Until we have a better fix, simply deleting the end-of-buffer check as 
you've done should have the same end effect, but expect a diff to try out 
at some point here...


Philip Guenther

Re: acpiec on acer aspire S7 with CURRENT

2016-10-14 Thread Ilya Kaliman 
dmesg:
OpenBSD 6.0 (GENERIC.MP) #1: Thu Sep  8 16:32:34 PDT 2016
i...@puffy.my.domain:/usr/src/sys/arch/amd64/compile/GENERIC.MP
RTC BIOS diagnostic error 80
real mem = 8468033536 (8075MB)
avail mem = 8206921728 (7826MB)
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 2.7 @ 0xe6a80 (27 entries)
bios0: vendor Insyde Corp. version "V2.12" date 05/20/2014
bios0: Acer Aspire S7-392
acpi0 at bios0: rev 2
acpi0: sleep states S0 S3 S4 S5
acpi0: tables DSDT FACP TCPA UEFI FPDT MSDM ASF! HPET APIC MCFG SSDT
BOOT ASPT DBGP SSDT SSDT SSDT SSDT SSDT DMAR
acpi0: wakeup devices P0P1(S4) GLAN(S4) EHC1(S3) EHC2(S3) XHC_(S3)
HDEF(S4) TPD4(S4) PXSX(S4) RP01(S4) PXSX(S4) RP02(S4) PXSX(S4)
RP03(S4) PXSX(S4) RP04(S4) PXSX(S4) [...]
acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpihpet0 at acpi0: 14318179 Hz
acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: Intel(R) Core(TM) i5-4210U CPU @ 1.70GHz, 1596.68 MHz
cpu0: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SENSOR,ARAT
cpu0: 256KB 64b/line 8-way L2 cache
cpu0: smt 0, core 0, package 0
mtrr: Pentium Pro MTRR support, 10 var ranges, 88 fixed ranges
cpu0: apic clock running at 99MHz
cpu0: mwait min=64, max=64, C-substates=0.2.1.2.4.1.1.1, IBE
cpu1 at mainbus0: apid 1 (application processor)
cpu1: Intel(R) Core(TM) i5-4210U CPU @ 1.70GHz, 1596.31 MHz
cpu1: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SENSOR,ARAT
cpu1: 256KB 64b/line 8-way L2 cache
cpu1: smt 1, core 0, package 0
cpu2 at mainbus0: apid 2 (application processor)
cpu2: Intel(R) Core(TM) i5-4210U CPU @ 1.70GHz, 1596.31 MHz
cpu2: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SENSOR,ARAT
cpu2: 256KB 64b/line 8-way L2 cache
cpu2: smt 0, core 1, package 0
cpu3 at mainbus0: apid 3 (application processor)
cpu3: Intel(R) Core(TM) i5-4210U CPU @ 1.70GHz, 1596.31 MHz
cpu3: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SENSOR,ARAT
cpu3: 256KB 64b/line 8-way L2 cache
cpu3: smt 1, core 1, package 0
ioapic0 at mainbus0: apid 2 pa 0xfec0, version 20, 40 pins
acpimcfg0 at acpi0 addr 0xe000, bus 0-255
acpiprt0 at acpi0: bus 0 (PCI0)
acpiprt1 at acpi0: bus -1 (P0P1)
acpiprt2 at acpi0: bus -1 (RP01)
acpiprt3 at acpi0: bus -1 (RP02)
acpiprt4 at acpi0: bus 1 (RP03)
acpiprt5 at acpi0: bus -1 (RP04)
acpiprt6 at acpi0: bus -1 (RP05)
acpiprt7 at acpi0: bus -1 (RP06)
acpiprt8 at acpi0: bus -1 (RP07)
acpiprt9 at acpi0: bus -1 (RP08)
acpiprt10 at acpi0: bus -1 (PEG0)
acpiprt11 at acpi0: bus -1 (PEG1)
acpiprt12 at acpi0: bus -1 (PEG2)
acpiec0 at acpi0: Failed to read resource settings
acpicpu0 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33),
C1(1000@1 mwait.1), PSS
acpicpu1 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33),
C1(1000@1 mwait.1), PSS
acpicpu2 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33),
C1(1000@1 mwait.1), PSS
acpicpu3 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33),
C1(1000@1 mwait.1), PSS
acpitz0 at acpi0: critical temperature is 99 degC
acpitz1 at acpi0: critical temperature is 98 degC
"ACPI0008" at acpi0 not configured
acpiac0 at acpi0: AC unit online
acpibat0 at acpi0: BAT0 model "AP13F3N" serial  2358 type LION oem "4f594e4153"
acpibtn0 at acpi0: PWRB
"10250759" at acpi0 not configured
"SYN1B78" at acpi0 not configured
"PNP0C14" at acpi0 not configured
dwiic0 at acpi0: I2C1 addr 0xfe105000/0x1000 irq 7
iic0 at dwiic0
"BCM2E4E" at acpi0 not configured
acpibtn1 at acpi0: LID0
acpi0: WARNING EC not initialized
acpi0: WARNING EC not initialized
acpibtn2 at acpi0: SLPB
"PNP0C14" at acpi0 not configured
"INT340E" at acpi0 not configured
"INT33A0" at acpi0 not configured
"PNP0C31" at acpi0 not configured
acpivideo0 at acpi0: GFX0
acpivout0 at acpivideo0: DD1F
cpu0: Enhanced SpeedStep 1596 MHz: speeds: 2401, 2400, 2300, 2200,
2000, 1900, 1700,

Re: acpiec on acer aspire S7 with CURRENT

2016-10-14 Thread Paul Irofti 
> Which acpi table should I attach?

Use sendbug(1). See also

http://www.openbsd.org/report.html#bugtypes

Re: acpiec on acer aspire S7 with CURRENT

2016-10-13 Thread Ilya Kaliman 
Hi!

Thanks for looking into this. I've tried the patch and the "found GPIO
port" printf is not triggered because sc->sc_acpi->sc_hw_reduced is 0
for me.

Which acpi table should I attach?

Thanks,
Ilya


RSD PTR: Checksum=188, OEMID=ACRSYS, RsdtAddress=0x9aafe124


RSDT: Length=112, Revision=1, Checksum=171,
OEMID=ACRSYS, OEM Table ID=ACRPRDCT, OEM Revision=0x1,
Creator ID=, Creator Revision=0x113


Entries={ 0x9aaf7000, 0x9aafd000, 0x9aafc000, 0x9aafa000,
0x9aaf9000, 0x9aaf8000, 0x9aaf6000, 0x9aaf5000, 0x9aaf4000,
0x9aae2000, 0x9aae, 0x9aade000, 0x9aadd000, 0x9aada000,
0x9aad9000, 0x9aad5000, 0x9aad4000, 0x9aad3000, 0x9aae3000 }


DSDT=0x9aae4000
INT_MODEL=APIC
SCI_INT=9
SMI_CMD=0xb2, ACPI_ENABLE=0xa0, ACPI_DISABLE=0xa1, S4BIOS_REQ=0x0
PM1a_EVT_BLK=0x1800-0x1803
PM1a_CNT_BLK=0x1804-0x1805
PM2_CNT_BLK=0x1850-0x1850
PM2_TMR_BLK=0x1808-0x180b
PM2_GPE0_BLK=0x1880-0x189f
P_LVL2_LAT=101ms, P_LVL3_LAT=57ms
FLUSH_SIZE=1024, FLUSH_STRIDE=16
DUTY_OFFSET=1, DUTY_WIDTH=3
DAY_ALRM=13, MON_ALRM=0, CENTURY=0
Flags={WBINVD,PROC_C1,SLP_BUTTON,RTC_S4}


DSDT: Length=61736, Revision=1, Checksum=95,
OEMID=ACRSYS, OEM Table ID=ACRPRDCT, OEM Revision=0x0,
Creator ID=1025, Creator Revision=0x4


TCPA: Length=50, Revision=2, Checksum=139,
OEMID=ACRSYS, OEM Table ID=ACRPRDCT, OEM Revision=0x0,
Creator ID=1025, Creator Revision=0x4


UEFI: Length=566, Revision=1, Checksum=189,
OEMID=ACRSYS, OEM Table ID=ACRPRDCT, OEM Revision=0x1,
Creator ID=1025, Creator Revision=0x4


FPDT: Length=68, Revision=1, Checksum=181,
OEMID=ACRSYS, OEM Table ID=ACRPRDCT, OEM Revision=0x1,
Creator ID=1025, Creator Revision=0x4


MSDM: Length=85, Revision=3, Checksum=230,
OEMID=ACRSYS, OEM Table ID=ACRPRDCT, OEM Revision=0x1,
Creator ID=1025, Creator Revision=0x4


ASF!: Length=165, Revision=32, Checksum=231,
OEMID=ACRSYS, OEM Table ID=ACRPRDCT, OEM Revision=0x1,
Creator ID=1025, Creator Revision=0x4


HPET: Length=56, Revision=1, Checksum=170,
OEMID=ACRSYS, OEM Table ID=ACRPRDCT, OEM Revision=0x1,
Creator ID=1025, Creator Revision=0x4


APIC: Length=140, Revision=3, Checksum=12,
OEMID=ACRSYS, OEM Table ID=ACRPRDCT, OEM Revision=0x1,
Creator ID=1025, Creator Revision=0x4


MCFG: Length=60, Revision=1, Checksum=210,
OEMID=ACRSYS, OEM Table ID=ACRPRDCT, OEM Revision=0x1,
Creator ID=1025, Creator Revision=0x4


SSDT: Length=2101, Revision=1, Checksum=93,
OEMID=ACRSYS, OEM Table ID=ACRPRDCT, OEM Revision=0x1000,
Creator ID=1025, Creator Revision=0x4


BOOT: Length=40, Revision=1, Checksum=106,
OEMID=ACRSYS, OEM Table ID=ACRPRDCT, OEM Revision=0x1,
Creator ID=1025, Creator Revision=0x4


ASPT: Length=52, Revision=7, Checksum=152,
OEMID=ACRSYS, OEM Table ID=ACRPRDCT, OEM Revision=0x1,
Creator ID=1025, Creator Revision=0x4


DBGP: Length=52, Revision=1, Checksum=172,
OEMID=ACRSYS, OEM Table ID=ACRPRDCT, OEM Revision=0x1,
Creator ID=1025, Creator Revision=0x4


SSDT: Length=1337, Revision=1, Checksum=105,
OEMID=ACRSYS, OEM Table ID=ACRPRDCT, OEM Revision=0x3000,
Creator ID=1025, Creator Revision=0x4


SSDT: Length=2776, Revision=1, Checksum=140,
OEMID=ACRSYS, OEM Table ID=ACRPRDCT, OEM Revision=0x3000,
Creator ID=1025, Creator Revision=0x4


SSDT: Length=12958, Revision=1, Checksum=91,
OEMID=ACRSYS, OEM Table ID=ACRPRDCT, OEM Revision=0x3000,
Creator ID=1025, Creator Revision=0x4


SSDT: Length=824, Revision=1, Checksum=146,
OEMID=ACRSYS, OEM Table ID=ACRPRDCT, OEM Revision=0x1000,
Creator ID=1025, Creator Revision=0x4


SSDT: Length=1593, Revision=1, Checksum=58,
OEMID=ACRSYS, OEM Table ID=ACRPRDCT, OEM Revision=0x1000,
Creator ID=1025, Creator Revision=0x4


DMAR: Length=136, Revision=1, Checksum=227,
OEMID=ACRSYS, OEM Table ID=ACRPRDCT, OEM Revision=0x1,
Creator ID=1025, Creator Revision=0x4






On Thu, Oct 13, 2016 at 1:19 AM, Paul Irofti  wrote:
>> The diff you sent simply ignores the issue which obviously can not be
>> used in the tree.
>
> The following diff starts working in the right direction, but there is
> probably more to do in order to support machines like yours.
>
> It would be interesting to see if that printf gets triggered though, so
> please test and let me know. Also, send that acpidump.
>
>
> Index: acpiec.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpiec.c,v
> retrieving revision 1.54
> diff -u -p -u -p -r1.54 acpiec.c
> --- acpiec.c23 Aug 2016 18:26:21 -  1.54
> +++ acpiec.c13 Oct 2016 08:16:52 -
> @@ -410,8 +410,8 @@ int
>

Re: acpiec on acer aspire S7 with CURRENT

2016-10-13 Thread Paul Irofti 
> The diff you sent simply ignores the issue which obviously can not be
> used in the tree.

The following diff starts working in the right direction, but there is
probably more to do in order to support machines like yours.

It would be interesting to see if that printf gets triggered though, so
please test and let me know. Also, send that acpidump.


Index: acpiec.c
===
RCS file: /cvs/src/sys/dev/acpi/acpiec.c,v
retrieving revision 1.54
diff -u -p -u -p -r1.54 acpiec.c
--- acpiec.c23 Aug 2016 18:26:21 -  1.54
+++ acpiec.c13 Oct 2016 08:16:52 -
@@ -410,8 +410,8 @@ int
 acpiec_getcrs(struct acpiec_softc *sc, struct acpi_attach_args *aa)
 {
struct aml_valueres;
-   bus_size_t  ec_sc, ec_data;
-   int dtype, ctype;
+   bus_size_t  ec_sc, ec_data, ec_gpio;
+   int dtype, ctype, iotype;
char*buf;
int size, ret;
int64_t gpe;
@@ -480,6 +480,20 @@ acpiec_getcrs(struct acpiec_softc *sc, s
 
buf += ret;
size -= ret;
+
+   if (sc->sc_acpi->sc_hw_reduced) {
+   ret = acpiec_getregister(buf, size, , _gpio);
+   if (ret <= 0) {
+   dnprintf(10, "%s: failed to read GPIO from _CRS\n",
+   DEVNAME(sc));
+   aml_freevalue();
+   return (1);
+   }
+   printf("%s: found GPIO port\n", DEVNAME(sc));
+
+   buf += ret;
+   size -= ret;
+   }
 
if (size != 2 || *buf != RES_TYPE_ENDTAG) {
dnprintf(10, "%s: no _CRS end tag\n", DEVNAME(sc));

Re: acpiec on acer aspire S7 with CURRENT

2016-10-13 Thread Paul Irofti 
On Mon, Oct 10, 2016 at 03:04:33PM -0700, Ilya Kaliman wrote:
> Hi tech,
> 
> while trying the latest snapshot I've noticed that the following
> warning is printed to a console several times a second (this does not
> happen in 6.0):
> 
> acpi0: WARNING EC not initialized
> 
> The investigation shows that the acpiec initialization fails in
> acpiec.c line 484:
> 
> if (size != 2 || *buf != RES_TYPE_ENDTAG)
> 
> On my system at this point the size value is 22 and the *buf is 0x47
> (RES_TYPE_IOPORT).
> 
> I am not sure what the proper fix is, but removing the whole _CRS
> ENDTAG check solves the problem (see attached patch).
> 
> Thanks,
> Ilya

Hi,

I think you are running on a hardware reduced ACPI machine, but I can
not tell for sure without a proper bug report (i.e. at least acpidump).

The diff you sent simply ignores the issue which obviously can not be
used in the tree.

Paul

> 
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpiec.c,v
> retrieving revision 1.54
> diff -u -p -r1.54 acpiec.c
> --- acpiec.c23 Aug 2016 18:26:21 -  1.54
> +++ acpiec.c10 Oct 2016 21:55:59 -
> @@ -477,15 +477,6 @@ acpiec_getcrs(struct acpiec_softc *sc, s
> aml_freevalue();
> return (1);
> }
> -
> -   buf += ret;
> -   size -= ret;
> -
> -   if (size != 2 || *buf != RES_TYPE_ENDTAG) {
> -   dnprintf(10, "%s: no _CRS end tag\n", DEVNAME(sc));
> -   aml_freevalue();
> -   return (1);
> -   }
> aml_freevalue();