On 9 December 2010 01:12, green <[email protected]> wrote:
> Hi,
>
> KanjiMonster asked me as a response to my threat on the OpenWRT forum
> (https://forum.openwrt.org/viewtopic.php?pid=122891) to send my patch as a
> mail attachment to the dev-list.

Good to see it here. I probably should have linked you to
<https://dev.openwrt.org/wiki/SubmittingPatches>, as I have some style
nitpicks ;-).

> As I am currently working with the WarpComm TK71 Qseven module based on the
> Marvell Kirkwood CPU and I liked to use OpenWRT as a basic environment I
> decided to port it based on the linux kernel patch provided by the board
> manufacturer ( http://www.warpcomm.org/downloads/tk71/20100805/linux-2.6.34-
> tk71.diff ).

Nice hardware. And it looks like they are actively supporting it by
providing updated patches.

> The kernel is based on the 2.6.34 kernel but works great for the 2.6.35
> version as well.

Good to know.

Now on to my nitpicks:
First some meta stuff:

Try to send inline, that makes it easier to comment on patches. But if
your email client automatically wraps too long lines or replaces tabs
with spaces, then it might be better to send inline. See the page
linked above for further info.

About the patch itself:
(...)
> ++    .gpio_card_detect = 29,
> ++};
> ++
> ++static unsigned int tk71_mpp_config[] __initdata = {
> ++#if 0
> ++    MPP0_SPI_SCn,           /* SPI - currently unused */
> ++    MPP1_SPI_MOSI,
> ++    MPP2_SPI_SCK,
> ++    MPP3_SPI_MISO,
> ++#endif
> ++    MPP7_PEX_RST_OUTn,      /* PCIe #reset */
> ++    MPP8_TW_SDA,            /* I2C */
> ++    MPP9_TW_SCK,            /* I2C */
(...)
> ++    kirkwood_nand_init(ARRAY_AND_SIZE(tk71_nand_parts), 25);
> ++
> ++    /* kirkwood_spi_init(); */ /* if you want to use SPI, uncomment this 
> and MPP setup above*/
> ++
> ++    kirkwood_i2c_init();

perhaps this should be exposed by a Kconfig option instead of
requiring to edit the sources?

> +diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
> +index 346ae14..e150045 100644
> +--- a/arch/arm/mm/Kconfig
> ++++ b/arch/arm/mm/Kconfig
> +@@ -382,6 +382,17 @@ config CPU_FEROCEON_OLD_ID
> +       for which the CPU ID is equal to the ARM926 ID.
> +       Relevant for Feroceon-1850 and early Feroceon-2850.
> +
> ++config CPU_FEROCEON_DISABLE_WAITFORIRQ
> ++    bool "Disable wait_for_irq (for JTAG debugging, see help)"
> ++    depends on CPU_FEROCEON
> ++    default n
> ++    help
> ++      On some Feroceon implementations it is necessary to disable
> ++      the wait_for_interrupt in cpu_feroceon_do_idle in order to
> ++      enable proper JTAG debugging (random kernel panics can happen
> ++      otherwise).
> ++      For production this can safely be left enabled.
> ++
> + # ARMv6
> + config CPU_V6
> +     bool "Support ARM V6 processor" if ARCH_INTEGRATOR || MACH_REALVIEW_EB 
> || MACH_REALVIEW_PBX || ARCH_DOVE
> +diff --git a/arch/arm/mm/proc-feroceon.S b/arch/arm/mm/proc-feroceon.S
> +index 53e6323..9a99066 100644
> +--- a/arch/arm/mm/proc-feroceon.S
> ++++ b/arch/arm/mm/proc-feroceon.S
> +@@ -123,9 +123,11 @@ ENTRY(cpu_feroceon_reset)
> +  */
> +     .align  5
> + ENTRY(cpu_feroceon_do_idle)
> ++#ifndef CONFIG_CPU_FEROCEON_DISABLE_WAITFORIRQ
> +     mov     r0, #0
> +     mcr     p15, 0, r0, c7, c10, 4          @ Drain write buffer
> +     mcr     p15, 0, r0, c7, c0, 4           @ Wait for interrupt
> ++#endif
> +     mov     pc, lr
> +
> + /*

This should be in its own kernel patch as this isn't TK71 specific (I assume).

+diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
+index e345ec8..ab16557 100644
+--- a/drivers/net/mv643xx_eth.c
++++ b/drivers/net/mv643xx_eth.c
> +@@ -2781,11 +2781,13 @@ static struct phy_device *phy_scan(struct 
> mv643xx_eth_private *mp,
> +
> +             if (phydev == NULL) {
> +                     phydev = bus->phy_map[addr];
> +-                    if (phydev != NULL)
> ++                    if (phydev != NULL) {
> +                             phy_addr_set(mp, addr);
> ++                            printk(KERN_INFO "mv643xx_eth: found PHY @ %d 
> ID %08x\n", phydev->addr, phydev->phy_id);
> ++                    } else
> ++                            printk(KERN_ERR "mv643xx_eth: PHY @ %d not 
> found!\n", addr);
> +             }
> +     }
> +-

Unnecessary white space change.

> +     return phydev;
> + }
> +

This should also be a separate kernel patch if not dropped since it
might produce quite a bit of output when scanning the whole address
range.


I can't comment the kernel config changes, since I don't know enough
about them to recognize which are required, and which aren't (e.g. is
the CONFIG_EXT4_FS_XATTR really needed?).

You should split this into three patches, one for the board support
and one for each of the kernel changes.
Also I saw there is an updated version of the patch available on their
site, which claims to fix some sata problems, perhaps you could
recreate the patch based on this version, assuming it has additional
changes?

Regards,
Jonas
_______________________________________________
openwrt-devel mailing list
[email protected]
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Reply via email to