| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CA+5PVA4wpGfSXbX9oTdtDaGiWFkwZfiL9Md6SGJpXtOK6rG2ww@mail.gmail.com>
Date: Wed, 23 Nov 2011 09:10:53 -0500
From: Josh Boyer <jwboyer@...il.com>
To: Tanmay Inamdar <tinamdar@....com>
Cc: linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] powerpc/40x: Add APM8018X SOC support
On Wed, Nov 23, 2011 at 4:44 AM, Tanmay Inamdar <tinamdar@....com> wrote:
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index b177caa..3f2cc36 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -978,3 +978,9 @@ config PPC_LIB_RHEAP
> bool
>
> source "arch/powerpc/kvm/Kconfig"
> +
> +config UART_16550_WORD_ADDRESSABLE
> + bool
> + default n
> + help
> + Enable this if your UART 16550 is word addressable.
Ugh. What is this for? More specifically, if the UART requires word
reads and writes, shouldn't it be using reg-shift/reg-offset in the
device tree? I'm confused why UDBG would need this sort of code, but
the runtime serial driver doesn't?
> diff --git a/arch/powerpc/boot/dts/klondike.dts b/arch/powerpc/boot/dts/klondike.dts
> + OCM: ocm@...00000 {
> + compatible = "ibm,ocm";
> + status = "enabled";
> + cell-index = <1>;
> + reg = < 0x20000000 0x1f000 /* 128K - 4K NAND */
> + 0xfffe0000 0x1f000>; /* 128K - 4K I2C */
> + bootmode = "nand";
> + };
What is this? There's nothing in the kernel or in this patch that
binds with "ibm,ocm". Also, that 'bootmode' property doesn't seem
like a hardware value, but a human descriptor of something that
switches it to boot from NAND.
> + crypto: crypto@...00000 {
> + device_type = "crypto";
> + compatible = "405ex-crypto", "amcc,ppc405ex-crypto", "amcc,ppc4xx-crypto";
Why is the "405ex-crypto" string there?
> + EDMA: edma@...80000 {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "amcc,edma";
> + device_type = "dma";
> + /*complQ-fifo-memory = "ocm";*/
> + cell-index = <0>;
> + reg = <0x40080000 0x00010000>;
> + dcr-reg = <0x060 0x09f>;
> +
> + interrupt-parent = <&UIC0>;
> + interrupts = </*complQ A */ 0x4 4
> + /*EDMA Err */ 0x6 4 >;
> +
> + dma-channel@0 {
> + compatible = "amcc,edma-channel";
> + /*descriptor-memory = "ocm";*/
> + cell-index = <0>;
> + dcr-reg = <0x0000006a 0x0000006b>;
> + };
> + };
What's this? Again, nothing binds to "amcc,edma" in the kernel or
patch. At the very least this (and OCM above) need some binding
descriptions added to Documentation/devicetree/bindings/powerpc/4xx/
> +
> + MSI: dwc_pcie-msi@...90000 {
> + compatible = "amcc,dwc_pcie-msi", "dwc_pcie-msi";
> + status = "ok";
Is the status property something that is set by U-Boot, or will it
always be "ok"? If the latter, I don't think you need to specify it
at all.
> + reg = <0x40090000 0x100>;
> + interrupts =<0x0 0x1 0x2 0x3>;
> + interrupt-parent = <&MSI>;
> + #interrupt-cells = <1>;
> + #address-cells = <0>;
> + #size-cells = <0>;
> + interrupt-map = <0x0 &UIC0 0x0C 0x1
> + 0x1 &UIC0 0x0D 0x1
> + 0x2 &UIC0 0x0E 0x1
> + 0x3 &UIC0 0x0F 0x1>;
> + };
Same binding comment here.
> +
> + AHB: ahb {
> + device_type = "ahb";
I doubt the device_type is needed.
> + compatible = "amcc,405ex-ahb", "ibm,ahb";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> + clock-frequency = <0>; /* Filled in by U-Boot */
Same binding comment for ahb.
> +
> + lcd: lcd@...60000 {
> + device_type = "lcd";
Drop device_type.
> + compatible = "apm,apm-lcd","apm,db9000";
> + version = "apm-1.1";
Why is 'version' there? Version of what? The hardware itself, the
driver? I doubt this is needed.
> + reg = <0x58060000 0x00001000>;
> + interrupt-parent = <&UIC0>;
> + /*
> + * interrupt index 0 for chip 1.0
> + * interrupt index 1 for chip 1.1
> + */
> + interrupts = <0x1c 2 0x1c 4>;
> + };
Same binding comment for apm,apm-lcd. I'm just going to assert again
that anything that doesn't have a defined binding and/or driver needs
to be documented when it's introduced. Repeat that for the rest of
the patch :).
> +
> + sdhc0: sdhc@...50000 {
> + device_type = "sdhc";
Drop device_type.
> + compatible = "amcc,ahb-sdhc";
> + #interrupt-cells = <1>;
> + reg = <0x58050000 0x100>;
> + interrupt-parent = <&UIC0>;
> + interrupts = <0x18 0x4>;
> + bootmode = "i2c";
> + };
> +
> + tdm0: tdm@...10000 {
> + device_type = "tdm";
Drop device_type.
> + status = "disabled";
Is that set by U-Boot?
> + compatible = "apm,ahb-tdm";
> + #interrupt-cells = <1>;
> + reg = <0x58010000 0x100>;
> + interrupt-parent = <&UIC1>;
> + interrupts = <0x15 0x1>;
> + };
> +
> + usbotg0: usbotg@...80000 {
> + device_type = "usb";
Drop device_type.
> + compatible = "apm,usb-otg";
> + reg = <0x58080000 0x10000>;
> + interrupt-parent = <&UIC0>;
> + interrupts = <0x1B 4>;
> + mode = "host";
> + };
> + spi0: spi@...00000 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + device_type = "spi";
Drop device_type. I think you're starting to get the trend, so repeat
for the rest of the devices.
> + compatible = "apm,apm-spi";
> + reg = <0x50000000 0x100>;
> + interrupt-parent = <&UIC0>;
> + interrupts = <0x7 1>; /* interrupt number->0x7 Polarity->HIGH Sensitivity->LEVEL */
> + half_duplex = <0x1>; /*0 = rx/tx mode, 1 = eprom read mode */
> + sysclk = <100000000>; /* input clock */
> + bus_num = <0x0>; /* SPI = 0 */
> +
> + PCIE0: pciex@...20000 {
> + device_type = "pci";
> + compatible = "ibm,plb-pciex", "dwc-pciex", "amcc,dwc-pciex";
Why the unprefixed dwc-pciex compatible property?
> + #interrupt-cells = <1>;
> + #size-cells = <2>;
> + #address-cells = <3>;
> + primary;
> + port = <0>; /* port number */
> + status = "ok";
> + PCIE1: pciex@...30000 {
> + device_type = "pci";
> + compatible = "ibm,plb-pciex", "dwc-pciex", "amcc,dwc-pciex";
> + #interrupt-cells = <1>;
> + #size-cells = <2>;
> + #address-cells = <3>;
> + primary;
> + port = <1>; /* port number */
> + status = "disabled";
Is this set by U-Boot?
> + sata@...40000 {
> + compatible = "sata-ahci";
Uh.. what?
> + reg = <0x58040000 0x2000>;
> + interrupt-parent = <&UIC0>;
> + interrupts = <0x1a 1>;
> + ufc@...E000000 {
> + compatible = "ibm,ufc";
> + reg = <0xFE000000 0x00010000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + bootmode = "nand";
Is UFC some kind of new flash controller that isn't NDFC? Also,
bootmode seems to be a human and/or driver variable, not a description
of the hardware.
> + UART0: serial@...01000 {
> + device_type = "serial";
> + compatible = "ns16550";
> + reg = <0x50001000 0x00000100>;
> + virtual-reg = <0x50001000>;
> + clock-frequency = <300000000>; /* Filled in by U-Boot */
> + current-speed = <115200>;
> + interrupt-parent = <&UIC0>;
> + interrupts = <0x0 0x4>;
> + /*reg-shift = <2>;*/
This is commented out, but seems to be needed when you take the
word-addressed UDBG thing into account?
> + IEEE1588_0: ieee1588ts0@...a5000 {
> + status = "ok";
> + compatible = "ieee1588-ts";
What is that?
> diff --git a/arch/powerpc/configs/40x/klondike_defconfig b/arch/powerpc/configs/40x/klondike_defconfig
> new file mode 100644
> index 0000000..840f438
> --- /dev/null
> +++ b/arch/powerpc/configs/40x/klondike_defconfig
> @@ -0,0 +1,1353 @@
> +#
> +# Automatically generated file; DO NOT EDIT.
> +# Linux/powerpc 3.2.0-rc2 Kernel Configuration
> +#
> +# CONFIG_PPC64 is not set
This is a full defconfig. We don't need a full config file. You can
generate one with 'make savedefconfig' that contains only the options
you need to set.
> diff --git a/arch/powerpc/kernel/udbg_16550.c b/arch/powerpc/kernel/udbg_16550.c
> index 6837f83..dd3bce9 100644
> --- a/arch/powerpc/kernel/udbg_16550.c
> +++ b/arch/powerpc/kernel/udbg_16550.c
> @@ -18,6 +18,19 @@ extern void real_writeb(u8 data, volatile u8 __iomem *addr);
> extern u8 real_205_readb(volatile u8 __iomem *addr);
> extern void real_205_writeb(u8 data, volatile u8 __iomem *addr);
>
> +#ifdef CONFIG_UART_16550_WORD_ADDRESSABLE
> +struct NS16550 {
> + /* this struct must be packed */
> + unsigned char rbr; /* 0 */ u8 s0[3];
An array of length 3 for something "word-addressable"? When did words
change to 3 bytes? Now, I still haven't finished my coffee yet, but
that is really confusing.
> + unsigned char ier; /* 1 */ u8 s1[3];
> + unsigned char fcr; /* 2 */ u8 s2[3];
> + unsigned char lcr; /* 3 */ u8 s3[3];
> + unsigned char mcr; /* 4 */ u8 s4[3];
> + unsigned char lsr; /* 5 */ u8 s5[3];
> + unsigned char msr; /* 6 */ u8 s6[3];
> + unsigned char scr; /* 7 */ u8 s7[3];
> +};
> +#else
> struct NS16550 {
> /* this struct must be packed */
> unsigned char rbr; /* 0 */
> @@ -29,6 +42,7 @@ struct NS16550 {
> unsigned char msr; /* 6 */
> unsigned char scr; /* 7 */
> };
> +#endif /* CONFIG_UART_16550_WORD_ADDRESSABLE */
>
> #define thr rbr
> #define iir fcr
> @@ -52,8 +66,16 @@ static struct NS16550 __iomem *udbg_comport;
> static void udbg_550_flush(void)
> {
> if (udbg_comport) {
> +#if defined(CONFIG_APM8018X)
> + int index;
> + for (index = 0; index < 3500; index++) {
> + if ((in_8(&udbg_comport->lsr) & LSR_THRE) == LSR_THRE)
> + break;
> + }
> +#else
What is index, and why do you read the same register 3500 times? That
doesn't sound like an index, it sounds like some kind of poor-mans
timeout.
> while ((in_8(&udbg_comport->lsr) & LSR_THRE) == 0)
> /* wait for idle */;
> +#endif /* CONFIG_APM8018X */
> }
> }
>
> diff --git a/arch/powerpc/platforms/40x/Kconfig b/arch/powerpc/platforms/40x/Kconfig
> index 1530229..3d0d1d9 100644
> --- a/arch/powerpc/platforms/40x/Kconfig
> +++ b/arch/powerpc/platforms/40x/Kconfig
> @@ -186,3 +186,14 @@ config IBM405_ERR51
> # bool
> # depends on !STB03xxx && PPC4xx_DMA
> # default y
> +#
> +
> +config APM8018X
> + bool "APM8018X"
> + depends on 40x
> + default y
default n please.
> + select PPC40x_SIMPLE
> + select UART_16550_WORD_ADDRESSABLE
> + help
> + This option enables support for the AppliedMicro Klondike board.
> +
> diff --git a/arch/powerpc/platforms/40x/ppc40x_simple.c b/arch/powerpc/platforms/40x/ppc40x_simple.c
> index e8dd5c5..c8576af 100644
> --- a/arch/powerpc/platforms/40x/ppc40x_simple.c
> +++ b/arch/powerpc/platforms/40x/ppc40x_simple.c
> @@ -17,7 +17,7 @@
> #include <asm/pci-bridge.h>
> #include <asm/ppc4xx.h>
> #include <asm/prom.h>
> -#include <asm/time.h>
> +#include <linux/time.h>
Is this needed for a reason? If so, it should be submitted as a separate patch.
> #include <asm/udbg.h>
> #include <asm/uic.h>
>
> @@ -29,6 +29,7 @@ static __initdata struct of_device_id ppc40x_of_bus[] = {
> { .compatible = "ibm,plb4", },
> { .compatible = "ibm,opb", },
> { .compatible = "ibm,ebc", },
> + { .compatible = "ibm,ahb", },
> { .compatible = "simple-bus", },
> {},
> };
> @@ -55,6 +56,7 @@ static const char *board[] __initdata = {
> "amcc,haleakala",
> "amcc,kilauea",
> "amcc,makalu",
> + "amcc,klondike",
> "est,hotfoot"
> };
>
> --
> 1.6.1.rc3
>
>
--
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