* [PATCH v2 0/3] add CNS3xxx AHCI support
From: Lin Mac @ 2011-01-06 6:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <4D24B53C.4020009@pobox.com>
2011/1/6 Jeff Garzik <jgarzik@pobox.com>:
> On 01/05/2011 12:43 AM, mkl0301 at gmail.com wrote:
>>
>> From: Mac Lin<mkl0301@gmail.com>
>>
>> v2:
>> ?- Switch ahci_platform to module device table matching to add SoC
>> specific support
>>
>> v1: http://www.spinics.net/lists/arm-kernel/msg106236.html
>> ?- Add CNS3xxx SoC specific AHCI support
>>
>> This patchset is based on linux-2.6.37-rc2
>>
>> Mac Lin (3):
>> ? ? ? ahci_platform: rename to ahci_pltfm, but keep the original module
>> name
>> ? ? ? ahci_pltfm: switch to module device table matching
>> ? ? ? ahci_platform: add support for CNS3xxx SoC devices
>>
>> ?arch/arm/mach-cns3xxx/devices.c | ? ?2 +-
>> ?drivers/ata/Kconfig ? ? ? ? ? ? | ? 11 ++
>> ?drivers/ata/Makefile ? ? ? ? ? ?| ? ?5 +-
>> ?drivers/ata/ahci_cns3xxx.c ? ? ?| ? 62 +++++++++++
>> ?drivers/ata/ahci_platform.c ? ? | ?197
>> ------------------------------------
>> ?drivers/ata/ahci_pltfm.c ? ? ? ?| ?212
>> +++++++++++++++++++++++++++++++++++++++
>> ?drivers/ata/ahci_pltfm.h ? ? ? ?| ? 19 ++++
>> ?7 files changed, 309 insertions(+), 199 deletions(-)
>> ?create mode 100644 drivers/ata/ahci_cns3xxx.c
>> ?delete mode 100644 drivers/ata/ahci_platform.c
>> ?create mode 100644 drivers/ata/ahci_pltfm.c
>> ?create mode 100644 drivers/ata/ahci_pltfm.h
>
> It is overkill to rename the entirety of ahci_platform just for one override
> function.
> This sort of thing I would have expected to be added directly to
> ahci_platform.c.
It might be overkill for only one controller. but it is more clean and
readable to have different SoC specific changes in separate files,
especially when more SoCs need to make similar changes.
I will add them directly to ahci_platform.c if you insist.
Best Regards,
Mac Lin
^ permalink raw reply
* [PATCH 5/5] S5PC110: add MIPI-DSI based sample lcd panel driver.
From: InKi Dae @ 2011-01-06 6:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20110106051458.GP23889@linux-sh.org>
thank you for your reviews.
all the problems you pointed out would be corrected for actual lcd
panel driver.
(this lcd panel driver has being worked locally and it will be posted
in the future)
after this patch, I posted next patches.
refer to below please.
http://permalink.gmane.org/gmane.linux.ports.arm.kernel/101521
in this patch, I removed sample driver and also have code compacting
and refactoring for MIPI-DSI driver. so could you please give me your
review about this one?
2011/1/6 Paul Mundt <lethal@linux-sh.org>:
> On Tue, Dec 28, 2010 at 08:26:35PM +0900, Inki Dae wrote:
>> this patch addes MIPI-DSI based sample panel driver.
>> to write MIPI-DSI based lcd panel driver, you can refer to
>> this sample driver.
>>
>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
>
> Sample drivers are ok, but unless it has some sort of practical run-time
> use you are probably better off just including this along with your
> subsystem/platform documentation in Documentation somewhere. You can
> search for .c files there to see how others are doing it.
>
> Having said that ..
>
>> diff --git a/drivers/video/s5p_mipi_sample.c b/drivers/video/s5p_mipi_sample.c
>> new file mode 100644
>> index 0000000..8a8abfe
>> --- /dev/null
>> +++ b/drivers/video/s5p_mipi_sample.c
>> @@ -0,0 +1,220 @@
>> +/* linux/drivers/video/sample.c
>> + *
> This is precisely why file comments are useless, since they invariably
> fail to match up.
>
>> + * MIPI-DSI based sample AMOLED lcd panel driver.
>> + *
>> + * Inki Dae, <inki.dae@samsung.com>
>> + *
> No Copyright notice?
>
>> +static void sample_long_command(struct sample *lcd)
>> +{
>> + ? ? struct dsim_master_ops *ops = lcd_to_master_ops(lcd);
>> + ? ? unsigned char data_to_send[5] = {
>> + ? ? ? ? ? ? 0x00, 0x00, 0x00, 0x00, 0x00
>> + ? ? };
>> +
>> + ? ? ops->cmd_write(lcd_to_master(lcd), MIPI_DSI_DCS_LONG_WRITE,
>> + ? ? ? ? ? ? (unsigned int) data_to_send, sizeof(data_to_send));
>> +}
>> +
>> +static void sample_short_command(struct sample *lcd)
>> +{
>> + ? ? struct dsim_master_ops *ops = lcd_to_master_ops(lcd);
>> +
>> + ? ? ops->cmd_write(lcd_to_master(lcd), MIPI_DSI_DCS_SHORT_WRITE_PARAM,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x00, 0x00);
>> +}
>> +
> ops->cmd_write() can fail for any number of reasons, so you will want to
> change these to make sure that you are actually handling the error cases.
>
>> +static int sample_panel_init(struct sample *lcd)
>> +{
>> + ? ? sample_long_command(lcd);
>> + ? ? sample_short_command(lcd);
>> +
>> + ? ? return 0;
>
> At which point you can fail the initialization instead of just blowing up
> later.
>
>> +static int sample_gamma_ctrl(struct sample *lcd, int gamma)
>> +{
>> + ? ? struct dsim_master_ops *ops = lcd_to_master_ops(lcd);
>> +
>> + ? ? /* change transfer mode to LP mode */
>> + ? ? if (ops->change_dsim_transfer_mode)
>> + ? ? ? ? ? ? ops->change_dsim_transfer_mode(lcd_to_master(lcd), 0);
>> +
> ops->change_dsim_transfer_mode() can also fail. You could do this more
> cleanly as:
>
> enum { DSIM_XFER_LP, DSIM_XFER_HS };
>
> static inline int dsim_set_xfer_mode(struct mipi_dsim_device *dsim, int mode)
> {
> ? ? ? ?struct mipi_dsim_master_ops *ops = dsim->master_ops;
>
> ? ? ? ?if (!ops->change_dsim_transfer_mode)
> ? ? ? ? ? ? ? ?return -ENOSYS; /* not implemented */
>
> ? ? ? ?return ops->change_dsim_transfer_mode(dsim, mode);
> }
>
> Then simply do your sample_gamma_ctrl() as:
>
> ? ? ? ?ret = dsim_set_xfer_mode(dsim, DSIM_XFER_LP);
> ? ? ? ?if (ret != 0)
> ? ? ? ? ? ? ? ?return ret;
>
>> + ? ? /* update gamma table. */
>> +
>
> ? ? ? ?return dsim_set_xfer_mode(dsim, DSIM_XFER_HS);
>> +}
>> +
>
> Or something similar. Your sample code should really be as
> self-documenting and error-proof as possible. You don't really want to be
> in a situation where subtle bugs leak through that then everyone who uses
> this code as a reference will carry over!
>
>> +static int sample_set_brightness(struct backlight_device *bd)
>> +{
>> + ? ? int ret = 0, brightness = bd->props.brightness;
>> + ? ? struct sample *lcd = bl_get_data(bd);
>> +
>> + ? ? if (brightness < MIN_BRIGHTNESS ||
>> + ? ? ? ? ? ? brightness > bd->props.max_brightness) {
>> + ? ? ? ? ? ? dev_err(lcd->dev, "lcd brightness should be %d to %d.\n",
>> + ? ? ? ? ? ? ? ? ? ? MIN_BRIGHTNESS, MAX_BRIGHTNESS);
>> + ? ? ? ? ? ? return -EINVAL;
>> + ? ? }
>> +
>> + ? ? ret = sample_gamma_ctrl(lcd, bd->props.brightness);
>> + ? ? if (ret) {
>> + ? ? ? ? ? ? dev_err(&bd->dev, "lcd brightness setting failed.\n");
>> + ? ? ? ? ? ? return -EIO;
>> + ? ? }
>> +
> With your current approach this error condition will never be reached,
> for example.
>
>> +static int sample_probe(struct mipi_lcd_device *mipi_dev)
>> +{
>> + ? ? struct sample *lcd = NULL;
>> + ? ? struct backlight_device *bd = NULL;
>> +
>> + ? ? lcd = kzalloc(sizeof(struct sample), GFP_KERNEL);
>> + ? ? if (!lcd) {
>> + ? ? ? ? ? ? dev_err(&mipi_dev->dev, "failed to allocate sample structure.\n");
>> + ? ? ? ? ? ? return -ENOMEM;
>> + ? ? }
>> +
>> + ? ? lcd->mipi_dev = mipi_dev;
>> + ? ? lcd->ddi_pd =
>> + ? ? ? ? ? ? (struct mipi_ddi_platform_data *)device_to_ddi_pd(mipi_dev);
>
> Does this really need to be casted?
>
>> + ? ? lcd->dev = &mipi_dev->dev;
>> +
>> + ? ? dev_set_drvdata(&mipi_dev->dev, lcd);
>> +
>> + ? ? bd = backlight_device_register("sample-bl", lcd->dev, lcd,
>> + ? ? ? ? ? ? &sample_backlight_ops, NULL);
>> +
>> + ? ? lcd->bd = bd;
>> +
> And here you have no error checking for backlight registration, so you
> will get a NULL pointer deref:
>
>> + ? ? bd->props.max_brightness = MAX_BRIGHTNESS;
>> + ? ? bd->props.brightness = MAX_BRIGHTNESS;
>> +
> here. backlight_device_register() returns an ERR_PTR(), so you will want
> to do an IS_ERR() check, which you can then map back with PTR_ERR() for a
> more precise idea of why it failed.
>
>> + ? ? /* lcd power on */
>> + ? ? if (lcd->ddi_pd->lcd_power_on)
>> + ? ? ? ? ? ? lcd->ddi_pd->lcd_power_on(NULL, 1);
>> +
> You may wish to use enums for this too. It's not strictly necessary, but
> it does help to clarify which is the on and which is the off position.
>
>> + ? ? mdelay(lcd->ddi_pd->reset_delay);
>> +
>> + ? ? /* lcd reset */
>> + ? ? if (lcd->ddi_pd->lcd_reset)
>> + ? ? ? ? ? ? lcd->ddi_pd->lcd_reset(NULL);
>> +
> Reset can fail?
>
>> + ? ? sample_panel_init(lcd);
>> +
>> + ? ? return 0;
>> +}
> sample_panel_init() can fail as well, and in both cases you will need to
> clean up all of the above work.
>
>> +
>> +#ifdef CONFIG_PM
>> +static int sample_suspend(struct mipi_lcd_device *mipi_dev)
>> +{
>> + ? ? struct sample *lcd = dev_get_drvdata(&mipi_dev->dev);
>> +
>> + ? ? /* some work. */
>> +
>> + ? ? mdelay(lcd->ddi_pd->power_off_delay);
>> +
> Adding delays in the suspend/resume path sounds like a pretty bad idea,
> is there a technical reason for it? If so, please document it, so people
> get some idea of where their suspend/resume latencies are coming from,
> and why.
>
>> +static struct mipi_lcd_driver sample_mipi_driver = {
>> + ? ? .name = "sample",
>> +
>> + ? ? .probe = sample_probe,
>
> No remove?
>
>> + ? ? .suspend = sample_suspend,
>> + ? ? .resume = sample_resume,
>> +};
>> +
>> +static int sample_init(void)
>> +{
>> + ? ? s5p_mipi_register_lcd_driver(&sample_mipi_driver);
>> +
>> + ? ? return 0;
>> +}
>> +
> This should be:
>
> ? ? ? ?return s5p_mipi_register_lcd_driver(&sample_mipi_driver);
>
> And sample_init should be __init annotated.
>
>> +static void sample_exit(void)
>> +{
>> + ? ? return;
>> +}
>> +
> This should be balanced with a
>
> ? ? ? ?s5p_mipi_unregister_lcd_driver(&sample_mipi_driver);
>
>> +module_init(sample_init);
>> +module_exit(sample_exit);
>> +
>> +MODULE_AUTHOR("Inki Dae <inki.dae@samsung.com>");
>> +MODULE_DESCRIPTION("MIPI-DSI based sample AMOLED LCD Panel Driver");
>> +MODULE_LICENSE("GPL");
>
> Since you have a fairly complex subsystem it's probably a good idea to
> work out how your MODULE_ALIAS() is going to work, so that you can handle
> autoprobe for these things via udev. The fact you have no exit path
> definitely suggests you haven't tested this in a modular configuration,
> so there is probably going to be quite a bit of work to do here.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply
* [PATCH 1/1] arm: omap: gpio: define .disable callback for gpio irq chip
From: Eduardo Valentin @ 2011-01-06 6:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <87aajfvstw.fsf@ti.com>
On Wed, Jan 05, 2011 at 03:22:51PM -0800, ext Kevin Hilman wrote:
> Eduardo Valentin <eduardo.valentin@nokia.com> writes:
>
> > Hello Russell,
> >
> > On Wed, Jan 05, 2011 at 06:19:18PM +0000, Russell King wrote:
> >> On Wed, Jan 05, 2011 at 07:58:03PM +0200, Eduardo Valentin wrote:
> >> > Currently, if one calls disable_irq(gpio_irq), the irq
> >> > won't get disabled.
> >> >
> >> > This is happening because the omap gpio code defines only
> >> > a .mask callback. And the default_disable function is just
> >> > a stub. The result is that, when someone calls disable_irq
> >> > for an irq in a gpio line, it will be kept enabled.
> >> >
> >> > This patch solves this issue by setting the .disable
> >> > callback to point to the same .mask callback.
> >>
> >> Amd this is a problem because?
> >
> > errr.. because the interrupt is enabled when it was supposed to be disabled?
> >
>
> As Russell pointed out, it's not actually "supposed" to be.
>
> With lazy disable, what disable_irq() does is prevent the *handler* from
> ever being called. If another interrupt arrives, it will be caught by
> the genirq core, marked as IRQ_PENDING and then masked. This "don't
> disable unless we really have to" is the desired behavior of the lazy
> disable feature.
Right. I'm convinced that the handler won't be called because of the lazy
disable mechanism.
>
> >>
> >> The way this works is that although it isn't disabled at that point,
> >> if it never triggers, then everything remains happy. However, if it
> >> does trigger, the genirq code will then mask the interrupt and won't
> >> call the handler.
> >
> > Right.. I didn't see from this point. I will check how that gets unmasked.
> > But even so, if I understood correctly what you described, it would still
> > open a time window which the system would see at least 1 interrupt during
> > the time it was not suppose to. And that can wakeup a system which is in
> > deep sleep mode, either via dynamic idle or static suspend.
> >
> > It is unlikely, I know. But it can still happen. And can be avoided.
>
> If the GPIO is configured as a wakeup source, then wouldn't you want
> activity on that GPIO to wake up the system?
Yes I would want it.. of course, if the interrupt is enabled though..
I'm still trying to find a valid situation where someone disables an irq
but still wants its activity to be a wakeup source. I couldn't find so far..
>
> If you don't want wakeups on that GPIO, then the driver should probably
> be using disable_irq_wake().
Yes. Let's take this situation. Let's assume a driver, at its suspend callback,
explicitly reports to the system that its irq can be disabled and also should
not be seen as a wakeup source, by calling disable_irq(gpio_irq) and
disable_irq_wake(gpio_irq).
What should be the expected system behavior when the user says echo mem > /sys/power/state?
^ permalink raw reply
* [PATCH] ARM i.MX53: enable fec driver on EVK board
From: Baruch Siach @ 2011-01-06 6:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1294292731-2896-1-git-send-email-yong.shen@freescale.com>
Hi Yong,
On Thu, Jan 06, 2011 at 01:45:31PM +0800, yong.shen at freescale.com wrote:
> From: Yong Shen <yong.shen@freescale.com>
>
> 1. Adjust FEC base address name to fit macro definition
> 2. Add platform data and reset function for FEC
>
> Signed-off-by: Yong Shen <yong.shen@freescale.com>
> ---
> arch/arm/mach-mx5/board-mx53_evk.c | 27 +++++++++++++++++++++++++++
> arch/arm/mach-mx5/devices-imx53.h | 4 ++++
> arch/arm/plat-mxc/devices/Kconfig | 2 +-
> arch/arm/plat-mxc/devices/platform-fec.c | 5 +++++
> arch/arm/plat-mxc/include/mach/mx53.h | 2 +-
> 5 files changed, 38 insertions(+), 2 deletions(-)
[snip]
> @@ -60,11 +65,33 @@ static inline void mx53_evk_init_uart(void)
> imx53_add_imx_uart(2, &mx53_evk_uart_pdata);
> }
>
> +static inline void fec_reset(void)
For the sake of clearer stack traces I prefer a less generic name for this
routine, something like mx53_evk_fec_reset().
> +{
> + int ret;
> +
> + /* reset FEC PHY */
> + ret = gpio_request(SMD_FEC_PHY_RST, "fec-phy-reset");
> + if (ret) {
> + printk(KERN_ERR"failed to get GPIO_FEC_PHY_RESET: %d\n", ret);
> + return;
> + }
> + gpio_direction_output(SMD_FEC_PHY_RST, 0);
> + gpio_set_value(SMD_FEC_PHY_RST, 0);
This seems to be redundant. gpio_direction_output() has already set the value
to 0.
> + msleep(1);
> + gpio_set_value(SMD_FEC_PHY_RST, 1);
> +}
> +
> +static struct fec_platform_data fec_pdata = {
> + .phy = PHY_INTERFACE_MODE_RMII,
> +};
> +
[snip]
baruch
--
~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
^ permalink raw reply
* [PATCH] ARM i.MX53: enable fec driver on EVK board
From: yong.shen at freescale.com @ 2011-01-06 5:45 UTC (permalink / raw)
To: linux-arm-kernel
From: Yong Shen <yong.shen@freescale.com>
1. Adjust FEC base address name to fit macro definition
2. Add platform data and reset function for FEC
Signed-off-by: Yong Shen <yong.shen@freescale.com>
---
arch/arm/mach-mx5/board-mx53_evk.c | 27 +++++++++++++++++++++++++++
arch/arm/mach-mx5/devices-imx53.h | 4 ++++
arch/arm/plat-mxc/devices/Kconfig | 2 +-
arch/arm/plat-mxc/devices/platform-fec.c | 5 +++++
arch/arm/plat-mxc/include/mach/mx53.h | 2 +-
5 files changed, 38 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-mx5/board-mx53_evk.c b/arch/arm/mach-mx5/board-mx53_evk.c
index fa97d0d..b567ff0 100644
--- a/arch/arm/mach-mx5/board-mx53_evk.c
+++ b/arch/arm/mach-mx5/board-mx53_evk.c
@@ -21,14 +21,19 @@
#include <linux/init.h>
#include <linux/clk.h>
+#include <linux/fec.h>
+#include <linux/delay.h>
#include <mach/common.h>
#include <mach/hardware.h>
+#include <mach/gpio.h>
#include <asm/mach-types.h>
#include <asm/mach/arch.h>
#include <asm/mach/time.h>
#include <mach/imx-uart.h>
#include <mach/iomux-mx53.h>
+#define SMD_FEC_PHY_RST IMX_GPIO_NR(7, 6)
+
#include "crm_regs.h"
#include "devices-imx53.h"
@@ -60,11 +65,33 @@ static inline void mx53_evk_init_uart(void)
imx53_add_imx_uart(2, &mx53_evk_uart_pdata);
}
+static inline void fec_reset(void)
+{
+ int ret;
+
+ /* reset FEC PHY */
+ ret = gpio_request(SMD_FEC_PHY_RST, "fec-phy-reset");
+ if (ret) {
+ printk(KERN_ERR"failed to get GPIO_FEC_PHY_RESET: %d\n", ret);
+ return;
+ }
+ gpio_direction_output(SMD_FEC_PHY_RST, 0);
+ gpio_set_value(SMD_FEC_PHY_RST, 0);
+ msleep(1);
+ gpio_set_value(SMD_FEC_PHY_RST, 1);
+}
+
+static struct fec_platform_data fec_pdata = {
+ .phy = PHY_INTERFACE_MODE_RMII,
+};
+
static void __init mx53_evk_board_init(void)
{
mxc_iomux_v3_setup_multiple_pads(mx53_evk_pads,
ARRAY_SIZE(mx53_evk_pads));
mx53_evk_init_uart();
+ fec_reset();
+ imx53_add_fec(&fec_pdata);
}
static void __init mx53_evk_timer_init(void)
diff --git a/arch/arm/mach-mx5/devices-imx53.h b/arch/arm/mach-mx5/devices-imx53.h
index 9d0ec25..dbf1989 100644
--- a/arch/arm/mach-mx5/devices-imx53.h
+++ b/arch/arm/mach-mx5/devices-imx53.h
@@ -8,6 +8,10 @@
#include <mach/mx53.h>
#include <mach/devices-common.h>
+extern const struct imx_fec_data imx53_fec_data __initconst;
+#define imx53_add_fec(pdata) \
+ imx_add_fec(&imx53_fec_data, pdata)
+
extern const struct imx_imx_uart_1irq_data imx53_imx_uart_data[] __initconst;
#define imx53_add_imx_uart(id, pdata) \
imx_add_imx_uart_1irq(&imx53_imx_uart_data[id], pdata)
diff --git a/arch/arm/plat-mxc/devices/Kconfig b/arch/arm/plat-mxc/devices/Kconfig
index 2537166..b9ab1d5 100644
--- a/arch/arm/plat-mxc/devices/Kconfig
+++ b/arch/arm/plat-mxc/devices/Kconfig
@@ -1,6 +1,6 @@
config IMX_HAVE_PLATFORM_FEC
bool
- default y if ARCH_MX25 || SOC_IMX27 || SOC_IMX35 || SOC_IMX51
+ default y if ARCH_MX25 || SOC_IMX27 || SOC_IMX35 || SOC_IMX51 || SOC_IMX53
config IMX_HAVE_PLATFORM_FLEXCAN
select HAVE_CAN_FLEXCAN if CAN
diff --git a/arch/arm/plat-mxc/devices/platform-fec.c b/arch/arm/plat-mxc/devices/platform-fec.c
index 269ec78..b50c351 100644
--- a/arch/arm/plat-mxc/devices/platform-fec.c
+++ b/arch/arm/plat-mxc/devices/platform-fec.c
@@ -36,6 +36,11 @@ const struct imx_fec_data imx51_fec_data __initconst =
imx_fec_data_entry_single(MX51);
#endif
+#ifdef CONFIG_SOC_IMX53
+const struct imx_fec_data imx53_fec_data __initconst =
+ imx_fec_data_entry_single(MX53);
+#endif
+
struct platform_device *__init imx_add_fec(
const struct imx_fec_data *data,
const struct fec_platform_data *pdata)
diff --git a/arch/arm/plat-mxc/include/mach/mx53.h b/arch/arm/plat-mxc/include/mach/mx53.h
index 9577cdb..a35e0c7 100644
--- a/arch/arm/plat-mxc/include/mach/mx53.h
+++ b/arch/arm/plat-mxc/include/mach/mx53.h
@@ -136,7 +136,7 @@
#define MX53_MIPI_HSC_BASE_ADDR (MX53_AIPS2_BASE_ADDR + 0x000DC000)
#define MX53_MLB_BASE_ADDR (MX53_AIPS2_BASE_ADDR + 0x000E4000)
#define MX53_SSI3_BASE_ADDR (MX53_AIPS2_BASE_ADDR + 0x000E8000)
-#define MX53_MXC_FEC_BASE_ADDR (MX53_AIPS2_BASE_ADDR + 0x000EC000)
+#define MX53_FEC_BASE_ADDR (MX53_AIPS2_BASE_ADDR + 0x000EC000)
#define MX53_TVE_BASE_ADDR (MX53_AIPS2_BASE_ADDR + 0x000F0000)
#define MX53_VPU_BASE_ADDR (MX53_AIPS2_BASE_ADDR + 0x000F4000)
#define MX53_SAHARA_BASE_ADDR (MX53_AIPS2_BASE_ADDR + 0x000F8000)
--
1.7.1
^ permalink raw reply related
* [PATCH] arm: mm: Poison freed init memory
From: Stephen Boyd @ 2011-01-06 5:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20110105202645.GL8638@n2100.arm.linux.org.uk>
On 01/05/2011 12:26 PM, Russell King - ARM Linux wrote:
> On Wed, Jan 05, 2011 at 11:47:25AM -0800, Stephen Boyd wrote:
>> Poisoning __init marked memory can be useful when tracking down
>> obscure memory corruption bugs. When a pointer is 0xCCCCCCCC in an
>
> That's a bad idea for a value. With a 3GB page offset and 256MB or
> more memory, accesses to such an address will always succeed.
>
> There's two things to be considered when selecting a possible poison
> value:
>
> 1. what value is guaranteed to provoke an undefined instruction exception?
> 2. what value when used as an address and dereferenced is mostly always
> going to abort?
>
> 1 for ARM mode implies an 0xe7fXXXfX value. For Thumb mode 0xdeXX. We
> use this space for breakpoints.
>
> 2 unfortunately depends on the platform.
A coworker proposed we use a SWI instruction. We could do that if the
poison is 0xEF and then do something in the SWI handler where that
number causes us to blow up?
If I'm following correctly, point 1 is about __init functions and point
2 is about __initdata. I'm more concerned about __initdata because
__init functions called from non __init marked functions are usually
caught with section mismatch checks. Also, if we're jumping to
0xCCCCCCCC we're probably not in the text section of the kernel with a
3GB offset anymore, right? __initdata is easier to reference from
anywhere (ignoring function pointers) and it would definitely be nice to
find those invalid accesses quicker. For point 2, perhaps an unaligned
access would trigger sometimes?
Swapping the cheese for some cheese flavored poison is better than nothing.
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply
* [PATCH 5/5] S5PC110: add MIPI-DSI based sample lcd panel driver.
From: Paul Mundt @ 2011-01-06 5:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1293535595-24861-1-git-send-email-inki.dae@samsung.com>
On Tue, Dec 28, 2010 at 08:26:35PM +0900, Inki Dae wrote:
> this patch addes MIPI-DSI based sample panel driver.
> to write MIPI-DSI based lcd panel driver, you can refer to
> this sample driver.
>
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
Sample drivers are ok, but unless it has some sort of practical run-time
use you are probably better off just including this along with your
subsystem/platform documentation in Documentation somewhere. You can
search for .c files there to see how others are doing it.
Having said that ..
> diff --git a/drivers/video/s5p_mipi_sample.c b/drivers/video/s5p_mipi_sample.c
> new file mode 100644
> index 0000000..8a8abfe
> --- /dev/null
> +++ b/drivers/video/s5p_mipi_sample.c
> @@ -0,0 +1,220 @@
> +/* linux/drivers/video/sample.c
> + *
This is precisely why file comments are useless, since they invariably
fail to match up.
> + * MIPI-DSI based sample AMOLED lcd panel driver.
> + *
> + * Inki Dae, <inki.dae@samsung.com>
> + *
No Copyright notice?
> +static void sample_long_command(struct sample *lcd)
> +{
> + struct dsim_master_ops *ops = lcd_to_master_ops(lcd);
> + unsigned char data_to_send[5] = {
> + 0x00, 0x00, 0x00, 0x00, 0x00
> + };
> +
> + ops->cmd_write(lcd_to_master(lcd), MIPI_DSI_DCS_LONG_WRITE,
> + (unsigned int) data_to_send, sizeof(data_to_send));
> +}
> +
> +static void sample_short_command(struct sample *lcd)
> +{
> + struct dsim_master_ops *ops = lcd_to_master_ops(lcd);
> +
> + ops->cmd_write(lcd_to_master(lcd), MIPI_DSI_DCS_SHORT_WRITE_PARAM,
> + 0x00, 0x00);
> +}
> +
ops->cmd_write() can fail for any number of reasons, so you will want to
change these to make sure that you are actually handling the error cases.
> +static int sample_panel_init(struct sample *lcd)
> +{
> + sample_long_command(lcd);
> + sample_short_command(lcd);
> +
> + return 0;
At which point you can fail the initialization instead of just blowing up
later.
> +static int sample_gamma_ctrl(struct sample *lcd, int gamma)
> +{
> + struct dsim_master_ops *ops = lcd_to_master_ops(lcd);
> +
> + /* change transfer mode to LP mode */
> + if (ops->change_dsim_transfer_mode)
> + ops->change_dsim_transfer_mode(lcd_to_master(lcd), 0);
> +
ops->change_dsim_transfer_mode() can also fail. You could do this more
cleanly as:
enum { DSIM_XFER_LP, DSIM_XFER_HS };
static inline int dsim_set_xfer_mode(struct mipi_dsim_device *dsim, int mode)
{
struct mipi_dsim_master_ops *ops = dsim->master_ops;
if (!ops->change_dsim_transfer_mode)
return -ENOSYS; /* not implemented */
return ops->change_dsim_transfer_mode(dsim, mode);
}
Then simply do your sample_gamma_ctrl() as:
ret = dsim_set_xfer_mode(dsim, DSIM_XFER_LP);
if (ret != 0)
return ret;
> + /* update gamma table. */
> +
return dsim_set_xfer_mode(dsim, DSIM_XFER_HS);
> +}
> +
Or something similar. Your sample code should really be as
self-documenting and error-proof as possible. You don't really want to be
in a situation where subtle bugs leak through that then everyone who uses
this code as a reference will carry over!
> +static int sample_set_brightness(struct backlight_device *bd)
> +{
> + int ret = 0, brightness = bd->props.brightness;
> + struct sample *lcd = bl_get_data(bd);
> +
> + if (brightness < MIN_BRIGHTNESS ||
> + brightness > bd->props.max_brightness) {
> + dev_err(lcd->dev, "lcd brightness should be %d to %d.\n",
> + MIN_BRIGHTNESS, MAX_BRIGHTNESS);
> + return -EINVAL;
> + }
> +
> + ret = sample_gamma_ctrl(lcd, bd->props.brightness);
> + if (ret) {
> + dev_err(&bd->dev, "lcd brightness setting failed.\n");
> + return -EIO;
> + }
> +
With your current approach this error condition will never be reached,
for example.
> +static int sample_probe(struct mipi_lcd_device *mipi_dev)
> +{
> + struct sample *lcd = NULL;
> + struct backlight_device *bd = NULL;
> +
> + lcd = kzalloc(sizeof(struct sample), GFP_KERNEL);
> + if (!lcd) {
> + dev_err(&mipi_dev->dev, "failed to allocate sample structure.\n");
> + return -ENOMEM;
> + }
> +
> + lcd->mipi_dev = mipi_dev;
> + lcd->ddi_pd =
> + (struct mipi_ddi_platform_data *)device_to_ddi_pd(mipi_dev);
Does this really need to be casted?
> + lcd->dev = &mipi_dev->dev;
> +
> + dev_set_drvdata(&mipi_dev->dev, lcd);
> +
> + bd = backlight_device_register("sample-bl", lcd->dev, lcd,
> + &sample_backlight_ops, NULL);
> +
> + lcd->bd = bd;
> +
And here you have no error checking for backlight registration, so you
will get a NULL pointer deref:
> + bd->props.max_brightness = MAX_BRIGHTNESS;
> + bd->props.brightness = MAX_BRIGHTNESS;
> +
here. backlight_device_register() returns an ERR_PTR(), so you will want
to do an IS_ERR() check, which you can then map back with PTR_ERR() for a
more precise idea of why it failed.
> + /* lcd power on */
> + if (lcd->ddi_pd->lcd_power_on)
> + lcd->ddi_pd->lcd_power_on(NULL, 1);
> +
You may wish to use enums for this too. It's not strictly necessary, but
it does help to clarify which is the on and which is the off position.
> + mdelay(lcd->ddi_pd->reset_delay);
> +
> + /* lcd reset */
> + if (lcd->ddi_pd->lcd_reset)
> + lcd->ddi_pd->lcd_reset(NULL);
> +
Reset can fail?
> + sample_panel_init(lcd);
> +
> + return 0;
> +}
sample_panel_init() can fail as well, and in both cases you will need to
clean up all of the above work.
> +
> +#ifdef CONFIG_PM
> +static int sample_suspend(struct mipi_lcd_device *mipi_dev)
> +{
> + struct sample *lcd = dev_get_drvdata(&mipi_dev->dev);
> +
> + /* some work. */
> +
> + mdelay(lcd->ddi_pd->power_off_delay);
> +
Adding delays in the suspend/resume path sounds like a pretty bad idea,
is there a technical reason for it? If so, please document it, so people
get some idea of where their suspend/resume latencies are coming from,
and why.
> +static struct mipi_lcd_driver sample_mipi_driver = {
> + .name = "sample",
> +
> + .probe = sample_probe,
No remove?
> + .suspend = sample_suspend,
> + .resume = sample_resume,
> +};
> +
> +static int sample_init(void)
> +{
> + s5p_mipi_register_lcd_driver(&sample_mipi_driver);
> +
> + return 0;
> +}
> +
This should be:
return s5p_mipi_register_lcd_driver(&sample_mipi_driver);
And sample_init should be __init annotated.
> +static void sample_exit(void)
> +{
> + return;
> +}
> +
This should be balanced with a
s5p_mipi_unregister_lcd_driver(&sample_mipi_driver);
> +module_init(sample_init);
> +module_exit(sample_exit);
> +
> +MODULE_AUTHOR("Inki Dae <inki.dae@samsung.com>");
> +MODULE_DESCRIPTION("MIPI-DSI based sample AMOLED LCD Panel Driver");
> +MODULE_LICENSE("GPL");
Since you have a fairly complex subsystem it's probably a good idea to
work out how your MODULE_ALIAS() is going to work, so that you can handle
autoprobe for these things via udev. The fact you have no exit path
definitely suggests you haven't tested this in a modular configuration,
so there is probably going to be quite a bit of work to do here.
^ permalink raw reply
* [RFC 1/5] ARM: P2V: separate PHYS_OFFSET from platform definitions
From: Magnus Damm @ 2011-01-06 5:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <E1PaDOb-00023Z-CR@rmk-PC.arm.linux.org.uk>
On Wed, Jan 5, 2011 at 5:22 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> This uncouple PHYS_OFFSET from the platform definitions, thereby
> facilitating run-time computation of the physical memory offset.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
> ?arch/arm/include/asm/memory.h ? ? ? ? ? ? ? ? ?| ? ?2 ++
> ?arch/arm/kernel/tcm.c ? ? ? ? ? ? ? ? ? ? ? ? ?| ? ?2 +-
...
> ?arch/arm/mach-shmobile/include/mach/memory.h ? | ? ?2 +-
Applied on top of sh-2.6 git c422a5992e9ca072e8e472ee8f518649fb07470e
and successfully booted on a sh7372-based Mackerel board.
Acked-by: Magnus Damm <damm@opensource.se>
^ permalink raw reply
* [PATCH v3 05/10] net/fec: add dual fec support for mx28
From: Shawn Guo @ 2011-01-06 4:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20110105163449.GU25121@pengutronix.de>
Hi Uwe,
On Wed, Jan 05, 2011 at 05:34:49PM +0100, Uwe Kleine-K?nig wrote:
> Hello,
>
> On Wed, Jan 05, 2011 at 10:07:32PM +0800, Shawn Guo wrote:
> > This patch is to add mx28 dual fec support. Here are some key notes
> > for mx28 fec controller.
> >
> > - The mx28 fec controller naming ENET-MAC is a different IP from FEC
> > used on other i.mx variants. But they are basically compatible
> > on software interface, so it's possible to share the same driver.
> > - ENET-MAC design made an improper assumption that it runs on a
> > big-endian system. As the result, driver has to swap every frame
> > going to and coming from the controller.
> > - The external phys can only be configured by fec0, which means fec1
> > can not work independently and both phys need to be configured by
> > mii_bus attached on fec0.
> > - ENET-MAC reset will get mac address registers reset too.
> > - ENET-MAC MII/RMII mode and 10M/100M speed are configured
> > differently FEC.
> > - ETHER_EN bit must be set to get ENET-MAC interrupt work.
> >
> > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > ---
> > Changes for v3:
> > - Move v2 changes into patch #3
> > - Use device name to check if it's running on ENET-MAC
> >
> > drivers/net/Kconfig | 7 ++-
> > drivers/net/fec.c | 140 +++++++++++++++++++++++++++++++++++++++++++++------
> > drivers/net/fec.h | 5 +-
> > 3 files changed, 131 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> > index 4f1755b..f34629b 100644
> > --- a/drivers/net/Kconfig
> > +++ b/drivers/net/Kconfig
> > @@ -1944,18 +1944,19 @@ config 68360_ENET
> > config FEC
> > bool "FEC ethernet controller (of ColdFire and some i.MX CPUs)"
> > depends on M523x || M527x || M5272 || M528x || M520x || M532x || \
> > - MACH_MX27 || ARCH_MX35 || ARCH_MX25 || ARCH_MX5
> > + MACH_MX27 || ARCH_MX35 || ARCH_MX25 || ARCH_MX5 || SOC_IMX28
> > select PHYLIB
> > help
> > Say Y here if you want to use the built-in 10/100 Fast ethernet
> > controller on some Motorola ColdFire and Freescale i.MX processors.
> >
> > config FEC2
> > - bool "Second FEC ethernet controller (on some ColdFire CPUs)"
> > + bool "Second FEC ethernet controller"
> > depends on FEC
> > help
> > Say Y here if you want to use the second built-in 10/100 Fast
> > - ethernet controller on some Motorola ColdFire processors.
> > + ethernet controller on some Motorola ColdFire and Freescale
> > + i.MX processors.
> >
> > config FEC_MPC52xx
> > tristate "MPC52xx FEC driver"
> > diff --git a/drivers/net/fec.c b/drivers/net/fec.c
> > index 8a1c51f..67ba263 100644
> > --- a/drivers/net/fec.c
> > +++ b/drivers/net/fec.c
> > @@ -17,6 +17,8 @@
> > *
> > * Bug fixes and cleanup by Philippe De Muyter (phdm at macqel.be)
> > * Copyright (c) 2004-2006 Macq Electronique SA.
> > + *
> > + * Copyright (C) 2010 Freescale Semiconductor, Inc.
> > */
> >
> > #include <linux/module.h>
> > @@ -45,20 +47,33 @@
> >
> > #include <asm/cacheflush.h>
> >
> > -#ifndef CONFIG_ARCH_MXC
> > +#if !defined(CONFIG_ARCH_MXC) && !defined(CONFIG_SOC_IMX28)
> maybe !defined(CONFIG_ARM)?
>
Sounds good.
> > #include <asm/coldfire.h>
> > #include <asm/mcfsim.h>
> > #endif
> >
> > #include "fec.h"
> >
> > -#ifdef CONFIG_ARCH_MXC
> > -#include <mach/hardware.h>
> > +#if defined(CONFIG_ARCH_MXC) || defined(CONFIG_SOC_IMX28)
> > #define FEC_ALIGNMENT 0xf
> > #else
> > #define FEC_ALIGNMENT 0x3
> > #endif
> >
> > +#define DRIVER_NAME "fec"
> > +#define ENET_MAC_NAME "enet-mac"
> > +
> > +static struct platform_device_id fec_devtype[] = {
> > + {
> > + .name = DRIVER_NAME,
> > + }, {
> > + .name = ENET_MAC_NAME,
> > + }
> I'd done it differently:
>
> {
> .name = "fec",
> .driver_data = 0,
> }, {
> .name = "imx28-fec",
> .driver_data = HAS_ENET_MAC | ...,
> }
>
> and then test the bits in driver_data (which you get using
> platform_get_device_id() when you need to distinguish.
> Comparing names doesn't scale, assume there are three further features
> to distinguish, then you need to use strtok or index and get device
> names like enet-mac-with-feature1-but-without-feature2-and-feature3.
>
Makes sense. The frame endian issue will be fixed in future revision,
so I would define bit SWAP_FRAME for testing.
> > +};
> > +
> > +static unsigned fec_is_enetmac;
> > +static struct mii_bus *fec_mii_bus;
> In practice this might work, but actually these are per-device
> properties, not driver-global. So it should go into the private data
> struct.
>
Since it's just a tmp variable for holding fec0 mii_bus in function
fec_enet_mii_init, I would move it into the function as a static
variable.
> > +
> > static unsigned char macaddr[ETH_ALEN];
> > module_param_array(macaddr, byte, NULL, 0);
> > MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
> > @@ -129,7 +144,8 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
> > * account when setting it.
> > */
> > #if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || \
> > - defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARCH_MXC)
> > + defined(CONFIG_M520x) || defined(CONFIG_M532x) || \
> > + defined(CONFIG_ARCH_MXC) || defined(CONFIG_SOC_IMX28)
> > #define OPT_FRAME_SIZE (PKT_MAXBUF_SIZE << 16)
> > #else
> > #define OPT_FRAME_SIZE 0
> > @@ -208,6 +224,17 @@ static void fec_stop(struct net_device *dev);
> > /* Transmitter timeout */
> > #define TX_TIMEOUT (2 * HZ)
> >
> > +static void *swap_buffer(void *bufaddr, int len)
> > +{
> > + int i;
> > + unsigned int *buf = bufaddr;
> > +
> > + for (i = 0; i < (len + 3) / 4; i++, buf++)
> > + *buf = __swab32(*buf);
> Would it better to use cpu_to_be32 here? Then the compiler might
> be smart enough to optimize it away on BE. (Currently the code
> generated for a BE build would be wrong with your patch, wouldn't it?)
Yes.
> > +
> > + return bufaddr;
> > +}
> > +
> > static netdev_tx_t
> > fec_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > {
> > @@ -256,6 +283,14 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > bufaddr = fep->tx_bounce[index];
> > }
> >
> > + /*
> > + * enet-mac design made an improper assumption that it's running
> > + * on a big endian system. As the result, driver has to swap
> if he was really aware that he limits the performant use of the fec to
> big endian systems, can you please make him stop designing hardware!?
>
You over looked my power :) BTW, he had left Freescale.
> > + * every frame going to and coming from the controller.
> > + */
> > + if (fec_is_enetmac)
> > + swap_buffer(bufaddr, skb->len);
> > +
> > /* Save skb pointer */
> > fep->tx_skbuff[fep->skb_cur] = skb;
> >
> > @@ -487,6 +522,9 @@ fec_enet_rx(struct net_device *dev)
> > dma_unmap_single(NULL, bdp->cbd_bufaddr, bdp->cbd_datlen,
> > DMA_FROM_DEVICE);
> >
> > + if (fec_is_enetmac)
> > + swap_buffer(data, pkt_len);
> > +
> > /* This does 16 byte alignment, exactly what we need.
> > * The packet length includes FCS, but we don't want to
> > * include that when passing upstream as it messes up
> > @@ -689,6 +727,7 @@ static int fec_enet_mii_probe(struct net_device *dev)
> > char mdio_bus_id[MII_BUS_ID_SIZE];
> > char phy_name[MII_BUS_ID_SIZE + 3];
> > int phy_id;
> > + int dev_id = fep->pdev->id;
> >
> > fep->phy_dev = NULL;
> >
> > @@ -700,6 +739,8 @@ static int fec_enet_mii_probe(struct net_device *dev)
> > continue;
> > if (fep->mii_bus->phy_map[phy_id]->phy_id == 0)
> > continue;
> > + if (fec_is_enetmac && dev_id--)
> > + continue;
> > strncpy(mdio_bus_id, fep->mii_bus->id, MII_BUS_ID_SIZE);
> > break;
> > }
> > @@ -741,6 +782,28 @@ static int fec_enet_mii_init(struct platform_device *pdev)
> > struct fec_enet_private *fep = netdev_priv(dev);
> > int err = -ENXIO, i;
> >
> > + /*
> > + * The dual fec interfaces are not equivalent with enet-mac.
> > + * Here are the differences:
> > + *
> > + * - fec0 supports MII & RMII modes while fec1 only supports RMII
> > + * - fec0 acts as the 1588 time master while fec1 is slave
> > + * - external phys can only be configured by fec0
> > + *
> > + * That is to say fec1 can not work independently. It only works
> > + * when fec0 is working. The reason behind this design is that the
> > + * second interface is added primarily for Switch mode.
> > + *
> > + * Because of the last point above, both phys are attached on fec0
> > + * mdio interface in board design, and need to be configured by
> > + * fec0 mii_bus.
> > + */
> > + if (fec_is_enetmac && pdev->id) {
> > + /* fec1 uses fec0 mii_bus */
> > + fep->mii_bus = fec_mii_bus;
> > + return 0;
> > + }
> > +
> > fep->mii_timeout = 0;
> >
> > /*
> > @@ -777,6 +840,10 @@ static int fec_enet_mii_init(struct platform_device *pdev)
> > if (mdiobus_register(fep->mii_bus))
> > goto err_out_free_mdio_irq;
> >
> > + /* save fec0 mii_bus */
> > + if (fec_is_enetmac)
> > + fec_mii_bus = fep->mii_bus;
> > +
> > return 0;
> >
> > err_out_free_mdio_irq:
> > @@ -1149,11 +1216,22 @@ fec_restart(struct net_device *dev, int duplex)
> > {
> > struct fec_enet_private *fep = netdev_priv(dev);
> > int i;
> > + u32 val, temp_mac[2];
> >
> > /* Whack a reset. We should wait for this. */
> > writel(1, fep->hwp + FEC_ECNTRL);
> > udelay(10);
> >
> > + /*
> > + * enet-mac reset will reset mac address registers too,
> > + * so need to reconfigure it.
> > + */
> > + if (fec_is_enetmac) {
> > + memcpy(&temp_mac, dev->dev_addr, ETH_ALEN);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > + writel(cpu_to_be32(temp_mac[0]), fep->hwp + FEC_ADDR_LOW);
> > + writel(cpu_to_be32(temp_mac[1]), fep->hwp + FEC_ADDR_HIGH);
> where is the value saved to temp_mac[]? For me it looks you write
> uninitialized data into the mac registers.
memcpy above.
--
Regards,
Shawn
^ permalink raw reply
* [PATCH] omap: wd_timer: Fix crash frm wdt_probe when!CONFIG_RUNTIME_PM
From: Santosh Shilimkar @ 2011-01-06 3:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20110105221839.GD8717@n2100.arm.linux.org.uk>
> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Thursday, January 06, 2011 3:49 AM
> To: Santosh Shilimkar
> Cc: linux-omap at vger.kernel.org; Paul Walmsley; linux-arm-
> kernel at lists.infradead.org
> Subject: Re: [PATCH] omap: wd_timer: Fix crash frm wdt_probe
> when!CONFIG_RUNTIME_PM
>
> On Wed, Jan 05, 2011 at 07:10:55PM +0530, Santosh Shilimkar wrote:
> > Commit ff2516fb 'wd_timer: disable on boot via hwmod postsetup
> mechanism'
> > introduced watchdog timer state state management using
> postsetup_state.
> > This was done to allow some board files to support watchdog
> coverage
> > throughout kernel initialization and it work as intended when
> RUNTIME_PM
> > is enabled.
> >
> > With !CONFIG_RUNTIME_PM and no board is specifically requests
> watchdog
> > to remain enabled the omap_wdt_probe crashesh. This is because
> hwmod
> > in absense of runtime PM unable to turn watchdog clocks because
> it's
> > state is set to be disabled. For rest of the device, the state is
> > set as enabled in absense of RUNTIME_PM
>
> Err... wasn't this provoked by an attempt to fix the LDP issue, that
> is
> (I believe) because the boot loader enables the watchdog and pre-
> hwmod
> kernels used to disable it. Post-hwmod kernels stopped disabling
> the
> watchdog, resulting in a few seconds booting userspace before the
> system
> resets itself.
Yes. That's managed through the shutdown part. Apart from that there
is another enhancement done in case some one wants to have WDT running
throughout the kernel boot.
Regards,
Santosh
^ permalink raw reply
* [PATCH] omap: wd_timer: Fix crash frm wdt_probe when !CONFIG_RUNTIME_PM
From: Santosh Shilimkar @ 2011-01-06 3:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <87aajfz18c.fsf@ti.com>
> -----Original Message-----
> From: Kevin Hilman [mailto:khilman at ti.com]
> Sent: Wednesday, January 05, 2011 11:23 PM
> To: Santosh Shilimkar
> Cc: Paul Walmsley; linux-arm-kernel at lists.infradead.org; linux-
> omap at vger.kernel.org
> Subject: Re: [PATCH] omap: wd_timer: Fix crash frm wdt_probe when
> !CONFIG_RUNTIME_PM
>
> Santosh Shilimkar <santosh.shilimkar@ti.com> writes:
>
> > Commit ff2516fb 'wd_timer: disable on boot via hwmod postsetup
> mechanism'
> > introduced watchdog timer state state management using
> postsetup_state.
> > This was done to allow some board files to support watchdog
> coverage
> > throughout kernel initialization and it work as intended when
> RUNTIME_PM
> > is enabled.
> >
> > With !CONFIG_RUNTIME_PM and no board is specifically requests
> watchdog
> > to remain enabled the omap_wdt_probe crashesh. This is because
> hwmod
> > in absense of runtime PM unable to turn watchdog clocks because
> it's
> > state is set to be disabled. For rest of the device, the state is
> > set as enabled in absense of RUNTIME_PM
> >
> > [ 1.372558] Unhandled fault: imprecise external abort (0x1406)
> at
> > 0xad733eeb
> > [ 1.379913] Internal error: : 1406 [#1] SMP
> > [ 1.384277] last sysfs file:
> > [ 1.387359] Modules linked in:
> > [ 1.390563] CPU: 0 Tainted: G W
> > (2.6.37-rc7-00265-g4298a4c-dirty #23)
> > [ 1.398468] PC is at omap_wdt_disable+0x2c/0x3c
> > [ 1.403198] LR is at omap_wdt_probe+0x124/0x1e0
> > [ 1.407928] pc : [<c02f5bf4>] lr : [<c03be10c>] psr:
> 60000013
> > [ 1.407958] sp : df833f00 ip : 00000000 fp : 00000000
> > [ 1.419921] r10: c0ac57ac r9 : df959e00 r8 : 00000000
> > [ 1.425384] r7 : df959e08 r6 : df8000c0 r5 : df95bebc r4 :
> df87dde0
> > [ 1.432189] r3 : fc314000 r2 : 00005555 r1 : fc314034 r0 :
> df87dde0
> >
> > This patch make the default watchdog state to be enabled in case
> of
> > !CONFIG_RUNTIME_PM. This fixes the crash
> >
> > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> > Cc: Paul Walmsley <paul@pwsan.com>
> > ---
> > Paul, I am not too sure if it breaks your _shutdown idea of
> watchdog
> > timer. Patch generated against 'omap-for-linus' branch and boot
> > tested on OMAP4 with and without CONFIG_OMAP_WATCHDOG.
> >
> > arch/arm/mach-omap2/io.c | 4 ++++
> > 1 files changed, 4 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
> > index e66687b..b879a16 100644
> > --- a/arch/arm/mach-omap2/io.c
> > +++ b/arch/arm/mach-omap2/io.c
> > @@ -378,7 +378,11 @@ void __init
> omap2_init_common_infrastructure(void)
> > * XXX ideally we could detect whether the MPU WDT was
> currently
> > * enabled here and make this conditional
> > */
> > +#ifdef CONFIG_PM_RUNTIME
> > postsetup_state = _HWMOD_STATE_DISABLED;
> > +#else
> > + postsetup_state = _HWMOD_STATE_ENABLED;
> > +#endif
>
> You shouldn't need the 'else' part of this since the default a few
> lines
> above this code is already setting that for the !CONFIG_PM_RUNTIME
> case.
Yep. Just 'CONFIG_PM_RUNTIME' wrapping is enough.
^ permalink raw reply
* [PATCH 0/3] hvc_dcc cleanups and fixes
From: Greg KH @ 2011-01-06 3:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <4D251F9D.2070406@codeaurora.org>
On Wed, Jan 05, 2011 at 05:49:17PM -0800, Stephen Boyd wrote:
> Greg,
>
> On 12/20/2010 12:08 PM, Stephen Boyd wrote:
> > Here are the split patches.
>
> Should I resend these with the proper acks or can/have you picked these
> patches up?
They are in my to-apply queue after .38-rc1 is out.
thanks,
greg k-h
^ permalink raw reply
* [PATCH 0/3] hvc_dcc cleanups and fixes
From: Stephen Boyd @ 2011-01-06 1:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1292875718-7980-1-git-send-email-sboyd@codeaurora.org>
Greg,
On 12/20/2010 12:08 PM, Stephen Boyd wrote:
> Here are the split patches.
Should I resend these with the proper acks or can/have you picked these
patches up?
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply
* [PATCH v3 08/10] ARM: mxs: add ocotp read function
From: Shawn Guo @ 2011-01-06 1:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20110105175617.GD12222@shareable.org>
On Wed, Jan 05, 2011 at 05:56:17PM +0000, Jamie Lokier wrote:
> Jamie Iles wrote:
> > On Wed, Jan 05, 2011 at 05:44:09PM +0100, Uwe Kleine-K?nig wrote:
> > > Hello Jamie,
> > > On Wed, Jan 05, 2011 at 04:16:46PM +0000, Jamie Iles wrote:
> > > > On Wed, Jan 05, 2011 at 10:07:35PM +0800, Shawn Guo wrote:
> > > > > + /* check both BUSY and ERROR cleared */
> > > > > + while ((__raw_readl(ocotp_base) &
> > > > > + (BM_OCOTP_CTRL_BUSY | BM_OCOTP_CTRL_ERROR)) && --timeout)
> > > > > + /* nothing */;
> > > >
> > > > Is it worth using cpu_relax() in these polling loops?
> > > I don't know what cpu_relax does for other platforms, but on ARM it's
> > > just a memory barrier which AFAICT doesn't help here at all (which
> > > doesn't need to be correct). Why do you think it would be better?
> >
> > Well I don't see that there's anything broken without cpu_relax() but
> > using cpu_relax() seems to be the most common way of doing busy polling
> > loops that I've seen. It's also a bit easier to read than a comment and
> > semi-colon. Perhaps it's just personal preference.
>
> cpu_relax() is a hint to the CPU to, for example, save power or be
> less aggressive on the memory bus (to save power or be fairer).
>
> Currently these architectures do more than just a barrier in cpu_relax():
> x86, IA64, PowerPC, Tile and S390.
>
> Although it's just a hint on ARM at the moment, it might change in
> future - especially with power mattering on so many ARM systems.
> (Even now, just changing it to a very short udelay might save power
> on existing ARMs without breaking drivers.)
>
Sounds reasonable. I would take the suggestion. Thanks, both Jamie.
--
Regards,
Shawn
^ permalink raw reply
* [PATCH v3 08/10] ARM: mxs: add ocotp read function
From: Jamie Lokier @ 2011-01-06 0:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20110105201502.GK8638@n2100.arm.linux.org.uk>
Russell King - ARM Linux wrote:
> On Wed, Jan 05, 2011 at 07:44:18PM +0000, Jamie Lokier wrote:
> > 'git show 534be1d5' explains how it works: cpu_relax() flushes buffered
> > writes from _this_ CPU, so that other CPUs which are polling can make
> > progress, which avoids this CPU getting stuck if there is an indirect
> > dependency (no matter how convoluted) between what it's polling and which
> > it wrote just before.
> >
> > So cpu_relax() is *essential* in some polling loops, not a hint.
> >
> > In principle that could happen for I/O polling, if (a) buffered memory
> > writes are delayed by I/O read transactions, and (b) the device state we're
> > waiting on depends on I/O yet to be done on another CPU, which could be
> > polling memory first (e.g. a spinlock).
> >
> > I doubt (a) in practice - but what about buses that block during I/O read?
> > (I have a chip like that here, but it's ARMv4T.)
>
> Let's be clear - ARMv5 and below generally are well ordered architectures
> within the limits of caching. There are cases where the write buffer
> allows two writes to pass each other. However, for IO we generally map
> these - especially for ARMv4 and below - as 'uncacheable unbufferable'.
> So on these, if the program says "read this location" the pipeline will
> stall until the read has been issued - and if you use the result in the
> next instruction, it will stall until the data is available. So really,
> it's not a problem here.
>
> ARMv6 and above have a weakly ordered memory model with speculative
> prefetching, so memory reads/writes can be completely unordered. Device
> accesses can pass memory accesses, but device accesses are always visible
> in program order with respect to each other.
>
> So, if you're spinning in a loop reading an IO device, all previous IO
> accesses will be completed (in all ARM architectures) before the result
> of your read is evaluated.
No, that wasn't the scenario - it was:
You're spinning reading an IO device, whose state depends indirectly
on a *CPU memory* write that is forever buffered.
(Go and re-read 'git show 534be1d5' if you haven't already.)
The indirect dependence is that another CPU needs to see that write
before it can tell the device to change state in whatever way the
first CPU is polling for.
It's probably clearer in code:
CPU #1
spin_lock(&mydev->lock);
/* Look at state. */
spin_unlock(&mydev->lock); <-- THIS MEMORY WRITE BUFFERED FOREVER
/* We expect this to be quick enough that polling is cool. */
while (readl(mydev->reg_status) & MYDEV_STATUS_BUSY) {
/* If only we had cpu_relax() */
}
CPU #2
spin_lock(&mydev->lock); <-- STUCK HERE
/* Look@state. */
spin_unlock(&mydev->lock);
writel(MYDEV_TRIGGER, mydev->reg_go); /* Device is BUSY until this. */
The deadlock in this code (might) happen when CPU #2 is waiting for
the spinlock, and CPU #1's memory write remains in its write buffer
during CPU #1's polling loop.
If that can happen, it's fixed by adding cpu_relax() - to generic
driver code with polling loops.
It can only happen if any CPUs (i.e. ARMv6) that buffer writes due to
prioritising continuous memory reads also have that effect for
continuous IO reads. This might even apply to non-ARM archs with
non-trivial cpu_relax() definitions; I don't know as they don't always
explain why.
The above driver style isn't particularly obvious, but there are a lot
of drivers with almost every conceivable access pattern. If you use
your imagination, especially if the second code is an interrupt
handler, it's plausible. Even though this example would be better
sleeping and waiting normally - there's nothing inherently forbidden
about the above pattern (except that cpu_relax() is needed).
> (But, let's make you squirm some more - mb() on ARMv6 and above may
> equate to a CPU memory barrier _plus_ a few IO accesses to the external
> L2 cache controller - which will be ordered wrt other IO accesses of
> course.)
I squirm at all modern ARM architectures. Omit the slightest highly
version-specific thing, or run a kernel built with slightly wrong
config options, and it's fine except for random, very rare memory or
I/O corruption. The workarounds and special bits seem to get more and
more convoluted with each version.
-- Jamie
^ permalink raw reply
* [PATCH] OMAP3+: sr_device: include pm header
From: Kevin Hilman @ 2011-01-06 0:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1294083341-32147-1-git-send-email-nm@ti.com>
Nishanth Menon <nm@ti.com> writes:
> omap_enable_smartreflex_on_init is meant to be used by boards
> which would like to have SR enabled by default on the platform, while
> omap_devinit_smartreflex is used by pm code, the protos are defined
> in pm.h. This header should be included to ensure that sr_device
> function definitions match the prototypes.
>
> including pm.h fixes the sparse warnings (with CONFIG_OMAP_SMARTREFLEX=y):
> arch/arm/mach-omap2/sr_device.c:138:13: warning: symbol 'omap_enable_smartreflex_on_init' was not declared. Should it be static?
> arch/arm/mach-omap2/sr_device.c:143:12: warning: symbol 'omap_devinit_smartreflex' was not declared. Should it be static?
>
> Signed-off-by: Nishanth Menon <nm@ti.com>
Thanks, will queue as a fix for 2.6.38-rc cycle.
Kevin
> Based on:
> git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap-2.6.git
> branch: omap-for-linus
>
> arch/arm/mach-omap2/sr_device.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c
> index 786d685..b1e0af1 100644
> --- a/arch/arm/mach-omap2/sr_device.c
> +++ b/arch/arm/mach-omap2/sr_device.c
> @@ -27,6 +27,7 @@
> #include <plat/voltage.h>
>
> #include "control.h"
> +#include "pm.h"
>
> static bool sr_enable_on_init;
^ permalink raw reply
* [PATCH] arm: mach-omap2: pm: cleanup !CONFIG_SUSPEND handling
From: Kevin Hilman @ 2011-01-06 0:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1293640483-25610-1-git-send-email-aaro.koskinen@nokia.com>
Hi Aaro,
Aaro Koskinen <aaro.koskinen@nokia.com> writes:
> Make !CONFIG_SUSPEND init declarations identical on all OMAPs and
> eliminate some ifdefs.
>
> Signed-off-by: Aaro Koskinen <aaro.koskinen@nokia.com>
I like this solution, but it introduces compiler warnings:
/opt/home/khilman/work.local/kernel/omap/pm/arch/arm/mach-omap2/pm44xx.c: In function 'omap4_pm_init':
/opt/home/khilman/work.local/kernel/omap/pm/arch/arm/mach-omap2/pm44xx.c:119: warning: passing argument 1 of 'suspend_set_ops' discards qualifiers from pointer target type
/opt/home/khilman/work.local/kernel/omap/pm/include/linux/suspend.h:125: note: expected 'struct platform_suspend_ops *' but argument is of type 'const struct platform_suspend_ops *'
/opt/home/khilman/work.local/kernel/omap/pm/arch/arm/mach-omap2/pm24xx.c: In function 'omap2_pm_init':
/opt/home/khilman/work.local/kernel/omap/pm/arch/arm/mach-omap2/pm24xx.c:585: warning: passing argument 1 of 'suspend_set_ops' discards qualifiers from pointer target type
/opt/home/khilman/work.local/kernel/omap/pm/include/linux/suspend.h:125: note: expected 'struct platform_suspend_ops *' but argument is of type 'const struct platform_suspend_ops *'
/opt/home/khilman/work.local/kernel/omap/pm/arch/arm/mach-omap2/pm34xx.c: In function 'omap3_pm_init':
/opt/home/khilman/work.local/kernel/omap/pm/arch/arm/mach-omap2/pm34xx.c:1072: warning: passing argument 1 of 'suspend_set_ops' discards qualifiers from pointer target type
/opt/home/khilman/work.local/kernel/omap/pm/include/linux/suspend.h:125:
note: expected 'struct platform_suspend_ops *' but argument is of type
'const struct platform_suspend_ops *'
As you likely noticed, removing the const leads to checkpatch warnings:
WARNING: struct platform_suspend_ops should normally be const
so the choice is between a checkpatch warning or a bunch of compiler
warnings.
Alternatively, I just posted a patch[1] to linux-pm propsing to fix this
at the source. Let's see what happens there. Merging $SUBJECT patch
will depend on how this is fixed upstream.
Kevin
[1] https://patchwork.kernel.org/patch/455831/
> ---
> arch/arm/mach-omap2/pm.h | 4 ++++
> arch/arm/mach-omap2/pm24xx.c | 16 ++++++++--------
> arch/arm/mach-omap2/pm34xx.c | 16 ++++++++--------
> arch/arm/mach-omap2/pm44xx.c | 17 +++++++++--------
> 4 files changed, 29 insertions(+), 24 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
> index 1c1b0ab..704766b 100644
> --- a/arch/arm/mach-omap2/pm.h
> +++ b/arch/arm/mach-omap2/pm.h
> @@ -138,4 +138,8 @@ static inline int omap4_twl_init(void)
> }
> #endif
>
> +#ifndef CONFIG_SUSPEND
> +#define omap_pm_ops NULL
> +#endif
> +
> #endif
> diff --git a/arch/arm/mach-omap2/pm24xx.c b/arch/arm/mach-omap2/pm24xx.c
> index dac2d1d..e65b329 100644
> --- a/arch/arm/mach-omap2/pm24xx.c
> +++ b/arch/arm/mach-omap2/pm24xx.c
> @@ -350,14 +350,14 @@ static void omap2_pm_end(void)
> enable_hlt();
> }
>
> -static struct platform_suspend_ops omap_pm_ops = {
> - .begin = omap2_pm_begin,
> - .enter = omap2_pm_enter,
> - .end = omap2_pm_end,
> - .valid = suspend_valid_only_mem,
> +static const struct platform_suspend_ops omap_pm_ops[] = {
> + {
> + .begin = omap2_pm_begin,
> + .enter = omap2_pm_enter,
> + .end = omap2_pm_end,
> + .valid = suspend_valid_only_mem,
> + }
> };
> -#else
> -static const struct platform_suspend_ops __initdata omap_pm_ops;
> #endif /* CONFIG_SUSPEND */
>
> /* XXX This function should be shareable between OMAP2xxx and OMAP3 */
> @@ -582,7 +582,7 @@ static int __init omap2_pm_init(void)
> omap24xx_cpu_suspend_sz);
> }
>
> - suspend_set_ops(&omap_pm_ops);
> + suspend_set_ops(omap_pm_ops);
> pm_idle = omap2_pm_idle;
>
> return 0;
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 5b323f2..a4c9283 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -605,11 +605,13 @@ static void omap3_pm_end(void)
> return;
> }
>
> -static struct platform_suspend_ops omap_pm_ops = {
> - .begin = omap3_pm_begin,
> - .end = omap3_pm_end,
> - .enter = omap3_pm_enter,
> - .valid = suspend_valid_only_mem,
> +static const struct platform_suspend_ops omap_pm_ops[] = {
> + {
> + .begin = omap3_pm_begin,
> + .end = omap3_pm_end,
> + .enter = omap3_pm_enter,
> + .valid = suspend_valid_only_mem,
> + }
> };
> #endif /* CONFIG_SUSPEND */
>
> @@ -1067,9 +1069,7 @@ static int __init omap3_pm_init(void)
> core_clkdm = clkdm_lookup("core_clkdm");
>
> omap_push_sram_idle();
> -#ifdef CONFIG_SUSPEND
> - suspend_set_ops(&omap_pm_ops);
> -#endif /* CONFIG_SUSPEND */
> + suspend_set_ops(omap_pm_ops);
>
> pm_idle = omap3_pm_idle;
> omap3_idle_init();
> diff --git a/arch/arm/mach-omap2/pm44xx.c b/arch/arm/mach-omap2/pm44xx.c
> index e9f4862..6022c0a 100644
> --- a/arch/arm/mach-omap2/pm44xx.c
> +++ b/arch/arm/mach-omap2/pm44xx.c
> @@ -16,6 +16,7 @@
> #include <linux/err.h>
> #include <linux/slab.h>
>
> +#include "pm.h"
> #include "powerdomain.h"
> #include <mach/omap4-common.h>
>
> @@ -65,11 +66,13 @@ static void omap4_pm_end(void)
> return;
> }
>
> -static struct platform_suspend_ops omap_pm_ops = {
> - .begin = omap4_pm_begin,
> - .end = omap4_pm_end,
> - .enter = omap4_pm_enter,
> - .valid = suspend_valid_only_mem,
> +static const struct platform_suspend_ops omap_pm_ops[] = {
> + {
> + .begin = omap4_pm_begin,
> + .end = omap4_pm_end,
> + .enter = omap4_pm_enter,
> + .valid = suspend_valid_only_mem,
> + }
> };
> #endif /* CONFIG_SUSPEND */
>
> @@ -113,9 +116,7 @@ static int __init omap4_pm_init(void)
> }
> #endif
>
> -#ifdef CONFIG_SUSPEND
> - suspend_set_ops(&omap_pm_ops);
> -#endif /* CONFIG_SUSPEND */
> + suspend_set_ops(omap_pm_ops);
>
> err2:
> return ret;
^ permalink raw reply
* [PATCH] PM: fix suspend_set_ops() to take const pointer
From: Kevin Hilman @ 2011-01-06 0:22 UTC (permalink / raw)
To: linux-arm-kernel
checkpatch warns that pointers for certain structs should be const,
and platform_suspend_ops is one of those structs. If you follow this
suggestion you trade a checkpatch warning for a compiler warning of
the form:
warning: passing argument 1 of 'suspend_set_ops' discards qualifiers from pointer target type
Fix suspend_set_ops() to take a const pointer so everyone expects
const.
Signed-off-by: Kevin Hilman <khilman@ti.com>
---
Applies to v2.6.37, but also applies cleanly to suspend-2.6/linux-next
include/linux/suspend.h | 4 ++--
kernel/power/suspend.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 2669751..bb98383 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -122,7 +122,7 @@ struct platform_suspend_ops {
* suspend_set_ops - set platform dependent suspend operations
* @ops: The new suspend operations to set.
*/
-extern void suspend_set_ops(struct platform_suspend_ops *ops);
+extern void suspend_set_ops(const struct platform_suspend_ops *ops);
extern int suspend_valid_only_mem(suspend_state_t state);
/**
@@ -147,7 +147,7 @@ extern int pm_suspend(suspend_state_t state);
#else /* !CONFIG_SUSPEND */
#define suspend_valid_only_mem NULL
-static inline void suspend_set_ops(struct platform_suspend_ops *ops) {}
+static inline void suspend_set_ops(const struct platform_suspend_ops *ops) {}
static inline int pm_suspend(suspend_state_t state) { return -ENOSYS; }
#endif /* !CONFIG_SUSPEND */
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index ecf7705..0c0af3d 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -36,7 +36,7 @@ static struct platform_suspend_ops *suspend_ops;
* suspend_set_ops - Set the global suspend method table.
* @ops: Pointer to ops structure.
*/
-void suspend_set_ops(struct platform_suspend_ops *ops)
+void suspend_set_ops(const struct platform_suspend_ops *ops)
{
mutex_lock(&pm_mutex);
suspend_ops = ops;
--
1.7.3.4
^ permalink raw reply related
* still nfs problems [Was: Linux 2.6.37-rc8]
From: Russell King - ARM Linux @ 2011-01-05 23:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <AANLkTi=SjMinMp+m726GS1iehj6cQgNy1RqSoUqKhjtv@mail.gmail.com>
On Wed, Jan 05, 2011 at 03:28:53PM -0800, Linus Torvalds wrote:
> On Wed, Jan 5, 2011 at 3:06 PM, Trond Myklebust
> <Trond.Myklebust@netapp.com> wrote:
> >
> > Yes. The fix I sent out was a call to invalidate_kernel_vmap_range(),
> > which takes care of invalidating the cache prior to a virtual address
> > read.
> >
> > My question was specifically about the write through the regular kernel
> > mapping: according to Russell and my reading of the cachetlb.txt
> > documentation, flush_dcache_page() is only guaranteed to have an effect
> > on page cache pages.
>
> I don't think that should ever matter. It's not like the hardware can
> know whether it's a dcache page or not.
>
> And if the sw implementation cares, it's doing something really odd.
^ permalink raw reply
* still nfs problems [Was: Linux 2.6.37-rc8]
From: Linus Torvalds @ 2011-01-05 23:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1294268808.2952.18.camel@heimdal.trondhjem.org>
On Wed, Jan 5, 2011 at 3:06 PM, Trond Myklebust
<Trond.Myklebust@netapp.com> wrote:
>
> Yes. The fix I sent out was a call to invalidate_kernel_vmap_range(),
> which takes care of invalidating the cache prior to a virtual address
> read.
>
> My question was specifically about the write through the regular kernel
> mapping: according to Russell and my reading of the cachetlb.txt
> documentation, flush_dcache_page() is only guaranteed to have an effect
> on page cache pages.
I don't think that should ever matter. It's not like the hardware can
know whether it's a dcache page or not.
And if the sw implementation cares, it's doing something really odd.
But who knows - there's a lot of crap out there, and people sometimes
do really odd things to work around the brokenness of a VIVT cache
with aliases.
Linus
^ permalink raw reply
* still nfs problems [Was: Linux 2.6.37-rc8]
From: James Bottomley @ 2011-01-05 23:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1294268808.2952.18.camel@heimdal.trondhjem.org>
On Wed, 2011-01-05 at 18:06 -0500, Trond Myklebust wrote:
> On Wed, 2011-01-05 at 13:30 -0800, Linus Torvalds wrote:
> > On Wed, Jan 5, 2011 at 1:16 PM, Trond Myklebust
> > <Trond.Myklebust@netapp.com> wrote:
> > >
> > > So what should be the preferred way to ensure data gets flushed when
> > > you've written directly to a page, and then want to read through the
> > > vm_map_ram() virtual range? Should we be adding new semantics to
> > > flush_kernel_dcache_page()?
> >
> > The "preferred way" is actually simple: "don't do that". IOW, if some
> > page is accessed through a virtual mapping you've set up, then
> > _always_ access it through that virtual mapping.
> >
> > Now, when that is impossible (and yes, it sometimes is), then you
> > should flush after doing all writes. And if you do the write through
> > the regular kernel mapping, you should use flush_dcache_page(). And if
> > you did it through the virtual mapping, you should use
> > "flush_kernel_vmap_range()" or whatever.
> >
> > NOTE! I really didn't look those up very closely, and if the accesses
> > can happen concurrently you are basically screwed, so you do need to
> > do locking or something else to guarantee that there is some nice
> > sequential order. And maybe I forgot something. Which is why I do
> > suggest "don't do that" as a primary approach to the problem if at all
> > possible.
> >
> > Oh, and you may need to flush before reading too (and many writes do
> > end up being "read-modify-write" cycles) in case it's possible that
> > you have stale data from a previous read that was then invalidated by
> > a write to the aliasing address. Even if that write was flushed out,
> > the stale read data may exist at the virtual address. I forget what
> > all we required - in the end the only sane model is "virtual caches
> > suck so bad that anybody who does them should be laughed at for being
> > a retard".
>
> Yes. The fix I sent out was a call to invalidate_kernel_vmap_range(),
> which takes care of invalidating the cache prior to a virtual address
> read.
>
> My question was specifically about the write through the regular kernel
> mapping: according to Russell and my reading of the cachetlb.txt
> documentation, flush_dcache_page() is only guaranteed to have an effect
> on page cache pages.
> flush_kernel_dcache_page() (not to be confused with flush_dcache_page)
> would appear to be the closest fit according to my reading of the
> documentation, however the ARM implementation appears to be a no-op...
It depends on exactly what you're doing. In the worst case, (ping pong
reads and writes through both aliases) you have to flush and invalidate
both alias 1 alias 2 each time you access on one and then another.
Can you explain how the code works? it looks to me like you read the xdr
stuff through the vmap region then write it out directly to the pages?
*if* this is just a conversion, *and* you never need to read the new
data through the vmap alias, you might be able to get away with a
flush_dcache_page in nfs_readdir_release_array(). If the access pattern
is more complex, you'll need more stuff splashed through the loop
(including vmap invalidation/flushing).
Is there any way you could just rewrite nfs_readdir_add_to_array() to
use the vmap address instead of doing a kmap? That way everything will
go through a single alias and not end up with this incoherency.
James
^ permalink raw reply
* [PATCH 2/2] OMAP3: beagle xm: enable upto 800MHz OPP
From: Kevin Hilman @ 2011-01-05 23:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1294260576-20237-3-git-send-email-nm@ti.com>
Nishanth Menon <nm@ti.com> writes:
> Beagle XM uses 3730 and the board design allows enabling 800MHz and 1GHz
> OPPs. However, We need Smart reflex class 1.5 and ABB to enable 1GHz safely.
> For the moment, we tweak the default table to allow for 800Mhz OPP usage.
Isn't this common to any board using 3730 (or 3630?)
IOW, what is Beagle specific about this?
Kevin
> Reported-by: Koen Kooi <koen@beagleboard.org>
> Tested-by: Koen Kooi <koen@beagleboard.org>
>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
> arch/arm/mach-omap2/board-omap3beagle.c | 50 +++++++++++++++++++++++++++++++
> 1 files changed, 50 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/board-omap3beagle.c b/arch/arm/mach-omap2/board-omap3beagle.c
> index 6c12760..0b99b80 100644
> --- a/arch/arm/mach-omap2/board-omap3beagle.c
> +++ b/arch/arm/mach-omap2/board-omap3beagle.c
> @@ -23,6 +23,7 @@
> #include <linux/gpio.h>
> #include <linux/input.h>
> #include <linux/gpio_keys.h>
> +#include <linux/opp.h>
>
> #include <linux/mtd/mtd.h>
> #include <linux/mtd/partitions.h>
> @@ -44,10 +45,12 @@
> #include <plat/gpmc.h>
> #include <plat/nand.h>
> #include <plat/usb.h>
> +#include <plat/omap_device.h>
>
> #include "mux.h"
> #include "hsmmc.h"
> #include "timer-gp.h"
> +#include "pm.h"
>
> #define NAND_BLOCK_SIZE SZ_128K
>
> @@ -556,6 +559,52 @@ static struct omap_musb_board_data musb_board_data = {
> .power = 100,
> };
>
> +static void __init beagle_opp_init(void)
> +{
> + int r = 0;
> +
> + /* Initialize the omap3 opp table */
> + if (omap3_opp_init()) {
> + pr_err("%s: opp default init failed\n", __func__);
> + return;
> + }
> +
> + /* Custom OPP enabled for XM */
> + if (omap3_beagle_get_rev() == OMAP3BEAGLE_BOARD_XM) {
> + struct omap_hwmod *mh = omap_hwmod_lookup("mpu");
> + struct omap_hwmod *dh = omap_hwmod_lookup("iva");
> + struct device *dev;
> +
> + if (!mh || !dh) {
> + pr_err("%s: Aiee.. no mpu/dsp devices? %p %p\n",
> + __func__, mh, dh);
> + r = -EINVAL;
> + } else {
> + /* Enable MPU 1GHz and lower opps */
> + dev = &mh->od->pdev.dev;
> + r = opp_enable(dev, 800000000);
> + /* TODO: MPU 1GHz needs SR and ABB */
> +
> + /* Enable IVA 800MHz and lower opps */
> + dev = &dh->od->pdev.dev;
> + r |= opp_enable(dev, 660000000);
> + /* TODO: DSP 800MHz needs SR and ABB */
> + }
> + if (r) {
> + pr_err("%s: failed to enable higher opp %d\n",
> + __func__, r);
> + /*
> + * Cleanup - disable the higher freqs - we dont care
> + * about the results
> + */
> + dev = &mh->od->pdev.dev;
> + opp_disable(dev, 800000000);
> + dev = &dh->od->pdev.dev;
> + opp_disable(dev, 660000000);
> + }
> + }
> +}
> +
> static void __init omap3_beagle_init(void)
> {
> omap3_mux_init(board_mux, OMAP_PACKAGE_CBB);
> @@ -579,6 +628,7 @@ static void __init omap3_beagle_init(void)
> omap_mux_init_signal("sdrc_cke1", OMAP_PIN_OUTPUT);
>
> beagle_display_init();
> + beagle_opp_init();
> }
>
> MACHINE_START(OMAP3_BEAGLE, "OMAP3 Beagle Board")
^ permalink raw reply
* [PATCH 1/1] arm: omap: gpio: define .disable callback for gpio irq chip
From: Kevin Hilman @ 2011-01-05 23:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20110105192425.GA24729@besouro.research.nokia.com>
Eduardo Valentin <eduardo.valentin@nokia.com> writes:
> Hello Russell,
>
> On Wed, Jan 05, 2011 at 06:19:18PM +0000, Russell King wrote:
>> On Wed, Jan 05, 2011 at 07:58:03PM +0200, Eduardo Valentin wrote:
>> > Currently, if one calls disable_irq(gpio_irq), the irq
>> > won't get disabled.
>> >
>> > This is happening because the omap gpio code defines only
>> > a .mask callback. And the default_disable function is just
>> > a stub. The result is that, when someone calls disable_irq
>> > for an irq in a gpio line, it will be kept enabled.
>> >
>> > This patch solves this issue by setting the .disable
>> > callback to point to the same .mask callback.
>>
>> Amd this is a problem because?
>
> errr.. because the interrupt is enabled when it was supposed to be disabled?
>
As Russell pointed out, it's not actually "supposed" to be.
With lazy disable, what disable_irq() does is prevent the *handler* from
ever being called. If another interrupt arrives, it will be caught by
the genirq core, marked as IRQ_PENDING and then masked. This "don't
disable unless we really have to" is the desired behavior of the lazy
disable feature.
>>
>> The way this works is that although it isn't disabled at that point,
>> if it never triggers, then everything remains happy. However, if it
>> does trigger, the genirq code will then mask the interrupt and won't
>> call the handler.
>
> Right.. I didn't see from this point. I will check how that gets unmasked.
> But even so, if I understood correctly what you described, it would still
> open a time window which the system would see at least 1 interrupt during
> the time it was not suppose to. And that can wakeup a system which is in
> deep sleep mode, either via dynamic idle or static suspend.
>
> It is unlikely, I know. But it can still happen. And can be avoided.
If the GPIO is configured as a wakeup source, then wouldn't you want
activity on that GPIO to wake up the system?
If you don't want wakeups on that GPIO, then the driver should probably
be using disable_irq_wake().
Kevin
^ permalink raw reply
* [PATCH 3/6] i2c/pxa2xx: Add PCI support for PXA I2C controller
From: Greg KH @ 2011-01-05 23:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20110105230342.GO8638@n2100.arm.linux.org.uk>
On Wed, Jan 05, 2011 at 11:03:42PM +0000, Russell King - ARM Linux wrote:
> On Wed, Jan 05, 2011 at 05:51:00PM +0100, Sebastian Andrzej Siewior wrote:
> > +static void plat_dev_release(struct device *dev)
> > +{
> > + struct ce4100_i2c_device *sd = container_of(dev,
> > + struct ce4100_i2c_device, pdev.dev);
> > +
> > + of_device_node_put(&sd->pdev.dev);
> > +}
> > +static int add_i2c_device(struct pci_dev *dev, int bar,
> > + struct ce4100_i2c_device *sd)
> > +{
> > + struct platform_device *pdev = &sd->pdev;
> > + struct i2c_pxa_platform_data *pdata = &sd->pdata;
> ...
> > + pdev->name = "ce4100-i2c";
> > + pdev->dev.release = plat_dev_release;
> > + pdev->dev.parent = &dev->dev;
> > +
> > + pdev->dev.platform_data = pdata;
> > + pdev->resource = sd->res;
> ...
> > +static int __devinit ce4100_i2c_probe(struct pci_dev *dev,
> > + const struct pci_device_id *ent)
> > +{
> > + sds = kzalloc(sizeof(*sds), GFP_KERNEL);
> > + if (!sds)
> > + goto err_mem;
> > +
> > + pci_set_drvdata(dev, sds);
> > +
> > + for (i = 0; i < ARRAY_SIZE(sds->sd); i++) {
> > + ret = add_i2c_device(dev, i, &sds->sd[i]);
> > + if (ret) {
> > + while (--i >= 0)
> > + platform_device_unregister(&sds->sd[i].pdev);
> ...
> > +static void __devexit ce4100_i2c_remove(struct pci_dev *dev)
> ...
> > + for (i = 0; i < ARRAY_SIZE(sds->sd); i++)
> > + platform_device_unregister(&sds->sd[i].pdev);
> > +
> > + pci_disable_device(dev);
> > + kfree(sds);
> > +}
>
> I see we're still repeating the same mistakes with lifetime rules of
> sysfs objects.
>
> Any struct device which has been registered into the device model can
> remain indefinitely live after it's been unregistered (by, eg, if
> userspace holds a reference to it via sysfs.)
Actually this race is almost not possible these days with the rework
that Tejun did on sysfs, so it's not easy to test for this anymore.
> Only once the release method has been called is it safe to give up the
> memory backing it - and that also goes for the code comprising the
> function implementing the release.
Yes.
> This effectively prevents modules having release functions in them -
> while you can put module use count manipulation in to prevent unloading,
> it effectively prevents the module from being unloaded until you've
> unbound the device.
>
> I think you should be trying to use the platform_device_alloc()
> interfaces here, rather than trying to reinvent them. The only issue I
> see with that is the of_device_node_put() call. Maybe OF/DT/device model
> people can provide some pointers? Adding Greg for the device model
> maintainer view.
Don't reinvent functions that the core already provides, that's not a
good idea.
Sebastian, why didn't those functions work for you?
thanks,
greg k-h
^ permalink raw reply
* still nfs problems [Was: Linux 2.6.37-rc8]
From: Trond Myklebust @ 2011-01-05 23:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <AANLkTi=3Gu-rz=-OdNtUXn4qw60Df6=YePnzvB=s-+Ov@mail.gmail.com>
On Wed, 2011-01-05 at 13:30 -0800, Linus Torvalds wrote:
> On Wed, Jan 5, 2011 at 1:16 PM, Trond Myklebust
> <Trond.Myklebust@netapp.com> wrote:
> >
> > So what should be the preferred way to ensure data gets flushed when
> > you've written directly to a page, and then want to read through the
> > vm_map_ram() virtual range? Should we be adding new semantics to
> > flush_kernel_dcache_page()?
>
> The "preferred way" is actually simple: "don't do that". IOW, if some
> page is accessed through a virtual mapping you've set up, then
> _always_ access it through that virtual mapping.
>
> Now, when that is impossible (and yes, it sometimes is), then you
> should flush after doing all writes. And if you do the write through
> the regular kernel mapping, you should use flush_dcache_page(). And if
> you did it through the virtual mapping, you should use
> "flush_kernel_vmap_range()" or whatever.
>
> NOTE! I really didn't look those up very closely, and if the accesses
> can happen concurrently you are basically screwed, so you do need to
> do locking or something else to guarantee that there is some nice
> sequential order. And maybe I forgot something. Which is why I do
> suggest "don't do that" as a primary approach to the problem if at all
> possible.
>
> Oh, and you may need to flush before reading too (and many writes do
> end up being "read-modify-write" cycles) in case it's possible that
> you have stale data from a previous read that was then invalidated by
> a write to the aliasing address. Even if that write was flushed out,
> the stale read data may exist at the virtual address. I forget what
> all we required - in the end the only sane model is "virtual caches
> suck so bad that anybody who does them should be laughed at for being
> a retard".
Yes. The fix I sent out was a call to invalidate_kernel_vmap_range(),
which takes care of invalidating the cache prior to a virtual address
read.
My question was specifically about the write through the regular kernel
mapping: according to Russell and my reading of the cachetlb.txt
documentation, flush_dcache_page() is only guaranteed to have an effect
on page cache pages.
flush_kernel_dcache_page() (not to be confused with flush_dcache_page)
would appear to be the closest fit according to my reading of the
documentation, however the ARM implementation appears to be a no-op...
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust at netapp.com
www.netapp.com
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox