[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5422120712767fee47dcca066bde7962@mail.ncrmnt.org>
Date: Tue, 23 Jun 2015 19:56:04 +0300
From: Andrew <andrew@...mnt.org>
To: Russell King - ARM Linux <linux@....linux.org.uk>
Cc: Pawel Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>,
Arnd Bergmann <arnd@...db.de>,
Rob Herring <robh+dt@...nel.org>,
Pavel Shevchenko <pshevch@...ule.ru>,
Andrew Andrianov <andrianov@...ule.ru>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] ARM: rcm-k1879xb1: Add support for K1879XB1 SoC
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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists