linux-mips
[Top] [All Lists]

Re: [PATCH V7 2/4] MIPS: Add board support for Loongson1B

To: Ralf Baechle <ralf@linux-mips.org>
Subject: Re: [PATCH V7 2/4] MIPS: Add board support for Loongson1B
From: Kelvin Cheung <keguang.zhang@gmail.com>
Date: Thu, 21 Jun 2012 15:34:54 +0800
Cc: linux-mips@linux-mips.org, linux-kernel@vger.kernel.org, wuzhangjin@gmail.com, zhzhl555@gmail.com
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type; bh=hSr0miyH16wNzmjXevPO7uG8ALJpwK5MC1mIBo1IsHU=; b=I0e/umFM+BUlJrfVTDaGDLazsImSH+v8LfuS21L/Z2SzmOD9Kt7foaHIkgDGlX32SS CB/KBTa5Glc247Tk39vauN0deUrUMHcXhWtMMvru4NvbG374wDhlJ4LH4YXuduMgpnjR nvqB8bSgnOZDvEMLhJShPKRk6yvnrGg243y6AegWpQHhL5Tnb/6Lvvrs0V9VbtR5KaU6 7q+gbCNJ/2XMFdl4z/A+1+bF2vNT3Zy5tnKp2toUrpu2mQ+LOp337wJzKBuAYaHt/mPt MzIZ7Wu5UlAGNpZvizTAlDA2zR+H2yeux9uGVve3k+VjiBw+/3A3i+aSaheSGkEdR/s8 JGTg==
In-reply-to: <20120620192551.GC29446@linux-mips.org>
List-archive: <http://www.linux-mips.org/archives/linux-mips/>
List-help: <mailto:ecartis@linux-mips.org?Subject=help>
List-id: linux-mips <linux-mips.eddie.linux-mips.org>
List-owner: <mailto:ralf@linux-mips.org>
List-post: <mailto:linux-mips@linux-mips.org>
List-software: Ecartis version 1.0.0
List-subscribe: <mailto:ecartis@linux-mips.org?subject=subscribe%20linux-mips>
List-unsubscribe: <mailto:ecartis@linux-mips.org?subject=unsubscribe%20linux-mips>
References: <1339757617-2187-1-git-send-email-keguang.zhang@gmail.com> <1339757617-2187-3-git-send-email-keguang.zhang@gmail.com> <20120620192551.GC29446@linux-mips.org>
Sender: linux-mips-bounce@linux-mips.org


2012/6/21 Ralf Baechle <ralf@linux-mips.org>
On Fri, Jun 15, 2012 at 06:53:35PM +0800, Kelvin Cheung wrote:

> +#include <linux/clk.h>

> +static LIST_HEAD(clocks);
> +static DEFINE_MUTEX(clocks_mutex);
> +
> +struct clk *clk_get(struct device *dev, const char *name)
> +{
> +     struct clk *c;
> +     struct clk *ret = NULL;
> +
> +     mutex_lock(&clocks_mutex);
> +     list_for_each_entry(c, &clocks, node) {
> +             if (!strcmp(c->name, name)) {
> +                     ret = c;
> +                     break;
> +             }
> +     }
> +     mutex_unlock(&clocks_mutex);
> +
> +     return ret;
> +}
> +EXPORT_SYMBOL(clk_get);

This redefines a function that already is declared in <linux/clk.h> and
defined in drivers/clk/clkdev.c.  Why?

My intention is to provide minimum clock support,
just as other platforms (bcm63xx, jz4740, etc) did.
Is that OK?
 

> +int clk_register(struct clk *clk)
> +{
> +     mutex_lock(&clocks_mutex);
> +     list_add(&clk->node, &clocks);
> +     if (clk->ops->init)
> +             clk->ops->init(clk);
> +     mutex_unlock(&clocks_mutex);
> +
> +     return 0;
> +}
> +EXPORT_SYMBOL(clk_register);

Same here.

> diff --git a/arch/mips/loongson1/common/prom.c b/arch/mips/loongson1/common/prom.c
> new file mode 100644
> index 0000000..1f8e49f
> --- /dev/null
> +++ b/arch/mips/loongson1/common/prom.c
> @@ -0,0 +1,87 @@
> +/*
> + * Copyright (c) 2011 Zhang, Keguang <keguang.zhang@gmail.com>
> + *
> + * Modified from arch/mips/pnx833x/common/prom.c.
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/serial_reg.h>
> +#include <asm/bootinfo.h>
> +
> +#include <loongson1.h>
> +#include <prom.h>
> +
> +int prom_argc;
> +char **prom_argv, **prom_envp;
> +unsigned long memsize, highmemsize;
> +
> +char *prom_getenv(char *envname)
> +{
> +     char **env = prom_envp;
> +     int i;
> +
> +     i = strlen(envname);
> +
> +     while (*env) {
> +             if (strncmp(envname, *env, i) == 0 && *(*env+i) == '=')
> +                     return *env + i + 1;
> +             env++;
> +     }
> +
> +     return 0;
> +}
[...]

Please take a look at sjhill's firmware cleanup patchset which is going to
remove a fair chunk of duplication of firmware related code.

This is just a heads up; you need to do nothing because that patchset is not
applied yet.)

Yes, I have noticed those patches.
I will make changes on my code accordingly once those patches are accepted.
 

> +const char *get_system_type(void)
> +{
> +     unsigned int processor_id = (&current_cpu_data)->processor_id;
> +
> +     switch (processor_id & PRID_REV_MASK) {
> +     case PRID_REV_LOONGSON1B:
> +             return "LOONGSON LS1B";
> +     default:
> +             return "LOONGSON (unknown)";
> +     }
> +}

You probably should return a string identifying the system, not the SOC
being used.  So this function should probably go to board.c.

Here, I just want to return the SOC type, not system.
Perhaps, another CPU, for example, Loongson1A will be added in the future.
So this function should probably stay in common/setup.c.
 

> diff --git a/arch/mips/loongson1/ls1b/board.c b/arch/mips/loongson1/ls1b/board.c
> new file mode 100644
> index 0000000..1ec350d
> --- /dev/null
> +++ b/arch/mips/loongson1/ls1b/board.c
> @@ -0,0 +1,39 @@
> +/*
> + * Copyright (c) 2011 Zhang, Keguang <keguang.zhang@gmail.com>
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +
> +#include <platform.h>
> +
> +#include <linux/serial_8250.h>
> +#include <loongson1.h>
> +
> +static struct platform_device *ls1b_platform_devices[] __initdata = {
> +     &ls1x_uart_device,
> +#if IS_ENABLED(CONFIG_STMMAC_ETH)
> +     &ls1x_eth0_device,
> +#endif
> +#if IS_ENABLED(CONFIG_USB_EHCI_HCD)
> +     &ls1x_ehci_device,
> +#endif
> +#if IS_ENABLED(CONFIG_RTC_DRV_LOONGSON1)
> +     &ls1x_rtc_device,
> +#endif
> +};

Don't ifdef the platform devices.  The platform devices should always
be registered if a system actually has the underlying hardware.  If the
driver has been compiled or not does not matter.

OK, I will fix this later.
 

And the final plug - take a look at FDT for a future revision of this
code :)


Yes, I am aware that the FDT is the final target.
But PMON which is the bootloader of Loongson CPU does not support FDT at present.
I will remember this.
 
 Ralf



--
Best Regards!
Kelvin



<Prev in Thread] Current Thread [Next in Thread>