target: "mcr" and "mrc" are ARM-specific

Switch "mrc" and "mcr" commands to be toplevel ARM operations,
as they should initially have been.

Correct the usage message for both commands:  it matches ARM
documentation (as one wants!) instead of reordering them to
match the funky mrc() and mcr() method usage (sigh).

For Cortex-A8: restore a line that got accidentally dropped,
so the secure monitor mode shadow registers will show again.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

src/target/arm11.c

@@ -1219,6 +1219,13 @@ static int arm11_remove_watchpoint(struct target *target,
 	return ERROR_FAIL;
 }
 
+static int arm11_mrc(struct target *target, int cpnum,
+		uint32_t op1, uint32_t op2,
+		uint32_t CRn, uint32_t CRm, uint32_t *value);
+static int arm11_mcr(struct target *target, int cpnum,
+		uint32_t op1, uint32_t op2, uint32_t CRn,
+		uint32_t CRm, uint32_t value);
+
 static int arm11_target_create(struct target *target, Jim_Interp *interp)
 {
 	struct arm11_common *arm11;
@@ -1238,6 +1245,9 @@ static int arm11_target_create(struct target *target, Jim_Interp *interp)
 
 	armv4_5_init_arch_info(target, &arm11->arm);
 
+	arm11->arm.mrc = arm11_mrc;
+	arm11->arm.mcr = arm11_mcr;
+
 	arm11->target = target;
 
 	arm11->jtag_info.tap = target->tap;
@@ -1679,7 +1689,4 @@ struct target_type arm11_target = {
 	.target_create =	arm11_target_create,
 	.init_target =		arm11_init_target,
 	.examine =		arm11_examine,
-
-	.mrc =			arm11_mrc,
-	.mcr =			arm11_mcr,
 };

src/target/arm720t.c

@@ -378,11 +378,24 @@ static int arm720t_init_target(struct command_context *cmd_ctx, struct target *t
 	return arm7tdmi_init_target(cmd_ctx, target);
 }
 
+/* FIXME remove forward decls */
+static int arm720t_mrc(struct target *target, int cpnum,
+		uint32_t op1, uint32_t op2,
+		uint32_t CRn, uint32_t CRm,
+		uint32_t *value);
+static int arm720t_mcr(struct target *target, int cpnum,
+		uint32_t op1, uint32_t op2,
+		uint32_t CRn, uint32_t CRm,
+		uint32_t value);
+
 static int arm720t_init_arch_info(struct target *target,
 		struct arm720t_common *arm720t, struct jtag_tap *tap)
 {
 	struct arm7_9_common *arm7_9 = &arm720t->arm7_9_common;
 
+	arm7_9->armv4_5_common.mrc = arm720t_mrc;
+	arm7_9->armv4_5_common.mcr = arm720t_mcr;
+
 	arm7tdmi_init_arch_info(target, arm7_9, tap);
 
 	arm720t->common_magic = ARM720T_COMMON_MAGIC;
@@ -556,6 +569,4 @@ struct target_type arm720t_target =
 	.target_create = arm720t_target_create,
 	.init_target = arm720t_init_target,
 	.examine = arm7_9_examine,
-	.mrc = arm720t_mrc,
-	.mcr = arm720t_mcr,
 };

src/target/arm920t.c

@@ -624,10 +624,23 @@ int arm920t_soft_reset_halt(struct target *target)
 	return ERROR_OK;
 }
 
