From: Arnd Bergmann <arnd@arndb•de>
To: linuxppc-dev@ozlabs•org
Cc: Olof Johansson <olof@lixom•net>, paulus@samba•org, anton@samba•org
Subject: Re: [4/5] powerpc: PA Semi PWRficient platform support
Date: Tue, 5 Sep 2006 23:37:10 +0200 [thread overview]
Message-ID: <200609052337.11557.arnd@arndb.de> (raw)
In-Reply-To: <20060905122956.2cebd36d@localhost.localdomain>
On Tuesday 05 September 2006 19:29, Olof Johansson wrote:
> Base patch for PA6T and PA6T-1682M. This introduces the
> arch/powerpc/platform/pasemi directory, together with basic
> implementations for various setup.
>
> Much of this was based on other platform code, i.e. Maple, etc.
Very nice patch set, as expected ;-)
See below for the mandatory nitpicking.
> Index: merge/arch/powerpc/Kconfig
> ===================================================================
> --- merge.orig/arch/powerpc/Kconfig
> +++ merge/arch/powerpc/Kconfig
> @@ -413,6 +409,17 @@ config PPC_MAPLE
> This option enables support for the Maple 970FX Evaluation Board.
> For more informations, refer to <http://www.970eval.com>
>
> +config PPC_PASEMI
> + depends on PPC_MULTIPLATFORM && PPC64
> + bool "PA Semi SoC-based platforms"
> + default n
> + select MPIC
> + select PPC_UDBG_16550
> + select GENERIC_TBSYNC
> + help
> + This option enables support for PA Semi's PWRficient line
> + of SoC processors, including PA6T-1682M
IIRC, the GENERIC_TBSYNC code is really inefficient. Don't you
have any other way of implementing that?
> +
> +#undef DEBUG
> +
I know that this is done in other places as well, but it seems
rather pointless, and it provides setting DEBUG from EXTRA_CFLAGS
in the Makefile.
> +static struct pci_ops pa_pxp_ops =
> +{
> + pa_pxp_read_config,
> + pa_pxp_write_config
> +};
spacing: '{' after '=', and ',' after the final member.
> +
> +static void __init setup_pa_pxp(struct pci_controller* hose)
> +{
> + hose->ops = &pa_pxp_ops;
> + hose->cfg_data = ioremap(0xe0000000, 0x1000000);
> +}
Shouldn't that be in the device tree?
> + bus_range = (int *) get_property(dev, "bus-range", &len);
Unneeded cast
> + if (bus_range == NULL || len < 2 * sizeof(int)) {
> + printk(KERN_WARNING "Can't get bus-range for %s, assume bus 0\n",
> + dev->full_name);
> + }
if (!bus_range || len < 2 * sizeof(int)) {
> +
> + hose = pcibios_alloc_controller(dev);
> + if (hose == NULL)
> + return -ENOMEM;
if (!hose)
> + if (root == NULL) {
if (!root)
> +#undef DEBUG
remove
> +extern int pas_set_rtc_time(struct rtc_time *tm);
> +extern void pas_get_rtc_time(struct rtc_time *tm);
> +extern unsigned long pas_get_boot_time(void);
> +extern void pas_pci_init(void);
> +extern void pas_pcibios_fixup(void);
Extern declarations should never be in a source file, please
move them to a header
> +static void iommu_dev_setup_null(struct pci_dev *dev) { }
> +static void iommu_bus_setup_null(struct pci_bus *bus) { }
> +
> +static void __init pas_init_early(void)
> +{
> + /* No iommu code yet */
> + ppc_md.iommu_dev_setup = iommu_dev_setup_null;
> + ppc_md.iommu_bus_setup = iommu_bus_setup_null;
> + pci_direct_iommu_init();
> +}
> +
> +/* No legacy IO on our parts */
> +static int pas_check_legacy_ioport(unsigned int baseport)
> +{
> + return -ENODEV;
> +}
Should we maybe change the default behavior so that you don't need
to provide nops for these functions?
> +
> +static __init void pas_init_IRQ(void)
> +{
> + struct device_node *np = NULL;
> + struct device_node *root, *mpic_node = NULL;
> + unsigned long openpic_addr = 0;
These three should not be initialized.
> + BUG_ON(mpic == NULL);
BUG_ON(!mpic);
> +
> +#undef DEBUG
remove
> +void pas_get_rtc_time(struct rtc_time *tm)
> +{
> +}
> +
> +int pas_set_rtc_time(struct rtc_time *tm)
> +{
> + return -ENODEV;
> +}
again, it probably makes sense to not have to provide these.
Arnd <><
next prev parent reply other threads:[~2006-09-05 21:37 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20060904175742.5472a6fa@localhost.localdomain>
2006-09-05 17:26 ` [1/5] powerpc: Reduce default cacheline size to 64 bytes Olof Johansson
2006-09-05 17:27 ` [2/5] powerpc: Divorce CPU_FTR_CTRL from CPU_FTR_PPCAS_ARCH_V2_BASE Olof Johansson
2006-09-05 17:28 ` [3/5] powerpc: PA6T cputable entry, PVR value Olof Johansson
2006-09-05 18:43 ` Michael Neuling
2006-09-05 17:28 ` [4/5] powerpc: PA Semi PWRficient platform support Olof Johansson
2006-09-05 21:31 ` Benjamin Herrenschmidt
2006-09-05 22:10 ` Olof Johansson
2006-09-05 17:29 ` Olof Johansson
2006-09-05 19:49 ` Roland Dreier
2006-09-05 20:15 ` Olof Johansson
2006-09-05 21:37 ` Arnd Bergmann [this message]
2006-09-05 21:48 ` Olof Johansson
2006-09-06 13:38 ` Segher Boessenkool
2006-09-06 15:10 ` Olof Johansson
2006-09-06 15:26 ` Segher Boessenkool
2006-09-07 0:58 ` Benjamin Herrenschmidt
2006-09-07 11:30 ` Segher Boessenkool
2006-09-07 0:58 ` Benjamin Herrenschmidt
2006-09-07 11:28 ` Segher Boessenkool
2006-09-07 12:47 ` Olof Johansson
2006-09-07 22:33 ` Benjamin Herrenschmidt
2006-09-05 17:30 ` [5/5] powerpc: PA Semi PWRficient MAINTAINER entry Olof Johansson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=200609052337.11557.arnd@arndb.de \
--to=arnd@arndb$(echo .)de \
--cc=anton@samba$(echo .)org \
--cc=linuxppc-dev@ozlabs$(echo .)org \
--cc=olof@lixom$(echo .)net \
--cc=paulus@samba$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox