Re: [PATCH] zynqmp: restore the jtag interface

["Jorge Ramirez-Ortiz, Foundries" <jorge@foundries.io>]

On 12/08/21, Michal Simek wrote:
> 
> 
> On 7/16/21 7:16 PM, Jorge Ramirez-Ortiz wrote:
> > As a security feature, if boot.bin was configured for secure boot the
> > CSU will disable the JTAG interface on all cases.
> > 
> > Some boards might rely on this interface for flashing to QSPI in which
> > case those systems might end up bricked during development.
> > 
> > This commit attempts to restore the interface - if the CSU allows for it.
> 
> sorry I missed this.
> 
> > 
> > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> > ---
> >  arch/arm/mach-zynqmp/Kconfig                 |  9 ++++++++
> >  arch/arm/mach-zynqmp/include/mach/hardware.h | 23 +++++++++++++------
> >  board/xilinx/zynqmp/zynqmp.c                 | 24 ++++++++++++++++++++
> >  3 files changed, 49 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/arm/mach-zynqmp/Kconfig b/arch/arm/mach-zynqmp/Kconfig
> > index 39144d654e..1e551c0020 100644
> > --- a/arch/arm/mach-zynqmp/Kconfig
> > +++ b/arch/arm/mach-zynqmp/Kconfig
> > @@ -149,6 +149,15 @@ config SPL_ZYNQMP_ALT_BOOTMODE_ENABLED
> >  	  Overwrite bootmode selected via boot mode pins to tell SPL what should
> >  	  be the next boot device.
> >  
> > +config SPL_ZYNQMP_RESTORE_JTAG
> > +       bool "Restore JTAG"
> > +       depends on SPL
> > +       default n
> 
> no need for default n - that's default option anyway.

ok

> 
> > +       help
> > +	 Booting SPL in secure mode causes the CSU to disable the JTAG interface
> > +	 even if no eFuses were burnt. This option restores the interface if
> > +	 possible.
> 
> Indentation is weird. Please look at tabs/spaces.

ok

> 
> > +
> >  config ZYNQ_SDHCI_MAX_FREQ
> >  	default 200000000
> >  
> > diff --git a/arch/arm/mach-zynqmp/include/mach/hardware.h b/arch/arm/mach-zynqmp/include/mach/hardware.h
> > index 3776499070..58822e3f25 100644
> > --- a/arch/arm/mach-zynqmp/include/mach/hardware.h
> > +++ b/arch/arm/mach-zynqmp/include/mach/hardware.h
> > @@ -42,17 +42,20 @@
> >  struct crlapb_regs {
> >  	u32 reserved0[36];
> >  	u32 cpu_r5_ctrl; /* 0x90 */
> > -	u32 reserved1[37];
> > +	u32 reserved1[7];
> > +	u32 dbg_lpd_ctrl; /* 0xB0 */
> > +	u32 reserved2[29];
> >  	u32 timestamp_ref_ctrl; /* 0x128 */
> > -	u32 reserved2[53];
> > +	u32 reserved3[53];
> >  	u32 boot_mode; /* 0x200 */
> > -	u32 reserved3_0[7];
> > +	u32 reserved4_0[7];
> >  	u32 reset_reason; /* 0x220 */
> > -	u32 reserved3_1[6];
> > +	u32 reserved4_1[6];
> >  	u32 rst_lpd_top; /* 0x23C */
> > -	u32 reserved4[4];
> > +	u32 rst_lpd_dbg; /* 0x240 */
> > +	u32 reserved5[3];
> >  	u32 boot_pin_ctrl; /* 0x250 */
> > -	u32 reserved5[21];
> > +	u32 reserved6[21];
> >  };
> >  
> >  #define crlapb_base ((struct crlapb_regs *)ZYNQMP_CRL_APB_BASEADDR)
> > @@ -141,9 +144,15 @@ struct apu_regs {
> >  struct csu_regs {
> >  	u32 reserved0[4];
> >  	u32 multi_boot;
> > -	u32 reserved1[11];
> > +	u32 reserved1[7];
> > +	u32 jtag_chain_status_wr;
> > +	u32 jtag_chain_status;
> > +	u32 jtag_sec;
> > +	u32 jtag_dap_cfg;
> >  	u32 idcode;
> >  	u32 version;
> > +	u32 reserved2[3055];
> > +	u32 pcap_prog;
> >  };
> >  
> >  #define csu_base ((struct csu_regs *)ZYNQMP_CSU_BASEADDR)
> > diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
> > index 1748fec2e4..feffda54e7 100644
> > --- a/board/xilinx/zynqmp/zynqmp.c
> > +++ b/board/xilinx/zynqmp/zynqmp.c
> > @@ -355,6 +355,25 @@ static int multi_boot(void)
> >  	return 0;
> >  }
> >  
> > +#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ZYNQMP_RESTORE_JTAG)
> > +static void restore_jtag(void)
> > +{
> > +	const u32 disable_security_gate = 0xff;
> > +	const u32 setup_clock = 0x01002002;
> > +	const u32 setup_jtag = 0x3;
> > +	const u32 release_pl = 0x1;
> > +	const u32 enable_debug = 0xff;
> > +	const u32 do_reset = 0x0;
> 
> All above should be just macros. Please put them to hardware.h

you mean something like #define CSU_SETUP_CLOCK 0x01002002?
I dont really agree with that because we'll lose the context where it
is being used (and the context is given by the restore jtag function)
unless we do CSU_RESTORE_JTAG_SETUP_CLOCK ....
is this what you want? 

> 
> > +
> > +	writel(disable_security_gate, &csu_base->jtag_sec);
> > +	writel(enable_debug, &csu_base->jtag_dap_cfg);
> > +	writel(setup_jtag, &csu_base->jtag_chain_status_wr);
> > +	writel(setup_clock, &crlapb_base->dbg_lpd_ctrl);
> > +	writel(do_reset, &crlapb_base->rst_lpd_dbg);
> > +	writel(release_pl, &csu_base->pcap_prog);
> > +}
> > +#endif
> > +
> >  #define PS_SYSMON_ANALOG_BUS_VAL	0x3210
> >  #define PS_SYSMON_ANALOG_BUS_REG	0xFFA50914
> >  
> > @@ -374,6 +393,11 @@ int board_init(void)
> >  		zynqmp_pmufw_load_config_object(zynqmp_pm_cfg_obj,
> >  						zynqmp_pm_cfg_obj_size);
> >  	printf("Silicon version:\t%d\n", zynqmp_get_silicon_version());
> > +
> > +#if defined(CONFIG_SPL_ZYNQMP_RESTORE_JTAG)
> > +	/* the CSU disables the JTAG interface when secure boot is enabled */
> > +	restore_jtag();
> > +#endif
> 
> As checkpatch suggests
> WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef'
> where possible
> 
> 
> >  #else
> >  	if (CONFIG_IS_ENABLED(DM_I2C) && CONFIG_IS_ENABLED(I2C_EEPROM))
> >  		xilinx_read_eeprom();
> > 
> 
> Thanks,
> Michal
> 
> -- 
> Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
> w: www.monstr.eu p: +42-0-721842854
> Maintainer of Linux kernel - Xilinx Microblaze
> Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
> U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs
>