Change in coreboot[master]: device/azalia_device.c: Add codec reset helpers

[Angel Pons (Code Review)]

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48355 )

Change subject: device/azalia_device.c: Add codec reset helpers
......................................................................

device/azalia_device.c: Add codec reset helpers

Many uses of `azalia_set_bits` are used to toggle the reset bit. They
will be put to use in subsequent commits, one change per function.

Change-Id: If0594fdaf99319f08a2e272cd37958f0f216e654
Signed-off-by: Angel Pons 
---
M src/device/azalia_device.c
M src/include/device/azalia_device.h
2 files changed, 14 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/48355/1

diff --git a/src/device/azalia_device.c b/src/device/azalia_device.c
index b18d938..a457cf1 100644
--- a/src/device/azalia_device.c
+++ b/src/device/azalia_device.c
@@ -34,6 +34,18 @@
    return 0;
 }

+int azalia_enter_reset(void *port)
+{
+	/* Set bit 0 to 0 to enter reset state (BAR + 0x8)[0] */
+	azalia_set_bits(base + HDA_GCTL_REG, HDA_GCTL_CRST, 0);
+}
+
+int azalia_exit_reset(u8 *base)
+{
+	/* Set bit 0 to 1 to exit reset state (BAR + 0x8)[0] */
+	azalia_set_bits(base + HDA_GCTL_REG, HDA_GCTL_CRST, HDA_GCTL_CRST);
+}
+
 static int codec_detect(u8 *base)
 {
    u32 reg32;
diff --git a/src/include/device/azalia_device.h b/src/include/device/azalia_device.h
index 1945725..1b4e769 100644
--- a/src/include/device/azalia_device.h
+++ b/src/include/device/azalia_device.h
@@ -19,6 +19,8 @@
 #define   HDA_ICII_VALID	(1 << 1)

int azalia_set_bits(void *port, u32 mask, u32 val);
+int azalia_enter_reset(u8 *base);
+int azalia_exit_reset(u8 *base);
 u32 azalia_find_verb(const u32 *verb_table, u32 verb_table_bytes, u32 viddid, const u32 **verb);
 void azalia_audio_init(struct device *dev);
 extern struct device_operations default_azalia_audio_ops;

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48355 )

Change subject: device/azalia_device.c: Add codec reset helpers
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48355/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/48355/3//COMMIT_MSG@9 
PS3, Line 9: Many uses of `azalia_set_bits` are used to toggle the reset bit. They
           : will be put to use in subsequent commits, one change per function.
           : 
maybe simply describe what you do here?

"Add common codec reset helpers to be able to reduce code duplication in the subsequent changes."?

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48355 )

Change subject: device/azalia_device.c: Add codec reset helpers
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48355/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/48355/3//COMMIT_MSG@9 
PS3, Line 9: Many uses of `azalia_set_bits` are used to toggle the reset bit. They
           : will be put to use in subsequent commits, one change per function.
           :
> maybe simply describe what you do here? […]

Sorry, I don't understand what exactly is wrong with this commit message (the summary is also part of the commit message, btw). I use the summary to state what this change does, and then explain why I'm doing it as well as what I want this for. I'm not going to repeat the summary in the commit message, because it is redundant.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48355 )

Change subject: device/azalia_device.c: Add codec reset helpers
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48355/3/src/device/azalia_device.c 
File src/device/azalia_device.c:

https://review.coreboot.org/c/coreboot/+/48355/3/src/device/azalia_device.c@... 
PS3, Line 37: void *port
Needs an update

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48355 )

Change subject: device/azalia_device.c: Add codec reset helpers
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48355/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/48355/3//COMMIT_MSG@9 
PS3, Line 9: Many uses of `azalia_set_bits` are used to toggle the reset bit. They
           : will be put to use in subsequent commits, one change per function.
           :
> Sorry, I don't understand what exactly is wrong with this commit message

The message is confusing (at least to me) and reading it first I actually had no idea what the change is for.
> I'm not going to repeat the summary in the commit message, because it is redundant.

A "summary" is redundant? You don't say...

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48355 )

Change subject: device/azalia_device.c: Add codec reset helpers
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48355/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/48355/3//COMMIT_MSG@9 
PS3, Line 9: Many uses of `azalia_set_bits` are used to toggle the reset bit. They
           : will be put to use in subsequent commits, one change per function.
           :
> > Sorry, I don't understand what exactly is wrong with this commit message […]

