* RE: [RFC PATCH 06/15] dt: xilinx_hwicap: merge platform and of_platformdriver bindings
From: Stephen Neuendorffer @ 2011-02-25 18:25 UTC (permalink / raw)
To: Grant Likely, Benjamin Herrenschmidt, devicetree-discuss, sfr,
linux-kernel, davem
Cc: sparclinux, linuxppc-dev
In-Reply-To: <20110223043405.20795.19903.stgit@localhost6.localdomain6>
Minor nit below
Acked-by: Stephen Neuendorffer <stephen.neuendorffer@xilinx.com>
Steve
> -----Original Message-----
> From: linuxppc-dev-bounces+stephen=3Dneuendorffer.name@lists.ozlabs.org
[mailto:linuxppc-dev-
> bounces+stephen=3Dneuendorffer.name@lists.ozlabs.org] On Behalf Of Grant
Likely
> Sent: Tuesday, February 22, 2011 8:34 PM
> To: Benjamin Herrenschmidt; devicetree-discuss@lists.ozlabs.org;
sfr@canb.auug.org.au; linux-
> kernel@vger.kernel.org; davem@davemloft.net
> Cc: sparclinux@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
> Subject: [RFC PATCH 06/15] dt: xilinx_hwicap: merge platform and
of_platformdriver bindings
> =
> of_platform_driver is getting removed, and a single platform_driver
> can now support both devicetree and non-devicetree use cases. This
> patch merges the two driver registrations.
> =
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
> drivers/char/xilinx_hwicap/xilinx_hwicap.c | 129
+++++++++++-----------------
> 1 files changed, 49 insertions(+), 80 deletions(-)
> =
> diff --git a/drivers/char/xilinx_hwicap/xilinx_hwicap.c
b/drivers/char/xilinx_hwicap/xilinx_hwicap.c
> index 9f2272e..a55e877 100644
> --- a/drivers/char/xilinx_hwicap/xilinx_hwicap.c
> +++ b/drivers/char/xilinx_hwicap/xilinx_hwicap.c
> @@ -714,20 +714,29 @@ static int __devexit hwicap_remove(struct device
*dev)
> return 0; /* success */
> }
> =
> -static int __devinit hwicap_drv_probe(struct platform_device *pdev)
> +#ifdef CONFIG_OF
> +static int __devinit hwicap_of_probe(struct platform_device *op)
> {
> - struct resource *res;
> - const struct config_registers *regs;
> + struct resource res;
> + const unsigned int *id;
> const char *family;
> + int rc;
> + const struct hwicap_driver_config *config =3D
op->dev.of_match->data;
> + const struct config_registers *regs;
> =
> - res =3D platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (!res)
> - return -ENODEV;
> +
> + rc =3D of_address_to_resource(op->dev.of_node, 0, &res);
> + if (rc) {
> + dev_err(&op->dev, "invalid address\n");
> + return rc;
> + }
> +
> + id =3D of_get_property(op->dev.of_node, "port-number", NULL);
> =
> /* It's most likely that we're using V4, if the family is not
> specified */
> regs =3D &v4_config_registers;
> - family =3D pdev->dev.platform_data;
> + family =3D of_get_property(op->dev.of_node, "xlnx,family", NULL);
> =
> if (family) {
> if (!strcmp(family, "virtex2p")) {
> @@ -738,54 +747,33 @@ static int __devinit hwicap_drv_probe(struct
platform_device *pdev)
> regs =3D &v5_config_registers;
> }
> }
> -
> - return hwicap_setup(&pdev->dev, pdev->id, res,
> - &buffer_icap_config, regs);
> + return hwicap_setup(&op->dev, id ? *id : -1, &res, config,
> + regs);
> }
> -
> -static int __devexit hwicap_drv_remove(struct platform_device *pdev)
> +#else
> +static inline int hwicap_of_probe(struct platform_device *op)
> {
> - return hwicap_remove(&pdev->dev);
> + return -EINVAL;
> }
> +#endif /* CONFIG_OF */
> =
> -static struct platform_driver hwicap_platform_driver =3D {
> - .probe =3D hwicap_drv_probe,
> - .remove =3D hwicap_drv_remove,
> - .driver =3D {
> - .owner =3D THIS_MODULE,
> - .name =3D DRIVER_NAME,
> - },
> -};
> -
> -/*
---------------------------------------------------------------------
> - * OF bus binding
> - */
> -
> -#if defined(CONFIG_OF)
> -static int __devinit
> -hwicap_of_probe(struct platform_device *op, const struct of_device_id
*match)
> +static int __devinit hwicap_drv_probe(struct platform_device *pdev)
> {
> - struct resource res;
> - const unsigned int *id;
> - const char *family;
> - int rc;
> - const struct hwicap_driver_config *config =3D match->data;
> + struct resource *res;
> const struct config_registers *regs;
> + const char *family;
> =
> - dev_dbg(&op->dev, "hwicap_of_probe(%p, %p)\n", op, match);
> -
> - rc =3D of_address_to_resource(op->dev.of_node, 0, &res);
> - if (rc) {
> - dev_err(&op->dev, "invalid address\n");
> - return rc;
> - }
> + if (pdev->dev.of_match)
> + return hwicap_of_probe(pdev);
> =
> - id =3D of_get_property(op->dev.of_node, "port-number", NULL);
> + res =3D platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENODEV;
> =
> /* It's most likely that we're using V4, if the family is not
> specified */
> regs =3D &v4_config_registers;
> - family =3D of_get_property(op->dev.of_node, "xlnx,family", NULL);
> + family =3D pdev->dev.platform_data;
> =
> if (family) {
> if (!strcmp(family, "virtex2p")) {
> @@ -796,15 +784,17 @@ hwicap_of_probe(struct platform_device *op,
const struct of_device_id *match)
> regs =3D &v5_config_registers;
> }
> }
> - return hwicap_setup(&op->dev, id ? *id : -1, &res, config,
> - regs);
> +
> + return hwicap_setup(&pdev->dev, pdev->id, res,
> + &buffer_icap_config, regs);
> }
> =
> -static int __devexit hwicap_of_remove(struct platform_device *op)
> +static int __devexit hwicap_drv_remove(struct platform_device *pdev)
> {
> - return hwicap_remove(&op->dev);
> + return hwicap_remove(&pdev->dev);
> }
> =
> +#ifdef CONFIG_OF
> /* Match table for of_platform binding */
Does the comment still make sense?
> static const struct of_device_id __devinitconst hwicap_of_match[] =3D {
> { .compatible =3D "xlnx,opb-hwicap-1.00.b", .data =3D
&buffer_icap_config},
> @@ -812,33 +802,23 @@ static const struct of_device_id __devinitconst
hwicap_of_match[] =3D {
> {},
> };
> MODULE_DEVICE_TABLE(of, hwicap_of_match);
> +#else
> +#define hwicap_of_match NULL
> +#endif
> =
> -static struct of_platform_driver hwicap_of_driver =3D {
> - .probe =3D hwicap_of_probe,
> - .remove =3D __devexit_p(hwicap_of_remove),
> +static struct platform_driver hwicap_platform_driver =3D {
> + .probe =3D hwicap_drv_probe,
> + .remove =3D hwicap_drv_remove,
> .driver =3D {
> - .name =3D DRIVER_NAME,
> .owner =3D THIS_MODULE,
> + .name =3D DRIVER_NAME,
> .of_match_table =3D hwicap_of_match,
> },
> };
> =
> -/* Registration helpers to keep the number of #ifdefs to a minimum */
> -static inline int __init hwicap_of_register(void)
> -{
> - pr_debug("hwicap: calling of_register_platform_driver()\n");
> - return of_register_platform_driver(&hwicap_of_driver);
> -}
> -
> -static inline void __exit hwicap_of_unregister(void)
> -{
> - of_unregister_platform_driver(&hwicap_of_driver);
> -}
> -#else /* CONFIG_OF */
> -/* CONFIG_OF not enabled; do nothing helpers */
> -static inline int __init hwicap_of_register(void) { return 0; }
> -static inline void __exit hwicap_of_unregister(void) { }
> -#endif /* CONFIG_OF */
> +/*
---------------------------------------------------------------------
> + * OF bus binding
> + */
> =
> static int __init hwicap_module_init(void)
> {
> @@ -856,21 +836,12 @@ static int __init hwicap_module_init(void)
> return retval;
> =
> retval =3D platform_driver_register(&hwicap_platform_driver);
> -
> - if (retval)
> - goto failed1;
> -
> - retval =3D hwicap_of_register();
> -
> if (retval)
> - goto failed2;
> + goto failed;
> =
> return retval;
> =
> - failed2:
> - platform_driver_unregister(&hwicap_platform_driver);
> -
> - failed1:
> + failed:
> unregister_chrdev_region(devt, HWICAP_DEVICES);
> =
> return retval;
> @@ -884,8 +855,6 @@ static void __exit hwicap_module_cleanup(void)
> =
> platform_driver_unregister(&hwicap_platform_driver);
> =
> - hwicap_of_unregister();
> -
> unregister_chrdev_region(devt, HWICAP_DEVICES);
> }
> =
> =
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
This email and any attachments are intended for the sole use of the named r=
ecipient(s) and contain(s) confidential information that may be proprietary=
, privileged or copyrighted under applicable law. If you are not the intend=
ed recipient, do not read, copy, or forward this email message or any attac=
hments. Delete this email message and any attachments immediately.
^ permalink raw reply
* Re: Resources used by driver ?
From: Guillaume Dargaud @ 2011-02-25 14:18 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <201102251401.33063.dargaud@lpsc.in2p3.fr>
> When I start oprofile, I get the following:
> # opcontrol --vmlinux=/vmlinux
> The specified file /vmlinux does not seem to be valid
> Make sure you are using the non-compressed image file (e.g. vmlinux not
> vmlinuz)
Sorry, I had an incomplete install of objdump/binutils on my target. I'm now
trying to understand how to use oprofile.
--
Guillaume Dargaud
http://www.gdargaud.net/
^ permalink raw reply
* Re: Resources used by driver ?
From: Guillaume Dargaud @ 2011-02-25 13:01 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <AANLkTin9zsXJX0QrJ2cAZJjAdbGYnQ-S4QCdpzEbrUF_@mail.gmail.com>
On Wednesday 16 February 2011 08:01:10 Srivatsan S wrote:
> You can use oprofile to profile the functions of your module. Follow the
> link.
> http://oprofile.sourceforge.net/ <http://oprofile.sourceforge.net/news/>
OK, after some wrestling, I got oprofile installed in my OS (buildroot 2011.02-
rc1) and my kernel (xilinx patches 2.6.35+).
When I start oprofile, I get the following:
# opcontrol --vmlinux=/vmlinux
The specified file /vmlinux does not seem to be valid
Make sure you are using the non-compressed image file (e.g. vmlinux not
vmlinuz)
# file /vmlinux
/vmlinux: ELF 32-bit MSB executable, PowerPC or cisco 4500, version 1 (SYSV),
statically linked, with unknown capability 0x41000000 = 0x11676e75, with
unknown capability 0x10000 = 0x90402, not stripped
I used the vmlinux generated found in ../linus-2.6-xlnx/vmlinux
What's wrong with it ?
--
Guillaume Dargaud
http://www.gdargaud.net/
^ permalink raw reply
* Re: [RFC PATCH 01/15] dt/powerpc: move of_bus_type infrastructure to ibmebus
From: Benjamin Herrenschmidt @ 2011-02-25 8:53 UTC (permalink / raw)
To: Arnd Bergmann
Cc: sfr, devicetree-discuss, linux-kernel, sparclinux, linuxppc-dev,
davem
In-Reply-To: <201102241546.48790.arnd@arndb.de>
On Thu, 2011-02-24 at 15:46 +0100, Arnd Bergmann wrote:
> On Wednesday 23 February 2011, Grant Likely wrote:
> > arch/powerpc/kernel/ibmebus.c is the only remaining user of the
> > of_bus_type support code for initializing the bus and registering
> > drivers. All others have either been switched to the vanilla platform
> > bus or already have their own infrastructure.
> >
> > This patch moves the functionality that ibmebus is using out of
> > drivers/of/{platform,device}.c and into ibmebus.c where it is actually
> > used. Also renames the moved symbols from of_platform_* to
> > ibmebus_bus_* to reflect the actual usage.
> >
> > This patch is part of moving all of the of_platform_bus_type users
> > over to the platform_bus_type.
> >
> > Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>
> The ibmebus is essentially the platform bus of the IBM Power Systems (a.k.a.
> pSeries a.k.a. System p), I think it would make a lot of sense to convert
> the two drivers (ehca and ehea) on this bus into platform drivers as well.
>
> The original reason for this bus was to provide a different IOMMU for them
> than what is used on the PCI devices. This should now be possible in simpler
> ways.
Well, it's really the IBM "GX" bus but yeah, it could be converted I
suppose. The "special" iommu functions mostly derive from the fact that
the two adapters we are interested in have their own translation
mechanism built-in. The GX bus sits "above" the TCEs used as an iommu on
these systems.
> > +static void ibmebus_bus_device_shutdown(struct device *dev)
> > +{
> > + struct platform_device *of_dev = to_platform_device(dev);
> > + struct of_platform_driver *drv = to_of_platform_driver(dev->driver);
> > +
> > + if (dev->driver && drv->shutdown)
> > + drv->shutdown(of_dev);
> > +}
>
> neither of the drivers provides a shutdown function.
Right but they might grow one ... and people will wonder why it doesn't
work if you remove the above :-)
.../...
> These are also unused in the drivers.
Same reasoning, however it becomes moot if we convert them to platform.
Cheers,
Ben.
^ permalink raw reply
* RE: Open Firmware and interrupt trigger
From: Benjamin Herrenschmidt @ 2011-02-25 8:47 UTC (permalink / raw)
To: Robert Thorhuus; +Cc: avorontsov@mvista.com, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <9081694521850B4D91387C8C2E612B591062A0DD26@ESESSCMS0361.eemea.ericsson.se>
On Fri, 2011-02-25 at 08:29 +0100, Robert Thorhuus wrote:
> Thank you Benjamin!
>
> Sorry for not using your qouting schema :(
>
> Benjamin, you are right about the IRQ flags. Those interrupt.h flags
> seems to differ from my processor reference manual.
Check the bindings or the driver. IE. What interrupt controller does
your board/processor use ? Use the flags defined by the device-tree
bindings for that controller.
> None the less. Antov, I saw that the code snippet I refer to below:
>
> > ret = of_irq_to_resource(dn, 0, &irq_res);
> > if (ret == NO_IRQ)
> > irq_res.start = irq_res.end = 0;
> > else
> > irq_res.flags = 0;
>
> , originates from the first version of the file pata_of_platform.c and
> you are the creator :)
I don't think so :-)
>From the git commit logs:
Author: Anton Vorontsov <avorontsov@ru.mvista.com> 2008-01-10 06:10:41
Committer: Olof Johansson <olof@lixom.net> 2008-01-16 03:23:43
> Could you explain the hardcoded ".flags = 0" part?
> Looking amatuer-vise in the code it seems that the only thing one is
> able to control (through Device Tree) regarding interrupts and pata,
> is the actual IRQ number. Is this a correct assumption?
I think the trick is that of_irq_to_resource() will establish the right
trigger settings already, so the flags might not be relevant from a
driver standpoint. 0 means use the default as established by
of_irq_to_resource() when it creates the interrupt mapping for that
interrupt.
I suspect the problem might be the value in your device-tree being
incorrect. You need to dbl check the binding (ie. the definition of that
value for your specific type of interrupt controller). You won't find
that in the processor documentation but rather in the binding document
describing the way that interrupt controller is represented in a
device-tree.
Tell us what the interrupt controller is (what your platform/processor
is) and we should be able to help you get it right.
Cheers,
Ben.
> Thanks
> BR
> Robert
>
>
> -----Original Message-----
> From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org]
> Sent: den 24 februari 2011 21:47
> To: Robert Thorhuus
> Cc: linuxppc-dev@lists.ozlabs.org
> Subject: Re: Open Firmware and interrupt trigger
>
> On Wed, 2011-02-23 at 22:18 +0100, Robert Thorhuus wrote:
> > Hello!
> >
> > I'm quite new to linux and Open Firmware.
> >
> > I have a PPC processor. To this I have a Compact Flash connected.
> The Compact Flash is using external interrupt 0 of the processor.
> > In my DTS file I have specified a Compact Flash node and within it I
> have an interrupt element:
> > interrupt = <0 2 0 0>;
> >
> > Here I thought the first number was the ID of the interrupt and the
> second one should be a number indicating how the interrupt is
> triggered (high, low, raising, falling).
> >
> > The interrupt is active low.
> >
> > But I could not get it to work which ever value I chose.
> >
> > Looking in the code I found this in function __devinit
> pata_of_platform_probe in file pata_of_platform.c:
> >
> > ret = of_irq_to_resource(dn, 0, &irq_res);
> > if (ret == NO_IRQ)
> > irq_res.start = irq_res.end = 0;
> > else
> > irq_res.flags = 0;
> >
> > Here "flags" will be zero whatever I do in the DTS. As far as I can
> understand the flags are defined in interrupts.h:
> > #define IRQF_TRIGGER_NONE 0x00000000
> > #define IRQF_TRIGGER_RISING 0x00000001
> > #define IRQF_TRIGGER_FALLING 0x00000002
> > #define IRQF_TRIGGER_HIGH 0x00000004
> > #define IRQF_TRIGGER_LOW 0x00000008
>
> Actually, the .dts flags depend on the specific interrupt controller
> you are using. For example, MPIC uses a different mapping scheme (for
> historical reasons). Check booting-without-of.txt.
>
> > So modifying the code to:
> > else
> > irq_res.flags = 2;
> >
> > I get it to work.
> >
> > Could someone please explain to me why the "flags" parameter is
> hardcoded zero or just point in a good direction.
>
> That does indeed look odd. Might be worth trying to figure out with
> the git history who came up with that code in the first place and ask
> that person. Without answer, I think it's valid to patch that out.
>
> Cheers,
> Ben.
>
> > Thank you
> >
> > BR
> > Robert
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
>
^ permalink raw reply
* Re: [PATCH V11 2/4] ptp: Added a clock that uses the eTSEC found on the MPC85xx.
From: Richard Cochran @ 2011-02-25 7:53 UTC (permalink / raw)
To: Scott Wood
Cc: John Stultz, Christoph Lameter, Russell King, Rodolfo Giometti,
Arnd Bergmann, Peter Zijlstra, linux-api, devicetree-discuss,
linux-kernel, Paul Mackerras, Alan Cox, netdev, Mike Frysinger,
Thomas Gleixner, linuxppc-dev, David Miller, linux-arm-kernel,
Krzysztof Halasa
In-Reply-To: <20110224112731.2d9bf080@schlenkerla>
On Thu, Feb 24, 2011 at 11:27:31AM -0600, Scott Wood wrote:
> My vote, if it goes in a separate node at all, is "fsl,etsec-ptp",
So, that is what the patch does.
> and let the driver use SVR.
What is SVR?
Thanks,
Richard
^ permalink raw reply
* RE: Open Firmware and interrupt trigger
From: Robert Thorhuus @ 2011-02-25 7:29 UTC (permalink / raw)
To: avorontsov@mvista.com; +Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <1298580418.8833.428.camel@pasglop>
Thank you Benjamin!
Sorry for not using your qouting schema :(
Benjamin, you are right about the IRQ flags. Those interrupt.h flags seems =
to differ from my processor reference manual.
None the less. Antov, I saw that the code snippet I refer to below:
> ret =3D of_irq_to_resource(dn, 0, &irq_res);
> if (ret =3D=3D NO_IRQ)
> irq_res.start =3D irq_res.end =3D 0;
> else
> irq_res.flags =3D 0;
, originates from the first version of the file pata_of_platform.c and you =
are the creator :)
Could you explain the hardcoded ".flags =3D 0" part?=20
Looking amatuer-vise in the code it seems that the only thing one is able t=
o control (through Device Tree) regarding interrupts and pata, is the actua=
l IRQ number. Is this a correct assumption?
Thanks
BR
Robert
-----Original Message-----
From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org]=20
Sent: den 24 februari 2011 21:47
To: Robert Thorhuus
Cc: linuxppc-dev@lists.ozlabs.org
Subject: Re: Open Firmware and interrupt trigger
On Wed, 2011-02-23 at 22:18 +0100, Robert Thorhuus wrote:
> Hello!
>=20
> I'm quite new to linux and Open Firmware.
>=20
> I have a PPC processor. To this I have a Compact Flash connected. The Com=
pact Flash is using external interrupt 0 of the processor.
> In my DTS file I have specified a Compact Flash node and within it I have=
an interrupt element:
> interrupt =3D <0 2 0 0>;
>=20
> Here I thought the first number was the ID of the interrupt and the secon=
d one should be a number indicating how the interrupt is triggered (high, l=
ow, raising, falling).
>=20
> The interrupt is active low.
>=20
> But I could not get it to work which ever value I chose.
>=20
> Looking in the code I found this in function __devinit pata_of_platform_p=
robe in file pata_of_platform.c:
>=20
> ret =3D of_irq_to_resource(dn, 0, &irq_res);
> if (ret =3D=3D NO_IRQ)
> irq_res.start =3D irq_res.end =3D 0;
> else
> irq_res.flags =3D 0;
>=20
> Here "flags" will be zero whatever I do in the DTS. As far as I can under=
stand the flags are defined in interrupts.h:
> #define IRQF_TRIGGER_NONE 0x00000000
> #define IRQF_TRIGGER_RISING 0x00000001
> #define IRQF_TRIGGER_FALLING 0x00000002
> #define IRQF_TRIGGER_HIGH 0x00000004
> #define IRQF_TRIGGER_LOW 0x00000008
Actually, the .dts flags depend on the specific interrupt controller you ar=
e using. For example, MPIC uses a different mapping scheme (for historical =
reasons). Check booting-without-of.txt.
> So modifying the code to:
> else
> irq_res.flags =3D 2;
>=20
> I get it to work.
>=20
> Could someone please explain to me why the "flags" parameter is hardcoded=
zero or just point in a good direction.
That does indeed look odd. Might be worth trying to figure out with the git=
history who came up with that code in the first place and ask that person.=
Without answer, I think it's valid to patch that out.
Cheers,
Ben.
> Thank you
>=20
> BR
> Robert
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* [PATCH 03/21]arch:powerpc:eeh.c remove one to many l's in the word.
From: Justin P. Mattock @ 2011-02-25 6:10 UTC (permalink / raw)
To: trivial; +Cc: paulus, linuxppc-dev, Justin P. Mattock, linux-kernel
The patch below removes an extra "l" in the word.
Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
---
arch/powerpc/platforms/pseries/eeh.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/eeh.c b/arch/powerpc/platforms/pseries/eeh.c
index 17a11c8..3cc4d10 100644
--- a/arch/powerpc/platforms/pseries/eeh.c
+++ b/arch/powerpc/platforms/pseries/eeh.c
@@ -876,7 +876,7 @@ void eeh_restore_bars(struct pci_dn *pdn)
*
* Save the values of the device bars. Unlike the restore
* routine, this routine is *not* recursive. This is because
- * PCI devices are added individuallly; but, for the restore,
+ * PCI devices are added individually; but, for the restore,
* an entire slot is reset at a time.
*/
static void eeh_save_bars(struct pci_dn *pdn)
--
1.7.4.1
^ permalink raw reply related
* Re: PowerPC BUG: using smp_processor_id() in preemptible code
From: Peter Zijlstra @ 2011-02-24 21:27 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: linuxppc-dev, Jeremy Fitzhardinge, Hugh Dickins, linux-kernel
In-Reply-To: <1298582606.8833.509.camel@pasglop>
On Fri, 2011-02-25 at 08:23 +1100, Benjamin Herrenschmidt wrote:
>
> I don't think that's needed here as there shall be no batching happening
> on the vmalloc space, but it can't hurt to merge it regardless :-)
Ah, due to the !batch->active thing? OK, then yeah Hugh's bit is
sufficient.
^ permalink raw reply
* Re: PowerPC BUG: using smp_processor_id() in preemptible code
From: Benjamin Herrenschmidt @ 2011-02-24 21:23 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linuxppc-dev, Jeremy Fitzhardinge, Hugh Dickins, linux-kernel
In-Reply-To: <1298581675.5226.840.camel@laptop>
On Thu, 2011-02-24 at 22:07 +0100, Peter Zijlstra wrote:
>
> Lovely problem :-), benh mentioned it on IRC, but I never got around
> to
> finding the email thread, thanks for the CC.
>
> > What would be better for 2.6.38 and 2.6.37-stable? Moving that call
> to
> > vunmap_page_range back under vb->lock, or the partial-Peter-patch
> below?
> > And then what should be done for 2.6.39?
>
> I think you'll also need the arch/powerpc/kernel/process.c changes
> that
> cause context switches to flush the tlb_batch queues.
I don't think that's needed here as there shall be no batching happening
on the vmalloc space, but it can't hurt to merge it regardless :-)
Cheers,
Ben.
^ permalink raw reply
* Re: PowerPC BUG: using smp_processor_id() in preemptible code
From: Benjamin Herrenschmidt @ 2011-02-24 21:12 UTC (permalink / raw)
To: Hugh Dickins
Cc: Jeremy Fitzhardinge, linuxppc-dev, Peter Zijlstra, linux-kernel
In-Reply-To: <alpine.LSU.2.00.1102241216420.1708@sister.anvils>
On Thu, 2011-02-24 at 12:47 -0800, Hugh Dickins wrote:
> Reading back, I see Jeremy suggested moving vb_free()'s call to
> vunmap_page_range() back inside vb->lock: it certainly was his moving
> the call out from under that lock that brought the issue to my notice;
> but it looked as if there were other paths which would give preemptible
> PowerPC the same issue, just paths I happen not to go down myself. I'm
> not sure, I didn't take the time to follow it up properly, expecting
> further insight to arrive shortly from Ben!
Yeah, sorry, I've been too over extended lately...
> And, as threatened, Jeremy has further vmalloc changes queued up in
> mmotm, which certainly make the patch below inadequate, and I imagine
> the vunmap_page_range() movement too. I'm currently (well, I think most
> recent mmotm doesn't even boot on my ppc) having to disable preemption
> in the kernel case of apply_to_pte_range().
>
> What would be better for 2.6.38 and 2.6.37-stable? Moving that call to
> vunmap_page_range back under vb->lock, or the partial-Peter-patch below?
> And then what should be done for 2.6.39?
Patch is fine. I should send it to Linus. It's not like we have a batch
on the vmalloc space anyways since it doesnt do the arch_lazy_mmu stuff,
so it's really about protecting the per-cpu variable.
Cheers,
Ben.
> --- 2.6.38-rc5/arch/powerpc/mm/tlb_hash64.c 2010-02-24 10:52:17.000000000 -0800
> +++ linux/arch/powerpc/mm/tlb_hash64.c 2011-02-15 23:27:21.000000000 -0800
> @@ -38,13 +38,11 @@ DEFINE_PER_CPU(struct ppc64_tlb_batch, p
> * neesd to be flushed. This function will either perform the flush
> * immediately or will batch it up if the current CPU has an active
> * batch on it.
> - *
> - * Must be called from within some kind of spinlock/non-preempt region...
> */
> void hpte_need_flush(struct mm_struct *mm, unsigned long addr,
> pte_t *ptep, unsigned long pte, int huge)
> {
> - struct ppc64_tlb_batch *batch = &__get_cpu_var(ppc64_tlb_batch);
> + struct ppc64_tlb_batch *batch = &get_cpu_var(ppc64_tlb_batch);
> unsigned long vsid, vaddr;
> unsigned int psize;
> int ssize;
> @@ -99,6 +97,7 @@ void hpte_need_flush(struct mm_struct *m
> */
> if (!batch->active) {
> flush_hash_page(vaddr, rpte, psize, ssize, 0);
> + put_cpu_var(ppc64_tlb_batch);
> return;
> }
>
> @@ -127,6 +126,7 @@ void hpte_need_flush(struct mm_struct *m
> batch->index = ++i;
> if (i >= PPC64_TLB_BATCH_NR)
> __flush_tlb_pending(batch);
> + put_cpu_var(ppc64_tlb_batch);
> }
>
> /*
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: PowerPC BUG: using smp_processor_id() in preemptible code
From: Peter Zijlstra @ 2011-02-24 21:07 UTC (permalink / raw)
To: Hugh Dickins; +Cc: Jeremy Fitzhardinge, linuxppc-dev, linux-kernel
In-Reply-To: <alpine.LSU.2.00.1102241216420.1708@sister.anvils>
On Thu, 2011-02-24 at 12:47 -0800, Hugh Dickins wrote:
Lovely problem :-), benh mentioned it on IRC, but I never got around to
finding the email thread, thanks for the CC.
> What would be better for 2.6.38 and 2.6.37-stable? Moving that call to
> vunmap_page_range back under vb->lock, or the partial-Peter-patch below?
> And then what should be done for 2.6.39?
I think you'll also need the arch/powerpc/kernel/process.c changes that
cause context switches to flush the tlb_batch queues.
> --- 2.6.38-rc5/arch/powerpc/mm/tlb_hash64.c 2010-02-24 10:52:17.000000000 -0800
> +++ linux/arch/powerpc/mm/tlb_hash64.c 2011-02-15 23:27:21.000000000 -0800
> @@ -38,13 +38,11 @@ DEFINE_PER_CPU(struct ppc64_tlb_batch, p
> * neesd to be flushed. This function will either perform the flush
> * immediately or will batch it up if the current CPU has an active
> * batch on it.
> - *
> - * Must be called from within some kind of spinlock/non-preempt region...
> */
> void hpte_need_flush(struct mm_struct *mm, unsigned long addr,
> pte_t *ptep, unsigned long pte, int huge)
> {
> - struct ppc64_tlb_batch *batch = &__get_cpu_var(ppc64_tlb_batch);
> + struct ppc64_tlb_batch *batch = &get_cpu_var(ppc64_tlb_batch);
> unsigned long vsid, vaddr;
> unsigned int psize;
> int ssize;
> @@ -99,6 +97,7 @@ void hpte_need_flush(struct mm_struct *m
> */
> if (!batch->active) {
> flush_hash_page(vaddr, rpte, psize, ssize, 0);
> + put_cpu_var(ppc64_tlb_batch);
> return;
> }
>
> @@ -127,6 +126,7 @@ void hpte_need_flush(struct mm_struct *m
> batch->index = ++i;
> if (i >= PPC64_TLB_BATCH_NR)
> __flush_tlb_pending(batch);
> + put_cpu_var(ppc64_tlb_batch);
> }
>
> /*
^ permalink raw reply
* Re: PowerPC BUG: using smp_processor_id() in preemptible code
From: Hugh Dickins @ 2011-02-24 20:47 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Jeremy Fitzhardinge, linuxppc-dev, Peter Zijlstra, linux-kernel
In-Reply-To: <1293705910.17779.60.camel@pasglop>
On Thu, 30 Dec 2010, Benjamin Herrenschmidt wrote:
> On Wed, 2010-12-29 at 14:54 -0800, Hugh Dickins wrote:
> > With recent 2.6.37-rc, with CONFIG_PREEMPT=y CONFIG_DEBUG_PREEMPT=y
> > on the PowerPC G5, I get spammed by BUG warnings each time I swapoff:
> >
> > BUG: using smp_processor_id() in preemptible [00000000] code: swapoff/3974
> > caller is .hpte_need_flush+0x4c/0x2e8
> > Call Trace:
> > [c0000001b4a3f830] [c00000000000f3cc] .show_stack+0x6c/0x16c (unreliable)
> > [c0000001b4a3f8e0] [c00000000023eda0] .debug_smp_processor_id+0xe4/0x11c
> > [c0000001b4a3f970] [c00000000002f2f4] .hpte_need_flush+0x4c/0x2e8
> > [c0000001b4a3fa30] [c0000000000e7ef8] .vunmap_pud_range+0x148/0x200
> > [c0000001b4a3fb10] [c0000000000e8058] .vunmap_page_range+0xa8/0xd4
> > [c0000001b4a3fbb0] [c0000000000e80a4] .free_unmap_vmap_area+0x20/0x38
> > [c0000001b4a3fc40] [c0000000000e8138] .remove_vm_area+0x7c/0xb4
> > [c0000001b4a3fcd0] [c0000000000e8308] .__vunmap+0x50/0x104
> > [c0000001b4a3fd60] [c0000000000ef3fc] .SyS_swapoff+0x59c/0x6a8
> > [c0000001b4a3fe30] [c0000000000075a8] syscall_exit+0x0/0x40
> >
> > I notice hpte_need_flush() itself acknowledges
> > * Must be called from within some kind of spinlock/non-preempt region...
>
> Yes, we assume that the PTE lock is always held when modifying page
> tables...
Right, not for the kernel page tables. I remember when doing the
pte_offset_map_lock() stuff, that it appeared that the kernel would
already be in trouble if it did not have higher serialization for
its pte updates, so no point in using a page_table_lock for them.
>
> > Though I didn't actually bisect, I believe this is since Jeremy's
> > 64141da587241301ce8638cc945f8b67853156ec "vmalloc: eagerly clear ptes
> > on vunmap", which moves a call to vunmap_page_range() from one place
> > (which happened to be inside a spinlock) to another (where it's not).
> >
> > I guess my warnings would be easily silenced by moving that call to
> > vunmap_page_range() down just inside the spinlock below it; but I'm
> > dubious that that's the right fix - it looked as if there are other
> > paths through vmalloc.c where vunmap_page_range() has been getting
> > called without preemption disabled, long before Jeremy's change,
> > just paths that I never happen to go down in my limited testing.
> >
> > For the moment I'm using the obvious patch below to keep it quiet;
> > but I doubt that this is the right patch either. I'm hoping that
> > ye who understand the importance of hpte_need_flush() will be best
> > able to judge what to do. Or might there be other architectures
> > expecting to be unpreemptible there?
>
> Well, it looks like our kernel mappings tend to take some nasty
> shortcuts with the PTE locking, which I suppose are legit but do break
> some of my assumptions there. I need to have a closer look. Thanks for
> the report !
None of us have progressed this since late in 2.6.37-rc, and now
it's late in 2.6.38-rc and we're still in the same situation.
The patch I'm currently using to suppress all the noise (I suppose
I could just turn off DEBUG_PREEMPT but that would be cheating) is an
extract from Peter's preemptible mmu_gather patches, below, but I don't
really know if it's valid to extract it in this way.
Reading back, I see Jeremy suggested moving vb_free()'s call to
vunmap_page_range() back inside vb->lock: it certainly was his moving
the call out from under that lock that brought the issue to my notice;
but it looked as if there were other paths which would give preemptible
PowerPC the same issue, just paths I happen not to go down myself. I'm
not sure, I didn't take the time to follow it up properly, expecting
further insight to arrive shortly from Ben!
And, as threatened, Jeremy has further vmalloc changes queued up in
mmotm, which certainly make the patch below inadequate, and I imagine
the vunmap_page_range() movement too. I'm currently (well, I think most
recent mmotm doesn't even boot on my ppc) having to disable preemption
in the kernel case of apply_to_pte_range().
What would be better for 2.6.38 and 2.6.37-stable? Moving that call to
vunmap_page_range back under vb->lock, or the partial-Peter-patch below?
And then what should be done for 2.6.39?
Hugh
--- 2.6.38-rc5/arch/powerpc/mm/tlb_hash64.c 2010-02-24 10:52:17.000000000 -0800
+++ linux/arch/powerpc/mm/tlb_hash64.c 2011-02-15 23:27:21.000000000 -0800
@@ -38,13 +38,11 @@ DEFINE_PER_CPU(struct ppc64_tlb_batch, p
* neesd to be flushed. This function will either perform the flush
* immediately or will batch it up if the current CPU has an active
* batch on it.
- *
- * Must be called from within some kind of spinlock/non-preempt region...
*/
void hpte_need_flush(struct mm_struct *mm, unsigned long addr,
pte_t *ptep, unsigned long pte, int huge)
{
- struct ppc64_tlb_batch *batch = &__get_cpu_var(ppc64_tlb_batch);
+ struct ppc64_tlb_batch *batch = &get_cpu_var(ppc64_tlb_batch);
unsigned long vsid, vaddr;
unsigned int psize;
int ssize;
@@ -99,6 +97,7 @@ void hpte_need_flush(struct mm_struct *m
*/
if (!batch->active) {
flush_hash_page(vaddr, rpte, psize, ssize, 0);
+ put_cpu_var(ppc64_tlb_batch);
return;
}
@@ -127,6 +126,7 @@ void hpte_need_flush(struct mm_struct *m
batch->index = ++i;
if (i >= PPC64_TLB_BATCH_NR)
__flush_tlb_pending(batch);
+ put_cpu_var(ppc64_tlb_batch);
}
/*
^ permalink raw reply
* Re: Open Firmware and interrupt trigger
From: Benjamin Herrenschmidt @ 2011-02-24 20:46 UTC (permalink / raw)
To: Robert Thorhuus; +Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <9081694521850B4D91387C8C2E612B591062A0D5CA@ESESSCMS0361.eemea.ericsson.se>
On Wed, 2011-02-23 at 22:18 +0100, Robert Thorhuus wrote:
> Hello!
>
> I'm quite new to linux and Open Firmware.
>
> I have a PPC processor. To this I have a Compact Flash connected. The Compact Flash is using external interrupt 0 of the processor.
> In my DTS file I have specified a Compact Flash node and within it I have an interrupt element:
> interrupt = <0 2 0 0>;
>
> Here I thought the first number was the ID of the interrupt and the second one should be a number indicating how the interrupt is triggered (high, low, raising, falling).
>
> The interrupt is active low.
>
> But I could not get it to work which ever value I chose.
>
> Looking in the code I found this in function __devinit pata_of_platform_probe in file pata_of_platform.c:
>
> ret = of_irq_to_resource(dn, 0, &irq_res);
> if (ret == NO_IRQ)
> irq_res.start = irq_res.end = 0;
> else
> irq_res.flags = 0;
>
> Here "flags" will be zero whatever I do in the DTS. As far as I can understand the flags are defined in interrupts.h:
> #define IRQF_TRIGGER_NONE 0x00000000
> #define IRQF_TRIGGER_RISING 0x00000001
> #define IRQF_TRIGGER_FALLING 0x00000002
> #define IRQF_TRIGGER_HIGH 0x00000004
> #define IRQF_TRIGGER_LOW 0x00000008
Actually, the .dts flags depend on the specific interrupt controller you
are using. For example, MPIC uses a different mapping scheme (for
historical reasons). Check booting-without-of.txt.
> So modifying the code to:
> else
> irq_res.flags = 2;
>
> I get it to work.
>
> Could someone please explain to me why the "flags" parameter is hardcoded zero or just point in a good direction.
That does indeed look odd. Might be worth trying to figure out with the
git history who came up with that code in the first place and ask that
person. Without answer, I think it's valid to patch that out.
Cheers,
Ben.
> Thank you
>
> BR
> Robert
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Re: mpic_alloc: Differences between of_address_to_resource() and of_get_property()+of_translate_address()
From: Benjamin Herrenschmidt @ 2011-02-24 20:43 UTC (permalink / raw)
To: Moffett, Kyle D; +Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <031BE41A-C26B-419E-A515-F88C0BF4CA5D@boeing.com>
On Thu, 2011-02-24 at 11:43 -0600, Moffett, Kyle D wrote:
> Hello everyone,
>
> I'm currently cleaning up a new P2020 (mpc85xx) board port for submission and I was noticing a lot of commonalities between the various ports.
>
> In particular, at least 80% of the mpic_alloc() callers seem to do something like this (with more error-checking):
>
> struct resource r;
> of_address_to_resource(np, 0, &r);
> mpic_alloc(np, r.start, [...]);
>
> But mpic_alloc() itself seems to have some logic for digging the base address out of OpenFirmware already:
>
> if (paddr == 0 && !(mpic->flags & MPIC_USES_DCR)) {
> const u32 *reg = of_get_property(node, "reg", NULL);
> BUG_ON(reg == NULL);
> paddr = of_translate_address(node, reg);
> BUG_ON(paddr == OF_BAD_ADDR);
> }
>
> As far as I can tell, the code in mpic_alloc() is a very limited form of of_address_to_resource() without a
> lot of the special cases, but I can't tell what the effect of those special cases would be on the boards
> relying on mpic_alloc(np, 0, [...]);
Probably none, ie, it's probably historical and could probably be
converted.
> I'd like to just convert mpic_alloc() to do the of_address_to_resource() internally (instead of the existing logic),
> but I'd be afraid of breaking other systems.
>
> Any comments/advice?
I think it can be tried :-) The risk should be reasonably low.
Cheers,
Ben.
> Cheers,
> Kyle Moffett
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Re: Flushing data cache on PPC405 in Linux
From: Dan Malek @ 2011-02-24 20:15 UTC (permalink / raw)
To: John Linn; +Cc: linuxppc-dev
In-Reply-To: <119fc163-ae81-4492-955a-94d1234ab6d9@VA3EHSMHS021.ehs.local>
On Feb 24, 2011, at 6:43 AM, John Linn wrote:
> It seems like this also depends on that fact that __GFP_COLD will
> work,
> otherwise some of the data could
> already be in the cache such that you're not guaranteed to get
> everything out of the cache.
I wouldn't count on GFP_COLD as a guarantee the data isn't
in the cache. It's likely, but the generic MM functions just
ensures they are old pages, based on an indication from the
caller that freed the pages.
-- Dan
^ permalink raw reply
* Re: rtc on PowerMac7,3
From: kevin diggs @ 2011-02-24 18:52 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Linux PPC Development
In-Reply-To: <1298506192.8833.222.camel@pasglop>
Hi,
Thanks for taking some of your valuable time to reply.
Now I can't get it to fail. I don't know what I did wrong??? These
things are tryin' to push me over the edge!
Part of the problem may be the /dev/rtc (10:135 or whatever the PC
numbers are) PC device that gets put into /dev/ (udev) on YDL 6.0.
Should probably figure out who is adding it and nuke it.
Sorry for the noise!?!
kevin
On Wed, Feb 23, 2011 at 6:09 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> Not sure, I haven't looked at that RTC stuff for ages. Basically the
> platform code provides generic RTC hooks (in that G5 it's going to be
> via the via-pmu) and it should "just work". That code hasn't been
> touched for eons.
>
> Cheers,
> Ben.
>
^ permalink raw reply
* mpic_alloc: Differences between of_address_to_resource() and of_get_property()+of_translate_address()
From: Moffett, Kyle D @ 2011-02-24 17:43 UTC (permalink / raw)
To: linuxppc-dev@lists.ozlabs.org
Hello everyone,
I'm currently cleaning up a new P2020 (mpc85xx) board port for submission a=
nd I was noticing a lot of commonalities between the various ports.
In particular, at least 80% of the mpic_alloc() callers seem to do somethin=
g like this (with more error-checking):
struct resource r;
of_address_to_resource(np, 0, &r);
mpic_alloc(np, r.start, [...]);
But mpic_alloc() itself seems to have some logic for digging the base addre=
ss out of OpenFirmware already:
if (paddr =3D=3D 0 && !(mpic->flags & MPIC_USES_DCR)) {
const u32 *reg =3D of_get_property(node, "reg", NULL);
BUG_ON(reg =3D=3D NULL);
paddr =3D of_translate_address(node, reg);
BUG_ON(paddr =3D=3D OF_BAD_ADDR);
}
As far as I can tell, the code in mpic_alloc() is a very limited form of of=
_address_to_resource() without a lot of the special cases, but I can't tell=
what the effect of those special cases would be on the boards relying on m=
pic_alloc(np, 0, [...]);
I'd like to just convert mpic_alloc() to do the of_address_to_resource() in=
ternally (instead of the existing logic), but I'd be afraid of breaking oth=
er systems.
Any comments/advice?
Cheers,
Kyle Moffett
^ permalink raw reply
* Re: [PATCH V11 2/4] ptp: Added a clock that uses the eTSEC found on the MPC85xx.
From: Scott Wood @ 2011-02-24 17:27 UTC (permalink / raw)
To: Richard Cochran
Cc: Mike Frysinger, Russell King, Arnd Bergmann, Peter Zijlstra,
linux-api, devicetree-discuss, linuxppc-dev, linux-kernel,
David Miller, Paul Mackerras, linux-arm-kernel, netdev,
John Stultz, Thomas Gleixner, Rodolfo Giometti, Christoph Lameter,
Alan Cox, Krzysztof Halasa
In-Reply-To: <20110224165004.GB15234@riccoc20.at.omicron.at>
On Thu, 24 Feb 2011 17:50:04 +0100
Richard Cochran <richardcochran@gmail.com> wrote:
> On Wed, Feb 23, 2011 at 01:24:44PM -0600, Scott Wood wrote:
> > Whatever string is used should be written into a binding document.
> >
> > fsl,etsec-v1.6-ptp seems like it would be just as good for that purpose.
> >
> > Even just fsl,etsec-ptp will identify the binding, though it's lacking in
> > identifying the hardware (in the absence of access to the eTSEC ID
> > registers).
>
> I read the conversation, and I don't mind admitting that I do not
> understand what you both are arguing/discussing about.
>
> How should I set the strings? Like this?
>
> arch/powerpc/boot/dts/mpc8313erdb.dts:
> ptp_clock@24E00 {
> compatible = "fsl,mpc8313-etsec-ptp";
> }
> arch/powerpc/boot/dts/mpc8572ds.dts:
> ptp_clock@24E00 {
> compatible = "fsl,mpc8572-etsec-ptp";
> }
> arch/powerpc/boot/dts/p2020ds.dts:
> ptp_clock@24E00 {
> compatible = "fsl,p2020ds-etsec-ptp";
> }
> arch/powerpc/boot/dts/p2020rdb.dts:
> ptp_clock@24E00 {
> compatible = "fsl,p2020rdb-etsec-ptp";
> }
>
> drivers/net/gianfar_ptp.c:
>
> static struct of_device_id match_table[] = {
> { .compatible = "fsl,mpc8313-etsec-ptp" },
> { .compatible = "fsl,mpc8572-etsec-ptp" },
> { .compatible = "fsl,p2020ds-etsec-ptp" },
> { .compatible = "fsl,p2020rdb-etsec-ptp" },
> {},
> };
Those last two are boards, not chips. I don't think even Grant is asking
to take things that far.
My vote, if it goes in a separate node at all, is "fsl,etsec-ptp", and let
the driver use SVR. Even encoding an etsec version in the compatible
string would be difficult, unless fixed up by u-boot, as it appears to
differ based on chip revision (and the chip manuals seem to often not match
the hardware regarding the advertised eTSEC revision) and we don't normally
have separate dts files for different revisions of the same chip. Plus,
our docs (at least the public ones) don't seem to be very helpful in
determining what version of eTSEC implies what.
If you want to use chip-based compatibles instead, then use the actual name
of the chip. You'll need to verify 100% compatibility if you want to claim
compatibility with another chip; it's probably easier/safer to just list
every single Freescale chip that has this type of PTP in a huge compatible
table, like PCI drivers do.
-Scott
^ permalink raw reply
* Re: [PATCH V11 2/4] ptp: Added a clock that uses the eTSEC found on the MPC85xx.
From: Richard Cochran @ 2011-02-24 17:26 UTC (permalink / raw)
To: Grant Likely
Cc: Thomas Gleixner, Rodolfo Giometti, Arnd Bergmann, Peter Zijlstra,
linux-api, devicetree-discuss, linux-kernel, Russell King,
Paul Mackerras, John Stultz, Alan Cox, netdev, Mike Frysinger,
Christoph Lameter, linuxppc-dev, David Miller, linux-arm-kernel,
Krzysztof Halasa
In-Reply-To: <20110223165058.GE14597@angua.secretlab.ca>
On Wed, Feb 23, 2011 at 09:50:58AM -0700, Grant Likely wrote:
> On Wed, Feb 23, 2011 at 11:38:17AM +0100, Richard Cochran wrote:
> > +Clock Properties:
> > +
> > + - tclk-period Timer reference clock period in nanoseconds.
> > + - tmr-prsc Prescaler, divides the output clock.
> > + - tmr-add Frequency compensation value.
> > + - cksel 0= external clock, 1= eTSEC system clock, 3= RTC clock input.
> > + Currently the driver only supports choice "1".
>
> I'd be hesitant about defining something that isn't actually
> implemented yet. You may find the binding to be insufficient at a
> later date.
Okay, I'll remove it.
We never got the external VCO working anyhow.
> > + - tmr-fiper1 Fixed interval period pulse generator.
> > + - tmr-fiper2 Fixed interval period pulse generator.
> > + - max-adj Maximum frequency adjustment in parts per billion.
>
> These are all custom properties (not part of any shared binding) so
> they should probably be prefixed with 'fsl,'.
Okay, fine.
> > + The calculation for tmr_fiper2 is the same as for tmr_fiper1. The
> > + driver expects that tmr_fiper1 will be correctly set to produce a 1
> > + Pulse Per Second (PPS) signal, since this will be offered to the PPS
> > + subsystem to synchronize the Linux clock.
>
> Good documentation, thanks. Question though, how many of these values
> will the end user (or board builder) be likely to want to change. It
> is risky encoding the calculation results into the device tree when
> they aren't the actually parameters that will be manipulated, or at
> least very user-unfriendly.
The whole thing is pretty opaque, and my explanation is (IMHO) way
better that Freescale's documentation of how the fipers work.
The board designer / system designer will want to set these carefully,
but never change them. Basically, for a given input clock, there is
only one optimal setting.
I think the device tree is the right place for that kind of setting.
The fiper1 signal should always be a 1 PPS. We could make fiper2 run
time programmable via PHC ioctls, but I think this can wait.
> > + etsects->irq = irq_of_parse_and_map(node, 0);
>
> Use platform_get_irq().
Okay.
> > + etsects->regs = of_iomap(node, 0);
>
> Use platform_get_resource(), and don't forget to request the
> resources.
Okay, but didn't you tell me before to do this way?
http://marc.info/?l=linux-netdev&m=127662247203659&w=4
> > +static struct of_platform_driver gianfar_ptp_driver = {
>
> Use a platform_driver instead. of_platform_driver is deprecated and
> being removed.
Ja, should have noticed that myself, sorry.
> > +++ b/drivers/net/gianfar_ptp_reg.h
>
> This data is only used by gianfar_ptp.c, so there is no need for a
> separate include file. Move the contents of gianfar_ptp_reg.h into
> gianfar_ptp.c
You are right, of course, since private #defines and declarations
should simply stay in their .c files. Some people think that all
#defines and declarations must go into a header file.
I am not one of those people, but in this case, I generated the file
from a little tool I wrote and so kept it separate.
Still, it is no trouble to combine the header into the driver .c file.
Thanks for your review,
Richard
^ permalink raw reply
* Re: [PATCH V11 2/4] ptp: Added a clock that uses the eTSEC found on the MPC85xx.
From: Scott Wood @ 2011-02-24 17:08 UTC (permalink / raw)
To: Richard Cochran
Cc: Mike Frysinger, Russell King, Arnd Bergmann, Peter Zijlstra,
linux-api, devicetree-discuss, linuxppc-dev, linux-kernel,
David Miller, Paul Mackerras, linux-arm-kernel, netdev,
John Stultz, Thomas Gleixner, Rodolfo Giometti, Christoph Lameter,
Alan Cox, Krzysztof Halasa
In-Reply-To: <20110224163944.GA15234@riccoc20.at.omicron.at>
On Thu, 24 Feb 2011 17:39:44 +0100
Richard Cochran <richardcochran@gmail.com> wrote:
> On Wed, Feb 23, 2011 at 10:54:59AM -0700, Grant Likely wrote:
> > On Wed, Feb 23, 2011 at 11:26:12AM -0600, Scott Wood wrote:
>
> > > The eTSEC revision is probeable as well, but due the way PTP is described as
> > > a separate node, the driver doesn't have straightforward access to those
> > > registers.
> >
> > Ignorant question: Should the ptp be described as a separate node?
>
> Well, the PTP Hardware Clock function is logically separate from the
> MAC function.
The eTSEC node doesn't describe the MAC function, it describes the whole
device (or at least it should... we make an exception for MDIO, which
should probably have been a subnode instead).
> PHCs can be implemented in the MAC, in the PHY, or in
> between in an FPGA on MII bus.
>
> If the PHC is in the MAC, then it might be wise to implement one
> driver that offers both the MAC and the PHC.
>
> In the case of gianfar, it is not really necessary to combine the PHC
> into the gianfar driver, since the registers are pretty well
> separated.
How the drivers are structured in Linux is a separate concern from how the
devices are described in the device tree. The tree is supposed to be an
OS-independent representation of hardware.
If Linux has multiple drivers that correspond to portions of one node, a
toplevel driver can register platform devices for the components, adding
in any additional information like versioning that it gets from the
toplevel registers.
-Scott
^ permalink raw reply
* Re: [PATCH V11 2/4] ptp: Added a clock that uses the eTSEC found on the MPC85xx.
From: Richard Cochran @ 2011-02-24 16:50 UTC (permalink / raw)
To: Scott Wood
Cc: Mike Frysinger, Russell King, Arnd Bergmann, Peter Zijlstra,
linux-api, devicetree-discuss, linuxppc-dev, linux-kernel,
David Miller, Paul Mackerras, linux-arm-kernel, netdev,
John Stultz, Thomas Gleixner, Rodolfo Giometti, Christoph Lameter,
Alan Cox, Krzysztof Halasa
In-Reply-To: <20110223132444.65dfdda4@schlenkerla>
On Wed, Feb 23, 2011 at 01:24:44PM -0600, Scott Wood wrote:
> Whatever string is used should be written into a binding document.
>
> fsl,etsec-v1.6-ptp seems like it would be just as good for that purpose.
>
> Even just fsl,etsec-ptp will identify the binding, though it's lacking in
> identifying the hardware (in the absence of access to the eTSEC ID
> registers).
I read the conversation, and I don't mind admitting that I do not
understand what you both are arguing/discussing about.
How should I set the strings? Like this?
arch/powerpc/boot/dts/mpc8313erdb.dts:
ptp_clock@24E00 {
compatible = "fsl,mpc8313-etsec-ptp";
}
arch/powerpc/boot/dts/mpc8572ds.dts:
ptp_clock@24E00 {
compatible = "fsl,mpc8572-etsec-ptp";
}
arch/powerpc/boot/dts/p2020ds.dts:
ptp_clock@24E00 {
compatible = "fsl,p2020ds-etsec-ptp";
}
arch/powerpc/boot/dts/p2020rdb.dts:
ptp_clock@24E00 {
compatible = "fsl,p2020rdb-etsec-ptp";
}
drivers/net/gianfar_ptp.c:
static struct of_device_id match_table[] = {
{ .compatible = "fsl,mpc8313-etsec-ptp" },
{ .compatible = "fsl,mpc8572-etsec-ptp" },
{ .compatible = "fsl,p2020ds-etsec-ptp" },
{ .compatible = "fsl,p2020rdb-etsec-ptp" },
{},
};
Please let me know if this is what you meant.
Thanks,
Richard
^ permalink raw reply
* Re: [PATCH V11 2/4] ptp: Added a clock that uses the eTSEC found on the MPC85xx.
From: Richard Cochran @ 2011-02-24 16:39 UTC (permalink / raw)
To: Grant Likely
Cc: Mike Frysinger, Russell King, Arnd Bergmann, Peter Zijlstra,
linux-api, devicetree-discuss, linuxppc-dev, linux-kernel,
David Miller, Paul Mackerras, linux-arm-kernel, netdev,
Scott Wood, John Stultz, Thomas Gleixner, Rodolfo Giometti,
Christoph Lameter, Alan Cox, Krzysztof Halasa
In-Reply-To: <20110223175459.GH14597@angua.secretlab.ca>
On Wed, Feb 23, 2011 at 10:54:59AM -0700, Grant Likely wrote:
> On Wed, Feb 23, 2011 at 11:26:12AM -0600, Scott Wood wrote:
> > The eTSEC revision is probeable as well, but due the way PTP is described as
> > a separate node, the driver doesn't have straightforward access to those
> > registers.
>
> Ignorant question: Should the ptp be described as a separate node?
Well, the PTP Hardware Clock function is logically separate from the
MAC function. PHCs can be implemented in the MAC, in the PHY, or in
between in an FPGA on MII bus.
If the PHC is in the MAC, then it might be wise to implement one
driver that offers both the MAC and the PHC.
In the case of gianfar, it is not really necessary to combine the PHC
into the gianfar driver, since the registers are pretty well
separated. Also, given the size and complexity (and churn over time)
of the gianfar driver, I decided to keep the PHC separate.
Right now, the driver correctly handles all the clock revisions in the
boards that I have (mpc8313, mpc8572, p2020ds, p2020rdb).
If checking the revision becomes important, then we can always export
a function from gianfar to provide this.
Thanks,
Richard
^ permalink raw reply
* Re: [RFC PATCH 13/15] dt/serial: Eliminate users of of_platform_{, un}register_driver
From: Arnd Bergmann @ 2011-02-24 16:34 UTC (permalink / raw)
To: devicetree-discuss; +Cc: sfr, linux-kernel, sparclinux, linuxppc-dev, davem
In-Reply-To: <20110223043442.20795.4037.stgit@localhost6.localdomain6>
On Wednesday 23 February 2011, Grant Likely wrote:
> Get rid of users of of_platform_driver in drivers/serial. The
> of_platform_{,un}register_driver functions are going away, so the
> users need to be converted to using the platform_bus_type directly.
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
of_serial changes look good.
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: [RFC PATCH 01/15] dt/powerpc: move of_bus_type infrastructure to ibmebus
From: Arnd Bergmann @ 2011-02-24 14:46 UTC (permalink / raw)
To: devicetree-discuss; +Cc: sfr, linux-kernel, sparclinux, linuxppc-dev, davem
In-Reply-To: <20110223043340.20795.84024.stgit@localhost6.localdomain6>
On Wednesday 23 February 2011, Grant Likely wrote:
> arch/powerpc/kernel/ibmebus.c is the only remaining user of the
> of_bus_type support code for initializing the bus and registering
> drivers. All others have either been switched to the vanilla platform
> bus or already have their own infrastructure.
>
> This patch moves the functionality that ibmebus is using out of
> drivers/of/{platform,device}.c and into ibmebus.c where it is actually
> used. Also renames the moved symbols from of_platform_* to
> ibmebus_bus_* to reflect the actual usage.
>
> This patch is part of moving all of the of_platform_bus_type users
> over to the platform_bus_type.
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
The ibmebus is essentially the platform bus of the IBM Power Systems (a.k.a.
pSeries a.k.a. System p), I think it would make a lot of sense to convert
the two drivers (ehca and ehea) on this bus into platform drivers as well.
The original reason for this bus was to provide a different IOMMU for them
than what is used on the PCI devices. This should now be possible in simpler
ways.
>
> +static void ibmebus_bus_device_shutdown(struct device *dev)
> +{
> + struct platform_device *of_dev = to_platform_device(dev);
> + struct of_platform_driver *drv = to_of_platform_driver(dev->driver);
> +
> + if (dev->driver && drv->shutdown)
> + drv->shutdown(of_dev);
> +}
neither of the drivers provides a shutdown function.
> +#ifdef CONFIG_PM_SLEEP
> +static int ibmebus_bus_legacy_suspend(struct device *dev, pm_message_t mesg)
> +{
> + struct platform_device *of_dev = to_platform_device(dev);
> + struct of_platform_driver *drv = to_of_platform_driver(dev->driver);
> + int ret = 0;
> +
> + if (dev->driver && drv->suspend)
> + ret = drv->suspend(of_dev, mesg);
> + return ret;
> +}
> +
> +static int ibmebus_bus_legacy_resume(struct device *dev)
> +{
> + struct platform_device *of_dev = to_platform_device(dev);
> + struct of_platform_driver *drv = to_of_platform_driver(dev->driver);
> + int ret = 0;
> +
> + if (dev->driver && drv->resume)
> + ret = drv->resume(of_dev);
> + return ret;
> +}
> +
> +static int ibmebus_bus_pm_prepare(struct device *dev)
> +{
> + struct device_driver *drv = dev->driver;
> + int ret = 0;
> +
> + if (drv && drv->pm && drv->pm->prepare)
> + ret = drv->pm->prepare(dev);
> +
> + return ret;
> +}
> +
> +static void ibmebus_bus_pm_complete(struct device *dev)
> +{
> + struct device_driver *drv = dev->driver;
> +
> + if (drv && drv->pm && drv->pm->complete)
> + drv->pm->complete(dev);
> +}
> +
> +#ifdef CONFIG_SUSPEND
> +
> +static int ibmebus_bus_pm_suspend(struct device *dev)
> +{
> + struct device_driver *drv = dev->driver;
> + int ret = 0;
> +
> + if (!drv)
> + return 0;
> +
> + if (drv->pm) {
> + if (drv->pm->suspend)
> + ret = drv->pm->suspend(dev);
> + } else {
> + ret = ibmebus_bus_legacy_suspend(dev, PMSG_SUSPEND);
> + }
> +
> + return ret;
> +}
> +
> +static int ibmebus_bus_pm_suspend_noirq(struct device *dev)
> +{
> + struct device_driver *drv = dev->driver;
> + int ret = 0;
> +
> + if (!drv)
> + return 0;
> +
> + if (drv->pm) {
> + if (drv->pm->suspend_noirq)
> + ret = drv->pm->suspend_noirq(dev);
> + }
> +
> + return ret;
> +}
> +
> +static int ibmebus_bus_pm_resume(struct device *dev)
> +{
> + struct device_driver *drv = dev->driver;
> + int ret = 0;
> +
> + if (!drv)
> + return 0;
> +
> + if (drv->pm) {
> + if (drv->pm->resume)
> + ret = drv->pm->resume(dev);
> + } else {
> + ret = ibmebus_bus_legacy_resume(dev);
> + }
> +
> + return ret;
> +}
> +
> +static int ibmebus_bus_pm_resume_noirq(struct device *dev)
> +{
> + struct device_driver *drv = dev->driver;
> + int ret = 0;
> +
> + if (!drv)
> + return 0;
> +
> + if (drv->pm) {
> + if (drv->pm->resume_noirq)
> + ret = drv->pm->resume_noirq(dev);
> + }
> +
> + return ret;
> +}
These are also unused in the drivers.
> +#ifdef CONFIG_HIBERNATION
> +
> +static int ibmebus_bus_pm_freeze(struct device *dev)
> +{
> + struct device_driver *drv = dev->driver;
> + int ret = 0;
> +
> + if (!drv)
> + return 0;
> +
> + if (drv->pm) {
> + if (drv->pm->freeze)
> + ret = drv->pm->freeze(dev);
> + } else {
> + ret = ibmebus_bus_legacy_suspend(dev, PMSG_FREEZE);
> + }
> +
> + return ret;
> +}
> +
> +static int ibmebus_bus_pm_freeze_noirq(struct device *dev)
> +{
> + struct device_driver *drv = dev->driver;
> + int ret = 0;
> +
> + if (!drv)
> + return 0;
> +
> + if (drv->pm) {
> + if (drv->pm->freeze_noirq)
> + ret = drv->pm->freeze_noirq(dev);
> + }
> +
> + return ret;
> +}
> +
> +static int ibmebus_bus_pm_thaw(struct device *dev)
> +{
> + struct device_driver *drv = dev->driver;
> + int ret = 0;
> +
> + if (!drv)
> + return 0;
> +
> + if (drv->pm) {
> + if (drv->pm->thaw)
> + ret = drv->pm->thaw(dev);
> + } else {
> + ret = ibmebus_bus_legacy_resume(dev);
> + }
> +
> + return ret;
> +}
> +
> +static int ibmebus_bus_pm_thaw_noirq(struct device *dev)
> +{
> + struct device_driver *drv = dev->driver;
> + int ret = 0;
> +
> + if (!drv)
> + return 0;
> +
> + if (drv->pm) {
> + if (drv->pm->thaw_noirq)
> + ret = drv->pm->thaw_noirq(dev);
> + }
> +
> + return ret;
> +}
> +
> +static int ibmebus_bus_pm_poweroff(struct device *dev)
> +{
> + struct device_driver *drv = dev->driver;
> + int ret = 0;
> +
> + if (!drv)
> + return 0;
> +
> + if (drv->pm) {
> + if (drv->pm->poweroff)
> + ret = drv->pm->poweroff(dev);
> + } else {
> + ret = ibmebus_bus_legacy_suspend(dev, PMSG_HIBERNATE);
> + }
> +
> + return ret;
> +}
> +
> +static int ibmebus_bus_pm_poweroff_noirq(struct device *dev)
> +{
> + struct device_driver *drv = dev->driver;
> + int ret = 0;
> +
> + if (!drv)
> + return 0;
> +
> + if (drv->pm) {
> + if (drv->pm->poweroff_noirq)
> + ret = drv->pm->poweroff_noirq(dev);
> + }
> +
> + return ret;
> +}
> +
> +static int ibmebus_bus_pm_restore(struct device *dev)
> +{
> + struct device_driver *drv = dev->driver;
> + int ret = 0;
> +
> + if (!drv)
> + return 0;
> +
> + if (drv->pm) {
> + if (drv->pm->restore)
> + ret = drv->pm->restore(dev);
> + } else {
> + ret = ibmebus_bus_legacy_resume(dev);
> + }
> +
> + return ret;
> +}
> +
> +static int ibmebus_bus_pm_restore_noirq(struct device *dev)
> +{
> + struct device_driver *drv = dev->driver;
> + int ret = 0;
> +
> + if (!drv)
> + return 0;
> +
> + if (drv->pm) {
> + if (drv->pm->restore_noirq)
> + ret = drv->pm->restore_noirq(dev);
> + }
> +
> + return ret;
> +}
And these, too.
Arnd
^ 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