[PATCH] zynqmp: restore the jtag interface

[Michal Simek]

On 8/27/21 11:17 AM, Jorge Ramirez-Ortiz, Foundries wrote:
> 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 at 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? 

Macro name is up to you but should register description.

For example yours disable_security_gate
is based on reg database ssss_pmu_sec + ssss_pltap_sec + ssss_dap_sec
together.

#define CSU_JTAG_SEC_GATE_DISABLE	0x1ff

And then you can use directly:

writel(CSU_JTAG_SEC_GATE_DISABLE, &csu_base->jtag_sec);

Thanks,
Michal