On 11/20/2012 05:32 PM, Will Deacon wrote: > Hi Gregory, > > Thanks for turning this around so quickly! I still have a few comments on > your assembly, but you've got the right idea:
I was a little too quick I think: I have just passed a few hours to fix a bug which appeared when I have added the HWIOCC patches. But I see that you have already found it! > > On Mon, Nov 19, 2012 at 08:35:55PM +0000, Gregory CLEMENT wrote: >> From: Yehuda Yitschak <[email protected]> >> >> Signed-off-by: Yehuda Yitschak <[email protected]> >> Signed-off-by: Gregory CLEMENT <[email protected]> > > [...] > >> +/* Function defined in coherncy_ll.S */ >> +extern void ll_set_cpu_coherent(void __iomem *base_addr, >> + unsigned int hw_cpu_id); >> + >> +int set_cpu_coherent(unsigned int hw_cpu_id, int smp_group_id) >> +{ >> + if (!coherency_base) { >> + pr_warn("Can't make CPU %d cache coherent.\n", hw_cpu_id); >> + pr_warn("Coherency fabric is not initialized\n"); >> + return 1; >> + } >> + ll_set_cpu_coherent(coherency_base, hw_cpu_id); >> + return 0; >> +} > > Yup, something like this is neater I reckon. You could even make > ll_set_cpu_coherent return 0 if you wanted, but it's up to you. > >> +#include <linux/linkage.h> >> +#define ARMADA_XP_CFB_CTL_REG_OFFSET 0x0 >> +#define ARMADA_XP_CFB_CFG_REG_OFFSET 0x4 >> + >> + .text >> +/* >> + * r0: CFB base adresse register > > address > >> + * r1: HW CPU id >> + */ >> +ENTRY(ll_set_cpu_coherent) >> + /* Create bit by cpu index */ >> + add r1,r1,#24 >> + mov r3, #1 > > Can you instead mov r3, #(1 << 24) and get rid of these two instructions? I guess > >> + lsl r1, r3, r1 >> + >> + >> + /* Add CPU to SMP group - Atomic */ >> + orr r0, r0, #ARMADA_XP_CFB_CTL_REG_OFFSET > > An add might be clearer here, despite the address alignment. Yes I have change it already, and I don't change r0 any more as it is the base register: I was lucky that ARMADA_XP_CFB_CFG_REG_OFFSET was equal to 0! > >> + ldr r4, [r0] >> + orr r4 , r4, r1 >> + str r4,[r0] > > You haven't saved r4, so you can't use it here. It looks like you have r2 > spare -- why not use that instead? True! And it was a bug in fact. When I read the datasheet on PCS I stuck on the expression register 4, which is r3 and not r4! :( Now it's fixed and of course now I use r2 > >> + /* Enable coherency on CPU - Atomic*/ >> + orr r0, r0, #ARMADA_XP_CFB_CFG_REG_OFFSET > > add yes I have already fixed it too. > >> + ldr r4, [r0] >> + orr r4 , r4, r1 >> + str r4,[r0] > > mov r0, #0 here if you want to return 0. Well, why not. > >> + mov pc, lr >> +ENDPROC(ll_set_cpu_coherent) > > Will > > _______________________________________________ > linux-arm-kernel mailing list > [email protected] > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com _______________________________________________ devicetree-discuss mailing list [email protected] https://lists.ozlabs.org/listinfo/devicetree-discuss
