Hi Mark,

Here you open code an MFCR instruction.

I guess MFCR stands for something like move-from-control-register, and MTCR
stands for move-to-control-register?

I see that later on you have helpers for specific registers, e.g. mfcr_cpuidrr().

You might want to follow the example of arm64's read_sysreg() and
write_sysreg(), and have general purpose helpers for thos instructions, e.g.

#define mfcr(reg)						\
({								\
	unsigned long __mfcr_val;				\
	asm volatile("mfcr %0, " #reg "\n" : "=r" (__mfr_val));	\
	__mfcr_val;						\
})

... which avoids needing helpers for each register, as you can do:

ss1_val = mfcr(ss1);
cpuidrr_val = mfcr(cpuidrr);

OK, good idea.

quoted

+	if (memblock_start_of_DRAM() != (PHYS_OFFSET + CONFIG_RAM_BASE)) {
+		pr_err("C-SKY: dts-DRAM doesn't fit .config: %x-%x.\n",
+			memblock_start_of_DRAM(),
+			PHYS_OFFSET + CONFIG_RAM_BASE);
+		return;
+	}

If this is a problem, is it safe to continue at all?

Why does the base address of RAM matter?

We use mips-like direct-mapping tech, and it need 512MB boundary
alignment. And few users need non-512MB boundary phy-addr start,
so we give the CONFIG_RAM_BASE for determine the offset to PHYS_OFFSET.

quoted

+
+	mtcr_msa0(PHYS_OFFSET | 0xe);
+	mtcr_msa1(PHYS_OFFSET | 0x6);

As with MFCR, you could use a generic helper here, e.g.

#define mtcr(val, reg)								\
do {										\
	asm volatile("mtcr %0, " #reg "\n" : "=r" ((unsigned long)val));	\
} while (0);

mtcr(PHYS_OFFSET | 0xe, msa0)
mtcr(PHYS_OFFSET | 0x6, msa1)

OK

quoted

+	seq_printf(m, "C-SKY CPU : %s\n", CSKYCPU_DEF_NAME);
+	seq_printf(m, "revision  : 0x%08x\n", mfcr_cpuidrr());
+	seq_printf(m, "ccr reg   : 0x%08x\n", mfcr_ccr());
+	seq_printf(m, "ccr2 reg  : 0x%08x\n", mfcr_ccr2());
+	seq_printf(m, "hint reg  : 0x%08x\n", mfcr_hint());
+	seq_printf(m, "msa0 reg  : 0x%08x\n", mfcr_msa0());
+	seq_printf(m, "msa1 reg  : 0x%08x\n", mfcr_msa1());

Do these need to be exposed to userspace?

Yes, I'll add more explain info.

Does this arch support SMP? I see you don't log information per-cpu.

This patch-set doesn't support SMP.

quoted

diff --git a/arch/csky/kernel/entry.S b/arch/csky/kernel/entry.S

quoted

+#define THREADSIZE_MASK_BIT 13

You might want to define this as THREAD_SHIFT, and define THREAD_SIZE in terms
of it, so that they're guaranteed to be in sync

e.g. in your <asm/thread_info.h> have:

#define THREAD_SHIFT	13
#define THREAD_SIZE	(1 << THREAD_SHIFT)

OK

If you have a spare register that you can point at the current task (or you
have preemption-safe percpu ops), I'd recommend moving the thread_info off of
the stack, and implementing THREAD_INFO_IN_TASK_STRUCT.

Em... I'll think about it.

For consistency, and in case you change your stack size in future, this should
be THREADSIZE_MASK_BIT.

OK

quoted

+        if (unlikely(address >= VMALLOC_START && address <= VMALLOC_END))
+                goto vmalloc_fault;

You might want to check if this was a user mode fault here, so that users can't trigger vmalloc faults.

Is it necessary to check user mode? If a user-process touch a
kernel-addr, it will cause a supervisor exception.

Best Regards
  Guo Ren