From: Olof Johansson <olof@lixom•net>
To: Arnd Bergmann <arnd@arndb•de>
Cc: linuxppc-dev@ozlabs•org, paulus@samba•org, anton@samba•org
Subject: Re: [4/5] powerpc: PA Semi PWRficient platform support
Date: Tue, 5 Sep 2006 16:48:55 -0500 [thread overview]
Message-ID: <20060905164855.3c4a73da@localhost.localdomain> (raw)
In-Reply-To: <200609052337.11557.arnd@arndb.de>
On Tue, 5 Sep 2006 23:37:10 +0200 Arnd Bergmann <arnd@arndb•de> wrote:
> 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.
Thanks Arnd, good feedback, see some comments below.
>
>
> > 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?
Yes, we do. It'll be submitted in a later patch for various reasons. We'll use
the generic sync for now.
> > +
> > +#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.
Yes, mostly leftovers from older debug code. I've taken a few out already,
will go through and take care of the rest.
> > +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?
I was torn between using device tree and hardcoded values here, and went with
hardcoded since that's what maple uses. They're not movable on the chip, but
for future versions I guess device tree makes more sense (if it for some
reason will move).
>
> > + 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?
Good point, most platforms no longer implement this anyway. I'll code that up
as a separate patch and submit later.
>
> > +
> > +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.
Yep, I could take out the #if 0 as well. It's all a big placeholder anyway.
-Olof
next prev parent reply other threads:[~2006-09-05 21:49 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
2006-09-05 21:48 ` Olof Johansson [this message]
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=20060905164855.3c4a73da@localhost.localdomain \
--to=olof@lixom$(echo .)net \
--cc=anton@samba$(echo .)org \
--cc=arnd@arndb$(echo .)de \
--cc=linuxppc-dev@ozlabs$(echo .)org \
--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