| Message ID | 20240221-jaguar-v3-11-1f256a82201b@theobroma-systems.com |
|---|---|
| State | Superseded |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | rockchip: add support for Theobroma JAGUAR SBC-RK3588-AMR | expand |
Hi Quentin, On 2024/2/21 18:37, Quentin Schulz wrote: > From: Quentin Schulz<quentin.schulz@theobroma-systems.com> > > Rockchip SoCs have some jtag/sdmmc autoswitching that simply doesn't > work really well.[00] The Linux kernel disables it for all SoCs[01], so > U-Boot needs to do the same in order to fix issues related to SD card on > RK3588. This autoswitching is enabled (by default) via the force_jtag > bitfield in SYS_GRF_SOC_CON6 in the TRM part1. > > The conditions to enter forced JTAG mode are the following: > - force_jtag is 1 > - GPIO4_D2 is muxed for SDMMC function > - GPIO3_D3 is muxed for SDMMC function > - SDMMC_DET is inactive I have explain the reason and send fix for other boards in kernel for why the force jtag function not working correctly, see patch [1] and I copy the commit message here: RK3588 has force_jtage feature which is enable JTAG function via sdmmc pins automatically when there is no SD card insert, this feature will need the GPIO0A4 works in sdmmc_det function like other mmc signal instead of GPIO function, or else the force_jtag can not auto be disabled when SD card insert. For the rk3588 boards available on mainline kernel, 10 boards enable SD card function and 9 of then use GPIO0A4 as sdmmc_det, they should all able to use the force_jtag after my patch apply. The jaguar-rk3588 still have the issue because there is no sdmmc_det used. The condition of force JTAG mode work correctly is : - force_jtag in grf is 1; - all the SDMMC signal including SDMMC_DET set as SDMMC function in GRF IOC/IOMUX reg; So please update the commit message and it would be better to move this setting in jaguar-rk3588 board so that other board can enable force_jtag with only modify kernel but not both kernel and U-Boot. Thanks, - Kever [1]https://lkml.org/lkml/2024/2/4/48 > > SDMMC_DET may be put in the inactive mode by HW design (e.g. because the > detection pin is unused (e.g. card-detect-less designs), or another GPIO > is used instead, and SDMMC_DET is pulled-low by default). > > For some reason, when booting from SD card, everything's fine. But when > we don't boot from SD card, U-Boot needs to set it up correctly to allow > accessing SD cards, at the very least for RK3588-Jaguar > (card-detect-less design with SDMMC_DET pulled-low in HW). > > Therefore, let's disable the JTAG mode for the SDMMC pins so that the SD > controller can be used either as a fallback mechanism to find U-Boot > proper when the boot medium used to load TPL/SPL cannot find U-Boot > proper, or to be used in U-Boot CLI. > > Note that soc_con[0] is reserved. But considering that it's way more > user-friendly to access soc_con1 from the TRM with soc_con[1] than > soc_con[0], and that soc_con0 would actually be located at 4 bytes > before soc_con1, let's just make soc_con0 part of the soc_con array. > > [00]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c9b75d51c940c25587a2ad72ec7ec60490abfb6c > [01]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/soc/rockchip/grf.c > > Cc: Quentin Schulz<foss+uboot@0leil.net> > Signed-off-by: Quentin Schulz<quentin.schulz@theobroma-systems.com> > --- > arch/arm/include/asm/arch-rockchip/grf_rk3588.h | 24 ++++++++++++++++++++++++ > arch/arm/mach-rockchip/rk3588/rk3588.c | 7 +++++++ > 2 files changed, 31 insertions(+) > > diff --git a/arch/arm/include/asm/arch-rockchip/grf_rk3588.h b/arch/arm/include/asm/arch-rockchip/grf_rk3588.h > index e0694068bb1..f0ecff97f0b 100644 > --- a/arch/arm/include/asm/arch-rockchip/grf_rk3588.h > +++ b/arch/arm/include/asm/arch-rockchip/grf_rk3588.h > @@ -32,4 +32,28 @@ struct rk3588_pmu1grf { > > check_member(rk3588_pmu1grf, sd_detect_cnt, 0x03b0); > > +#define SYS_GRF_BASE 0xfd58c000 > + > +struct rk3588_sysgrf { > + unsigned int wdt_con0; > + unsigned int reserved0[(0x0010 - 0x0000) / 4 - 1]; > + unsigned int uart_con[2]; > + unsigned int reserved1[(0x00c0 - 0x0014) / 4 - 1]; > + unsigned int gic_con0; > + unsigned int reserved2[(0x0200 - 0x00c0) / 4 - 1]; > + unsigned int memcfg_con[32]; > + unsigned int reserved3[(0x0300 - 0x027c) / 4 - 1]; > + /* soc_con0 is reserved */ > + unsigned int soc_con[14]; > + unsigned int reserved4[(0x0380 - 0x0334) / 4 - 1]; > + unsigned int soc_status[4]; > + unsigned int reserved5[(0x0500 - 0x038c) / 4 - 1]; > + unsigned int otp_key08; > + unsigned int otp_key0d; > + unsigned int otp_key0e; > + unsigned int reserved6[(0x0600 - 0x0508) / 4 - 1]; > + unsigned int chip_id; > +}; > + > +check_member(rk3588_sysgrf, chip_id, 0x0600); > #endif /*__SOC_ROCKCHIP_RK3588_GRF_H__ */ > diff --git a/arch/arm/mach-rockchip/rk3588/rk3588.c b/arch/arm/mach-rockchip/rk3588/rk3588.c > index 38e95a6e2b2..c5eeda9d751 100644 > --- a/arch/arm/mach-rockchip/rk3588/rk3588.c > +++ b/arch/arm/mach-rockchip/rk3588/rk3588.c > @@ -11,6 +11,7 @@ > #include <asm/arch-rockchip/bootrom.h> > #include <asm/arch-rockchip/hardware.h> > #include <asm/arch-rockchip/ioc_rk3588.h> > +#include <asm/arch-rockchip/grf_rk3588.h> > > #define FIREWALL_DDR_BASE 0xfe030000 > #define FW_DDR_MST5_REG 0x54 > @@ -35,6 +36,8 @@ > #define BUS_IOC_GPIO2D_IOMUX_SEL_H 0x5c > #define BUS_IOC_GPIO3A_IOMUX_SEL_L 0x60 > > +#define SYS_GRF_FORCE_JTAG BIT(14) > + > /** > * Boot-device identifiers used by the BROM on RK3588 when device is booted > * from SPI flash. IOMUX used for SPI flash affect the value used by the BROM > @@ -134,6 +137,7 @@ void rockchip_stimer_init(void) > int arch_cpu_init(void) > { > #ifdef CONFIG_SPL_BUILD > + static struct rk3588_sysgrf * const sys_grf = (void *)SYS_GRF_BASE; > int secure_reg; > > /* Set the SDMMC eMMC crypto_ns FSPI access secure area */ > @@ -168,6 +172,9 @@ int arch_cpu_init(void) > secure_reg = readl(FIREWALL_SYSMEM_BASE + FW_SYSM_MST27_REG); > secure_reg &= 0xffff0000; > writel(secure_reg, FIREWALL_SYSMEM_BASE + FW_SYSM_MST27_REG); > + > + /* Disable JTAG exposed on SDMMC */ > + rk_clrreg(&sys_grf->soc_con[6], SYS_GRF_FORCE_JTAG); > #endif > > return 0; >
Hi Kever, On 3/8/24 11:05, Kever Yang wrote: > Hi Quentin, > > On 2024/2/21 18:37, Quentin Schulz wrote: >> From: Quentin Schulz<quentin.schulz@theobroma-systems.com> >> >> Rockchip SoCs have some jtag/sdmmc autoswitching that simply doesn't >> work really well.[00] The Linux kernel disables it for all SoCs[01], so >> U-Boot needs to do the same in order to fix issues related to SD card on >> RK3588. This autoswitching is enabled (by default) via the force_jtag >> bitfield in SYS_GRF_SOC_CON6 in the TRM part1. >> >> The conditions to enter forced JTAG mode are the following: >> - force_jtag is 1 >> - GPIO4_D2 is muxed for SDMMC function >> - GPIO3_D3 is muxed for SDMMC function >> - SDMMC_DET is inactive > > I have explain the reason and send fix for other boards in kernel for > why the force jtag > > function not working correctly, see patch [1] and I copy the commit > message here: > > RK3588 has force_jtage feature which is enable JTAG function via sdmmc > pins automatically when there is no SD card insert, this feature will > need the GPIO0A4 works in sdmmc_det function like other mmc signal instead > of GPIO function, or else the force_jtag can not auto be disabled when > SD card insert. > > For the rk3588 boards available on mainline kernel, 10 boards enable SD > card function > and 9 of then use GPIO0A4 as sdmmc_det, they should all able to use the > force_jtag after > my patch apply. > The jaguar-rk3588 still have the issue because there is no sdmmc_det used. > The condition of force JTAG mode work correctly is : > - force_jtag in grf is 1; > - all the SDMMC signal including SDMMC_DET set as SDMMC function in GRF > IOC/IOMUX reg; > > So please update the commit message and it would be better to move this > setting in jaguar-rk3588 board Thanks, it's a tiny bit clearer to me now. We never exit this JTAG mode on Jaguar because we have GPIO0A4 floating (with internal pull-down). On Tiger (patches coming this month I hope), it's hardwired to ground but we use a different GPIO as card detect, so we'll have a similar issue. In any case, thanks for the explanation. Though, I do not understand why you absolutely want this force_jtag to be always enabled. It is a debug feature. Even more so, this is a security issue. I don't think anyone wants to have the JTAG enabled if the user plays with the card detect pin or boot without an SD card connected. I really insist this should be enabled on the SoC level, the same way that the Linux kernel does it. We could compromise on a Kconfig symbol to enable force_jtag for people who want it but still default to disabled on the SoC level, but I'm not really seeing a reason for it. > so that other board can enable force_jtag with only modify kernel but > not both kernel and U-Boot. > If you want JTAG support in U-Boot, you would patch U-Boot. If you want JTAG support in the kernel, you would patch the kernel. If you want it in both, you'd patch both. I'm not sure to understand what disabling force_jtag in U-Boot has to do with force_jtag enabled from the Linux kernel, what am I missing here? Cheers, Quentin
Hi Quentin, On 2024/3/9 01:55, Quentin Schulz wrote: > Hi Kever, > > On 3/8/24 11:05, Kever Yang wrote: >> Hi Quentin, >> >> On 2024/2/21 18:37, Quentin Schulz wrote: >>> From: Quentin Schulz<quentin.schulz@theobroma-systems.com> >>> >>> Rockchip SoCs have some jtag/sdmmc autoswitching that simply doesn't >>> work really well.[00] The Linux kernel disables it for all SoCs[01], so >>> U-Boot needs to do the same in order to fix issues related to SD >>> card on >>> RK3588. This autoswitching is enabled (by default) via the force_jtag >>> bitfield in SYS_GRF_SOC_CON6 in the TRM part1. >>> >>> The conditions to enter forced JTAG mode are the following: >>> - force_jtag is 1 >>> - GPIO4_D2 is muxed for SDMMC function >>> - GPIO3_D3 is muxed for SDMMC function >>> - SDMMC_DET is inactive >> >> I have explain the reason and send fix for other boards in kernel >> for why the force jtag >> >> function not working correctly, see patch [1] and I copy the commit >> message here: >> >> RK3588 has force_jtage feature which is enable JTAG function via sdmmc >> pins automatically when there is no SD card insert, this feature will >> need the GPIO0A4 works in sdmmc_det function like other mmc signal >> instead >> of GPIO function, or else the force_jtag can not auto be disabled when >> SD card insert. >> >> For the rk3588 boards available on mainline kernel, 10 boards enable >> SD card function >> and 9 of then use GPIO0A4 as sdmmc_det, they should all able to use >> the force_jtag after >> my patch apply. >> The jaguar-rk3588 still have the issue because there is no sdmmc_det >> used. >> The condition of force JTAG mode work correctly is : >> - force_jtag in grf is 1; >> - all the SDMMC signal including SDMMC_DET set as SDMMC function in >> GRF IOC/IOMUX reg; >> >> So please update the commit message and it would be better to move >> this setting in jaguar-rk3588 board > > Thanks, it's a tiny bit clearer to me now. We never exit this JTAG > mode on Jaguar because we have GPIO0A4 floating (with internal > pull-down). On Tiger (patches coming this month I hope), it's > hardwired to ground but we use a different GPIO as card detect, so > we'll have a similar issue. In any case, thanks for the explanation. > > Though, I do not understand why you absolutely want this force_jtag to > be always enabled. It is a debug feature. You are right, it's not correct to always enabled for this debug feature. Let's leave this as rk3588 common code. I will add some document in rockchip.rst for people to enable this feature easier, this feature also important for some project during develop. Thanks, - Kever > > Even more so, this is a security issue. I don't think anyone wants to > have the JTAG enabled if the user plays with the card detect pin or > boot without an SD card connected. I really insist this should be > enabled on the SoC level, the same way that the Linux kernel does it. > > We could compromise on a Kconfig symbol to enable force_jtag for > people who want it but still default to disabled on the SoC level, but > I'm not really seeing a reason for it. > >> so that other board can enable force_jtag with only modify kernel but >> not both kernel and U-Boot. >> > If you want JTAG support in U-Boot, you would patch U-Boot. If you > want JTAG support in the kernel, you would patch the kernel. If you > want it in both, you'd patch both. I'm not sure to understand what > disabling force_jtag in U-Boot has to do with force_jtag enabled from > the Linux kernel, what am I missing here? > > Cheers, > Quentin
Hello all, On 2024-03-11 10:07, Kever Yang wrote: > Hi Quentin, > > On 2024/3/9 01:55, Quentin Schulz wrote: >> Hi Kever, >> >> On 3/8/24 11:05, Kever Yang wrote: >>> Hi Quentin, >>> >>> On 2024/2/21 18:37, Quentin Schulz wrote: >>>> From: Quentin Schulz<quentin.schulz@theobroma-systems.com> >>>> >>>> Rockchip SoCs have some jtag/sdmmc autoswitching that simply doesn't >>>> work really well.[00] The Linux kernel disables it for all SoCs[01], >>>> so >>>> U-Boot needs to do the same in order to fix issues related to SD >>>> card on >>>> RK3588. This autoswitching is enabled (by default) via the >>>> force_jtag >>>> bitfield in SYS_GRF_SOC_CON6 in the TRM part1. >>>> >>>> The conditions to enter forced JTAG mode are the following: >>>> - force_jtag is 1 >>>> - GPIO4_D2 is muxed for SDMMC function >>>> - GPIO3_D3 is muxed for SDMMC function >>>> - SDMMC_DET is inactive >>> >>> I have explain the reason and send fix for other boards in kernel >>> for why the force jtag >>> >>> function not working correctly, see patch [1] and I copy the commit >>> message here: >>> >>> RK3588 has force_jtage feature which is enable JTAG function via >>> sdmmc >>> pins automatically when there is no SD card insert, this feature will >>> need the GPIO0A4 works in sdmmc_det function like other mmc signal >>> instead >>> of GPIO function, or else the force_jtag can not auto be disabled >>> when >>> SD card insert. >>> >>> For the rk3588 boards available on mainline kernel, 10 boards enable >>> SD card function >>> and 9 of then use GPIO0A4 as sdmmc_det, they should all able to use >>> the force_jtag after >>> my patch apply. >>> The jaguar-rk3588 still have the issue because there is no sdmmc_det >>> used. >>> The condition of force JTAG mode work correctly is : >>> - force_jtag in grf is 1; >>> - all the SDMMC signal including SDMMC_DET set as SDMMC function in >>> GRF IOC/IOMUX reg; >>> >>> So please update the commit message and it would be better to move >>> this setting in jaguar-rk3588 board >> >> Thanks, it's a tiny bit clearer to me now. We never exit this JTAG >> mode on Jaguar because we have GPIO0A4 floating (with internal >> pull-down). On Tiger (patches coming this month I hope), it's >> hardwired to ground but we use a different GPIO as card detect, so >> we'll have a similar issue. In any case, thanks for the explanation. >> >> Though, I do not understand why you absolutely want this force_jtag to >> be always enabled. It is a debug feature. > > You are right, it's not correct to always enabled for this debug > feature. Let's leave this as rk3588 common code. > > I will add some document in rockchip.rst for people to enable this > feature easier, this feature also important for some project during > develop. I think that having a new configuration option that enables JTAG access would be good. Of course, it would remain disabled by default, but would allow anyone in need of JTAG access to get it working rather easily. >> Even more so, this is a security issue. I don't think anyone wants to >> have the JTAG enabled if the user plays with the card detect pin or >> boot without an SD card connected. I really insist this should be >> enabled on the SoC level, the same way that the Linux kernel does it. >> >> We could compromise on a Kconfig symbol to enable force_jtag for >> people who want it but still default to disabled on the SoC level, but >> I'm not really seeing a reason for it. >> >>> so that other board can enable force_jtag with only modify kernel but >>> not both kernel and U-Boot. >>> >> If you want JTAG support in U-Boot, you would patch U-Boot. If you >> want JTAG support in the kernel, you would patch the kernel. If you >> want it in both, you'd patch both. I'm not sure to understand what >> disabling force_jtag in U-Boot has to do with force_jtag enabled from >> the Linux kernel, what am I missing here? >> >> Cheers, >> Quentin
diff --git a/arch/arm/include/asm/arch-rockchip/grf_rk3588.h b/arch/arm/include/asm/arch-rockchip/grf_rk3588.h index e0694068bb1..f0ecff97f0b 100644 --- a/arch/arm/include/asm/arch-rockchip/grf_rk3588.h +++ b/arch/arm/include/asm/arch-rockchip/grf_rk3588.h @@ -32,4 +32,28 @@ struct rk3588_pmu1grf { check_member(rk3588_pmu1grf, sd_detect_cnt, 0x03b0); +#define SYS_GRF_BASE 0xfd58c000 + +struct rk3588_sysgrf { + unsigned int wdt_con0; + unsigned int reserved0[(0x0010 - 0x0000) / 4 - 1]; + unsigned int uart_con[2]; + unsigned int reserved1[(0x00c0 - 0x0014) / 4 - 1]; + unsigned int gic_con0; + unsigned int reserved2[(0x0200 - 0x00c0) / 4 - 1]; + unsigned int memcfg_con[32]; + unsigned int reserved3[(0x0300 - 0x027c) / 4 - 1]; + /* soc_con0 is reserved */ + unsigned int soc_con[14]; + unsigned int reserved4[(0x0380 - 0x0334) / 4 - 1]; + unsigned int soc_status[4]; + unsigned int reserved5[(0x0500 - 0x038c) / 4 - 1]; + unsigned int otp_key08; + unsigned int otp_key0d; + unsigned int otp_key0e; + unsigned int reserved6[(0x0600 - 0x0508) / 4 - 1]; + unsigned int chip_id; +}; + +check_member(rk3588_sysgrf, chip_id, 0x0600); #endif /*__SOC_ROCKCHIP_RK3588_GRF_H__ */ diff --git a/arch/arm/mach-rockchip/rk3588/rk3588.c b/arch/arm/mach-rockchip/rk3588/rk3588.c index 38e95a6e2b2..c5eeda9d751 100644 --- a/arch/arm/mach-rockchip/rk3588/rk3588.c +++ b/arch/arm/mach-rockchip/rk3588/rk3588.c @@ -11,6 +11,7 @@ #include <asm/arch-rockchip/bootrom.h> #include <asm/arch-rockchip/hardware.h> #include <asm/arch-rockchip/ioc_rk3588.h> +#include <asm/arch-rockchip/grf_rk3588.h> #define FIREWALL_DDR_BASE 0xfe030000 #define FW_DDR_MST5_REG 0x54 @@ -35,6 +36,8 @@ #define BUS_IOC_GPIO2D_IOMUX_SEL_H 0x5c #define BUS_IOC_GPIO3A_IOMUX_SEL_L 0x60 +#define SYS_GRF_FORCE_JTAG BIT(14) + /** * Boot-device identifiers used by the BROM on RK3588 when device is booted * from SPI flash. IOMUX used for SPI flash affect the value used by the BROM @@ -134,6 +137,7 @@ void rockchip_stimer_init(void) int arch_cpu_init(void) { #ifdef CONFIG_SPL_BUILD + static struct rk3588_sysgrf * const sys_grf = (void *)SYS_GRF_BASE; int secure_reg; /* Set the SDMMC eMMC crypto_ns FSPI access secure area */ @@ -168,6 +172,9 @@ int arch_cpu_init(void) secure_reg = readl(FIREWALL_SYSMEM_BASE + FW_SYSM_MST27_REG); secure_reg &= 0xffff0000; writel(secure_reg, FIREWALL_SYSMEM_BASE + FW_SYSM_MST27_REG); + + /* Disable JTAG exposed on SDMMC */ + rk_clrreg(&sys_grf->soc_con[6], SYS_GRF_FORCE_JTAG); #endif return 0;