* RE: [PATCH 1/3] MSI driver for Freescale 83xx/85xx/86xx cpu
From: Jin Zhengxiong @ 2008-04-21 10:01 UTC (permalink / raw)
To: michael; +Cc: linuxppc-dev, Gala Kumar
In-Reply-To: <1208758907.9955.4.camel@concordia>
Hi, Michael,
Thank you very much for you input, please see my inline answer.
B.R
Jason=20
> -----Original Message-----
> From: Michael Ellerman [mailto:michael@ellerman.id.au]=20
> Sent: Monday, April 21, 2008 2:22 PM
> To: Jin Zhengxiong
> Cc: linuxppc-dev@ozlabs.org; Gala Kumar
> Subject: Re: [PATCH 1/3] MSI driver for Freescale 83xx/85xx/86xx cpu
>=20
> > This MSI driver can be used on 83xx/85xx/86xx board.
> > In this driver, virtual interrupt host and chip were
> > setup. There are 256 MSI interrupts in this host, Every 32
> > MSI interrupts cascaded to one IPIC/MPIC interrupt.
> > The chip was treated as edge sensitive and some necessary
> > functions were setup for this chip.
> >=20
> > Before using the MSI interrupt, PCI/PCIE device need to
> > ask for a MSI interrupt in the 256 MSI interrupts. A 256bit
> > bitmap show which MSI interrupt was used, reserve bit in
> > the bitmap can be used to force the device use some designate
> > MSI interrupt in the 256 MSI interrupts. Sometimes this is useful
> > for testing the all the MSI interrupts. The msi-available-ranges
> > property in the dts file was used for this purpose.
> =20
>=20
> Hi Jason, some comments inline ...
>=20
> =20
> > diff --git a/arch/powerpc/sysdev/Makefile=20
> b/arch/powerpc/sysdev/Makefile
> > index 6d386d0..bfd3fe4 100644
> > --- a/arch/powerpc/sysdev/Makefile
> > +++ b/arch/powerpc/sysdev/Makefile
> > @@ -4,6 +4,7 @@ endif
> > =20
> > mpic-msi-obj-$(CONFIG_PCI_MSI) +=3D mpic_msi.o=20
> mpic_u3msi.o mpic_pasemi_msi.o
> > obj-$(CONFIG_MPIC) +=3D mpic.o $(mpic-msi-obj-y)
> > +obj-$(CONFIG_PCI_MSI) +=3D fsl_msi.o
>=20
> Do we really always want to build this if we have MSI? Might it depend
> on something else as well? CONFIG_FSL_PCI maybe?
>=20
I'll try to change the depend.
> > diff --git a/arch/powerpc/sysdev/fsl_msi.c=20
> b/arch/powerpc/sysdev/fsl_msi.c
> > new file mode 100644
> > index 0000000..e8132cf
> > --- /dev/null
> > +++ b/arch/powerpc/sysdev/fsl_msi.c
> > @@ -0,0 +1,457 @@
> > +/*
> > + * Copyright (C) 2007-2008 Freescale Semiconductor, Inc.=20
> All rights reserved.
> > + *
> > + * Author: Tony Li <tony.li@freescale.com>
> > + * Jason Jin <Jason.jin@freescale.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; version 2 of the
> > + * License.
> > + *
> > + */
> > +
> > +#include <linux/irq.h>
> > +#include <linux/bootmem.h>
> > +#include <linux/bitmap.h>
> > +#include <linux/msi.h>
> > +#include <asm/prom.h>
> > +#include <asm/hw_irq.h>
> > +#include <linux/pci.h>
> > +#include <asm/ppc-pci.h>
> > +#include <linux/of_platform.h>
> > +
> > +#include <sysdev/fsl_soc.h>
> > +#include "fsl_msi.h"
>=20
> People consider it good style to have the linux includes before the
> asm includes.
>=20
OK
> > +#ifdef DEBUG
> > +#define pr_debug(fmt...) printk(fmt)
> > +#else
> > +#define pr_debug(fmt...)
> > +#endif
>=20
> Please don't do this, just use pr_debug(). In fact I don't=20
> see where you
> do use it :)
>=20
OK.
> > +/* A bit ugly, can we get this from the pci_dev somehow? */
> > +static struct fsl_msi *fsl_msi;
>=20
> I recognise that comment :) The answer is "no" we can't (easily) get
> this from the pci_dev.
>=20
> > +static inline u32 fsl_msi_read(u32 __iomem *base,
> > + unsigned int reg)
>=20
> This would fit in 80 chars wouldn't it?
>=20
OK
> > +{
> > + return in_be32(base + (reg >> 2));
> > +}
> > +
> > +static inline void fsl_msi_write(u32 __iomem *base,
> > + unsigned int reg, u32 value)
> > +{
> > + out_be32(base + (reg >> 2), value);
> > +}
> > +
> > +#define fsl_msi_irq_to_hw(virq) ((unsigned=20
> int)irq_map[virq].hwirq)
>=20
> I can't see where this is used, you probably don't need it.
>
I'll remove this.
=20
> > +/*
> > + * We do not need this actually. The MSIR register has=20
> been read once
> > + * in the cascade interrupt. So, this MSI interrupt has been acked
> > +*/
> > +static void fsl_msi_end_irq(unsigned int virq)
> > +{
> > +}
>=20
> I guess the generic code assumes you have an ack, bummer.
>=20
Yes, the generic code did not check it.
> > +static struct irq_chip fsl_msi_chip =3D {
> > + .mask =3D mask_msi_irq,
> > + .unmask =3D unmask_msi_irq,
> > + .ack =3D fsl_msi_end_irq,
> > + .typename =3D " FSL-MSI ",
>=20
> I'd rather you didn't try and pad the name by hand, if we=20
> want /proc/interrupts
> to look pretty we should do that in show_interrupts().
>=20
Thanks, but I feel it's easier and cleaner to add the typename here to
make=20
the /proc/interrupts clear.=20
> > +static int fsl_msi_host_match(struct irq_host *h, struct=20
> device_node *node)
> > +{
> > + struct fsl_msi *msi =3D h->host_data;
> > +
> > + /* Exact match, unless node is NULL */
> > + return msi->of_node =3D=3D NULL || msi->of_node =3D=3D node;
> > +}
>=20
> Do you really want the MSI to be the default irq host?
>=20
Thanks. I'll change the host match function.
> > +static int fsl_msi_host_map(struct irq_host *h, unsigned int virq,
> > + irq_hw_number_t hw)
> > +{
> > + struct fsl_msi *msi =3D h->host_data;
> > + struct irq_chip *chip =3D msi->hc_irq;
> > +
> > + set_irq_chip_data(virq, msi);
>=20
> You don't seem to use chip_data anywhere?
>=20
I'll check this.
> > + get_irq_desc(virq)->status |=3D IRQ_TYPE_EDGE_FALLING;
> > +
> > + set_irq_chip_and_handler(virq, chip, handle_edge_irq);
> > +
> > + return 0;
> > +}
> > +
> > +static struct irq_host_ops fsl_msi_host_ops =3D {
> > + .match =3D fsl_msi_host_match,
> > + .map =3D fsl_msi_host_map,
> > +};
> > +
> > +irq_hw_number_t fsl_msi_alloc_hwirqs(struct fsl_msi *msi, int num)
> > +{
> > + unsigned long flags;
> > + int offset, order =3D get_count_order(num);
> > +
> > + spin_lock_irqsave(&msi->bitmap_lock, flags);
> > +
> > + offset =3D bitmap_find_free_region(msi->fsl_msi_bitmap,
> > + NR_MSI_IRQS, order);
> > +
> > + spin_unlock_irqrestore(&msi->bitmap_lock, flags);
> > +
> > + pr_debug("%s: allocated 0x%x (2^%d) at offset 0x%x\n",
> > + __func__, num, order, offset);
> > +
> > + return offset;
> > +}
> > +
> > +void fsl_msi_free_hwirqs(struct fsl_msi *msi, int offset, int num)
> > +{
> > + unsigned long flags;
> > + int order =3D get_count_order(num);
> > +
> > + pr_debug("%s: freeing 0x%x (2^%d) at offset 0x%x\n",
> > + __func__, num, order, offset);
> > +
> > + spin_lock_irqsave(&msi->bitmap_lock, flags);
> > + bitmap_release_region(msi->fsl_msi_bitmap, offset, order);
> > + spin_unlock_irqrestore(&msi->bitmap_lock, flags);
> > +}
> > +
> > +static int fsl_msi_reserve_dt_hwirqs(struct fsl_msi *msi)
> > +{
> > + int i, len;
> > + const u32 *p;
> > +
> > + p =3D of_get_property(msi->of_node, "msi-available-ranges", &len);
> > + if (!p) {
> > + pr_debug("fsl_msi: no msi-available-ranges=20
> property found \
> > + on %s\n", msi->of_node->full_name);
> > + return -ENODEV;
> > + }
> > +
> > + if (len & 0x8 !=3D 0) {
> > + printk(KERN_WARNING "fsl_msi: Malformed=20
> msi-available-ranges "
> > + "property on %s\n", msi->of_node->full_name);
> > + return -EINVAL;
> > + }
>=20
> Do you really want a bitwise and with 0x8?
>=20
The range for the msi interrupt can be seperated to several part.=20
This can used to check the if the ranges is correct.=20
> > +
> > + bitmap_allocate_region(msi->fsl_msi_bitmap, 0,
> > + get_count_order(msi->irq_count));
> > +
> > + /* Format is: (<u32 start> <u32 count>)+ */
> > + len /=3D sizeof(u32);
> > + len /=3D 2;
> > + for (i =3D 0; i < len; i++, p +=3D 2)
> > + fsl_msi_free_hwirqs(msi, *p, *(p + 1));
> > +
> > + return 0;
> > +}
>=20
> We could share a bunch of that code with mpic_msi.c, but=20
> that's not your
> job - I'll look at doing a patch once this goes in.
>=20
Thanks for the code you write for the MPIC msi.
I'll use this until you share the code.
> > +static int fsl_msi_init_allocator(struct fsl_msi *msi)
> > +{
> > + int rc, size;
> > +
> > + size =3D BITS_TO_LONGS(NR_MSI_IRQS) * sizeof(long);
> > +
> > + msi->fsl_msi_bitmap =3D kmalloc(size, GFP_KERNEL);
> > +
> > + if (msi->fsl_msi_bitmap =3D=3D NULL) {
> > + pr_debug("%s: ENOMEM allocating allocator bitmap!\n",
> > + __func__);
> > + return -ENOMEM;
> > + }
> > + memset(msi->fsl_msi_bitmap, 0, size);
>=20
> Use kzalloc() and you can lose the memset.
>=20
OK.
> > + rc =3D fsl_msi_reserve_dt_hwirqs(msi);
>=20
> This routine is badly named (my fault), it really frees the available
> MSI ranges.
>=20
> > + if (rc)
> > + goto out_free;
> > +
> > + return 0;
> > +out_free:
> > + if (mem_init_done)
> > + kfree(msi->fsl_msi_bitmap);
>=20
> You don't need to test mem_init_done, this is running at initcall time
> which is way later than mem_init.
>=20
Yes, I'll remove this. This code once used at just after the mpic was
intialized,and this
was left here.
> > + msi->fsl_msi_bitmap =3D NULL;
> > + return rc;
> > +
> > +}
> > +
> > +static int fsl_msi_check_device(struct pci_dev *pdev, int=20
> nvec, int type)
> > +{
> > + if (type =3D=3D PCI_CAP_ID_MSIX)
> > + pr_debug("fslmsi: MSI-X untested, trying anyway.\n");
> > +
> > + return 0;
> > +}
> > +
> > +static void fsl_teardown_msi_irqs(struct pci_dev *pdev)
> > +{
> > + struct msi_desc *entry;
> > + struct fsl_msi *msi =3D fsl_msi;
> > +
> > + list_for_each_entry(entry, &pdev->msi_list, list) {
> > + if (entry->irq =3D=3D NO_IRQ)
> > + continue;
> > + set_irq_msi(entry->irq, NULL);
> > + fsl_msi_free_hwirqs(msi, virq_to_hw(entry->irq), 1);
> > + irq_dispose_mapping(entry->irq);
> > + }
> > +
> > + return;
> > +}
> > +
> > +static void fsl_compose_msi_msg(struct pci_dev *pdev, int hwirq,
> > + struct msi_msg *msg)
> > +{
> > + unsigned int srs;
> > + unsigned int ibs;
> > + struct fsl_msi *msi =3D fsl_msi;
> > +
> > + srs =3D hwirq / INT_PER_MSIR;
> > + ibs =3D hwirq % INT_PER_MSIR;
> > +
> > + msg->address_lo =3D msi->msi_addr_lo;
> > + msg->address_hi =3D msi->msi_addr_hi;
> > + msg->data =3D (srs << 5) | (ibs & 0x1F);
>=20
> Is the 5 and 0x1F independent of the INT_PER_MSIR value? Given the
> current values isn't this a no-op, or am I missing something?
>=20
Do you mean there're another way to get the msg->data from the hwirq? =20
> > + pr_debug("%s: allocated srs: %d, ibs: %d\n",
> > + __func__, srs, ibs);
> > +}
> > +
> > +static int fsl_setup_msi_irqs(struct pci_dev *pdev, int=20
> nvec, int type)
> > +{
> > + irq_hw_number_t hwirq;
> > + int rc;
> > + unsigned int virq;
> > + struct msi_desc *entry;
> > + struct msi_msg msg;
> > + struct fsl_msi *msi =3D fsl_msi;
>=20
> A couple of places you put this into a local called "msi"=20
> which is not the
> greatest name in the world IMHO :)
>=20
Thank you, I'll try to use another name, Do you have any suggestion?
> > + list_for_each_entry(entry, &pdev->msi_list, list) {
> > + hwirq =3D fsl_msi_alloc_hwirqs(msi, 1);
> > + if (hwirq < 0) {
> > + rc =3D hwirq;
> > + pr_debug("%s: fail allocating msi interrupt\n",
> > + __func__);
> > + goto out_free;
> > + }
> > +
> > + virq =3D irq_create_mapping(msi->irqhost, hwirq);
> > +
> > + if (virq =3D=3D NO_IRQ) {
> > + pr_debug("%s: fail mapping hwirq 0x%lx\n",
> > + __func__, hwirq);
> > + fsl_msi_free_hwirqs(msi, hwirq, 1);
> > + rc =3D -ENOSPC;
> > + goto out_free;
> > + }
> > + set_irq_msi(virq, entry);
> > +
> > + fsl_compose_msi_msg(pdev, hwirq, &msg);
> > + write_msi_msg(virq, &msg);
> > + }
> > + return 0;
> > +
> > +out_free:
> > + fsl_teardown_msi_irqs(pdev);
>=20
> You don't need to call teardown, the generic code does that for you.
>=20
OK.
> > + return rc;
> > +}
> > +
> > +void fsl_msi_cascade(unsigned int irq, struct irq_desc *desc)
> > +{
> > + unsigned int cascade_irq;
> > + struct fsl_msi *msi =3D fsl_msi;
> > + int msir_index =3D -1;
> > + int i;
> > + u32 msir_value =3D 0;
> > + u32 intr_index;
> > + u32 have_shift =3D 0;
> > +
> > + spin_lock(&desc->lock);
> > + if ((msi->feature & FSL_PIC_IP_MASK) =3D=3D FSL_PIC_IP_IPIC) {
> > + if (desc->chip->mask_ack)
> > + desc->chip->mask_ack(irq);
> > + else {
> > + desc->chip->mask(irq);
> > + desc->chip->ack(irq);
> > + }
> > + }
> > +
> > + if (unlikely(desc->status & IRQ_INPROGRESS))
> > + goto unlock;
> > +
> > + for (i =3D 0; i < NR_MSIR; i++)
> > + if (irq =3D=3D msi->msir[i]) {
> > + msir_index =3D i;
> > + break;
> > + }
>=20
> This is a bit ugly :) Because you get the *msi from fsl_msi=20
> (above), you could
> store the msir_index in the handler_data (with set_irq_data),=20
> and save doing
> this loop.
>=20
I'll find if the handler data *msi was used somewhere. if not I change
to
save the msir_index to the handler data.=20
> > + if (i >=3D NR_MSIR)
> > + cascade_irq =3D NO_IRQ;
> > +
> > + desc->status |=3D IRQ_INPROGRESS;
> > + switch (fsl_msi->feature & FSL_PIC_IP_MASK) {
> > + case FSL_PIC_IP_MPIC:
> > + msir_value =3D fsl_msi_read(msi->msi_regs,=20
> msir_index * 0x10);
> > + break;
> > + case FSL_PIC_IP_IPIC:
> > + msir_value =3D fsl_msi_read(msi->msi_regs,=20
> msir_index * 0x4);
> > + break;
> > + }
> > +
> > + while (msir_value) {
> > + intr_index =3D ffs(msir_value) - 1;
> > +
> > + cascade_irq =3D irq_linear_revmap(msi->irqhost,
> > + (msir_index * INT_PER_MSIR + intr_index=20
> + have_shift));
> > +
> > + if (cascade_irq !=3D NO_IRQ)
> > + generic_handle_irq(cascade_irq);
> > + have_shift +=3D (intr_index + 1);
> > + msir_value =3D (msir_value >> (intr_index + 1));
> > + }
>=20
> It took me a while to grok all the shifting and so on here, I=20
> don't know if
> there's a cleaner way to do it.
>=20
To improve the efficency here, I tried to use the bit operation which
can=20
carry out by the cpu with one instruction. Maybe I also need to improve=20
readability :).
> > + desc->status &=3D ~IRQ_INPROGRESS;
> > +
> > + switch (fsl_msi->feature & FSL_PIC_IP_MASK) {
> > + case FSL_PIC_IP_MPIC:
> > + desc->chip->eoi(irq);
> > + break;
> > + case FSL_PIC_IP_IPIC:
> > + if (!(desc->status & IRQ_DISABLED) &&=20
> desc->chip->unmask)
> > + desc->chip->unmask(irq);
>=20
> Missing indent.
>=20
OK.
> > + break;
> > + }
> > +unlock:
> > + spin_unlock(&desc->lock);
> > +}
> > +
> > +static int __devinit fsl_of_msi_probe(struct of_device *dev,
> > + const struct of_device_id *match)
> > +{
> > + struct fsl_msi *msi;
> > + struct resource res;
> > + int err, i, count;
> > + int rc;
> > + int virt_msir;
> > + const u32 *p;
> > + struct fsl_msi_data *tmp_data;
> > +
> > + printk(KERN_INFO "Setting up fsl msi support\n");
>=20
> KERN_DEBUG would do here IMHO, users don't usually need to see it.
>=20
> > + msi =3D kzalloc(sizeof(struct fsl_msi), GFP_KERNEL);
> > + if (!msi) {
> > + dev_err(&dev->dev, "No memory for MSI structure\n");
> > + err =3D -ENOMEM;
> > + goto error_out;
> > + }
> > +
> > + msi->of_node =3D dev->node;
> > +
> > + msi->irqhost =3D irq_alloc_host(of_node_get(dev->node),
> > + IRQ_HOST_MAP_LINEAR,
> > + NR_MSI_IRQS, &fsl_msi_host_ops, 0);
> > + if (msi->irqhost =3D=3D NULL) {
> > + dev_err(&dev->dev, "No memory for MSI irqhost\n");
> > + err =3D -ENOMEM;
>=20
> You need an of_node_put(dev->node) in the error case here.
>=20
Thanks, I'll add it.
> > + goto error_out;
> > + }
> > +
> > + /*Get the MSI reg base */
>=20
> Missing space after /*
>=20
> > + err =3D of_address_to_resource(dev->node, 0, &res);
> > + if (err) {
> > + dev_err(&dev->dev, "Can't get %s property 'reg'\n",
> > + dev->node->full_name);
>=20
> That's a little misleading, aren't there's lots of reasons
> of_address_to_resource() might fail?
>=20
> > + goto error_out;
> > + }
> > +
> > + msi->msi_regs =3D ioremap(res.start, res.end - res.start + 1);
>=20
> ioremap() can fail.
>=20
OK, I'll add the error process code here.
> > + tmp_data =3D (struct fsl_msi_data *)match->data;
> > +
> > + msi->feature =3D tmp_data->fsl_pic_ip;
> > +
> > + msi->irqhost->host_data =3D msi;
> > + msi->hc_irq =3D &fsl_msi_chip;
>=20
> Any reason this needs to be a variable, isn't it always &fsl_msi_chip?
>=20
> > + msi->msi_addr_hi =3D 0x0;
> > + msi->msi_addr_lo =3D res.start + tmp_data->msiir_offset;
> > +
> > + msi->irq_count =3D NR_MSI_IRQS;
>=20
> Ditto.
>=20
> > + p =3D of_get_property(dev->node, "interrupts", &count);
> > + if (!p) {
> > + dev_err(&dev->dev, "no msi-interrupts property=20
> found on %s\n",
> > + dev->node->full_name);
> > + err =3D -ENODEV;
> > + goto error_out;
> > + }
> > + if (count % 8 !=3D 0) {
> > + dev_err(&dev->dev, "Malformed msi-interrupts=20
> property on %s\n",
> > + dev->node->full_name);
>=20
> Messages don't match the code, "interrupts" vs "msi-interrupts".
>=20
Thanks, I'll change the description.
> > + err =3D -EINVAL;
> > + goto error_out;
> > + }
> > +
> > + count /=3D sizeof(u32);
> > + for (i =3D 0; i < count / 2; i++) {
> > + if (i > NR_MSIR)
> > + break;
> > + virt_msir =3D irq_of_parse_and_map(dev->node, i);
> > + if (virt_msir !=3D NO_IRQ) {
> > + set_irq_data(virt_msir, msi);
> > + set_irq_chained_handler(virt_msir,=20
> fsl_msi_cascade);
> > + msi->msir[i] =3D virt_msir;
> > + } else
> > + msi->msir[i] =3D NO_IRQ;
> > + }
> > +
> > + rc =3D fsl_msi_init_allocator(msi);
> > + if (rc) {
> > + dev_err(&dev->dev, "Error allocating MSI bitmap\n");
>=20
> If you hit this then the error case needs to get rid of all=20
> the irq mappings
> you created just above (which it doesn't). It would be=20
> simpler if you just move
> this call above the for loop, then you don't need to worry about it.
>
Thanks, good idea.
=20
> > + goto error_out;
> > + }
> > +
> > + fsl_msi =3D msi;
> > +
> > + WARN_ON(ppc_md.setup_msi_irqs);
> > + ppc_md.setup_msi_irqs =3D fsl_setup_msi_irqs;
> > + ppc_md.teardown_msi_irqs =3D fsl_teardown_msi_irqs;
> > + ppc_md.msi_check_device =3D fsl_msi_check_device;
> > + return 0;
> > +error_out:
> > + if (msi)
> > + kfree(msi);
>=20
> kfree(NULL) is fine.
>=20
> > + return err;
> > +}
> > +
> > +static const struct fsl_msi_data mpic_msi_feature =3D=20
> {FSL_PIC_IP_MPIC, 0x140};
> > +static const struct fsl_msi_data ipic_msi_feature =3D=20
> {FSL_PIC_IP_IPIC, 0x38};
> > +
> > +static const struct of_device_id fsl_of_msi_ids[] =3D {
> > + {
> > + .compatible =3D "fsl,MPIC-MSI",
> > + .data =3D (void *)&mpic_msi_feature,
> > + },
> > + {
> > + .compatible =3D "fsl,IPIC-MSI",
> > + .data =3D (void *)&ipic_msi_feature,
> > + },
> > + {}
> > +};
> > +
> > +static struct of_platform_driver fsl_of_msi_driver =3D {
> > + .name =3D "fsl-of-msi",
> > + .match_table =3D fsl_of_msi_ids,
> > + .probe =3D fsl_of_msi_probe,
> > +};
> > +
> > +static __init int fsl_of_msi_init(void)
> > +{
> > + return of_register_platform_driver(&fsl_of_msi_driver);
> > +}
> > +
> > +subsys_initcall(fsl_of_msi_init);
> > diff --git a/arch/powerpc/sysdev/fsl_msi.h=20
> b/arch/powerpc/sysdev/fsl_msi.h
> > new file mode 100644
> > index 0000000..7eef9ec
> > --- /dev/null
> > +++ b/arch/powerpc/sysdev/fsl_msi.h
> > @@ -0,0 +1,40 @@
> > +#ifndef _ASM_POWERPC_MSI_H
> > +#define _ASM_POWERPC_MSI_H
>=20
> Include guards don't match the filename, should be=20
> _POWERPC_SYSDEV_FSL_MSI_H
>=20
OK.
> > +#define NR_MSIR 8
> > +#define INT_PER_MSIR 32
> > +#define NR_MSI_IRQS (NR_MSIR * INT_PER_MSIR)
> > +
> > +#define FSL_PIC_IP_MASK 0x0000000F
> > +#define FSL_PIC_IP_MPIC 0x00000001
> > +#define FSL_PIC_IP_IPIC 0x00000002
> > +
> > +struct fsl_msi {
> > + /* Device node of the MSI interrupt*/
> > + struct device_node *of_node;
> > +
> > + struct irq_host *irqhost;
> > + struct irq_chip *hc_irq;
> > +
> > + unsigned long cascade_irq;
> > + unsigned int msir[NR_MSIR];
> > +
> > + u32 msi_addr_lo;
> > + u32 msi_addr_hi;
> > + void __iomem *msi_regs;
> > + u32 irq_count;
> > + u32 feature;
> > +
> > + spinlock_t fsl_msi_lock;
>=20
> Unused?
>=20
> > + unsigned long *fsl_msi_bitmap;
>=20
> Do we need fsl_msi in the name?
>=20
> > + spinlock_t bitmap_lock;
> > + const char *name;
>=20
> Unused?
>=20
> > +};
> > +
> > +struct fsl_msi_data {
> > + u32 fsl_pic_ip;
> > + u32 msiir_offset;
> > +};
> > +
> > +#endif /* _ASM_POWERPC_MSI_H */
> > +
> > diff --git a/arch/powerpc/sysdev/fsl_pci.c=20
> b/arch/powerpc/sysdev/fsl_pci.c
> > index bf13c21..fede767 100644
> > --- a/arch/powerpc/sysdev/fsl_pci.c
> > +++ b/arch/powerpc/sysdev/fsl_pci.c
> > @@ -106,6 +106,16 @@ void __init setup_pci_cmd(struct=20
> pci_controller *hose)
> > }
> > }
> > =20
> > +#ifdef CONFIG_PCI_MSI
> > +void __init setup_pci_pcsrbar(struct pci_controller *hose)
> > +{
> > + phys_addr_t immr_base;
> > +
> > + immr_base =3D get_immrbase();
> > + early_write_config_dword(hose, 0, 0,=20
> PCI_BASE_ADDRESS_0, immr_base);
> > +}
> > +#endif
>=20
> Up to you, but I prefer an empty static inline here which=20
> saves an #ifdef
> at the call site.
>=20
> > static int fsl_pcie_bus_fixup;
> > =20
> > static void __init quirk_fsl_pcie_header(struct pci_dev *dev)
> > @@ -211,6 +221,10 @@ int __init fsl_add_bridge(struct=20
> device_node *dev, int is_primary)
> > /* Setup PEX window registers */
> > setup_pci_atmu(hose, &rsrc);
> > =20
> > + /*Setup PEXCSRBAR */
> > +#ifdef CONFIG_PCI_MSI
> > + setup_pci_pcsrbar(hose);
> > +#endif
> > return 0;
> > }
>=20
>=20
> cheers
>=20
^ permalink raw reply
* Re: linux-next: x86-latest/powerpc-next merge conflict
From: Alexander van Heukelum @ 2008-04-21 11:19 UTC (permalink / raw)
To: Ingo Molnar, Stephen Rothwell; +Cc: linuxppc-dev, linux-next, Paul Mackerras
In-Reply-To: <20080421095102.GB1666@elte.hu>
On Mon, 21 Apr 2008 11:51:02 +0200, "Ingo Molnar" <mingo@elte.hu> said:
>
> * Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> > Hi all,
> >
> > Today's linux-next merge of the x86-latest tree got a conflict in
> > include/asm-powerpc/bitops.h between commit
> > cd008c0f03f3d451e5fbd108b8e74079d402be64 ("generic: implement __fls on
> > all 64-bit archs") from the x86-latest tree and commit
> > 9f264be6101c42cb9e471c58322fb83a5cde1461 ("[POWERPC] Optimize fls64()
> > on 64-bit processors") from the powerpc-next tree. The fixup was not
> > quite trivial and is worth a look to see if I got it right.
Powerpc would pick up an optimized version via this chain: generic fls64
->
powerpc __fls --> __ilog2 --> asm (PPC_CNTLZL "%0,%1" : "=r" (lz) : "r"
(x)).
However, the generic version of fls64 first tests the argument for zero.
From
your code I derive that the count-leading-zeroes instruction for
argument zero
is defined as cntlzl(0) == BITS_PER_LONG. In that case the explicit test
for zero is not needed, which makes the powerpc-specific one added here
an improvement over the generic one.
I've tried to take a look if you got it right, but the linux-next tree
on git.kernel.org is 5 days old. If that's the current state then it's
not merged right ;).
Greetings,
Alexander
> Paul, do you agree with those generic bitops changes? Just in case it's
> not obvious from previous discussions: we'll push them upstream via a
> separate pull request, not via usual x86.git changes. They originated
> from x86.git but grew into a more generic improvement for all. They sit
> in x86.git for tester convenience but are of course not pure x86 changes
> anymore.
>
> Ingo
--
Alexander van Heukelum
heukelum@fastmail.fm
--
http://www.fastmail.fm - A fast, anti-spam email service.
^ permalink raw reply
* Re: linux-next: x86-latest/powerpc-next merge conflict
From: Alexander van Heukelum @ 2008-04-21 11:30 UTC (permalink / raw)
To: Alexander van Heukelum, Ingo Molnar, Stephen Rothwell
Cc: linuxppc-dev, linux-next, Paul Mackerras
In-Reply-To: <1208776790.4613.1248992953@webmail.messagingengine.com>
On Mon, 21 Apr 2008 13:19:50 +0200, "Alexander van Heukelum"
<heukelum@fastmail.fm> said:
> On Mon, 21 Apr 2008 11:51:02 +0200, "Ingo Molnar" <mingo@elte.hu> said:
> > * Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> >
> > > Hi all,
> > >
> > > Today's linux-next merge of the x86-latest tree got a conflict in
> > > include/asm-powerpc/bitops.h between commit
> > > cd008c0f03f3d451e5fbd108b8e74079d402be64 ("generic: implement __fls on
> > > all 64-bit archs") from the x86-latest tree and commit
> > > 9f264be6101c42cb9e471c58322fb83a5cde1461 ("[POWERPC] Optimize fls64()
> > > on 64-bit processors") from the powerpc-next tree. The fixup was not
> > > quite trivial and is worth a look to see if I got it right.
>
> [...]
>
> I've tried to take a look if you got it right, but the linux-next tree
> on git.kernel.org is 5 days old. If that's the current state then it's
> not merged right ;).
And it just started showing the new version. The merge went fine.
> Greetings,
> Alexander
>
> > Paul, do you agree with those generic bitops changes? Just in case it's
> > not obvious from previous discussions: we'll push them upstream via a
> > separate pull request, not via usual x86.git changes. They originated
> > from x86.git but grew into a more generic improvement for all. They sit
> > in x86.git for tester convenience but are of course not pure x86 changes
> > anymore.
> >
> > Ingo
--
Alexander van Heukelum
heukelum@fastmail.fm
--
http://www.fastmail.fm - A fast, anti-spam email service.
^ permalink raw reply
* Re: pci issue - wrong detection of pci ressources
From: Christian Ehrhardt @ 2008-04-21 11:55 UTC (permalink / raw)
To: benh; +Cc: Hollis Blanchard, linuxppc-dev
In-Reply-To: <1208727408.7009.0.camel@pasglop>
[-- Attachment #1: Type: text/plain, Size: 2038 bytes --]
Benjamin Herrenschmidt wrote:
>> Yes you're right. Early at the pci initialization are errors of the allocation for pi ressources.
>> And that are exactly the ressources failing later, so that pci initialization seem to be the reason for my problem.
>> Was there any simple solution (e.g. just somehow increase memory reserved for pci) when you came across that issue Johan ?
>
> Hrm... I was expecting to see a lot more output here, make sure you have
> "debug" on your command line (or enable early debug output, same
> effect).
There is nothing more even with debug in kernel command line.
But I added some printk's around the resource handling to get a better feeling what's happening.
I attached the new extended bootlog and the patch with the printk's for a better understanding where which message is printed.
I tried to understand how I might e.g. increase the number of available resources or their size, but unfortunately that is not the simplest code when working with it the first time ;-)
> Cheers,
> Ben.
>
>
For comparison I defined DEBUG in the good kernel (arch=ppc) and that is what the initialization prints (pci ...:0a:1 is the secondary head of the same graphic card an it's not an issue if thats not allocated):
good case:
PCI: Probing PCI hardware
PCI: bridge rsrc 0..ffff (100), parent c0354624
PCI: bridge rsrc 80000000..8fffffff (200), parent c0354608
PCI:0000:00:0a.0: Resource 0: 0000000088000000-000000008fffffff (f=1208)
PCI:0000:00:0a.0: Resource 1: 000000000000ff00-000000000000ffff (f=101)
PCI:0000:00:0a.0: Resource 2: 0000000087ff0000-0000000087ffffff (f=200)
PCI:0000:00:0a.1: Resource 0: 0000000078000000-000000007fffffff (f=1208)
PCI: Cannot allocate resource region 0 of device 0000:00:0a.1
PCI:0000:00:0a.1: Resource 1: 0000000077ff0000-0000000077ffffff (f=200)
PCI: Cannot allocate resource region 1 of device 0000:00:0a.1
PCI: Failed to allocate mem resource #0:8000000@90000000 for 0000:00:0a.1
--
Grüsse / regards,
Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization
[-- Attachment #2: pci_resource_issue_bootmsg_badkernel.txt --]
[-- Type: text/plain, Size: 4899 bytes --]
PCI host bridge /plb/pci@1ec000000 (primary) ranges:
MEM 0x0000000180000000..0x000000018fffffff -> 0x0000000080000000
IO 0x00000001e8000000..0x00000001e80fffff -> 0x0000000000000000
4xx PCI DMA offset set to 0x00000000
PCI: Probing PCI hardware
PCI: Hiding 4xx host bridge resources 0000:00:00.0
Try to map irq for 0000:00:00.0...
-> got one, spec 2 cells (0x00000003 0x00000008...) on /interrupt-controller2
-> mapped to linux irq 16
Try to map irq for 0000:00:0a.0...
-> got one, spec 2 cells (0x00000003 0x00000008...) on /interrupt-controller2
-> mapped to linux irq 16
Try to map irq for 0000:00:0a.1...
PCI: PHB (bus 0) bridge rsrc 0: 0000000000000000-00000000000fffff [0x100], parent c0365060 (PCI IO)
__request_resource - request cf8045b0 name '/plb/pci@1ec000000' start 0x0 end 0x0
__request_resource - no conflict parent c0365060 sibling 00000000
PCI: PHB (bus 0) bridge rsrc 1: 0000000180000000-000000018fffffff [0x200], parent c0365038 (PCI mem)
__request_resource - request cf8045d8 name '/plb/pci@1ec000000' start 0x1 end 0x80000000
__request_resource - no conflict parent c0365038 sibling 00000000
PCI: Assigning unassigned resouces...
pci_assign_resource - allocate with IORESOURCE_PREFETCH
pci_bus_alloc_resource - enter
pci_assign_resource - second pci_bus_alloc_resource call
pci_bus_alloc_resource - enter
pci_bus_alloc_resource - call allocate ressource size 0 startcalc 134217728, align -1
find_resource - size 0, min 0x8000000, max 0x1
find_resource - found start 0x1 end 0x80000000
__request_resource - request cf810578 name '0000:00:0a.0' start 0x1 end 0x80000000
__request_resource - no conflict parent cf8045d8 sibling 00000000
pci_assign_resource - allocate with IORESOURCE_PREFETCH
pci_bus_alloc_resource - enter
pci_assign_resource - second pci_bus_alloc_resource call
pci_bus_alloc_resource - enter
pci_bus_alloc_resource - call allocate ressource size 0 startcalc 134217728, align -1
find_resource - size 0, min 0x8000000, max 0x1
find_resource - continue with start 0x1 on 88000000
find_resource - found start 0x1 end 0x88000000
__request_resource - request cf810178 name '0000:00:0a.1' start 0x1 end 0x88000000
__request_resource - no conflict parent cf8045d8 sibling 00000000
pci_assign_resource - allocate with IORESOURCE_PREFETCH
pci_bus_alloc_resource - enter
pci_assign_resource - second pci_bus_alloc_resource call
pci_bus_alloc_resource - enter
pci_bus_alloc_resource - call allocate ressource size 0 startcalc 131072, align -1
find_resource - size 0, min 0x20000, max 0x1
find_resource - continue with start 0x1 on 88000000
find_resource - continue with start 0x1 on 90000000
find_resource - no this - exit
PCI: pci_assign_resource - Failed to allocate mem resource #6:20000@190000000 for 0000:00:0a.0
pci_assign_resource - allocate with IORESOURCE_PREFETCH
pci_bus_alloc_resource - enter
pci_bus_alloc_resource - call allocate ressource size 0 startcalc 65536, align -1
find_resource - size 0, min 0x10000, max 0x1
find_resource - continue with start 0x1 on 88000000
find_resource - continue with start 0x1 on 90000000
find_resource - no this - exit
PCI: pci_assign_resource - Failed to allocate mem resource #2:10000@190000000 for 0000:00:0a.0
pci_assign_resource - allocate with IORESOURCE_PREFETCH
pci_bus_alloc_resource - enter
pci_bus_alloc_resource - call allocate ressource size 0 startcalc 65536, align -1
find_resource - size 0, min 0x10000, max 0x1
find_resource - continue with start 0x1 on 88000000
find_resource - continue with start 0x1 on 90000000
find_resource - no this - exit
PCI: pci_assign_resource - Failed to allocate mem resource #1:10000@190000000 for 0000:00:0a.1
pci_assign_resource - allocate with IORESOURCE_PREFETCH
pci_bus_alloc_resource - enter
pci_bus_alloc_resource - call allocate ressource size 0 startcalc 256, align -1
find_resource - size 0, min 0x100, max 0x0
find_resource - found start 0x0 end 0x1000
__request_resource - request cf8105a0 name '0000:00:0a.0' start 0x0 end 0x1000
__request_resource - no conflict parent cf8045b0 sibling 00000000
SCSI subsystem initialized
[...]
radeonfb_pci_register BEGIN
radeonfb_pci_register - resource0 0x1 - 0x80000000
radeonfb_pci_register - resource1 0x0 - 0x1000
radeonfb_pci_register - resource2 0x0 - 0x0
radeonfb_pci_register - got pci ressources fb_base_phys 0x1 mmio_base_phys 0x80000000
__request_resource - request cf8bfd20 name 'radeonfb framebuffer' start 0x1 end 0x80000000
__request_resource - request cf8bfd20 name 'radeonfb framebuffer' start 0x1 end 0x80000000
__request_resource - request cf8bfd20 name 'radeonfb framebuffer' start 0x1 end 0x80000000
__request_resource - no conflict parent cf810578 sibling 00000000
radeonfb_pci_register - call ioremap for base 0 and size 0
__ioremap(): phys addr 0x0 is RAM lr c029df88 - mem_init_done 1 highmem@10000000
radeonfb (0000:00:0a.0): cannot map MMIO
radeonfb: probe of 0000:00:0a.0 failed with error -5
[-- Attachment #3: debug_pci_radeonfb.diff --]
[-- Type: text/x-patch, Size: 7931 bytes --]
Subject: [PATCH] : ! debug only ! - printk's in pci ressource handling
From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
This defines debug for pci and put's some printk's to keep track of the pci
ressource handling. This patch is just for reference to understand the
resulting boot message log.
The patch is ugly - but for debugging only ;-)
Signed-off-by: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
---
[diffstat]
arch/powerpc/kernel/pci-common.c | 2 ++
arch/powerpc/mm/pgtable_32.c | 4 ++--
drivers/pci/bus.c | 3 +++
drivers/pci/setup-res.c | 12 ++++++++----
drivers/video/aty/radeon_base.c | 7 +++++++
drivers/video/aty/radeonfb.h | 4 ++--
kernel/resource.c | 17 ++++++++++++++++-
7 files changed, 40 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -37,6 +37,8 @@
#include <asm/machdep.h>
#include <asm/ppc-pci.h>
#include <asm/firmware.h>
+
+#define DEBUG
#ifdef DEBUG
#include <asm/udbg.h>
diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
--- a/arch/powerpc/mm/pgtable_32.c
+++ b/arch/powerpc/mm/pgtable_32.c
@@ -183,8 +183,8 @@ __ioremap(phys_addr_t addr, unsigned lon
* mem_init() sets high_memory so only do the check after that.
*/
if (mem_init_done && (p < virt_to_phys(high_memory))) {
- printk("__ioremap(): phys addr 0x%llx is RAM lr %p\n",
- (unsigned long long)p, __builtin_return_address(0));
+ printk("__ioremap(): phys addr 0x%llx is RAM lr %p - mem_init_done %d highmem@%p\n",
+ (unsigned long long)p, __builtin_return_address(0), mem_init_done, virt_to_phys(high_memory));
return NULL;
}
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -42,6 +42,8 @@ pci_bus_alloc_resource(struct pci_bus *b
{
int i, ret = -ENOMEM;
+printk(KERN_ERR"%s - enter\n", __func__);
+
type_mask |= IORESOURCE_IO | IORESOURCE_MEM;
for (i = 0; i < PCI_BUS_NUM_RESOURCES; i++) {
@@ -59,6 +61,7 @@ pci_bus_alloc_resource(struct pci_bus *b
!(res->flags & IORESOURCE_PREFETCH))
continue;
+printk(KERN_ERR"%s - call allocate ressource size %d startcalc %d, align %d\n", __func__, size, (r->start ? : min,-1), align);
/* Ok, try it out.. */
ret = allocate_resource(r, res, size,
r->start ? : min,
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -142,10 +142,13 @@ int pci_assign_resource(struct pci_dev *
required alignment in the "start" field. */
align = (resno < PCI_BRIDGE_RESOURCES) ? size : res->start;
+printk(KERN_ERR"%s - allocate with IORESOURCE_PREFETCH\n", __func__);
+
/* First, try exact prefetching match.. */
ret = pci_bus_alloc_resource(bus, res, size, align, min,
IORESOURCE_PREFETCH,
pcibios_align_resource, dev);
+
if (ret < 0 && (res->flags & IORESOURCE_PREFETCH)) {
/*
@@ -154,13 +157,14 @@ int pci_assign_resource(struct pci_dev *
* But a prefetching area can handle a non-prefetching
* window (it will just not perform as well).
*/
+printk(KERN_ERR"%s - second pci_bus_alloc_resource call\n", __func__);
ret = pci_bus_alloc_resource(bus, res, size, align, min, 0,
pcibios_align_resource, dev);
}
if (ret) {
- printk(KERN_ERR "PCI: Failed to allocate %s resource "
- "#%d:%llx@%llx for %s\n",
+ printk(KERN_ERR "PCI: %s - Failed to allocate %s resource "
+ "#%d:%llx@%llx for %s\n", __func__,
res->flags & IORESOURCE_IO ? "I/O" : "mem",
resno, (unsigned long long)size,
(unsigned long long)res->start, pci_name(dev));
@@ -197,8 +201,8 @@ int pci_assign_resource_fixed(struct pci
}
if (ret) {
- printk(KERN_ERR "PCI: Failed to allocate %s resource "
- "#%d:%llx@%llx for %s\n",
+ printk(KERN_ERR "PCI: %s - Failed to allocate %s resource "
+ "#%d:%llx@%llx for %s\n", __func__,
res->flags & IORESOURCE_IO ? "I/O" : "mem",
resno, (unsigned long long)(res->end - res->start + 1),
(unsigned long long)res->start, pci_name(dev));
diff --git a/drivers/video/aty/radeon_base.c b/drivers/video/aty/radeon_base.c
--- a/drivers/video/aty/radeon_base.c
+++ b/drivers/video/aty/radeon_base.c
@@ -2160,6 +2160,10 @@ static int __devinit radeonfb_pci_regist
int ret;
RTRACE("radeonfb_pci_register BEGIN\n");
+
+printk(KERN_ERR"%s - resource0 0x%0lx - 0x%0lx\n", __func__, pdev->resource[0].start, pdev->resource[0].end);
+printk(KERN_ERR"%s - resource1 0x%0lx - 0x%0lx\n", __func__, pdev->resource[1].start, pdev->resource[1].end);
+printk(KERN_ERR"%s - resource2 0x%0lx - 0x%0lx\n", __func__, pdev->resource[2].start, pdev->resource[2].end);
/* Enable device in PCI config */
ret = pci_enable_device(pdev);
@@ -2198,6 +2202,8 @@ static int __devinit radeonfb_pci_regist
rinfo->fb_base_phys = pci_resource_start (pdev, 0);
rinfo->mmio_base_phys = pci_resource_start (pdev, 2);
+printk(KERN_ERR"%s - got pci ressources fb_base_phys 0x%0lx mmio_base_phys 0x%0lx\n", __func__,rinfo->fb_base_phys,rinfo->mmio_base_phys);
+
/* request the mem regions */
ret = pci_request_region(pdev, 0, "radeonfb framebuffer");
if (ret < 0) {
@@ -2214,6 +2220,7 @@ static int __devinit radeonfb_pci_regist
}
/* map the regions */
+printk(KERN_ERR "%s - call ioremap for base %ld and size %d\n",__func__, rinfo->mmio_base_phys, RADEON_REGSIZE);
rinfo->mmio_base = ioremap(rinfo->mmio_base_phys, RADEON_REGSIZE);
if (!rinfo->mmio_base) {
printk(KERN_ERR "radeonfb (%s): cannot map MMIO\n",
diff --git a/drivers/video/aty/radeonfb.h b/drivers/video/aty/radeonfb.h
--- a/drivers/video/aty/radeonfb.h
+++ b/drivers/video/aty/radeonfb.h
@@ -287,8 +287,8 @@ struct radeonfb_info {
char name[DEVICE_NAME_SIZE];
- unsigned long mmio_base_phys;
- unsigned long fb_base_phys;
+ resource_size_t mmio_base_phys;
+ resource_size_t fb_base_phys;
void __iomem *mmio_base;
void __iomem *fb_base;
diff --git a/kernel/resource.c b/kernel/resource.c
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -152,6 +152,9 @@ static struct resource * __request_resou
resource_size_t end = new->end;
struct resource *tmp, **p;
+printk(KERN_ERR"%s - request %p name '%s' start 0x%0lx end 0x%0lx\n",
+ __func__, new, new->name, start, end);
+
if (end < start)
return root;
if (start < root->start)
@@ -165,6 +168,7 @@ static struct resource * __request_resou
new->sibling = tmp;
*p = new;
new->parent = root;
+printk(KERN_ERR"%s - no conflict parent %p sibling %p\n",__func__, new->parent, new->sibling);
return NULL;
}
p = &tmp->sibling;
@@ -305,6 +309,8 @@ static int find_resource(struct resource
{
struct resource *this = root->child;
+printk(KERN_ERR"%s - size %lx, min 0x%0lx, max 0x%0lx\n", __func__, size, min, max);
+
new->start = root->start;
/*
* Skip past an allocated resource that starts at 0, since the assignment
@@ -328,10 +334,16 @@ static int find_resource(struct resource
alignf(alignf_data, new, size, align);
if (new->start < new->end && new->end - new->start >= size - 1) {
new->end = new->start + size - 1;
+printk(KERN_ERR"%s - found start 0x%0lx end 0x%0lx\n", __func__, new->start, new->end);
return 0;
}
- if (!this)
+ if (!this) {
+printk(KERN_ERR"%s - no this - exit\n", __func__);
break;
+}
+else {
+printk(KERN_ERR"%s - continue with start 0x%0lx on %p\n", __func__, (this->end + 1), this->sibling);
+}
new->start = this->end + 1;
this = this->sibling;
}
@@ -385,6 +397,9 @@ int insert_resource(struct resource *par
{
int result;
struct resource *first, *next;
+
+printk(KERN_ERR"%s - insert %p (start 0x%0lx end 0x%0lx) under parent %p\n",
+ __func__, new, new->start, new->end, parent);
write_lock(&resource_lock);
^ permalink raw reply
* Re: [PATCH] rtc-pcf8563: Add device ids table
From: Laurent Pinchart @ 2008-04-21 11:56 UTC (permalink / raw)
To: Jochen Friedrich
Cc: Jean Delvare, linuxppc-dev list, Alessandro Zummo, i2c, rtc-linux
In-Reply-To: <480A2117.7020502@scram.de>
[-- Attachment #1: Type: text/plain, Size: 1538 bytes --]
On Saturday 19 April 2008 18:43, Jochen Friedrich wrote:
> Jean Delvare wrote:
> > On Sat, 19 Apr 2008 15:09:55 +0200, Jochen Friedrich wrote:
> > > Hi Jean,
> > >
> > > > > > Is the new-style driver conversion patch in 2.6.25-rc8-mm2
> > > > > > scheduled for 2.6.26 ?
> > > > > hope so! :)
> > > > Note that this patch depends on i2c patches which aren't upstream yet.
> > > Is there any chance they will got to 2.6.26?
> >
> > I'm not sure. I didn't have the time to look at it myself, but I am
> > under the impression that the powerpc folks are tired of having to wait
> > for me and may push it to Linus through their tree? That would be fine
> > with me, as I don't want to be the one delaying something many
> > developers want (but I also can't sign patches I've not even read.)
The patches are required for long awaited I2C support on various PowerPC
boards. As the ppc architecture support is going away in 2.6.27, it would be
nice to have them applied in 2.6.26.
> The required patches are:
>
> http://patchwork.ozlabs.org/linuxppc/patch?id=17833
> http://patchwork.ozlabs.org/linuxppc/patch?id=17834
>
> which are just the forward ported patches you sent to the poweprc mailing
> list some time ago:
>
> http://patchwork.ozlabs.org/linuxppc/patch?id=16282
> http://patchwork.ozlabs.org/linuxppc/patch?id=16283
>
> Thanks,
> Jochen
--
Laurent Pinchart
CSE Semaphore Belgium
Chaussee de Bruxelles, 732A
B-1410 Waterloo
Belgium
T +32 (2) 387 42 59
F +32 (2) 387 42 75
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: linux-next: x86-latest/powerpc-next merge conflict
From: Paul Mackerras @ 2008-04-21 12:10 UTC (permalink / raw)
To: Ingo Molnar
Cc: Alexander van Heukelum, Stephen Rothwell, linux-next,
linuxppc-dev
In-Reply-To: <20080421095102.GB1666@elte.hu>
Ingo Molnar writes:
> Paul, do you agree with those generic bitops changes? Just in case it's
Well, it looks OK, but I'm sure people are going to get confused with
fls vs. fls64 vs. __fls all being subtly different. I'd say it's
worth putting a little file in the Documentation directory to explain
it all.
> not obvious from previous discussions: we'll push them upstream via a
> separate pull request, not via usual x86.git changes. They originated
> from x86.git but grew into a more generic improvement for all. They sit
> in x86.git for tester convenience but are of course not pure x86 changes
> anymore.
I'm not sure why the "add __fls to all 64-bit architectures" change
has to be done as a single patch rather than a patch per architecture
going through the architecture maintainers. I suppose that avoids any
problem with some maintainers not sending it upstream quickly. I
would expect that if it is a single cross-architecture patch that it
would go through Andrew Morton, though. But if Andrew wants you to
handle it then I'm happy to give you an Acked-by for it.
Regards,
Paul.
^ permalink raw reply
* Re: linux-next: x86-latest/powerpc-next merge conflict
From: Paul Mackerras @ 2008-04-21 12:13 UTC (permalink / raw)
To: Alexander van Heukelum
Cc: Stephen Rothwell, Ingo Molnar, linux-next, linuxppc-dev
In-Reply-To: <1208776790.4613.1248992953@webmail.messagingengine.com>
Alexander van Heukelum writes:
> Powerpc would pick up an optimized version via this chain: generic fls64
> ->
> powerpc __fls --> __ilog2 --> asm (PPC_CNTLZL "%0,%1" : "=r" (lz) : "r"
> (x)).
Why wouldn't powerpc continue to use the fls64 that I have in there
now?
> However, the generic version of fls64 first tests the argument for zero.
> From
> your code I derive that the count-leading-zeroes instruction for
> argument zero
> is defined as cntlzl(0) == BITS_PER_LONG.
That is correct. If the argument is 0 then all of the zero bits are
leading zeroes. :)
Regards,
Paul.
^ permalink raw reply
* Re: [PATCH] rtc-pcf8563: Add device ids table
From: Stefan Roese @ 2008-04-21 12:12 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Alessandro Zummo, rtc-linux, i2c, Jean Delvare
In-Reply-To: <200804211356.10192.laurentp@cse-semaphore.com>
On Monday 21 April 2008, Laurent Pinchart wrote:
> > > > Is there any chance they will got to 2.6.26?
> > >
> > > I'm not sure. I didn't have the time to look at it myself, but I am
> > > under the impression that the powerpc folks are tired of having to wait
> > > for me and may push it to Linus through their tree? That would be fine
> > > with me, as I don't want to be the one delaying something many
> > > developers want (but I also can't sign patches I've not even read.)
>
> The patches are required for long awaited I2C support on various PowerPC
> boards. As the ppc architecture support is going away in 2.6.27, it would
> be nice to have them applied in 2.6.26.
Yes, please.
Best regards,
Stefan
^ permalink raw reply
* Re: pci issue - wrong detection of pci ressources
From: Sergei Shtylyov @ 2008-04-21 12:25 UTC (permalink / raw)
To: Christian Ehrhardt; +Cc: linuxppc-dev, Hollis Blanchard
In-Reply-To: <480C80B6.30904@linux.vnet.ibm.com>
Hello.
Christian Ehrhardt wrote:
>> Cheers,
>> Ben.
> For comparison I defined DEBUG in the good kernel (arch=ppc) and that is
> what the initialization prints (pci ...:0a:1 is the secondary head of
> the same graphic card an it's not an issue if thats not allocated):
> good case:
> PCI: Probing PCI hardware
> PCI: bridge rsrc 0..ffff (100), parent c0354624
> PCI: bridge rsrc 80000000..8fffffff (200), parent c0354608
> PCI:0000:00:0a.0: Resource 0: 0000000088000000-000000008fffffff (f=1208)
> PCI:0000:00:0a.0: Resource 1: 000000000000ff00-000000000000ffff (f=101)
> PCI:0000:00:0a.0: Resource 2: 0000000087ff0000-0000000087ffffff (f=200)
> PCI:0000:00:0a.1: Resource 0: 0000000078000000-000000007fffffff (f=1208)
> PCI: Cannot allocate resource region 0 of device 0000:00:0a.1
> PCI:0000:00:0a.1: Resource 1: 0000000077ff0000-0000000077ffffff (f=200)
> PCI: Cannot allocate resource region 1 of device 0000:00:0a.1
> PCI: Failed to allocate mem resource #0:8000000@90000000 for 0000:00:0a.1
> ------------------------------------------------------------------------
> PCI host bridge /plb/pci@1ec000000 (primary) ranges:
> MEM 0x0000000180000000..0x000000018fffffff -> 0x0000000080000000
> IO 0x00000001e8000000..0x00000001e80fffff -> 0x0000000000000000
> 4xx PCI DMA offset set to 0x00000000
> PCI: Probing PCI hardware
> PCI: Hiding 4xx host bridge resources 0000:00:00.0
> Try to map irq for 0000:00:00.0...
> -> got one, spec 2 cells (0x00000003 0x00000008...) on /interrupt-controller2
> -> mapped to linux irq 16
> Try to map irq for 0000:00:0a.0...
> -> got one, spec 2 cells (0x00000003 0x00000008...) on /interrupt-controller2
> -> mapped to linux irq 16
> Try to map irq for 0000:00:0a.1...
> PCI: PHB (bus 0) bridge rsrc 0: 0000000000000000-00000000000fffff [0x100], parent c0365060 (PCI IO)
> __request_resource - request cf8045b0 name '/plb/pci@1ec000000' start 0x0 end 0x0
> __request_resource - no conflict parent c0365060 sibling 00000000
> PCI: PHB (bus 0) bridge rsrc 1: 0000000180000000-000000018fffffff [0x200], parent c0365038 (PCI mem)
> __request_resource - request cf8045d8 name '/plb/pci@1ec000000' start 0x1 end 0x80000000
Bogus start/end value because you're not printing them right...
> __request_resource - no conflict parent c0365038 sibling 00000000
> PCI: Assigning unassigned resouces...
> pci_assign_resource - allocate with IORESOURCE_PREFETCH
> pci_bus_alloc_resource - enter
> pci_assign_resource - second pci_bus_alloc_resource call
> pci_bus_alloc_resource - enter
> pci_bus_alloc_resource - call allocate ressource size 0 startcalc 134217728, align -1
> find_resource - size 0, min 0x8000000, max 0x1
> find_resource - found start 0x1 end 0x80000000
> __request_resource - request cf810578 name '0000:00:0a.0' start 0x1 end 0x80000000
The start/end values are bogus -- not printed right.
> __request_resource - no conflict parent cf8045d8 sibling 00000000
> pci_assign_resource - allocate with IORESOURCE_PREFETCH
> pci_bus_alloc_resource - enter
> pci_assign_resource - second pci_bus_alloc_resource call
> pci_bus_alloc_resource - enter
> pci_bus_alloc_resource - call allocate ressource size 0 startcalc 134217728, align -1
> find_resource - size 0, min 0x8000000, max 0x1
> find_resource - continue with start 0x1 on 88000000
> find_resource - found start 0x1 end 0x88000000
> __request_resource - request cf810178 name '0000:00:0a.1' start 0x1 end 0x88000000
Same here.
> __request_resource - no conflict parent cf8045d8 sibling 00000000
> pci_assign_resource - allocate with IORESOURCE_PREFETCH
It's not clear why the register space is prefetchable.
> pci_bus_alloc_resource - enter
> pci_assign_resource - second pci_bus_alloc_resource call
> pci_bus_alloc_resource - enter
> pci_bus_alloc_resource - call allocate ressource size 0 startcalc 131072, align -1
> find_resource - size 0, min 0x20000, max 0x1
> find_resource - continue with start 0x1 on 88000000
> find_resource - continue with start 0x1 on 90000000
> find_resource - no this - exit
... and here. It's not clear why it failed with 188000000... maybe we ran
out of prefetchable space?
> PCI: pci_assign_resource - Failed to allocate mem resource #6:20000@190000000 for 0000:00:0a.0
> pci_assign_resource - allocate with IORESOURCE_PREFETCH
> pci_bus_alloc_resource - enter
> pci_bus_alloc_resource - call allocate ressource size 0 startcalc 65536, align -1
> find_resource - size 0, min 0x10000, max 0x1
> find_resource - continue with start 0x1 on 88000000
> find_resource - continue with start 0x1 on 90000000
> find_resource - no this - exit
> PCI: pci_assign_resource - Failed to allocate mem resource #2:10000@190000000 for 0000:00:0a.0
> pci_assign_resource - allocate with IORESOURCE_PREFETCH
> pci_bus_alloc_resource - enter
> pci_bus_alloc_resource - call allocate ressource size 0 startcalc 65536, align -1
> find_resource - size 0, min 0x10000, max 0x1
> find_resource - continue with start 0x1 on 88000000
> find_resource - continue with start 0x1 on 90000000
> find_resource - no this - exit
> PCI: pci_assign_resource - Failed to allocate mem resource #1:10000@190000000 for 0000:00:0a.1
> pci_assign_resource - allocate with IORESOURCE_PREFETCH
> pci_bus_alloc_resource - enter
> pci_bus_alloc_resource - call allocate ressource size 0 startcalc 256, align -1
> find_resource - size 0, min 0x100, max 0x0
> find_resource - found start 0x0 end 0x1000
> __request_resource - request cf8105a0 name '0000:00:0a.0' start 0x0 end 0x1000
> __request_resource - no conflict parent cf8045b0 sibling 00000000
> SCSI subsystem initialized
> [...]
> radeonfb_pci_register BEGIN
> radeonfb_pci_register - resource0 0x1 - 0x80000000
> radeonfb_pci_register - resource1 0x0 - 0x1000
> radeonfb_pci_register - resource2 0x0 - 0x0
The value here are bugus because you're not printing them right.
> radeonfb_pci_register - got pci ressources fb_base_phys 0x1 mmio_base_phys 0x80000000
> __request_resource - request cf8bfd20 name 'radeonfb framebuffer' start 0x1 end 0x80000000
> __request_resource - request cf8bfd20 name 'radeonfb framebuffer' start 0x1 end 0x80000000
> __request_resource - request cf8bfd20 name 'radeonfb framebuffer' start 0x1 end 0x80000000
> __request_resource - no conflict parent cf810578 sibling 00000000
> radeonfb_pci_register - call ioremap for base 0 and size 0
> __ioremap(): phys addr 0x0 is RAM lr c029df88 - mem_init_done 1 highmem@10000000
> radeonfb (0000:00:0a.0): cannot map MMIO
> radeonfb: probe of 0000:00:0a.0 failed with error -5
> ------------------------------------------------------------------------
> Subject: [PATCH] : ! debug only ! - printk's in pci ressource handling
> From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
> This defines debug for pci and put's some printk's to keep track of the pci
> ressource handling. This patch is just for reference to understand the
> resulting boot message log.
> The patch is ugly - but for debugging only ;-)
Moreover, it's inconsistent because you're not using the right format for
printing throughout it...
> Signed-off-by: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
[...]
> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -37,6 +37,8 @@
> #include <asm/machdep.h>
> #include <asm/ppc-pci.h>
> #include <asm/firmware.h>
> +
> +#define DEBUG
>
> #ifdef DEBUG
> #include <asm/udbg.h>
> diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
> --- a/arch/powerpc/mm/pgtable_32.c
> +++ b/arch/powerpc/mm/pgtable_32.c
> @@ -183,8 +183,8 @@ __ioremap(phys_addr_t addr, unsigned lon
> * mem_init() sets high_memory so only do the check after that.
> */
> if (mem_init_done && (p < virt_to_phys(high_memory))) {
> - printk("__ioremap(): phys addr 0x%llx is RAM lr %p\n",
> - (unsigned long long)p, __builtin_return_address(0));
> + printk("__ioremap(): phys addr 0x%llx is RAM lr %p - mem_init_done %d highmem@%p\n",
> + (unsigned long long)p, __builtin_return_address(0), mem_init_done, virt_to_phys(high_memory));
> return NULL;
> }
>
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -42,6 +42,8 @@ pci_bus_alloc_resource(struct pci_bus *b
> {
> int i, ret = -ENOMEM;
>
> +printk(KERN_ERR"%s - enter\n", __func__);
> +
> type_mask |= IORESOURCE_IO | IORESOURCE_MEM;
>
> for (i = 0; i < PCI_BUS_NUM_RESOURCES; i++) {
> @@ -59,6 +61,7 @@ pci_bus_alloc_resource(struct pci_bus *b
> !(res->flags & IORESOURCE_PREFETCH))
> continue;
>
> +printk(KERN_ERR"%s - call allocate ressource size %d startcalc %d, align %d\n", __func__, size, (r->start ? : min,-1), align);
> /* Ok, try it out.. */
> ret = allocate_resource(r, res, size,
> r->start ? : min,
> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> --- a/drivers/pci/setup-res.c
> +++ b/drivers/pci/setup-res.c
> @@ -142,10 +142,13 @@ int pci_assign_resource(struct pci_dev *
> required alignment in the "start" field. */
> align = (resno < PCI_BRIDGE_RESOURCES) ? size : res->start;
>
> +printk(KERN_ERR"%s - allocate with IORESOURCE_PREFETCH\n", __func__);
> +
> /* First, try exact prefetching match.. */
> ret = pci_bus_alloc_resource(bus, res, size, align, min,
> IORESOURCE_PREFETCH,
> pcibios_align_resource, dev);
> +
>
> if (ret < 0 && (res->flags & IORESOURCE_PREFETCH)) {
> /*
> @@ -154,13 +157,14 @@ int pci_assign_resource(struct pci_dev *
> * But a prefetching area can handle a non-prefetching
> * window (it will just not perform as well).
> */
> +printk(KERN_ERR"%s - second pci_bus_alloc_resource call\n", __func__);
> ret = pci_bus_alloc_resource(bus, res, size, align, min, 0,
> pcibios_align_resource, dev);
> }
>
> if (ret) {
> - printk(KERN_ERR "PCI: Failed to allocate %s resource "
> - "#%d:%llx@%llx for %s\n",
> + printk(KERN_ERR "PCI: %s - Failed to allocate %s resource "
> + "#%d:%llx@%llx for %s\n", __func__,
> res->flags & IORESOURCE_IO ? "I/O" : "mem",
> resno, (unsigned long long)size,
> (unsigned long long)res->start, pci_name(dev));
> @@ -197,8 +201,8 @@ int pci_assign_resource_fixed(struct pci
> }
>
> if (ret) {
> - printk(KERN_ERR "PCI: Failed to allocate %s resource "
> - "#%d:%llx@%llx for %s\n",
> + printk(KERN_ERR "PCI: %s - Failed to allocate %s resource "
> + "#%d:%llx@%llx for %s\n", __func__,
> res->flags & IORESOURCE_IO ? "I/O" : "mem",
> resno, (unsigned long long)(res->end - res->start + 1),
> (unsigned long long)res->start, pci_name(dev));
> diff --git a/drivers/video/aty/radeon_base.c b/drivers/video/aty/radeon_base.c
> --- a/drivers/video/aty/radeon_base.c
> +++ b/drivers/video/aty/radeon_base.c
> @@ -2160,6 +2160,10 @@ static int __devinit radeonfb_pci_regist
> int ret;
>
> RTRACE("radeonfb_pci_register BEGIN\n");
> +
> +printk(KERN_ERR"%s - resource0 0x%0lx - 0x%0lx\n", __func__, pdev->resource[0].start, pdev->resource[0].end);
> +printk(KERN_ERR"%s - resource1 0x%0lx - 0x%0lx\n", __func__, pdev->resource[1].start, pdev->resource[1].end);
> +printk(KERN_ERR"%s - resource2 0x%0lx - 0x%0lx\n", __func__, pdev->resource[2].start, pdev->resource[2].end);
You should have used %llx here -- the resources are 64-bit on 44x.
> /* Enable device in PCI config */
> ret = pci_enable_device(pdev);
> @@ -2198,6 +2202,8 @@ static int __devinit radeonfb_pci_regist
> rinfo->fb_base_phys = pci_resource_start (pdev, 0);
> rinfo->mmio_base_phys = pci_resource_start (pdev, 2);
>
> +printk(KERN_ERR"%s - got pci ressources fb_base_phys 0x%0lx mmio_base_phys 0x%0lx\n", __func__,rinfo->fb_base_phys,rinfo->mmio_base_phys);
> +
You've changed fb_base_phys and mmio_base_phys to resource_size_t which is
64-bit, so use %llx to print them.
> /* request the mem regions */
> ret = pci_request_region(pdev, 0, "radeonfb framebuffer");
> if (ret < 0) {
> @@ -2214,6 +2220,7 @@ static int __devinit radeonfb_pci_regist
> }
>
> /* map the regions */
> +printk(KERN_ERR "%s - call ioremap for base %ld and size %d\n",__func__, rinfo->mmio_base_phys, RADEON_REGSIZE);
You've changed mmio_base_phys to resource_size_t which is 64-bit, so use
%llx to print it.
> rinfo->mmio_base = ioremap(rinfo->mmio_base_phys, RADEON_REGSIZE);
> if (!rinfo->mmio_base) {
> printk(KERN_ERR "radeonfb (%s): cannot map MMIO\n",
> diff --git a/drivers/video/aty/radeonfb.h b/drivers/video/aty/radeonfb.h
> --- a/drivers/video/aty/radeonfb.h
> +++ b/drivers/video/aty/radeonfb.h
> @@ -287,8 +287,8 @@ struct radeonfb_info {
>
> char name[DEVICE_NAME_SIZE];
>
> - unsigned long mmio_base_phys;
> - unsigned long fb_base_phys;
> + resource_size_t mmio_base_phys;
> + resource_size_t fb_base_phys;
>
> void __iomem *mmio_base;
> void __iomem *fb_base;
> diff --git a/kernel/resource.c b/kernel/resource.c
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -152,6 +152,9 @@ static struct resource * __request_resou
> resource_size_t end = new->end;
> struct resource *tmp, **p;
>
> +printk(KERN_ERR"%s - request %p name '%s' start 0x%0lx end 0x%0lx\n",
> + __func__, new, new->name, start, end);
> +
You should have used %llx here too...
> if (end < start)
> return root;
> if (start < root->start)
> @@ -165,6 +168,7 @@ static struct resource * __request_resou
> new->sibling = tmp;
> *p = new;
> new->parent = root;
> +printk(KERN_ERR"%s - no conflict parent %p sibling %p\n",__func__, new->parent, new->sibling);
> return NULL;
> }
> p = &tmp->sibling;
> @@ -305,6 +309,8 @@ static int find_resource(struct resource
> {
> struct resource *this = root->child;
>
> +printk(KERN_ERR"%s - size %lx, min 0x%0lx, max 0x%0lx\n", __func__, size, min, max);
> +
Should have used %0llx here as well...
> new->start = root->start;
> /*
> * Skip past an allocated resource that starts at 0, since the assignment
> @@ -328,10 +334,16 @@ static int find_resource(struct resource
> alignf(alignf_data, new, size, align);
> if (new->start < new->end && new->end - new->start >= size - 1) {
> new->end = new->start + size - 1;
> +printk(KERN_ERR"%s - found start 0x%0lx end 0x%0lx\n", __func__, new->start, new->end);
And here too...
> return 0;
> }
> - if (!this)
> + if (!this) {
> +printk(KERN_ERR"%s - no this - exit\n", __func__);
> break;
> +}
> +else {
> +printk(KERN_ERR"%s - continue with start 0x%0lx on %p\n", __func__, (this->end + 1), this->sibling);
And here. Yet it's not clear why you call resource's 'end' 'start'...
> +}
> new->start = this->end + 1;
> this = this->sibling;
> }
> @@ -385,6 +397,9 @@ int insert_resource(struct resource *par
> {
> int result;
> struct resource *first, *next;
> +
> +printk(KERN_ERR"%s - insert %p (start 0x%0lx end 0x%0lx) under parent %p\n",
> + __func__, new, new->start, new->end, parent);
Use %llx hereas well...
>
> write_lock(&resource_lock);
>
WBR, Sergei
^ permalink raw reply
* Re: linux-next: x86-latest/powerpc-next merge conflict
From: Alexander van Heukelum @ 2008-04-21 13:07 UTC (permalink / raw)
To: Paul Mackerras; +Cc: Stephen Rothwell, Ingo Molnar, linux-next, linuxppc-dev
In-Reply-To: <18444.34002.74202.564600@cargo.ozlabs.ibm.com>
On Mon, 21 Apr 2008 22:13:06 +1000, "Paul Mackerras" <paulus@samba.org>
said:
> Alexander van Heukelum writes:
> > Powerpc would pick up an optimized version via this chain: generic fls64
> > ->
> > powerpc __fls --> __ilog2 --> asm (PPC_CNTLZL "%0,%1" : "=r" (lz) : "r"
> > (x)).
>
> Why wouldn't powerpc continue to use the fls64 that I have in there
> now?
In Linus' tree that would be the generic one that uses (the 32-bit)
fls():
static inline int fls64(__u64 x)
{
__u32 h = x >> 32;
if (h)
return fls(h) + 32;
return fls(x);
}
> > However, the generic version of fls64 first tests the argument for zero.
> > From
> > your code I derive that the count-leading-zeroes instruction for
> > argument zero
> > is defined as cntlzl(0) == BITS_PER_LONG.
>
> That is correct. If the argument is 0 then all of the zero bits are
> leading zeroes. :)
So... for 64-bit powerpc it makes sense to have its own implementation
and ignore the (improved) generic one and for 32-bit powerpc the generic
implementation of fls64 is fine. The current situation in linux-next
seems
optimal to me.
Greetings,
Alexander
> Regards,
> Paul.
--
Alexander van Heukelum
heukelum@fastmail.fm
--
http://www.fastmail.fm - I mean, what is it about a decent email service?
^ permalink raw reply
* Re: [PATCH 1/2] [v3][POWERPC] refactor dcr code
From: Josh Boyer @ 2008-04-21 13:03 UTC (permalink / raw)
To: Stephen Neuendorffer; +Cc: linuxppc-dev
In-Reply-To: <20080421035624.E528B48806A@mail212-sin.bigfish.com>
On Sun, 20 Apr 2008 20:56:20 -0700
"Stephen Neuendorffer" <stephen.neuendorffer@xilinx.com> wrote:
>
>
> > > +void dcr_unmap_generic(dcr_host_t host, unsigned int dcr_c)
> > > +{
> > > + if (host.type == NATIVE)
> > > + dcr_unmap_native(host.host.native, dcr_c);
> > > + else
> > > + dcr_unmap_mmio(host.host.mmio, dcr_c);
> >
> > What happens if host.type == INVALID? Same question for the other
> > accessors in dcr_*_generic.
>
> I guess looking back on it, I assumed that MAP_OK would return 0, meaning that behavior was undefined,
> but I agree it's probably safer to have some error reporting there... There starts to become a speed tradeoff
> at some point, which would make function pointers more attractive. If the ioremap does fail, or the
> dcr-access-method can't be determined, then dcr_unmap_mmio would probably SEGV anyway, although that's
> not something I'd really want to rely on. I'll put an error case in there.
Well, MAP_OK would return 0, and the caller of the original dcr_map
should probably return an error or something. But there's nothing to
enforce that. Which is true for the current code today as well, so
it's not something you've introduced.
I just thought that if you were going to go to the trouble of defining
an invalid host type, that you'd check for it in the accessor
functions. There is the concern of the speed tradeoffs. I suppose you
could just omit INVALID altogether and have it match the existing code
in behavior if it was too big of a deal.
Ben, any thoughts?
josh
^ permalink raw reply
* Re: [PATCH] [POWERPC] mpc5200: Switch mpc5200 dts files to dts-v1 format
From: Grant Likely @ 2008-04-21 13:21 UTC (permalink / raw)
To: Sascha Hauer; +Cc: linuxppc-dev, m8
In-Reply-To: <20080421084337.GB5618@pengutronix.de>
On Mon, Apr 21, 2008 at 2:43 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Fri, Apr 18, 2008 at 09:10:04AM -0700, Grant Likely wrote:
>
>
> > Update dts files to current format
> >
> > From: Grant Likely <grant.likely@secretlab.ca>
> >
> > Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> > ---
> >
> > arch/powerpc/boot/dts/cm5200.dts | 98 +++++++++++------------
> > arch/powerpc/boot/dts/lite5200.dts | 132 ++++++++++++++++----------------
> > arch/powerpc/boot/dts/lite5200b.dts | 146 +++++++++++++++++------------------
> > arch/powerpc/boot/dts/motionpro.dts | 118 ++++++++++++++--------------
> > arch/powerpc/boot/dts/tqm5200.dts | 80 ++++++++++---------
> > 5 files changed, 286 insertions(+), 288 deletions(-)
> >
> > diff --git a/arch/powerpc/boot/dts/cm5200.dts b/arch/powerpc/boot/dts/cm5200.dts
> > index c6ca631..2f74cc4 100644
> > --- a/arch/powerpc/boot/dts/cm5200.dts
> > +++ b/arch/powerpc/boot/dts/cm5200.dts
> > @@ -10,11 +10,7 @@
> > * option) any later version.
> > */
> >
> > -/*
> > - * WARNING: Do not depend on this tree layout remaining static just yet.
> > - * The MPC5200 device tree conventions are still in flux
> > - * Keep an eye on the linuxppc-dev mailing list for more details
> > - */
> > +/dts-v1/;
> >
> > / {
> > model = "schindler,cm5200";
> > @@ -29,10 +25,10 @@
> > PowerPC,5200@0 {
> > device_type = "cpu";
> > reg = <0>;
> > - d-cache-line-size = <20>;
> > - i-cache-line-size = <20>;
> > - d-cache-size = <4000>; // L1, 16K
> > - i-cache-size = <4000>; // L1, 16K
> > + d-cache-line-size = <32>;
> > + i-cache-line-size = <32>;
>
> Has the standard number format changed to decimal lately? Otherwise this
> looks wrong. There are more occurences of this in the other dts files.
Yes, that is the major change in v1 of the dts format. It uses C
style number format.
Cheers,
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: linux-next: x86-latest/powerpc-next merge conflict
From: Gabriel Paubert @ 2008-04-21 13:36 UTC (permalink / raw)
To: Alexander van Heukelum
Cc: Stephen Rothwell, Ingo Molnar, linux-next, Paul Mackerras,
linuxppc-dev
In-Reply-To: <1208783233.25773.1249008469@webmail.messagingengine.com>
On Mon, Apr 21, 2008 at 03:07:13PM +0200, Alexander van Heukelum wrote:
> On Mon, 21 Apr 2008 22:13:06 +1000, "Paul Mackerras" <paulus@samba.org>
> said:
> > Alexander van Heukelum writes:
> > > Powerpc would pick up an optimized version via this chain: generic fls64
> > > ->
> > > powerpc __fls --> __ilog2 --> asm (PPC_CNTLZL "%0,%1" : "=r" (lz) : "r"
> > > (x)).
> >
> > Why wouldn't powerpc continue to use the fls64 that I have in there
> > now?
>
> In Linus' tree that would be the generic one that uses (the 32-bit)
> fls():
>
> static inline int fls64(__u64 x)
> {
> __u32 h = x >> 32;
> if (h)
> return fls(h) + 32;
> return fls(x);
> }
>
> > > However, the generic version of fls64 first tests the argument for zero.
> > > From
> > > your code I derive that the count-leading-zeroes instruction for
> > > argument zero
> > > is defined as cntlzl(0) == BITS_PER_LONG.
> >
> > That is correct. If the argument is 0 then all of the zero bits are
> > leading zeroes. :)
>
> So... for 64-bit powerpc it makes sense to have its own implementation
> and ignore the (improved) generic one and for 32-bit powerpc the generic
> implementation of fls64 is fine. The current situation in linux-next
> seems
> optimal to me.
Not so sure, the optimal version of fls64 for 32 bit PPC seems to be:
cntlzw ch,h ; ch = fls32(h) where h = x>>32
cntlzw cl,l ; cl = fls32(l) where l = (__u32)x
srwi t1,ch,5
neg t1,t1 ; t1 = (h==0) ? -1 : 0
and cl,t1,cl ; cl = (h==0) ? cl : 0
add result,ch,cl
That's only 6 instructions without any branch, although the dependency
chain is 5 instructions long. Good luck getting the compiler to
generate something as compact as this.
Don't worry about the number of cntlzw, it's one clock on all 32 bit
PPC processors I know, some may even be able to perform 2 or 3 cntlzw
per clock.
Regards,
Gabriel
^ permalink raw reply
* Re: [PATCH] [POWERPC] 52xx: defconfigs for CM5200, Lite5200B, Motion-PRO and TQM5200
From: Bartlomiej Sieka @ 2008-04-21 13:57 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev
In-Reply-To: <20080415103635.9629.55344.stgit@pollux.denx.de>
Bartlomiej Sieka wrote:
> Board-specific defconfigs based on current mpc5200_defconfig, archival
> lite5200_defconfig, and [cm5200|motionpro|tqm5200]_defconfig from the
> linux-2.6-denx tree. Kernels build using these defconfigs were verified
> to boot with root filesystem mounted over NFS on Motion-PRO, TQM5200
> and Lite5200B boards. CM5200 target was not tested due to hardware
> unavailability.
Hello Grant,
There hasn't been any negitve comments on the above patch -- will you
push it upstream via your tree?
Regards,
Bartlomiej
^ permalink raw reply
* Re: [PATCH 1/5] [POWERPC] sysdev: implement FSL GTM support
From: Grant Likely @ 2008-04-21 14:03 UTC (permalink / raw)
To: Anton Vorontsov; +Cc: linuxppc-dev
In-Reply-To: <20080418190908.GA4407@polina.dev.rtsoft.ru>
2008/4/18 Anton Vorontsov <avorontsov@ru.mvista.com>:
> GTM stands for General-purpose Timers Module and able to generate
> timer{1,2,3,4} interrupts. These timers are used by the drivers that
> need time precise interrupts (like for USB transactions scheduling for
> the Freescale USB Host controller as found in some QE and CPM chips),
> or these timers could be used as wakeup events from the CPU deep-sleep
> mode.
Comments below.
> diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentatio=
n/powerpc/booting-without-of.txt
> index 4cc7800..827b630 100644
> --- a/Documentation/powerpc/booting-without-of.txt
> +++ b/Documentation/powerpc/booting-without-of.txt
> @@ -57,7 +57,8 @@ Table of Contents
> n) 4xx/Axon EMAC ethernet nodes
> o) Xilinx IP cores
> p) Freescale Synchronous Serial Interface
> - q) USB EHCI controllers
> + q) USB EHCI controllers
> + r) Freescale General-purpose Timers Module
>
> VII - Marvell Discovery mv64[345]6x System Controller chips
> 1) The /system-controller node
> @@ -2825,6 +2826,35 @@ platforms are moved over to use the flattened-dev=
ice-tree model.
> big-endian;
> };
>
> + r) Freescale General-purpose Timers Module
> +
> + Required properties:
> + - compatible : should be "fsl,gtm" ("fsl,qe-gtm" in addition for =
QE
> + GTMs or "fsl,cpm2-gtm" for CPM2 GTMs).
I don't think this is specific enough. It is a very real possibility
for Freescale to produce another part with a "general purpose timers
module" that isn't register level compatible (and fsl,i2c is an
example of what not to do). The compatible string should include the
exact chip version. Newer parts can also claim compatibility with
older ones.
Defining a 'generic' compatible value is also known as "making stuff up". =
:-)
> + - reg : should contain gtm registers location and length (0x40).
> + - interrupts : should contain four interrupts.
> + - interrupt-parent : interrupt source phandle.
> + - clock-frequency : specifies the frequency driving the timer.
> +
> + Example:
> +
> + timer@500 {
> + compatible =3D "fsl,gtm";
> + reg =3D <0x500 0x40>;
> + interrupts =3D <90 8 78 8 84 8 72 8>;
> + interrupt-parent =3D <&ipic>;
> + /* filled by u-boot */
> + clock-frequency =3D <0>;
> + };
> +
> + timer@440 {
> + compatible =3D "fsl,qe-gtm", "fsl,gtm";
> + reg =3D <0x440 0x40>;
> + interrupts =3D <12 13 14 15>;
> + interrupt-parent =3D <&qeic>;
> + /* filled by u-boot */
> + clock-frequency =3D <0>;
> + };
>
> VII - Marvell Discovery mv64[345]6x System Controller chips
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 7f2f126..ad874e4 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -525,6 +525,11 @@ config FSL_LBC
> help
> Freescale Localbus support
>
> +config FSL_GTM
> + bool
> + help
> + Freescale General-purpose Timers support
> +
> # Yes MCA RS/6000s exist but Linux-PPC does not currently support any
> config MCA
> bool
> diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile
> index 6d386d0..9d3ddd2 100644
> --- a/arch/powerpc/sysdev/Makefile
> +++ b/arch/powerpc/sysdev/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_MMIO_NVRAM) +=3D mmio_nvram.o
> obj-$(CONFIG_FSL_SOC) +=3D fsl_soc.o
> obj-$(CONFIG_FSL_PCI) +=3D fsl_pci.o
> obj-$(CONFIG_FSL_LBC) +=3D fsl_lbc.o
> +obj-$(CONFIG_FSL_GTM) +=3D fsl_gtm.o
> obj-$(CONFIG_RAPIDIO) +=3D fsl_rio.o
> obj-$(CONFIG_TSI108_BRIDGE) +=3D tsi108_pci.o tsi108_dev.o
> obj-$(CONFIG_QUICC_ENGINE) +=3D qe_lib/
> diff --git a/arch/powerpc/sysdev/fsl_gtm.c b/arch/powerpc/sysdev/fsl_gtm=
.c
> new file mode 100644
> index 0000000..d29df56
> --- /dev/null
> +++ b/arch/powerpc/sysdev/fsl_gtm.c
> @@ -0,0 +1,365 @@
> +/*
> + * Freescale General-purpose Timers Module
> + *
> + * Copyright (c) Freescale Semicondutor, Inc. 2006.
> + * Shlomi Gridish <gridish@freescale.com>
> + * Jerry Huang <Chang-Ming.Huang@freescale.com>
> + * Copyright (c) MontaVista Software, Inc. 2008.
> + * Anton Vorontsov <avorontsov@ru.mvista.com>
> + *
> + * This program is free software; you can redistribute it and/or modif=
y 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/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/spinlock.h>
> +#include <linux/bitops.h>
> +#include <asm/fsl_gtm.h>
> +
> +/**
> + * gtm_get_timer - request GTM timer to use it with the rest of GTM API
> + * @width: timer width (only 16 bits wide timers implemented so far=
)
> + *
> + * This function reserves GTM timer for later use. It returns gtm_timer
> + * structure to use with the rest of GTM API, you should use timer->irq
> + * to manage timer interrupt.
> + */
> +struct gtm_timer *gtm_get_timer(int width)
> +{
> + struct device_node *np;
> + struct gtm *gtm =3D NULL;
> + int i;
> +
> + if (width !=3D 16)
> + return ERR_PTR(-ENOSYS);
> +
> + for_each_compatible_node(np, NULL, "fsl,gtm") {
> + if (!np->data) {
> + WARN_ON(1);
> + continue;
> + }
> + gtm =3D np->data;
> +
> + spin_lock_irq(>m->lock);
> +
> + for (i =3D 0; i < ARRAY_SIZE(gtm->timers); i++) {
> + if (!gtm->timers[i].requested) {
> + gtm->timers[i].requested =3D true;
> + spin_unlock_irq(>m->lock);
> + of_node_put(np);
> + return >m->timers[i];
> + }
> + }
> +
> + spin_unlock_irq(>m->lock);
> + }
> +
> + if (gtm)
> + return ERR_PTR(-EBUSY);
> + return ERR_PTR(-ENODEV);
> +}
> +EXPORT_SYMBOL(gtm_get_timer);
This isn't a very efficient construct. You've already located all of
the relevant nodes in the gtm_init() function. Searching the device
tree is not efficient, so it should be avoided and only be performed
once. Can you move the extraction of data from the tree to gtm_init()
and only parse the drivers list of timers in this function?
> +
> +/**
> + * gtm_get_specific_timer - request specific GTM timer
> + * @gtm: specific GTM, pass here GTM's device_node->data
> + * @timer: specific timer number, Timer1 is 0.
> + * @width: timer width (only 16 bits wide timers implemented so far=
)
> + *
> + * This function reserves GTM timer for later use. It returns gtm_timer
> + * structure to use with the rest of GTM API, you should use timer->irq
> + * to manage timer interrupt.
> + */
> +struct gtm_timer *gtm_get_specific_timer(struct gtm *gtm, int timer, in=
t width)
> +{
> + struct gtm_timer *ret =3D ERR_PTR(-EBUSY);
> +
> + if (width !=3D 16)
> + return ERR_PTR(-ENOSYS);
> +
> + spin_lock_irq(>m->lock);
> +
> + if (gtm->timers[timer].requested)
> + goto out;
> +
> + ret =3D >m->timers[timer];
> + ret->requested =3D true;
> +
> +out:
> + spin_unlock_irq(>m->lock);
> + return ret;
> +}
> +EXPORT_SYMBOL(gtm_get_specific_timer);
> +
> +/**
> + * gtm_put_timer - release GTM timer
> + * @width: timer width (only 16 bits wide timers implemented so far=
)
> + *
> + * This function releases GTM timer so others may request it.
> + */
> +void gtm_put_timer(struct gtm_timer *tmr)
> +{
> + spin_lock_irq(&tmr->gtm->lock);
> +
> + tmr->requested =3D false;
> +
> + spin_unlock_irq(&tmr->gtm->lock);
> +}
> +EXPORT_SYMBOL(gtm_put_timer);
> +
> +/*
> + * This is back-end for the exported functions, it's used to reset sing=
le
> + * timer in reference mode.
> + */
> +static int gtm_reset_ref_timer16(struct gtm_timer *tmr, int frequency,
> + int reference_value, bool free_run)
> +{
> + struct gtm *gtm =3D tmr->gtm;
> + int num =3D tmr - >m->timers[0];
> + unsigned int prescaler;
> + u8 iclk =3D GTMDR_ICLK_ICLK;
> + u8 psr;
> + u8 sps;
> + unsigned long flags;
> + int max_prescaler =3D 256 * 256 * 16;
> +
> + /* CPM2 doesn't have primary prescaler */
> + if (!tmr->gtpsr)
> + max_prescaler /=3D 256;
> +
> + prescaler =3D gtm->clock / frequency;
> + /*
> + * We have two 8 bit prescalers -- primary and secondary (psr, s=
ps),
> + * plus "slow go" mode (clk / 16). So, total prescale value is
> + * 16 * (psr + 1) * (sps + 1). Though, for CPM2 GTMs we losing p=
sr.
> + */
> + if (prescaler > max_prescaler)
> + return -EINVAL;
> +
> + if (prescaler > max_prescaler / 16) {
> + iclk =3D GTMDR_ICLK_SLGO;
> + prescaler /=3D 16;
> + }
> +
> + if (prescaler <=3D 256) {
> + psr =3D 0;
> + sps =3D prescaler - 1;
> + } else {
> + psr =3D 256 - 1;
> + sps =3D prescaler / 256 - 1;
> + }
> +
> + spin_lock_irqsave(>m->lock, flags);
> +
> + /*
> + * Properly reset timers: stop, reset, set up prescalers, refere=
nce
> + * value and clear event register.
> + */
> + clrsetbits_8(tmr->gtcfr, ~(GTCFR_STP(num) | GTCFR_RST(num)),
> + GTCFR_STP(num) | GTCFR_RST(num));
> +
> + setbits8(tmr->gtcfr, GTCFR_STP(num));
> +
> + if (tmr->gtpsr)
> + out_be16(tmr->gtpsr, psr);
> + clrsetbits_be16(tmr->gtmdr, 0xFFFF, iclk | GTMDR_SPS(sps) |
> + GTMDR_ORI | (free_run ? GTMDR_FRR : 0));
> + out_be16(tmr->gtcnr, 0);
> + out_be16(tmr->gtrfr, reference_value);
> + out_be16(tmr->gtevr, 0xFFFF);
> +
> + /* Let it be. */
> + clrbits8(tmr->gtcfr, GTCFR_STP(num));
> +
> + spin_unlock_irqrestore(>m->lock, flags);
> +
> + return 0;
> +}
> +/**
> + * gtm_reset_timer16 - reset 16 bit timer with arbitrary precision
> + * @tmr: pointer to the gtm_timer structure obtained from gtm_get=
_timer
> + * @usec: timer interval in microseconds
> + * @reload: if set, the timer will reset upon expiry rather than
> + * continue running free.
> + *
> + * This function (re)sets the GTM timer so that it counts up to the req=
uested
> + * interval value, and fires the interrupt when the value is reached. T=
his
> + * function will reduce the precision of the timer as needed in order f=
or the
> + * requested timeout to fit in a 16-bit register.
> + */
> +int gtm_reset_timer16(struct gtm_timer *tmr, unsigned long usec, bool r=
eload)
> +{
> + /* quite obvious, frequency which is enough for =B5Sec precision=
*/
> + int freq =3D 1000000;
> + unsigned int bit;
> +
> + bit =3D fls_long(usec);
> + if (bit > 15) {
> + freq >>=3D bit - 15;
> + usec >>=3D bit - 15;
> + }
> +
> + if (!freq)
> + return -EINVAL;
> +
> + return gtm_reset_ref_timer16(tmr, freq, usec, reload);
> +}
> +EXPORT_SYMBOL(gtm_reset_timer16);
> +
> +/**
> + * gtm_reset_utimer16 - reset 16 bits timer
> + * @tmr: pointer to the gtm_timer structure obtained from gtm_get=
_timer
> + * @usec: timer interval in microseconds
> + * @reload: if set, the timer will reset upon expiry rather than
> + * continue running free.
> + *
> + * This function (re)sets GTM timer so that it counts up to the request=
ed
> + * interval value, and fires the interrupt when the value is reached. I=
f reload
> + * flag was set, timer will also reset itself upon reference value, oth=
erwise
> + * it continues to increment.
> + */
> +int gtm_reset_utimer16(struct gtm_timer *tmr, u16 usec, bool reload)
> +{
> + /* quite obvious, frequency which is enough for =B5Sec precision=
*/
> + const int freq =3D 1000000;
> +
> + /*
> + * We can lower the frequency (and probably power consumption) b=
y
> + * dividing both frequency and usec by 2 until there is no remai=
nder.
> + * But we won't bother with this unless savings are measured, so=
just
> + * run the timer as is.
> + */
> +
> + return gtm_reset_ref_timer16(tmr, freq, usec, reload);
> +}
> +EXPORT_SYMBOL(gtm_reset_utimer16);
> +
> +/**
> + * gtm_stop_timer16 - stop single timer
> + * @tmr: pointer to the gtm_timer structure obtained from gtm_get=
_timer
> + *
> + * This function simply stops the GTM timer.
> + */
> +void gtm_stop_timer16(struct gtm_timer *tmr)
> +{
> + struct gtm *gtm =3D tmr->gtm;
> + int num =3D tmr - >m->timers[0];
> + unsigned long flags;
> +
> + spin_lock_irqsave(>m->lock, flags);
> +
> + setbits8(tmr->gtcfr, GTCFR_STP(num));
> + out_be16(tmr->gtevr, 0xFFFF);
> +
> + spin_unlock_irqrestore(>m->lock, flags);
> +}
> +EXPORT_SYMBOL(gtm_stop_timer16);
> +
> +static void __init gtm_set_shortcuts(struct device_node *np,
> + struct gtm_timer *timers,
> + struct gtm_timers_regs __iomem *reg=
s)
> +{
> + /*
> + * Yeah, I don't like this either, but timers' registers a bit m=
essed,
> + * so we have to provide shortcuts to write timer independent co=
de.
> + * Alternative option is to create gt*() accessors, but that wil=
l be
> + * even uglier and cryptic.
> + */
> + timers[0].gtcfr =3D ®s->gtcfr1;
> + timers[0].gtmdr =3D ®s->gtmdr1;
> + timers[0].gtcnr =3D ®s->gtcnr1;
> + timers[0].gtrfr =3D ®s->gtrfr1;
> + timers[0].gtevr =3D ®s->gtevr1;
> +
> + timers[1].gtcfr =3D ®s->gtcfr1;
> + timers[1].gtmdr =3D ®s->gtmdr2;
> + timers[1].gtcnr =3D ®s->gtcnr2;
> + timers[1].gtrfr =3D ®s->gtrfr2;
> + timers[1].gtevr =3D ®s->gtevr2;
> +
> + timers[2].gtcfr =3D ®s->gtcfr2;
> + timers[2].gtmdr =3D ®s->gtmdr3;
> + timers[2].gtcnr =3D ®s->gtcnr3;
> + timers[2].gtrfr =3D ®s->gtrfr3;
> + timers[2].gtevr =3D ®s->gtevr3;
> +
> + timers[3].gtcfr =3D ®s->gtcfr2;
> + timers[3].gtmdr =3D ®s->gtmdr4;
> + timers[3].gtcnr =3D ®s->gtcnr4;
> + timers[3].gtrfr =3D ®s->gtrfr4;
> + timers[3].gtevr =3D ®s->gtevr4;
> +
> + /* CPM2 doesn't have primary prescaler */
> + if (!of_device_is_compatible(np, "fsl,cpm2-gtm")) {
> + timers[0].gtpsr =3D ®s->gtpsr1;
> + timers[1].gtpsr =3D ®s->gtpsr2;
> + timers[2].gtpsr =3D ®s->gtpsr3;
> + timers[3].gtpsr =3D ®s->gtpsr4;
> + }
> +}
> +
> +static int __init gtm_init(void)
> +{
> + struct device_node *np;
> +
> + for_each_compatible_node(np, NULL, "fsl,gtm") {
> + int i;
> + struct gtm *gtm;
> + const u32 *clock;
> + int size;
> +
> + gtm =3D kzalloc(sizeof(*gtm), GFP_KERNEL);
> + if (!gtm) {
> + pr_err("%s: unable to allocate memory\n",
> + np->full_name);
> + continue;
> + }
> +
> + spin_lock_init(>m->lock);
> +
> + clock =3D of_get_property(np, "clock-frequency", &size);
> + if (!clock || size !=3D sizeof(*clock)) {
> + pr_err("%s: no clock-frequency\n", np->full_name=
);
> + goto err;
> + }
> + gtm->clock =3D *clock;
> +
> + for (i =3D 0; i < ARRAY_SIZE(gtm->timers); i++) {
> + int ret;
> + struct resource irq;
> +
> + ret =3D of_irq_to_resource(np, i, &irq);
> + if (ret =3D=3D NO_IRQ) {
> + pr_err("%s: not enough interrupts specif=
ied\n",
> + np->full_name);
> + goto err;
> + }
> + gtm->timers[i].irq =3D irq.start;
> + gtm->timers[i].gtm =3D gtm;
> + }
> +
> + gtm->regs =3D of_iomap(np, 0);
> + if (!gtm->regs) {
> + pr_err("%s: unable to iomap registers\n",
> + np->full_name);
> + goto err;
> + }
> +
> + gtm_set_shortcuts(np, gtm->timers, gtm->regs);
> +
> + /* We don't want to lose the node and its ->data */
> + of_node_get(np);
> + np->data =3D gtm;
> +
> + continue;
> +err:
> + kfree(gtm);
> + }
> + return 0;
> +}
> +arch_initcall(gtm_init);
> diff --git a/include/asm-powerpc/fsl_gtm.h b/include/asm-powerpc/fsl_gtm=
.h
> new file mode 100644
> index 0000000..3eafb21
> --- /dev/null
> +++ b/include/asm-powerpc/fsl_gtm.h
> @@ -0,0 +1,108 @@
> +/*
> + * Freescale General-purpose Timers Module
> + *
> + * Copyright (c) Freescale Semicondutor, Inc. 2006.
> + * Shlomi Gridish <gridish@freescale.com>
> + * Jerry Huang <Chang-Ming.Huang@freescale.com>
> + * Copyright (c) MontaVista Software, Inc. 2008.
> + * Anton Vorontsov <avorontsov@ru.mvista.com>
> + *
> + * This program is free software; you can redistribute it and/or modif=
y 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.
> + */
> +
> +#ifndef __ASM_FSL_GTM_H
> +#define __ASM_FSL_GTM_H
> +
> +#include <linux/types.h>
> +#include <linux/spinlock.h>
> +#include <linux/io.h>
> +
> +#define GTCFR_STP(x) ((x) & 1 ? 1 << 5 : 1 << 1)
> +#define GTCFR_RST(x) ((x) & 1 ? 1 << 4 : 1 << 0)
> +
> +#define GTMDR_ICLK_MASK (3 << 1)
> +#define GTMDR_ICLK_ICAS (0 << 1)
> +#define GTMDR_ICLK_ICLK (1 << 1)
> +#define GTMDR_ICLK_SLGO (2 << 1)
> +#define GTMDR_FRR (1 << 3)
> +#define GTMDR_ORI (1 << 4)
> +#define GTMDR_SPS(x) ((x) << 8)
> +
> +struct gtm_timers_regs {
> + u8 gtcfr1; /* Timer 1, Timer 2 global config regist=
er */
> + u8 res0[0x3];
> + u8 gtcfr2; /* Timer 3, timer 4 global config regist=
er */
> + u8 res1[0xB];
> + __be16 gtmdr1; /* Timer 1 mode register */
> + __be16 gtmdr2; /* Timer 2 mode register */
> + __be16 gtrfr1; /* Timer 1 reference register */
> + __be16 gtrfr2; /* Timer 2 reference register */
> + __be16 gtcpr1; /* Timer 1 capture register */
> + __be16 gtcpr2; /* Timer 2 capture register */
> + __be16 gtcnr1; /* Timer 1 counter */
> + __be16 gtcnr2; /* Timer 2 counter */
> + __be16 gtmdr3; /* Timer 3 mode register */
> + __be16 gtmdr4; /* Timer 4 mode register */
> + __be16 gtrfr3; /* Timer 3 reference register */
> + __be16 gtrfr4; /* Timer 4 reference register */
> + __be16 gtcpr3; /* Timer 3 capture register */
> + __be16 gtcpr4; /* Timer 4 capture register */
> + __be16 gtcnr3; /* Timer 3 counter */
> + __be16 gtcnr4; /* Timer 4 counter */
> + __be16 gtevr1; /* Timer 1 event register */
> + __be16 gtevr2; /* Timer 2 event register */
> + __be16 gtevr3; /* Timer 3 event register */
> + __be16 gtevr4; /* Timer 4 event register */
> + __be16 gtpsr1; /* Timer 1 prescale register */
> + __be16 gtpsr2; /* Timer 2 prescale register */
> + __be16 gtpsr3; /* Timer 3 prescale register */
> + __be16 gtpsr4; /* Timer 4 prescale register */
> + u8 res2[0x40];
> +} __attribute__ ((packed));
> +
> +struct gtm_timer {
> + unsigned int irq;
> +
> + struct gtm *gtm;
> + bool requested;
> + u8 __iomem *gtcfr;
> + __be16 __iomem *gtmdr;
> + __be16 __iomem *gtpsr;
> + __be16 __iomem *gtcnr;
> + __be16 __iomem *gtrfr;
> + __be16 __iomem *gtevr;
> +};
> +
> +struct gtm {
> + unsigned int clock;
> + struct gtm_timers_regs __iomem *regs;
> + struct gtm_timer timers[4];
> + spinlock_t lock;
> +};
Consider moving the register definitions and structures inside the .c
file. This is all anonymous data as far as the caller is concerned.
If the structures are only access inside the driver then you gain
yourself a bit of safety by moving them.
> +
> +extern struct gtm_timer *gtm_get_timer(int width);
> +extern struct gtm_timer *gtm_get_specific_timer(struct gtm *gtm, int ti=
mer,
> + int width);
> +extern void gtm_put_timer(struct gtm_timer *tmr);
> +extern int gtm_reset_timer16(struct gtm_timer *tmr, unsigned long usec,
> + bool reload);
> +extern int gtm_reset_utimer16(struct gtm_timer *tmr, u16 usec, bool rel=
oad);
> +extern void gtm_stop_timer16(struct gtm_timer *tmr);
> +
> +/**
> + * gtm_ack_timer16 - acknowledge timer event (free-run timers only)
> + * @tmr: pointer to the gtm_timer structure obtained from gtm_get=
_timer
> + * @events: events mask to ack
> + *
> + * Thus function used to acknowledge timer interrupt event, use it insi=
de the
> + * interrupt handler.
> + */
> +static inline void gtm_ack_timer16(struct gtm_timer *tmr, u16 events)
> +{
> + out_be16(tmr->gtevr, events);
> +}
Drop 'inline' and expect gcc to do the right thing.
> +
> +#endif /* __ASM_FSL_GTM_H */
> --
> 1.5.5
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: pci issue - wrong detection of pci ressources
From: Christian Ehrhardt @ 2008-04-21 14:08 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: linuxppc-dev, Hollis Blanchard
In-Reply-To: <480C87BD.8090207@ru.mvista.com>
[-- Attachment #1: Type: text/plain, Size: 1198 bytes --]
Sergei Shtylyov wrote:
> Hello.
>
> Christian Ehrhardt wrote:
>
>>> Cheers,
>>> Ben.
>
>> For comparison I defined DEBUG in the good kernel (arch=ppc) and that
>> is what the initialization prints (pci ...:0a:1 is the secondary head
>> of the same graphic card an it's not an issue if thats not allocated):
[...]
> You've changed fb_base_phys and mmio_base_phys to resource_size_t
> which is 64-bit, so use %llx to print them.
>
Thanks your absolutely right, I sometimes forget that I need long long for 64bit on 32bit archs (and ignored the warnings :-( ).
I corrected the printk format strings and attached the new logs.
[...]
>> +else {
>> +printk(KERN_ERR"%s - continue with start 0x%0lx on %p\n", __func__, (this->end + 1), this->sibling);
>> +}
>> new->start = this->end + 1;
>> this = this->sibling;
>
> And here. Yet it's not clear why you call resource's 'end' 'start'...
It's the new->start that get's calculated one line after that new else part.
I printed that one to to see a bit how the loop iterates the resource elements.
>
> WBR, Sergei
--
Grüsse / regards,
Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization
[-- Attachment #2: pci_resource_issue_bootmsg_badkernel.txt --]
[-- Type: text/plain, Size: 5748 bytes --]
With DEBUG in arch/powerpc/kernel/pci-common.c, debug in commandline and a patch with some printk's (I attached the patch because it is the usual "put printk's everywhere" so the diff helps to understand where the prints come from). Corrected prinf format specifiers according to the comments from Sergei Shtylyov.
PCI host bridge /plb/pci@1ec000000 (primary) ranges:
MEM 0x0000000180000000..0x000000018fffffff -> 0x0000000080000000
IO 0x00000001e8000000..0x00000001e80fffff -> 0x0000000000000000
4xx PCI DMA offset set to 0x00000000
PCI: Probing PCI hardware
PCI: Hiding 4xx host bridge resources 0000:00:00.0
Try to map irq for 0000:00:00.0...
-> got one, spec 2 cells (0x00000003 0x00000008...) on /interrupt-controller2
-> mapped to linux irq 16
Try to map irq for 0000:00:0a.0...
-> got one, spec 2 cells (0x00000003 0x00000008...) on /interrupt-controller2
-> mapped to linux irq 16
Try to map irq for 0000:00:0a.1...
PCI: PHB (bus 0) bridge rsrc 0: 0000000000000000-00000000000fffff [0x100], parent c0365060 (PCI IO)
__request_resource - request 0xcf8045b0 name '/plb/pci@1ec000000' start 0x0 end 0xfffff
__request_resource - no conflict parent 0xc0365060 sibling 0x00000000
PCI: PHB (bus 0) bridge rsrc 1: 0000000180000000-000000018fffffff [0x200], parent c0365038 (PCI mem)
__request_resource - request 0xcf8045d8 name '/plb/pci@1ec000000' start 0x180000000 end 0x18fffffff
__request_resource - no conflict parent 0xc0365038 sibling 0x00000000
PCI: Assigning unassigned resouces...
pci_assign_unassigned_resources -#1- bus 0xcf82d400
pci_assign_unassigned_resources -#2- bus 0xcf82d400
pci_assign_resource - allocate with IORESOURCE_PREFETCH
pci_bus_alloc_resource - enter
pci_assign_resource - second pci_bus_alloc_resource call
pci_bus_alloc_resource - enter
pci_bus_alloc_resource - call allocate ressource size 0x8000000 startcalc 0xffffffff, align 0x8000000
find_resource - size 0x8000000, min 0x180000000, max 0xffffffffffffffff
find_resource - found start 0x180000000 end 0x187ffffff
__request_resource - request 0xcf810578 name '0000:00:0a.0' start 0x180000000 end 0x187ffffff
__request_resource - no conflict parent 0xcf8045d8 sibling 0x00000000
pci_assign_resource - allocate with IORESOURCE_PREFETCH
pci_bus_alloc_resource - enter
pci_assign_resource - second pci_bus_alloc_resource call
pci_bus_alloc_resource - enter
pci_bus_alloc_resource - call allocate ressource size 0x8000000 startcalc 0xffffffff, align 0x8000000
find_resource - size 0x8000000, min 0x180000000, max 0xffffffffffffffff
find_resource - continue with start 0x188000000 on 0x00000000
find_resource - found start 0x188000000 end 0x18fffffff
__request_resource - request 0xcf810178 name '0000:00:0a.1' start 0x188000000 end 0x18fffffff
__request_resource - no conflict parent 0xcf8045d8 sibling 0x00000000
pci_assign_resource - allocate with IORESOURCE_PREFETCH
pci_bus_alloc_resource - enter
pci_assign_resource - second pci_bus_alloc_resource call
pci_bus_alloc_resource - enter
pci_bus_alloc_resource - call allocate ressource size 0x20000 startcalc 0xffffffff, align 0x20000
find_resource - size 0x20000, min 0x180000000, max 0xffffffffffffffff
find_resource - continue with start 0x188000000 on 0xcf810178
find_resource - continue with start 0x190000000 on 0x00000000
find_resource - no this - exit
PCI: pci_assign_resource - Failed to allocate mem resource #6:20000@190000000 for 0000:00:0a.0
pci_assign_resource - allocate with IORESOURCE_PREFETCH
pci_bus_alloc_resource - enter
pci_bus_alloc_resource - call allocate ressource size 0x10000 startcalc 0xffffffff, align 0x10000
find_resource - size 0x10000, min 0x180000000, max 0xffffffffffffffff
find_resource - continue with start 0x188000000 on 0xcf810178
find_resource - continue with start 0x190000000 on 0x00000000
find_resource - no this - exit
PCI: pci_assign_resource - Failed to allocate mem resource #2:10000@190000000 for 0000:00:0a.0
pci_assign_resource - allocate with IORESOURCE_PREFETCH
pci_bus_alloc_resource - enter
pci_bus_alloc_resource - call allocate ressource size 0x10000 startcalc 0xffffffff, align 0x10000
find_resource - size 0x10000, min 0x180000000, max 0xffffffffffffffff
find_resource - continue with start 0x188000000 on 0xcf810178
find_resource - continue with start 0x190000000 on 0x00000000
find_resource - no this - exit
PCI: pci_assign_resource - Failed to allocate mem resource #1:10000@190000000 for 0000:00:0a.1
pci_assign_resource - allocate with IORESOURCE_PREFETCH
pci_bus_alloc_resource - enter
pci_bus_alloc_resource - call allocate ressource size 0x100 startcalc 0xffffffff, align 0x100
find_resource - size 0x100, min 0x1000, max 0xffffffffffffffff
find_resource - found start 0x1000 end 0x10ff
__request_resource - request 0xcf8105a0 name '0000:00:0a.0' start 0x1000 end 0x10ff
__request_resource - no conflict parent 0xcf8045b0 sibling 0x00000000
[...]
radeonfb_pci_register BEGIN
radeonfb_pci_register - resource0 0x180000000 - 0x187ffffff
radeonfb_pci_register - resource1 0x1000 - 0x10ff
radeonfb_pci_register - resource2 0x0 - 0x0
radeonfb_pci_register - got pci ressources fb_base_phys 0x180000000 mmio_base_phys 0x0
__request_resource - request 0xcf8bfd20 name 'radeonfb framebuffer' start 0x180000000 end 0x187ffffff
__request_resource - request 0xcf8bfd20 name 'radeonfb framebuffer' start 0x180000000 end 0x187ffffff
__request_resource - request 0xcf8bfd20 name 'radeonfb framebuffer' start 0x180000000 end 0x187ffffff
__request_resource - no conflict parent 0xcf810578 sibling 0x00000000
radeonfb_pci_register - call ioremap for base 0x0 and size 16384
__ioremap(): phys addr 0x0 is RAM lr 0xc029df88 - mem_init_done 1 highmem@0x10000000
radeonfb (0000:00:0a.0): cannot map MMIO
radeonfb: probe of 0000:00:0a.0 failed with error -5
[-- Attachment #3: debug_pci_radeonfb.diff --]
[-- Type: text/x-patch, Size: 8625 bytes --]
Subject: [PATCH] : ! debug only ! - printk's in pci ressource handling
From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
This defines debug for pci and put's some printk's to keep track of the pci
ressource handling. This patch is just for reference to understand the
resulting boot message log.
The patch is ugly - but for debugging only ;-)
Signed-off-by: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
---
[diffstat]
arch/powerpc/kernel/pci-common.c | 2 ++
arch/powerpc/mm/pgtable_32.c | 4 ++--
drivers/pci/bus.c | 3 +++
drivers/pci/setup-res.c | 12 ++++++++----
drivers/video/aty/radeon_base.c | 7 +++++++
drivers/video/aty/radeonfb.h | 4 ++--
kernel/resource.c | 17 ++++++++++++++++-
7 files changed, 40 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -37,6 +37,8 @@
#include <asm/machdep.h>
#include <asm/ppc-pci.h>
#include <asm/firmware.h>
+
+#define DEBUG
#ifdef DEBUG
#include <asm/udbg.h>
diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
--- a/arch/powerpc/mm/pgtable_32.c
+++ b/arch/powerpc/mm/pgtable_32.c
@@ -183,8 +183,8 @@ __ioremap(phys_addr_t addr, unsigned lon
* mem_init() sets high_memory so only do the check after that.
*/
if (mem_init_done && (p < virt_to_phys(high_memory))) {
- printk("__ioremap(): phys addr 0x%llx is RAM lr %p\n",
- (unsigned long long)p, __builtin_return_address(0));
+ printk("__ioremap(): phys addr 0x%llx is RAM lr 0x%p - mem_init_done %d highmem@0x%p\n",
+ (unsigned long long)p, __builtin_return_address(0), mem_init_done, virt_to_phys(high_memory));
return NULL;
}
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -42,6 +42,8 @@ pci_bus_alloc_resource(struct pci_bus *b
{
int i, ret = -ENOMEM;
+printk(KERN_ERR"%s - enter\n", __func__);
+
type_mask |= IORESOURCE_IO | IORESOURCE_MEM;
for (i = 0; i < PCI_BUS_NUM_RESOURCES; i++) {
@@ -59,6 +61,7 @@ pci_bus_alloc_resource(struct pci_bus *b
!(res->flags & IORESOURCE_PREFETCH))
continue;
+printk(KERN_ERR"%s - call allocate ressource size 0x%0llx startcalc 0x%0x, align 0x%0llx\n", __func__, size, (r->start ? : min,-1), align);
/* Ok, try it out.. */
ret = allocate_resource(r, res, size,
r->start ? : min,
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -544,10 +544,12 @@ pci_assign_unassigned_resources(void)
/* Depth first, calculate sizes and alignments of all
subordinate buses. */
list_for_each_entry(bus, &pci_root_buses, node) {
+printk(KERN_ERR"%s -#1- bus 0x%p\n",__func__, bus);
pci_bus_size_bridges(bus);
}
/* Depth last, allocate resources and update the hardware. */
list_for_each_entry(bus, &pci_root_buses, node) {
+printk(KERN_ERR"%s -#2- bus 0x%p\n",__func__, bus);
pci_bus_assign_resources(bus);
pci_enable_bridges(bus);
}
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -142,10 +142,13 @@ int pci_assign_resource(struct pci_dev *
required alignment in the "start" field. */
align = (resno < PCI_BRIDGE_RESOURCES) ? size : res->start;
+printk(KERN_ERR"%s - allocate with IORESOURCE_PREFETCH\n", __func__);
+
/* First, try exact prefetching match.. */
ret = pci_bus_alloc_resource(bus, res, size, align, min,
IORESOURCE_PREFETCH,
pcibios_align_resource, dev);
+
if (ret < 0 && (res->flags & IORESOURCE_PREFETCH)) {
/*
@@ -154,13 +157,14 @@ int pci_assign_resource(struct pci_dev *
* But a prefetching area can handle a non-prefetching
* window (it will just not perform as well).
*/
+printk(KERN_ERR"%s - second pci_bus_alloc_resource call\n", __func__);
ret = pci_bus_alloc_resource(bus, res, size, align, min, 0,
pcibios_align_resource, dev);
}
if (ret) {
- printk(KERN_ERR "PCI: Failed to allocate %s resource "
- "#%d:%llx@%llx for %s\n",
+ printk(KERN_ERR "PCI: %s - Failed to allocate %s resource "
+ "#%d:%llx@%llx for %s\n", __func__,
res->flags & IORESOURCE_IO ? "I/O" : "mem",
resno, (unsigned long long)size,
(unsigned long long)res->start, pci_name(dev));
@@ -197,8 +201,8 @@ int pci_assign_resource_fixed(struct pci
}
if (ret) {
- printk(KERN_ERR "PCI: Failed to allocate %s resource "
- "#%d:%llx@%llx for %s\n",
+ printk(KERN_ERR "PCI: %s - Failed to allocate %s resource "
+ "#%d:%llx@%llx for %s\n", __func__,
res->flags & IORESOURCE_IO ? "I/O" : "mem",
resno, (unsigned long long)(res->end - res->start + 1),
(unsigned long long)res->start, pci_name(dev));
diff --git a/drivers/video/aty/radeon_base.c b/drivers/video/aty/radeon_base.c
--- a/drivers/video/aty/radeon_base.c
+++ b/drivers/video/aty/radeon_base.c
@@ -2160,6 +2160,10 @@ static int __devinit radeonfb_pci_regist
int ret;
RTRACE("radeonfb_pci_register BEGIN\n");
+
+printk(KERN_ERR"%s - resource0 0x%0llx - 0x%0llx\n", __func__, pdev->resource[0].start, pdev->resource[0].end);
+printk(KERN_ERR"%s - resource1 0x%0llx - 0x%0llx\n", __func__, pdev->resource[1].start, pdev->resource[1].end);
+printk(KERN_ERR"%s - resource2 0x%0llx - 0x%0llx\n", __func__, pdev->resource[2].start, pdev->resource[2].end);
/* Enable device in PCI config */
ret = pci_enable_device(pdev);
@@ -2198,6 +2202,8 @@ static int __devinit radeonfb_pci_regist
rinfo->fb_base_phys = pci_resource_start (pdev, 0);
rinfo->mmio_base_phys = pci_resource_start (pdev, 2);
+printk(KERN_ERR"%s - got pci ressources fb_base_phys 0x%0llx mmio_base_phys 0x%0llx\n", __func__,rinfo->fb_base_phys,rinfo->mmio_base_phys);
+
/* request the mem regions */
ret = pci_request_region(pdev, 0, "radeonfb framebuffer");
if (ret < 0) {
@@ -2214,6 +2220,7 @@ static int __devinit radeonfb_pci_regist
}
/* map the regions */
+printk(KERN_ERR "%s - call ioremap for base 0x%0llx and size %d\n",__func__, rinfo->mmio_base_phys, RADEON_REGSIZE);
rinfo->mmio_base = ioremap(rinfo->mmio_base_phys, RADEON_REGSIZE);
if (!rinfo->mmio_base) {
printk(KERN_ERR "radeonfb (%s): cannot map MMIO\n",
diff --git a/drivers/video/aty/radeonfb.h b/drivers/video/aty/radeonfb.h
--- a/drivers/video/aty/radeonfb.h
+++ b/drivers/video/aty/radeonfb.h
@@ -287,8 +287,8 @@ struct radeonfb_info {
char name[DEVICE_NAME_SIZE];
- unsigned long mmio_base_phys;
- unsigned long fb_base_phys;
+ resource_size_t mmio_base_phys;
+ resource_size_t fb_base_phys;
void __iomem *mmio_base;
void __iomem *fb_base;
diff --git a/kernel/resource.c b/kernel/resource.c
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -152,6 +152,9 @@ static struct resource * __request_resou
resource_size_t end = new->end;
struct resource *tmp, **p;
+printk(KERN_ERR"%s - request 0x%p name '%s' start 0x%0llx end 0x%0llx\n",
+ __func__, new, new->name, start, end);
+
if (end < start)
return root;
if (start < root->start)
@@ -165,6 +168,7 @@ static struct resource * __request_resou
new->sibling = tmp;
*p = new;
new->parent = root;
+printk(KERN_ERR"%s - no conflict parent 0x%p sibling 0x%p\n",__func__, new->parent, new->sibling);
return NULL;
}
p = &tmp->sibling;
@@ -305,6 +309,8 @@ static int find_resource(struct resource
{
struct resource *this = root->child;
+printk(KERN_ERR"%s - size 0x%0llx, min 0x%0llx, max 0x%0llx\n", __func__, size, min, max);
+
new->start = root->start;
/*
* Skip past an allocated resource that starts at 0, since the assignment
@@ -328,10 +334,16 @@ static int find_resource(struct resource
alignf(alignf_data, new, size, align);
if (new->start < new->end && new->end - new->start >= size - 1) {
new->end = new->start + size - 1;
+printk(KERN_ERR"%s - found start 0x%0llx end 0x%0llx\n", __func__, new->start, new->end);
return 0;
}
- if (!this)
+ if (!this) {
+printk(KERN_ERR"%s - no this - exit\n", __func__);
break;
+}
+else {
+printk(KERN_ERR"%s - continue with start 0x%0llx on 0x%p\n", __func__, (this->end + 1), this->sibling);
+}
new->start = this->end + 1;
this = this->sibling;
}
@@ -385,6 +397,9 @@ int insert_resource(struct resource *par
{
int result;
struct resource *first, *next;
+
+printk(KERN_ERR"%s - insert 0x%p (start 0x%0llx end 0x%0llx) under parent 0x%p\n",
+ __func__, new, new->start, new->end, parent);
write_lock(&resource_lock);
^ permalink raw reply
* Re: [PATCH 3/5] [POWERPC] QE: prepare QE PIO code for GPIO LIB support
From: Grant Likely @ 2008-04-21 14:08 UTC (permalink / raw)
To: Anton Vorontsov; +Cc: linuxppc-dev
In-Reply-To: <20080418190934.GC4407@polina.dev.rtsoft.ru>
On Fri, Apr 18, 2008 at 1:09 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> - split and export __par_io_config_pin() out of par_io_config_pin(), so we
> could use the prefixed version with GPIO LIB API;
> - rename struct port_regs to qe_pio_regs, and place it into qe.h;
> - rename #define NUM_OF_PINS to QE_PIO_PINS, and place it into qe.h.
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
I don't see any mutual exclusion here. How is __par_io_config_pin()
protected when there are multiple callers?
> ---
> arch/powerpc/sysdev/qe_lib/qe_io.c | 94 +++++++++++++++++-------------------
> include/asm-powerpc/qe.h | 19 +++++++
> 2 files changed, 64 insertions(+), 49 deletions(-)
>
> diff --git a/arch/powerpc/sysdev/qe_lib/qe_io.c b/arch/powerpc/sysdev/qe_lib/qe_io.c
> index 93916a4..7c87460 100644
> --- a/arch/powerpc/sysdev/qe_lib/qe_io.c
> +++ b/arch/powerpc/sysdev/qe_lib/qe_io.c
> @@ -28,21 +28,7 @@
>
> #undef DEBUG
>
> -#define NUM_OF_PINS 32
> -
> -struct port_regs {
> - __be32 cpodr; /* Open drain register */
> - __be32 cpdata; /* Data register */
> - __be32 cpdir1; /* Direction register */
> - __be32 cpdir2; /* Direction register */
> - __be32 cppar1; /* Pin assignment register */
> - __be32 cppar2; /* Pin assignment register */
> -#ifdef CONFIG_PPC_85xx
> - u8 pad[8];
> -#endif
> -};
> -
> -static struct port_regs __iomem *par_io;
> +static struct qe_pio_regs __iomem *par_io;
> static int num_par_io_ports = 0;
>
> int par_io_init(struct device_node *np)
> @@ -64,69 +50,79 @@ int par_io_init(struct device_node *np)
> return 0;
> }
>
> -int par_io_config_pin(u8 port, u8 pin, int dir, int open_drain,
> - int assignment, int has_irq)
> +void __par_io_config_pin(struct qe_pio_regs __iomem *par_io, u8 pin, int dir,
> + int open_drain, int assignment, int has_irq)
> {
> - u32 pin_mask1bit, pin_mask2bits, new_mask2bits, tmp_val;
> -
> - if (!par_io)
> - return -1;
> + u32 pin_mask1bit;
> + u32 pin_mask2bits;
> + u32 new_mask2bits;
> + u32 tmp_val;
>
> /* calculate pin location for single and 2 bits information */
> - pin_mask1bit = (u32) (1 << (NUM_OF_PINS - (pin + 1)));
> + pin_mask1bit = (u32) (1 << (QE_PIO_PINS - (pin + 1)));
>
> /* Set open drain, if required */
> - tmp_val = in_be32(&par_io[port].cpodr);
> + tmp_val = in_be32(&par_io->cpodr);
> if (open_drain)
> - out_be32(&par_io[port].cpodr, pin_mask1bit | tmp_val);
> + out_be32(&par_io->cpodr, pin_mask1bit | tmp_val);
> else
> - out_be32(&par_io[port].cpodr, ~pin_mask1bit & tmp_val);
> + out_be32(&par_io->cpodr, ~pin_mask1bit & tmp_val);
>
> /* define direction */
> - tmp_val = (pin > (NUM_OF_PINS / 2) - 1) ?
> - in_be32(&par_io[port].cpdir2) :
> - in_be32(&par_io[port].cpdir1);
> + tmp_val = (pin > (QE_PIO_PINS / 2) - 1) ?
> + in_be32(&par_io->cpdir2) :
> + in_be32(&par_io->cpdir1);
>
> /* get all bits mask for 2 bit per port */
> - pin_mask2bits = (u32) (0x3 << (NUM_OF_PINS -
> - (pin % (NUM_OF_PINS / 2) + 1) * 2));
> + pin_mask2bits = (u32) (0x3 << (QE_PIO_PINS -
> + (pin % (QE_PIO_PINS / 2) + 1) * 2));
>
> /* Get the final mask we need for the right definition */
> - new_mask2bits = (u32) (dir << (NUM_OF_PINS -
> - (pin % (NUM_OF_PINS / 2) + 1) * 2));
> + new_mask2bits = (u32) (dir << (QE_PIO_PINS -
> + (pin % (QE_PIO_PINS / 2) + 1) * 2));
>
> /* clear and set 2 bits mask */
> - if (pin > (NUM_OF_PINS / 2) - 1) {
> - out_be32(&par_io[port].cpdir2,
> + if (pin > (QE_PIO_PINS / 2) - 1) {
> + out_be32(&par_io->cpdir2,
> ~pin_mask2bits & tmp_val);
> tmp_val &= ~pin_mask2bits;
> - out_be32(&par_io[port].cpdir2, new_mask2bits | tmp_val);
> + out_be32(&par_io->cpdir2, new_mask2bits | tmp_val);
> } else {
> - out_be32(&par_io[port].cpdir1,
> + out_be32(&par_io->cpdir1,
> ~pin_mask2bits & tmp_val);
> tmp_val &= ~pin_mask2bits;
> - out_be32(&par_io[port].cpdir1, new_mask2bits | tmp_val);
> + out_be32(&par_io->cpdir1, new_mask2bits | tmp_val);
> }
> /* define pin assignment */
> - tmp_val = (pin > (NUM_OF_PINS / 2) - 1) ?
> - in_be32(&par_io[port].cppar2) :
> - in_be32(&par_io[port].cppar1);
> + tmp_val = (pin > (QE_PIO_PINS / 2) - 1) ?
> + in_be32(&par_io->cppar2) :
> + in_be32(&par_io->cppar1);
>
> - new_mask2bits = (u32) (assignment << (NUM_OF_PINS -
> - (pin % (NUM_OF_PINS / 2) + 1) * 2));
> + new_mask2bits = (u32) (assignment << (QE_PIO_PINS -
> + (pin % (QE_PIO_PINS / 2) + 1) * 2));
> /* clear and set 2 bits mask */
> - if (pin > (NUM_OF_PINS / 2) - 1) {
> - out_be32(&par_io[port].cppar2,
> + if (pin > (QE_PIO_PINS / 2) - 1) {
> + out_be32(&par_io->cppar2,
> ~pin_mask2bits & tmp_val);
> tmp_val &= ~pin_mask2bits;
> - out_be32(&par_io[port].cppar2, new_mask2bits | tmp_val);
> + out_be32(&par_io->cppar2, new_mask2bits | tmp_val);
> } else {
> - out_be32(&par_io[port].cppar1,
> + out_be32(&par_io->cppar1,
> ~pin_mask2bits & tmp_val);
> tmp_val &= ~pin_mask2bits;
> - out_be32(&par_io[port].cppar1, new_mask2bits | tmp_val);
> + out_be32(&par_io->cppar1, new_mask2bits | tmp_val);
> }
> +}
> +EXPORT_SYMBOL(__par_io_config_pin);
> +
> +int par_io_config_pin(u8 port, u8 pin, int dir, int open_drain,
> + int assignment, int has_irq)
> +{
> + if (!par_io || port >= num_par_io_ports)
> + return -EINVAL;
>
> + __par_io_config_pin(&par_io[port], pin, dir, open_drain, assignment,
> + has_irq);
> return 0;
> }
> EXPORT_SYMBOL(par_io_config_pin);
> @@ -137,10 +133,10 @@ int par_io_data_set(u8 port, u8 pin, u8 val)
>
> if (port >= num_par_io_ports)
> return -EINVAL;
> - if (pin >= NUM_OF_PINS)
> + if (pin >= QE_PIO_PINS)
> return -EINVAL;
> /* calculate pin location */
> - pin_mask = (u32) (1 << (NUM_OF_PINS - 1 - pin));
> + pin_mask = (u32) (1 << (QE_PIO_PINS - 1 - pin));
>
> tmp_val = in_be32(&par_io[port].cpdata);
>
> diff --git a/include/asm-powerpc/qe.h b/include/asm-powerpc/qe.h
> index d217288..c4523ac 100644
> --- a/include/asm-powerpc/qe.h
> +++ b/include/asm-powerpc/qe.h
> @@ -84,8 +84,27 @@ extern spinlock_t cmxgcr_lock;
>
> /* Export QE common operations */
> extern void qe_reset(void);
> +
> +/* QE PIO */
> +#define QE_PIO_PINS 32
> +
> +struct qe_pio_regs {
> + __be32 cpodr; /* Open drain register */
> + __be32 cpdata; /* Data register */
> + __be32 cpdir1; /* Direction register */
> + __be32 cpdir2; /* Direction register */
> + __be32 cppar1; /* Pin assignment register */
> + __be32 cppar2; /* Pin assignment register */
> +#ifdef CONFIG_PPC_85xx
> + u8 pad[8];
> +#endif
> +};
> +
> extern int par_io_init(struct device_node *np);
> extern int par_io_of_config(struct device_node *np);
> +extern void __par_io_config_pin(struct qe_pio_regs __iomem *par_io, u8 pin,
> + int dir, int open_drain, int assignment,
> + int has_irq);
> extern int par_io_config_pin(u8 port, u8 pin, int dir, int open_drain,
> int assignment, int has_irq);
> extern int par_io_data_set(u8 port, u8 pin, u8 val);
> --
> 1.5.5
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH 4/5] [POWERPC] QE: implement support for the GPIO LIB API
From: Grant Likely @ 2008-04-21 14:19 UTC (permalink / raw)
To: Anton Vorontsov; +Cc: David Brownell, linuxppc-dev
In-Reply-To: <20080418190959.GD4407@polina.dev.rtsoft.ru>
On Fri, Apr 18, 2008 at 1:09 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> This is needed to access QE GPIOs via Linux GPIO API.
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
> index 827b630..5c9cfab 100644
> --- a/Documentation/powerpc/booting-without-of.txt
> +++ b/Documentation/powerpc/booting-without-of.txt
> @@ -1721,24 +1721,32 @@ platforms are moved over to use the flattened-device-tree model.
> information.
>
> Required properties:
> - - device_type : should be "par_io".
> + - #gpio-cells : should be "2".
> + - compatible : should be "fsl,qe-pario-bank-<bank>", "fsl,qe-pario-bank"
Once again; I don't like the generic compatible values. Please
include the exact chip name in the string. ie: "fsl,<chip>-qe-pario".
"fsl,qe-pario-bank" is not a real thing. If you want a common
compatible string that the driver can bind against, then choose one
real part and add it to the compatible list for all the other parts.
Also, why is <bank> encoded in compatible? Do the different banks
have different register interfaces?
> - reg : offset to the register set and its length.
> - - num-ports : number of Parallel I/O ports
> + - gpio-controller : node to identify gpio controllers.
>
> - Example:
> - par_io@1400 {
> - reg = <1400 100>;
> - #address-cells = <1>;
> - #size-cells = <0>;
> - device_type = "par_io";
> - num-ports = <7>;
> - ucc_pin@01 {
> - ......
> - };
> + For example, two QE Par I/O banks:
> + qe_pio_a: gpio-controller@1400 {
> + #gpio-cells = <2>;
> + compatible = "fsl,qe-pario-bank-a", "fsl,qe-pario-bank";
> + reg = <0x1400 0x18>;
> + gpio-controller;
> + };
>
> + qe_pio_e: gpio-controller@1460 {
> + #gpio-cells = <2>;
> + compatible = "fsl,qe-pario-bank-e", "fsl,qe-pario-bank";
> + reg = <0x1460 0x18>;
> + gpio-controller;
> + };
>
> vi) Pin configuration nodes
>
> + NOTE: pin configuration nodes are obsolete. Usually, their existance
> + is an evidence of the firmware shortcomings. Such fixups are
> + better handled by the Linux board file, not the device tree.
> +
> Required properties:
> - linux,phandle : phandle of this node; likely referenced by a QE
> device.
> diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
> index f38c50b..f6eecd1 100644
> --- a/arch/powerpc/platforms/Kconfig
> +++ b/arch/powerpc/platforms/Kconfig
> @@ -270,6 +270,8 @@ config QUICC_ENGINE
> bool
> select PPC_LIB_RHEAP
> select CRC32
> + select GENERIC_GPIO
> + select HAVE_GPIO_LIB
> help
> The QUICC Engine (QE) is a new generation of communications
> coprocessors on Freescale embedded CPUs (akin to CPM in older chips).
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index bbd2834..cee56f9 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -25,6 +25,15 @@ config DEBUG_GPIO
>
> # put expanders in the right section, in alphabetical order
>
> +comment "On-chip GPIOs:"
> +
> +config GPIO_QE
> + bool "QUICC Engine GPIOs"
> + depends on QUICC_ENGINE
> + help
> + Say Y here to use GPIOs on the Freescale PowerPC CPUs with
> + QUICC Engine block.
> +
> comment "I2C GPIO expanders:"
>
> config GPIO_PCA953X
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index fdde992..fd0a41f 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_HAVE_GPIO_LIB) += gpiolib.o
> obj-$(CONFIG_GPIO_MCP23S08) += mcp23s08.o
> obj-$(CONFIG_GPIO_PCA953X) += pca953x.o
> obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o
> +obj-$(CONFIG_GPIO_QE) += qe.o
Since this isn't an of_platform or a platform driver; I'd put it into
arch/powerpc instead.
> diff --git a/drivers/gpio/qe.c b/drivers/gpio/qe.c
> new file mode 100644
> index 0000000..474bc44
> --- /dev/null
> +++ b/drivers/gpio/qe.c
> +static int __init qe_add_gpiochips(void)
> +{
> + int ret;
> + struct device_node *np;
> +
> + for_each_compatible_node(np, NULL, "fsl,qe-pario-bank") {
> + struct qe_gpio_chip *qe_gc;
> + struct of_mm_gpio_chip *mm_gc;
> + struct of_gpio_chip *of_gc;
> + struct gpio_chip *gc;
> +
> + qe_gc = kzalloc(sizeof(*qe_gc), GFP_KERNEL);
> + if (!qe_gc) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + spin_lock_init(&qe_gc->lock);
> +
> + mm_gc = &qe_gc->mm_gc;
> + of_gc = &mm_gc->of_gc;
> + gc = &of_gc->gc;
> +
> + mm_gc->save_regs = qe_gpio_save_regs;
> + of_gc->gpio_cells = 2;
> + gc->ngpio = QE_PIO_PINS;
> + gc->direction_input = qe_gpio_dir_in;
> + gc->direction_output = qe_gpio_dir_out;
> + gc->get = qe_gpio_get;
> + gc->set = qe_gpio_set;
> +
> + ret = of_mm_gpiochip_add(np, mm_gc);
> + if (ret)
> + goto err;
> + }
> +
> + return 0;
> +err:
> + pr_err("%s: registration failed with status %d\n", np->full_name, ret);
> + of_node_put(np);
> + return ret;
> +}
> +arch_initcall(qe_add_gpiochips);
Should this really be a arch_initcall()? Would it be better for
platforms needing it to call it explicitly from one of the platform's
machine_arch_initcall()? Otherwise it gets called for all platforms
in a multiplatform kernel.
Cheers,
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: linux-next: x86-latest/powerpc-next merge conflict
From: Alexander van Heukelum @ 2008-04-21 14:19 UTC (permalink / raw)
To: Gabriel Paubert
Cc: Stephen Rothwell, Ingo Molnar, linux-next, Paul Mackerras,
linuxppc-dev
In-Reply-To: <20080421133606.GA27304@iram.es>
On Mon, 21 Apr 2008 15:36:06 +0200, "Gabriel Paubert" <paubert@iram.es>
said:
> On Mon, Apr 21, 2008 at 03:07:13PM +0200, Alexander van Heukelum wrote:
> > On Mon, 21 Apr 2008 22:13:06 +1000, "Paul Mackerras" <paulus@samba.org>
> > said:
> > > Alexander van Heukelum writes:
> > > > Powerpc would pick up an optimized version via this chain: generic =
fls64
> > > > ->
> > > > powerpc __fls --> __ilog2 --> asm (PPC_CNTLZL "%0,%1" : "=3Dr" (lz)=
: "r"
> > > > (x)).
> > >=20
> > > Why wouldn't powerpc continue to use the fls64 that I have in there
> > > now?
> >=20
> > In Linus' tree that would be the generic one that uses (the 32-bit)
> > fls():
> >=20
> > static inline int fls64(__u64 x)
> > {
> > __u32 h =3D x >> 32;
> > if (h)
> > return fls(h) + 32;
> > return fls(x);
> > }
> >=20
> > > > However, the generic version of fls64 first tests the argument for =
zero.
> > > > From
> > > > your code I derive that the count-leading-zeroes instruction for
> > > > argument zero
> > > > is defined as cntlzl(0) =3D=3D BITS_PER_LONG.
> > >=20
> > > That is correct. If the argument is 0 then all of the zero bits are
> > > leading zeroes. :)
> >=20
> > So... for 64-bit powerpc it makes sense to have its own implementation
> > and ignore the (improved) generic one and for 32-bit powerpc the generic
> > implementation of fls64 is fine. The current situation in linux-next
> > seems
> > optimal to me.
>=20
>=20
> Not so sure, the optimal version of fls64 for 32 bit PPC seems to be:
>=20
> cntlzw ch,h ; ch =3D fls32(h) where h =3D x>>32
> cntlzw cl,l ; cl =3D fls32(l) where l =3D (__u32)x
> srwi t1,ch,5
> neg t1,t1 ; t1 =3D (h=3D=3D0) ? -1 : 0
> and cl,t1,cl ; cl =3D (h=3D=3D0) ? cl : 0
> add result,ch,cl
>=20
> That's only 6 instructions without any branch, although the dependency=20
> chain is 5 instructions long. Good luck getting the compiler to=20
> generate something as compact as this.
I should not have said the magic word optimal, I guess ;). The code
you show would fit nicely as an arch-specific optimized version of
fls64 for 32-bit powerpc in include/arch-powerpc/bitops.h.
Greetings,
Alexander
(who is not going to write and test a patch with
powerpc inline assembly soon. srwi?)
> Don't worry about the number of cntlzw, it's one clock on all 32 bit=20
> PPC processors I know, some may even be able to perform 2 or 3 cntlzw=20
> per clock.
>=20
> Regards,
> Gabriel
>=20
--=20
Alexander van Heukelum
heukelum@fastmail.fm
--=20
http://www.fastmail.fm - Same, same, but different=85
^ permalink raw reply
* Re: [PATCH] [POWERPC] 52xx: defconfigs for CM5200, Lite5200B, Motion-PRO and TQM5200
From: Grant Likely @ 2008-04-21 14:26 UTC (permalink / raw)
To: Bartlomiej Sieka; +Cc: linuxppc-dev
In-Reply-To: <480C9D38.3020902@semihalf.com>
On Mon, Apr 21, 2008 at 7:57 AM, Bartlomiej Sieka <tur@semihalf.com> wrote:
> Bartlomiej Sieka wrote:
>
> > Board-specific defconfigs based on current mpc5200_defconfig, archival
> > lite5200_defconfig, and [cm5200|motionpro|tqm5200]_defconfig from the
> > linux-2.6-denx tree. Kernels build using these defconfigs were verified
> > to boot with root filesystem mounted over NFS on Motion-PRO, TQM5200
> > and Lite5200B boards. CM5200 target was not tested due to hardware
> > unavailability.
> >
>
> Hello Grant,
>
> There hasn't been any negitve comments on the above patch -- will you
> push it upstream via your tree?
Yes, of course. I would have acked it, but I never saw it (not cc'd).
Acked-by: Grant Likely <grant.likely@secretlab.ca>
Cheers,
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH 3/5] [POWERPC] QE: prepare QE PIO code for GPIO LIB support
From: Anton Vorontsov @ 2008-04-21 14:28 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev
In-Reply-To: <fa686aa40804210708t6a0dcba6k117996c38e5d1b8e@mail.gmail.com>
On Mon, Apr 21, 2008 at 08:08:39AM -0600, Grant Likely wrote:
> On Fri, Apr 18, 2008 at 1:09 PM, Anton Vorontsov
> <avorontsov@ru.mvista.com> wrote:
> > - split and export __par_io_config_pin() out of par_io_config_pin(), so we
> > could use the prefixed version with GPIO LIB API;
> > - rename struct port_regs to qe_pio_regs, and place it into qe.h;
> > - rename #define NUM_OF_PINS to QE_PIO_PINS, and place it into qe.h.
> >
> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
>
> I don't see any mutual exclusion here. How is __par_io_config_pin()
> protected when there are multiple callers?
The idea is to not have multiple callers.
Today most callers of par_io_config_pin() is various board fixups,
e.g. this in the setup_arch():
for (np = NULL; (np = of_find_node_by_name(np, "ucc")) != NULL;)
par_io_of_config(np);
Which calls par_io_config_pin(). But we're safe here, setup_arch called
very early.
The only abuser of this is mpc832x_rdb.c, but it will be converted
to the GPIO API.
> > ---
> > arch/powerpc/sysdev/qe_lib/qe_io.c | 94 +++++++++++++++++-------------------
> > include/asm-powerpc/qe.h | 19 +++++++
> > 2 files changed, 64 insertions(+), 49 deletions(-)
> >
> > diff --git a/arch/powerpc/sysdev/qe_lib/qe_io.c b/arch/powerpc/sysdev/qe_lib/qe_io.c
> > index 93916a4..7c87460 100644
> > --- a/arch/powerpc/sysdev/qe_lib/qe_io.c
> > +++ b/arch/powerpc/sysdev/qe_lib/qe_io.c
> > @@ -28,21 +28,7 @@
> >
> > #undef DEBUG
> >
> > -#define NUM_OF_PINS 32
> > -
> > -struct port_regs {
> > - __be32 cpodr; /* Open drain register */
> > - __be32 cpdata; /* Data register */
> > - __be32 cpdir1; /* Direction register */
> > - __be32 cpdir2; /* Direction register */
> > - __be32 cppar1; /* Pin assignment register */
> > - __be32 cppar2; /* Pin assignment register */
> > -#ifdef CONFIG_PPC_85xx
> > - u8 pad[8];
> > -#endif
> > -};
> > -
> > -static struct port_regs __iomem *par_io;
> > +static struct qe_pio_regs __iomem *par_io;
> > static int num_par_io_ports = 0;
> >
> > int par_io_init(struct device_node *np)
> > @@ -64,69 +50,79 @@ int par_io_init(struct device_node *np)
> > return 0;
> > }
> >
> > -int par_io_config_pin(u8 port, u8 pin, int dir, int open_drain,
> > - int assignment, int has_irq)
> > +void __par_io_config_pin(struct qe_pio_regs __iomem *par_io, u8 pin, int dir,
> > + int open_drain, int assignment, int has_irq)
> > {
> > - u32 pin_mask1bit, pin_mask2bits, new_mask2bits, tmp_val;
> > -
> > - if (!par_io)
> > - return -1;
> > + u32 pin_mask1bit;
> > + u32 pin_mask2bits;
> > + u32 new_mask2bits;
> > + u32 tmp_val;
> >
> > /* calculate pin location for single and 2 bits information */
> > - pin_mask1bit = (u32) (1 << (NUM_OF_PINS - (pin + 1)));
> > + pin_mask1bit = (u32) (1 << (QE_PIO_PINS - (pin + 1)));
> >
> > /* Set open drain, if required */
> > - tmp_val = in_be32(&par_io[port].cpodr);
> > + tmp_val = in_be32(&par_io->cpodr);
> > if (open_drain)
> > - out_be32(&par_io[port].cpodr, pin_mask1bit | tmp_val);
> > + out_be32(&par_io->cpodr, pin_mask1bit | tmp_val);
> > else
> > - out_be32(&par_io[port].cpodr, ~pin_mask1bit & tmp_val);
> > + out_be32(&par_io->cpodr, ~pin_mask1bit & tmp_val);
> >
> > /* define direction */
> > - tmp_val = (pin > (NUM_OF_PINS / 2) - 1) ?
> > - in_be32(&par_io[port].cpdir2) :
> > - in_be32(&par_io[port].cpdir1);
> > + tmp_val = (pin > (QE_PIO_PINS / 2) - 1) ?
> > + in_be32(&par_io->cpdir2) :
> > + in_be32(&par_io->cpdir1);
> >
> > /* get all bits mask for 2 bit per port */
> > - pin_mask2bits = (u32) (0x3 << (NUM_OF_PINS -
> > - (pin % (NUM_OF_PINS / 2) + 1) * 2));
> > + pin_mask2bits = (u32) (0x3 << (QE_PIO_PINS -
> > + (pin % (QE_PIO_PINS / 2) + 1) * 2));
> >
> > /* Get the final mask we need for the right definition */
> > - new_mask2bits = (u32) (dir << (NUM_OF_PINS -
> > - (pin % (NUM_OF_PINS / 2) + 1) * 2));
> > + new_mask2bits = (u32) (dir << (QE_PIO_PINS -
> > + (pin % (QE_PIO_PINS / 2) + 1) * 2));
> >
> > /* clear and set 2 bits mask */
> > - if (pin > (NUM_OF_PINS / 2) - 1) {
> > - out_be32(&par_io[port].cpdir2,
> > + if (pin > (QE_PIO_PINS / 2) - 1) {
> > + out_be32(&par_io->cpdir2,
> > ~pin_mask2bits & tmp_val);
> > tmp_val &= ~pin_mask2bits;
> > - out_be32(&par_io[port].cpdir2, new_mask2bits | tmp_val);
> > + out_be32(&par_io->cpdir2, new_mask2bits | tmp_val);
> > } else {
> > - out_be32(&par_io[port].cpdir1,
> > + out_be32(&par_io->cpdir1,
> > ~pin_mask2bits & tmp_val);
> > tmp_val &= ~pin_mask2bits;
> > - out_be32(&par_io[port].cpdir1, new_mask2bits | tmp_val);
> > + out_be32(&par_io->cpdir1, new_mask2bits | tmp_val);
> > }
> > /* define pin assignment */
> > - tmp_val = (pin > (NUM_OF_PINS / 2) - 1) ?
> > - in_be32(&par_io[port].cppar2) :
> > - in_be32(&par_io[port].cppar1);
> > + tmp_val = (pin > (QE_PIO_PINS / 2) - 1) ?
> > + in_be32(&par_io->cppar2) :
> > + in_be32(&par_io->cppar1);
> >
> > - new_mask2bits = (u32) (assignment << (NUM_OF_PINS -
> > - (pin % (NUM_OF_PINS / 2) + 1) * 2));
> > + new_mask2bits = (u32) (assignment << (QE_PIO_PINS -
> > + (pin % (QE_PIO_PINS / 2) + 1) * 2));
> > /* clear and set 2 bits mask */
> > - if (pin > (NUM_OF_PINS / 2) - 1) {
> > - out_be32(&par_io[port].cppar2,
> > + if (pin > (QE_PIO_PINS / 2) - 1) {
> > + out_be32(&par_io->cppar2,
> > ~pin_mask2bits & tmp_val);
> > tmp_val &= ~pin_mask2bits;
> > - out_be32(&par_io[port].cppar2, new_mask2bits | tmp_val);
> > + out_be32(&par_io->cppar2, new_mask2bits | tmp_val);
> > } else {
> > - out_be32(&par_io[port].cppar1,
> > + out_be32(&par_io->cppar1,
> > ~pin_mask2bits & tmp_val);
> > tmp_val &= ~pin_mask2bits;
> > - out_be32(&par_io[port].cppar1, new_mask2bits | tmp_val);
> > + out_be32(&par_io->cppar1, new_mask2bits | tmp_val);
> > }
> > +}
> > +EXPORT_SYMBOL(__par_io_config_pin);
> > +
> > +int par_io_config_pin(u8 port, u8 pin, int dir, int open_drain,
> > + int assignment, int has_irq)
> > +{
> > + if (!par_io || port >= num_par_io_ports)
> > + return -EINVAL;
> >
> > + __par_io_config_pin(&par_io[port], pin, dir, open_drain, assignment,
> > + has_irq);
> > return 0;
> > }
> > EXPORT_SYMBOL(par_io_config_pin);
> > @@ -137,10 +133,10 @@ int par_io_data_set(u8 port, u8 pin, u8 val)
> >
> > if (port >= num_par_io_ports)
> > return -EINVAL;
> > - if (pin >= NUM_OF_PINS)
> > + if (pin >= QE_PIO_PINS)
> > return -EINVAL;
> > /* calculate pin location */
> > - pin_mask = (u32) (1 << (NUM_OF_PINS - 1 - pin));
> > + pin_mask = (u32) (1 << (QE_PIO_PINS - 1 - pin));
> >
> > tmp_val = in_be32(&par_io[port].cpdata);
> >
> > diff --git a/include/asm-powerpc/qe.h b/include/asm-powerpc/qe.h
> > index d217288..c4523ac 100644
> > --- a/include/asm-powerpc/qe.h
> > +++ b/include/asm-powerpc/qe.h
> > @@ -84,8 +84,27 @@ extern spinlock_t cmxgcr_lock;
> >
> > /* Export QE common operations */
> > extern void qe_reset(void);
> > +
> > +/* QE PIO */
> > +#define QE_PIO_PINS 32
> > +
> > +struct qe_pio_regs {
> > + __be32 cpodr; /* Open drain register */
> > + __be32 cpdata; /* Data register */
> > + __be32 cpdir1; /* Direction register */
> > + __be32 cpdir2; /* Direction register */
> > + __be32 cppar1; /* Pin assignment register */
> > + __be32 cppar2; /* Pin assignment register */
> > +#ifdef CONFIG_PPC_85xx
> > + u8 pad[8];
> > +#endif
> > +};
> > +
> > extern int par_io_init(struct device_node *np);
> > extern int par_io_of_config(struct device_node *np);
> > +extern void __par_io_config_pin(struct qe_pio_regs __iomem *par_io, u8 pin,
> > + int dir, int open_drain, int assignment,
> > + int has_irq);
> > extern int par_io_config_pin(u8 port, u8 pin, int dir, int open_drain,
> > int assignment, int has_irq);
> > extern int par_io_data_set(u8 port, u8 pin, u8 val);
> > --
> > 1.5.5
> >
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@ozlabs.org
> > https://ozlabs.org/mailman/listinfo/linuxppc-dev
> >
>
>
>
> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.
--
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
^ permalink raw reply
* Re: [PATCH 1/5] [POWERPC] sysdev: implement FSL GTM support
From: Anton Vorontsov @ 2008-04-21 14:28 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev
In-Reply-To: <fa686aa40804210703iaa8251ctc125313a8e52f4ef@mail.gmail.com>
On Mon, Apr 21, 2008 at 08:03:31AM -0600, Grant Likely wrote:
> 2008/4/18 Anton Vorontsov <avorontsov@ru.mvista.com>:
> > GTM stands for General-purpose Timers Module and able to generate
> > timer{1,2,3,4} interrupts. These timers are used by the drivers that
> > need time precise interrupts (like for USB transactions scheduling for
> > the Freescale USB Host controller as found in some QE and CPM chips),
> > or these timers could be used as wakeup events from the CPU deep-sleep
> > mode.
>
> Comments below.
>
> > diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
> > index 4cc7800..827b630 100644
> > --- a/Documentation/powerpc/booting-without-of.txt
> > +++ b/Documentation/powerpc/booting-without-of.txt
> > @@ -57,7 +57,8 @@ Table of Contents
> > n) 4xx/Axon EMAC ethernet nodes
> > o) Xilinx IP cores
> > p) Freescale Synchronous Serial Interface
> > - q) USB EHCI controllers
> > + q) USB EHCI controllers
> > + r) Freescale General-purpose Timers Module
> >
> > VII - Marvell Discovery mv64[345]6x System Controller chips
> > 1) The /system-controller node
> > @@ -2825,6 +2826,35 @@ platforms are moved over to use the flattened-device-tree model.
> > big-endian;
> > };
> >
> > + r) Freescale General-purpose Timers Module
> > +
> > + Required properties:
> > + - compatible : should be "fsl,gtm" ("fsl,qe-gtm" in addition for QE
> > + GTMs or "fsl,cpm2-gtm" for CPM2 GTMs).
>
> I don't think this is specific enough. It is a very real possibility
> for Freescale to produce another part with a "general purpose timers
> module" that isn't register level compatible (and fsl,i2c is an
> example of what not to do). The compatible string should include the
> exact chip version. Newer parts can also claim compatibility with
> older ones.
Ok, I'll expand this to fsl,<cpu-specific>-gtm and
fsl,<cpu-specific>-{qe,cpm2}-gtm.
> Defining a 'generic' compatible value is also known as "making stuff up". :-)
>
> > + - reg : should contain gtm registers location and length (0x40).
> > + - interrupts : should contain four interrupts.
> > + - interrupt-parent : interrupt source phandle.
> > + - clock-frequency : specifies the frequency driving the timer.
> > +
> > + Example:
> > +
> > + timer@500 {
> > + compatible = "fsl,gtm";
> > + reg = <0x500 0x40>;
> > + interrupts = <90 8 78 8 84 8 72 8>;
> > + interrupt-parent = <&ipic>;
> > + /* filled by u-boot */
> > + clock-frequency = <0>;
> > + };
> > +
> > + timer@440 {
> > + compatible = "fsl,qe-gtm", "fsl,gtm";
> > + reg = <0x440 0x40>;
> > + interrupts = <12 13 14 15>;
> > + interrupt-parent = <&qeic>;
> > + /* filled by u-boot */
> > + clock-frequency = <0>;
> > + };
> >
> > VII - Marvell Discovery mv64[345]6x System Controller chips
> > ===========================================================
> > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > index 7f2f126..ad874e4 100644
> > --- a/arch/powerpc/Kconfig
> > +++ b/arch/powerpc/Kconfig
> > @@ -525,6 +525,11 @@ config FSL_LBC
> > help
> > Freescale Localbus support
> >
> > +config FSL_GTM
> > + bool
> > + help
> > + Freescale General-purpose Timers support
> > +
> > # Yes MCA RS/6000s exist but Linux-PPC does not currently support any
> > config MCA
> > bool
> > diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile
> > index 6d386d0..9d3ddd2 100644
> > --- a/arch/powerpc/sysdev/Makefile
> > +++ b/arch/powerpc/sysdev/Makefile
> > @@ -13,6 +13,7 @@ obj-$(CONFIG_MMIO_NVRAM) += mmio_nvram.o
> > obj-$(CONFIG_FSL_SOC) += fsl_soc.o
> > obj-$(CONFIG_FSL_PCI) += fsl_pci.o
> > obj-$(CONFIG_FSL_LBC) += fsl_lbc.o
> > +obj-$(CONFIG_FSL_GTM) += fsl_gtm.o
> > obj-$(CONFIG_RAPIDIO) += fsl_rio.o
> > obj-$(CONFIG_TSI108_BRIDGE) += tsi108_pci.o tsi108_dev.o
> > obj-$(CONFIG_QUICC_ENGINE) += qe_lib/
> > diff --git a/arch/powerpc/sysdev/fsl_gtm.c b/arch/powerpc/sysdev/fsl_gtm.c
> > new file mode 100644
> > index 0000000..d29df56
> > --- /dev/null
> > +++ b/arch/powerpc/sysdev/fsl_gtm.c
> > @@ -0,0 +1,365 @@
> > +/*
> > + * Freescale General-purpose Timers Module
> > + *
> > + * Copyright (c) Freescale Semicondutor, Inc. 2006.
> > + * Shlomi Gridish <gridish@freescale.com>
> > + * Jerry Huang <Chang-Ming.Huang@freescale.com>
> > + * Copyright (c) MontaVista Software, Inc. 2008.
> > + * Anton Vorontsov <avorontsov@ru.mvista.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 <linux/kernel.h>
> > +#include <linux/errno.h>
> > +#include <linux/io.h>
> > +#include <linux/of.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/bitops.h>
> > +#include <asm/fsl_gtm.h>
> > +
> > +/**
> > + * gtm_get_timer - request GTM timer to use it with the rest of GTM API
> > + * @width: timer width (only 16 bits wide timers implemented so far)
> > + *
> > + * This function reserves GTM timer for later use. It returns gtm_timer
> > + * structure to use with the rest of GTM API, you should use timer->irq
> > + * to manage timer interrupt.
> > + */
> > +struct gtm_timer *gtm_get_timer(int width)
> > +{
> > + struct device_node *np;
> > + struct gtm *gtm = NULL;
> > + int i;
> > +
> > + if (width != 16)
> > + return ERR_PTR(-ENOSYS);
> > +
> > + for_each_compatible_node(np, NULL, "fsl,gtm") {
> > + if (!np->data) {
> > + WARN_ON(1);
> > + continue;
> > + }
> > + gtm = np->data;
> > +
> > + spin_lock_irq(>m->lock);
> > +
> > + for (i = 0; i < ARRAY_SIZE(gtm->timers); i++) {
> > + if (!gtm->timers[i].requested) {
> > + gtm->timers[i].requested = true;
> > + spin_unlock_irq(>m->lock);
> > + of_node_put(np);
> > + return >m->timers[i];
> > + }
> > + }
> > +
> > + spin_unlock_irq(>m->lock);
> > + }
> > +
> > + if (gtm)
> > + return ERR_PTR(-EBUSY);
> > + return ERR_PTR(-ENODEV);
> > +}
> > +EXPORT_SYMBOL(gtm_get_timer);
>
> This isn't a very efficient construct. You've already located all of
> the relevant nodes in the gtm_init() function. Searching the device
> tree is not efficient, so it should be avoided and only be performed
> once. Can you move the extraction of data from the tree to gtm_init()
> and only parse the drivers list of timers in this function?
Well, yes. I can create a list, and place all the GTMs in there.
> > +
> > +/**
> > + * gtm_get_specific_timer - request specific GTM timer
> > + * @gtm: specific GTM, pass here GTM's device_node->data
> > + * @timer: specific timer number, Timer1 is 0.
> > + * @width: timer width (only 16 bits wide timers implemented so far)
> > + *
> > + * This function reserves GTM timer for later use. It returns gtm_timer
> > + * structure to use with the rest of GTM API, you should use timer->irq
> > + * to manage timer interrupt.
> > + */
> > +struct gtm_timer *gtm_get_specific_timer(struct gtm *gtm, int timer, int width)
> > +{
> > + struct gtm_timer *ret = ERR_PTR(-EBUSY);
> > +
> > + if (width != 16)
> > + return ERR_PTR(-ENOSYS);
> > +
> > + spin_lock_irq(>m->lock);
> > +
> > + if (gtm->timers[timer].requested)
> > + goto out;
> > +
> > + ret = >m->timers[timer];
> > + ret->requested = true;
> > +
> > +out:
> > + spin_unlock_irq(>m->lock);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(gtm_get_specific_timer);
> > +
> > +/**
> > + * gtm_put_timer - release GTM timer
> > + * @width: timer width (only 16 bits wide timers implemented so far)
> > + *
> > + * This function releases GTM timer so others may request it.
> > + */
> > +void gtm_put_timer(struct gtm_timer *tmr)
> > +{
> > + spin_lock_irq(&tmr->gtm->lock);
> > +
> > + tmr->requested = false;
> > +
> > + spin_unlock_irq(&tmr->gtm->lock);
> > +}
> > +EXPORT_SYMBOL(gtm_put_timer);
> > +
> > +/*
> > + * This is back-end for the exported functions, it's used to reset single
> > + * timer in reference mode.
> > + */
> > +static int gtm_reset_ref_timer16(struct gtm_timer *tmr, int frequency,
> > + int reference_value, bool free_run)
> > +{
> > + struct gtm *gtm = tmr->gtm;
> > + int num = tmr - >m->timers[0];
> > + unsigned int prescaler;
> > + u8 iclk = GTMDR_ICLK_ICLK;
> > + u8 psr;
> > + u8 sps;
> > + unsigned long flags;
> > + int max_prescaler = 256 * 256 * 16;
> > +
> > + /* CPM2 doesn't have primary prescaler */
> > + if (!tmr->gtpsr)
> > + max_prescaler /= 256;
> > +
> > + prescaler = gtm->clock / frequency;
> > + /*
> > + * We have two 8 bit prescalers -- primary and secondary (psr, sps),
> > + * plus "slow go" mode (clk / 16). So, total prescale value is
> > + * 16 * (psr + 1) * (sps + 1). Though, for CPM2 GTMs we losing psr.
> > + */
> > + if (prescaler > max_prescaler)
> > + return -EINVAL;
> > +
> > + if (prescaler > max_prescaler / 16) {
> > + iclk = GTMDR_ICLK_SLGO;
> > + prescaler /= 16;
> > + }
> > +
> > + if (prescaler <= 256) {
> > + psr = 0;
> > + sps = prescaler - 1;
> > + } else {
> > + psr = 256 - 1;
> > + sps = prescaler / 256 - 1;
> > + }
> > +
> > + spin_lock_irqsave(>m->lock, flags);
> > +
> > + /*
> > + * Properly reset timers: stop, reset, set up prescalers, reference
> > + * value and clear event register.
> > + */
> > + clrsetbits_8(tmr->gtcfr, ~(GTCFR_STP(num) | GTCFR_RST(num)),
> > + GTCFR_STP(num) | GTCFR_RST(num));
> > +
> > + setbits8(tmr->gtcfr, GTCFR_STP(num));
> > +
> > + if (tmr->gtpsr)
> > + out_be16(tmr->gtpsr, psr);
> > + clrsetbits_be16(tmr->gtmdr, 0xFFFF, iclk | GTMDR_SPS(sps) |
> > + GTMDR_ORI | (free_run ? GTMDR_FRR : 0));
> > + out_be16(tmr->gtcnr, 0);
> > + out_be16(tmr->gtrfr, reference_value);
> > + out_be16(tmr->gtevr, 0xFFFF);
> > +
> > + /* Let it be. */
> > + clrbits8(tmr->gtcfr, GTCFR_STP(num));
> > +
> > + spin_unlock_irqrestore(>m->lock, flags);
> > +
> > + return 0;
> > +}
> > +/**
> > + * gtm_reset_timer16 - reset 16 bit timer with arbitrary precision
> > + * @tmr: pointer to the gtm_timer structure obtained from gtm_get_timer
> > + * @usec: timer interval in microseconds
> > + * @reload: if set, the timer will reset upon expiry rather than
> > + * continue running free.
> > + *
> > + * This function (re)sets the GTM timer so that it counts up to the requested
> > + * interval value, and fires the interrupt when the value is reached. This
> > + * function will reduce the precision of the timer as needed in order for the
> > + * requested timeout to fit in a 16-bit register.
> > + */
> > +int gtm_reset_timer16(struct gtm_timer *tmr, unsigned long usec, bool reload)
> > +{
> > + /* quite obvious, frequency which is enough for µSec precision */
> > + int freq = 1000000;
> > + unsigned int bit;
> > +
> > + bit = fls_long(usec);
> > + if (bit > 15) {
> > + freq >>= bit - 15;
> > + usec >>= bit - 15;
> > + }
> > +
> > + if (!freq)
> > + return -EINVAL;
> > +
> > + return gtm_reset_ref_timer16(tmr, freq, usec, reload);
> > +}
> > +EXPORT_SYMBOL(gtm_reset_timer16);
> > +
> > +/**
> > + * gtm_reset_utimer16 - reset 16 bits timer
> > + * @tmr: pointer to the gtm_timer structure obtained from gtm_get_timer
> > + * @usec: timer interval in microseconds
> > + * @reload: if set, the timer will reset upon expiry rather than
> > + * continue running free.
> > + *
> > + * This function (re)sets GTM timer so that it counts up to the requested
> > + * interval value, and fires the interrupt when the value is reached. If reload
> > + * flag was set, timer will also reset itself upon reference value, otherwise
> > + * it continues to increment.
> > + */
> > +int gtm_reset_utimer16(struct gtm_timer *tmr, u16 usec, bool reload)
> > +{
> > + /* quite obvious, frequency which is enough for µSec precision */
> > + const int freq = 1000000;
> > +
> > + /*
> > + * We can lower the frequency (and probably power consumption) by
> > + * dividing both frequency and usec by 2 until there is no remainder.
> > + * But we won't bother with this unless savings are measured, so just
> > + * run the timer as is.
> > + */
> > +
> > + return gtm_reset_ref_timer16(tmr, freq, usec, reload);
> > +}
> > +EXPORT_SYMBOL(gtm_reset_utimer16);
> > +
> > +/**
> > + * gtm_stop_timer16 - stop single timer
> > + * @tmr: pointer to the gtm_timer structure obtained from gtm_get_timer
> > + *
> > + * This function simply stops the GTM timer.
> > + */
> > +void gtm_stop_timer16(struct gtm_timer *tmr)
> > +{
> > + struct gtm *gtm = tmr->gtm;
> > + int num = tmr - >m->timers[0];
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(>m->lock, flags);
> > +
> > + setbits8(tmr->gtcfr, GTCFR_STP(num));
> > + out_be16(tmr->gtevr, 0xFFFF);
> > +
> > + spin_unlock_irqrestore(>m->lock, flags);
> > +}
> > +EXPORT_SYMBOL(gtm_stop_timer16);
> > +
> > +static void __init gtm_set_shortcuts(struct device_node *np,
> > + struct gtm_timer *timers,
> > + struct gtm_timers_regs __iomem *regs)
> > +{
> > + /*
> > + * Yeah, I don't like this either, but timers' registers a bit messed,
> > + * so we have to provide shortcuts to write timer independent code.
> > + * Alternative option is to create gt*() accessors, but that will be
> > + * even uglier and cryptic.
> > + */
> > + timers[0].gtcfr = ®s->gtcfr1;
> > + timers[0].gtmdr = ®s->gtmdr1;
> > + timers[0].gtcnr = ®s->gtcnr1;
> > + timers[0].gtrfr = ®s->gtrfr1;
> > + timers[0].gtevr = ®s->gtevr1;
> > +
> > + timers[1].gtcfr = ®s->gtcfr1;
> > + timers[1].gtmdr = ®s->gtmdr2;
> > + timers[1].gtcnr = ®s->gtcnr2;
> > + timers[1].gtrfr = ®s->gtrfr2;
> > + timers[1].gtevr = ®s->gtevr2;
> > +
> > + timers[2].gtcfr = ®s->gtcfr2;
> > + timers[2].gtmdr = ®s->gtmdr3;
> > + timers[2].gtcnr = ®s->gtcnr3;
> > + timers[2].gtrfr = ®s->gtrfr3;
> > + timers[2].gtevr = ®s->gtevr3;
> > +
> > + timers[3].gtcfr = ®s->gtcfr2;
> > + timers[3].gtmdr = ®s->gtmdr4;
> > + timers[3].gtcnr = ®s->gtcnr4;
> > + timers[3].gtrfr = ®s->gtrfr4;
> > + timers[3].gtevr = ®s->gtevr4;
> > +
> > + /* CPM2 doesn't have primary prescaler */
> > + if (!of_device_is_compatible(np, "fsl,cpm2-gtm")) {
> > + timers[0].gtpsr = ®s->gtpsr1;
> > + timers[1].gtpsr = ®s->gtpsr2;
> > + timers[2].gtpsr = ®s->gtpsr3;
> > + timers[3].gtpsr = ®s->gtpsr4;
> > + }
> > +}
> > +
> > +static int __init gtm_init(void)
> > +{
> > + struct device_node *np;
> > +
> > + for_each_compatible_node(np, NULL, "fsl,gtm") {
> > + int i;
> > + struct gtm *gtm;
> > + const u32 *clock;
> > + int size;
> > +
> > + gtm = kzalloc(sizeof(*gtm), GFP_KERNEL);
> > + if (!gtm) {
> > + pr_err("%s: unable to allocate memory\n",
> > + np->full_name);
> > + continue;
> > + }
> > +
> > + spin_lock_init(>m->lock);
> > +
> > + clock = of_get_property(np, "clock-frequency", &size);
> > + if (!clock || size != sizeof(*clock)) {
> > + pr_err("%s: no clock-frequency\n", np->full_name);
> > + goto err;
> > + }
> > + gtm->clock = *clock;
> > +
> > + for (i = 0; i < ARRAY_SIZE(gtm->timers); i++) {
> > + int ret;
> > + struct resource irq;
> > +
> > + ret = of_irq_to_resource(np, i, &irq);
> > + if (ret == NO_IRQ) {
> > + pr_err("%s: not enough interrupts specified\n",
> > + np->full_name);
> > + goto err;
> > + }
> > + gtm->timers[i].irq = irq.start;
> > + gtm->timers[i].gtm = gtm;
> > + }
> > +
> > + gtm->regs = of_iomap(np, 0);
> > + if (!gtm->regs) {
> > + pr_err("%s: unable to iomap registers\n",
> > + np->full_name);
> > + goto err;
> > + }
> > +
> > + gtm_set_shortcuts(np, gtm->timers, gtm->regs);
> > +
> > + /* We don't want to lose the node and its ->data */
> > + of_node_get(np);
> > + np->data = gtm;
> > +
> > + continue;
> > +err:
> > + kfree(gtm);
> > + }
> > + return 0;
> > +}
> > +arch_initcall(gtm_init);
> > diff --git a/include/asm-powerpc/fsl_gtm.h b/include/asm-powerpc/fsl_gtm.h
> > new file mode 100644
> > index 0000000..3eafb21
> > --- /dev/null
> > +++ b/include/asm-powerpc/fsl_gtm.h
> > @@ -0,0 +1,108 @@
> > +/*
> > + * Freescale General-purpose Timers Module
> > + *
> > + * Copyright (c) Freescale Semicondutor, Inc. 2006.
> > + * Shlomi Gridish <gridish@freescale.com>
> > + * Jerry Huang <Chang-Ming.Huang@freescale.com>
> > + * Copyright (c) MontaVista Software, Inc. 2008.
> > + * Anton Vorontsov <avorontsov@ru.mvista.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.
> > + */
> > +
> > +#ifndef __ASM_FSL_GTM_H
> > +#define __ASM_FSL_GTM_H
> > +
> > +#include <linux/types.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/io.h>
> > +
> > +#define GTCFR_STP(x) ((x) & 1 ? 1 << 5 : 1 << 1)
> > +#define GTCFR_RST(x) ((x) & 1 ? 1 << 4 : 1 << 0)
> > +
> > +#define GTMDR_ICLK_MASK (3 << 1)
> > +#define GTMDR_ICLK_ICAS (0 << 1)
> > +#define GTMDR_ICLK_ICLK (1 << 1)
> > +#define GTMDR_ICLK_SLGO (2 << 1)
> > +#define GTMDR_FRR (1 << 3)
> > +#define GTMDR_ORI (1 << 4)
> > +#define GTMDR_SPS(x) ((x) << 8)
> > +
> > +struct gtm_timers_regs {
> > + u8 gtcfr1; /* Timer 1, Timer 2 global config register */
> > + u8 res0[0x3];
> > + u8 gtcfr2; /* Timer 3, timer 4 global config register */
> > + u8 res1[0xB];
> > + __be16 gtmdr1; /* Timer 1 mode register */
> > + __be16 gtmdr2; /* Timer 2 mode register */
> > + __be16 gtrfr1; /* Timer 1 reference register */
> > + __be16 gtrfr2; /* Timer 2 reference register */
> > + __be16 gtcpr1; /* Timer 1 capture register */
> > + __be16 gtcpr2; /* Timer 2 capture register */
> > + __be16 gtcnr1; /* Timer 1 counter */
> > + __be16 gtcnr2; /* Timer 2 counter */
> > + __be16 gtmdr3; /* Timer 3 mode register */
> > + __be16 gtmdr4; /* Timer 4 mode register */
> > + __be16 gtrfr3; /* Timer 3 reference register */
> > + __be16 gtrfr4; /* Timer 4 reference register */
> > + __be16 gtcpr3; /* Timer 3 capture register */
> > + __be16 gtcpr4; /* Timer 4 capture register */
> > + __be16 gtcnr3; /* Timer 3 counter */
> > + __be16 gtcnr4; /* Timer 4 counter */
> > + __be16 gtevr1; /* Timer 1 event register */
> > + __be16 gtevr2; /* Timer 2 event register */
> > + __be16 gtevr3; /* Timer 3 event register */
> > + __be16 gtevr4; /* Timer 4 event register */
> > + __be16 gtpsr1; /* Timer 1 prescale register */
> > + __be16 gtpsr2; /* Timer 2 prescale register */
> > + __be16 gtpsr3; /* Timer 3 prescale register */
> > + __be16 gtpsr4; /* Timer 4 prescale register */
> > + u8 res2[0x40];
> > +} __attribute__ ((packed));
> > +
> > +struct gtm_timer {
> > + unsigned int irq;
> > +
> > + struct gtm *gtm;
> > + bool requested;
> > + u8 __iomem *gtcfr;
> > + __be16 __iomem *gtmdr;
> > + __be16 __iomem *gtpsr;
> > + __be16 __iomem *gtcnr;
> > + __be16 __iomem *gtrfr;
> > + __be16 __iomem *gtevr;
> > +};
> > +
> > +struct gtm {
> > + unsigned int clock;
> > + struct gtm_timers_regs __iomem *regs;
> > + struct gtm_timer timers[4];
> > + spinlock_t lock;
> > +};
>
> Consider moving the register definitions and structures inside the .c
> file. This is all anonymous data as far as the caller is concerned.
> If the structures are only access inside the driver then you gain
> yourself a bit of safety by moving them.
Ok.
> > +
> > +extern struct gtm_timer *gtm_get_timer(int width);
> > +extern struct gtm_timer *gtm_get_specific_timer(struct gtm *gtm, int timer,
> > + int width);
> > +extern void gtm_put_timer(struct gtm_timer *tmr);
> > +extern int gtm_reset_timer16(struct gtm_timer *tmr, unsigned long usec,
> > + bool reload);
> > +extern int gtm_reset_utimer16(struct gtm_timer *tmr, u16 usec, bool reload);
> > +extern void gtm_stop_timer16(struct gtm_timer *tmr);
> > +
> > +/**
> > + * gtm_ack_timer16 - acknowledge timer event (free-run timers only)
> > + * @tmr: pointer to the gtm_timer structure obtained from gtm_get_timer
> > + * @events: events mask to ack
> > + *
> > + * Thus function used to acknowledge timer interrupt event, use it inside the
> > + * interrupt handler.
> > + */
> > +static inline void gtm_ack_timer16(struct gtm_timer *tmr, u16 events)
> > +{
> > + out_be16(tmr->gtevr, events);
> > +}
>
> Drop 'inline' and expect gcc to do the right thing.
Not sure about newer gccs, but older complained about "defined but not
used" functions w/o inline keyword. I'm almost sure that this is still
true. I can surely use __maybe_unused, but "static inline" is pretty
settled tradition, plus this function is small enough anyway.
--
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
^ permalink raw reply
* Re: [PATCH2/2] [POWERPC] CPM1: implement GPIO LIB API on CPM1 Freescale SoC.
From: Grant Likely @ 2008-04-21 14:30 UTC (permalink / raw)
To: Jochen Friedrich; +Cc: Scott Wood, linuxppc-dev list
In-Reply-To: <4808D606.5070309@scram.de>
On Fri, Apr 18, 2008 at 11:10 AM, Jochen Friedrich <jochen@scram.de> wrote:
> This patch implement GPIO LIB support for the CPM1 GPIOs.
>
> Signed-off-by: Jochen Friedrich <jochen@scram.de>
> ---
> arch/powerpc/platforms/8xx/Kconfig | 10 ++
> arch/powerpc/sysdev/cpm1.c | 261 +++++++++++++++++++++++++++++++++++-
> 2 files changed, 266 insertions(+), 5 deletions(-)
2 quick comments:
1. You need to include binding documentation to booting without of
2. You need to specifiy exact chip names in your compatible string.
"fsl,cpm1-pario-<bank>" is a made up thing.
Cheers,
g.
>
> diff --git a/arch/powerpc/platforms/8xx/Kconfig b/arch/powerpc/platforms/8xx/Kconfig
> index 6fc849e..3488bb7 100644
> --- a/arch/powerpc/platforms/8xx/Kconfig
> +++ b/arch/powerpc/platforms/8xx/Kconfig
> @@ -105,6 +105,16 @@ config 8xx_COPYBACK
>
> If in doubt, say Y here.
>
> +config 8xx_GPIO
> + bool "GPIO API Support"
> + select GENERIC_GPIO
> + select HAVE_GPIO_LIB
> + help
> + Saying Y here will cause the ports on an MPC8xx processor to be used
> + with the GPIO API. If you say N here, the kernel needs less memory.
> +
> + If in doubt, say Y here.
> +
> config 8xx_CPU6
> bool "CPU6 Silicon Errata (860 Pre Rev. C)"
> help
> diff --git a/arch/powerpc/sysdev/cpm1.c b/arch/powerpc/sysdev/cpm1.c
> index 58292a0..01ae3ae 100644
> --- a/arch/powerpc/sysdev/cpm1.c
> +++ b/arch/powerpc/sysdev/cpm1.c
> @@ -30,6 +30,7 @@
> #include <linux/interrupt.h>
> #include <linux/irq.h>
> #include <linux/module.h>
> +#include <linux/spinlock.h>
> #include <asm/page.h>
> #include <asm/pgtable.h>
> #include <asm/8xx_immap.h>
> @@ -42,6 +43,10 @@
>
> #include <asm/fs_pd.h>
>
> +#ifdef CONFIG_8xx_GPIO
> +#include <linux/of_gpio.h>
> +#endif
> +
> #define CPM_MAP_SIZE (0x4000)
>
> cpm8xx_t __iomem *cpmp; /* Pointer to comm processor space */
> @@ -290,20 +295,24 @@ struct cpm_ioport16 {
> __be16 res[3];
> };
>
> -struct cpm_ioport32 {
> - __be32 dir, par, sor;
> +struct cpm_ioport32b {
> + __be32 dir, par, odr, dat;
> +};
> +
> +struct cpm_ioport32e {
> + __be32 dir, par, sor, odr, dat;
> };
>
> static void cpm1_set_pin32(int port, int pin, int flags)
> {
> - struct cpm_ioport32 __iomem *iop;
> + struct cpm_ioport32e __iomem *iop;
> pin = 1 << (31 - pin);
>
> if (port == CPM_PORTB)
> - iop = (struct cpm_ioport32 __iomem *)
> + iop = (struct cpm_ioport32e __iomem *)
> &mpc8xx_immr->im_cpm.cp_pbdir;
> else
> - iop = (struct cpm_ioport32 __iomem *)
> + iop = (struct cpm_ioport32e __iomem *)
> &mpc8xx_immr->im_cpm.cp_pedir;
>
> if (flags & CPM_PIN_OUTPUT)
> @@ -498,3 +507,245 @@ int cpm1_clk_setup(enum cpm_clk_target target, int clock, int mode)
>
> return 0;
> }
> +
> +/*
> + * GPIO LIB API implementation
> + */
> +#ifdef CONFIG_8xx_GPIO
> +
> +struct cpm1_gpio16_chip {
> + struct of_mm_gpio_chip mm_gc;
> + spinlock_t lock;
> +
> + /* shadowed data register to clear/set bits safely */
> + u16 cpdata;
> +};
> +
> +static inline struct cpm1_gpio16_chip *
> +to_cpm1_gpio16_chip(struct of_mm_gpio_chip *mm_gc)
> +{
> + return container_of(mm_gc, struct cpm1_gpio16_chip, mm_gc);
> +}
> +
> +static void cpm1_gpio16_save_regs(struct of_mm_gpio_chip *mm_gc)
> +{
> + struct cpm1_gpio16_chip *cpm1_gc = to_cpm1_gpio16_chip(mm_gc);
> + struct cpm_ioport16 __iomem *iop = mm_gc->regs;
> +
> + cpm1_gc->cpdata = in_be16(&iop->dat);
> +}
> +
> +static int cpm1_gpio16_get(struct gpio_chip *gc, unsigned int gpio)
> +{
> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> + struct cpm_ioport16 __iomem *iop = mm_gc->regs;
> + u16 pin_mask;
> +
> + pin_mask = 1 << (15 - gpio);
> +
> + return !!(in_be16(&iop->dat) & pin_mask);
> +}
> +
> +static void cpm1_gpio16_set(struct gpio_chip *gc, unsigned int gpio, int value)
> +{
> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> + struct cpm1_gpio16_chip *cpm1_gc = to_cpm1_gpio16_chip(mm_gc);
> + struct cpm_ioport16 __iomem *iop = mm_gc->regs;
> + unsigned long flags;
> + u16 pin_mask = 1 << (15 - gpio);
> +
> + spin_lock_irqsave(&cpm1_gc->lock, flags);
> +
> + if (value)
> + cpm1_gc->cpdata |= pin_mask;
> + else
> + cpm1_gc->cpdata &= ~pin_mask;
> +
> + out_be16(&iop->dat, cpm1_gc->cpdata);
> +
> + spin_unlock_irqrestore(&cpm1_gc->lock, flags);
> +}
> +
> +static int cpm1_gpio16_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
> +{
> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> + struct cpm_ioport16 __iomem *iop = mm_gc->regs;
> + u16 pin_mask;
> +
> + pin_mask = 1 << (15 - gpio);
> +
> + setbits16(&iop->dir, pin_mask);
> +
> + cpm1_gpio16_set(gc, gpio, val);
> +
> + return 0;
> +}
> +
> +static int cpm1_gpio16_dir_in(struct gpio_chip *gc, unsigned int gpio)
> +{
> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> + struct cpm_ioport16 __iomem *iop = mm_gc->regs;
> + u16 pin_mask;
> +
> + pin_mask = 1 << (15 - gpio);
> +
> + clrbits16(&iop->dir, pin_mask);
> +
> + return 0;
> +}
> +
> +int cpm1_gpiochip_add16(struct device_node *np)
> +{
> + struct cpm1_gpio16_chip *cpm1_gc;
> + struct of_mm_gpio_chip *mm_gc;
> + struct of_gpio_chip *of_gc;
> + struct gpio_chip *gc;
> +
> + cpm1_gc = kzalloc(sizeof(*cpm1_gc), GFP_KERNEL);
> + if (!cpm1_gc)
> + return -ENOMEM;
> +
> + spin_lock_init(&cpm1_gc->lock);
> +
> + mm_gc = &cpm1_gc->mm_gc;
> + of_gc = &mm_gc->of_gc;
> + gc = &of_gc->gc;
> +
> + mm_gc->save_regs = cpm1_gpio16_save_regs;
> + of_gc->gpio_cells = 1;
> + gc->ngpio = 16;
> + gc->direction_input = cpm1_gpio16_dir_in;
> + gc->direction_output = cpm1_gpio16_dir_out;
> + gc->get = cpm1_gpio16_get;
> + gc->set = cpm1_gpio16_set;
> +
> + return of_mm_gpiochip_add(np, mm_gc);
> +}
> +
> +struct cpm1_gpio32_chip {
> + struct of_mm_gpio_chip mm_gc;
> + spinlock_t lock;
> +
> + /* shadowed data register to clear/set bits safely */
> + u32 cpdata;
> +};
> +
> +static inline struct cpm1_gpio32_chip *
> +to_cpm1_gpio32_chip(struct of_mm_gpio_chip *mm_gc)
> +{
> + return container_of(mm_gc, struct cpm1_gpio32_chip, mm_gc);
> +}
> +
> +static void cpm1_gpio32_save_regs(struct of_mm_gpio_chip *mm_gc)
> +{
> + struct cpm1_gpio32_chip *cpm1_gc = to_cpm1_gpio32_chip(mm_gc);
> + struct cpm_ioport32b __iomem *iop = mm_gc->regs;
> +
> + cpm1_gc->cpdata = in_be32(&iop->dat);
> +}
> +
> +static int cpm1_gpio32_get(struct gpio_chip *gc, unsigned int gpio)
> +{
> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> + struct cpm_ioport32b __iomem *iop = mm_gc->regs;
> + u32 pin_mask;
> +
> + pin_mask = 1 << (31 - gpio);
> +
> + return !!(in_be32(&iop->dat) & pin_mask);
> +}
> +
> +static void cpm1_gpio32_set(struct gpio_chip *gc, unsigned int gpio, int value)
> +{
> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> + struct cpm1_gpio32_chip *cpm1_gc = to_cpm1_gpio32_chip(mm_gc);
> + struct cpm_ioport32b __iomem *iop = mm_gc->regs;
> + unsigned long flags;
> + u32 pin_mask = 1 << (31 - gpio);
> +
> + spin_lock_irqsave(&cpm1_gc->lock, flags);
> +
> + if (value)
> + cpm1_gc->cpdata |= pin_mask;
> + else
> + cpm1_gc->cpdata &= ~pin_mask;
> +
> + out_be32(&iop->dat, cpm1_gc->cpdata);
> +
> + spin_unlock_irqrestore(&cpm1_gc->lock, flags);
> +}
> +
> +static int cpm1_gpio32_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
> +{
> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> + struct cpm_ioport32b __iomem *iop = mm_gc->regs;
> + u32 pin_mask;
> +
> + pin_mask = 1 << (31 - gpio);
> +
> + setbits32(&iop->dir, pin_mask);
> +
> + cpm1_gpio32_set(gc, gpio, val);
> +
> + return 0;
> +}
> +
> +static int cpm1_gpio32_dir_in(struct gpio_chip *gc, unsigned int gpio)
> +{
> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> + struct cpm_ioport32b __iomem *iop = mm_gc->regs;
> + u32 pin_mask;
> +
> + pin_mask = 1 << (31 - gpio);
> +
> + clrbits32(&iop->dir, pin_mask);
> +
> + return 0;
> +}
> +
> +int cpm1_gpiochip_add32(struct device_node *np)
> +{
> + struct cpm1_gpio32_chip *cpm1_gc;
> + struct of_mm_gpio_chip *mm_gc;
> + struct of_gpio_chip *of_gc;
> + struct gpio_chip *gc;
> +
> + cpm1_gc = kzalloc(sizeof(*cpm1_gc), GFP_KERNEL);
> + if (!cpm1_gc)
> + return -ENOMEM;
> +
> + spin_lock_init(&cpm1_gc->lock);
> +
> + mm_gc = &cpm1_gc->mm_gc;
> + of_gc = &mm_gc->of_gc;
> + gc = &of_gc->gc;
> +
> + mm_gc->save_regs = cpm1_gpio32_save_regs;
> + of_gc->gpio_cells = 1;
> + gc->ngpio = 32;
> + gc->direction_input = cpm1_gpio32_dir_in;
> + gc->direction_output = cpm1_gpio32_dir_out;
> + gc->get = cpm1_gpio32_get;
> + gc->set = cpm1_gpio32_set;
> +
> + return of_mm_gpiochip_add(np, mm_gc);
> +}
> +
> +static int cpm_init_par_io(void)
> +{
> + struct device_node *np;
> +
> + for_each_compatible_node(np, NULL, "fsl,cpm1-pario-bank16")
> + cpm1_gpiochip_add16(np);
> +
> + for_each_compatible_node(np, NULL, "fsl,cpm1-pario-bank32b")
> + cpm1_gpiochip_add32(np);
> +
> + /* Port E uses CPM2 layout */
> + for_each_compatible_node(np, NULL, "fsl,cpm1-pario-bank32e")
> + cpm2_gpiochip_add32(np);
> + return 0;
> +}
> +arch_initcall(cpm_init_par_io);
> +
> +#endif /* CONFIG_8xx_GPIO */
> --
> 1.5.5
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH 4/5] [POWERPC] QE: implement support for the GPIO LIB API
From: Anton Vorontsov @ 2008-04-21 14:33 UTC (permalink / raw)
To: Grant Likely; +Cc: David Brownell, linuxppc-dev
In-Reply-To: <fa686aa40804210719x475698f8n3b82e795547bfc70@mail.gmail.com>
On Mon, Apr 21, 2008 at 08:19:05AM -0600, Grant Likely wrote:
> On Fri, Apr 18, 2008 at 1:09 PM, Anton Vorontsov
> <avorontsov@ru.mvista.com> wrote:
> > This is needed to access QE GPIOs via Linux GPIO API.
> >
> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > ---
> > diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
> > index 827b630..5c9cfab 100644
> > --- a/Documentation/powerpc/booting-without-of.txt
> > +++ b/Documentation/powerpc/booting-without-of.txt
> > @@ -1721,24 +1721,32 @@ platforms are moved over to use the flattened-device-tree model.
> > information.
> >
> > Required properties:
> > - - device_type : should be "par_io".
> > + - #gpio-cells : should be "2".
> > + - compatible : should be "fsl,qe-pario-bank-<bank>", "fsl,qe-pario-bank"
>
> Once again; I don't like the generic compatible values. Please
> include the exact chip name in the string. ie: "fsl,<chip>-qe-pario".
>
> "fsl,qe-pario-bank" is not a real thing. If you want a common
> compatible string that the driver can bind against, then choose one
> real part and add it to the compatible list for all the other parts.
>
> Also, why is <bank> encoded in compatible? Do the different banks
> have different register interfaces?
Yes, they could. For example, interrupt pins are bank-specific.
> > - reg : offset to the register set and its length.
> > - - num-ports : number of Parallel I/O ports
> > + - gpio-controller : node to identify gpio controllers.
> >
> > - Example:
> > - par_io@1400 {
> > - reg = <1400 100>;
> > - #address-cells = <1>;
> > - #size-cells = <0>;
> > - device_type = "par_io";
> > - num-ports = <7>;
> > - ucc_pin@01 {
> > - ......
> > - };
> > + For example, two QE Par I/O banks:
> > + qe_pio_a: gpio-controller@1400 {
> > + #gpio-cells = <2>;
> > + compatible = "fsl,qe-pario-bank-a", "fsl,qe-pario-bank";
> > + reg = <0x1400 0x18>;
> > + gpio-controller;
> > + };
> >
> > + qe_pio_e: gpio-controller@1460 {
> > + #gpio-cells = <2>;
> > + compatible = "fsl,qe-pario-bank-e", "fsl,qe-pario-bank";
> > + reg = <0x1460 0x18>;
> > + gpio-controller;
> > + };
> >
> > vi) Pin configuration nodes
> >
> > + NOTE: pin configuration nodes are obsolete. Usually, their existance
> > + is an evidence of the firmware shortcomings. Such fixups are
> > + better handled by the Linux board file, not the device tree.
> > +
> > Required properties:
> > - linux,phandle : phandle of this node; likely referenced by a QE
> > device.
> > diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
> > index f38c50b..f6eecd1 100644
> > --- a/arch/powerpc/platforms/Kconfig
> > +++ b/arch/powerpc/platforms/Kconfig
> > @@ -270,6 +270,8 @@ config QUICC_ENGINE
> > bool
> > select PPC_LIB_RHEAP
> > select CRC32
> > + select GENERIC_GPIO
> > + select HAVE_GPIO_LIB
> > help
> > The QUICC Engine (QE) is a new generation of communications
> > coprocessors on Freescale embedded CPUs (akin to CPM in older chips).
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index bbd2834..cee56f9 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -25,6 +25,15 @@ config DEBUG_GPIO
> >
> > # put expanders in the right section, in alphabetical order
> >
> > +comment "On-chip GPIOs:"
> > +
> > +config GPIO_QE
> > + bool "QUICC Engine GPIOs"
> > + depends on QUICC_ENGINE
> > + help
> > + Say Y here to use GPIOs on the Freescale PowerPC CPUs with
> > + QUICC Engine block.
> > +
> > comment "I2C GPIO expanders:"
> >
> > config GPIO_PCA953X
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index fdde992..fd0a41f 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -7,3 +7,4 @@ obj-$(CONFIG_HAVE_GPIO_LIB) += gpiolib.o
> > obj-$(CONFIG_GPIO_MCP23S08) += mcp23s08.o
> > obj-$(CONFIG_GPIO_PCA953X) += pca953x.o
> > obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o
> > +obj-$(CONFIG_GPIO_QE) += qe.o
>
> Since this isn't an of_platform or a platform driver; I'd put it into
> arch/powerpc instead.
Heh.
> > diff --git a/drivers/gpio/qe.c b/drivers/gpio/qe.c
> > new file mode 100644
> > index 0000000..474bc44
> > --- /dev/null
> > +++ b/drivers/gpio/qe.c
> > +static int __init qe_add_gpiochips(void)
> > +{
> > + int ret;
> > + struct device_node *np;
> > +
> > + for_each_compatible_node(np, NULL, "fsl,qe-pario-bank") {
> > + struct qe_gpio_chip *qe_gc;
> > + struct of_mm_gpio_chip *mm_gc;
> > + struct of_gpio_chip *of_gc;
> > + struct gpio_chip *gc;
> > +
> > + qe_gc = kzalloc(sizeof(*qe_gc), GFP_KERNEL);
> > + if (!qe_gc) {
> > + ret = -ENOMEM;
> > + goto err;
> > + }
> > +
> > + spin_lock_init(&qe_gc->lock);
> > +
> > + mm_gc = &qe_gc->mm_gc;
> > + of_gc = &mm_gc->of_gc;
> > + gc = &of_gc->gc;
> > +
> > + mm_gc->save_regs = qe_gpio_save_regs;
> > + of_gc->gpio_cells = 2;
> > + gc->ngpio = QE_PIO_PINS;
> > + gc->direction_input = qe_gpio_dir_in;
> > + gc->direction_output = qe_gpio_dir_out;
> > + gc->get = qe_gpio_get;
> > + gc->set = qe_gpio_set;
> > +
> > + ret = of_mm_gpiochip_add(np, mm_gc);
> > + if (ret)
> > + goto err;
> > + }
> > +
> > + return 0;
> > +err:
> > + pr_err("%s: registration failed with status %d\n", np->full_name, ret);
> > + of_node_put(np);
> > + return ret;
> > +}
> > +arch_initcall(qe_add_gpiochips);
>
> Should this really be a arch_initcall()? Would it be better for
> platforms needing it to call it explicitly from one of the platform's
> machine_arch_initcall()? Otherwise it gets called for all platforms
> in a multiplatform kernel.
Ok, I'll place it into qe_reset().
--
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
^ permalink raw reply
* Re: [PATCH 4/5] [POWERPC] QE: implement support for the GPIO LIB API
From: Anton Vorontsov @ 2008-04-21 14:49 UTC (permalink / raw)
To: Grant Likely; +Cc: David Brownell, linuxppc-dev
In-Reply-To: <20080421143313.GB18008@polina.dev.rtsoft.ru>
On Mon, Apr 21, 2008 at 06:33:13PM +0400, Anton Vorontsov wrote:
[...]
> > > +static int __init qe_add_gpiochips(void)
> > > +{
> > > + int ret;
> > > + struct device_node *np;
> > > +
> > > + for_each_compatible_node(np, NULL, "fsl,qe-pario-bank") {
> > > + struct qe_gpio_chip *qe_gc;
> > > + struct of_mm_gpio_chip *mm_gc;
> > > + struct of_gpio_chip *of_gc;
> > > + struct gpio_chip *gc;
> > > +
> > > + qe_gc = kzalloc(sizeof(*qe_gc), GFP_KERNEL);
> > > + if (!qe_gc) {
> > > + ret = -ENOMEM;
> > > + goto err;
> > > + }
> > > +
> > > + spin_lock_init(&qe_gc->lock);
> > > +
> > > + mm_gc = &qe_gc->mm_gc;
> > > + of_gc = &mm_gc->of_gc;
> > > + gc = &of_gc->gc;
> > > +
> > > + mm_gc->save_regs = qe_gpio_save_regs;
> > > + of_gc->gpio_cells = 2;
> > > + gc->ngpio = QE_PIO_PINS;
> > > + gc->direction_input = qe_gpio_dir_in;
> > > + gc->direction_output = qe_gpio_dir_out;
> > > + gc->get = qe_gpio_get;
> > > + gc->set = qe_gpio_set;
> > > +
> > > + ret = of_mm_gpiochip_add(np, mm_gc);
> > > + if (ret)
> > > + goto err;
> > > + }
> > > +
> > > + return 0;
> > > +err:
> > > + pr_err("%s: registration failed with status %d\n", np->full_name, ret);
> > > + of_node_put(np);
> > > + return ret;
> > > +}
> > > +arch_initcall(qe_add_gpiochips);
> >
> > Should this really be a arch_initcall()? Would it be better for
> > platforms needing it to call it explicitly from one of the platform's
> > machine_arch_initcall()? Otherwise it gets called for all platforms
> > in a multiplatform kernel.
>
> Ok, I'll place it into qe_reset().
Ugh, no, I can't. qe_reset() is called too early. And I don't like
having every QE board file to call this through machine_arch_initcall...
Hmm, what to do.. well, is this thaat bad to call this for every
platform?..
--
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox