linux-kernel - Re: [PATCHv9 06/18] mfd: omap-prm: added chain interrupt handler

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20111118191849.GB11736@legolas.emea.dhcp.ti.com>
Date:	Fri, 18 Nov 2011 21:18:50 +0200
From:	Felipe Balbi <balbi@...com>
To:	Kevin Hilman <khilman@...com>
Cc:	Tero Kristo <t-kristo@...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

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 ?

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)