+/* FIXME remove forward decls */
+static int arm920t_mrc(struct target *target, int cpnum,
+		uint32_t op1, uint32_t op2,
+		uint32_t CRn, uint32_t CRm,
+		uint32_t *value);
+static int arm920t_mcr(struct target *target, int cpnum,
+		uint32_t op1, uint32_t op2,
+		uint32_t CRn, uint32_t CRm,
+		uint32_t value);
+
 int arm920t_init_arch_info(struct target *target, struct arm920t_common *arm920t, struct jtag_tap *tap)
 {
 	struct arm7_9_common *arm7_9 = &arm920t->arm7_9_common;
 
+	arm7_9->armv4_5_common.mrc = arm920t_mrc;
+	arm7_9->armv4_5_common.mcr = arm920t_mcr;
+
 	/* initialize arm7/arm9 specific info (including armv4_5) */
 	arm9tdmi_init_arch_info(target, arm7_9, tap);
 
@@ -1452,6 +1465,4 @@ struct target_type arm920t_target =
 	.target_create = arm920t_target_create,
 	.init_target = arm9tdmi_init_target,
 	.examine = arm7_9_examine,
-	.mrc = arm920t_mrc,
-	.mcr = arm920t_mcr,
 };

src/target/arm926ejs.c

@@ -673,6 +673,9 @@ int arm926ejs_init_arch_info(struct target *target, struct arm926ejs_common *arm
 {
 	struct arm7_9_common *arm7_9 = &arm926ejs->arm7_9_common;
 
+	arm7_9->armv4_5_common.mrc = arm926ejs_mrc;
+	arm7_9->armv4_5_common.mcr = arm926ejs_mcr;
+
 	/* initialize arm7/arm9 specific info (including armv4_5) */
 	arm9tdmi_init_arch_info(target, arm7_9, tap);
 
@@ -822,6 +825,4 @@ struct target_type arm926ejs_target =
 
 	.read_phys_memory = arm926ejs_read_phys_memory,
 	.write_phys_memory = arm926ejs_write_phys_memory,
-	.mrc = arm926ejs_mrc,
-	.mcr = arm926ejs_mcr,
 };

src/target/armv4_5.c

@@ -790,6 +790,137 @@ usage:
 	return retval;
 }
 
