mturney mturney has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
sc7180: GPIO: Add I2S configuration for sc7180
Configuring GPIO Pins as I2S mode for Audio speaker.
Change-Id: I681aa6d0d57671b0fd9b7bc88de6f2cc202a7af0 Signed-off-by: vsujithk vsujithk@codeaurora.org --- M src/soc/qualcomm/sc7180/Makefile.inc M src/soc/qualcomm/sc7180/bootblock.c A src/soc/qualcomm/sc7180/include/soc/qi2s.h A src/soc/qualcomm/sc7180/qi2s.c 4 files changed, 80 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/38593/1
diff --git a/src/soc/qualcomm/sc7180/Makefile.inc b/src/soc/qualcomm/sc7180/Makefile.inc index ba1f675..6d6222b 100644 --- a/src/soc/qualcomm/sc7180/Makefile.inc +++ b/src/soc/qualcomm/sc7180/Makefile.inc @@ -15,6 +15,7 @@ bootblock-y += qupv3_fw_config.c bootblock-y += qupv3_config.c bootblock-y += qcom_qup_se.c +bootblock-y += qi2s.c
################################################################################ verstage-y += timer.c @@ -27,7 +28,7 @@ verstage-y += qcom_qup_se.c verstage-y += qupv3_config.c verstage-$(CONFIG_DRIVERS_UART) += qupv3_uart.c - +verstage-y += qi2s.c ################################################################################ romstage-y += cbmem.c romstage-y += timer.c @@ -45,7 +46,7 @@ romstage-y += qcom_qup_se.c romstage-y += qupv3_config.c romstage-$(CONFIG_DRIVERS_UART) += qupv3_uart.c - +romstage-y += qi2s.c ################################################################################ ramstage-y += soc.c ramstage-y += timer.c @@ -61,7 +62,7 @@ ramstage-y += qupv3_config.c ramstage-y += qcom_qup_se.c ramstage-$(CONFIG_DRIVERS_UART) += qupv3_uart.c - +romstage-y += qi2s.c ################################################################################
CPPFLAGS_common += -Isrc/soc/qualcomm/sc7180/include diff --git a/src/soc/qualcomm/sc7180/bootblock.c b/src/soc/qualcomm/sc7180/bootblock.c index b247d2d..83b22ba 100644 --- a/src/soc/qualcomm/sc7180/bootblock.c +++ b/src/soc/qualcomm/sc7180/bootblock.c @@ -18,6 +18,7 @@ #include <soc/mmu.h> #include <soc/qspi.h> #include <soc/qupv3_fw_config.h> +#include <soc/qi2s.h>
void bootblock_soc_init(void) { @@ -25,4 +26,5 @@ clock_init(); quadspi_init(25 * MHz); qupv3_fw_init(); + qi2s_init(); } diff --git a/src/soc/qualcomm/sc7180/include/soc/qi2s.h b/src/soc/qualcomm/sc7180/include/soc/qi2s.h new file mode 100644 index 0000000..ee02094a --- /dev/null +++ b/src/soc/qualcomm/sc7180/include/soc/qi2s.h @@ -0,0 +1,22 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019-2020 Qualcomm Technologies. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef __SOC_QUALCOMM_SC7180_QI2S_H__ +#define __SOC_QUALCOMM_SC7180_QI2S_H__ + +void qi2s_init(void); + + +#endif /* __SOC_QUALCOMM_SC7180_QI2S_H__ */ diff --git a/src/soc/qualcomm/sc7180/qi2s.c b/src/soc/qualcomm/sc7180/qi2s.c new file mode 100644 index 0000000..9661e55 --- /dev/null +++ b/src/soc/qualcomm/sc7180/qi2s.c @@ -0,0 +1,52 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019-2020, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <arch/cache.h> +#include <device/mmio.h> +#include <soc/addressmap.h> +#include <soc/gpio.h> +#include <soc/clock.h> +#include <symbols.h> +#include <assert.h> +#include <gpio.h> +#include <string.h> +#include <soc/qi2s.h> + + +static void configure_gpios(void) +{ + + gpio_output(GPIO(49), 1); + gpio_output(GPIO(50), 1); + gpio_output(GPIO(51), 1); + gpio_output(GPIO(23), 1); + + gpio_configure(GPIO(23), 0, GPIO_PULL_UP, + GPIO_8MA, GPIO_OUTPUT); + gpio_configure(GPIO(49), 1, GPIO_PULL_UP, + GPIO_8MA, GPIO_OUTPUT); + gpio_configure(GPIO(50), 1, GPIO_PULL_UP, + GPIO_8MA, GPIO_OUTPUT); + gpio_configure(GPIO(51), 1, GPIO_PULL_UP, + GPIO_8MA, GPIO_OUTPUT); + +} + + +void qi2s_init() +{ + configure_gpios(); +} +
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... File src/soc/qualcomm/sc7180/qi2s.c:
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... PS1, Line 37: GPIO_8MA, GPIO_OUTPUT); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... PS1, Line 41: GPIO_8MA, GPIO_OUTPUT); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... PS1, Line 43: GPIO_8MA, GPIO_OUTPUT); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... PS1, Line 48: void qi2s_init() Bad function definition - void qi2s_init() should probably be void qi2s_init(void)
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... File src/soc/qualcomm/sc7180/qi2s.c:
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... PS1, Line 28: static void configure_gpios(void) This is mainboard-specific and belongs in ramstage. Please put it in src/mainboard/google/trogdor/mainboard.c.
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... PS1, Line 31: gpio_output(GPIO(49), 1); Do you need to do these when you're doing a gpio_configure() to a special function right afterwards anyway?
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... PS1, Line 36: gpio_configure(GPIO(23), 0, GPIO_PULL_UP, Looks like this one is just a normal enable GPIO, not a real I2S pin? then why the extra gpio_configure() with the pull and the drive strength? Shouldn't gpio_output() be enough for this one?
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... PS1, Line 38: 1 Please use the actual special function macros for the respective pin (e.g. GPIO49_FUNC_xxx) for these. Looks like function 1 for these three pins is just listed as "reserved", but that clearly seems inaccurate, so please rename those to I2S1_BCLK/I2S1_LRCLK/I2S1_DIN or something like that (in sc7180/include/soc/gpio.h).
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38593/2/src/soc/qualcomm/sc7180/qi2... File src/soc/qualcomm/sc7180/qi2s.c:
https://review.coreboot.org/c/coreboot/+/38593/2/src/soc/qualcomm/sc7180/qi2... PS2, Line 37: GPIO_8MA, GPIO_OUTPUT); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38593/2/src/soc/qualcomm/sc7180/qi2... PS2, Line 41: GPIO_8MA, GPIO_OUTPUT); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38593/2/src/soc/qualcomm/sc7180/qi2... PS2, Line 43: GPIO_8MA, GPIO_OUTPUT); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38593/2/src/soc/qualcomm/sc7180/qi2... PS2, Line 48: void qi2s_init() Bad function definition - void qi2s_init() should probably be void qi2s_init(void)
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38593/3/src/soc/qualcomm/sc7180/qi2... File src/soc/qualcomm/sc7180/qi2s.c:
https://review.coreboot.org/c/coreboot/+/38593/3/src/soc/qualcomm/sc7180/qi2... PS3, Line 37: GPIO_8MA, GPIO_OUTPUT); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38593/3/src/soc/qualcomm/sc7180/qi2... PS3, Line 41: GPIO_8MA, GPIO_OUTPUT); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38593/3/src/soc/qualcomm/sc7180/qi2... PS3, Line 43: GPIO_8MA, GPIO_OUTPUT); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38593/3/src/soc/qualcomm/sc7180/qi2... PS3, Line 48: void qi2s_init() Bad function definition - void qi2s_init() should probably be void qi2s_init(void)
Hello Ravi kumar, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38593
to look at the new patch set (#4).
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
sc7180: GPIO: Add I2S configuration for sc7180
Configuring GPIO Pins as I2S mode for Audio speaker.
Change-Id: I681aa6d0d57671b0fd9b7bc88de6f2cc202a7af0 Signed-off-by: V Sujith Kumar Reddy vsujithk@codeaurora.org --- M src/soc/qualcomm/sc7180/Makefile.inc M src/soc/qualcomm/sc7180/bootblock.c A src/soc/qualcomm/sc7180/include/soc/qi2s.h A src/soc/qualcomm/sc7180/qi2s.c 4 files changed, 79 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/38593/4
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38593/6/src/soc/qualcomm/sc7180/qi2... File src/soc/qualcomm/sc7180/qi2s.c:
https://review.coreboot.org/c/coreboot/+/38593/6/src/soc/qualcomm/sc7180/qi2... PS6, Line 16: #include <arch/cache.h> : #include <device/mmio.h> : #include <soc/addressmap.h> : #include <soc/gpio.h> : #include <soc/clock.h> : #include <symbols.h> : #include <assert.h> : #include <gpio.h> : #include <string.h> : #include <soc/qi2s.h> Looks like you don't use all of those includes. Please check, and include only what you use.
Ravi kumar has uploaded a new patch set (#8) to the change originally created by mturney mturney. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
sc7180: GPIO: Add I2S configuration for sc7180
Configuring GPIO Pins as I2S mode for Audio speaker.
Change-Id: I681aa6d0d57671b0fd9b7bc88de6f2cc202a7af0 Signed-off-by: V Sujith Kumar Reddy vsujithk@codeaurora.org --- M src/soc/qualcomm/sc7180/Makefile.inc M src/soc/qualcomm/sc7180/bootblock.c A src/soc/qualcomm/sc7180/include/soc/qi2s.h A src/soc/qualcomm/sc7180/qi2s.c 4 files changed, 78 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/38593/8
Hello Ravi kumar, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38593
to look at the new patch set (#10).
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
sc7180: GPIO: Add I2S configuration for sc7180
Configuring GPIO Pins as I2S mode for Audio speaker.
Change-Id: I681aa6d0d57671b0fd9b7bc88de6f2cc202a7af0 Signed-off-by: V Sujith Kumar Reddy vsujithk@codeaurora.org --- M src/mainboard/google/trogdor/mainboard.c M src/soc/qualcomm/sc7180/bootblock.c 2 files changed, 15 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/38593/10
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
Patch Set 10:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38593/10/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38593/10/src/mainboard/google/trogd... PS10, Line 38: GPIO_PULL_UP,GPIO_8MA, GPIO_OUTPUT); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/38593/10/src/mainboard/google/trogd... PS10, Line 41: GPIO_PULL_UP,GPIO_8MA, GPIO_OUTPUT); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/38593/10/src/mainboard/google/trogd... PS10, Line 44: GPIO_PULL_UP,GPIO_8MA, GPIO_OUTPUT); space required after that ',' (ctx:VxV)
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
Patch Set 10:
(1 comment)
LGTM after style issues.
https://review.coreboot.org/c/coreboot/+/38593/10/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38593/10/src/mainboard/google/trogd... PS10, Line 38: GPIO_PULL_UP,GPIO_8MA, GPIO_OUTPUT);
space required after that ',' (ctx:VxV)
Please fix these style errors
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
Patch Set 11:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38593/11/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38593/11/src/mainboard/google/trogd... PS11, Line 38: GPIO_PULL_UP,GPIO_8MA, GPIO_OUTPUT); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/38593/11/src/mainboard/google/trogd... PS11, Line 41: GPIO_PULL_UP,GPIO_8MA, GPIO_OUTPUT); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/38593/11/src/mainboard/google/trogd... PS11, Line 44: GPIO_PULL_UP,GPIO_8MA, GPIO_OUTPUT); space required after that ',' (ctx:VxV)
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
Patch Set 12:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38593/12/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38593/12/src/mainboard/google/trogd... PS12, Line 38: GPIO_PULL_UP,GPIO_8MA, GPIO_OUTPUT); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/38593/12/src/mainboard/google/trogd... PS12, Line 41: GPIO_PULL_UP,GPIO_8MA, GPIO_OUTPUT); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/38593/12/src/mainboard/google/trogd... PS12, Line 44: GPIO_PULL_UP,GPIO_8MA, GPIO_OUTPUT); space required after that ',' (ctx:VxV)
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
Patch Set 13:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38593/13/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38593/13/src/mainboard/google/trogd... PS13, Line 38: GPIO_PULL_UP,GPIO_8MA, GPIO_OUTPUT); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/38593/13/src/mainboard/google/trogd... PS13, Line 41: GPIO_PULL_UP,GPIO_8MA, GPIO_OUTPUT); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/38593/13/src/mainboard/google/trogd... PS13, Line 44: GPIO_PULL_UP,GPIO_8MA, GPIO_OUTPUT); space required after that ',' (ctx:VxV)
Ravi kumar has uploaded a new patch set (#14) to the change originally created by mturney mturney. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
sc7180: GPIO: Add I2S configuration for sc7180
Configuring GPIO Pins as I2S mode for Audio speaker.
Change-Id: I681aa6d0d57671b0fd9b7bc88de6f2cc202a7af0 Signed-off-by: V Sujith Kumar Reddy vsujithk@codeaurora.org --- M src/mainboard/google/trogdor/board.h M src/mainboard/google/trogdor/chromeos.c M src/mainboard/google/trogdor/mainboard.c 3 files changed, 23 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/38593/14
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38593/14/src/mainboard/google/trogd... File src/mainboard/google/trogdor/chromeos.c:
https://review.coreboot.org/c/coreboot/+/38593/14/src/mainboard/google/trogd... PS14, Line 32: gpio_output(GPIO_AMP_ENABLE, 1); This should be initialized to 0. It will only get changed to 1 by depthcharge once we're actually trying to use the speaker.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
Patch Set 16:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38593/16/src/mainboard/google/trogd... File src/mainboard/google/trogdor/board.h:
https://review.coreboot.org/c/coreboot/+/38593/16/src/mainboard/google/trogd... PS16, Line 26: board_id() < 1 Actually, let's please change this into a CONFIG(TROGDOR_REV0) check instead. The problem is that we now also have Lazor boards and they are starting board IDs at 0 again, but their schematics are based off Trogdor rev1.
https://review.coreboot.org/c/coreboot/+/38593/16/src/mainboard/google/trogd... File src/mainboard/google/trogdor/chromeos.c:
https://review.coreboot.org/c/coreboot/+/38593/16/src/mainboard/google/trogd... PS16, Line 32: gpio_output(GPIO_AMP_ENABLE, 1); Can we just merge the line from CB:40534 into this patch as well?
https://review.coreboot.org/c/coreboot/+/38593/16/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38593/16/src/mainboard/google/trogd... PS16, Line 37: GPIO_PULL_UP Can you please double-check that this is correct? Why would we have a pull-up on an output pin? According to my (limited) electrical understanding, that's basically never the right thing to do.
Ravi kumar has uploaded a new patch set (#18) to the change originally created by mturney mturney. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
sc7180: GPIO: Add I2S configuration for sc7180
Configuring GPIO Pins as I2S mode for Audio speaker.
Change-Id: I681aa6d0d57671b0fd9b7bc88de6f2cc202a7af0 Signed-off-by: V Sujith Kumar Reddy vsujithk@codeaurora.org --- M src/mainboard/google/trogdor/board.h M src/mainboard/google/trogdor/chromeos.c M src/mainboard/google/trogdor/mainboard.c 3 files changed, 23 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/38593/18
Ravi kumar has uploaded a new patch set (#19) to the change originally created by mturney mturney. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
sc7180: GPIO: Add I2S configuration for sc7180
Configuring GPIO Pins as I2S mode for Audio speaker.
Change-Id: I681aa6d0d57671b0fd9b7bc88de6f2cc202a7af0 Signed-off-by: V Sujith Kumar Reddy vsujithk@codeaurora.org --- M src/mainboard/google/trogdor/board.h M src/mainboard/google/trogdor/chromeos.c M src/mainboard/google/trogdor/mainboard.c 3 files changed, 21 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/38593/19
Hello Ravi kumar, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38593
to look at the new patch set (#20).
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
sc7180: GPIO: Add I2S configuration for sc7180
Configuring GPIO Pins as I2S mode for Audio speaker.
Change-Id: I681aa6d0d57671b0fd9b7bc88de6f2cc202a7af0 Signed-off-by: V Sujith Kumar Reddy vsujithk@codeaurora.org --- M src/mainboard/google/trogdor/board.h M src/mainboard/google/trogdor/chromeos.c 2 files changed, 7 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/38593/20
Ajit Pandey has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
Patch Set 21:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38593/16/src/mainboard/google/trogd... File src/mainboard/google/trogdor/board.h:
https://review.coreboot.org/c/coreboot/+/38593/16/src/mainboard/google/trogd... PS16, Line 26: board_id() < 1
Actually, let's please change this into a CONFIG(TROGDOR_REV0) check instead. […]
Done
https://review.coreboot.org/c/coreboot/+/38593/16/src/mainboard/google/trogd... File src/mainboard/google/trogdor/chromeos.c:
https://review.coreboot.org/c/coreboot/+/38593/16/src/mainboard/google/trogd... PS16, Line 32: gpio_output(GPIO_AMP_ENABLE, 1);
Can we just merge the line from CB:40534 into this patch as well?
Done
https://review.coreboot.org/c/coreboot/+/38593/16/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38593/16/src/mainboard/google/trogd... PS16, Line 37: GPIO_PULL_UP
Can you please double-check that this is correct? Why would we have a pull-up on an output pin? Acco […]
Hmm.. ideally it should be NO_PULL in case of output gpio but I guess PULL_UP also wouldn't affect functionality in case of output. Please let us know if you want us to change it to NO_PULL
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
Patch Set 21:
(7 comments)
https://review.coreboot.org/c/coreboot/+/38593/21//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38593/21//COMMIT_MSG@7 PS21, Line 7: for sc7180 Already in the prefix, but the prefix should be:
google/trogdor:
https://review.coreboot.org/c/coreboot/+/38593/21//COMMIT_MSG@8 PS21, Line 8: Please add a problem description. Something like:
The audio speaker does not work on Trogdor revision 1, as the layout was changed.
https://review.coreboot.org/c/coreboot/+/38593/21//COMMIT_MSG@9 PS21, Line 9: Audio audio
https://review.coreboot.org/c/coreboot/+/38593/21//COMMIT_MSG@9 PS21, Line 9: Configuring Configure
https://review.coreboot.org/c/coreboot/+/38593/21//COMMIT_MSG@9 PS21, Line 9: Configuring GPIO Pins as I2S mode for Audio speaker. … as per schematics?
https://review.coreboot.org/c/coreboot/+/38593/21//COMMIT_MSG@9 PS21, Line 9: Pins pins
https://review.coreboot.org/c/coreboot/+/38593/21//COMMIT_MSG@10 PS21, Line 10: Is there a bug for this issue. Please add it as reference.
Ajit Pandey has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
Patch Set 22:
(8 comments)
https://review.coreboot.org/c/coreboot/+/38593/21//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38593/21//COMMIT_MSG@7 PS21, Line 7: for sc7180
Already in the prefix, but the prefix should be: […]
done
https://review.coreboot.org/c/coreboot/+/38593/21//COMMIT_MSG@8 PS21, Line 8:
Please add a problem description. Something like: […]
done
https://review.coreboot.org/c/coreboot/+/38593/21//COMMIT_MSG@9 PS21, Line 9: Audio
audio
done
https://review.coreboot.org/c/coreboot/+/38593/21//COMMIT_MSG@9 PS21, Line 9: Configuring GPIO Pins as I2S mode for Audio speaker.
… as per schematics?
done
https://review.coreboot.org/c/coreboot/+/38593/21//COMMIT_MSG@9 PS21, Line 9: Configuring
Configure
done
https://review.coreboot.org/c/coreboot/+/38593/21//COMMIT_MSG@9 PS21, Line 9: Pins
pins
Done
https://review.coreboot.org/c/coreboot/+/38593/21//COMMIT_MSG@10 PS21, Line 10:
Is there a bug for this issue. Please add it as reference.
done
https://review.coreboot.org/c/coreboot/+/38593/16/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38593/16/src/mainboard/google/trogd... PS16, Line 37: GPIO_PULL_UP
Hmm.. […]
done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
Patch Set 22: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/38593/16/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38593/16/src/mainboard/google/trogd... PS16, Line 37: GPIO_PULL_UP
done
I know it's functionally the same, but it is potentially wasting power by leakage through the pull resistor. If it works without the pull, please disable it. (Also, more importantly, I noticed that the same pull seems to be configured in the kernel DTS, so please disable it there as well.)
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
Patch Set 22:
(7 comments)
I'm gonna merge this for now so we can move forward with this patch train, please submit that GPIO pull fix as a separate CL.
https://review.coreboot.org/c/coreboot/+/38593/14/src/mainboard/google/trogd... File src/mainboard/google/trogdor/chromeos.c:
https://review.coreboot.org/c/coreboot/+/38593/14/src/mainboard/google/trogd... PS14, Line 32: gpio_output(GPIO_AMP_ENABLE, 1);
This should be initialized to 0. […]
No longer relevant.
https://review.coreboot.org/c/coreboot/+/38593/10/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38593/10/src/mainboard/google/trogd... PS10, Line 38: GPIO_PULL_UP,GPIO_8MA, GPIO_OUTPUT);
Please fix these style errors
Done
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... File src/soc/qualcomm/sc7180/qi2s.c:
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... PS1, Line 28: static void configure_gpios(void)
This is mainboard-specific and belongs in ramstage. […]
No longer relevant.
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... PS1, Line 31: gpio_output(GPIO(49), 1);
Do you need to do these when you're doing a gpio_configure() to a special function right afterwards […]
No longer relevant.
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... PS1, Line 36: gpio_configure(GPIO(23), 0, GPIO_PULL_UP,
Looks like this one is just a normal enable GPIO, not a real I2S pin? then why the extra gpio_config […]
Done
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... PS1, Line 38: 1
Please use the actual special function macros for the respective pin (e.g. […]
Done
https://review.coreboot.org/c/coreboot/+/38593/6/src/soc/qualcomm/sc7180/qi2... File src/soc/qualcomm/sc7180/qi2s.c:
https://review.coreboot.org/c/coreboot/+/38593/6/src/soc/qualcomm/sc7180/qi2... PS6, Line 16: #include <arch/cache.h> : #include <device/mmio.h> : #include <soc/addressmap.h> : #include <soc/gpio.h> : #include <soc/clock.h> : #include <symbols.h> : #include <assert.h> : #include <gpio.h> : #include <string.h> : #include <soc/qi2s.h>
Looks like you don't use all of those includes. […]
No longer relevant.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
Patch Set 22:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... File src/soc/qualcomm/sc7180/qi2s.c:
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... PS1, Line 28: static void configure_gpios(void)
No longer relevant.
Ack
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... PS1, Line 31: gpio_output(GPIO(49), 1);
No longer relevant.
Ack
https://review.coreboot.org/c/coreboot/+/38593/6/src/soc/qualcomm/sc7180/qi2... File src/soc/qualcomm/sc7180/qi2s.c:
https://review.coreboot.org/c/coreboot/+/38593/6/src/soc/qualcomm/sc7180/qi2... PS6, Line 16: #include <arch/cache.h> : #include <device/mmio.h> : #include <soc/addressmap.h> : #include <soc/gpio.h> : #include <soc/clock.h> : #include <symbols.h> : #include <assert.h> : #include <gpio.h> : #include <string.h> : #include <soc/qi2s.h>
No longer relevant.
Ack
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
sc7180: GPIO: Add I2S configuration for sc7180
Configuring GPIO Pins as I2S mode for Audio speaker.
Change-Id: I681aa6d0d57671b0fd9b7bc88de6f2cc202a7af0 Signed-off-by: V Sujith Kumar Reddy vsujithk@codeaurora.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/38593 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- M src/mainboard/google/trogdor/board.h M src/mainboard/google/trogdor/chromeos.c 2 files changed, 7 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/mainboard/google/trogdor/board.h b/src/mainboard/google/trogdor/board.h index fa2fb9f..63d03db 100644 --- a/src/mainboard/google/trogdor/board.h +++ b/src/mainboard/google/trogdor/board.h @@ -11,7 +11,9 @@ #define GPIO_AP_EC_INT GPIO(94) #define GPIO_AP_SUSPEND GPIO(20) #define GPIO_WP_STATE GPIO(42) -#define GPIO_H1_AP_INT GPIO(21) +#define GPIO_H1_AP_INT (CONFIG(TROGDOR_REV0) ? GPIO(21) : GPIO(42)) +#define GPIO_SD_CD_L GPIO(69) +#define GPIO_AMP_ENABLE GPIO(23)
void setup_chromeos_gpios(void);
diff --git a/src/mainboard/google/trogdor/chromeos.c b/src/mainboard/google/trogdor/chromeos.c index 985ba0f..324d6ca 100644 --- a/src/mainboard/google/trogdor/chromeos.c +++ b/src/mainboard/google/trogdor/chromeos.c @@ -28,6 +28,10 @@ "EC interrupt"}, {GPIO_H1_AP_INT.addr, ACTIVE_LOW, gpio_get(GPIO_H1_AP_INT), "TPM interrupt"}, + {GPIO_SD_CD_L.addr, ACTIVE_LOW, gpio_get(GPIO_SD_CD_L), + "SD card detect"}, + {GPIO_AMP_ENABLE.addr, ACTIVE_HIGH, gpio_get(GPIO_AMP_ENABLE), + "speaker enable"}, };
lb_add_gpios(gpios, chromeos_gpios, ARRAY_SIZE(chromeos_gpios));
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
Patch Set 23:
Automatic boot test returned (PASS/FAIL/TOTAL): 2/2/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : FAIL : https://lava.9esec.io/r/3447 "QEMU x86 q35/ich9" using payload SeaBIOS : FAIL : https://lava.9esec.io/r/3446 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3445 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/3444
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Ajit Pandey (Code Review)
-
build bot (Jenkins) (Code Review)
-
HAOUAS Elyes (Code Review)
-
Julius Werner (Code Review)
-
mturney mturney (Code Review)
-
Paul Menzel (Code Review)
-
Ravi kumar (Code Review)