* Re: [alsa-devel] [RFC 09/14] SoundWire: Add support to handle Slave status change
From: Pierre-Louis Bossart @ 2016-11-14 17:38 UTC (permalink / raw)
To: Charles Keepax, Hardik Shah
Cc: alsa-devel, tiwai, plai, lgirdwood, linux-kernel, patches.audio,
broonie, Sanyog Kale
In-Reply-To: <20161114160805.GO1575@localhost.localdomain>
On 11/14/16 10:08 AM, Charles Keepax wrote:
> There are some issues with this, as the slave driver only probes
> when the device actually shows up on the bus. However often
> (especially in embedded contexts) some things may need to be
> done to enable the slave. For example it may be held in reset or
> its power supplies switched off until they are need. As such it
> generally helps if the device probe can be called before it shows
> up on the bus, the device probe can then do the necessary actions
> to enable the device at which point it will show up on the bus.
Yes, this point was made at the LPC miniconference. What's not clear to
me is if you would want the codec driver to be notified that the bus is
operational and let it handle things like sideband power management for
that device, or is someone else needs to know.
^ permalink raw reply
* Re: [PATCH] mfd: bcm590xx: Simplify a test
From: Lee Jones @ 2016-11-14 17:40 UTC (permalink / raw)
To: Christophe JAILLET; +Cc: linux-kernel, kernel-janitors
In-Reply-To: <20161101064905.11961-1-christophe.jaillet@wanadoo.fr>
On Tue, 01 Nov 2016, Christophe JAILLET wrote:
> 'i2c_new_dummy()' does not return an error pointer, so the test can be
> simplified to be more consistent.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> drivers/mfd/bcm590xx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied, thanks.
> diff --git a/drivers/mfd/bcm590xx.c b/drivers/mfd/bcm590xx.c
> index 0d76d690176b..c572a35a9341 100644
> --- a/drivers/mfd/bcm590xx.c
> +++ b/drivers/mfd/bcm590xx.c
> @@ -67,7 +67,7 @@ static int bcm590xx_i2c_probe(struct i2c_client *i2c_pri,
> /* Secondary I2C slave address is the base address with A(2) asserted */
> bcm590xx->i2c_sec = i2c_new_dummy(i2c_pri->adapter,
> i2c_pri->addr | BIT(2));
> - if (IS_ERR_OR_NULL(bcm590xx->i2c_sec)) {
> + if (!bcm590xx->i2c_sec) {
> dev_err(&i2c_pri->dev, "failed to add secondary I2C device\n");
> return -ENODEV;
> }
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v5 3/4] usb: musb: Add a new argument to musb_platform_set_mode()
From: Alexandre Bailon @ 2016-11-14 17:37 UTC (permalink / raw)
To: Bin Liu, david, balbi, kishon, khilman, linux-kernel, linux-usb,
nsekhar
In-Reply-To: <20161114173617.GA4512@uda0271908>
On 11/14/2016 06:36 PM, Bin Liu wrote:
> Hi,
>
> On Mon, Nov 07, 2016 at 02:05:07PM +0100, Alexandre Bailon wrote:
>> During the init, the driver will use musb_platform_set_mode()
>> to configure the controller mode and the PHY mode.
>> The PHY of DA8xx has some issues when the PHY is forced in host or device,
>> so we want to keep it in OTG mode unless the user request a specific mode
>> by using the sysfs.
>> Add the init argument to musb_platform_set_mode() in order to let the
>> callback change its behavior if it is called during the init.
>
> Tony's patch set which fixes musb pm regression (but has not been merged
> yet) introduces musb->is_initialized. You might want to use this new
> variable in da8xx_musb_set_mode(), instead of adding this init flag.
OK. I will take a look and update the series.
>
> Regards,
> -Bin.
>
>>
>> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
>> ---
>> drivers/usb/musb/am35x.c | 2 +-
>> drivers/usb/musb/blackfin.c | 2 +-
>> drivers/usb/musb/da8xx.c | 2 +-
>> drivers/usb/musb/davinci.c | 2 +-
>> drivers/usb/musb/musb_core.c | 12 ++++++------
>> drivers/usb/musb/musb_core.h | 6 +++---
>> drivers/usb/musb/musb_dsps.c | 2 +-
>> drivers/usb/musb/sunxi.c | 2 +-
>> drivers/usb/musb/tusb6010.c | 2 +-
>> 9 files changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/usb/musb/am35x.c b/drivers/usb/musb/am35x.c
>> index 50ca805..7136888 100644
>> --- a/drivers/usb/musb/am35x.c
>> +++ b/drivers/usb/musb/am35x.c
>> @@ -332,7 +332,7 @@ static irqreturn_t am35x_musb_interrupt(int irq, void *hci)
>> return ret;
>> }
>>
>> -static int am35x_musb_set_mode(struct musb *musb, u8 musb_mode)
>> +static int am35x_musb_set_mode(struct musb *musb, u8 musb_mode, bool init)
>> {
>> struct device *dev = musb->controller;
>> struct musb_hdrc_platform_data *plat = dev_get_platdata(dev);
>> diff --git a/drivers/usb/musb/blackfin.c b/drivers/usb/musb/blackfin.c
>> index 310238c..544e98f 100644
>> --- a/drivers/usb/musb/blackfin.c
>> +++ b/drivers/usb/musb/blackfin.c
>> @@ -356,7 +356,7 @@ static int bfin_musb_vbus_status(struct musb *musb)
>> return 0;
>> }
>>
>> -static int bfin_musb_set_mode(struct musb *musb, u8 musb_mode)
>> +static int bfin_musb_set_mode(struct musb *musb, u8 musb_mode, bool init)
>> {
>> return -EIO;
>> }
>> diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
>> index 6749aa1..ac0c2f7 100644
>> --- a/drivers/usb/musb/da8xx.c
>> +++ b/drivers/usb/musb/da8xx.c
>> @@ -335,7 +335,7 @@ static irqreturn_t da8xx_musb_interrupt(int irq, void *hci)
>> return ret;
>> }
>>
>> -static int da8xx_musb_set_mode(struct musb *musb, u8 musb_mode)
>> +static int da8xx_musb_set_mode(struct musb *musb, u8 musb_mode, bool init)
>> {
>> struct da8xx_glue *glue = dev_get_drvdata(musb->controller->parent);
>> enum phy_mode phy_mode;
>> diff --git a/drivers/usb/musb/davinci.c b/drivers/usb/musb/davinci.c
>> index cee61a5..d12b902 100644
>> --- a/drivers/usb/musb/davinci.c
>> +++ b/drivers/usb/musb/davinci.c
>> @@ -369,7 +369,7 @@ static irqreturn_t davinci_musb_interrupt(int irq, void *__hci)
>> return retval;
>> }
>>
>> -static int davinci_musb_set_mode(struct musb *musb, u8 mode)
>> +static int davinci_musb_set_mode(struct musb *musb, u8 mode, bool init)
>> {
>> /* EVM can't do this (right?) */
>> return -EIO;
>> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
>> index 27dadc0..4a8d394 100644
>> --- a/drivers/usb/musb/musb_core.c
>> +++ b/drivers/usb/musb/musb_core.c
>> @@ -1730,11 +1730,11 @@ musb_mode_store(struct device *dev, struct device_attribute *attr,
>>
>> spin_lock_irqsave(&musb->lock, flags);
>> if (sysfs_streq(buf, "host"))
>> - status = musb_platform_set_mode(musb, MUSB_HOST);
>> + status = musb_platform_set_mode(musb, MUSB_HOST, false);
>> else if (sysfs_streq(buf, "peripheral"))
>> - status = musb_platform_set_mode(musb, MUSB_PERIPHERAL);
>> + status = musb_platform_set_mode(musb, MUSB_PERIPHERAL, false);
>> else if (sysfs_streq(buf, "otg"))
>> - status = musb_platform_set_mode(musb, MUSB_OTG);
>> + status = musb_platform_set_mode(musb, MUSB_OTG, false);
>> else
>> status = -EINVAL;
>> spin_unlock_irqrestore(&musb->lock, flags);
>> @@ -2261,13 +2261,13 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
>> status = musb_host_setup(musb, plat->power);
>> if (status < 0)
>> goto fail3;
>> - status = musb_platform_set_mode(musb, MUSB_HOST);
>> + status = musb_platform_set_mode(musb, MUSB_HOST, true);
>> break;
>> case MUSB_PORT_MODE_GADGET:
>> status = musb_gadget_setup(musb);
>> if (status < 0)
>> goto fail3;
>> - status = musb_platform_set_mode(musb, MUSB_PERIPHERAL);
>> + status = musb_platform_set_mode(musb, MUSB_PERIPHERAL, true);
>> break;
>> case MUSB_PORT_MODE_DUAL_ROLE:
>> status = musb_host_setup(musb, plat->power);
>> @@ -2278,7 +2278,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
>> musb_host_cleanup(musb);
>> goto fail3;
>> }
>> - status = musb_platform_set_mode(musb, MUSB_OTG);
>> + status = musb_platform_set_mode(musb, MUSB_OTG, true);
>> break;
>> default:
>> dev_err(dev, "unsupported port mode %d\n", musb->port_mode);
>> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
>> index 2cb88a49..1bfc8fa 100644
>> --- a/drivers/usb/musb/musb_core.h
>> +++ b/drivers/usb/musb/musb_core.h
>> @@ -203,7 +203,7 @@ struct musb_platform_ops {
>> struct dma_controller *
>> (*dma_init) (struct musb *musb, void __iomem *base);
>> void (*dma_exit)(struct dma_controller *c);
>> - int (*set_mode)(struct musb *musb, u8 mode);
>> + int (*set_mode)(struct musb *musb, u8 mode, bool init);
>> void (*try_idle)(struct musb *musb, unsigned long timeout);
>> int (*recover)(struct musb *musb);
>>
>> @@ -558,12 +558,12 @@ static inline void musb_platform_disable(struct musb *musb)
>> musb->ops->disable(musb);
>> }
>>
>> -static inline int musb_platform_set_mode(struct musb *musb, u8 mode)
>> +static inline int musb_platform_set_mode(struct musb *musb, u8 mode, bool init)
>> {
>> if (!musb->ops->set_mode)
>> return 0;
>>
>> - return musb->ops->set_mode(musb, mode);
>> + return musb->ops->set_mode(musb, mode, init);
>> }
>>
>> static inline void musb_platform_try_idle(struct musb *musb,
>> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
>> index 0f17d21..a889679 100644
>> --- a/drivers/usb/musb/musb_dsps.c
>> +++ b/drivers/usb/musb/musb_dsps.c
>> @@ -462,7 +462,7 @@ static int dsps_musb_exit(struct musb *musb)
>> return 0;
>> }
>>
>> -static int dsps_musb_set_mode(struct musb *musb, u8 mode)
>> +static int dsps_musb_set_mode(struct musb *musb, u8 mode, bool init)
>> {
>> struct device *dev = musb->controller;
>> struct dsps_glue *glue = dev_get_drvdata(dev->parent);
>> diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
>> index 1408245..04bf763 100644
>> --- a/drivers/usb/musb/sunxi.c
>> +++ b/drivers/usb/musb/sunxi.c
>> @@ -346,7 +346,7 @@ static void sunxi_musb_dma_controller_destroy(struct dma_controller *c)
>> {
>> }
>>
>> -static int sunxi_musb_set_mode(struct musb *musb, u8 mode)
>> +static int sunxi_musb_set_mode(struct musb *musb, u8 mode, bool init)
>> {
>> struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);
>> enum phy_mode new_mode;
>> diff --git a/drivers/usb/musb/tusb6010.c b/drivers/usb/musb/tusb6010.c
>> index df7c9f4..8a74587 100644
>> --- a/drivers/usb/musb/tusb6010.c
>> +++ b/drivers/usb/musb/tusb6010.c
>> @@ -628,7 +628,7 @@ static void tusb_musb_set_vbus(struct musb *musb, int is_on)
>> * Note that if a mini-A cable is plugged in the ID line will stay down as
>> * the weak ID pull-up is not able to pull the ID up.
>> */
>> -static int tusb_musb_set_mode(struct musb *musb, u8 musb_mode)
>> +static int tusb_musb_set_mode(struct musb *musb, u8 musb_mode, bool init)
>> {
>> void __iomem *tbase = musb->ctrl_base;
>> u32 otg_stat, phy_otg_ctrl, phy_otg_ena, dev_conf;
>> --
>> 2.7.3
>>
^ permalink raw reply
* Re: [PATCH tip/core/rcu 6/7] rcu: Make expedited grace periods recheck dyntick idle state
From: Peter Zijlstra @ 2016-11-14 17:37 UTC (permalink / raw)
To: Josh Triplett
Cc: Paul E. McKenney, linux-kernel, mingo, jiangshanlai, dipankar,
akpm, mathieu.desnoyers, tglx, rostedt, dhowells, edumazet,
dvhart, fweisbec, oleg, bobby.prani
In-Reply-To: <20161114172512.bcwdy66elesds5t4@jtriplet-mobl2.jf.intel.com>
On Mon, Nov 14, 2016 at 09:25:12AM -0800, Josh Triplett wrote:
> On Mon, Nov 14, 2016 at 08:57:12AM -0800, Paul E. McKenney wrote:
> > Expedited grace periods check dyntick-idle state, and avoid sending
> > IPIs to idle CPUs, including those running guest OSes, and, on NOHZ_FULL
> > kernels, nohz_full CPUs. However, the kernel has been observed checking
> > a CPU while it was non-idle, but sending the IPI after it has gone
> > idle. This commit therefore rechecks idle state immediately before
> > sending the IPI, refraining from IPIing CPUs that have since gone idle.
> >
> > Reported-by: Rik van Riel <riel@redhat.com>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>
> atomic_add_return(0, ...) seems odd. Do you actually want that, rather
> than atomic_read(...)? If so, can you please document exactly why?
Yes that is weird. The only effective difference is that it would do a
load-exclusive instead of a regular load.
^ permalink raw reply
* Re: [PATCHSET] Add support for simplified async direct-io
From: Christoph Hellwig @ 2016-11-14 17:37 UTC (permalink / raw)
To: Jens Axboe; +Cc: axboe, linux-block, hch
In-Reply-To: <1479144519-15738-1-git-send-email-axboe@fb.com>
On Mon, Nov 14, 2016 at 10:28:37AM -0700, Jens Axboe wrote:
> This is on top of for-4.10/dio, which has Christophs simplified sync
> O_DIRECT support and the IO poll bits.
>
> The restriction on 4 inline vecs is removed on the sync support, we
> just allocate the bio_vec array if we have to. I realize this negates
> parts of the win of the patch for sync, but it's still a lot leaner
> than the old code. And it means we use the same path for any type
> of sync O_DIRECT, instead of potentially bouncing between the two.
>
> Second patch is adding async support for the code base. Lightly
> tested. Performance results, from fio:
І'd much rather add a separate path pased on the same scheme. And
because I prefer that so much I actually did it a while ago, although
it will need some minor updates for the current tree:
http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/new-dio
I'll see if I can get it quickly updated..
^ permalink raw reply
* Re: [PATCH] mfd: sun4i-gpadc: select regmap-irq
From: Lee Jones @ 2016-11-14 17:40 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Quentin Schulz, linux-kernel
In-Reply-To: <20161031153246.3561667-1-arnd@arndb.de>
On Mon, 31 Oct 2016, Arnd Bergmann wrote:
> The new sun4i mfd driver is lacking a dependency, triggering very rarely
> int randconfig kernel builds:
>
> drivers/mfd/sun4i-gpadc.o: In function `sun4i_gpadc_probe':
> sun4i-gpadc.c:(.text.sun4i_gpadc_probe+0x110): undefined reference to `devm_regmap_add_irq_chip'
>
> This adds a 'select REGMAP_IRQ', as the other drivers with this problem do.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/mfd/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
Applied, thanks.
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 30978295a1e8..3a4c43df9404 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -44,6 +44,7 @@ config MFD_SUN4I_GPADC
> tristate "Allwinner sunxi platforms' GPADC MFD driver"
> select MFD_CORE
> select REGMAP_MMIO
> + select REGMAP_IRQ
> depends on ARCH_SUNXI || COMPILE_TEST
> help
> Select this to get support for Allwinner SoCs (A10, A13 and A31) ADC.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v5 3/4] usb: musb: Add a new argument to musb_platform_set_mode()
From: Bin Liu @ 2016-11-14 17:36 UTC (permalink / raw)
To: Alexandre Bailon
Cc: david, balbi, kishon, khilman, linux-kernel, linux-usb, nsekhar
In-Reply-To: <1478523908-4383-4-git-send-email-abailon@baylibre.com>
Hi,
On Mon, Nov 07, 2016 at 02:05:07PM +0100, Alexandre Bailon wrote:
> During the init, the driver will use musb_platform_set_mode()
> to configure the controller mode and the PHY mode.
> The PHY of DA8xx has some issues when the PHY is forced in host or device,
> so we want to keep it in OTG mode unless the user request a specific mode
> by using the sysfs.
> Add the init argument to musb_platform_set_mode() in order to let the
> callback change its behavior if it is called during the init.
Tony's patch set which fixes musb pm regression (but has not been merged
yet) introduces musb->is_initialized. You might want to use this new
variable in da8xx_musb_set_mode(), instead of adding this init flag.
Regards,
-Bin.
>
> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> ---
> drivers/usb/musb/am35x.c | 2 +-
> drivers/usb/musb/blackfin.c | 2 +-
> drivers/usb/musb/da8xx.c | 2 +-
> drivers/usb/musb/davinci.c | 2 +-
> drivers/usb/musb/musb_core.c | 12 ++++++------
> drivers/usb/musb/musb_core.h | 6 +++---
> drivers/usb/musb/musb_dsps.c | 2 +-
> drivers/usb/musb/sunxi.c | 2 +-
> drivers/usb/musb/tusb6010.c | 2 +-
> 9 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/usb/musb/am35x.c b/drivers/usb/musb/am35x.c
> index 50ca805..7136888 100644
> --- a/drivers/usb/musb/am35x.c
> +++ b/drivers/usb/musb/am35x.c
> @@ -332,7 +332,7 @@ static irqreturn_t am35x_musb_interrupt(int irq, void *hci)
> return ret;
> }
>
> -static int am35x_musb_set_mode(struct musb *musb, u8 musb_mode)
> +static int am35x_musb_set_mode(struct musb *musb, u8 musb_mode, bool init)
> {
> struct device *dev = musb->controller;
> struct musb_hdrc_platform_data *plat = dev_get_platdata(dev);
> diff --git a/drivers/usb/musb/blackfin.c b/drivers/usb/musb/blackfin.c
> index 310238c..544e98f 100644
> --- a/drivers/usb/musb/blackfin.c
> +++ b/drivers/usb/musb/blackfin.c
> @@ -356,7 +356,7 @@ static int bfin_musb_vbus_status(struct musb *musb)
> return 0;
> }
>
> -static int bfin_musb_set_mode(struct musb *musb, u8 musb_mode)
> +static int bfin_musb_set_mode(struct musb *musb, u8 musb_mode, bool init)
> {
> return -EIO;
> }
> diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
> index 6749aa1..ac0c2f7 100644
> --- a/drivers/usb/musb/da8xx.c
> +++ b/drivers/usb/musb/da8xx.c
> @@ -335,7 +335,7 @@ static irqreturn_t da8xx_musb_interrupt(int irq, void *hci)
> return ret;
> }
>
> -static int da8xx_musb_set_mode(struct musb *musb, u8 musb_mode)
> +static int da8xx_musb_set_mode(struct musb *musb, u8 musb_mode, bool init)
> {
> struct da8xx_glue *glue = dev_get_drvdata(musb->controller->parent);
> enum phy_mode phy_mode;
> diff --git a/drivers/usb/musb/davinci.c b/drivers/usb/musb/davinci.c
> index cee61a5..d12b902 100644
> --- a/drivers/usb/musb/davinci.c
> +++ b/drivers/usb/musb/davinci.c
> @@ -369,7 +369,7 @@ static irqreturn_t davinci_musb_interrupt(int irq, void *__hci)
> return retval;
> }
>
> -static int davinci_musb_set_mode(struct musb *musb, u8 mode)
> +static int davinci_musb_set_mode(struct musb *musb, u8 mode, bool init)
> {
> /* EVM can't do this (right?) */
> return -EIO;
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index 27dadc0..4a8d394 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -1730,11 +1730,11 @@ musb_mode_store(struct device *dev, struct device_attribute *attr,
>
> spin_lock_irqsave(&musb->lock, flags);
> if (sysfs_streq(buf, "host"))
> - status = musb_platform_set_mode(musb, MUSB_HOST);
> + status = musb_platform_set_mode(musb, MUSB_HOST, false);
> else if (sysfs_streq(buf, "peripheral"))
> - status = musb_platform_set_mode(musb, MUSB_PERIPHERAL);
> + status = musb_platform_set_mode(musb, MUSB_PERIPHERAL, false);
> else if (sysfs_streq(buf, "otg"))
> - status = musb_platform_set_mode(musb, MUSB_OTG);
> + status = musb_platform_set_mode(musb, MUSB_OTG, false);
> else
> status = -EINVAL;
> spin_unlock_irqrestore(&musb->lock, flags);
> @@ -2261,13 +2261,13 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
> status = musb_host_setup(musb, plat->power);
> if (status < 0)
> goto fail3;
> - status = musb_platform_set_mode(musb, MUSB_HOST);
> + status = musb_platform_set_mode(musb, MUSB_HOST, true);
> break;
> case MUSB_PORT_MODE_GADGET:
> status = musb_gadget_setup(musb);
> if (status < 0)
> goto fail3;
> - status = musb_platform_set_mode(musb, MUSB_PERIPHERAL);
> + status = musb_platform_set_mode(musb, MUSB_PERIPHERAL, true);
> break;
> case MUSB_PORT_MODE_DUAL_ROLE:
> status = musb_host_setup(musb, plat->power);
> @@ -2278,7 +2278,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
> musb_host_cleanup(musb);
> goto fail3;
> }
> - status = musb_platform_set_mode(musb, MUSB_OTG);
> + status = musb_platform_set_mode(musb, MUSB_OTG, true);
> break;
> default:
> dev_err(dev, "unsupported port mode %d\n", musb->port_mode);
> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
> index 2cb88a49..1bfc8fa 100644
> --- a/drivers/usb/musb/musb_core.h
> +++ b/drivers/usb/musb/musb_core.h
> @@ -203,7 +203,7 @@ struct musb_platform_ops {
> struct dma_controller *
> (*dma_init) (struct musb *musb, void __iomem *base);
> void (*dma_exit)(struct dma_controller *c);
> - int (*set_mode)(struct musb *musb, u8 mode);
> + int (*set_mode)(struct musb *musb, u8 mode, bool init);
> void (*try_idle)(struct musb *musb, unsigned long timeout);
> int (*recover)(struct musb *musb);
>
> @@ -558,12 +558,12 @@ static inline void musb_platform_disable(struct musb *musb)
> musb->ops->disable(musb);
> }
>
> -static inline int musb_platform_set_mode(struct musb *musb, u8 mode)
> +static inline int musb_platform_set_mode(struct musb *musb, u8 mode, bool init)
> {
> if (!musb->ops->set_mode)
> return 0;
>
> - return musb->ops->set_mode(musb, mode);
> + return musb->ops->set_mode(musb, mode, init);
> }
>
> static inline void musb_platform_try_idle(struct musb *musb,
> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
> index 0f17d21..a889679 100644
> --- a/drivers/usb/musb/musb_dsps.c
> +++ b/drivers/usb/musb/musb_dsps.c
> @@ -462,7 +462,7 @@ static int dsps_musb_exit(struct musb *musb)
> return 0;
> }
>
> -static int dsps_musb_set_mode(struct musb *musb, u8 mode)
> +static int dsps_musb_set_mode(struct musb *musb, u8 mode, bool init)
> {
> struct device *dev = musb->controller;
> struct dsps_glue *glue = dev_get_drvdata(dev->parent);
> diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
> index 1408245..04bf763 100644
> --- a/drivers/usb/musb/sunxi.c
> +++ b/drivers/usb/musb/sunxi.c
> @@ -346,7 +346,7 @@ static void sunxi_musb_dma_controller_destroy(struct dma_controller *c)
> {
> }
>
> -static int sunxi_musb_set_mode(struct musb *musb, u8 mode)
> +static int sunxi_musb_set_mode(struct musb *musb, u8 mode, bool init)
> {
> struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);
> enum phy_mode new_mode;
> diff --git a/drivers/usb/musb/tusb6010.c b/drivers/usb/musb/tusb6010.c
> index df7c9f4..8a74587 100644
> --- a/drivers/usb/musb/tusb6010.c
> +++ b/drivers/usb/musb/tusb6010.c
> @@ -628,7 +628,7 @@ static void tusb_musb_set_vbus(struct musb *musb, int is_on)
> * Note that if a mini-A cable is plugged in the ID line will stay down as
> * the weak ID pull-up is not able to pull the ID up.
> */
> -static int tusb_musb_set_mode(struct musb *musb, u8 musb_mode)
> +static int tusb_musb_set_mode(struct musb *musb, u8 musb_mode, bool init)
> {
> void __iomem *tbase = musb->ctrl_base;
> u32 otg_stat, phy_otg_ctrl, phy_otg_ena, dev_conf;
> --
> 2.7.3
>
^ permalink raw reply
* Re: [Qemu-devel] [PATCH 3/3] 9pfs: add cleanup operation for proxy backend driver
From: Greg Kurz @ 2016-11-14 17:36 UTC (permalink / raw)
To: Li Qiang; +Cc: qemu-devel, liqiang6-s
In-Reply-To: <5829af16.84f4420a.a436f.4a79@mx.google.com>
On Mon, 14 Nov 2016 07:32:58 -0500
Li Qiang <liq3ea@gmail.com> wrote:
> From: Li Qiang <liq3ea@gmail.com>
>
> In the init operation of proxy backend dirver, it allocates a
> V9fsProxy struct and some other resources. We should free these
> resources when the 9pfs device is unrealized. This is what this
> patch does.
>
> Signed-off-by: Li Qiang <liq3ea@gmail.com>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
> hw/9pfs/9p-proxy.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
> index f2417b7..4b22f57 100644
> --- a/hw/9pfs/9p-proxy.c
> +++ b/hw/9pfs/9p-proxy.c
> @@ -1168,9 +1168,19 @@ static int proxy_init(FsContext *ctx)
> return 0;
> }
>
> +static void proxy_cleanup(FsContext *ctx)
> +{
> + V9fsProxy *proxy = ctx->private;
> + close(proxy->sockfd);
> + g_free(proxy->in_iovec.iov_base);
> + g_free(proxy->out_iovec.iov_base);
> + g_free(proxy);
> +}
> +
> FileOperations proxy_ops = {
> .parse_opts = proxy_parse_opts,
> .init = proxy_init,
> + .cleanup = proxy_cleanup,
> .lstat = proxy_lstat,
> .readlink = proxy_readlink,
> .close = proxy_close,
^ permalink raw reply
* Re: [Qemu-devel] [PATCH 2/3] 9pfs: add cleanup operation for handle backend driver
From: Greg Kurz @ 2016-11-14 17:36 UTC (permalink / raw)
To: Li Qiang; +Cc: qemu-devel, liqiang6-s
In-Reply-To: <5829af0f.84f4420a.a436f.4a73@mx.google.com>
On Mon, 14 Nov 2016 07:32:57 -0500
Li Qiang <liq3ea@gmail.com> wrote:
> From: Li Qiang <liq3ea@gmail.com>
>
> In the init operation of handle backend dirver, it allocates a
> handle_data struct and opens a mount file. We should free these
> resources when the 9pfs device is unrealized. This is what this
> patch does.
>
> Signed-off-by: Li Qiang <liq3ea@gmail.com>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
> hw/9pfs/9p-handle.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/hw/9pfs/9p-handle.c b/hw/9pfs/9p-handle.c
> index 3d77594..9b50f40 100644
> --- a/hw/9pfs/9p-handle.c
> +++ b/hw/9pfs/9p-handle.c
> @@ -649,6 +649,13 @@ out:
> return ret;
> }
>
> +static void handle_cleanup(FsContext *ctx)
> +{
> + struct handle_data *data = ctx->private;
> + close(data->mountfd);
> + g_free(data);
> +}
> +
> static int handle_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse)
> {
> const char *sec_model = qemu_opt_get(opts, "security_model");
> @@ -671,6 +678,7 @@ static int handle_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse)
> FileOperations handle_ops = {
> .parse_opts = handle_parse_opts,
> .init = handle_init,
> + .cleanup = handle_cleanup,
> .lstat = handle_lstat,
> .readlink = handle_readlink,
> .close = handle_close,
^ permalink raw reply
* Re: [PATCH 0/5] net: thunderx: Miscellaneous fixes
From: Matthias Brugger @ 2016-11-14 17:35 UTC (permalink / raw)
To: David Miller
Cc: sunil.kovvuri, netdev, sgoutham, linux-kernel, linux-arm-kernel
In-Reply-To: <20161114.123253.476733911183519472.davem@davemloft.net>
On 11/14/2016 06:32 PM, David Miller wrote:
> From: Matthias Brugger <mbrugger@suse.com>
> Date: Mon, 14 Nov 2016 13:01:25 +0100
>
>>
>>
>> On 14/11/16 11:54, sunil.kovvuri@gmail.com wrote:
>>> From: Sunil Goutham <sgoutham@cavium.com>
>>>
>>> This patchset includes fixes for incorrect LMAC credits,
>>> unreliable driver statistics, memory leak upon interface
>>> down e.t.c
>>>
>>
>> Are these fixes relevant to for older kernels as well?
>> If so, please add "Cc: stable@vger.kernel.org" to the Sigend-off list.
>
> This is not appropriate for networking patches.
>
> People instead explicitly request -stable inclusion when the
> submit networking changes to me, and I queue them up for
> later submission.
>
Ok, thanks for clarification.
^ permalink raw reply
* Re: [PATCH net 2/3] bpf, mlx5: fix various refcount/prog issues in mlx5e_xdp_set
From: Alexei Starovoitov @ 2016-11-14 17:35 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, bblanco, tariqt, zhiyisun, ranas, netdev
In-Reply-To: <58297AAD.8060003@iogearbox.net>
On Mon, Nov 14, 2016 at 09:49:49AM +0100, Daniel Borkmann wrote:
> On 11/14/2016 03:49 AM, Alexei Starovoitov wrote:
> >On Mon, Nov 14, 2016 at 01:43:41AM +0100, Daniel Borkmann wrote:
> [...]
> >>diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> >>index 751e806..a0fca9f 100644
> >>--- a/kernel/bpf/syscall.c
> >>+++ b/kernel/bpf/syscall.c
> >>@@ -682,6 +682,17 @@ struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i)
> >> }
> >> EXPORT_SYMBOL_GPL(bpf_prog_add);
> >>
> >>+void bpf_prog_sub(struct bpf_prog *prog, int i)
> >>+{
> >>+ /* Only to be used for undoing previous bpf_prog_add() in some
> >>+ * error path. We still know that another entity in our call
> >>+ * path holds a reference to the program, thus atomic_sub() can
> >>+ * be safely used in such cases!
> >>+ */
> >>+ WARN_ON(atomic_sub_return(i, &prog->aux->refcnt) == 0);
> >>+}
> >>+EXPORT_SYMBOL_GPL(bpf_prog_sub);
> >
> >the patches look good. I'm only worried about net/net-next merge
> >conflict here. (I would have to deal with it as well).
> >So instead of copying the above helper can we apply net-next's
> >'bpf, mlx4: fix prog refcount in mlx4_en_try_alloc_resources error path'
> >patch to net without mlx4_xdp_set hunk and then apply
> >the rest of this patch?
> >Even better is to send this patch 2/3 to net-next?
> >yes, it's an issue, but very small one. There is no security
> >concern here, so I would prefer to avoid merge conflict.
> >Did you do a test merge of net/net-next by any chance?
>
> Yes, I did a test merge and git resolved the above just fine w/o
> any conflicts. I have no strong opinion whether net or net-next.
> If preferred, I can just resend this series in the evening against
> net-next instead, perhaps that's a bit better.
I have slight preference to go via net-next, but since it merges fine,
I don't mind net route too.
^ permalink raw reply
* [PATCH 0/5] net: thunderx: Miscellaneous fixes
From: Matthias Brugger @ 2016-11-14 17:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161114.123253.476733911183519472.davem@davemloft.net>
On 11/14/2016 06:32 PM, David Miller wrote:
> From: Matthias Brugger <mbrugger@suse.com>
> Date: Mon, 14 Nov 2016 13:01:25 +0100
>
>>
>>
>> On 14/11/16 11:54, sunil.kovvuri at gmail.com wrote:
>>> From: Sunil Goutham <sgoutham@cavium.com>
>>>
>>> This patchset includes fixes for incorrect LMAC credits,
>>> unreliable driver statistics, memory leak upon interface
>>> down e.t.c
>>>
>>
>> Are these fixes relevant to for older kernels as well?
>> If so, please add "Cc: stable at vger.kernel.org" to the Sigend-off list.
>
> This is not appropriate for networking patches.
>
> People instead explicitly request -stable inclusion when the
> submit networking changes to me, and I queue them up for
> later submission.
>
Ok, thanks for clarification.
^ permalink raw reply
* Re: [PATCH v2 0/2] bnx2: Wait for in-flight DMA to complete at probe stage
From: Paul Menzel @ 2016-11-14 17:35 UTC (permalink / raw)
To: David Miller
Cc: bhe, netdev, michael.chan, linux-kernel, Dept-GELinuxNICDev,
rasesh.mody, harish.patil, frank, jsr, jroedel, dyoung
In-Reply-To: <20161114.122838.1039215178593568702.davem@davemloft.net>
On 11/14/16 18:28, David Miller wrote:
> From: Paul Menzel <pmenzel@molgen.mpg.de>
> Date: Mon, 14 Nov 2016 09:25:47 +0100
>> On 11/13/16 06:01, Baoquan He wrote:
>>> This is v2 post.
>>>
>>> In commit 3e1be7a ("bnx2: Reset device during driver initialization"),
>>> firmware requesting code was moved from open stage to probe stage.
>>> The reason is in kdump kernel hardware iommu need device be reset in
>>> driver probe stage, otherwise those in-flight DMA from 1st kernel
>>> will continue going and look up into the newly created io-page tables.
>>> However bnx2 chip resetting involves firmware requesting issue, that
>>> need be done in open stage.
>>>
>>> Michale Chan suggested we can just wait for the old in-flight DMA to
>>> complete at probe stage, then though without device resetting, we
>>> don't need to worry the old in-flight DMA could continue looking up
>>> the newly created io-page tables.
>>>
>>> v1->v2:
>>> Michael suggested to wait for the in-flight DMA to complete at probe
>>> stage. So give up the old method of trying to reset chip at probe
>>> stage, take the new way accordingly.
>>
>> thank you for posting the updated series. Could you please resend a v3
>> with stable@vger.kernel.org added [1]?
>
> This is not the proper procedure for networking patches.
Oh, I didn’t know that. Sorry for spreading false information.
Kind regards,
Paul
^ permalink raw reply
* Re: [PATCH v2 1/2] mtd: nand: Support controllers with custom page
From: Boris Brezillon @ 2016-11-14 17:34 UTC (permalink / raw)
To: Marc Gonzalez; +Cc: Richard Weinberger, linux-mtd, Mason, Sebastian Frias
In-Reply-To: <5829EDE7.1060300@sigmadesigns.com>
Hi Marc,
On Mon, 14 Nov 2016 18:01:27 +0100
Marc Gonzalez <marc_gonzalez@sigmadesigns.com> wrote:
> If your controller already sends the required NAND commands when
> reading or writing a page, then the framework is not supposed to
> send READ0 and SEQIN/PAGEPROG respectively.
>
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
> v2:
> (chip) in NAND_HAS_SUBPAGE_WRITE macro
> s/nand_default_page_accessors/nand_standard_page_accessors/
> scripts/checkpatch.pl --strict v2-0001-mtd-nand-Support-controllers-with-custom-page-acc.patch
> total: 0 errors, 0 warnings, 0 checks, 91 lines checked
> ---
> drivers/mtd/nand/nand_base.c | 31 ++++++++++++++++++++++++++++---
> include/linux/mtd/nand.h | 12 ++++++++++++
> 2 files changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 50cdf37cb8e4..db5f27db3748 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -1970,7 +1970,8 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
> __func__, buf);
>
> read_retry:
> - chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
> + if (nand_standard_page_accessors(&chip->ecc))
> + chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
>
> /*
> * Now read the page into the buffer. Absent an error,
> @@ -2658,7 +2659,8 @@ static int nand_write_page(struct mtd_info *mtd, struct nand_chip *chip,
> else
> subpage = 0;
>
> - chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
> + if (nand_standard_page_accessors(&chip->ecc))
> + chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
>
> if (unlikely(raw))
> status = chip->ecc.write_page_raw(mtd, chip, buf,
> @@ -2681,7 +2683,8 @@ static int nand_write_page(struct mtd_info *mtd, struct nand_chip *chip,
>
> if (!cached || !NAND_HAS_CACHEPROG(chip)) {
>
> - chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
> + if (nand_standard_page_accessors(&chip->ecc))
> + chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
> status = chip->waitfunc(mtd, chip);
> /*
> * See if operation failed and additional status checks are
> @@ -4539,6 +4542,25 @@ static bool nand_ecc_strength_good(struct mtd_info *mtd)
> return corr >= ds_corr && ecc->strength >= chip->ecc_strength_ds;
> }
>
> +static bool invalid_ecc_page_accessors(struct nand_chip *chip)
> +{
> + struct nand_ecc_ctrl *ecc = &chip->ecc;
> +
> + if (nand_standard_page_accessors(ecc))
> + return false;
> +
> + /*
> + * NAND_ECC_CUSTOM_PAGE_ACCESS flag is set, make sure the NAND
> + * controller driver implements all the page accessors because
> + * default helpers are not suitable when the core does not
> + * send the READ0/PAGEPROG commands.
> + */
> + return (!ecc->read_page || !ecc->write_page ||
> + !ecc->read_page_raw || !ecc->write_page_raw ||
> + (NAND_HAS_SUBPAGE_READ(chip) && !ecc->read_subpage) ||
> + (NAND_HAS_SUBPAGE_WRITE(chip) && !ecc->write_subpage));
Just had a closer look at the nand_write_page() function, and it seems
that !NAND_NO_SUBPAGE_WRITE && !ecc->write_subpage is valid use case.
In this situation, nand_write_page() uses ecc->write_page().
I know it's a real mess, and each time I look at this subpage support
detection, I have a hard time remembering how it works, and why it's
done this way. The idea is that, even if the controller does not
explicitly implement ->write_subpage(), the core will fill the page
buffer with 0xff, and since programming NANDs is just about moving bits
from 1 to zero, when you leave bits to one, they are just and-ed with
the previous content.
The correct test is:
return (!ecc->read_page || !ecc->write_page ||
!ecc->read_page_raw || !ecc->write_page_raw ||
(NAND_HAS_SUBPAGE_READ(chip) && !ecc->read_subpage) ||
(NAND_HAS_SUBPAGE_WRITE(chip) && !ecc->write_subpage &&
ecc->hwctl && ecc->calculate))
> +}
> +
> /**
> * nand_scan_tail - [NAND Interface] Scan for the NAND device
> * @mtd: MTD device structure
> @@ -4559,6 +4581,9 @@ int nand_scan_tail(struct mtd_info *mtd)
> !(chip->bbt_options & NAND_BBT_USE_FLASH)))
> return -EINVAL;
>
> + if (WARN_ON(invalid_ecc_page_accessors(chip)))
> + return -EINVAL;
> +
Sorry, I didn't notice that one in my previous review. We're currently
trying to get rid of all BUG() and WARN_ON() calls in the NAND
framework. Can you replace this WARN_ON() by a pr_err() message?
> if (!(chip->options & NAND_OWN_BUFFERS)) {
> nbuf = kzalloc(sizeof(*nbuf) + mtd->writesize
> + mtd->oobsize * 3, GFP_KERNEL);
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 06d0c9d740f7..c5f3a012ae62 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -142,6 +142,12 @@ enum nand_ecc_algo {
> */
> #define NAND_ECC_GENERIC_ERASED_CHECK BIT(0)
> #define NAND_ECC_MAXIMIZE BIT(1)
> +/*
> + * If your controller already sends the required NAND commands when
> + * reading or writing a page, then the framework is not supposed to
> + * send READ0 and SEQIN/PAGEPROG respectively.
> + */
> +#define NAND_ECC_CUSTOM_PAGE_ACCESS BIT(2)
>
> /* Bit mask for flags passed to do_nand_read_ecc */
> #define NAND_GET_DEVICE 0x80
> @@ -186,6 +192,7 @@ enum nand_ecc_algo {
> /* Macros to identify the above */
> #define NAND_HAS_CACHEPROG(chip) ((chip->options & NAND_CACHEPRG))
> #define NAND_HAS_SUBPAGE_READ(chip) ((chip->options & NAND_SUBPAGE_READ))
> +#define NAND_HAS_SUBPAGE_WRITE(chip) !((chip)->options & NAND_NO_SUBPAGE_WRITE)
>
> /* Non chip related options */
> /* This option skips the bbt scan during initialization. */
> @@ -568,6 +575,11 @@ struct nand_ecc_ctrl {
> int page);
> };
>
> +static inline int nand_standard_page_accessors(struct nand_ecc_ctrl *ecc)
> +{
> + return !(ecc->options & NAND_ECC_CUSTOM_PAGE_ACCESS);
> +}
> +
> /**
> * struct nand_buffers - buffer structure for read/write
> * @ecccalc: buffer pointer for calculated ECC, size is oobsize.
^ permalink raw reply
* Re: [PATCH] drm/i915/GuC: Combine the two kernel parameter into one
From: Srivatsa, Anusha @ 2016-11-14 17:34 UTC (permalink / raw)
To: Tvrtko Ursulin, Mcgee, Jeff
Cc: intel-gfx@lists.freedesktop.org, Vivi, Rodrigo
In-Reply-To: <9fd4c39b-77f8-6d23-e4dc-727891068cbc@linux.intel.com>
>-----Original Message-----
>From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com]
>Sent: Monday, November 14, 2016 1:35 AM
>To: Mcgee, Jeff <jeff.mcgee@intel.com>; Srivatsa, Anusha
><anusha.srivatsa@intel.com>
>Cc: Ursulin, Tvrtko <tvrtko.ursulin@intel.com>; intel-gfx@lists.freedesktop.org;
>Vivi, Rodrigo <rodrigo.vivi@intel.com>
>Subject: Re: [Intel-gfx] [PATCH] drm/i915/GuC: Combine the two kernel
>parameter into one
>
>
>[corrected my email in cc]
Thankyou!
>On 12/11/2016 02:21, Jeff McGee wrote:
>> On Wed, Nov 09, 2016 at 11:11:07AM -0800, Anusha Srivatsa wrote:
>>> Replace i915.enable_guc_loading and i915.enable_guc_submission with a
>>> single parameter - i915.enable_guc. Where:
>>>
>>> -1 : Platform default (Only load GuC)
>>> 0 : Do not use GuC
>>> 1 : Load GuC, do not use Command Submission through GuC
>>> 2 : Load and use GuC for Command Submission
>>>
>> I think this approach could get ugly as we add more GuC functionality
>> and the list of combinations under this one parameter grows.
Yes I see your point.
>> What is the issue we are trying to solve? I thought it was that folks
>> didn't like that we had an option to just load GuC and do nothing with it.
>> Can those folks please comment?
Two parameters was not desired. One parameter that could control the GuC functionality was something that led to this patch.
>I am not one of those folks but I also am sure about the proposed change. Same
>concern about extensibility and also usability.
>
>What is the difference between -1 and 1 for example? Is 1 equivalent to the
>current "must use" (2) option? And -1 is equivalent to the current "try to use" (1)?
Right now -1 is for platform default and 1 is for loading and no submission. -1: platform default for now is set to do only loading of GuC firmware, unless we change that. For now both 1 and -1 are the same.
In future say if for SKL we make loading and submission as default, the code has to be changes slightly.
>Then you got proposed 2 (load and use guc) but that does not capture the option
>for built-in GuC firmware Dave has planned for in his version. I don't know if that
>is real or not, just saying
>
>I am also not sure it is so imperative to change this at all. But if people do want to
>change it we should make it really good. :)
I agree. I sent this patch to see what people feel about it and if we have to go ahead with these changes.
>One idea could be to hide the guc loading form the user altogether and hence
>improve usability (decrease exposed complexity) by having only two parameters;
>i915.enable_guc_scheduling and i915.enable_huc.
That's a good point. But with this we will have two parameters (which kills the point of why the patch was written in the first place), then we can rather leave it the way it is. Right?
-Anusha
>Whether or not firmware would be loaded would depend on if either of the two is
>turned on. That would also preserve the option of current fallback or wedge
>behaviour for guc.
>
>Regards,
>
>Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [RFC PATCH 01/24] ARM: GICv3 ITS: parse and store ITS subnodes from hardware DT
From: Andre Przywara @ 2016-11-14 17:35 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini; +Cc: xen-devel
In-Reply-To: <0310f4b8-c9f9-a478-65e0-1259b2baefd4@arm.com>
Hi,
On 01/11/16 15:13, Julien Grall wrote:
> Hi Andre,
>
> On 28/09/2016 19:24, Andre Przywara wrote:
>> Parse the DT GIC subnodes to find every ITS MSI controller the hardware
>> offers. Store that information in a list to both propagate all of them
>> later to Dom0, but also to be able to iterate over all ITSes.
>> This introduces an ITS Kconfig option.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>> xen/arch/arm/Kconfig | 5 ++++
>> xen/arch/arm/Makefile | 1 +
>> xen/arch/arm/gic-its.c | 67
>> +++++++++++++++++++++++++++++++++++++++++++
>> xen/arch/arm/gic-v3.c | 6 ++++
>> xen/include/asm-arm/gic-its.h | 57 ++++++++++++++++++++++++++++++++++++
>> 5 files changed, 136 insertions(+)
>> create mode 100644 xen/arch/arm/gic-its.c
>> create mode 100644 xen/include/asm-arm/gic-its.h
>>
>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>> index 797c91f..9fe3b8e 100644
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -45,6 +45,11 @@ config ACPI
>> config HAS_GICV3
>> bool
>>
>> +config HAS_ITS
>> + bool "GICv3 ITS MSI controller support"
>> + depends on ARM_64
>
> HAS_GICV3 will only be selected for 64-bit. It would need some rework to
> be supported on 32-bit. So I would drop this dependency.
OK, makes sense.
>> + depends on HAS_GICV3
>> +
>
> I am not convinced that we should (currently) let the user selecting the
> ITS support. It increases the test coverage (we have to test with and
> without). Do we expect people using GICv3 without ITS?
My concern was more that if it breaks something, people can just disable
it. But I have to go through the patches again to see if disabling it
really brings us something (because thinking about it I don't think so).
So given the test coverage argument I think we should at least enable it
by default for ARM64. Is there some "expert options" group somewhere
where we could insert the option to turn it off?
Cheers,
Andre.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply
* Re: [PATCH v3] ip6_output: ensure flow saddr actually belongs to device
From: Hannes Frederic Sowa @ 2016-11-14 17:33 UTC (permalink / raw)
To: David Ahern, Jason A. Donenfeld, Netdev, WireGuard mailing list,
LKML, YOSHIFUJI Hideaki
In-Reply-To: <c75290bd-7ea2-0bf8-7ac0-fc5a3c81e312@cumulusnetworks.com>
On 14.11.2016 18:17, David Ahern wrote:
> On 11/14/16 10:04 AM, Hannes Frederic Sowa wrote:
>> On 14.11.2016 17:55, David Ahern wrote:
>>> On 11/14/16 9:44 AM, Hannes Frederic Sowa wrote:
>>>> On Mon, Nov 14, 2016, at 00:28, Jason A. Donenfeld wrote:
>>>>> This puts the IPv6 routing functions in parity with the IPv4 routing
>>>>> functions. Namely, we now check in v6 that if a flowi6 requests an
>>>>> saddr, the returned dst actually corresponds to a net device that has
>>>>> that saddr. This mirrors the v4 logic with __ip_dev_find in
>>>>> __ip_route_output_key_hash. In the event that the returned dst is not
>>>>> for a dst with a dev that has the saddr, we return -EINVAL, just like
>>>>> v4; this makes it easy to use the same error handlers for both cases.
>>>>>
>>>>> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
>>>>> Cc: David Ahern <dsa@cumulusnetworks.com>
>>>>> ---
>>>>> Changes from v2:
>>>>> It turns out ipv6_chk_addr already has the device enumeration
>>>>> logic that we need by simply passing NULL.
>>>>>
>>>>> net/ipv6/ip6_output.c | 4 ++++
>>>>> 1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
>>>>> index 6001e78..b3b5cb6 100644
>>>>> --- a/net/ipv6/ip6_output.c
>>>>> +++ b/net/ipv6/ip6_output.c
>>>>> @@ -926,6 +926,10 @@ static int ip6_dst_lookup_tail(struct net *net,
>>>>> const struct sock *sk,
>>>>> int err;
>>>>> int flags = 0;
>>>>>
>>>>> + if (!ipv6_addr_any(&fl6->saddr) &&
>>>>> + !ipv6_chk_addr(net, &fl6->saddr, NULL, 1))
>>>>> + return -EINVAL;
>>>>
>>>> Hmm, this check is too permissive, no?
>>>>
>>>> E.g. what happens if you move a link local address from one interface to
>>>> another? In this case this code would still allow the saddr to be used.
>>>
>>> This check -- like the ipv4 variant -- only verifies the saddr is locally assigned. If the address moves interfaces it should be fine.
>>
>> But in this case we should actually bail out, no?
>>
>> Let's say, user assumes we are on ifindex eth0 with LL address from
>> eth0. Suddenly the LL address from eth0 is moved to eth1, we can't
>> accept this source address anymore and need to return -EINVAL, too.
>
> so you mean if rt6_need_strict(&fl6->saddr) then the dev needs to be considered.
Exactly, like we do in the user space facing APIs.
>>>> I just also quickly read up on the history (sorry was travelling last
>>>> week) and wonder if you ever saw a user space facing bug or if this is
>>>> basically some difference you saw while writing out of tree code?
>>>
>>> I checked the userspace API this morning. bind and cmsg for example check that the address is valid with calls to ipv6_chk_addr.
>>
>> Hmm, so it fixes no real bug.
>>
>> Because of translations of flowi6_oif we actually can't do a correct
>> check of source address for cases like the one I outlined above? Hmm,
>> maybe we should simply depend on user space checks.
>
> I believe Jason's case is forwarding path and the ipv6_stub->ipv6_dst_lookup API.
It is not a kernel API, because we don't support something like that for
external kernel modules. We basically exported ipv6_dst_lookup to allow
some IPv4 code to do ipv6 stunts when the IPv6 module is loaded. ;)
Bye,
Hannes
^ permalink raw reply
* Re: [PATCH 1/2] mfd: pm8921: add support to pm8821
From: Srinivas Kandagatla @ 2016-11-14 17:33 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Lee Jones, Rob Herring, Andy Gross, devicetree, linux-kernel,
linux-arm-msm, linux-soc, linux-arm-kernel
In-Reply-To: <20161108190751.GO25787@tuxbot>
Thanks Bjorn for review comments.
On 08/11/16 19:07, Bjorn Andersson wrote:
> On Tue 08 Nov 08:29 PST 2016, Srinivas Kandagatla wrote:
>
>> This patch adds support to PM8821 PMIC and interrupt support.
>> PM8821 is companion device that supplements primary PMIC PM8921 IC.
>>
>
> Linus Walleij has a patch out for renaming a lot of things in this file,
> so we should probably make sure that lands and then rebase this ontop.
>
Yep, Will rebase on top of it.
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>> Tested this patch for MPP and IRQ functionality on IFC6410 and SD600 EVAL
>> board with mpps PM8821 and PM8921.
>>
>> .../devicetree/bindings/mfd/qcom-pm8xxx.txt | 1 +
>> drivers/mfd/pm8921-core.c | 368 +++++++++++++++++++--
>> 2 files changed, 340 insertions(+), 29 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt b/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
>> index 37a088f..8f1b4ec 100644
>> --- a/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
>> +++ b/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
>> @@ -11,6 +11,7 @@ voltages and other various functionality to Qualcomm SoCs.
>> Definition: must be one of:
>> "qcom,pm8058"
>> "qcom,pm8921"
>> + "qcom,pm8821"
>>
>> - #address-cells:
>> Usage: required
>> diff --git a/drivers/mfd/pm8921-core.c b/drivers/mfd/pm8921-core.c
>> index 0e3a2ea..28c2470 100644
>> --- a/drivers/mfd/pm8921-core.c
>> +++ b/drivers/mfd/pm8921-core.c
>> @@ -28,16 +28,26 @@
>> #include <linux/mfd/core.h>
>>
>> #define SSBI_REG_ADDR_IRQ_BASE 0x1BB
>> -
>> -#define SSBI_REG_ADDR_IRQ_ROOT (SSBI_REG_ADDR_IRQ_BASE + 0)
>> -#define SSBI_REG_ADDR_IRQ_M_STATUS1 (SSBI_REG_ADDR_IRQ_BASE + 1)
>> -#define SSBI_REG_ADDR_IRQ_M_STATUS2 (SSBI_REG_ADDR_IRQ_BASE + 2)
>> -#define SSBI_REG_ADDR_IRQ_M_STATUS3 (SSBI_REG_ADDR_IRQ_BASE + 3)
>> -#define SSBI_REG_ADDR_IRQ_M_STATUS4 (SSBI_REG_ADDR_IRQ_BASE + 4)
>> -#define SSBI_REG_ADDR_IRQ_BLK_SEL (SSBI_REG_ADDR_IRQ_BASE + 5)
>> -#define SSBI_REG_ADDR_IRQ_IT_STATUS (SSBI_REG_ADDR_IRQ_BASE + 6)
>> -#define SSBI_REG_ADDR_IRQ_CONFIG (SSBI_REG_ADDR_IRQ_BASE + 7)
>> -#define SSBI_REG_ADDR_IRQ_RT_STATUS (SSBI_REG_ADDR_IRQ_BASE + 8)
>
> Keep these (per argumentation that follows), but try to name them
> appropriately.
>
Yes, I agree, I will address all the comments related to register
defines in next version.
...
>
>>
>> #define PM_IRQF_LVL_SEL 0x01 /* level select */
>> #define PM_IRQF_MASK_FE 0x02 /* mask falling edge */
>> @@ -54,30 +64,41 @@
>> #define REG_HWREV_2 0x0E8 /* PMIC4 revision 2 */
>>
>> #define PM8921_NR_IRQS 256
>> +#define PM8821_NR_IRQS 112
>>
>> struct pm_irq_chip {
>> struct regmap *regmap;
>> spinlock_t pm_irq_lock;
>> struct irq_domain *irqdomain;
>> + unsigned int irq_reg_base;
>> unsigned int num_irqs;
>> unsigned int num_blocks;
>> unsigned int num_masters;
>> u8 config[0];
>> };
>>
>> +struct pm8xxx_data {
>> + int num_irqs;
>> + unsigned int irq_reg_base;
>
> As far as I can see this is always SSBI_PM8821_REG_ADDR_IRQ_BASE in the
> 8821 functions and SSBI_REG_ADDR_IRQ_BASE in the pm8xxx functions. If
> you have disjunct code paths I think it's better to not obscure this
> with a variable.
>
> Try renaming the constants appropriately instead. This also has the
> benefit of reducing the size of the patch slightly.
>
Yep, will remove reg_base variable.
>>
...
>>
>> +static int pm8821_read_master_irq(const struct pm_irq_chip *chip,
>> + int m, unsigned int *master)
>> +{
>
> I think you should inline this, as you already have the calls unrolled
> in pm8821_irq_handler().
We can just call regmap_read directly from the caller function, and get
rid of this function all together.
>
>> + unsigned int base;
>> +
>> + if (!m)
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
>> + else
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
>> +
>> + return regmap_read(chip->regmap, base, master);
>> +}
>> +
>> +static int pm8821_read_block_irq(struct pm_irq_chip *chip, int master,
>> + u8 block, unsigned int *bits)
>> +{
>> + int rc;
>> +
>> + unsigned int base;
>
> Odd empty line between rc and base. (And btw, sorting your local
> variables in descending length make things pretty).
Yep, will fix it in next version.
>
>> +
>> + if (!master)
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
>> + else
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
>> +
>> + spin_lock(&chip->pm_irq_lock);
>
> The reason why this is done under a lock in the other case is because
> the status register is paged, so you shouldn't need it here.
>
Thanks for the info, will remove it.
> With this updated I think you can favorably inline this into
> pm8821_irq_block_handler().
>
>> +
>> + rc = regmap_read(chip->regmap, base + block, bits);
>> + if (rc)
>> + pr_err("Failed Reading Status rc=%d\n", rc);
>> +
>> + spin_unlock(&chip->pm_irq_lock);
>> + return rc;
>> +}
>> +
>> +static int pm8821_irq_block_handler(struct pm_irq_chip *chip,
>> + int master_number, int block)
>> +{
>> + int pmirq, irq, i, ret;
>> + unsigned int bits;
>> +
>> + ret = pm8821_read_block_irq(chip, master_number, block, &bits);
>> + if (ret) {
>> + pr_err("Failed reading %d block ret=%d", block, ret);
>> + return ret;
>> + }
>> + if (!bits) {
>> + pr_err("block bit set in master but no irqs: %d", block);
>> + return 0;
>> + }
>> +
>> + /* Convert block offset to global block number */
>> + block += (master_number * PM8821_BLOCKS_PER_MASTER) - 1;
>
> So this is block -= 1 for master 0 and block += 6 for master 1, is the
> latter correct?
>
Yes, both of them are correct.
for master 0 which has block numbers from 1-7 should translate to 0-6 in
linear space.
for master 1 which has block numbers from 1-7 should translate to 7-13
in linear space.
so for master0 it is -=1 and and for master1 it is +=6 seems correct.
>> +
>> + /* Check IRQ bits */
>> + for (i = 0; i < 8; i++) {
>> + if (bits & BIT(i)) {
>> + pmirq = block * 8 + i;
>> + irq = irq_find_mapping(chip->irqdomain, pmirq);
>> + generic_handle_irq(irq);
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int pm8821_irq_read_master(struct pm_irq_chip *chip,
>> + int master_number, u8 master_val)
>
> This isn't so much a matter of "reading master X" as "handle master X".
>
Agreed, it would be more consistent with pm8xxx too.
> Also, you don't care about the return value, so no need to return one...
>
Yep will fix it.
>> +{
>> + int ret = 0;
>> + int block;
>> +
>> + for (block = 1; block < 8; block++) {
>> + if (master_val & BIT(block)) {
>> + ret |= pm8821_irq_block_handler(chip,
>> + master_number, block);
>> + }
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static void pm8821_irq_handler(struct irq_desc *desc)
>> +{
>> + struct pm_irq_chip *chip = irq_desc_get_handler_data(desc);
>> + struct irq_chip *irq_chip = irq_desc_get_chip(desc);
>> + int ret;
>> + unsigned int master;
>> +
>> + chained_irq_enter(irq_chip, desc);
>> + /* check master 0 */
>> + ret = pm8821_read_master_irq(chip, 0, &master);
>> + if (ret) {
>> + pr_err("Failed to re:Qad master 0 ret=%d\n", ret);
>> + return;
>> + }
>> +
>> + if (master & ~PM8821_IRQ_MASTER1_SET)
>
> Rather than having a define for MASTER1_SET use BIT(0) here and write a
> comment like:
>
Yep, I will add some comments in this area.
> "bits 1 through 7 marks the first 7 blocks"
>
>> + pm8821_irq_read_master(chip, 0, master);
>> +
>
> and then
>
> "bit 0 is set if second master contains any bits"
>
> Or just skip this optimization and check the two masters unconditionally
> in a loop.
>
>> + /* check master 1 */
>> + if (!(master & PM8821_IRQ_MASTER1_SET))
>> + goto done;
>> +
>> + ret = pm8821_read_master_irq(chip, 1, &master);
>> + if (ret) {
>> + pr_err("Failed to read master 1 ret=%d\n", ret);
>> + return;
>> + }
>> +
>> + pm8821_irq_read_master(chip, 1, master);
>> +
>> +done:
>> + chained_irq_exit(irq_chip, desc);
>> +}
>> +
>> static void pm8xxx_irq_mask_ack(struct irq_data *d)
>> {
>> struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> @@ -254,13 +394,15 @@ static int pm8xxx_irq_get_irqchip_state(struct irq_data *d,
>> irq_bit = pmirq % 8;
>>
>> spin_lock(&chip->pm_irq_lock);
>> - rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, block);
>> + rc = regmap_write(chip->regmap, chip->irq_reg_base +
>> + SSBI_REG_ADDR_IRQ_BLK_SEL, block);
>> if (rc) {
>> pr_err("Failed Selecting Block %d rc=%d\n", block, rc);
>> goto bail;
>> }
>>
>> - rc = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_RT_STATUS, &bits);
>> + rc = regmap_read(chip->regmap, chip->irq_reg_base +
>> + SSBI_REG_ADDR_IRQ_RT_STATUS, &bits);
>> if (rc) {
>> pr_err("Failed Reading Status rc=%d\n", rc);
>> goto bail;
>> @@ -299,6 +441,151 @@ static const struct irq_domain_ops pm8xxx_irq_domain_ops = {
>> .map = pm8xxx_irq_domain_map,
>> };
>>
>> +static void pm8821_irq_mask_ack(struct irq_data *d)
>> +{
>> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> + unsigned int base, pmirq = irqd_to_hwirq(d);
>> + u8 block, master;
>> + int irq_bit, rc;
>> +
>> + block = pmirq / 8;
>> + master = block / PM8821_BLOCKS_PER_MASTER;
>> + irq_bit = pmirq % 8;
>> + block %= PM8821_BLOCKS_PER_MASTER;
>
> You can deobfuscate this somewhat by instead of testing for !master
> below you just do:
>
> if (block < PM8821_BLOCKS_PER_MASTER) {
> base =
> } else {
> base =
> block -= PM8821_BLOCKS_PER_MASTER;
> }
>
Done some cleanup in register defines which avoids this totally.
>> +
>> + if (!master)
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
>> + else
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
>> +
>> + spin_lock(&chip->pm_irq_lock);
>
> The irqchip code grabs a lock on the irq_desc, so this can't race with
> unmask - and the regmap_update_bits() is internally protecting the
> read/write cycle.
>
> So you shouldn't need to lock around this section.
>
Yep.
>> + rc = regmap_update_bits(chip->regmap,
>> + base + PM8821_IRQ_MASK_REG_OFFSET + block,
>> + BIT(irq_bit), BIT(irq_bit));
>> +
>> + if (rc) {
>> + pr_err("Failed to read/write mask IRQ:%d rc=%d\n", pmirq, rc);
>> + goto fail;
>> + }
>> +
>> + rc = regmap_update_bits(chip->regmap,
>> + base + PM8821_IRQ_CLEAR_OFFSET + block,
>> + BIT(irq_bit), BIT(irq_bit));
>> +
>> + if (rc) {
>> + pr_err("Failed to read/write IT_CLEAR IRQ:%d rc=%d\n",
>> + pmirq, rc);
>> + }
>> +
>> +fail:
>> + spin_unlock(&chip->pm_irq_lock);
>> +}
>> +
>> +static void pm8821_irq_unmask(struct irq_data *d)
>> +{
>> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> + unsigned int base, pmirq = irqd_to_hwirq(d);
>> + int irq_bit, rc;
>> + u8 block, master;
>> +
>> + block = pmirq / 8;
>> + master = block / PM8821_BLOCKS_PER_MASTER;
>> + irq_bit = pmirq % 8;
>> + block %= PM8821_BLOCKS_PER_MASTER;
>
> As mask().
>
>> +
>> + if (!master)
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
>> + else
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
>> +
>> + spin_lock(&chip->pm_irq_lock);
>
> As mask().
>
>> +
>> + rc = regmap_update_bits(chip->regmap,
>> + base + PM8821_IRQ_MASK_REG_OFFSET + block,
>> + BIT(irq_bit), ~BIT(irq_bit));
>> +
>> + if (rc)
>> + pr_err("Failed to read/write unmask IRQ:%d rc=%d\n", pmirq, rc);
>> +
>> + spin_unlock(&chip->pm_irq_lock);
>> +}
>> +
>> +static int pm8821_irq_set_type(struct irq_data *d, unsigned int flow_type)
>> +{
>> +
>> + /*
>> + * PM8821 IRQ controller does not have explicit software support for
>> + * IRQ flow type.
>> + */
>
> Is returning "success" here the right thing to do? Shouldn't we just
> omit the function? Or did you perhaps hit some clients that wouldn't
> deal with that?
>
Will remove this totally.
>> + return 0;
>> +}
>> +
>> +static int pm8821_irq_get_irqchip_state(struct irq_data *d,
>> + enum irqchip_irq_state which,
>> + bool *state)
>> +{
>> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> + int pmirq, rc;
>> + u8 block, irq_bit, master;
>> + unsigned int bits;
>> + unsigned int base;
>> + unsigned long flags;
>> +
>> + pmirq = irqd_to_hwirq(d);
>> +
>> + block = pmirq / 8;
>> + master = block / PM8821_BLOCKS_PER_MASTER;
>> + irq_bit = pmirq % 8;
>> + block %= PM8821_BLOCKS_PER_MASTER;
>> +
>
> Simplify as in mask().
taken care by new register defines.
>
>> + if (!master)
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
>> + else
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
>> +
>> + spin_lock_irqsave(&chip->pm_irq_lock, flags);
>
> No need to lock here as we're just reading a single register.
>
yep done.
>> +
>> + rc = regmap_read(chip->regmap,
>> + base + PM8821_IRQ_RT_STATUS_OFFSET + block, &bits);
>> + if (rc) {
>> + pr_err("Failed Reading Status rc=%d\n", rc);
>> + goto bail_out;
>> + }
>> +
>> + *state = !!(bits & BIT(irq_bit));
>> +
>> +bail_out:
>> + spin_unlock_irqrestore(&chip->pm_irq_lock, flags);
>> +
>> + return rc;
>> +}
>> +
>> +static struct irq_chip pm8821_irq_chip = {
>> + .name = "pm8821",
>> + .irq_mask_ack = pm8821_irq_mask_ack,
>> + .irq_unmask = pm8821_irq_unmask,
>> + .irq_set_type = pm8821_irq_set_type,
>> + .irq_get_irqchip_state = pm8821_irq_get_irqchip_state,
>> + .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
>> +};
>> +
>
> Regards,
> Bjorn
>
^ permalink raw reply
* [PATCH 1/2] mfd: pm8921: add support to pm8821
From: Srinivas Kandagatla @ 2016-11-14 17:33 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161108190751.GO25787@tuxbot>
Thanks Bjorn for review comments.
On 08/11/16 19:07, Bjorn Andersson wrote:
> On Tue 08 Nov 08:29 PST 2016, Srinivas Kandagatla wrote:
>
>> This patch adds support to PM8821 PMIC and interrupt support.
>> PM8821 is companion device that supplements primary PMIC PM8921 IC.
>>
>
> Linus Walleij has a patch out for renaming a lot of things in this file,
> so we should probably make sure that lands and then rebase this ontop.
>
Yep, Will rebase on top of it.
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>> Tested this patch for MPP and IRQ functionality on IFC6410 and SD600 EVAL
>> board with mpps PM8821 and PM8921.
>>
>> .../devicetree/bindings/mfd/qcom-pm8xxx.txt | 1 +
>> drivers/mfd/pm8921-core.c | 368 +++++++++++++++++++--
>> 2 files changed, 340 insertions(+), 29 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt b/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
>> index 37a088f..8f1b4ec 100644
>> --- a/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
>> +++ b/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
>> @@ -11,6 +11,7 @@ voltages and other various functionality to Qualcomm SoCs.
>> Definition: must be one of:
>> "qcom,pm8058"
>> "qcom,pm8921"
>> + "qcom,pm8821"
>>
>> - #address-cells:
>> Usage: required
>> diff --git a/drivers/mfd/pm8921-core.c b/drivers/mfd/pm8921-core.c
>> index 0e3a2ea..28c2470 100644
>> --- a/drivers/mfd/pm8921-core.c
>> +++ b/drivers/mfd/pm8921-core.c
>> @@ -28,16 +28,26 @@
>> #include <linux/mfd/core.h>
>>
>> #define SSBI_REG_ADDR_IRQ_BASE 0x1BB
>> -
>> -#define SSBI_REG_ADDR_IRQ_ROOT (SSBI_REG_ADDR_IRQ_BASE + 0)
>> -#define SSBI_REG_ADDR_IRQ_M_STATUS1 (SSBI_REG_ADDR_IRQ_BASE + 1)
>> -#define SSBI_REG_ADDR_IRQ_M_STATUS2 (SSBI_REG_ADDR_IRQ_BASE + 2)
>> -#define SSBI_REG_ADDR_IRQ_M_STATUS3 (SSBI_REG_ADDR_IRQ_BASE + 3)
>> -#define SSBI_REG_ADDR_IRQ_M_STATUS4 (SSBI_REG_ADDR_IRQ_BASE + 4)
>> -#define SSBI_REG_ADDR_IRQ_BLK_SEL (SSBI_REG_ADDR_IRQ_BASE + 5)
>> -#define SSBI_REG_ADDR_IRQ_IT_STATUS (SSBI_REG_ADDR_IRQ_BASE + 6)
>> -#define SSBI_REG_ADDR_IRQ_CONFIG (SSBI_REG_ADDR_IRQ_BASE + 7)
>> -#define SSBI_REG_ADDR_IRQ_RT_STATUS (SSBI_REG_ADDR_IRQ_BASE + 8)
>
> Keep these (per argumentation that follows), but try to name them
> appropriately.
>
Yes, I agree, I will address all the comments related to register
defines in next version.
...
>
>>
>> #define PM_IRQF_LVL_SEL 0x01 /* level select */
>> #define PM_IRQF_MASK_FE 0x02 /* mask falling edge */
>> @@ -54,30 +64,41 @@
>> #define REG_HWREV_2 0x0E8 /* PMIC4 revision 2 */
>>
>> #define PM8921_NR_IRQS 256
>> +#define PM8821_NR_IRQS 112
>>
>> struct pm_irq_chip {
>> struct regmap *regmap;
>> spinlock_t pm_irq_lock;
>> struct irq_domain *irqdomain;
>> + unsigned int irq_reg_base;
>> unsigned int num_irqs;
>> unsigned int num_blocks;
>> unsigned int num_masters;
>> u8 config[0];
>> };
>>
>> +struct pm8xxx_data {
>> + int num_irqs;
>> + unsigned int irq_reg_base;
>
> As far as I can see this is always SSBI_PM8821_REG_ADDR_IRQ_BASE in the
> 8821 functions and SSBI_REG_ADDR_IRQ_BASE in the pm8xxx functions. If
> you have disjunct code paths I think it's better to not obscure this
> with a variable.
>
> Try renaming the constants appropriately instead. This also has the
> benefit of reducing the size of the patch slightly.
>
Yep, will remove reg_base variable.
>>
...
>>
>> +static int pm8821_read_master_irq(const struct pm_irq_chip *chip,
>> + int m, unsigned int *master)
>> +{
>
> I think you should inline this, as you already have the calls unrolled
> in pm8821_irq_handler().
We can just call regmap_read directly from the caller function, and get
rid of this function all together.
>
>> + unsigned int base;
>> +
>> + if (!m)
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
>> + else
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
>> +
>> + return regmap_read(chip->regmap, base, master);
>> +}
>> +
>> +static int pm8821_read_block_irq(struct pm_irq_chip *chip, int master,
>> + u8 block, unsigned int *bits)
>> +{
>> + int rc;
>> +
>> + unsigned int base;
>
> Odd empty line between rc and base. (And btw, sorting your local
> variables in descending length make things pretty).
Yep, will fix it in next version.
>
>> +
>> + if (!master)
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
>> + else
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
>> +
>> + spin_lock(&chip->pm_irq_lock);
>
> The reason why this is done under a lock in the other case is because
> the status register is paged, so you shouldn't need it here.
>
Thanks for the info, will remove it.
> With this updated I think you can favorably inline this into
> pm8821_irq_block_handler().
>
>> +
>> + rc = regmap_read(chip->regmap, base + block, bits);
>> + if (rc)
>> + pr_err("Failed Reading Status rc=%d\n", rc);
>> +
>> + spin_unlock(&chip->pm_irq_lock);
>> + return rc;
>> +}
>> +
>> +static int pm8821_irq_block_handler(struct pm_irq_chip *chip,
>> + int master_number, int block)
>> +{
>> + int pmirq, irq, i, ret;
>> + unsigned int bits;
>> +
>> + ret = pm8821_read_block_irq(chip, master_number, block, &bits);
>> + if (ret) {
>> + pr_err("Failed reading %d block ret=%d", block, ret);
>> + return ret;
>> + }
>> + if (!bits) {
>> + pr_err("block bit set in master but no irqs: %d", block);
>> + return 0;
>> + }
>> +
>> + /* Convert block offset to global block number */
>> + block += (master_number * PM8821_BLOCKS_PER_MASTER) - 1;
>
> So this is block -= 1 for master 0 and block += 6 for master 1, is the
> latter correct?
>
Yes, both of them are correct.
for master 0 which has block numbers from 1-7 should translate to 0-6 in
linear space.
for master 1 which has block numbers from 1-7 should translate to 7-13
in linear space.
so for master0 it is -=1 and and for master1 it is +=6 seems correct.
>> +
>> + /* Check IRQ bits */
>> + for (i = 0; i < 8; i++) {
>> + if (bits & BIT(i)) {
>> + pmirq = block * 8 + i;
>> + irq = irq_find_mapping(chip->irqdomain, pmirq);
>> + generic_handle_irq(irq);
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int pm8821_irq_read_master(struct pm_irq_chip *chip,
>> + int master_number, u8 master_val)
>
> This isn't so much a matter of "reading master X" as "handle master X".
>
Agreed, it would be more consistent with pm8xxx too.
> Also, you don't care about the return value, so no need to return one...
>
Yep will fix it.
>> +{
>> + int ret = 0;
>> + int block;
>> +
>> + for (block = 1; block < 8; block++) {
>> + if (master_val & BIT(block)) {
>> + ret |= pm8821_irq_block_handler(chip,
>> + master_number, block);
>> + }
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static void pm8821_irq_handler(struct irq_desc *desc)
>> +{
>> + struct pm_irq_chip *chip = irq_desc_get_handler_data(desc);
>> + struct irq_chip *irq_chip = irq_desc_get_chip(desc);
>> + int ret;
>> + unsigned int master;
>> +
>> + chained_irq_enter(irq_chip, desc);
>> + /* check master 0 */
>> + ret = pm8821_read_master_irq(chip, 0, &master);
>> + if (ret) {
>> + pr_err("Failed to re:Qad master 0 ret=%d\n", ret);
>> + return;
>> + }
>> +
>> + if (master & ~PM8821_IRQ_MASTER1_SET)
>
> Rather than having a define for MASTER1_SET use BIT(0) here and write a
> comment like:
>
Yep, I will add some comments in this area.
> "bits 1 through 7 marks the first 7 blocks"
>
>> + pm8821_irq_read_master(chip, 0, master);
>> +
>
> and then
>
> "bit 0 is set if second master contains any bits"
>
> Or just skip this optimization and check the two masters unconditionally
> in a loop.
>
>> + /* check master 1 */
>> + if (!(master & PM8821_IRQ_MASTER1_SET))
>> + goto done;
>> +
>> + ret = pm8821_read_master_irq(chip, 1, &master);
>> + if (ret) {
>> + pr_err("Failed to read master 1 ret=%d\n", ret);
>> + return;
>> + }
>> +
>> + pm8821_irq_read_master(chip, 1, master);
>> +
>> +done:
>> + chained_irq_exit(irq_chip, desc);
>> +}
>> +
>> static void pm8xxx_irq_mask_ack(struct irq_data *d)
>> {
>> struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> @@ -254,13 +394,15 @@ static int pm8xxx_irq_get_irqchip_state(struct irq_data *d,
>> irq_bit = pmirq % 8;
>>
>> spin_lock(&chip->pm_irq_lock);
>> - rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, block);
>> + rc = regmap_write(chip->regmap, chip->irq_reg_base +
>> + SSBI_REG_ADDR_IRQ_BLK_SEL, block);
>> if (rc) {
>> pr_err("Failed Selecting Block %d rc=%d\n", block, rc);
>> goto bail;
>> }
>>
>> - rc = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_RT_STATUS, &bits);
>> + rc = regmap_read(chip->regmap, chip->irq_reg_base +
>> + SSBI_REG_ADDR_IRQ_RT_STATUS, &bits);
>> if (rc) {
>> pr_err("Failed Reading Status rc=%d\n", rc);
>> goto bail;
>> @@ -299,6 +441,151 @@ static const struct irq_domain_ops pm8xxx_irq_domain_ops = {
>> .map = pm8xxx_irq_domain_map,
>> };
>>
>> +static void pm8821_irq_mask_ack(struct irq_data *d)
>> +{
>> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> + unsigned int base, pmirq = irqd_to_hwirq(d);
>> + u8 block, master;
>> + int irq_bit, rc;
>> +
>> + block = pmirq / 8;
>> + master = block / PM8821_BLOCKS_PER_MASTER;
>> + irq_bit = pmirq % 8;
>> + block %= PM8821_BLOCKS_PER_MASTER;
>
> You can deobfuscate this somewhat by instead of testing for !master
> below you just do:
>
> if (block < PM8821_BLOCKS_PER_MASTER) {
> base =
> } else {
> base =
> block -= PM8821_BLOCKS_PER_MASTER;
> }
>
Done some cleanup in register defines which avoids this totally.
>> +
>> + if (!master)
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
>> + else
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
>> +
>> + spin_lock(&chip->pm_irq_lock);
>
> The irqchip code grabs a lock on the irq_desc, so this can't race with
> unmask - and the regmap_update_bits() is internally protecting the
> read/write cycle.
>
> So you shouldn't need to lock around this section.
>
Yep.
>> + rc = regmap_update_bits(chip->regmap,
>> + base + PM8821_IRQ_MASK_REG_OFFSET + block,
>> + BIT(irq_bit), BIT(irq_bit));
>> +
>> + if (rc) {
>> + pr_err("Failed to read/write mask IRQ:%d rc=%d\n", pmirq, rc);
>> + goto fail;
>> + }
>> +
>> + rc = regmap_update_bits(chip->regmap,
>> + base + PM8821_IRQ_CLEAR_OFFSET + block,
>> + BIT(irq_bit), BIT(irq_bit));
>> +
>> + if (rc) {
>> + pr_err("Failed to read/write IT_CLEAR IRQ:%d rc=%d\n",
>> + pmirq, rc);
>> + }
>> +
>> +fail:
>> + spin_unlock(&chip->pm_irq_lock);
>> +}
>> +
>> +static void pm8821_irq_unmask(struct irq_data *d)
>> +{
>> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> + unsigned int base, pmirq = irqd_to_hwirq(d);
>> + int irq_bit, rc;
>> + u8 block, master;
>> +
>> + block = pmirq / 8;
>> + master = block / PM8821_BLOCKS_PER_MASTER;
>> + irq_bit = pmirq % 8;
>> + block %= PM8821_BLOCKS_PER_MASTER;
>
> As mask().
>
>> +
>> + if (!master)
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
>> + else
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
>> +
>> + spin_lock(&chip->pm_irq_lock);
>
> As mask().
>
>> +
>> + rc = regmap_update_bits(chip->regmap,
>> + base + PM8821_IRQ_MASK_REG_OFFSET + block,
>> + BIT(irq_bit), ~BIT(irq_bit));
>> +
>> + if (rc)
>> + pr_err("Failed to read/write unmask IRQ:%d rc=%d\n", pmirq, rc);
>> +
>> + spin_unlock(&chip->pm_irq_lock);
>> +}
>> +
>> +static int pm8821_irq_set_type(struct irq_data *d, unsigned int flow_type)
>> +{
>> +
>> + /*
>> + * PM8821 IRQ controller does not have explicit software support for
>> + * IRQ flow type.
>> + */
>
> Is returning "success" here the right thing to do? Shouldn't we just
> omit the function? Or did you perhaps hit some clients that wouldn't
> deal with that?
>
Will remove this totally.
>> + return 0;
>> +}
>> +
>> +static int pm8821_irq_get_irqchip_state(struct irq_data *d,
>> + enum irqchip_irq_state which,
>> + bool *state)
>> +{
>> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> + int pmirq, rc;
>> + u8 block, irq_bit, master;
>> + unsigned int bits;
>> + unsigned int base;
>> + unsigned long flags;
>> +
>> + pmirq = irqd_to_hwirq(d);
>> +
>> + block = pmirq / 8;
>> + master = block / PM8821_BLOCKS_PER_MASTER;
>> + irq_bit = pmirq % 8;
>> + block %= PM8821_BLOCKS_PER_MASTER;
>> +
>
> Simplify as in mask().
taken care by new register defines.
>
>> + if (!master)
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
>> + else
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
>> +
>> + spin_lock_irqsave(&chip->pm_irq_lock, flags);
>
> No need to lock here as we're just reading a single register.
>
yep done.
>> +
>> + rc = regmap_read(chip->regmap,
>> + base + PM8821_IRQ_RT_STATUS_OFFSET + block, &bits);
>> + if (rc) {
>> + pr_err("Failed Reading Status rc=%d\n", rc);
>> + goto bail_out;
>> + }
>> +
>> + *state = !!(bits & BIT(irq_bit));
>> +
>> +bail_out:
>> + spin_unlock_irqrestore(&chip->pm_irq_lock, flags);
>> +
>> + return rc;
>> +}
>> +
>> +static struct irq_chip pm8821_irq_chip = {
>> + .name = "pm8821",
>> + .irq_mask_ack = pm8821_irq_mask_ack,
>> + .irq_unmask = pm8821_irq_unmask,
>> + .irq_set_type = pm8821_irq_set_type,
>> + .irq_get_irqchip_state = pm8821_irq_get_irqchip_state,
>> + .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
>> +};
>> +
>
> Regards,
> Bjorn
>
^ permalink raw reply
* Re: [PATCH 1/2] mfd: pm8921: add support to pm8821
From: Srinivas Kandagatla @ 2016-11-14 17:33 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Lee Jones, Rob Herring, Andy Gross,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
linux-soc-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20161108190751.GO25787@tuxbot>
Thanks Bjorn for review comments.
On 08/11/16 19:07, Bjorn Andersson wrote:
> On Tue 08 Nov 08:29 PST 2016, Srinivas Kandagatla wrote:
>
>> This patch adds support to PM8821 PMIC and interrupt support.
>> PM8821 is companion device that supplements primary PMIC PM8921 IC.
>>
>
> Linus Walleij has a patch out for renaming a lot of things in this file,
> so we should probably make sure that lands and then rebase this ontop.
>
Yep, Will rebase on top of it.
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>> Tested this patch for MPP and IRQ functionality on IFC6410 and SD600 EVAL
>> board with mpps PM8821 and PM8921.
>>
>> .../devicetree/bindings/mfd/qcom-pm8xxx.txt | 1 +
>> drivers/mfd/pm8921-core.c | 368 +++++++++++++++++++--
>> 2 files changed, 340 insertions(+), 29 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt b/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
>> index 37a088f..8f1b4ec 100644
>> --- a/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
>> +++ b/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
>> @@ -11,6 +11,7 @@ voltages and other various functionality to Qualcomm SoCs.
>> Definition: must be one of:
>> "qcom,pm8058"
>> "qcom,pm8921"
>> + "qcom,pm8821"
>>
>> - #address-cells:
>> Usage: required
>> diff --git a/drivers/mfd/pm8921-core.c b/drivers/mfd/pm8921-core.c
>> index 0e3a2ea..28c2470 100644
>> --- a/drivers/mfd/pm8921-core.c
>> +++ b/drivers/mfd/pm8921-core.c
>> @@ -28,16 +28,26 @@
>> #include <linux/mfd/core.h>
>>
>> #define SSBI_REG_ADDR_IRQ_BASE 0x1BB
>> -
>> -#define SSBI_REG_ADDR_IRQ_ROOT (SSBI_REG_ADDR_IRQ_BASE + 0)
>> -#define SSBI_REG_ADDR_IRQ_M_STATUS1 (SSBI_REG_ADDR_IRQ_BASE + 1)
>> -#define SSBI_REG_ADDR_IRQ_M_STATUS2 (SSBI_REG_ADDR_IRQ_BASE + 2)
>> -#define SSBI_REG_ADDR_IRQ_M_STATUS3 (SSBI_REG_ADDR_IRQ_BASE + 3)
>> -#define SSBI_REG_ADDR_IRQ_M_STATUS4 (SSBI_REG_ADDR_IRQ_BASE + 4)
>> -#define SSBI_REG_ADDR_IRQ_BLK_SEL (SSBI_REG_ADDR_IRQ_BASE + 5)
>> -#define SSBI_REG_ADDR_IRQ_IT_STATUS (SSBI_REG_ADDR_IRQ_BASE + 6)
>> -#define SSBI_REG_ADDR_IRQ_CONFIG (SSBI_REG_ADDR_IRQ_BASE + 7)
>> -#define SSBI_REG_ADDR_IRQ_RT_STATUS (SSBI_REG_ADDR_IRQ_BASE + 8)
>
> Keep these (per argumentation that follows), but try to name them
> appropriately.
>
Yes, I agree, I will address all the comments related to register
defines in next version.
...
>
>>
>> #define PM_IRQF_LVL_SEL 0x01 /* level select */
>> #define PM_IRQF_MASK_FE 0x02 /* mask falling edge */
>> @@ -54,30 +64,41 @@
>> #define REG_HWREV_2 0x0E8 /* PMIC4 revision 2 */
>>
>> #define PM8921_NR_IRQS 256
>> +#define PM8821_NR_IRQS 112
>>
>> struct pm_irq_chip {
>> struct regmap *regmap;
>> spinlock_t pm_irq_lock;
>> struct irq_domain *irqdomain;
>> + unsigned int irq_reg_base;
>> unsigned int num_irqs;
>> unsigned int num_blocks;
>> unsigned int num_masters;
>> u8 config[0];
>> };
>>
>> +struct pm8xxx_data {
>> + int num_irqs;
>> + unsigned int irq_reg_base;
>
> As far as I can see this is always SSBI_PM8821_REG_ADDR_IRQ_BASE in the
> 8821 functions and SSBI_REG_ADDR_IRQ_BASE in the pm8xxx functions. If
> you have disjunct code paths I think it's better to not obscure this
> with a variable.
>
> Try renaming the constants appropriately instead. This also has the
> benefit of reducing the size of the patch slightly.
>
Yep, will remove reg_base variable.
>>
...
>>
>> +static int pm8821_read_master_irq(const struct pm_irq_chip *chip,
>> + int m, unsigned int *master)
>> +{
>
> I think you should inline this, as you already have the calls unrolled
> in pm8821_irq_handler().
We can just call regmap_read directly from the caller function, and get
rid of this function all together.
>
>> + unsigned int base;
>> +
>> + if (!m)
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
>> + else
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
>> +
>> + return regmap_read(chip->regmap, base, master);
>> +}
>> +
>> +static int pm8821_read_block_irq(struct pm_irq_chip *chip, int master,
>> + u8 block, unsigned int *bits)
>> +{
>> + int rc;
>> +
>> + unsigned int base;
>
> Odd empty line between rc and base. (And btw, sorting your local
> variables in descending length make things pretty).
Yep, will fix it in next version.
>
>> +
>> + if (!master)
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
>> + else
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
>> +
>> + spin_lock(&chip->pm_irq_lock);
>
> The reason why this is done under a lock in the other case is because
> the status register is paged, so you shouldn't need it here.
>
Thanks for the info, will remove it.
> With this updated I think you can favorably inline this into
> pm8821_irq_block_handler().
>
>> +
>> + rc = regmap_read(chip->regmap, base + block, bits);
>> + if (rc)
>> + pr_err("Failed Reading Status rc=%d\n", rc);
>> +
>> + spin_unlock(&chip->pm_irq_lock);
>> + return rc;
>> +}
>> +
>> +static int pm8821_irq_block_handler(struct pm_irq_chip *chip,
>> + int master_number, int block)
>> +{
>> + int pmirq, irq, i, ret;
>> + unsigned int bits;
>> +
>> + ret = pm8821_read_block_irq(chip, master_number, block, &bits);
>> + if (ret) {
>> + pr_err("Failed reading %d block ret=%d", block, ret);
>> + return ret;
>> + }
>> + if (!bits) {
>> + pr_err("block bit set in master but no irqs: %d", block);
>> + return 0;
>> + }
>> +
>> + /* Convert block offset to global block number */
>> + block += (master_number * PM8821_BLOCKS_PER_MASTER) - 1;
>
> So this is block -= 1 for master 0 and block += 6 for master 1, is the
> latter correct?
>
Yes, both of them are correct.
for master 0 which has block numbers from 1-7 should translate to 0-6 in
linear space.
for master 1 which has block numbers from 1-7 should translate to 7-13
in linear space.
so for master0 it is -=1 and and for master1 it is +=6 seems correct.
>> +
>> + /* Check IRQ bits */
>> + for (i = 0; i < 8; i++) {
>> + if (bits & BIT(i)) {
>> + pmirq = block * 8 + i;
>> + irq = irq_find_mapping(chip->irqdomain, pmirq);
>> + generic_handle_irq(irq);
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int pm8821_irq_read_master(struct pm_irq_chip *chip,
>> + int master_number, u8 master_val)
>
> This isn't so much a matter of "reading master X" as "handle master X".
>
Agreed, it would be more consistent with pm8xxx too.
> Also, you don't care about the return value, so no need to return one...
>
Yep will fix it.
>> +{
>> + int ret = 0;
>> + int block;
>> +
>> + for (block = 1; block < 8; block++) {
>> + if (master_val & BIT(block)) {
>> + ret |= pm8821_irq_block_handler(chip,
>> + master_number, block);
>> + }
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static void pm8821_irq_handler(struct irq_desc *desc)
>> +{
>> + struct pm_irq_chip *chip = irq_desc_get_handler_data(desc);
>> + struct irq_chip *irq_chip = irq_desc_get_chip(desc);
>> + int ret;
>> + unsigned int master;
>> +
>> + chained_irq_enter(irq_chip, desc);
>> + /* check master 0 */
>> + ret = pm8821_read_master_irq(chip, 0, &master);
>> + if (ret) {
>> + pr_err("Failed to re:Qad master 0 ret=%d\n", ret);
>> + return;
>> + }
>> +
>> + if (master & ~PM8821_IRQ_MASTER1_SET)
>
> Rather than having a define for MASTER1_SET use BIT(0) here and write a
> comment like:
>
Yep, I will add some comments in this area.
> "bits 1 through 7 marks the first 7 blocks"
>
>> + pm8821_irq_read_master(chip, 0, master);
>> +
>
> and then
>
> "bit 0 is set if second master contains any bits"
>
> Or just skip this optimization and check the two masters unconditionally
> in a loop.
>
>> + /* check master 1 */
>> + if (!(master & PM8821_IRQ_MASTER1_SET))
>> + goto done;
>> +
>> + ret = pm8821_read_master_irq(chip, 1, &master);
>> + if (ret) {
>> + pr_err("Failed to read master 1 ret=%d\n", ret);
>> + return;
>> + }
>> +
>> + pm8821_irq_read_master(chip, 1, master);
>> +
>> +done:
>> + chained_irq_exit(irq_chip, desc);
>> +}
>> +
>> static void pm8xxx_irq_mask_ack(struct irq_data *d)
>> {
>> struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> @@ -254,13 +394,15 @@ static int pm8xxx_irq_get_irqchip_state(struct irq_data *d,
>> irq_bit = pmirq % 8;
>>
>> spin_lock(&chip->pm_irq_lock);
>> - rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, block);
>> + rc = regmap_write(chip->regmap, chip->irq_reg_base +
>> + SSBI_REG_ADDR_IRQ_BLK_SEL, block);
>> if (rc) {
>> pr_err("Failed Selecting Block %d rc=%d\n", block, rc);
>> goto bail;
>> }
>>
>> - rc = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_RT_STATUS, &bits);
>> + rc = regmap_read(chip->regmap, chip->irq_reg_base +
>> + SSBI_REG_ADDR_IRQ_RT_STATUS, &bits);
>> if (rc) {
>> pr_err("Failed Reading Status rc=%d\n", rc);
>> goto bail;
>> @@ -299,6 +441,151 @@ static const struct irq_domain_ops pm8xxx_irq_domain_ops = {
>> .map = pm8xxx_irq_domain_map,
>> };
>>
>> +static void pm8821_irq_mask_ack(struct irq_data *d)
>> +{
>> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> + unsigned int base, pmirq = irqd_to_hwirq(d);
>> + u8 block, master;
>> + int irq_bit, rc;
>> +
>> + block = pmirq / 8;
>> + master = block / PM8821_BLOCKS_PER_MASTER;
>> + irq_bit = pmirq % 8;
>> + block %= PM8821_BLOCKS_PER_MASTER;
>
> You can deobfuscate this somewhat by instead of testing for !master
> below you just do:
>
> if (block < PM8821_BLOCKS_PER_MASTER) {
> base =
> } else {
> base =
> block -= PM8821_BLOCKS_PER_MASTER;
> }
>
Done some cleanup in register defines which avoids this totally.
>> +
>> + if (!master)
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
>> + else
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
>> +
>> + spin_lock(&chip->pm_irq_lock);
>
> The irqchip code grabs a lock on the irq_desc, so this can't race with
> unmask - and the regmap_update_bits() is internally protecting the
> read/write cycle.
>
> So you shouldn't need to lock around this section.
>
Yep.
>> + rc = regmap_update_bits(chip->regmap,
>> + base + PM8821_IRQ_MASK_REG_OFFSET + block,
>> + BIT(irq_bit), BIT(irq_bit));
>> +
>> + if (rc) {
>> + pr_err("Failed to read/write mask IRQ:%d rc=%d\n", pmirq, rc);
>> + goto fail;
>> + }
>> +
>> + rc = regmap_update_bits(chip->regmap,
>> + base + PM8821_IRQ_CLEAR_OFFSET + block,
>> + BIT(irq_bit), BIT(irq_bit));
>> +
>> + if (rc) {
>> + pr_err("Failed to read/write IT_CLEAR IRQ:%d rc=%d\n",
>> + pmirq, rc);
>> + }
>> +
>> +fail:
>> + spin_unlock(&chip->pm_irq_lock);
>> +}
>> +
>> +static void pm8821_irq_unmask(struct irq_data *d)
>> +{
>> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> + unsigned int base, pmirq = irqd_to_hwirq(d);
>> + int irq_bit, rc;
>> + u8 block, master;
>> +
>> + block = pmirq / 8;
>> + master = block / PM8821_BLOCKS_PER_MASTER;
>> + irq_bit = pmirq % 8;
>> + block %= PM8821_BLOCKS_PER_MASTER;
>
> As mask().
>
>> +
>> + if (!master)
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
>> + else
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
>> +
>> + spin_lock(&chip->pm_irq_lock);
>
> As mask().
>
>> +
>> + rc = regmap_update_bits(chip->regmap,
>> + base + PM8821_IRQ_MASK_REG_OFFSET + block,
>> + BIT(irq_bit), ~BIT(irq_bit));
>> +
>> + if (rc)
>> + pr_err("Failed to read/write unmask IRQ:%d rc=%d\n", pmirq, rc);
>> +
>> + spin_unlock(&chip->pm_irq_lock);
>> +}
>> +
>> +static int pm8821_irq_set_type(struct irq_data *d, unsigned int flow_type)
>> +{
>> +
>> + /*
>> + * PM8821 IRQ controller does not have explicit software support for
>> + * IRQ flow type.
>> + */
>
> Is returning "success" here the right thing to do? Shouldn't we just
> omit the function? Or did you perhaps hit some clients that wouldn't
> deal with that?
>
Will remove this totally.
>> + return 0;
>> +}
>> +
>> +static int pm8821_irq_get_irqchip_state(struct irq_data *d,
>> + enum irqchip_irq_state which,
>> + bool *state)
>> +{
>> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> + int pmirq, rc;
>> + u8 block, irq_bit, master;
>> + unsigned int bits;
>> + unsigned int base;
>> + unsigned long flags;
>> +
>> + pmirq = irqd_to_hwirq(d);
>> +
>> + block = pmirq / 8;
>> + master = block / PM8821_BLOCKS_PER_MASTER;
>> + irq_bit = pmirq % 8;
>> + block %= PM8821_BLOCKS_PER_MASTER;
>> +
>
> Simplify as in mask().
taken care by new register defines.
>
>> + if (!master)
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
>> + else
>> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
>> +
>> + spin_lock_irqsave(&chip->pm_irq_lock, flags);
>
> No need to lock here as we're just reading a single register.
>
yep done.
>> +
>> + rc = regmap_read(chip->regmap,
>> + base + PM8821_IRQ_RT_STATUS_OFFSET + block, &bits);
>> + if (rc) {
>> + pr_err("Failed Reading Status rc=%d\n", rc);
>> + goto bail_out;
>> + }
>> +
>> + *state = !!(bits & BIT(irq_bit));
>> +
>> +bail_out:
>> + spin_unlock_irqrestore(&chip->pm_irq_lock, flags);
>> +
>> + return rc;
>> +}
>> +
>> +static struct irq_chip pm8821_irq_chip = {
>> + .name = "pm8821",
>> + .irq_mask_ack = pm8821_irq_mask_ack,
>> + .irq_unmask = pm8821_irq_unmask,
>> + .irq_set_type = pm8821_irq_set_type,
>> + .irq_get_irqchip_state = pm8821_irq_get_irqchip_state,
>> + .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
>> +};
>> +
>
> Regards,
> Bjorn
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 0/5] net: thunderx: Miscellaneous fixes
From: David Miller @ 2016-11-14 17:32 UTC (permalink / raw)
To: mbrugger; +Cc: sunil.kovvuri, netdev, sgoutham, linux-kernel, linux-arm-kernel
In-Reply-To: <2948c47d-0f15-8153-440b-7a2c753b7251@suse.com>
From: Matthias Brugger <mbrugger@suse.com>
Date: Mon, 14 Nov 2016 13:01:25 +0100
>
>
> On 14/11/16 11:54, sunil.kovvuri@gmail.com wrote:
>> From: Sunil Goutham <sgoutham@cavium.com>
>>
>> This patchset includes fixes for incorrect LMAC credits,
>> unreliable driver statistics, memory leak upon interface
>> down e.t.c
>>
>
> Are these fixes relevant to for older kernels as well?
> If so, please add "Cc: stable@vger.kernel.org" to the Sigend-off list.
This is not appropriate for networking patches.
People instead explicitly request -stable inclusion when the
submit networking changes to me, and I queue them up for
later submission.
^ permalink raw reply
* [PATCH 0/5] net: thunderx: Miscellaneous fixes
From: David Miller @ 2016-11-14 17:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <2948c47d-0f15-8153-440b-7a2c753b7251@suse.com>
From: Matthias Brugger <mbrugger@suse.com>
Date: Mon, 14 Nov 2016 13:01:25 +0100
>
>
> On 14/11/16 11:54, sunil.kovvuri at gmail.com wrote:
>> From: Sunil Goutham <sgoutham@cavium.com>
>>
>> This patchset includes fixes for incorrect LMAC credits,
>> unreliable driver statistics, memory leak upon interface
>> down e.t.c
>>
>
> Are these fixes relevant to for older kernels as well?
> If so, please add "Cc: stable at vger.kernel.org" to the Sigend-off list.
This is not appropriate for networking patches.
People instead explicitly request -stable inclusion when the
submit networking changes to me, and I queue them up for
later submission.
^ permalink raw reply
* Re: [PATCH] arm64: dts: juno: fix cluster sleep state entry latency on all SoC versions
From: Liviu Dudau @ 2016-11-14 17:32 UTC (permalink / raw)
To: Sudeep Holla
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, Lorenzo Pieralisi,
Jon Medhurst (Tixy)
In-Reply-To: <1479137189-15378-1-git-send-email-sudeep.holla-5wv7dgnIgG8@public.gmane.org>
On Mon, Nov 14, 2016 at 03:26:29PM +0000, Sudeep Holla wrote:
> The core and the cluster sleep state entry latencies can't be same as
> cluster sleep involves more work compared to core level e.g. shared
> cache maintenance.
>
> Experiments have shown on an average about 100us more latency for the
> cluster sleep state compared to the core level sleep. This patch fixes
> the entry latency for the cluster sleep state.
>
> Fixes: 28e10a8f3a03 ("arm64: dts: juno: Add idle-states to device tree")
> Cc: Liviu Dudau <liviu.dudau-5wv7dgnIgG8@public.gmane.org>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
> Cc: "Jon Medhurst (Tixy)" <tixy-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
Looks sensible to me.
Reviewed-by: Liviu Dudau <Liviu.Dudau-5wv7dgnIgG8@public.gmane.org>
> ---
> arch/arm64/boot/dts/arm/juno-r1.dts | 2 +-
> arch/arm64/boot/dts/arm/juno-r2.dts | 2 +-
> arch/arm64/boot/dts/arm/juno.dts | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> Hi,
>
> This was found recently when I found that core sleep was chosen when
> entering suspend-to-idle state on Juno. Since the wakeup(entry+exit)
> latency matched for the both states, cpu sleep state was chosen to enter
> in suspend-to-idle.
>
> Regards,
> Sudeep
>
> diff --git a/arch/arm64/boot/dts/arm/juno-r1.dts b/arch/arm64/boot/dts/arm/juno-r1.dts
> index 3be8a3ef671c..eec37feee8fc 100644
> --- a/arch/arm64/boot/dts/arm/juno-r1.dts
> +++ b/arch/arm64/boot/dts/arm/juno-r1.dts
> @@ -76,7 +76,7 @@
> compatible = "arm,idle-state";
> arm,psci-suspend-param = <0x1010000>;
> local-timer-stop;
> - entry-latency-us = <300>;
> + entry-latency-us = <400>;
> exit-latency-us = <1200>;
> min-residency-us = <2500>;
> };
> diff --git a/arch/arm64/boot/dts/arm/juno-r2.dts b/arch/arm64/boot/dts/arm/juno-r2.dts
> index 614fc9227943..28f40ec44090 100644
> --- a/arch/arm64/boot/dts/arm/juno-r2.dts
> +++ b/arch/arm64/boot/dts/arm/juno-r2.dts
> @@ -76,7 +76,7 @@
> compatible = "arm,idle-state";
> arm,psci-suspend-param = <0x1010000>;
> local-timer-stop;
> - entry-latency-us = <300>;
> + entry-latency-us = <400>;
> exit-latency-us = <1200>;
> min-residency-us = <2500>;
> };
> diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> index 6b4135e9cfe5..ac5ceb73f45f 100644
> --- a/arch/arm64/boot/dts/arm/juno.dts
> +++ b/arch/arm64/boot/dts/arm/juno.dts
> @@ -76,7 +76,7 @@
> compatible = "arm,idle-state";
> arm,psci-suspend-param = <0x1010000>;
> local-timer-stop;
> - entry-latency-us = <300>;
> + entry-latency-us = <400>;
> exit-latency-us = <1200>;
> min-residency-us = <2500>;
> };
> --
> 2.7.4
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] arm64: dts: juno: fix cluster sleep state entry latency on all SoC versions
From: Liviu Dudau @ 2016-11-14 17:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479137189-15378-1-git-send-email-sudeep.holla@arm.com>
On Mon, Nov 14, 2016 at 03:26:29PM +0000, Sudeep Holla wrote:
> The core and the cluster sleep state entry latencies can't be same as
> cluster sleep involves more work compared to core level e.g. shared
> cache maintenance.
>
> Experiments have shown on an average about 100us more latency for the
> cluster sleep state compared to the core level sleep. This patch fixes
> the entry latency for the cluster sleep state.
>
> Fixes: 28e10a8f3a03 ("arm64: dts: juno: Add idle-states to device tree")
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: "Jon Medhurst (Tixy)" <tixy@linaro.org>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
Looks sensible to me.
Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>
> ---
> arch/arm64/boot/dts/arm/juno-r1.dts | 2 +-
> arch/arm64/boot/dts/arm/juno-r2.dts | 2 +-
> arch/arm64/boot/dts/arm/juno.dts | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> Hi,
>
> This was found recently when I found that core sleep was chosen when
> entering suspend-to-idle state on Juno. Since the wakeup(entry+exit)
> latency matched for the both states, cpu sleep state was chosen to enter
> in suspend-to-idle.
>
> Regards,
> Sudeep
>
> diff --git a/arch/arm64/boot/dts/arm/juno-r1.dts b/arch/arm64/boot/dts/arm/juno-r1.dts
> index 3be8a3ef671c..eec37feee8fc 100644
> --- a/arch/arm64/boot/dts/arm/juno-r1.dts
> +++ b/arch/arm64/boot/dts/arm/juno-r1.dts
> @@ -76,7 +76,7 @@
> compatible = "arm,idle-state";
> arm,psci-suspend-param = <0x1010000>;
> local-timer-stop;
> - entry-latency-us = <300>;
> + entry-latency-us = <400>;
> exit-latency-us = <1200>;
> min-residency-us = <2500>;
> };
> diff --git a/arch/arm64/boot/dts/arm/juno-r2.dts b/arch/arm64/boot/dts/arm/juno-r2.dts
> index 614fc9227943..28f40ec44090 100644
> --- a/arch/arm64/boot/dts/arm/juno-r2.dts
> +++ b/arch/arm64/boot/dts/arm/juno-r2.dts
> @@ -76,7 +76,7 @@
> compatible = "arm,idle-state";
> arm,psci-suspend-param = <0x1010000>;
> local-timer-stop;
> - entry-latency-us = <300>;
> + entry-latency-us = <400>;
> exit-latency-us = <1200>;
> min-residency-us = <2500>;
> };
> diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> index 6b4135e9cfe5..ac5ceb73f45f 100644
> --- a/arch/arm64/boot/dts/arm/juno.dts
> +++ b/arch/arm64/boot/dts/arm/juno.dts
> @@ -76,7 +76,7 @@
> compatible = "arm,idle-state";
> arm,psci-suspend-param = <0x1010000>;
> local-timer-stop;
> - entry-latency-us = <300>;
> + entry-latency-us = <400>;
> exit-latency-us = <1200>;
> min-residency-us = <2500>;
> };
> --
> 2.7.4
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
?\_(?)_/?
^ permalink raw reply
* [PATCH 2/2] ARM: dts: apq8064: add support to pm8821
From: Srinivas Kandagatla @ 2016-11-14 17:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161108191343.GP25787@tuxbot>
Thanks Bjorn for review comments.
On 08/11/16 19:13, Bjorn Andersson wrote:
> On Tue 08 Nov 08:29 PST 2016, Srinivas Kandagatla wrote:
>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>> arch/arm/boot/dts/qcom-apq8064.dtsi | 28 ++++++++++++++++++++++++++++
>> 1 file changed, 28 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi
>> index 1dbe697..fde006c 100644
>> --- a/arch/arm/boot/dts/qcom-apq8064.dtsi
>> +++ b/arch/arm/boot/dts/qcom-apq8064.dtsi
>> @@ -627,6 +627,34 @@
>> clock-names = "core";
>> };
>>
>> + qcom,ssbi at c00000 {
>
> No "qcom," in the node name.
Will fix it in next version, I agree with rest of the comments too.
All of them will be fixed in next version.
>
>> + compatible = "qcom,ssbi";
>> + reg = <0x00c00000 0x1000>;
>> + qcom,controller-type = "pmic-arbiter";
>> +
>> + pmicintc2: pmic at 1 {
>
> I think we should follow Linus' lead and label this "pm8821".
>
>> + compatible = "qcom,pm8821";
>> + interrupt-parent = <&tlmm_pinmux>;
>> + interrupts = <76 8>;
>
> Please spell out IRQ_TYPE_LEVEL_LOW.
>
> And interrupts-extended = <&tlmm_pinmux 76 IRQ_TYPE_LEVEL_LOW> combines
> the two lines nicely.
>
>> + #interrupt-cells = <2>;
>> + interrupt-controller;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + pm8821_mpps: mpps at 50 {
>> +
>
> Extra newline.
>
>> + compatible = "qcom,pm8821-mpp", "qcom,ssbi-mpp";
>> + reg = <0x50>;
>> +
>> + interrupts = <24 1>, <25 1>, <26 1>,
>> + <27 1>;
>
> I think these should be IRQ_TYPE_NONE per the discussion on how to share
> interrupts between the gpio/mpp driver and other clients.
>
> On the other hand, per the pm8821 driver we only support LEVEL_LOW
> (high?).
>
>> +
>> + gpio-controller;
>> + #gpio-cells = <2>;
>> + };
>> + };
>> + };
>> +
>
> Regards,
> Bjorn
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.