+static int jim_mcrmrc(Jim_Interp *interp, int argc, Jim_Obj *const *argv)
+{
+	struct command_context *context;
+	struct target *target;
+	struct arm *arm;
+	int retval;
+
+	context = Jim_GetAssocData(interp, "context");
+	if (context == NULL) {
+		LOG_ERROR("%s: no command context", __func__);
+		return JIM_ERR;
+	}
+	target = get_current_target(context);
+	if (target == NULL) {
+		LOG_ERROR("%s: no current target", __func__);
+		return JIM_ERR;
+	}
+	if (!target_was_examined(target)) {
+		LOG_ERROR("%s: not yet examined", target_name(target));
+		return JIM_ERR;
+	}
+	arm = target_to_arm(target);
+	if (!is_arm(arm)) {
+		LOG_ERROR("%s: not an ARM", target_name(target));
+		return JIM_ERR;
+	}
+
+	if ((argc < 6) || (argc > 7)) {
+		/* FIXME use the command name to verify # params... */
+		LOG_ERROR("%s: wrong number of arguments", __func__);
+		return JIM_ERR;
+	}
+
+	int cpnum;
+	uint32_t op1;
+	uint32_t op2;
+	uint32_t CRn;
+	uint32_t CRm;
+	uint32_t value;
+	long l;
+
+	/* NOTE:  parameter sequence matches ARM instruction set usage:
+	 *	MCR	pNUM, op1, rX, CRn, CRm, op2	; write CP from rX
+	 *	MRC	pNUM, op1, rX, CRn, CRm, op2	; read CP into rX
+	 * The "rX" is necessarily omitted; it uses Tcl mechanisms.
+	 */
+	retval = Jim_GetLong(interp, argv[1], &l);
+	if (retval != JIM_OK)
+		return retval;
+	if (l & ~0xf) {
+		LOG_ERROR("%s: %s %d out of range", __func__,
+				"coprocessor", (int) l);
+		return JIM_ERR;
+	}
+	cpnum = l;
+
+	retval = Jim_GetLong(interp, argv[2], &l);
+	if (retval != JIM_OK)
+		return retval;
+	if (l & ~0x7) {
+		LOG_ERROR("%s: %s %d out of range", __func__,
+				"op1", (int) l);
+		return JIM_ERR;
+	}
+	op1 = l;
+
+	retval = Jim_GetLong(interp, argv[3], &l);
+	if (retval != JIM_OK)
+		return retval;
+	if (l & ~0xf) {
+		LOG_ERROR("%s: %s %d out of range", __func__,
+				"CRn", (int) l);
+		return JIM_ERR;
+	}
+	CRn = l;
+
+	retval = Jim_GetLong(interp, argv[4], &l);
+	if (retval != JIM_OK)
+		return retval;
+	if (l & ~0xf) {
+		LOG_ERROR("%s: %s %d out of range", __func__,
+				"CRm", (int) l);
+		return JIM_ERR;
+	}
+	CRm = l;
+
+	retval = Jim_GetLong(interp, argv[5], &l);
+	if (retval != JIM_OK)
+		return retval;
+	if (l & ~0x7) {
+		LOG_ERROR("%s: %s %d out of range", __func__,
+				"op2", (int) l);
+		return JIM_ERR;
+	}
+	op2 = l;
+
+	value = 0;
+
+	/* FIXME don't assume "mrc" vs "mcr" from the number of params;
+	 * that could easily be a typo!  Check both...
+	 *
+	 * FIXME change the call syntax here ... simplest to just pass
+	 * the MRC() or MCR() instruction to be executed.  That will also
+	 * let us support the "mrc2" and "mcr2" opcodes (toggling one bit)
+	 * if that's ever needed.
+	 */
+	if (argc == 7) {
+		retval = Jim_GetLong(interp, argv[6], &l);
+		if (retval != JIM_OK) {
+			return retval;
+		}
+		value = l;
+
+		/* NOTE: parameters reordered! */
+		// ARMV4_5_MCR(cpnum, op1, 0, CRn, CRm, op2)
+		retval = arm->mcr(target, cpnum, op1, op2, CRn, CRm, value);
+		if (retval != ERROR_OK)
+			return JIM_ERR;
+	} else {
+		/* NOTE: parameters reordered! */
+		// ARMV4_5_MRC(cpnum, op1, 0, CRn, CRm, op2)
+		retval = arm->mrc(target, cpnum, op1, op2, CRn, CRm, &value);
+		if (retval != ERROR_OK)
+			return JIM_ERR;
+
+		Jim_SetResult(interp, Jim_NewIntObj(interp, value));
+	}
+
+	return JIM_OK;
+}
+
 static const struct command_registration arm_exec_command_handlers[] = {
 	{
 		.name = "reg",
@@ -811,6 +942,20 @@ static const struct command_registration arm_exec_command_handlers[] = {
 		.usage = "<address> [<count> ['thumb']]",
 		.help = "disassemble instructions ",
 	},
+	{
+		.name = "mcr",
+		.mode = COMMAND_EXEC,
+		.jim_handler = &jim_mcrmrc,
+		.help = "write coprocessor register",
+		.usage = "cpnum op1 CRn op2 CRm value",
+	},
+	{
+		.name = "mrc",
+		.jim_handler = &jim_mcrmrc,
+		.help = "read coprocessor register",
+		.usage = "cpnum op1 CRn op2 CRm",
+	},
+
 	COMMAND_REGISTRATION_DONE
 };
 const struct command_registration arm_command_handlers[] = {
@@ -1252,6 +1397,24 @@ static int arm_full_context(struct target *target)
 	return retval;
 }
 
+static int arm_default_mrc(struct target *target, int cpnum,
+		uint32_t op1, uint32_t op2,
+		uint32_t CRn, uint32_t CRm,
+		uint32_t *value)
+{
+	LOG_ERROR("%s doesn't implement MRC", target_type_name(target));
+	return ERROR_FAIL;
+}
+
+static int arm_default_mcr(struct target *target, int cpnum,
+		uint32_t op1, uint32_t op2,
+		uint32_t CRn, uint32_t CRm,
+		uint32_t value)
+{
+	LOG_ERROR("%s doesn't implement MCR", target_type_name(target));
+	return ERROR_FAIL;
+}
+
 int armv4_5_init_arch_info(struct target *target, struct arm *armv4_5)
 {
 	target->arch_info = armv4_5;
@@ -1267,5 +1430,10 @@ int armv4_5_init_arch_info(struct target *target, struct arm *armv4_5)
 	if (!armv4_5->full_context && armv4_5->read_core_reg)
 		armv4_5->full_context = arm_full_context;
 
+	if (!armv4_5->mrc)
+		armv4_5->mrc = arm_default_mrc;
+	if (!armv4_5->mcr)
+		armv4_5->mcr = arm_default_mcr;
+
 	return ERROR_OK;
 }

src/target/armv4_5.h

@@ -112,11 +112,26 @@ struct arm
 	/** Handle for the Embedded Trace Module, if one is present. */
 	struct etm_context *etm;
 
+	/* FIXME all these methods should take "struct arm *" not target */
+
 	int (*full_context)(struct target *target);
 	int (*read_core_reg)(struct target *target, struct reg *reg,
 			int num, enum armv4_5_mode mode);
 	int (*write_core_reg)(struct target *target, struct reg *reg,
 			int num, enum armv4_5_mode mode, uint32_t value);
+
+	/** Read coprocessor register.  */
+	int (*mrc)(struct target *target, int cpnum,
+			uint32_t op1, uint32_t op2,
+			uint32_t CRn, uint32_t CRm,
+			uint32_t *value);
+
+	/* Write coprocessor register.  */
+	int (*mcr)(struct target *target, int cpnum,
+			uint32_t op1, uint32_t op2,
+			uint32_t CRn, uint32_t CRm,
+			uint32_t value);
+
 	void *arch_info;
 };
 

