Re: [PATCH 5/8] gpio: omap: convert gpio irq functions to use GPIO offset
On 03/20/2015 01:03 AM, Tony Lindgren wrote:
* grygorii.stras...@linaro.org [150319 10:26]:
From: Grygorii Strashko
Convert GPIO IRQ functions to use GPIO offset instead of system
GPIO numbers. This allows to drop unneeded conversations between
system GPIO <-> GPIO offset which are done in many places and
many times.
It is safe to do now because:
- gpiolib always passes GPIO offset to GPIO controller
- OMAP GPIO driver converted to use IRQ domain, so
struct irq_data->hwirq contains GPIO offset
This is preparation step before removing:
#define GPIO_INDEX(bank, gpio)
#define GPIO_BIT(bank, gpio)
int omap_irq_to_gpio()
...
static void omap_gpio_unmask_irq(struct irq_data *d)
{
struct gpio_bank *bank = omap_irq_data_get_bank(d);
- unsigned int gpio = omap_irq_to_gpio(bank, d->hwirq);
+ unsigned offset = d->hwirq;
unsigned int irq_mask = GPIO_BIT(bank, gpio);
u32 trigger = irqd_get_trigger_type(d);
unsigned long flags;
This series up to this patch produces a build error that
would break git bisect:
drivers/gpio/gpio-omap.c: In function ‘omap_gpio_unmask_irq’:
drivers/gpio/gpio-omap.c:866:37: error: ‘gpio’ undeclared (first use in this
function)
unsigned int irq_mask = GPIO_BIT(bank, gpio);
Oh. Thanks for catching that :) - splitting/rebasing issue.
--
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/8] gpio: omap: cleanup: get rid of system GPIO <-> GPIO offset converseations
On 03/20/2015 01:00 AM, Tony Lindgren wrote: > * grygorii.stras...@linaro.org [150319 10:26]: >> From: Grygorii Strashko >> >> Now in TI OMAP GPIO driver there are a lot of places where >> System GPIO number calculated and then converted to GPIO offset. >> What is worse is that in many place such conversation performed twice >> or even three times. But actually, we don't need to do that at all, because >> - gpiolib always passes GPIO offset to GPIO controller >> - OMAP GPIO driver converted to use IRQ domain, so >>struct irq_data->hwirq contains GPIO offset >> >> Hence, it is safe to convert all GPIO OMAP functions to use GPIO >> offset instead of system GPIO numbers. Also, this allows to remove >> unneeded conversations routines >> #define GPIO_INDEX(bank, gpio) >> #define GPIO_BIT(bank, gpio) >> int omap_irq_to_gpio() > > Right on! This is excellent news. I've tested this set on top of -rc4 > plus the patch mentioned below, and I'm not seeing regressions on > the platforms I tested with. Wake up to smsc911x ping with off-idle > still works on omap3. > > I only briefly tested on omap1 (osk5912), so I'like to wait for > Aaro's ack before this gets merged. > > I found one build error, other than that, for the whole series > please feel free to add: > > Tested-by: Tony Lindgren Thanks Tony. Yep. I'll wait for news from Aaro then will re-send if ok. -- regards, -grygorii -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 4/5] i2c: davinci: use bus recovery infrastructure
Hi,
On 03/18/2015 10:31 PM, Wolfram Sang wrote:
> Hi,
>
> so, the bus recovery patches look fine to me in general.
>
> It is only this one question left which I always had with bus recovery.
> Maybe you guys can join me thinking about it.
Ok. Thanks and sorry for delayed reply - missed your e-mail :(
I'll resend them next week.
>
>> @@ -376,8 +366,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
>> i2c_msg *msg, int stop)
>>dev->adapter.timeout);
>> if (r == 0) {
>> dev_err(dev->dev, "controller timed out\n");
>> -davinci_i2c_recover_bus(dev);
>> -i2c_davinci_init(dev);
>> +i2c_recover_bus(adap);
>> dev->buf_len = 0;
>> return -ETIMEDOUT;
>
> The I2C specs say in 3.1.16 that the recovery procedure should be used
> when SDA is stuck low. So, I do wonder if we should apply the recovery
> after a timeout. Stuck SDA might be one reason for timeout, but there
> may be others...
This is ancient code. And regarding your question -
Might be it would be reasonable to add call of
i2c_davinci_wait_bus_not_busy() at the end of i2c_davinci_xfer()?
This way we will wait for Bus Free before performing recovery.
Of course, i2c_davinci_wait_bus_not_busy() has to be fixed first
as proposed by Alexander Sverdlin here:
https://patchwork.ozlabs.org/patch/448994/.
--
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFT OMAP1 PATCH 7/8] gpio: omap: get rid of omap_irq_to_gpio()
On 03/20/2015 08:56 PM, Javier Martinez Canillas wrote: > Hello Grygorii, > > On Thu, Mar 19, 2015 at 6:25 PM, wrote: >> From: Grygorii Strashko >> >> Now OMAP GPIO driver prepared for omap_irq_to_gpio() removing. >> Do it ;) >> >> Signed-off-by: Grygorii Strashko > > I don't have access to OMAP1 boards to test but Tony did it o an > osk5912 and Aaro can confirm that it doesn't cause regressions on > other OMAP1 boards. Thanks. But could you clarify one point to me please? "Aaro can confirm that it doesn't cause regressions on other OMAP1 boards" - Does It mean I can add his Tested-by? Or I still have to wait for his direct reply? Just to avoid any confusions ;) > Acked-by: Javier Martinez Canillas > -- regards, -grygorii -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 4/5] i2c: davinci: use bus recovery infrastructure
On 04/03/2015 11:18 PM, Wolfram Sang wrote: > >>> The I2C specs say in 3.1.16 that the recovery procedure should be used >>> when SDA is stuck low. So, I do wonder if we should apply the recovery >>> after a timeout. Stuck SDA might be one reason for timeout, but there >>> may be others... >> >> This is ancient code. And regarding your question - >> Might be it would be reasonable to add call of >> i2c_davinci_wait_bus_not_busy() at the end of i2c_davinci_xfer()? >> This way we will wait for Bus Free before performing recovery. > > That might be an improvement, but the generic question still remains: > Is a timeout a reason for recovery? SDA stuck low is one reason for a > timeout. I have problems making up my mind here between being pragmatic > and being in accordance with the specs. The timeout here means there were no responses from I2C controller within some reasonable time period (default - 1 sec). Which in turn means that Bus/HW state is "unknown" and init&recovery seems reasonable here, but yes - "init&recovery" could be optimized more, but, in my opinion, only as subsequent patches. Actually, i2c_generic_recovery() will just exit if SDA is high already. > >> Of course, i2c_davinci_wait_bus_not_busy() has to be fixed first >> as proposed by Alexander Sverdlin here: >> https://patchwork.ozlabs.org/patch/448994/. > > Okay, good that you said it. So I'll give his patch series priority over > this one. Sorry, but this series already mises few merge windows and it has a lot of revied-by and tested-by, so could we proceed please? Re-based & re-sent http://www.spinics.net/lists/arm-kernel/msg410810.html -- regards, -grygorii -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 4/5] i2c: davinci: use bus recovery infrastructure
On 04/06/2015 07:09 PM, Wolfram Sang wrote: Of course, i2c_davinci_wait_bus_not_busy() has to be fixed first as proposed by Alexander Sverdlin here: https://patchwork.ozlabs.org/patch/448994/. Okay, good that you said it. So I'll give his patch series priority over this one. Sorry, but this series already mises few merge windows and it has a lot of revied-by and tested-by, so could we proceed please? Re-based & re-sent http://www.spinics.net/lists/arm-kernel/msg410810.html ??? Didn't you say above that Alexaders's patch is needed first? Sorry for misunderstanding. I said that if We'd like to continue and optimize more recovery path then yes - Alexaders's patch will be needed (patch 2 from his series [PATCH 2/3] i2c: davinci: Refactor i2c_davinci_wait_bus_not_busy(), which, in turn need to be rebased as the first one in his series and re-send). And in my opinion all such improvements could be done by subsequent patches. -- regards, -grygorii -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] gpio: max732x: Fix irq-events handler
On 04/21/2015 08:28 PM, Semen Protsenko wrote:
> MAX732X doesn't support edge type interrupt. So replace
> handle_edge_irq() with handle_level_irq(), which uses
> irq_mask/irq_unmask callbacks instead of irq_ack().
>
> This wrong handler may lead to NULL pointer dereference in some cases.
> E.g. this was observed on hibernation process:
>
>Unable to handle kernel NULL pointer dereference at virtual address 0
>Backtrace:
>(handle_edge_irq) from (resend_irqs)
>(resend_irqs) from (tasklet_action)
>(tasklet_action) from (__do_softirq)
>(__do_softirq) from (run_ksoftirqd)
>(run_ksoftirqd) from (smpboot_thread_fn)
>(smpboot_thread_fn) from (kthread)
>(kthread) from (ret_from_fork)
>
> Signed-off-by: Semen Protsenko
> ---
> drivers/gpio/gpio-max732x.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-max732x.c b/drivers/gpio/gpio-max732x.c
> index 1885e5c..edade14 100644
> --- a/drivers/gpio/gpio-max732x.c
> +++ b/drivers/gpio/gpio-max732x.c
> @@ -530,7 +530,7 @@ static int max732x_irq_setup(struct max732x_chip *chip,
> ret = gpiochip_irqchip_add(&chip->gpio_chip,
> &max732x_irq_chip,
> irq_base,
> - handle_edge_irq,
> + handle_level_irq,
Wouldn't handle_simple_irq() be a better choice here?
> IRQ_TYPE_NONE);
> if (ret) {
> dev_err(&client->dev,
>
--
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] drivers/rtc/rtc-ds1307.c: Enable the mcp794xx alarm after programming time
Hi, On 04/21/2015 03:51 AM, Nishanth Menon wrote: > Alarm interrupt enable register is at offset 0x7, while the time > registers for the alarm follow that. When we program Alarm interrupt > enable prior to programming the time, it is possible that previous > time value could be close or match at the time of alarm enable > resulting in interrupt trigger which is unexpected (and does not match > the time we expect it to trigger). > > To prevent this scenario from occuring, program the ALM0_EN bit only > after the alarm time is appropriately programmed. > > Ofcourse, I2C programming is non-atomic, so there are loopholes where > the interrupt wont trigger if the time requested is in the past at > the time of programming the ALM0_EN bit. However, we will not have > unexpected interrupts while the time is programmed after the interrupt > are enabled. I think it will be nice if you will mention that you going to follow vendor recommendations - AN1491 Configuring the MCP794XX RTCC Family http://ww1.microchip.com/downloads/en/AppNotes/01491A.pdf ;) "Also, it is recommended that the alarm registers be loaded before the alarm is enabled." > > Signed-off-by: Nishanth Menon > --- > Changes in v2: > - minor typo fix in comments > - merged up code that I missed committing in > > V1: https://patchwork.kernel.org/patch/6245041/ > > drivers/rtc/rtc-ds1307.c | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c > index 4ffabb322a9a..3cd4783375a5 100644 > --- a/drivers/rtc/rtc-ds1307.c > +++ b/drivers/rtc/rtc-ds1307.c > @@ -742,17 +742,17 @@ static int mcp794xx_set_alarm(struct device *dev, > struct rtc_wkalrm *t) > regs[6] &= ~MCP794XX_BIT_ALMX_IF; > /* Set alarm match: second, minute, hour, day, date, month. */ > regs[6] |= MCP794XX_MSK_ALMX_MATCH; > - > - if (t->enabled) > - regs[0] |= MCP794XX_BIT_ALM0_EN; > - else > - regs[0] &= ~MCP794XX_BIT_ALM0_EN; > + /* Disable interrupt. We will not enable until completely programmed */ > + regs[0] &= ~MCP794XX_BIT_ALM0_EN; > > ret = ds1307->write_block_data(client, MCP794XX_REG_CONTROL, 10, regs); > if (ret < 0) > return ret; > > - return 0; > + if (!t->enabled) > + return 0; > + regs[0] |= MCP794XX_BIT_ALM0_EN; > + return i2c_smbus_write_byte_data(client, MCP794XX_REG_CONTROL, regs[0]); So, It seems, that right sequence should be: - disable alarmX - read alarmX regs - configure alarmX regs - load alarmX regs - enable alarmX More over, looks like, alarm/alarm IRQ should be enabled/disabled separately from set_alarm/RTC_ALM_SET by RTC_AIE_ON, RTC_AIE_OFF. Should it? > } > > static int mcp794xx_alarm_irq_enable(struct device *dev, unsigned int > enabled) > -- regards, -grygorii -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] drivers/rtc/rtc-ds1307.c: Enable the mcp794xx alarm after programming time
On 04/23/2015 03:00 AM, Nishanth Menon wrote: > On 04/22/2015 08:26 AM, grygorii.stras...@linaro.org wrote: >> Hi, >> >> On 04/21/2015 03:51 AM, Nishanth Menon wrote: >>> Alarm interrupt enable register is at offset 0x7, while the time >>> registers for the alarm follow that. When we program Alarm interrupt >>> enable prior to programming the time, it is possible that previous >>> time value could be close or match at the time of alarm enable >>> resulting in interrupt trigger which is unexpected (and does not match >>> the time we expect it to trigger). >>> >>> To prevent this scenario from occuring, program the ALM0_EN bit only >>> after the alarm time is appropriately programmed. >>> >>> Ofcourse, I2C programming is non-atomic, so there are loopholes where >>> the interrupt wont trigger if the time requested is in the past at >>> the time of programming the ALM0_EN bit. However, we will not have >>> unexpected interrupts while the time is programmed after the interrupt >>> are enabled. >> >> I think it will be nice if you will mention that you going to follow >> vendor recommendations - AN1491 Configuring the MCP794XX RTCC Family >> http://ww1.microchip.com/downloads/en/AppNotes/01491A.pdf >> ;) >> "Also, it is recommended that the alarm registers be loaded >> before the alarm is enabled." >> > > Hmm... i did not know that existed, thanks for digging it up.. that > teaches me to look for docs before putting a scope/LA on the board > (not that I regret doing that)... That said, reading the app note, I > kind of realized: > a) that playing with ST bit for programming time is not done, but > then, that implies that oscillator will have to be restarted (upto a > few seconds for certain crystals).. but that said, it does not seem > mandatory or seem to (yet seen) functional issues... > > b) We dont have flexibility yet to describe if we do indeed have a > backup battery or not - VBATEN should be set only if we have a backup > battery on the platform :( - on some it might even be optional thanks > to certain compliance requirements of shipping boards internationally > and general "unlike" of lithium ion in cargo hold.. > > c) we dont have capability to control the alarm polarity in the driver > which, by the way, we probably should also control OUT polarity (when > ALARM is not asserted).. > > d) we dont have support for external 32k oscillator(X1 only) instead > of assuming we always have a 32k crystal(X1 and X2)... > > Ugghhh... more cleaning up to do for the future.. > > that said, the sequence it does recommend (in page 4): > The following steps show how the Alarm 0 is config- > ured. Alarm 1 can be configured in the same manner. > 1.Write 0x23 to the Alarm0 Seconds register > [0x0A]. > 2.Write 0x47 to the Alarm0 Minutes register > [0x0B]. > 3.Write 0x71 to the Alarm0 Hours register [0x0C] > – 11 hours in 12-hour format. > 4.Write 0x72 to the Alarm0 Day register [0x0D] – > Tuesday + Alarm Polarity Low + Match on all. > The Alarm0 Interrupt Flag is also cleared. > 5.Write 0x14 to the Alarm0 Date register [0x0E]. > 6.Write 0x08 to the Alarm0 Month register [0x0F]. > With all the Alarm0 registers set we can now activate > the Alarm0 on the Control register. > 7.Write 0x10 to the Control register [0x07] – > Alarm0 enabled no CLKOUT, Alarm1 disabled > > before this patch we do ( http://pastebin.ubuntu.com/10863880/) > CONTROL r[7] = 0x90 (OUT=1, ALM0EN=1) > OSCTRIM r[8] = 0x00 > EEUNLOCK r[9] = 0x00 > ALM0SEC r[A] = 0x01 > ALM0MIN r[B] = 0x45 > ALM0HOUR r[C] = 0x23 > ALM0WKDAY r[D] = 0x75 <-ALMOIF is cleared > ALM0DATE r[E] = 0x09 > ALM0MTH r[F] = 0x04 > RSRVED r[10] = 0x01 > > with this patch, we do: > burst(CONTROL r[7] = 0x80 (OUT=1) > OSCTRIM r[8] = 0x00 > EEUNLOCK r[9] = 0x00 > ALM0SEC r[A] = 0x01 > ALM0MIN r[B] = 0x45 > ALM0HOUR r[C] = 0x23 > ALM0WKDAY r[D] = 0x75 <-ALMOIF is cleared > ALM0DATE r[E] = 0x09 > ALM0MTH r[F] = 0x04 > RSRVED r[10] = 0x01 > ) > CONTROL r[7] = 0x90 (OUT=1, ALM0EN=1) > > Which is slightly unoptimal way of what the app note recommends. - as > I mentioned earlier in this thread, I will try and do optimizations in > a later patch. > > Given that Andrew had picked up this patch, I dont see a reason to > respin this yet. but will include the app note for future patches - > thanks for pointing it out to me. ^^ Up to you. Np, Always yours! >>> >>&g
Re: [PATCH V2] drivers/rtc/rtc-ds1307.c: Enable the mcp794xx alarm after programming time
On 04/23/2015 04:11 PM, Nishanth Menon wrote: On 04/23/2015 05:17 AM, grygorii.stras...@linaro.org wrote: On 04/23/2015 03:00 AM, Nishanth Menon wrote: On 04/22/2015 08:26 AM, grygorii.stras...@linaro.org wrote: Hi, On 04/21/2015 03:51 AM, Nishanth Menon wrote: Alarm interrupt enable register is at offset 0x7, while the time registers for the alarm follow that. When we program Alarm interrupt enable prior to programming the time, it is possible that previous time value could be close or match at the time of alarm enable resulting in interrupt trigger which is unexpected (and does not match the time we expect it to trigger). To prevent this scenario from occuring, program the ALM0_EN bit only after the alarm time is appropriately programmed. Ofcourse, I2C programming is non-atomic, so there are loopholes where the interrupt wont trigger if the time requested is in the past at the time of programming the ALM0_EN bit. However, we will not have unexpected interrupts while the time is programmed after the interrupt are enabled. I think it will be nice if you will mention that you going to follow vendor recommendations - AN1491 Configuring the MCP794XX RTCC Family http://ww1.microchip.com/downloads/en/AppNotes/01491A.pdf ;) "Also, it is recommended that the alarm registers be loaded before the alarm is enabled." Hmm... i did not know that existed, thanks for digging it up.. that teaches me to look for docs before putting a scope/LA on the board (not that I regret doing that)... That said, reading the app note, I kind of realized: a) that playing with ST bit for programming time is not done, but then, that implies that oscillator will have to be restarted (upto a few seconds for certain crystals).. but that said, it does not seem mandatory or seem to (yet seen) functional issues... b) We dont have flexibility yet to describe if we do indeed have a backup battery or not - VBATEN should be set only if we have a backup battery on the platform :( - on some it might even be optional thanks to certain compliance requirements of shipping boards internationally and general "unlike" of lithium ion in cargo hold.. c) we dont have capability to control the alarm polarity in the driver which, by the way, we probably should also control OUT polarity (when ALARM is not asserted).. d) we dont have support for external 32k oscillator(X1 only) instead of assuming we always have a 32k crystal(X1 and X2)... Ugghhh... more cleaning up to do for the future.. that said, the sequence it does recommend (in page 4): The following steps show how the Alarm 0 is config- ured. Alarm 1 can be configured in the same manner. 1.Write 0x23 to the Alarm0 Seconds register [0x0A]. 2.Write 0x47 to the Alarm0 Minutes register [0x0B]. 3.Write 0x71 to the Alarm0 Hours register [0x0C] – 11 hours in 12-hour format. 4.Write 0x72 to the Alarm0 Day register [0x0D] – Tuesday + Alarm Polarity Low + Match on all. The Alarm0 Interrupt Flag is also cleared. 5.Write 0x14 to the Alarm0 Date register [0x0E]. 6.Write 0x08 to the Alarm0 Month register [0x0F]. With all the Alarm0 registers set we can now activate the Alarm0 on the Control register. 7.Write 0x10 to the Control register [0x07] – Alarm0 enabled no CLKOUT, Alarm1 disabled before this patch we do ( http://pastebin.ubuntu.com/10863880/) CONTROL r[7] = 0x90 (OUT=1, ALM0EN=1) OSCTRIM r[8] = 0x00 EEUNLOCK r[9] = 0x00 ALM0SEC r[A] = 0x01 ALM0MIN r[B] = 0x45 ALM0HOUR r[C] = 0x23 ALM0WKDAY r[D] = 0x75 <-ALMOIF is cleared ALM0DATE r[E] = 0x09 ALM0MTH r[F] = 0x04 RSRVED r[10] = 0x01 with this patch, we do: burst( CONTROL r[7] = 0x80 (OUT=1) OSCTRIM r[8] = 0x00 EEUNLOCK r[9] = 0x00 ALM0SEC r[A] = 0x01 ALM0MIN r[B] = 0x45 ALM0HOUR r[C] = 0x23 ALM0WKDAY r[D] = 0x75 <-ALMOIF is cleared ALM0DATE r[E] = 0x09 ALM0MTH r[F] = 0x04 RSRVED r[10] = 0x01 ) CONTROL r[7] = 0x90 (OUT=1, ALM0EN=1) Which is slightly unoptimal way of what the app note recommends. - as I mentioned earlier in this thread, I will try and do optimizations in a later patch. Given that Andrew had picked up this patch, I dont see a reason to respin this yet. but will include the app note for future patches - thanks for pointing it out to me. ^^ Up to you. Np, Always yours! Considering the narrow focus of the current patch (which does fix an issue that it attempts to), can I get an Ack? Reviewed-by: Grygorii Strashko -- regards, -grygorii -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 0/3] i2c: davinci improvements and fixes
On 04/10/2015 06:59 PM, Wolfram Sang wrote: On Mon, Apr 06, 2015 at 03:38:38PM +0300, grygorii.stras...@linaro.org wrote: From: Grygorii Strashko This series converts driver to use I2C bus recovery infrastructure and adds Davinci I2C bus recovery mechanizm based on using ICPFUNC registers. These patches are combination of two patches from Ben Gardiner [1] and Mike Looijmans [2] which i've reworked to use I2C bus recovery infrastructure Applied to for-next, thanks for keeping at it and providing useful info! Thanks -- regards, -grygorii -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v1] clk: Add Suspend/Resume Callbacks in Clock
On 05/27/2015 02:11 PM, Pankaj Dev wrote:
> Currently the CCF doesnt have any interface with the system when it
> goes into suspend state. This patch, on similar lines of syscore,
> adds clk_suspend/clk_resume functions before system enters suspend state
> Also there are suspend/resume hooks added for the clock driver
>
> Acked-by: Olivier Bideau
> Acked-by: Patrice Chotard
> Signed-off-by: Pankaj Dev
Nice:) Second try. FYI http://www.spinics.net/lists/linux-clk/msg00743.html
Regarding your patch:
* @prepare:Prepare the clock for enabling. This must not return until
* the clock is fully prepared, and it's safe to call clk_enable.
* This callback is intended to allow clock implementations to
* do any initialisation that may sleep. Called with
* prepare_lock held.
.un/prepare() callbacks intended to be used for slow clocks and may sleep, so
you can't call them when Syscore is suspended and with disabled IRQs.
> ---
> drivers/clk/clk.c| 194
> ++
> include/linux/clk-provider.h | 21 +
> include/linux/clk.h | 23 +
> kernel/kexec.c |6 ++
> kernel/power/hibernate.c | 16
> kernel/power/suspend.c |7 ++
> 6 files changed, 267 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 237f23f..3e3b1d7 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2811,6 +2811,200 @@ int clk_notifier_unregister(struct clk *clk, struct
> notifier_block *nb)
> }
> EXPORT_SYMBOL_GPL(clk_notifier_unregister);
>
> +#ifdef CONFIG_PM_SLEEP
> +int __clk_resume(struct clk_core *clk)
> +{
> + int ret = 0;
> + u8 p_index;
> + unsigned long rate, parent_rate = 0;
> +
> + /* restore parent */
> + if (clk->ops->set_parent) {
> + p_index = clk_fetch_parent_index(clk, clk->parent);
> +
> + if (p_index != clk->ops->get_parent(clk->hw))
> + ret = clk->ops->set_parent(clk->hw, p_index);
> +
> + if (ret) {
> + pr_err("%s: Fail to set parent %s\n",
> +__func__, clk->name);
> + goto out;
> + }
> + }
> +
> + /* restore rate */
> + if (clk->ops->set_rate) {
> + if (clk->parent)
> + parent_rate = clk->parent->rate;
> +
> + rate = clk->ops->recalc_rate(clk->hw, parent_rate);
> +
> + if (rate != clk->rate)
> + ret = clk->ops->set_rate(clk->hw, clk->rate,
> + parent_rate);
> +
> + if (ret) {
> + pr_err("%s: Fail to set rate %s\n",
> +__func__, clk->name);
> + goto out;
> + }
> + }
> +
> + /* restore prepare status */
> + if (clk->ops->prepare && clk->prepare_count) {
> + if (clk->ops->is_prepared) {
> + if (!clk->ops->is_prepared(clk->hw))
> + ret = clk->ops->prepare(clk->hw);
> + } else {
> + ret = clk->ops->prepare(clk->hw);
> + }
> + if (ret) {
> + pr_err("%s: Fail to prepare %s\n",
> +__func__, clk->name);
> + goto out;
> + }
> + }
> +
> + /* restore enable status */
> + if (clk->ops->enable && clk->enable_count) {
> + if (clk->ops->is_enabled) {
> + if (!clk->ops->is_enabled(clk->hw))
> + ret = clk->ops->enable(clk->hw);
> + } else {
> + ret = clk->ops->enable(clk->hw);
> + }
> + if (ret) {
> + pr_err("%s: Fail to enable %s\n",
> +__func__, clk->name);
> + goto out;
> + }
> + }
> + if (clk->ops->disable && !clk->enable_count) {
> + if (clk->ops->is_enabled) {
> + if (clk->ops->is_enabled(clk->hw))
> + clk->ops->disable(clk->hw);
> + } else {
> + clk->ops->disable(clk->hw);
> + }
> + }
> +
> + /* restore unprepare status */
> + if (clk->ops->unprepare && !clk->prepare_count) {
> + if (clk->ops->is_prepared) {
> + if (!clk->ops->is_prepared(clk->hw))
> + clk->ops->unprepare(clk->hw);
> + } else {
> + clk->ops->unprepare(clk->hw);
> + }
> + }
> +
> +out:
> + return ret;
> +}
> +
> +static int _clk_resume(struct clk_core *clk)
> +{
> + struct clk_core *child;
> + int ret = 0;
> +
> + if (!clk)
> + return 0;
> +
> + if (clk->ops->resume)
> + ret = clk->ops-
Re: [PATCH] gpiolib: debugfs: display gpios requested as irq only
Hi Linus, On 06/01/2015 04:09 PM, Linus Walleij wrote: On Mon, May 25, 2015 at 8:54 PM, grygorii.stras...@linaro.org wrote: .. Looks like it's time to drop this stuff :,,( Ooops missed this part of the discussion. Indeed it will call accessors on non-requested GPIO lines. Damned. Taking these patches out again. Yep. I've missed to recall v2 patches, sorry for that. -- regards, -grygorii -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 00/21] On-demand device registration
Hi Tomeu, On 05/28/2015 07:33 AM, Rob Herring wrote: > On Mon, May 25, 2015 at 9:53 AM, Tomeu Vizoso > wrote: >> I have a problem with the panel on my Tegra Chromebook taking longer than >> expected to be ready during boot (Stéphane Marchesin reported what is >> basically the same issue in [0]), and have looked into ordered probing as a >> better way of solving this than moving nodes around in the DT or playing with >> initcall levels. >> >> While reading the thread [1] that Alexander Holler started with his series to >> make probing order deterministic, it occurred to me that it should be >> possible >> to achieve the same by registering devices as they are referenced by other >> devices. > > I like the concept and novel approach. > >> This basically reuses the information that is already implicit in the probe() >> implementations, saving us from refactoring existing drivers or adding >> information to DTBs. >> >> Something I'm not completely happy with is that I have had to move the call >> to >> of_platform_populate after all platform drivers have been registered. >> Otherwise I don't see how I could register drivers on demand as we don't have >> yet each driver's compatible strings. > > Yeah, this is the opposite of what we'd really like. Ideally, we would > have a solution that works for modules too. However, we're no worse > off. We pretty much build-in dependencies to avoid module ordering > problems. > > Perhaps we need to make the probing on-demand rather than simply on > device<->driver match occurring. > >> For machs that don't move of_platform_populate() to a later point, these >> patches shouldn't cause any problems but it's not guaranteed that we'll avoid >> all the deferred probes as some drivers may not be registered yet. > > Ideally, of_platform_populate is not explicitly called by each > platform. So I think we need to make this work for the default case. > >> I have tested this on boards with Tegra, iMX.6 and Exynos SoCs, and these >> patches were enough to eliminate all the deferred probes. >> >> With this series I get the kernel to output to the panel in 0.5s, instead of >> 2.8s. > > That's certainly compelling. I've found your idea about moving device registration later during System boot very interesting so I've decided to try it on dra7-evem (TI) :). It's good to know time during Kernel boot when we can assume that all drivers are ready for probing, so there are more ways to control probing order. Pls, Note here that TI OMAP2+ mach is not pure DT mach - it's combination of DT and not DT devices/drivers. Ok. So What was done... LKML Linux 4.1-rc3 (a simple case) 1) use your patches 3/4 as reference (only these two patches :) 2) move of_platform_populate later at device_initcall_sync time Boot time reduction ~0.4 sec TI Android Kernel 3.14 (NOT a simple case) 1) use your patches 3/4 as reference (only these two patches :) 2) move of_platform_populate later at device_initcall_sync time 3) make it to boot (not sure I've fixed all issues, but those which break the System boot): - split non-DT and DT devices registration in platform code; - keep non-DT devices registration from .init_machine() [arch_initcall] - move DT-devices registration at device_initcall_sync time - fix drivers which use platform_driver_probe(). Note. Now there are at about ~190 occurrences of this macro in Kernel. - re-order few devices in DT (4 devices) - fix one driver which uses of_find_device_by_node() wrongly Note. This API is used some times with assumption that requested dev has been probed already. Boot time reduction ~0.3 sec. Probing of some devices are still deferred. TI Android Kernel 3.14 + of_platform_device_ensure 1) backport of_platform_device_ensure() (also need patches "of: Introduce device tree node flag helpers" and "of: Keep track of populated platform devices") 2) back-port all your patches which uses of_platform_device_ensure() 3) make it to boot: - drop patch dma: of: Probe DMA controllers on demand - fix deadlock in regulator core: regulator_dev_lookup() called from regulator_register() in K3.14 4) get rid of deferred probes - add of_platform_device_ensure() calls in: - drivers/video/fbdev/omap2/dss/output.c - drivers/extcon/extcon-class.c Boot time reduction: NONE !? So few comments from above: - registering devices later during the System boot may improve boot time. But resolving of all deferred probes may NOT improve boot time ;) Have you seen smth like this? - usage of of_platform_device_ensure() will require continuous fixing of Kernel :( - late_initcall is not (as for me not safe) a good time to register devices. A lot of platforms/subsystems/frameworks perform their final initialization or clean-up steps, with assumption that System mostly ready to work. For example, CPUIdle/CPUFreq are allowed and other PM staff. CPUIdle and driver's probing are not friends. What would be nice to have for now in my op
Re: [PATCH 00/21] On-demand device registration
On 06/04/2015 11:39 AM, Tomeu Vizoso wrote:
> On 3 June 2015 at 21:57, grygorii.stras...@linaro.org
> wrote:
>> On 05/28/2015 07:33 AM, Rob Herring wrote:
>>> On Mon, May 25, 2015 at 9:53 AM, Tomeu Vizoso
>>> wrote:
>>>> I have a problem with the panel on my Tegra Chromebook taking longer than
>>>> expected to be ready during boot (Stéphane Marchesin reported what is
>>>> basically the same issue in [0]), and have looked into ordered probing as a
>>>> better way of solving this than moving nodes around in the DT or playing
>>>> with
>>>> initcall levels.
>>>>
>>>> While reading the thread [1] that Alexander Holler started with his series
>>>> to
>>>> make probing order deterministic, it occurred to me that it should be
>>>> possible
>>>> to achieve the same by registering devices as they are referenced by other
>>>> devices.
>>>
>>> I like the concept and novel approach.
>>>
>>>> This basically reuses the information that is already implicit in the
>>>> probe()
>>>> implementations, saving us from refactoring existing drivers or adding
>>>> information to DTBs.
>>>>
>>>> Something I'm not completely happy with is that I have had to move the
>>>> call to
>>>> of_platform_populate after all platform drivers have been registered.
>>>> Otherwise I don't see how I could register drivers on demand as we don't
>>>> have
>>>> yet each driver's compatible strings.
>>>
>>> Yeah, this is the opposite of what we'd really like. Ideally, we would
>>> have a solution that works for modules too. However, we're no worse
>>> off. We pretty much build-in dependencies to avoid module ordering
>>> problems.
>>>
>>> Perhaps we need to make the probing on-demand rather than simply on
>>> device<->driver match occurring.
>>>
>>>> For machs that don't move of_platform_populate() to a later point, these
>>>> patches shouldn't cause any problems but it's not guaranteed that we'll
>>>> avoid
>>>> all the deferred probes as some drivers may not be registered yet.
>>>
>>> Ideally, of_platform_populate is not explicitly called by each
>>> platform. So I think we need to make this work for the default case.
>>>
>>>> I have tested this on boards with Tegra, iMX.6 and Exynos SoCs, and these
>>>> patches were enough to eliminate all the deferred probes.
>>>>
>>>> With this series I get the kernel to output to the panel in 0.5s, instead
>>>> of 2.8s.
>>>
>>> That's certainly compelling.
>>
>> I've found your idea about moving device registration later during System
>> boot
>> very interesting so I've decided to try it on dra7-evem (TI) :).
>> It's good to know time during Kernel boot when we can assume that all
>> drivers are
>> ready for probing, so there are more ways to control probing order.
>
> Thanks, though right now I'm following Rob's suggestion and only delay
> probing, not registration. The patch is really simple (applies on
> linux-next, with async probing):
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 8da8e07..7e6b1e1 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -407,6 +407,11 @@ int driver_probe_device(struct device_driver
> *drv, struct device *dev)
> if (!device_is_registered(dev))
> return -ENODEV;
>
> + if (!driver_deferred_probe_enable) {
> + driver_deferred_probe_add(dev);
> + return 0;
> + }
> +
> pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
> drv->bus->name, __func__, dev_name(dev), drv->name);
>
> @@ -585,7 +590,7 @@ EXPORT_SYMBOL_GPL(device_attach);
>
> void device_initial_probe(struct device *dev)
> {
> - __device_attach(dev, true);
> + __device_attach(dev, driver_deferred_probe_enable);
> }
>
> static int __driver_attach(struct device *dev, void *data)
Can't boot my 3.14 kernel with this change :( Most probably,
the problem is related to platform_driver_probe() usage :(
Have no time to play with it now :(, but recommend you to check also
earlyprintk, last log message I can see:
[1.435522] bootconsole [earlycon0] disabled
But, nice try ;) Seems -EPROBE_DEFER is reality of life which
has to be accepte
Re: [PATCH] gpiolib: debugfs: display gpios requested as irq only
On 05/21/2015 05:25 PM, Johan Hovold wrote:
> On Tue, May 19, 2015 at 04:28:55PM +0200, Linus Walleij wrote:
>> On Mon, May 18, 2015 at 1:02 PM, Johan Hovold wrote:
>>> On Fri, May 15, 2015 at 04:25:21PM +0300, grygorii.stras...@linaro.org
>>> wrote:
>>
>>>> GPIOs 192-223, platform/48051000.gpio, gpio:
>>>> gpio-203 (vtt_fixed ) out hi requested
>>>
>>> This is backwards. All gpios *should* be requested. *If* we are to
>>> include not-requested gpios in the debug output, then it is those pins
>>> that need to be marked as not-requested.
>>
>> It depends, really. As concluded in earlier discussions when we
>> introduced gpiochip_[un]lock_as_irq() the gpiolib and irqchip APIs
>> are essentially orthogonal.
>
> [...]
>
>> So to atleast try to safeguard from a scenario such as
>>
>> - Client A requests IRQ from the irqchip side of the API
>>and sets up a level active-low IRQ on it
>>
>> - Client B request the same line as GPIO
>>
>> - Client B sets it to output and drivers it low.
>>
>> - Client A crashes in an infinite IRQ loop as that line
>>is not hammered low and will generate IRQs until
>>the end of time.
>>
>> I introduced the gpiochip_[un]lock_as_irq() calls so we
>> could safeguard against this. Notably that blocks client A
>> from setting the line as output. I hope.
>
> A problem with the current implementation is that it uses as a flag
> rather than a refcount. As I pointed out elsewhere in this thread, it is
> possible to request a shared IRQ (e.g. via the sysfs interface) and
> release it, thereby making it possible to change the direction of the
> pin while still in use for irq.
Yes (checked). And this is an issue which need to be fixed.
- gpio sysfs should not call gpiochip_un/lock_as_irq()
- gpio drivers should use gpiochip API or implement
.irq_release/request_resources() callbacks
in this case case IRQ core will do just what is required. Right?
>
>> I thought this would mean the line would only be used as IRQ
>> in this case, so I could block any gpiod_get() calls to that
>> line but *of course* some driver is using the IRQ and snooping
>> into the GPIO value at the same time. So can't simplify things
>> like so either.
>>
>> Maybe I'm smashing open doors here...
>
> No, I understand that use case. But there are some issues with how it's
> currently implemented. Besides the example above, nothing pins a gpio
> chip driver in memory unless it has requested gpios, specifically,
> requesting a pin for irq use is not enough.
ok. An issue. Possible fix below:
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index ea11706..64392ad 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -514,6 +514,9 @@ static int gpiochip_irq_reqres(struct irq_data *d)
{
struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+ if (!try_module_get(chip->owner))
+ return -ENODEV;
+
if (gpiochip_lock_as_irq(chip, d->hwirq)) {
chip_err(chip,
"unable to lock HW IRQ %lu for IRQ\n",
@@ -528,6 +531,7 @@ static void gpiochip_irq_relres(struct irq_data *d)
struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
gpiochip_unlock_as_irq(chip, d->hwirq);
+ module_put(chip->owner);
}
>
>> Anyway to get back to the original statement:
>>
>>> This is backwards. All gpios *should* be requested. *If* we are to
>>> include not-requested gpios in the debug output, then it is those pins
>>> that need to be marked as not-requested.
>>
>> This is correct, all GPIOs accessed *as gpios* should be
>> requested, no matter if they are then cast to IRQs by gpiod_to_irq
>> or not. However if the same hardware is used as only "some IRQ"
>> through it's irqchip interface, it needs not be requested, but
>> that is by definition not a GPIO, it is an IRQ.
>
> True. And since it is not a GPIO, should it show up in
> /sys/kernel/debug/gpio? ;)
"Nice" idea :)
This information needed for debugging and testing which includes
checking of pin state (hi/lo) - especially useful during board's
bring-up when a lot of mistakes are detected related to wrong usage
of IRQ/GPIO numbers.
--
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpiolib: debugfs: display gpios requested as irq only
On 05/24/2015 08:12 PM, Johan Hovold wrote:
> On Thu, May 21, 2015 at 11:33:01PM +0300, grygorii.stras...@linaro.org wrote:
>> On 05/21/2015 05:25 PM, Johan Hovold wrote:
>>> On Tue, May 19, 2015 at 04:28:55PM +0200, Linus Walleij wrote:
>
>>>> I introduced the gpiochip_[un]lock_as_irq() calls so we
>>>> could safeguard against this. Notably that blocks client A
>>>> from setting the line as output. I hope.
>>>
>>> A problem with the current implementation is that it uses as a flag
>>> rather than a refcount. As I pointed out elsewhere in this thread, it is
>>> possible to request a shared IRQ (e.g. via the sysfs interface) and
>>> release it, thereby making it possible to change the direction of the
>>> pin while still in use for irq.
>>
>> Yes (checked). And this is an issue which need to be fixed.
>> - gpio sysfs should not call gpiochip_un/lock_as_irq()
>
> This is a known but unrelated issue. The lock/unlock call in the sysfs
> implementation is at worst redundant. I suggested removing it earlier,
> but Linus pointed out that there were still a few drivers not
> implementing the irq resource callbacks that need to be updated first.
>
>> - gpio drivers should use gpiochip API or implement
>>.irq_release/request_resources() callbacks
>>
>> in this case case IRQ core will do just what is required. Right?
>
> No, the problem is that the "lock" is implemented using a flag rather
> than a refcount.
I've rechecked __setup_irq() and what I can see from it is that
irq_request_resources(), __irq_set_trigger() and irq_startup()
functions will be called only when the first IRQ action is installed.
So, It looks like flag should work here. Am I missing smth?
>
>>>> I thought this would mean the line would only be used as IRQ
>>>> in this case, so I could block any gpiod_get() calls to that
>>>> line but *of course* some driver is using the IRQ and snooping
>>>> into the GPIO value at the same time. So can't simplify things
>>>> like so either.
>>>>
>>>> Maybe I'm smashing open doors here...
>>>
>>> No, I understand that use case. But there are some issues with how it's
>>> currently implemented. Besides the example above, nothing pins a gpio
>>> chip driver in memory unless it has requested gpios, specifically,
>>> requesting a pin for irq use is not enough.
>>
>> ok. An issue. Possible fix below:
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index ea11706..64392ad 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -514,6 +514,9 @@ static int gpiochip_irq_reqres(struct irq_data *d)
>> {
>> struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>>
>> + if (!try_module_get(chip->owner))
>> + return -ENODEV;
>> +
>> if (gpiochip_lock_as_irq(chip, d->hwirq)) {
>> chip_err(chip,
>> "unable to lock HW IRQ %lu for IRQ\n",
>> @@ -528,6 +531,7 @@ static void gpiochip_irq_relres(struct irq_data *d)
>> struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>>
>> gpiochip_unlock_as_irq(chip, d->hwirq);
>> + module_put(chip->owner);
>> }
>
> The resource callbacks would be the place to do resource allocation, but
> the above snippet is obviously broken as its leaking resources in the
> error path.
True, Thanks. This was the very fast try to solve issue. It could be converted
to the patch if GPIO maintainers agree with this approach.
>
>>>> Anyway to get back to the original statement:
>>>>
>>>>> This is backwards. All gpios *should* be requested. *If* we are to
>>>>> include not-requested gpios in the debug output, then it is those pins
>>>>> that need to be marked as not-requested.
>>>>
>>>> This is correct, all GPIOs accessed *as gpios* should be
>>>> requested, no matter if they are then cast to IRQs by gpiod_to_irq
>>>> or not. However if the same hardware is used as only "some IRQ"
>>>> through it's irqchip interface, it needs not be requested, but
>>>> that is by definition not a GPIO, it is an IRQ.
>>>
>>> True. And since it is not a GPIO, should it show up in
>>> /sys/kernel/debug/gpio? ;)
>>
>> "Nice" idea :)
>> This information needed for debugging and testing which includes
>> checking of pin state (hi/lo) - espe
Re: [PATCH] gpiolib: debugfs: display gpios requested as irq only
Hi Johan, On 05/18/2015 02:02 PM, Johan Hovold wrote: > On Fri, May 15, 2015 at 04:25:21PM +0300, grygorii.stras...@linaro.org wrote: >> From: Grygorii Strashko >> >> Now GPIOs, wich are requested as IRQ only, will not be displayed >> through GPIO debugfs. For example: >> # cat /proc/interrupts >> CPU0 CPU1 >> ... >> 209: 0 0 4805d000.gpio 11 Edge 0-0021 >> >> # cat /debug/gpio >> ... >> GPIOs 160-191, platform/4805d000.gpio, gpio: >> <--- no info about gpio used as IRQ only here >> >> GPIOs 192-223, platform/48051000.gpio, gpio: >> gpio-203 (vtt_fixed ) out hi >> ... >> >> Hence, improve GPIO debugfs code to show such kind of gpio and print >> IRQ number also. In addition, add marker "requested" for GPIOs wich >> were requested by using gpioX_request(). >> >> After this patch sys/kernel/debug/gpio will produce following output: >> >> # cat /debug/gpio >> ... >> GPIOs 160-191, platform/4805d000.gpio, gpio: >> gpio-171 ((null) ) in hi IRQ209 >> >> GPIOs 192-223, platform/48051000.gpio, gpio: >> gpio-203 (vtt_fixed ) out hi requested > > This is backwards. All gpios *should* be requested. *If* we are to > include not-requested gpios in the debug output, then it is those pins > that need to be marked as not-requested. Sry, but I didn't fully understand your point here ( - Why is it backward? Now GPIO can be requested in three ways: 1) As pure GPIO (gpioX_request()) 2) As pure GPIO IRQ, especially in DT boot case. DT: interrupt-parent = <&gpio6>; interrupts = <11 IRQ_TYPE_EDGE_FALLING>; Code: platform_get_irq() or of_irq_get() request_irq() 3) combination of (1) and (2) with one restriction - GPIO direction should be 'In' and can't be changed. Personally I'm using this case for debug purposes to do a fast check of GPIO pin state through GPIO sysfs (GPIO export) when such GPIO is used as GPIO IRQ in some driver and I don't see that corresponding IRQ is triggered as expected. So, this patch just adds missed information in GPIO debugfs for the case (2) in general. Of course, format of the marker "requested" is discussable. Could be: - "requested" --> "not-requested" - "I R" or "I G" where I - IRQ, G - GPIO, R - requested - etc. > > The irq-number mapping could perhaps be useful, but it should go in a > separate patch. I'd suggest adding a '-' before the irq-number (e.g. > "IRQ-209"). I've thought about this, but finally decided not to split it. Could be done, if you insist ) -- regards, -grygorii -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Calling irq_set_irq_wake() from .set_irq_wake()?
On 05/18/2015 05:31 PM, Thomas Gleixner wrote:
> On Sun, 17 May 2015, Geert Uytterhoeven wrote:
> At least the recursive locking message no longer appears after the revert.
>
> [ 30.591905] PM: Syncing filesystems ... done.
> [ 30.623060] Freezing user space processes ... (elapsed 0.003 seconds)
> done.
> [ 30.634470] Freezing remaining freezable tasks ... (elapsed 0.002
> seconds) done.
> [ 30.658288] sd 0:0:0:0: [sda] Synchronizing SCSI cache
> [ 30.663678]
> [ 30.663681] =
> [ 30.663683] [ INFO: possible recursive locking detected ]
> [ 30.663688] 4.1.0-rc3 #1115 Not tainted
> [ 30.663693] -
> [ 30.663697] suspend.sh/2319 is trying to acquire lock:
> [ 30.663719] (class){..}, at: []
> __irq_get_desc_lock+0x48/0x88
> [ 30.663722]
> [ 30.663722] but task is already holding lock:
> [ 30.663734] (class){..}, at: []
> __irq_get_desc_lock+0x48/0x88
Does this mean .set_irq_wake() cannot call irq_set_irq_wake()?
>
> It can call it, if it's guaranteed that this wont deadlock.
>
> To tell lockdep that you sure about that, you need to set a different
> lock class for the child interrupts. irq_set_lockdep_class() is what
> you want to use here.
Hm. Seems we already have corresponding call in gpiochip_irq_map:
static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq,
irq_hw_number_t hwirq)
{
struct gpio_chip *chip = d->host_data;
irq_set_chip_data(irq, chip);
irq_set_lockdep_class(irq, &gpiochip_irq_lock_class);
commit e45d1c80c0eee88e82751461e9cac49d9ed287bc
Author: Linus Walleij
Date: Tue Apr 22 14:01:46 2014 +0200
gpio: put GPIO IRQs into their own lock clas
added in Kernel v3.16
Roger, can you confirm that you've observed this issue with latest kernel, pls?
>
Many GPIO drivers do that, as they need to propagate wake-up state to the
parent interrupt controller?
>>>
>>> As I remember, there was similar problem, so I found corresponding patch
>>> (just FYI)
>>>
>>> ab2b926 mfd: Fix twl6030 lockdep recursion warning on setting wake IRQs
>>>
>>> Not sure such kind of solution is the best choice (
>>
>> That looks like a convoluted solution...
>
> Indeed. See above.
--
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpiolib: debugfs: display gpios requested as irq only
On 05/18/2015 06:08 PM, Johan Hovold wrote: > On Mon, May 18, 2015 at 04:06:08PM +0300, grygorii.stras...@linaro.org wrote: >> On 05/18/2015 02:02 PM, Johan Hovold wrote: >>> On Fri, May 15, 2015 at 04:25:21PM +0300, grygorii.stras...@linaro.org >>> wrote: >>>> From: Grygorii Strashko >>>> >>>> Now GPIOs, wich are requested as IRQ only, will not be displayed >>>> through GPIO debugfs. For example: >>>># cat /proc/interrupts >>>> CPU0 CPU1 >>>> ... >>>> 209: 0 0 4805d000.gpio 11 Edge 0-0021 >>>> >>>># cat /debug/gpio >>>> ... >>>> GPIOs 160-191, platform/4805d000.gpio, gpio: >>>> <--- no info about gpio used as IRQ only here >>>> >>>> GPIOs 192-223, platform/48051000.gpio, gpio: >>>>gpio-203 (vtt_fixed ) out hi >>>> ... >>>> >>>> Hence, improve GPIO debugfs code to show such kind of gpio and print >>>> IRQ number also. In addition, add marker "requested" for GPIOs wich >>>> were requested by using gpioX_request(). >>>> >>>> After this patch sys/kernel/debug/gpio will produce following output: >>>> >>>># cat /debug/gpio >>>> ... >>>> GPIOs 160-191, platform/4805d000.gpio, gpio: >>>>gpio-171 ((null) ) in hi IRQ209 >>>> >>>> GPIOs 192-223, platform/48051000.gpio, gpio: >>>>gpio-203 (vtt_fixed ) out hi requested >>> >>> This is backwards. All gpios *should* be requested. *If* we are to >>> include not-requested gpios in the debug output, then it is those pins >>> that need to be marked as not-requested. >> >> Sry, but I didn't fully understand your point here ( - Why is it >> backward? > > My main point was that you should mark the pins used for irq only > instead of the other way round. > > We should also consider making sure that pins only used for irq are also > requested. > > With the current sysfs-interface it is for example to possible to > request the same irq, and when that pin is unexported (or only irq > released) the pin will no longer be marked for irq (just a flag that is > cleared) so that the direction can be changed... > > gpiolib also depends on pins to be requested to prevent the > gpio-controller driver from being unloaded while in use. > > [...] > >> Of course, format of the marker "requested" is discussable. Could be: >> - "requested" --> "not-requested" >> - "I R" or "I G" where I - IRQ, G - GPIO, R - requested >> - etc. > > How about instead of > > GPIOs 160-191, platform/4805d000.gpio, gpio: > gpio-171 ((null) ) in hi IRQ209 > > you do something like: > > GPIOs 160-191, platform/4805d000.gpio, gpio: > gpio-171 ( ) in hi IRQ-209 In general agree, but i propose to do it as GPIOs 160-191, platform/4805d000.gpio, gpio: gpio-171 ((null) ) in hi IRQ-209 My intention is - this interface could be considered as more or less stable, so it is better to add additional information at the end of each line to avoid potential breakage of User space SW (test/debug scripts). if you agree I'll update and re-send. >>> The irq-number mapping could perhaps be useful, but it should go in a >>> separate patch. I'd suggest adding a '-' before the irq-number (e.g. >>> "IRQ-209"). >> >> I've thought about this, but finally decided not to split it. >> Could be done, if you insist ) > > Yes, you should split self-contained, logical changes into separate > patches (so they can be reviewed, applied or rejected separately as > well). np. There will be two patches - one to display "irq-only" gpios - second to add IRQ number Thanks. -- regards, -grygorii -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpiolib: debugfs: display gpios requested as irq only
Hi Linus, On 05/19/2015 05:12 PM, Linus Walleij wrote: > On Mon, May 18, 2015 at 5:17 PM, grygorii.stras...@linaro.org > wrote: >> On 05/18/2015 06:08 PM, Johan Hovold wrote: > >>>GPIOs 160-191, platform/4805d000.gpio, gpio: >>>gpio-171 ( ) in hi IRQ-209 >> >> In general agree, but i propose to do it as >> GPIOs 160-191, platform/4805d000.gpio, gpio: >> gpio-171 ((null) ) in hi IRQ-209 >> >> My intention is - this interface could be considered as more or less stable, >> so >> it is better to add additional information at the end of each line to avoid >> potential breakage of User space SW (test/debug scripts). > > What? If I wanted a stable interface I would use sysfs and document > the ABI in Documentation/ABI/*. > > debugfs is not ABI. > > Debugfs is instable by definition, it is not for production. If tests depend > on > it they need to be ready to break and be updated, and in such case > it is a very very good idea to put any such tests in tools/* in the > kernel itself, as does trace-cmd and friends so you can patch the > tests at the same time you patch the code. Okay. Sorry, My comment was not fully correct - keyword was "more or less stable" and of course it is not ABI. Any way, the question is till here - How would it better to do? gpio-171 ( ) in hi IRQ-209 -- or -- gpio-171 ((null) ) in hi IRQ-209 Thanks a lot for your comments. -- regards, -grygorii -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PM / clock_ops: Fix clock error check in __pm_clk_add()
Hi Dmitry,
On 05/09/2015 12:05 AM, Dmitry Torokhov wrote:
> On Fri, May 08, 2015 at 10:59:04PM +0200, Geert Uytterhoeven wrote:
>> On Fri, May 8, 2015 at 7:19 PM, Dmitry Torokhov
>> wrote:
>>> On Fri, May 08, 2015 at 10:47:43AM +0200, Geert Uytterhoeven wrote:
In the final iteration of commit 245bd6f6af8a62a2 ("PM / clock_ops: Add
pm_clk_add_clk()"), a refcount increment was added by Grygorii Strashko.
However, the accompanying IS_ERR() check operates on the wrong clock
pointer, which is always zero at this point, i.e. not an error.
This may lead to a NULL pointer dereference later, when __clk_get()
tries to dereference an error pointer.
Check the passed clock pointer instead to fix this.
>>>
>>> Frankly I would remove the check altogether. Why do we only check for
>>> IS_ERR and not NULL or otherwise validate the pointer? The clk is passed
>>
>> __clk_get() does the NULL check.
>
> No, not really. It _handles_ clk being NULL and returns "everything is
> fine". In any case it is __clk_get's decision what to do.
>
> I dislike gratuitous checks of arguments passed in. Instead of relying
> on APIs refusing grabage we better not pass garbage to these APIs in the
> first place. So I'd change it to trust that we are given a usable
> pointer and simply do:
>
> if (!__clk_get(clk)) {
> kfree(ce);
> return -ENOENTl
> }
Not sure this is right thing to do, because this API initially
was intended to be used as below [1]:
clk = of_clk_get(dev->of_node, i));
ret = pm_clk_add_clk(dev, clk);
clk_put(clk);
and of_clk_get may return ERR_PTR().
So, personally I prefer initial fix from Geert.
[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/296586.html
--
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PM / clock_ops: Fix clock error check in __pm_clk_add()
On 05/12/2015 07:42 PM, Dmitry Torokhov wrote:
> On Tue, May 12, 2015 at 04:55:39PM +0300, grygorii.stras...@linaro.org wrote:
>> On 05/09/2015 12:05 AM, Dmitry Torokhov wrote:
>>> On Fri, May 08, 2015 at 10:59:04PM +0200, Geert Uytterhoeven wrote:
>>>> On Fri, May 8, 2015 at 7:19 PM, Dmitry Torokhov
>>>> wrote:
>>>>> On Fri, May 08, 2015 at 10:47:43AM +0200, Geert Uytterhoeven wrote:
>>>>>> In the final iteration of commit 245bd6f6af8a62a2 ("PM / clock_ops: Add
>>>>>> pm_clk_add_clk()"), a refcount increment was added by Grygorii Strashko.
>>>>>> However, the accompanying IS_ERR() check operates on the wrong clock
>>>>>> pointer, which is always zero at this point, i.e. not an error.
>>>>>> This may lead to a NULL pointer dereference later, when __clk_get()
>>>>>> tries to dereference an error pointer.
>>>>>>
>>>>>> Check the passed clock pointer instead to fix this.
>>>>>
>>>>> Frankly I would remove the check altogether. Why do we only check for
>>>>> IS_ERR and not NULL or otherwise validate the pointer? The clk is passed
>>>>
>>>> __clk_get() does the NULL check.
>>>
>>> No, not really. It _handles_ clk being NULL and returns "everything is
>>> fine". In any case it is __clk_get's decision what to do.
>>>
>>> I dislike gratuitous checks of arguments passed in. Instead of relying
>>> on APIs refusing grabage we better not pass garbage to these APIs in the
>>> first place. So I'd change it to trust that we are given a usable
>>> pointer and simply do:
>>>
>>> if (!__clk_get(clk)) {
>>> kfree(ce);
>>> return -ENOENTl
>>> }
>>
>> Not sure this is right thing to do, because this API initially
>> was intended to be used as below [1]:
>> clk = of_clk_get(dev->of_node, i));
>> ret = pm_clk_add_clk(dev, clk);
>> clk_put(clk);
>>
>> and of_clk_get may return ERR_PTR().
>
> Jeez, that sequence was not meant to be taken literally, it does miss
> error handling completely. If you notice the majority of users of this
> API do something like below:
>
> i = 0;
> while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
> dev_dbg(dev, "adding clock '%s' to list of PM clocks\n",
> __clk_get_name(clk));
> error = pm_clk_add_clk(dev, clk);
> clk_put(clk);
> if (error) {
> dev_err(dev, "pm_clk_add_clk failed %d\n", error);
> pm_clk_destroy(dev);
> return error;
> }
> }
>
> i.e. it already validates clk pointer before passing it on since it
> needs to know when to stop iterating.
np. It's just my opinion - if you agree that code will just crash
in case of passing invalid @clk argument (in worst case:)
int __clk_get(struct clk *clk)
{
struct clk_core *core = !clk ? NULL : clk->core;
^^^ here
let it be.
--
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio: pcf875x: Revert "gpio: pcf857x: Propagate wake-up setting to parent irq controller"
Hi,
On 05/11/2015 08:36 PM, Geert Uytterhoeven wrote:
> On Mon, May 11, 2015 at 4:13 PM, Roger Quadros wrote:
>> commit b80eef95beb0 ('gpio: pcf857x: Propagate wake-up setting to parent irq
>> controller')
>> introduces the following recursive locking warning while suspending dra7-evm.
>>
>> The issue addressed by that commit has been already resolved by
>> commit 10a50f1ab5f0 ('genirq: Set IRQCHIP_SKIP_SET_WAKE flag for
>> dummy_irq_chip')
>
> That's not 100% correct: commit b80eef95beb0 ('gpio: pcf857x: Propagate
> wake-up
> setting to parent irq controller') fixes _two_ things:
>1. warning due to missing irq_set_wake / IRQCHIP_SKIP_SET_WAKE,
>2. propagating set_wake, so the parent interrupt controller stays awake, as
> it's needed for wake-up,
>
> Only the first issue is addressed by commit 10a50f1ab5f0 ('genirq: Set
> IRQCHIP_SKIP_SET_WAKE flag for dummy_irq_chip').
>
>> and so let's revert commit b80eef95beb0 ('gpio: pcf857x: Propagate wake-up
>> setting to parent irq controller')
>>
>> At least the recursive locking message no longer appears after the revert.
>>
>> [ 30.591905] PM: Syncing filesystems ... done.
>> [ 30.623060] Freezing user space processes ... (elapsed 0.003 seconds)
>> done.
>> [ 30.634470] Freezing remaining freezable tasks ... (elapsed 0.002
>> seconds) done.
>> [ 30.658288] sd 0:0:0:0: [sda] Synchronizing SCSI cache
>> [ 30.663678]
>> [ 30.663681] =
>> [ 30.663683] [ INFO: possible recursive locking detected ]
>> [ 30.663688] 4.1.0-rc3 #1115 Not tainted
>> [ 30.663693] -
>> [ 30.663697] suspend.sh/2319 is trying to acquire lock:
>> [ 30.663719] (class){..}, at: []
>> __irq_get_desc_lock+0x48/0x88
>> [ 30.663722]
>> [ 30.663722] but task is already holding lock:
>> [ 30.663734] (class){..}, at: []
>> __irq_get_desc_lock+0x48/0x88
>
> Does this mean .set_irq_wake() cannot call irq_set_irq_wake()?
> Many GPIO drivers do that, as they need to propagate wake-up state to the
> parent interrupt controller?
As I remember, there was similar problem, so I found corresponding patch (just
FYI)
ab2b926 mfd: Fix twl6030 lockdep recursion warning on setting wake IRQs
Not sure such kind of solution is the best choice (
--
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] leds: fix hibernation on arm when gpio-led used with CPU led trigger
Hi Bryan, all,
On 02/05/2015 04:37 PM, grygorii.stras...@linaro.org wrote:
> From: Grygorii Strashko
>
> Setting a dev_pm_ops suspend/resume pair of callbacks but not a set of
> hibernation callbacks means those pm functions will not be
> called upon hibernation - that leads to system crash on ARM during
> freezing if gpio-led is used in combination with CPU led trigger.
> It may happen after freeze_noirq stage (GPIO is suspended)
> and before syscore_suspend stage (CPU led trigger is suspended)
> - usually when disable_nonboot_cpus() is called.
>
> Log:
>PM: noirq freeze of devices complete after 1.425 msecs
>Disabling non-boot CPUs ...
> ^ system may crash or stuck here with message (TI AM572x)
>
>WARNING: CPU: 0 PID: 3100 at drivers/bus/omap_l3_noc.c:148
> l3_interrupt_handler+0x22c/0x370()
>4400.ocp:L3 Custom Error: MASTER MPU TARGET L4_PER1_P3 (Idle): Data
> Access in Supervisor mode during Functional access
>
>CPU1: shutdown
> ^ or here
>
> Fix this by using SIMPLE_DEV_PM_OPS, which appropriately
> assigns the suspend and hibernation callbacks and move
> led_suspend/led_resume under CONFIG_PM_SLEEP to avoid
> build warnings.
>
> Signed-off-by: Grygorii Strashko
> ---
> This patch actually fixes commit:
> 73e1ab4 "leds: Convert led class driver from legacy pm ops to dev_pm_ops"
>
> Changes in v2:
> - fixed compile time warnings when !CONFIG_PM_SLEEP
> - updated commit message
>
> v1:
> - https://lkml.org/lkml/2015/2/2/476
>
> drivers/leds/led-class.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index dbeebac..5033e0d 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -183,6 +183,7 @@ void led_classdev_resume(struct led_classdev *led_cdev)
> }
> EXPORT_SYMBOL_GPL(led_classdev_resume);
>
> +#ifdef CONFIG_PM_SLEEP
> static int led_suspend(struct device *dev)
> {
> struct led_classdev *led_cdev = dev_get_drvdata(dev);
> @@ -202,11 +203,9 @@ static int led_resume(struct device *dev)
>
> return 0;
> }
> +#endif
>
> -static const struct dev_pm_ops leds_class_dev_pm_ops = {
> - .suspend= led_suspend,
> - .resume = led_resume,
> -};
> +static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume);
>
> /**
>* led_classdev_register - register a new object of led_classdev class.
>
Could you pay attention on this patch, pls? It fixes a real issue for me.
Actually, I'd like to note that it fixes regression introduced by commit
73e1ab41a80d36207e8ea7cc2c9897afdeeef387
"leds: Convert led class driver from legacy pm ops to dev_pm_ops" from
Shuah Khan (Jun 20 2013).
--
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: ARM: OMPA4+: is it expected dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64)); to fail?
Hi Russell,
On 03/10/2015 01:05 PM, Russell King - ARM Linux wrote:
> On Fri, Mar 06, 2015 at 11:47:48PM +0200, grygorii.stras...@linaro.org wrote:
>> On 03/05/2015 10:17 PM, Russell King - ARM Linux wrote:
>>> On Thu, Mar 05, 2015 at 08:55:07PM +0200, grygorii.stras...@linaro.org
>>> wrote:
>>>> The dma_coerce_mask_and_coherent() will fail in case 'Example 3' and
>>>> succeed in cases 1,2.
>>>> dma-mapping.c --> __dma_supported()
>>>>if (sizeof(mask) != sizeof(dma_addr_t) && <== true for all OMAP4+
>>>>mask > (dma_addr_t)~0 &&<== true for DMA_BIT_MASK(64)
>>>>dma_to_pfn(dev, ~0) < max_pfn) { <== true only for Example 3
>>>
>>> Hmm, I think this may make more sense to be "< max_pfn - 1" here, as
>>> that would be better suited to our intention.
>>>
>>> The result of dma_to_pfn(dev, ~0) is the maximum PFN which we could
>>> address via DMA, but we're comparing it with the maximum PFN in the
>>> system plus 1 - so we need to subtract one from it.
>>
>> Ok. I'll try it.
>
> Any news on this - I think it is a real off-by-one bug which we should
> fix in any case.
Sorry for delay, there was a day-off on my side.
As per my test results - with above change
dma_coerce_mask_and_coherent(DMA_BIT_MASK(64)) and friends will succeed always.
=== Test results:
Test case 1:
Input data:
- RAM: start = 0x8000 size = 0x8000
- CONFIG_ARM_LPAE=n and sizeof(phys_addr_t) = 4
a) NO changes:
memory registered within memblock as:
memory.cnt = 0x1
memory[0x0] [0x008000-0x00fffe], 0x7fff bytes flags:
0x0
max_pfn = 0xF
max_mapnr = 0x7
dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); -- succeeded
b) with change in __dma_supported():
if (sizeof(mask) != sizeof(dma_addr_t) &&
mask > (dma_addr_t)~0 &&
- dma_to_pfn(dev, ~0) < max_pfn) {
+ dma_to_pfn(dev, ~0) < (max_pfn - 1)) {
if (warn) {
memory registered within memblock as:
memory.cnt = 0x1
memory[0x0] [0x008000-0x00fffe], 0x7fff bytes flags:
0x0
max_pfn = 0xF
max_mapnr = 0x7
dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); -- succeeded
Test case 2:
Input data:
- RAM: start = 0x8000 size = 0x8000
- CONFIG_ARM_LPAE=y and sizeof(phys_addr_t) = 8
a) NO changes:
memory registered within memblock as:
memory.cnt = 0x1
memory[0x0] [0x008000-0x00], 0x8000 bytes flags:
0x0
max_pfn = 0x10
max_mapnr = 0x8
dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); -- failed
[5.468470] asoc-simple-card sound@0: Coherent DMA mask 0x
is larger than dma_addr_t allows
[5.478706] asoc-simple-card sound@0: Driver did not use or check the return
value from dma_set_coherent_mask()?
[5.496620] davinci-mcasp 48468000.mcasp: ASoC: pcm constructor failed: -5
[5.503844] asoc-simple-card sound@0: ASoC: can't create pcm
davinci-mcasp.0-tlv320aic3x-hifi :-5
b) with change in __dma_supported():
if (sizeof(mask) != sizeof(dma_addr_t) &&
mask > (dma_addr_t)~0 &&
- dma_to_pfn(dev, ~0) < max_pfn) {
+ dma_to_pfn(dev, ~0) < (max_pfn - 1)) {
if (warn) {
memory registered within memblock as:
memory.cnt = 0x1
memory[0x0] [0x008000-0x00], 0x8000 bytes flags:
0x0
max_pfn = 0x10
max_mapnr = 0x8
dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); -- succeeded
regards,
-grygorii
--
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: ARM: OMPA4+: is it expected dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64)); to fail?
Hi Arnd,
On 03/09/2015 11:33 PM, Arnd Bergmann wrote:
> On Thursday 05 March 2015 20:55:07 grygorii.stras...@linaro.org wrote:
>> Hi All,
>>
>> Now I can see very interesting behavior related to
>> dma_coerce_mask_and_coherent()
>> and friends which I'd like to explain and clarify.
>>
>> Below is set of questions I have (why - I explained below):
>> - Is expected dma_coerce_mask_and_coherent(DMA_BIT_MASK(64)) and friends to
>> fail on 32 bits HW?
>
> No. dma_coerce_mask_and_coherent() is meant to ignore the actual mask. It's
> usually considered a bug to use this function for that reason.
>
>> - What is expected value for max_pfn: max_phys_pfn or max_phys_pfn + 1?
>>
>> - What is expected value for struct memblock_region->size: mem_range_size or
>> mem_range_size - 1?
>>
>> - What is expected value to be returned by memblock_end_of_DRAM():
>>@base + @size(max_phys_addr + 1) or @base + @size - 1(max_phys_addr)?
>>
>>
>> I'm working with BeaglBoard-X15 (AM572x/DRA7xx) board and have following
>> code in OMAP ASOC driver
>> which is failed SOMETIMES during the boot with error -EIO.
>> === to omap-pcm.c:
>> omap_pcm_new() {
>> ...
>> ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(64));
>> ^^ failed sometimes
>> if (ret)
>> return ret;
>> }
>
> The code should be fixed to use dma_set_mask_and_coherent(), which is
> expected to
> fail if the bus is incapable of addressing all RAM within the mask.
>
>> I'd be very appreciated for any comments/clarification on questions I've
>> listed at the
>> beginning of my e-mail - there are no patches from my side as I'd like to
>> understand
>> expected behavior of the kernel first (especially taking into account that
>> any
>> memblock changes might affect on at least half of arches).
>
> Is the device you have actually 64-bit capable?
>
> Is the bus it is connected to 64-bit wide?
As I mentioned before - The device was fixed by switching to use 32 bit mask
"The issue with omap-pcm was simply fixed by using DMA_BIT_MASK(32), ".
>
> Does the dma-ranges property of the parent bus reflect the correct address
> width?
dma-ranges is not used and all devices are created with default mask
DMA_BIT_MASK(32);
My goal was to clarify above questions (first of all), because on my HW I can
see
different values of max_pfn, max_mapnr and memblock configuration depending on
CONFIG_ARM_LPAE=n|y and when RAM is defined as: start = 0x8000 size =
0x8000.
(and also between kernels 3.14 and LKML).
Looks like such RAM configuration is a corner case, which is not always handled
as expected
(and how is it expected to be handled?).
For example:
before commit ARM: 8025/1: Get rid of meminfo
- registered RAM start = 0x8000 size = 0x8000 will be adjusted by
arm_add_memory()
and final RAM configuration will be start = 0x8000 size = 0x7000
after this commit:
- code will try to register start = 0x8000 size = 0x8000, but memblock
will
adjust it to start = 0x8000 size = 0x7fff.
--
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 0/8] gpio: omap: cleanup: get rid of system GPIO <-> GPIO offset converseations
On 03/23/2015 02:18 PM, grygorii.stras...@linaro.org wrote: From: Grygorii Strashko Now in TI OMAP GPIO driver there are a lot of places where System GPIO number calculated and then converted to GPIO offset. What is worse is that in many place such conversation performed twice or even three times. But actually, we don't need to do that at all, because - gpiolib always passes GPIO offset to GPIO controller - OMAP GPIO driver converted to use IRQ domain, so struct irq_data->hwirq contains GPIO offset Hence, it is safe to convert all GPIO OMAP functions to use GPIO offset instead of system GPIO numbers. Also, this allows to remove unneeded conversations routines #define GPIO_INDEX(bank, gpio) #define GPIO_BIT(bank, gpio) int omap_irq_to_gpio() Tested on: - dra7-evm. - omap1 (osk5912), 770 and E3. Last two patches have to be tested on OMAP1: - gpio: omap: get rid of omap_irq_to_gpio() - gpio: omap: get rid of GPIO_INDEX() macro Based on top of Linux 4.0-rc4 plus patch '[PATCH 1/2] gpio: omap: irq_shutdown: remove unnecessary call of gpiochip_unlock_as_irq' http://www.spinics.net/lists/linux-omap/msg116482.html Changes in v2: - fixed build failure with Patch 5, no functional code changes. Tested-by: Tony Lindgren Tested-by: Aaro Koskinen Acked-by: Santosh Shilimkar Acked-by: Javier Martinez Canillas Thanks Linus. regards, -grygorii -- regards, -grygorii -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
ARM: OMPA4+: is it expected dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64)); to fail?
Hi All,
Now I can see very interesting behavior related to
dma_coerce_mask_and_coherent()
and friends which I'd like to explain and clarify.
Below is set of questions I have (why - I explained below):
- Is expected dma_coerce_mask_and_coherent(DMA_BIT_MASK(64)) and friends to
fail on 32 bits HW?
- What is expected value for max_pfn: max_phys_pfn or max_phys_pfn + 1?
- What is expected value for struct memblock_region->size: mem_range_size or
mem_range_size - 1?
- What is expected value to be returned by memblock_end_of_DRAM():
@base + @size(max_phys_addr + 1) or @base + @size - 1(max_phys_addr)?
I'm working with BeaglBoard-X15 (AM572x/DRA7xx) board and have following code
in OMAP ASOC driver
which is failed SOMETIMES during the boot with error -EIO.
=== to omap-pcm.c:
omap_pcm_new() {
...
ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(64));
^^ failed sometimes
if (ret)
return ret;
}
What I can see is that dma_coerce_mask_and_coherent() and etc may fail or
succeed
depending on - max_pfn value.
-> max_pfn value depends on memblock configuration
max_pfn = max_high = PFN_DOWN(memblock_end_of_DRAM());
|- PFN_DOWN(memblock.memory.regions[last_idx].base +
memblock.memory.regions[last_idx].size)
-> memblock configuration depends on
a) CONFIG_ARM_LPAE=y|n (my system really works with 32 bit address space)
b) RAM configuration
Example 1 CONFIG_ARM_LPAE=n:
memory {
device_type = "memory";
reg = <0x8000 0x6000>; /* 1536 MB */
};
memblock will be configured as:
memory.cnt = 0x1
memory[0x0] [0x008000-0x00dfff], 0x6000 bytes
flags: 0x0
^^
max_pfn = 0x000E
Example 2 CONFIG_ARM_LPAE=n:
memory {
device_type = "memory";
reg = <0x8000 0x8000>;
};
memblock will be configured as:
memory.cnt = 0x1
memory[0x0] [0x008000-0x00fffe], 0x7fff bytes
flags: 0x0
^^
max_pfn = 0x000F
Example 3 CONFIG_ARM_LPAE=y (but system really works with 32 bit address space):
memory {
device_type = "memory";
reg = <0x8000 0x8000>;
};
memblock will be configured as:
memory.cnt = 0x1
memory[0x0] [0x008000-0x00], 0x8000 bytes
flags: 0x0
^^
max_pfn = 0x0010
The dma_coerce_mask_and_coherent() will fail in case 'Example 3' and succeed in
cases 1,2.
dma-mapping.c --> __dma_supported()
if (sizeof(mask) != sizeof(dma_addr_t) && <== true for all OMAP4+
mask > (dma_addr_t)~0 &&<== true for DMA_BIT_MASK(64)
dma_to_pfn(dev, ~0) < max_pfn) { <== true only for Example 3
I've tracked down patch which changes memblock behavior to:
commit eb18f1b5bfb99b1d7d2f5d792e6ee5c9b7d89330
Author: Tejun Heo
Date: Thu Dec 8 10:22:07 2011 -0800
memblock: Make memblock functions handle overflowing range @size
This commit is pretty old :( and it doesn't takes into account LPAE mode
where phys_addr_t is 64 bit, but physically accessible addresses <= 40 bit
(memblock_cap_size()).
The issue with omap-pcm was simply fixed by using DMA_BIT_MASK(32), but It
seems problem is
wider and above behavior of dma_set_maskX() and memblock confused me a bit.
I'd be very appreciated for any comments/clarification on questions I've listed
at the
beginning of my e-mail - there are no patches from my side as I'd like to
understand
expected behavior of the kernel first (especially taking into account that any
memblock changes might affect on at least half of arches).
Thanks.
Additional info:
memblock: Make memblock functions handle overflowing range @size
https://lkml.org/lkml/2011/7/26/235
[alsa-devel] [PATCH] ASoC: omap-pcm: Lower the dma coherent mask to 32bits
http://mailman.alsa-project.org/pipermail/alsa-devel/2013-December/069817.html
--
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: ARM: OMPA4+: is it expected dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64)); to fail?
Hi Russell,
On 03/05/2015 10:17 PM, Russell King - ARM Linux wrote:
> On Thu, Mar 05, 2015 at 08:55:07PM +0200, grygorii.stras...@linaro.org wrote:
>> Now I can see very interesting behavior related to
>> dma_coerce_mask_and_coherent()
>> and friends which I'd like to explain and clarify.
>>
>> Below is set of questions I have (why - I explained below):
>> - Is expected dma_coerce_mask_and_coherent(DMA_BIT_MASK(64)) and friends to
>> fail on 32 bits HW?
>
> Not really.
>
>> - What is expected value for max_pfn: max_phys_pfn or max_phys_pfn + 1?
>
> mm/page_owner.c:
> /* Find an allocated page */
> for (; pfn < max_pfn; pfn++) {
>
> drivers/base/platform.c:u32 low_totalram = ((max_pfn - 1) << PAGE_SHIFT);
> drivers/base/platform.c:u32 high_totalram = ((max_pfn - 1) >> (32 -
> PAGE_SHIFT));
>
> So, there's ample evidence that max_pfn is one more than the greatest pfn
> which may be used in the system.
>
>> - What is expected value for struct memblock_region->size: mem_range_size or
>> mem_range_size - 1?
>
> A size is a size - it's a number of bytes contained within the region.
> If it is value 1, then there is exactly one byte in the region. If
> there are 0x7fff, then there are 2G-1 bytes in the region, not 2G.
Thanks - it seems clear now.
>> - What is expected value to be returned by memblock_end_of_DRAM():
>>@base + @size(max_phys_addr + 1) or @base + @size - 1(max_phys_addr)?
>
> The last address plus one in the system. However, there's a problem here.
> On a 32-bit system, phys_addr_t may be 32-bit. If it is 32-bit, then
> "last address plus one" could be zero, which makes no sense. Hence, it
> is artificially reduced to 0xf000, thereby omitting the final page.
^ this part seems not fully true now, because for ARM32 + DT the
fdt.c->early_init_dt_add_memory_arch() is called instead of arm_add_memory()
and it works in a different way a bit.
For example, I don't see below message when reg = <0x8000 0x8000>:
"Truncating memory at 0x8000 to fit in 32-bit physical address space"
instead memblock silently configured as
memory.cnt = 0x1
memory[0x0].base = 0x8000
memory[0x0].size = 0x7fff
>
>> Example 3 CONFIG_ARM_LPAE=y (but system really works with 32 bit address
>> space):
>> memory {
>> device_type = "memory";
>> reg = <0x8000 0x8000>;
>> };
>>
>>memblock will be configured as:
>> memory.cnt = 0x1
>> memory[0x0] [0x008000-0x00], 0x8000 bytes
>> flags: 0x0
>> ^^
>>max_pfn = 0x0010
>>
>> The dma_coerce_mask_and_coherent() will fail in case 'Example 3' and succeed
>> in cases 1,2.
>> dma-mapping.c --> __dma_supported()
>> if (sizeof(mask) != sizeof(dma_addr_t) && <== true for all OMAP4+
>> mask > (dma_addr_t)~0 &&<== true for DMA_BIT_MASK(64)
>> dma_to_pfn(dev, ~0) < max_pfn) { <== true only for Example 3
>
> Hmm, I think this may make more sense to be "< max_pfn - 1" here, as
> that would be better suited to our intention.
>
> The result of dma_to_pfn(dev, ~0) is the maximum PFN which we could
> address via DMA, but we're comparing it with the maximum PFN in the
> system plus 1 - so we need to subtract one from it.
Ok. I'll try it.
>
> Please think about this and test this out; I'm not back to normal yet
> (post-op) so I could very well not be thinking straight yet.
Thanks for your comments. I hope you feel better.
--
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] leds: fix hibernation on arm when gpio-led used with CPU led trigger
Hi All,
On 02/24/2015 06:12 PM, grygorii.stras...@linaro.org wrote:
On 02/05/2015 04:37 PM, grygorii.stras...@linaro.org wrote:
From: Grygorii Strashko
Setting a dev_pm_ops suspend/resume pair of callbacks but not a set of
hibernation callbacks means those pm functions will not be
called upon hibernation - that leads to system crash on ARM during
freezing if gpio-led is used in combination with CPU led trigger.
It may happen after freeze_noirq stage (GPIO is suspended)
and before syscore_suspend stage (CPU led trigger is suspended)
- usually when disable_nonboot_cpus() is called.
Log:
PM: noirq freeze of devices complete after 1.425 msecs
Disabling non-boot CPUs ...
^ system may crash or stuck here with message (TI AM572x)
WARNING: CPU: 0 PID: 3100 at drivers/bus/omap_l3_noc.c:148
l3_interrupt_handler+0x22c/0x370()
4400.ocp:L3 Custom Error: MASTER MPU TARGET L4_PER1_P3 (Idle): Data
Access in Supervisor mode during Functional access
CPU1: shutdown
^ or here
Fix this by using SIMPLE_DEV_PM_OPS, which appropriately
assigns the suspend and hibernation callbacks and move
led_suspend/led_resume under CONFIG_PM_SLEEP to avoid
build warnings.
Signed-off-by: Grygorii Strashko
---
This patch actually fixes commit:
73e1ab4 "leds: Convert led class driver from legacy pm ops to dev_pm_ops"
On 02/24/2015 06:12 PM, grygorii.stras...@linaro.org wrote:
Changes in v2:
- fixed compile time warnings when !CONFIG_PM_SLEEP
- updated commit message
v1:
- https://lkml.org/lkml/2015/2/2/476
drivers/leds/led-class.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index dbeebac..5033e0d 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -183,6 +183,7 @@ void led_classdev_resume(struct led_classdev *led_cdev)
}
EXPORT_SYMBOL_GPL(led_classdev_resume);
+#ifdef CONFIG_PM_SLEEP
static int led_suspend(struct device *dev)
{
struct led_classdev *led_cdev = dev_get_drvdata(dev);
@@ -202,11 +203,9 @@ static int led_resume(struct device *dev)
return 0;
}
+#endif
-static const struct dev_pm_ops leds_class_dev_pm_ops = {
- .suspend= led_suspend,
- .resume = led_resume,
-};
+static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume);
/**
* led_classdev_register - register a new object of led_classdev class.
Could you pay attention on this patch, pls? It fixes a real issue for me.
Actually, I'd like to note that it fixes regression introduced by commit
73e1ab41a80d36207e8ea7cc2c9897afdeeef387
"leds: Convert led class driver from legacy pm ops to dev_pm_ops" from
Shuah Khan (Jun 20 2013).
Gentle reminder. Any comments on this?
--
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/5] i2c: recovery: change input parameter to i2c_adapter for prepare/unprepare_recovery
Hi Wolfram,
On 03/12/2015 01:32 PM, Alexander Sverdlin wrote:
> On 01/12/14 16:34, Grygorii Strashko wrote:
>> This patch changes type of input parameter for .prepare/unprepare_recovery()
>> callbacks from struct i2c_bus_recovery_info * to struct i2c_adapter *.
>> This allows to simplify implementation of these callbacks and avoid
>> type conversations from i2c_bus_recovery_info to i2c_adapter.
>> The i2c_bus_recovery_info can be simply retrieved from struct i2c_adapter
>> which contains pointer on it.
>>
>> CC: Sekhar Nori
>> CC: Kevin Hilman
>> CC: Santosh Shilimkar
>> CC: Murali Karicheri
>> Acked-by: Uwe Kleine-König
>> Signed-off-by: Grygorii Strashko
>> ---
>> drivers/i2c/i2c-core.c | 4 ++--
>> include/linux/i2c.h| 4 ++--
>> 2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> index 2f90ac6..72b6e34 100644
>> --- a/drivers/i2c/i2c-core.c
>> +++ b/drivers/i2c/i2c-core.c
>> @@ -563,7 +563,7 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
>> int i = 0, val = 1, ret = 0;
>>
>> if (bri->prepare_recovery)
>> -bri->prepare_recovery(bri);
>> +bri->prepare_recovery(adap);
>>
>> /*
>> * By this time SCL is high, as we need to give 9 falling-rising edges
>> @@ -588,7 +588,7 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
>> }
>>
>> if (bri->unprepare_recovery)
>> -bri->unprepare_recovery(bri);
>> +bri->unprepare_recovery(adap);
>>
>> return ret;
>> }
>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>> index b556e0a..cf9380f 100644
>> --- a/include/linux/i2c.h
>> +++ b/include/linux/i2c.h
>> @@ -404,8 +404,8 @@ struct i2c_bus_recovery_info {
>> void (*set_scl)(struct i2c_adapter *, int val);
>> int (*get_sda)(struct i2c_adapter *);
>>
>> -void (*prepare_recovery)(struct i2c_bus_recovery_info *bri);
>> -void (*unprepare_recovery)(struct i2c_bus_recovery_info *bri);
>> +void (*prepare_recovery)(struct i2c_adapter *);
>> +void (*unprepare_recovery)(struct i2c_adapter *);
>>
>> /* gpio recovery */
>> int scl_gpio;
>>
>
> Acked-by: Alexander Sverdlin
>
Could I ask you to consider this patch for 4.1?
Now my I2C Davinci series is based on it and probably I2C OMAP changes
will need to be based on it too - if Felipe would agree with this of course.
regards,
-grygorii
--
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
