linux-kernel - Re: [RFC PATCH v3 1/3] regmap-irq: Extend sub-irq to support non-fixed reg strides

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: <d78cefad64d528e7c894c153950e4b4d2a18b300.camel@fi.rohmeurope.com>
Date:   Mon, 12 Apr 2021 11:08:49 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     "Markus.Elfring@....de" <Markus.Elfring@....de>,
        "gurus@...eaurora.org" <gurus@...eaurora.org>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "lee.jones@...aro.org" <lee.jones@...aro.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>
CC:     "linux@...ck-us.net" <linux@...ck-us.net>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "subbaram@...eaurora.org" <subbaram@...eaurora.org>,
        "collinsd@...eaurora.org" <collinsd@...eaurora.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "joe@...ches.com" <joe@...ches.com>,
        "bjorn.andersson@...aro.org" <bjorn.andersson@...aro.org>,
        "aghayal@...eaurora.org" <aghayal@...eaurora.org>,
        "linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>
Subject: Re: [RFC PATCH v3 1/3] regmap-irq: Extend sub-irq to support
 non-fixed reg strides

Hi,

On Mon, 2021-04-12 at 09:05 +0300, Matti Vaittinen wrote:
> Hi All,
> 
> On Wed, 2021-03-10 at 16:39 -0800, Guru Das Srinagesh wrote:
> > Qualcomm's MFD chips have a top level interrupt status register and
> > sub-irqs (peripherals).  When a bit in the main status register
> > goes
> > high, it means that the peripheral corresponding to that bit has an
> > unserviced interrupt. If the bit is not set, this means that the
> > corresponding peripheral does not.
> > 
> > Commit a2d21848d9211d ("regmap: regmap-irq: Add main status
> > register
> > support") introduced the sub-irq logic that is currently applied
> > only
> > when reading status registers, but not for any other functions like
> > acking
> > or masking. Extend the use of sub-irq to all other functions, with
> > two
> > caveats regarding the specification of offsets:
> > 
> > - Each member of the sub_reg_offsets array should be of length 1
> > - The specified offsets should be the unequal strides for each sub-
> > irq
> >   device.
> > 
> > In QCOM's case, all the *_base registers are to be configured to
> > the
> > base addresses of the first sub-irq group, with offsets of each
> > subsequent group calculated as a difference from these addresses.
> > 
> > Continuing from the example mentioned in the cover letter:
> > 
> > 	/*
> > 	 * Address of MISC_INT_MASK		= 0x1011
> > 	 * Address of TEMP_ALARM_INT_MASK	= 0x2011
> > 	 * Address of GPIO01_INT_MASK		= 0x3011
> > 	 *
> > 	 * Calculate offsets as:
> > 	 * offset_0 = 0x1011 - 0x1011 = 0       (to access MISC's
> > 	 * 					 registers)
> > 	 * offset_1 = 0x2011 - 0x1011 = 0x1000
> > 	 * offset_2 = 0x3011 - 0x1011 = 0x2000
> > 	 */
> > 
> > 	static unsigned int sub_unit0_offsets[] = {0};
> > 	static unsigned int sub_unit1_offsets[] = {0x1000};
> > 	static unsigned int sub_unit2_offsets[] = {0x2000};
> > 
> > 	static struct regmap_irq_sub_irq_map chip_sub_irq_offsets[] = {
> > 		REGMAP_IRQ_MAIN_REG_OFFSET(sub_unit0_offsets),
> > 		REGMAP_IRQ_MAIN_REG_OFFSET(sub_unit0_offsets),
> > 		REGMAP_IRQ_MAIN_REG_OFFSET(sub_unit0_offsets),
> > 	};
> > 
> > 	static struct regmap_irq_chip chip_irq_chip = {
> > 	--------8<--------
> > 	.not_fixed_stride = true,
> > 	.mask_base	  = MISC_INT_MASK,
> > 	.type_base	  = MISC_INT_TYPE,
> > 	.ack_base	  = MISC_INT_ACK,
> > 	.sub_reg_offsets  = chip_sub_irq_offsets,
> > 	--------8<--------
> > 	};
> > 
> > Signed-off-by: Guru Das Srinagesh <gurus@...eaurora.org>
> > ---
> >  drivers/base/regmap/regmap-irq.c | 81 ++++++++++++++++++++++++++--
> > ------------
> >  include/linux/regmap.h           |  7 ++++
> >  2 files changed, 60 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/base/regmap/regmap-irq.c
> > b/drivers/base/regmap/regmap-irq.c
> > index 19db764..e1d8fc9e 100644
> > --- a/drivers/base/regmap/regmap-irq.c
> > +++ b/drivers/base/regmap/regmap-irq.c
> > @@ -45,6 +45,27 @@ struct regmap_irq_chip_data {
> >  	bool clear_status:1;
> >  };
> > 
> 
> Sorry that I am late with the "review" but I only now noticed this
> change when I was following the references from PM8008 PMIC patch
> mail.
> 
> 
> >  
> > +static int sub_irq_reg(struct regmap_irq_chip_data *data,
> > +		       unsigned int base_reg, int i)
> 
> Do I read this correctly - this function should map the main status
> bit
> (given in i) to the (sub)IRQ register, right? How does this work for
> cases where one bit corresponds to more than one sub-register? Or do
> I
> misunderstand the purpose of this function? (This is the case with
> both
> the BD70528 and BD71828).

I did some quick test with BD71815 which I had at home. And it seems to
be I did indeed misunderstand this :) This is not for converting the
main-IRQ bits to sub-irq registers - this is for getting the sub-IRQ
register address based on the 'sub IRQ register index'.

So I do apologize the noise, it seems all is well and everything
(except my self confidence) keeps working as it did :)

Thanks for the improvement Guru Das!

Best Regards
	Matti Vaittinen