| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1321881005.2153.14.camel@sokoban>
Date: Mon, 21 Nov 2011 15:10:05 +0200
From: Tero Kristo <t-kristo@...com>
To: <balbi@...com>
CC: Kevin Hilman <khilman@...com>, <linux-omap@...r.kernel.org>,
Paul Walmsley <paul@...an.com>,
"Cousson, Benoit" <b-cousson@...com>,
Tony Lindgren <tony@...mide.com>,
<linux-kernel@...r.kernel.org>,
"Govindraj.R" <govindraj.raja@...com>,
"Avinash.H.M" <avinashhm@...com>,
<linux-arm-kernel@...ts.infradead.org>,
Samuel Ortiz <sameo@...ux.intel.com>
Subject: Re: [PATCHv9 06/18] mfd: omap-prm: added chain interrupt handler
On Fri, 2011-11-18 at 21:18 +0200, Felipe Balbi wrote:
> Hi,
>
> On Thu, Nov 17, 2011 at 02:34:46PM -0800, Kevin Hilman wrote:
> > Tero Kristo <t-kristo@...com> writes:
> >
> > > Introduce a chained interrupt handler mechanism for the PRCM
> > > interrupt, so that individual PRCM event can cleanly be handled by
> > > handlers in separate drivers. We do this by introducing PRCM event
> > > names, which are then matched to the particular PRCM interrupt bit
> > > depending on the specific OMAP SoC being used.
> > >
> > > PRCM interrupts have two priority levels, high or normal. High priority
> > > is needed for IO event handling, so that we can be sure that IO events
> > > are processed before other events. This reduces latency for IO event
> > > customers and also prevents incorrect ack sequence on OMAP3.
> > >
> > > Signed-off-by: Tero Kristo <t-kristo@...com>
> > > Cc: Paul Walmsley <paul@...an.com>
> > > Cc: Kevin Hilman <khilman@...com>
> > > Cc: Avinash.H.M <avinashhm@...com>
> > > Cc: Cousson, Benoit <b-cousson@...com>
> > > Cc: Tony Lindgren <tony@...mide.com>
> > > Cc: Govindraj.R <govindraj.raja@...com>
> > > Cc: Samuel Ortiz <sameo@...ux.intel.com>
> > > ---
> > > drivers/mfd/omap-prm-common.c | 239 +++++++++++++++++++++++++++++++++++++++++
> > > drivers/mfd/omap-prm.h | 40 +++++++
> > > drivers/mfd/omap3xxx-prm.c | 29 +++++-
> > > drivers/mfd/omap4xxx-prm.c | 28 +++++-
> > > 4 files changed, 334 insertions(+), 2 deletions(-)
> > > create mode 100644 drivers/mfd/omap-prm.h
> > >
> > > diff --git a/drivers/mfd/omap-prm-common.c b/drivers/mfd/omap-prm-common.c
> > > index 39b199c8..2886eb2 100644
> > > --- a/drivers/mfd/omap-prm-common.c
> > > +++ b/drivers/mfd/omap-prm-common.c
> > > @@ -15,10 +15,249 @@
> > > #include <linux/ctype.h>
> > > #include <linux/module.h>
> > > #include <linux/io.h>
> > > +#include <linux/irq.h>
> > > +#include <linux/interrupt.h>
> > > #include <linux/slab.h>
> > > #include <linux/init.h>
> > > #include <linux/err.h>
> > >
> > > +#include "omap-prm.h"
> > > +
> > > +#define OMAP_PRCM_MAX_NR_PENDING_REG 2
> > > +
> > > +struct omap_prm_device {
> > > + const struct omap_prcm_irq_setup *irq_setup;
> > > + const struct omap_prcm_irq *irqs;
> > > + struct irq_chip_generic **irq_chips;
> > > + int nr_irqs;
> > > + u32 *saved_mask;
> > > + u32 *priority_mask;
> > > + int base_irq;
> > > + int irq;
> > > + void __iomem *base;
> > > +};
> > > +
> > > +static struct omap_prm_device prm_dev;
> >
> > This shouldn't be statically allocated, and needlessly forces us to
> > assume a single, global PRM (which is the case today, but who knows...)
> >
> > Instead, it should be allocated at init time and associated with the
> > instance (using set_drvdata or somthing.)
> >
> > > +static inline u32 prm_read_reg(int offset)
> > > +{
> > > + return __raw_readl(prm_dev.base + offset);
> > > +}
> > > +
> > > +static inline void prm_write_reg(u32 value, int offset)
> > > +{
> > > + __raw_writel(value, prm_dev.base + offset);
> > > +}
> >
> > This doesn't seem right either.
> >
> > The register layout/access parts are what are are different between the
> > OMAP3 and OMAP4 versions, so I would expect anything that accesses
> > registers to be going through the SoC specific code.
> >
> > I'm having some second thoughts about the split of common and SoC
> > specific code here. Currently the SoC specific code is basically
> > identical (ignoring the s/omap3/omap4/ throughout.)
> >
> > I think we need to discuss this further, but what seems to me that the
> > current design is to have 2 separate drivers, with some common helper
> > functions. I'm starting to think that what we need instead is a single,
> > common driver with a set of SoC-specific functions that implement the
> > SoC-specific details. This latter approach follows what is done in the
> > powerdomain code today for example: common code in powerdomain.c and SoC
> > specific implementation of all the "ops" in powerdomain2xxx_3xxx.c and
> > powerdomain4xxx.c.
>
> Is it so that only register layout is different ? In that case isn't it
> better to use driver_data field of the id_table structure to pass
> different register offsets based on the e.g. driver name ? Something
> like below:
>
> static const struct platform_device_id omap_prm_id_table[] __devinitconst = {
> {
> .name = "omap3-prm",
> .driver_data = (kernel_ulong_t) &omap3_prm_data,
> },
> {
> .name = "omap4-prm",
> .driver_data = (kernel_ulong_t) &omap4_prm_data,
> },
> };
> MODULE_DEVICE_TABLE(platform, omap_prm_id_table);
>
> struct platform_driver omap_prm_driver = {
> .probe = omap_prm_probe,
> .remove = omap_prm_remove,
> .id_table = omap_prm_id_table,
> };
>
> then on probe you get your id, copy id->driver_data to your own
> structure and use that to access your registers. Works for you ?
>
Functionality is rather different for some of the features also, e.g.
latency calculation for different sleep modes (if we want to move this
here at some point.) PRCM interrupt handling is really similar for both
OMAP3 and OMAP4, thus there are currently almost no differences between
these two, because the driver only supports the interrupt handling as of
now. But yes, I agree with Kevin that this should probably be a common
driver for both, and having some device specific extensions added to
this once needed. I have a few questions though:
- How should the omap revision be passed to the driver? Somewhere in the
platform data? This has been grieving me a lot with this driver, as it
doesn't seem too easy to reach a solution that would be accepted by
everyone.
- How about device tree related stuff? Should the driver just use this
and not support platform data at all?
-Tero
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists