Re: [PATCH 1/4] ARM: rcm-k1879xb1: Add support for K1879XB1 SoC

[Andrew]

Russell King - ARM Linux ÐÐÑÐÐ 23.06.2015 19:11:
> On Tue, Jun 23, 2015 at 06:50:01PM +0300, Andrew Andrianov wrote:
> > +config ARCH_RCM_K1879XB1
> > +	bool "RC Module K1879XB1YA"
> > +	depends on MMU
> > +	select CPU_V6
> > +        select ARM_AMBA
> > +	select ARM_VIC
>
> Obviously something's up with the indentation on the ARM_AMBA line...
> Also, these select statements should be sorted alphabetically to help
> avoid future merge conflicts.
>
> > @@ -1417,6 +1418,7 @@ config DEBUG_UART_PHYS
> >  	default 0xfffb9800 if DEBUG_OMAP1UART3 || DEBUG_OMAP7XXUART3
> >  	default 0xfffe8600 if DEBUG_UART_BCM63XX
> >  	default 0xfffff700 if ARCH_IOP33X
> > +	default 0x2002b000 if ARCH_RCM_K1879XB1
> >  	depends on ARCH_EP93XX || \
> >  	        DEBUG_LL_UART_8250 || DEBUG_LL_UART_PL01X || \
> >  		DEBUG_LL_UART_EFM32 || \
> > @@ -1511,6 +1513,7 @@ config DEBUG_UART_VIRT
> >  	default 0xfefff700 if ARCH_IOP33X
> >  	default 0xff003000 if DEBUG_U300_UART
> >  	default 0xffd01000 if DEBUG_HIP01_UART
> > +	default 0xf802b000 if ARCH_RCM_K1879XB1
>
> Both of these lists have an order - please add your entries in order as
> well.
>
> >  	default DEBUG_UART_PHYS if !MMU
> >  	depends on DEBUG_LL_UART_8250 || DEBUG_LL_UART_PL01X || \
> >  		DEBUG_UART_8250 || DEBUG_UART_PL01X || DEBUG_MESON_UARTAO || \
> > diff --git a/arch/arm/Makefile b/arch/arm/Makefile
> > index 985227c..9beb65f 100644
> > --- a/arch/arm/Makefile
> > +++ b/arch/arm/Makefile
> > @@ -184,6 +184,7 @@ machine-$(CONFIG_ARCH_OMAP2PLUS)	+= omap2
> >  machine-$(CONFIG_ARCH_ORION5X)		+= orion5x
> >  machine-$(CONFIG_ARCH_PICOXCELL)	+= picoxcell
> >  machine-$(CONFIG_ARCH_PXA)		+= pxa
> > +machine-$(CONFIG_ARCH_RCM_K1879XB1)	+= rcm-k1879xb1
> >  machine-$(CONFIG_ARCH_QCOM)		+= qcom
>
> R doesn't come between P and Q, the order of these options is even
> documented in the commentry above the list (so here we have more
> proof that comments are worthless. :()

Oops, I must've mixed it up during re-bases and it went unnoticed.

> > +static struct map_desc k1879_io_desc[] __initdata = {
> > +	{
> > +		.virtual	= RCM_K1879_AREA0_VIRT_BASE,
> > +		.pfn		= __phys_to_pfn(RCM_K1879_AREA0_PHYS_BASE),
> > +		.length		= RCM_K1879_AREA0_SIZE,
> > +		.type		= MT_DEVICE,
> > +	},
> > +	{
> > +		.virtual	= RCM_K1879_AREA1_VIRT_BASE,
> > +		.pfn		= __phys_to_pfn(RCM_K1879_AREA1_PHYS_BASE),
> > +		.length		= RCM_K1879_AREA1_SIZE,
> > +		.type		= MT_DEVICE,
> > +	},
> > +};
>
> We've moved away from static mapping - is there a reason to keep these?
>
> > +static void __iomem *k1879_mif_base(void)
> > +{
> > +	BUG_ON(!g_k1879_mif);
> > +	return g_k1879_mif;
> > +}
> > +
> > +static void __iomem *k1879_sctl_base(void)
> > +{
> > +	return (void __iomem *)RCM_K1879_SCTL_VIRT_BASE;
> > +}
> > +
> > +static void k1879_level_irq_i2c0_fixup(unsigned int irq, struct 
> > irq_desc *desc)
> > +{
> > +	writel(1, k1879_mif_base() + RCM_K1879_MIF_I2C_INT_STAT);
> > +	handle_level_irq(irq, desc);
> > +}
> > +
> > +static void k1879_level_irq_i2c1_fixup(unsigned int irq, struct 
> > irq_desc *desc)
> > +{
> > +	writel(1 << 0, k1879_sctl_base() + RCM_K1879_SCTL_INT_P_OUT);
> > +	handle_level_irq(irq, desc);
> > +}
> > +
> > +static void k1879_level_irq_i2c2_fixup(unsigned int irq, struct 
> > irq_desc *desc)
> > +{
> > +	writel(1 << 1, k1879_sctl_base() + RCM_K1879_SCTL_INT_P_OUT);
> > +	handle_level_irq(irq, desc);
> > +}
> > +
> > +static void k1879_level_irq_i2c3_fixup(unsigned int irq, struct 
> > irq_desc *desc)
> > +{
> > +	writel(1 << 2, k1879_sctl_base() + RCM_K1879_SCTL_INT_P_OUT);
> > +	handle_level_irq(irq, desc);
> > +}
>
> I'm not sure this is the best way to deal with this - but I need to 
> look
> at the irqchip stuff to suggest an alternative.  Maybe you could 
> provide
> a description of what's going on here first?

As far as I remember that's a workaround. The problem here is that 
Aeroflex
Gaisler i2c has impulse interrupts and that had some hw problems 
connecting
with VIC.
Therefore hardware folks have made it level-triggered, and you have to 
clear
'pending' bit in SCTL register each time the interrupt arrives.

This is very platform-specific, so I don't think hacking i2c driver 
itself would be any
better.

> > +void k1879_restart(enum reboot_mode mode, const char *cmd)
>
> static?
>
> > +{
> > +	/* The recommended way to do a soft-reboot on this platform
> > +	   is write directly to watchdog registers and cause an immediate
> > +	   system reboot
> > +	*/
> > +	void __iomem *regs;
> > +
> > +	pr_info("k1879: Requested system restart\n");
>
> I assume this is debugging - do you need to print this?
>
> > +	regs = ioremap_nocache(0x20025000, 0xfff);
>
> Please use ioremap() instead.  Both of these calls may sleep, and so
> should not be used in this path (which can be called from atomic
> contexts.)  Hence, this needs to happen elsewhere (in your 
> .init_machine
> method.)

Thanks for the quick response, I'll fix all the things you've noted and 
resend
the patches tomorrow.

--
Regards,
Andrew
RC Module :: http://module.ru
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/