src/target/cortex_a8.c

@@ -936,7 +936,7 @@ static void cortex_a8_post_debug_entry(struct target *target)
 	int retval;
 
 	/* MRC p15,0,<Rt>,c1,c0,0 ; Read CP15 System Control Register */
-	retval = target->type->mrc(target, 15,
+	retval = armv7a->armv4_5_common.mrc(target, 15,
 			0, 0,	/* op1, op2 */
 			1, 0,	/* CRn, CRm */
 			&cortex_a8->cp15_control_reg);
@@ -947,7 +947,7 @@ static void cortex_a8_post_debug_entry(struct target *target)
 		uint32_t cache_type_reg;
 
 		/* MRC p15,0,<Rt>,c0,c0,1 ; Read CP15 Cache Type Register */
-		retval = target->type->mrc(target, 15,
+		retval = armv7a->armv4_5_common.mrc(target, 15,
 				0, 1,	/* op1, op2 */
 				0, 0,	/* CRn, CRm */
 				&cache_type_reg);
@@ -1535,6 +1535,7 @@ static int cortex_a8_examine_first(struct target *target)
 	LOG_DEBUG("ttypr = 0x%08" PRIx32, ttypr);
 	LOG_DEBUG("didr = 0x%08" PRIx32, didr);
 
+	armv7a->armv4_5_common.core_type = ARM_MODE_MON;
 	cortex_a8_dpm_setup(cortex_a8, didr);
 
 	/* Setup Breakpoint Register Pairs */
@@ -1611,6 +1612,9 @@ static int cortex_a8_init_arch_info(struct target *target,
 	cortex_a8->common_magic = CORTEX_A8_COMMON_MAGIC;
 	armv4_5->arch_info = armv7a;
 
+	armv4_5->mrc = cortex_a8_mrc,
+	armv4_5->mcr = cortex_a8_mcr,
+
 	/* prepare JTAG information for the new target */
 	cortex_a8->jtag_info.tap = tap;
 	cortex_a8->jtag_info.scann_size = 4;
@@ -1626,7 +1630,6 @@ static int cortex_a8_init_arch_info(struct target *target,
 
 	cortex_a8->fast_reg_read = 0;
 
-
 	/* register arch-specific functions */
 	armv7a->examine_debug_reason = NULL;
 
@@ -1752,6 +1755,4 @@ struct target_type cortexa8_target = {
 	.target_create = cortex_a8_target_create,
 	.init_target = cortex_a8_init_target,
 	.examine = cortex_a8_examine,
-	.mrc = cortex_a8_mrc,
-	.mcr = cortex_a8_mcr,
 };

src/target/target.c

@@ -44,8 +44,6 @@
 #include "jtag.h"
 
 
-static int jim_mcrmrc(Jim_Interp *interp, int argc, Jim_Obj *const *argv);
-
 static int target_array2mem(Jim_Interp *interp, struct target *target, int argc, Jim_Obj *const *argv);
 static int target_mem2array(Jim_Interp *interp, struct target *target, int argc, Jim_Obj *const *argv);
 
@@ -665,84 +663,6 @@ static void target_reset_examined(struct target *target)
 	target->examined = false;
 }
 
-
-
-static int default_mrc(struct target *target, int cpnum, uint32_t op1, uint32_t op2, uint32_t CRn, uint32_t CRm, uint32_t *value)
-{
-	LOG_ERROR("Not implemented: %s", __func__);
-	return ERROR_FAIL;
-}
-
-static int default_mcr(struct target *target, int cpnum, uint32_t op1, uint32_t op2, uint32_t CRn, uint32_t CRm, uint32_t value)
-{
-	LOG_ERROR("Not implemented: %s", __func__);
-	return ERROR_FAIL;
-}
-
-static int arm_cp_check(struct target *target, int cpnum, uint32_t op1, uint32_t op2, uint32_t CRn, uint32_t CRm)
-{
-	/* basic check */
-	if (!target_was_examined(target))
-	{
-		LOG_ERROR("Target not examined yet");
-		return ERROR_FAIL;
-	}
-
-	if ((cpnum <0) || (cpnum > 15))
-	{
-		LOG_ERROR("Illegal co-processor %d", cpnum);
-		return ERROR_FAIL;
-	}
-
-	if (op1 > 7)
-	{
-		LOG_ERROR("Illegal op1");
-		return ERROR_FAIL;
-	}
-
-	if (op2 > 7)
-	{
-		LOG_ERROR("Illegal op2");
-		return ERROR_FAIL;
-	}
-
-	if (CRn > 15)
-	{
-		LOG_ERROR("Illegal CRn");
-		return ERROR_FAIL;
-	}
-
-	if (CRm > 15)
-	{
-		LOG_ERROR("Illegal CRm");
-		return ERROR_FAIL;
-	}
-
-	return ERROR_OK;
-}
-
-int target_mrc(struct target *target, int cpnum, uint32_t op1, uint32_t op2, uint32_t CRn, uint32_t CRm, uint32_t *value)
-{
-	int retval;
-
-	retval = arm_cp_check(target, cpnum, op1, op2, CRn, CRm);
-	if (retval != ERROR_OK)
-		return retval;
-
-	return target->type->mrc(target, cpnum, op1, op2, CRn, CRm, value);
-}
-
-int target_mcr(struct target *target, int cpnum, uint32_t op1, uint32_t op2, uint32_t CRn, uint32_t CRm, uint32_t value)
-{
-	int retval;
-
-	retval = arm_cp_check(target, cpnum, op1, op2, CRn, CRm);
-	if (retval != ERROR_OK)
-		return retval;
-
-	return target->type->mcr(target, cpnum, op1, op2, CRn, CRm, value);
-}
-
 static int
 err_read_phys_memory(struct target *target, uint32_t address,
 		uint32_t size, uint32_t count, uint8_t *buffer)
@@ -781,39 +701,6 @@ int target_init(struct command_context *cmd_ctx)
 			return retval;
 		}
 
-		/**
-		 * @todo MCR/MRC are ARM-specific; don't require them in
-		 * all targets, or for ARMs without coprocessors.
-		 */
-		if (target->type->mcr == NULL)
-		{
-			target->type->mcr = default_mcr;
-		} else
-		{
-			const struct command_registration mcr_cmd = {
-				.name = "mcr",
-				.mode = COMMAND_EXEC,
-				.jim_handler = &jim_mcrmrc,
-				.help = "write coprocessor",
-				.usage = "<cpnum> <op1> <op2> <CRn> <CRm> <value>",
-			};
-			register_command(cmd_ctx, NULL, &mcr_cmd);
-		}
-
-		if (target->type->mrc == NULL)
-		{
-			target->type->mrc = default_mrc;
-		} else
-		{
-			const struct command_registration mrc_cmd = {
-				.name = "mrc",
-				.jim_handler = &jim_mcrmrc,
-				.help = "read coprocessor",
-				.usage = "<cpnum> <op1> <op2> <CRn> <CRm>",
-			};
-			register_command(cmd_ctx, NULL, &mrc_cmd);
-		}
-
 
 		/**
 		 * @todo get rid of those *memory_imp() methods, now that all
@@ -4883,92 +4770,6 @@ COMMAND_HANDLER(handle_fast_load_command)
 	return retval;
 }
 
-static int jim_mcrmrc(Jim_Interp *interp, int argc, Jim_Obj *const *argv)
-{
-	struct command_context *context;
-	struct target *target;
-	int retval;
-
-	context = Jim_GetAssocData(interp, "context");
-	if (context == NULL) {
-		LOG_ERROR("array2mem: no command context");
-		return JIM_ERR;
-	}
-	target = get_current_target(context);
-	if (target == NULL) {
-		LOG_ERROR("array2mem: no current target");
-		return JIM_ERR;
-	}
-
-	if ((argc < 6) || (argc > 7))
-	{
-		return JIM_ERR;
-	}
-
-	int cpnum;
-	uint32_t op1;
-	uint32_t op2;
-	uint32_t CRn;
-	uint32_t CRm;
-	uint32_t value;
-
-	int e;
-	long l;
-	e = Jim_GetLong(interp, argv[1], &l);
-	if (e != JIM_OK) {
-		return e;
-	}
-	cpnum = l;
-
-	e = Jim_GetLong(interp, argv[2], &l);
-	if (e != JIM_OK) {
-		return e;
-	}
-	op1 = l;
-
-	e = Jim_GetLong(interp, argv[3], &l);
-	if (e != JIM_OK) {
-		return e;
-	}
-	CRn = l;
-
-	e = Jim_GetLong(interp, argv[4], &l);
-	if (e != JIM_OK) {
-		return e;
-	}
-	CRm = l;
-
-	e = Jim_GetLong(interp, argv[5], &l);
-	if (e != JIM_OK) {
-		return e;
-	}
-	op2 = l;
-
-	value = 0;
-
-	if (argc == 7)
-	{
-		e = Jim_GetLong(interp, argv[6], &l);
-		if (e != JIM_OK) {
-			return e;
-		}
-		value = l;
-
-		retval = target_mcr(target, cpnum, op1, op2, CRn, CRm, value);
-		if (retval != ERROR_OK)
-			return JIM_ERR;
-	} else
-	{
-		retval = target_mrc(target, cpnum, op1, op2, CRn, CRm, &value);
-		if (retval != ERROR_OK)
-			return JIM_ERR;
-
-		Jim_SetResult(interp, Jim_NewIntObj(interp, value));
-	}
-
-	return JIM_OK;
-}
-
 static const struct command_registration target_command_handlers[] = {
 	{
 		.name = "targets",

src/target/target_type.h

@@ -213,11 +213,6 @@ struct target_type
 
 	int (*mmu)(struct target *target, int *enabled);
 
-	/* Read coprocessor - arm specific. Default implementation returns error. */
-	int (*mrc)(struct target *target, int cpnum, uint32_t op1, uint32_t op2, uint32_t CRn, uint32_t CRm, uint32_t *value);
-
-	/* Write coprocessor. Default implementation returns error.  */
-	int (*mcr)(struct target *target, int cpnum, uint32_t op1, uint32_t op2, uint32_t CRn, uint32_t CRm, uint32_t value);
 };
 
 #endif // TARGET_TYPE_H