I'm not sure how to parse it either. "They" in the last sentence seems
to be referring to the helpers. But when reading the sentence, one automa-
tically assumes it refers to the subject of the first sentence of the
paragraph, and fails. It seems one has to shuffle all three sentences
around until it makes sense.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48355 )

Change subject: device/azalia_device.c: Add codec reset helpers
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48355/3/src/device/azalia_device.c 
File src/device/azalia_device.c:

https://review.coreboot.org/c/coreboot/+/48355/3/src/device/azalia_device.c@... 
PS3, Line 37: void *port
> Needs an update

Done

[Angel Pons (Code Review)]

Hello  Felix Singer, build bot (Jenkins), Nico Huber, Arthur Heymans, Michael Niewöhner, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/48355

to look at the new patch set (#4).

Change subject: device/azalia_device.c: Add codec reset helpers
......................................................................

device/azalia_device.c: Add codec reset helpers

Many uses of `azalia_set_bits` are used to toggle the reset bit. To
avoid having to repeat the register operations and the corresponding
comment, create two helpers with self-explanatory names. They will be
put to use in subsequent commits, with one change for each function.

Change-Id: If0594fdaf99319f08a2e272cd37958f0f216e654
Signed-off-by: Angel Pons 
---
M src/device/azalia_device.c
M src/include/device/azalia_device.h
2 files changed, 14 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/48355/4

[Angel Pons (Code Review)]

Hello  Felix Singer, build bot (Jenkins), Nico Huber, Arthur Heymans, Michael Niewöhner, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/48355

to look at the new patch set (#5).

Change subject: device/azalia_device.c: Add codec reset helpers
......................................................................

device/azalia_device.c: Add codec reset helpers

Many uses of `azalia_set_bits` are used to toggle the reset bit. To
avoid having to repeat the register operations and the corresponding
comment, create two helpers with self-explanatory names. They will be
put to use in subsequent commits, with one change for each function.

Change-Id: If0594fdaf99319f08a2e272cd37958f0f216e654
Signed-off-by: Angel Pons 
---
M src/device/azalia_device.c
M src/include/device/azalia_device.h
2 files changed, 14 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/48355/5

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48355 )

Change subject: device/azalia_device.c: Add codec reset helpers
......................................................................

Patch Set 5: Code-Review+2

[Hung-Te Lin (Code Review)]

Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48355 )

Change subject: device/azalia_device.c: Add codec reset helpers
......................................................................

device/azalia_device.c: Add codec reset helpers

Many uses of `azalia_set_bits` are used to toggle the reset bit. To
avoid having to repeat the register operations and the corresponding
comment, create two helpers with self-explanatory names. They will be
put to use in subsequent commits, with one change for each function.

Change-Id: If0594fdaf99319f08a2e272cd37958f0f216e654
Signed-off-by: Angel Pons 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/48355
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Rudolph 
---
M src/device/azalia_device.c
M src/include/device/azalia_device.h
2 files changed, 14 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Rudolph: Looks good to me, approved

diff --git a/src/device/azalia_device.c b/src/device/azalia_device.c
index ab370f9..9306e2a 100644
--- a/src/device/azalia_device.c
+++ b/src/device/azalia_device.c
@@ -34,6 +34,18 @@
    return 0;
 }

+int azalia_enter_reset(u8 *base)
+{
+	/* Set bit 0 to 0 to enter reset state (BAR + 0x8)[0] */
+	return azalia_set_bits(base + HDA_GCTL_REG, HDA_GCTL_CRST, 0);
+}
+
+int azalia_exit_reset(u8 *base)
+{
+	/* Set bit 0 to 1 to exit reset state (BAR + 0x8)[0] */
+	return azalia_set_bits(base + HDA_GCTL_REG, HDA_GCTL_CRST, HDA_GCTL_CRST);
+}
+
 static int codec_detect(u8 *base)
 {
    u32 reg32;
diff --git a/src/include/device/azalia_device.h b/src/include/device/azalia_device.h
index 1945725..1b4e769 100644
--- a/src/include/device/azalia_device.h
+++ b/src/include/device/azalia_device.h
@@ -19,6 +19,8 @@
 #define   HDA_ICII_VALID	(1 << 1)

int azalia_set_bits(void *port, u32 mask, u32 val);
+int azalia_enter_reset(u8 *base);
+int azalia_exit_reset(u8 *base);
 u32 azalia_find_verb(const u32 *verb_table, u32 verb_table_bytes, u32 viddid, const u32 **verb);
 void azalia_audio_init(struct device *dev);
 extern struct device_operations default_azalia_audio_ops;