* [PATCH/WIP/RFC 13/14] shmobile-ipmmu: Remove unneeded lock_add spinlock
From: Laurent Pinchart @ 2012-12-16 17:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1355678760-27357-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com>
ipmmu_add_device() can never race with
shmobile_iommu_attach_all_devices(), called by ipmmu_iommu_init() as the
former is called from an arch initcall and the later from a subsys
initcall.
Remove the unneeded spinlock as well as the arm_iommu_attach_device() in
ipmmu_add_device(), as the condition that guards the call is always
false.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
drivers/iommu/shmobile-iommu.c | 18 +++---------------
1 files changed, 3 insertions(+), 15 deletions(-)
diff --git a/drivers/iommu/shmobile-iommu.c b/drivers/iommu/shmobile-iommu.c
index bf1d0a4..2592d25 100644
--- a/drivers/iommu/shmobile-iommu.c
+++ b/drivers/iommu/shmobile-iommu.c
@@ -51,7 +51,6 @@ static struct dma_iommu_mapping *iommu_mapping;
static struct device *ipmmu_devices;
static struct dma_pool *l1pool, *l2pool;
static spinlock_t lock;
-static DEFINE_SPINLOCK(lock_add);
static struct shmobile_iommu_domain *attached;
static int num_attached_devices;
@@ -313,15 +312,12 @@ static struct iommu_ops shmobile_iommu_ops = {
static int shmobile_iommu_attach_all_devices(struct shmobile_ipmmu *ipmmu)
{
struct device *dev;
- int ret = 0;
- spin_lock(&lock_add);
iommu_mapping = arm_iommu_create_mapping(&platform_bus_type, 0x0,
L1_LEN << 20, 0);
- if (IS_ERR_OR_NULL(iommu_mapping)) {
- ret = PTR_ERR(iommu_mapping);
- goto err;
- }
+ if (IS_ERR_OR_NULL(iommu_mapping))
+ return PTR_ERR(iommu_mapping);
+
for (dev = ipmmu_devices; dev; ) {
struct shmobile_iommu_arch_data *data = dev->archdata.iommu;
@@ -332,8 +328,6 @@ static int shmobile_iommu_attach_all_devices(struct shmobile_ipmmu *ipmmu)
dev = data->next;
}
-err:
- spin_unlock(&lock_add);
return 0;
}
@@ -347,14 +341,8 @@ int ipmmu_add_device(struct device *dev)
dev->archdata.iommu = data;
- spin_lock(&lock_add);
data->next = ipmmu_devices;
ipmmu_devices = dev;
- if (!IS_ERR_OR_NULL(iommu_mapping)) {
- if (arm_iommu_attach_device(dev, iommu_mapping))
- pr_err("arm_iommu_attach_device failed\n");
- }
- spin_unlock(&lock_add);
return 0;
}
--
1.7.8.6
^ permalink raw reply related
* [PATCH/WIP/RFC 14/14] shmobile-ipmmu: Store iommu_mapping in struct shmobile_ipmmu
From: Laurent Pinchart @ 2012-12-16 17:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1355678760-27357-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com>
And remove the global iommu_mapping variable.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
drivers/iommu/shmobile-iommu.c | 14 ++++++++------
drivers/iommu/shmobile-ipmmu.h | 4 ++++
2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/iommu/shmobile-iommu.c b/drivers/iommu/shmobile-iommu.c
index 2592d25..8cf45df 100644
--- a/drivers/iommu/shmobile-iommu.c
+++ b/drivers/iommu/shmobile-iommu.c
@@ -47,7 +47,6 @@ struct shmobile_iommu_domain {
atomic_t active;
};
-static struct dma_iommu_mapping *iommu_mapping;
static struct device *ipmmu_devices;
static struct dma_pool *l1pool, *l2pool;
static spinlock_t lock;
@@ -311,19 +310,22 @@ static struct iommu_ops shmobile_iommu_ops = {
static int shmobile_iommu_attach_all_devices(struct shmobile_ipmmu *ipmmu)
{
+ struct dma_iommu_mapping *mapping;
struct device *dev;
- iommu_mapping = arm_iommu_create_mapping(&platform_bus_type, 0x0,
- L1_LEN << 20, 0);
- if (IS_ERR_OR_NULL(iommu_mapping))
- return PTR_ERR(iommu_mapping);
+ mapping = arm_iommu_create_mapping(&platform_bus_type, 0,
+ L1_LEN << 20, 0);
+ if (IS_ERR(mapping))
+ return PTR_ERR(mapping);
+
+ ipmmu->iommu_mapping = mapping;
for (dev = ipmmu_devices; dev; ) {
struct shmobile_iommu_arch_data *data = dev->archdata.iommu;
data->ipmmu = ipmmu;
- if (arm_iommu_attach_device(dev, iommu_mapping))
+ if (arm_iommu_attach_device(dev, mapping))
pr_err("arm_iommu_attach_device failed\n");
dev = data->next;
diff --git a/drivers/iommu/shmobile-ipmmu.h b/drivers/iommu/shmobile-ipmmu.h
index 1458a97..f8f0f57 100644
--- a/drivers/iommu/shmobile-ipmmu.h
+++ b/drivers/iommu/shmobile-ipmmu.h
@@ -1,11 +1,15 @@
#ifndef __SHMOBILE_IPMMU_H__
#define __SHMOBILE_IPMMU_H__
+struct dma_iommu_mapping;
+
struct shmobile_ipmmu {
struct device *dev;
void __iomem *ipmmu_base;
int tlb_enabled;
struct mutex flush_lock;
+
+ struct dma_iommu_mapping *iommu_mapping;
};
#ifdef CONFIG_SHMOBILE_IPMMU_TLB
--
1.7.8.6
^ permalink raw reply related
* Re: [PATCH 6/7] ARM: mackerel: support booting with or without DT
From: Grant Likely @ 2012-12-16 20:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1355503531-7276-7-git-send-email-g.liakhovetski@gmx.de>
On Fri, 14 Dec 2012 17:45:30 +0100, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> This patch adds dynamic switching to booting either with or without DT.
> So far only a part of the board initialisation can be done via DT. Devices,
> that still need platform data are kept that way. Devices, that can be
> initialised from DT will not be supplied from the platform data, if a DT
> image is detected.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>
> +static int mackerel_i2c_bus_notify(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + struct device *dev = data;
> +
> + if (action != BUS_NOTIFY_ADD_DEVICE ||
> + strcmp(dev_name(dev->parent), "fff20000.i2c"))
> + return NOTIFY_DONE;
> +
> + i2c_new_device(to_i2c_adapter(dev), &i2c0_devices[1]);
> +
> + return NOTIFY_OK;
> +}
> +
> +static struct notifier_block mackerel_i2c_notifier = {
> + .notifier_call = mackerel_i2c_bus_notify,
> +};
This looks dodgy. It appears that the purpose of this hook is to create
the tca6408-keys device because it has some platform data. However,
this hook will try to create the device every time BUS_NOTIFY_ADD_DEVICE
happens on the fff20000.i2c bus *including* when the tca6408-keys device
gets added.
The correct approach when you need to add specific platform data is to
still put the device into the device tree and make the notifier look for
that specific device. Then the platform data pointer can be attached at
BUS_NOTIFY_ADD_DEVICE time.
However, it doesn't look like you need a notifier at all. From what I
can see the tca6408-keys device does have platform data, but it is all
simple (no callback pointers). You should instead encode the platform
data into device tree properties and extract them from the driver.
g.
^ permalink raw reply
* Re: [PATCH 6/7] ARM: mackerel: support booting with or without DT
From: Guennadi Liakhovetski @ 2012-12-16 21:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121216204636.ACD283E0AE3@localhost>
Hi Grant
On Sun, 16 Dec 2012, Grant Likely wrote:
> On Fri, 14 Dec 2012 17:45:30 +0100, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > This patch adds dynamic switching to booting either with or without DT.
> > So far only a part of the board initialisation can be done via DT. Devices,
> > that still need platform data are kept that way. Devices, that can be
> > initialised from DT will not be supplied from the platform data, if a DT
> > image is detected.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> >
> > +static int mackerel_i2c_bus_notify(struct notifier_block *nb,
> > + unsigned long action, void *data)
> > +{
> > + struct device *dev = data;
> > +
> > + if (action != BUS_NOTIFY_ADD_DEVICE ||
> > + strcmp(dev_name(dev->parent), "fff20000.i2c"))
> > + return NOTIFY_DONE;
> > +
> > + i2c_new_device(to_i2c_adapter(dev), &i2c0_devices[1]);
> > +
> > + return NOTIFY_OK;
> > +}
> > +
> > +static struct notifier_block mackerel_i2c_notifier = {
> > + .notifier_call = mackerel_i2c_bus_notify,
> > +};
>
> This looks dodgy. It appears that the purpose of this hook is to create
> the tca6408-keys device because it has some platform data. However,
> this hook will try to create the device every time BUS_NOTIFY_ADD_DEVICE
> happens on the fff20000.i2c bus *including* when the tca6408-keys device
> gets added.
I think, this is only called once, when the i2c adapter device is added in
i2c_register_adapter(). I2C client devices have these adapter devices as
their parents, and those devices have "i2c-%d" as their names. Since all
client devices get destroyed when the adapter is unbound, the above should
be safe.
> The correct approach when you need to add specific platform data is to
> still put the device into the device tree and make the notifier look for
> that specific device. Then the platform data pointer can be attached at
> BUS_NOTIFY_ADD_DEVICE time.
>
> However, it doesn't look like you need a notifier at all. From what I
> can see the tca6408-keys device does have platform data, but it is all
> simple (no callback pointers). You should instead encode the platform
> data into device tree properties and extract them from the driver.
Yes, this is the ultimate goal. But the purpose of this patch series is to
move whatever devices are possible right _now_ to DT. Ultimately all of
them should be migrated. So, yes, we could try to begin with tca6408,
because the above hack is certainly the ugliest one in this series, but in
principle this is just one of several devices, that we have to keep in
platform data for now and aim at removing these temporary hacks as soon as
respective drivers implement sufficient DT support.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH 2/2] net: sh_eth: Add Device Tree support
From: Nobuhiro Iwamatsu @ 2012-12-17 0:35 UTC (permalink / raw)
To: linux-sh
In-Reply-To: <1355666066-25649-3-git-send-email-hechtb+renesas@gmail.com>
Hi, Bastian.
I already sent a patch which support OF of sh-eth.
http://patchwork.ozlabs.org/patch/191713/.
And please CC netdev.
Best regards,
Nobuhiro
On Sun, Dec 16, 2012 at 10:54 PM, Bastian Hecht <hechtb@gmail.com> wrote:
> We add support for Device Tree probing in order to support the Armadillo
> DT testcase.
>
> Until we figure out what portions of the platform config are board
> dependent and should be used as OF properties and what portions are only
> SoC related and can be stored in the driver itself, I have attached all
> platform configurations to the OF match in the driver.
>
> Not-yet-signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
> ---
> drivers/net/ethernet/renesas/sh_eth.c | 72 ++++++++++++++++++++++++++++++++-
> 1 file changed, 71 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index c8bfea0..1fbbf69 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -31,6 +31,8 @@
> #include <linux/platform_device.h>
> #include <linux/mdio-bitbang.h>
> #include <linux/netdevice.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> #include <linux/phy.h>
> #include <linux/cache.h>
> #include <linux/io.h>
> @@ -2347,6 +2349,63 @@ static const struct net_device_ops sh_eth_netdev_ops = {
> .ndo_change_mtu = eth_change_mtu,
> };
>
> +#ifdef CONFIG_OF
> +struct sh_eth_soc_config {
> + int phy;
> + phy_interface_t phy_interface;
> + unsigned edmac_endian:1;
> + unsigned register_type:2;
> +};
> +
> +static struct sh_eth_soc_config sh_eth_r8a7740_config = {
> + .phy = 0x00,
> + .phy_interface = PHY_INTERFACE_MODE_MII,
> + .edmac_endian = EDMAC_LITTLE_ENDIAN,
> + .register_type = SH_ETH_REG_GIGABIT,
> +};
> +
> +static const struct of_device_id sh_eth_match[] = {
> + { .compatible = "renesas,sh-eth-r8a7740",
> + .data = &sh_eth_r8a7740_config },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, sh_eth_match);
> +
> +static struct sh_eth_plat_data *sh_eth_parse_dt(struct device *dev)
> +{
> + const struct of_device_id *match;
> + struct sh_eth_plat_data *pd;
> + struct sh_eth_soc_config *config;
> +
> + match = of_match_device(sh_eth_match, dev);
> + if (match)
> + config = (struct sh_eth_soc_config *)match->data;
> + else {
> + dev_err(dev, "%s: no OF configuration attached\n", __func__);
> + return NULL;
> + }
> +
> + pd = devm_kzalloc(dev, sizeof(struct sh_eth_plat_data), GFP_KERNEL);
> + if (!pd) {
> + dev_err(dev, "failed to allocate setup data\n");
> + return NULL;
> + }
> +
> + pd->phy = config->phy;
> + pd->phy_interface = config->phy_interface;
> + pd->edmac_endian = config->edmac_endian;
> + pd->register_type = config->register_type;
> +
> + return pd;
> +}
> +#else
> +static struct sh_eth_plat_data *sh_eth_parse_dt(struct device *dev)
> +{
> + return NULL;
> +}
> +#define sh_eth_match NULL
> +#endif
> +
> static int sh_eth_drv_probe(struct platform_device *pdev)
> {
> int ret, devno = 0;
> @@ -2403,7 +2462,17 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
> pm_runtime_enable(&pdev->dev);
> pm_runtime_resume(&pdev->dev);
>
> - pd = (struct sh_eth_plat_data *)(pdev->dev.platform_data);
> + if (pdev->dev.of_node)
> + pd = sh_eth_parse_dt(&pdev->dev);
> + else
> + pd = pdev->dev.platform_data;
> +
> + if (!pd) {
> + dev_err(&pdev->dev, "failed to obtain device info\n");
> + ret = -ENXIO;
> + goto out_release;
> + }
> +
> /* get PHY ID */
> mdp->phy_id = pd->phy;
> mdp->phy_interface = pd->phy_interface;
> @@ -2533,6 +2602,7 @@ static struct platform_driver sh_eth_driver = {
> .driver = {
> .name = CARDNAME,
> .pm = &sh_eth_dev_pm_ops,
> + .of_match_table = sh_eth_match,
> },
> };
>
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Nobuhiro Iwamatsu
^ permalink raw reply
* Re: [PATCH 2/2] net: sh_eth: Add Device Tree support
From: Kuninori Morimoto @ 2012-12-17 0:48 UTC (permalink / raw)
To: linux-sh
In-Reply-To: <1355666066-25649-3-git-send-email-hechtb+renesas@gmail.com>
Hi Bastian
> We add support for Device Tree probing in order to support the Armadillo
> DT testcase.
>
> Until we figure out what portions of the platform config are board
> dependent and should be used as OF properties and what portions are only
> SoC related and can be stored in the driver itself, I have attached all
> platform configurations to the OF match in the driver.
>
> Not-yet-signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
> ---
> drivers/net/ethernet/renesas/sh_eth.c | 72 ++++++++++++++++++++++++++++++++-
> 1 file changed, 71 insertions(+), 1 deletion(-)
I'm not sure current status,
but I guess Iwamatsu-san already had sent sh_eth DT support ?
http://permalink.gmane.org/gmane.linux.network/246019
Best regards
---
Kuninori Morimoto
^ permalink raw reply
* Re: [PATCH 1/2] ARM: shmobile: Armadillo800eva: DT boot testcase with ethernet support
From: Kuninori Morimoto @ 2012-12-17 0:54 UTC (permalink / raw)
To: linux-sh
In-Reply-To: <87bok7lm0h.wl%kuninori.morimoto.gx@renesas.com>
Hi Bastian
> This patch strips off most of the board code. We only keep the GPIO
> configurations used for the ethernet module sh_eth. We add the ethernet
> controller in the DT config file r8a7740-armadillo800eva.dts and make
> sure that clocks are turned on working with the different naming scheme
> for devices when using DT instead of the old platform code.
>
> Never-signed-off-by: Bastian Hecht <hechtb@gmail.com>
> ---
(snip)
> -/*
> - * CON1 Camera Module
> - * CON2 Extension Bus
> - * CON3 HDMI Output
> - * CON4 Composite Video Output
> - * CON5 H-UDI JTAG
> - * CON6 ARM JTAG
> - * CON7 SD1
> - * CON8 SD2
> - * CON9 RTC BackUp
> - * CON10 Monaural Mic Input
> - * CON11 Stereo Headphone Output
> - * CON12 Audio Line Output(L)
> - * CON13 Audio Line Output(R)
> - * CON14 AWL13 Module
> - * CON15 Extension
> - * CON16 LCD1
> - * CON17 LCD2
> - * CON19 Power Input
> - * CON20 USB1
> - * CON21 USB2
> - * CON22 Serial
> - * CON23 LAN
> - * CON24 USB3
> - * LED1 Camera LED(Yellow)
> - * LED2 Power LED (Green)
> - * ED3-LED6 User LED(Yellow)
> - * LED7 LAN link LED(Green)
> - * LED8 LAN activity LED(Yellow)
> - */
> -
> -/*
> - * DipSwitch
> - *
> - * SW1
> - *
> - * -12345678-+---------------+----------------------------
> - * 1 | boot | hermit
> - * 0 | boot | OS auto boot
> - * -12345678-+---------------+----------------------------
> - * 00 | boot device | eMMC
> - * 10 | boot device | SDHI0 (CON7)
> - * 01 | boot device | -
> - * 11 | boot device | Extension Buss (CS0)
> - * -12345678-+---------------+----------------------------
> - * 0 | Extension Bus | D8-D15 disable, eMMC enable
> - * 1 | Extension Bus | D8-D15 enable, eMMC disable
> - * -12345678-+---------------+----------------------------
> - * 0 | SDHI1 | COM8 disable, COM14 enable
> - * 1 | SDHI1 | COM8 enable, COM14 disable
> - * -12345678-+---------------+----------------------------
> - * 0 | USB0 | COM20 enable, COM24 disable
> - * 1 | USB0 | COM20 disable, COM24 enable
> - * -12345678-+---------------+----------------------------
> - * 00 | JTAG | SH-X2
> - * 10 | JTAG | ARM
> - * 01 | JTAG | -
> - * 11 | JTAG | Boundary Scan
> - *-----------+---------------+----------------------------
> - */
> -
> -/*
> - * FSI-WM8978
> - *
> - * this command is required when playback.
> - *
> - * # amixer set "Headphone" 50
> - */
Please keep these comments.
it is very important information for users.
^ permalink raw reply
* Re: [PATCH 0/2] Preliminary Armadillo DT support
From: Kuninori Morimoto @ 2012-12-17 1:01 UTC (permalink / raw)
To: linux-sh
In-Reply-To: <1355666066-25649-1-git-send-email-hechtb+renesas@gmail.com>
Hi Bastian
> This is a minimal setup for DT booting Armadillo boards. It's based on Simon
> Horman's reference implementation for the kz9mg board.
>
> The only device added so far is the ethernet controller sh_eth. For this the
> driver is enhanced to support DT-based probing in the 2nd patch.
>
> Care should be taken as the lines
> - /* armadillo 800 eva extal1 is 24MHz */
> - clk_set_rate(xtal1, 24000000);
> are removed and the r8a7740 code uses 25MHz by default. I'm unsure if just some clock
> calculations get incorrect or if we overclock our board somewhere.
>
> The patches are based on git://git.kernel.org/pub/scm/linux/kernel/git/horms/devel/of
> revision 9fcbfcfd31c782840ebb57f283c174c749467434.
>
> Bastian Hecht (2):
> ARM: shmobile: Armadillo800eva: DT boot testcase with ethernet
> support
> net: sh_eth: Add Device Tree support
>
> arch/arm/boot/dts/r8a7740-armadillo800eva.dts | 7 +
> arch/arm/mach-shmobile/board-armadillo800eva.c | 1167 +-----------------------
> arch/arm/mach-shmobile/clock-r8a7740.c | 1 +
> arch/arm/mach-shmobile/include/mach/common.h | 2 +
> drivers/net/ethernet/renesas/sh_eth.c | 72 +-
> 5 files changed, 86 insertions(+), 1163 deletions(-)
Preliminary Armadillo800eva DT support itself is very good.
But I guess these patches kill off "current" Armadillo800eva users who are using many devices ?
How about this ?
1) add new file like board-armadillo800eva-dt.c for DT support
2) keep adding new DT suuport on board-armadillo800eva-dt.c
3) If board-armadillo800eva-dt.c has full-support,
remove old board-armadillo800eva.c
Best regards
---
Kuninori Morimoto
^ permalink raw reply
* Re: [PATCH 3/4 v2] spi: sh-msiof: Add device tree parsing to driver
From: Nobuhiro Iwamatsu @ 2012-12-17 2:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1355313289-29769-3-git-send-email-hechtb+renesas@gmail.com>
Hi, Bastian.
On Wed, Dec 12, 2012 at 8:54 PM, Bastian Hecht <hechtb@gmail.com> wrote:
> From: Bastian Hecht <hechtb@gmail.com>
>
> This adds the capability to retrieve setup data from the device tree
> node. The usage of platform data is still available.
>
> Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
> ---
> v2:
> - renamed property "chip_select" to "num-cs"
> - renamed property "tx_fifo_size" to "renesas,tx-fifo-size"
> - renamed property "rx_fifo_size" to "renesas,rx-fifo-size"
>
> drivers/spi/spi-sh-msiof.c | 56 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
> index 96358d0..8b40d08 100644
> --- a/drivers/spi/spi-sh-msiof.c
> +++ b/drivers/spi/spi-sh-msiof.c
> @@ -20,6 +20,7 @@
> #include <linux/io.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> +#include <linux/of.h>
> #include <linux/platform_device.h>
> #include <linux/pm_runtime.h>
>
> @@ -592,6 +593,37 @@ static u32 sh_msiof_spi_txrx_word(struct spi_device *spi, unsigned nsecs,
> return 0;
> }
>
> +#ifdef CONFIG_OF
> +static struct sh_msiof_spi_info *sh_msiof_spi_parse_dt(struct device *dev)
> +{
> + struct sh_msiof_spi_info *info;
> + struct device_node *np = dev->of_node;
> + u32 num_cs = 0;
> +
> + info = devm_kzalloc(dev, sizeof(struct sh_msiof_spi_info), GFP_KERNEL);
> + if (!info) {
> + dev_err(dev, "failed to allocate setup data\n");
> + return NULL;
> + }
> +
> + /* Parse the MSIOF properties */
> + of_property_read_u32(np, "num-cs", &num_cs);
> + of_property_read_u32(np, "renesas,tx-fifo-size",
> + &info->tx_fifo_override);
> + of_property_read_u32(np, "renesas,rx-fifo-size",
> + &info->rx_fifo_override);
> +
> + info->num_chipselect = num_cs;
> +
> + return info;
> +}
> +#else
> +static struct sh_msiof_spi_info *sh_msiof_spi_parse_dt(struct device *dev)
> +{
> + return NULL;
> +}
> +#endif
> +
> static int sh_msiof_spi_probe(struct platform_device *pdev)
> {
> struct resource *r;
> @@ -610,7 +642,17 @@ static int sh_msiof_spi_probe(struct platform_device *pdev)
> p = spi_master_get_devdata(master);
>
> platform_set_drvdata(pdev, p);
> - p->info = pdev->dev.platform_data;
> + if (pdev->dev.of_node)
> + p->info = sh_msiof_spi_parse_dt(&pdev->dev);
> + else
> + p->info = pdev->dev.platform_data;
> +
> + if (!p->info) {
> + dev_err(&pdev->dev, "failed to obtain device info\n");
> + ret = -ENXIO;
> + goto err1;
> + }
> +
> init_completion(&p->done);
>
> p->clk = clk_get(&pdev->dev, NULL);
> @@ -715,6 +757,17 @@ static int sh_msiof_spi_runtime_nop(struct device *dev)
> return 0;
> }
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id sh_msiof_match[] = {
> + { .compatible = "renesas,sh-msiof", },
> + { .compatible = "renesas,sh-mobile-msiof", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, sh_msiof_match);
> +#else
> +#define sh_msiof_match NULL
> +#endif
You can remove ifdef if you use of_match_ptr().
> +
> static struct dev_pm_ops sh_msiof_spi_dev_pm_ops = {
> .runtime_suspend = sh_msiof_spi_runtime_nop,
> .runtime_resume = sh_msiof_spi_runtime_nop,
> @@ -727,6 +780,7 @@ static struct platform_driver sh_msiof_spi_drv = {
> .name = "spi_sh_msiof",
> .owner = THIS_MODULE,
> .pm = &sh_msiof_spi_dev_pm_ops,
> + .of_match_table = sh_msiof_match,
You can use of_match_ptr(sh_msiof_match).
> },
> };
> module_platform_driver(sh_msiof_spi_drv);
> --
> 1.7.9.5
Best regards,
Nobuhiro
--
Nobuhiro Iwamatsu
iwamatsu at {nigauri.org / debian.org}
GPG ID: 40AD1FA6
^ permalink raw reply
* Re: [PATCH/WIP/RFC 02/14] shmobile-iommu: Move IPMMU driver to drivers/iommu
From: Damian Hobson-Garcia @ 2012-12-17 3:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1355678760-27357-3-git-send-email-laurent.pinchart+renesas@ideasonboard.com>
Hi Laurent,
On 2012/12/17 2:25, Laurent Pinchart wrote:
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> arch/arm/mach-shmobile/Kconfig | 6 ------
> arch/arm/mach-shmobile/Makefile | 3 ---
> drivers/iommu/Kconfig | 6 ++++++
> drivers/iommu/Makefile | 1 +
> .../ipmmu.c => drivers/iommu/shmobile-ipmmu.c | 0
> 5 files changed, 7 insertions(+), 9 deletions(-)
> rename arch/arm/mach-shmobile/ipmmu.c => drivers/iommu/shmobile-ipmmu.c (100%)
I agree that arch/arm is not a good place, but I'm not completely sure
that ipmmu.c belongs in drivers/iommu. The reason is because of the PMB
functionality provided by the IPMMU. The PMB provides a fixed address
remapping capability that is completely unrelated to the IOMMU
functionality. Since this remapping is done by writing the IPMMU
registers directly, instead of via a page table it doesn't really fit in
well with the IOMMU API (it also supports things like tiled/linear
address translation, which require some other method to set up). Since
the PMB and the IOMMU functions of the IPPMU share the same register
address space, we would like to have one driver to handle the register
accesses of both of these functions. That driver is ipmmu.c. So if
ipmmu.c is in drivers/iommu, the entire IOMMU subsystem must be enabled
in order to use the PMB functionality.
So maybe it might be better to treat the IPMMU like a multifuction
device, with a core driver (ipmmu.c) in one location and the function
implementations in their own respective directories. Does drivers/mfd
sound like a good place for it?
Thanks,
Damian.
--
Damian Hobson-Garcia
IGEL Co.,Ltd
http://www.igel.co.jp
^ permalink raw reply
* Re: [PATCH 3/7] ARM: sh7372: support mixed DT and board code interrupt controller init
From: Guennadi Liakhovetski @ 2012-12-17 8:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121215075156.GD9073@verge.net.au>
Hi Simon
On Sat, 15 Dec 2012, Simon Horman wrote:
> On Fri, Dec 14, 2012 at 05:45:27PM +0100, Guennadi Liakhovetski wrote:
> > Extend DT interrupt controller initialisation to automatically fall back to
> > platform data based configuration, if booting without DT. This simplifies
> > implementing boards, capable of booting in either mode with a single kernel.
>
> Hi Guennadi,
>
> Do you have a case in mind where this will be used?
> My thinking until now has been that sh7372_init_irq_of() should only be called
> when a board is being initialised using DT.
As discussed in follow-ups to another patch from this series, the idea was
to only have one sh7372_init_irq(_of)() function exported, but it's not
too important, I'll drop this from v2.
Thanks
Guennadi
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> > arch/arm/mach-shmobile/intc-sh7372.c | 6 ++++++
> > 1 files changed, 6 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/arm/mach-shmobile/intc-sh7372.c b/arch/arm/mach-shmobile/intc-sh7372.c
> > index c923518..9c13ecc 100644
> > --- a/arch/arm/mach-shmobile/intc-sh7372.c
> > +++ b/arch/arm/mach-shmobile/intc-sh7372.c
> > @@ -23,6 +23,7 @@
> > #include <linux/irq.h>
> > #include <linux/io.h>
> > #include <linux/sh_intc.h>
> > +#include <mach/common.h>
> > #include <mach/intc.h>
> > #include <mach/irqs.h>
> > #include <asm/mach-types.h>
> > @@ -629,6 +630,11 @@ static const struct of_device_id irq_of_match[] __initconst = {
> >
> > void __init sh7372_init_irq_of(void)
> > {
> > + if (!of_have_populated_dt()) {
> > + sh7372_init_irq();
> > + return;
> > + }
> > +
> > of_irq_init(irq_of_match);
> >
> > sh7372_init_intc(0xe6940000, 0xe6950000, 0xffd20000, 0xffd50000,
> > --
> > 1.7.2.5
> >
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH 5/7] ARM: sh7372: allow boards supporting booting with or without DT
From: Guennadi Liakhovetski @ 2012-12-17 8:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121215080542.GF9073@verge.net.au>
Hi Simon
On Sat, 15 Dec 2012, Simon Horman wrote:
> On Fri, Dec 14, 2012 at 05:45:29PM +0100, Guennadi Liakhovetski wrote:
> > For boards booting without DT no changes should be caused by this patch.
> > When booting with DT, devices, whose drivers support DT probing, will not
> > be registered.
>
> This relates in part to my comment on "ARM: sh7372: support mixed DT and
> board code interrupt controller init".
>
> It is my understanding that sh7372_add_standard_devices_dt() already
> exists in setup-sh7372.c and that it is appropriate to use for bring
> up boards with DT.
I think, we partially clarified this already in the discussion of patch
6/7. This patch makes it possible to use standard functions, e.g.
sh7372_add_standard_devices() from both DT and non-DT boots. Whereas when
booting with DT, devices, for which sufficient DT support already exists,
have to be initialised from DT.
Thanks
Guennadi
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> > arch/arm/mach-shmobile/setup-sh7372.c | 17 ++++++++++++++---
> > 1 files changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm/mach-shmobile/setup-sh7372.c b/arch/arm/mach-shmobile/setup-sh7372.c
> > index bbe6e2a..3e6bf3d 100644
> > --- a/arch/arm/mach-shmobile/setup-sh7372.c
> > +++ b/arch/arm/mach-shmobile/setup-sh7372.c
> > @@ -981,9 +981,12 @@ static struct platform_device *sh7372_early_devices[] __initdata = {
> > &tmu01_device,
> > };
> >
> > -static struct platform_device *sh7372_late_devices[] __initdata = {
> > +static struct platform_device *sh7372_late_devices_dt[] __initdata = {
> > &iic0_device,
> > &iic1_device,
> > +};
> > +
> > +static struct platform_device *sh7372_late_devices[] __initdata = {
> > &dma0_device,
> > &dma1_device,
> > &dma2_device,
> > @@ -1012,13 +1015,11 @@ void __init sh7372_add_standard_devices(void)
> > { "A3SP", &scif4_device, },
> > { "A3SP", &scif5_device, },
> > { "A3SP", &scif6_device, },
> > - { "A3SP", &iic1_device, },
> > { "A3SP", &dma0_device, },
> > { "A3SP", &dma1_device, },
> > { "A3SP", &dma2_device, },
> > { "A3SP", &usb_dma0_device, },
> > { "A3SP", &usb_dma1_device, },
> > - { "A4R", &iic0_device, },
> > { "A4R", &veu0_device, },
> > { "A4R", &veu1_device, },
> > { "A4R", &veu2_device, },
> > @@ -1027,6 +1028,10 @@ void __init sh7372_add_standard_devices(void)
> > { "A4R", &tmu00_device, },
> > { "A4R", &tmu01_device, },
> > };
> > + struct pm_domain_device domain_devices_dt[] = {
> > + { "A3SP", &iic1_device, },
> > + { "A4R", &iic0_device, },
> > + };
> >
> > sh7372_init_pm_domains();
> >
> > @@ -1035,9 +1040,15 @@ void __init sh7372_add_standard_devices(void)
> >
> > platform_add_devices(sh7372_late_devices,
> > ARRAY_SIZE(sh7372_late_devices));
> > + if (!of_have_populated_dt())
> > + platform_add_devices(sh7372_late_devices_dt,
> > + ARRAY_SIZE(sh7372_late_devices_dt));
> >
> > rmobile_add_devices_to_domains(domain_devices,
> > ARRAY_SIZE(domain_devices));
> > + if (!of_have_populated_dt())
> > + rmobile_add_devices_to_domains(domain_devices_dt,
> > + ARRAY_SIZE(domain_devices_dt));
> > }
> >
> > static void __init sh7372_earlytimer_init(void)
> > --
> > 1.7.2.5
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH 02/10] SH: intc: Add support OF of IRQ
From: Laurent Pinchart @ 2012-12-17 8:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1355562224-29448-3-git-send-email-horms+renesas@verge.net.au>
Hi Iwamatsu-san,
Thank your for the patch.
On Saturday 15 December 2012 18:03:36 Simon Horman wrote:
> From: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
>
> Add information of device node to struct intc_desc.
>
> Cc: Magnus Damm <damm@opensource.se>
> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
>
> ---
>
> v7
> * Delete "renesas,sh_intcs" and "renesas,sh_intca_irq_pins" as compatible.
> Update their documentation.
> * Remove of_sh_intc_get_meminfo() and of_sh_intc_get_pint and
> of_sh_intc_get_intc(). They are not used.
>
> v2 - v6
> * No change
> ---
> Documentation/devicetree/bindings/sh/intc.txt | 15 +----
> drivers/sh/intc/core.c | 2 +-
> drivers/sh/intc/internals.h | 3 +-
> drivers/sh/intc/irqdomain.c | 6 +-
> drivers/sh/intc/of_intc.c | 76 ----------------------
> include/linux/sh_intc.h | 29 +---------
> 6 files changed, 9 insertions(+), 122 deletions(-)
Shouldn't this be squashed into patch 01/10 ? 01/10 adds DT bindings support,
and you already modify them in this patch.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 05/10] ARM: shmobile: Add DT table of INTC for sh7372
From: Guennadi Liakhovetski @ 2012-12-17 8:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1355562224-29448-6-git-send-email-horms+renesas@verge.net.au>
Hi Simon
On Sat, 15 Dec 2012, Simon Horman wrote:
> From: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
>
> Cc: Magnus Damm <damm@opensource.se>
> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
What happened to intcs support? Why has it been dropped? Without it many
important devices cannot be set up from DT, e.g. I2C #0.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH/WIP/RFC 02/14] shmobile-iommu: Move IPMMU driver to drivers/iommu
From: Laurent Pinchart @ 2012-12-17 8:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <50CE8D24.3010604@igel.co.jp>
Hi Damian,
(CC'ing iommu@lists.linux-foundation.org)
On Monday 17 December 2012 12:10:28 Damian Hobson-Garcia wrote:
> On 2012/12/17 2:25, Laurent Pinchart wrote:
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >
> > arch/arm/mach-shmobile/Kconfig | 6 ------
> > arch/arm/mach-shmobile/Makefile | 3 ---
> > drivers/iommu/Kconfig | 6 ++++++
> > drivers/iommu/Makefile | 1 +
> > .../ipmmu.c => drivers/iommu/shmobile-ipmmu.c | 0
> > 5 files changed, 7 insertions(+), 9 deletions(-)
> > rename arch/arm/mach-shmobile/ipmmu.c => drivers/iommu/shmobile-ipmmu.c
> > (100%)
>
> I agree that arch/arm is not a good place, but I'm not completely sure that
> ipmmu.c belongs in drivers/iommu. The reason is because of the PMB
> functionality provided by the IPMMU. The PMB provides a fixed address
> remapping capability that is completely unrelated to the IOMMU
> functionality. Since this remapping is done by writing the IPMMU registers
> directly, instead of via a page table it doesn't really fit in well with the
> IOMMU API (it also supports things like tiled/linear address translation,
> which require some other method to set up). Since the PMB and the IOMMU
> functions of the IPPMU share the same register address space, we would like
> to have one driver to handle the register accesses of both of these
> functions. That driver is ipmmu.c. So if ipmmu.c is in drivers/iommu, the
> entire IOMMU subsystem must be enabled in order to use the PMB
> functionality. So maybe it might be better to treat the IPMMU like a
> multifuction device, with a core driver (ipmmu.c) in one location and the
> function implementations in their own respective directories. Does
> drivers/mfd sound like a good place for it?
I've thought about this as well. The IPMMU indeed provides two different
functions, so drivers/mfd/ could be a candidate. This being said, both the
IOMMU function and the PMB function are related to virtual memory space
management, so they're not totally unrelated. I agree that the PMB function
isn't really an IOMMU in the sense that it will likely not be exposed through
the existing IOMMU API.
However, drivers/iommu/ seems to me like a more natural place to store the
IPMMU driver compared to drivers/mfd/. Enabling IOMMU support
(CONFIG_IOMMU_SUPPORT) doesn't mean the IOMMU core (CONFIG_IOMMU_API) will be
compiled in. There would thus be no extra code compiled in if the IOMMU
function of the IPMMU is disabled.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 09/10] ARM: shmobile: sh7372: Do not initialise TMU when using DT
From: Guennadi Liakhovetski @ 2012-12-17 9:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1355562224-29448-10-git-send-email-horms+renesas@verge.net.au>
Hi Simon
On Sat, 15 Dec 2012, Simon Horman wrote:
> This is in preparation for initialising INTC using DT.
> The proposed INTC configuration is not complete and does
> not allow the TMU to be initialised, to exclude it when using DT.
Not sure why you decide to not include incs, as I said, it is quite
important on sh7372. If we do find a way to include it back, then this
patch becomes unnecessary too.
Thanks
Guennadi
> Cc: Magnus Damm <damm@opensource.se>
> Cc: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> ---
> arch/arm/mach-shmobile/setup-sh7372.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/mach-shmobile/setup-sh7372.c b/arch/arm/mach-shmobile/setup-sh7372.c
> index a07954f..90af2e9 100644
> --- a/arch/arm/mach-shmobile/setup-sh7372.c
> +++ b/arch/arm/mach-shmobile/setup-sh7372.c
> @@ -968,7 +968,7 @@ static struct platform_device spu1_device = {
> .num_resources = ARRAY_SIZE(spu1_resources),
> };
>
> -static struct platform_device *sh7372_early_devices[] __initdata = {
> +static struct platform_device *sh7372_early_devices_dt[] __initdata = {
> &scif0_device,
> &scif1_device,
> &scif2_device,
> @@ -977,6 +977,9 @@ static struct platform_device *sh7372_early_devices[] __initdata = {
> &scif5_device,
> &scif6_device,
> &cmt2_device,
> +};
> +
> +static struct platform_device *sh7372_early_devices[] __initdata = {
> &tmu00_device,
> &tmu01_device,
> };
> @@ -1030,6 +1033,8 @@ void __init sh7372_add_standard_devices(void)
>
> sh7372_init_pm_domains();
>
> + platform_add_devices(sh7372_early_devices_dt,
> + ARRAY_SIZE(sh7372_early_devices_dt));
> platform_add_devices(sh7372_early_devices,
> ARRAY_SIZE(sh7372_early_devices));
>
> @@ -1048,6 +1053,8 @@ static void __init sh7372_earlytimer_init(void)
>
> void __init sh7372_add_early_devices(void)
> {
> + early_platform_add_devices(sh7372_early_devices_dt,
> + ARRAY_SIZE(sh7372_early_devices_dt));
> early_platform_add_devices(sh7372_early_devices,
> ARRAY_SIZE(sh7372_early_devices));
>
> @@ -1064,8 +1071,8 @@ void __init sh7372_add_early_devices_dt(void)
> {
> shmobile_setup_delay(800, 1, 3); /* Cortex-A8 @ 800MHz */
>
> - early_platform_add_devices(sh7372_early_devices,
> - ARRAY_SIZE(sh7372_early_devices));
> + early_platform_add_devices(sh7372_early_devices_dt,
> + ARRAY_SIZE(sh7372_early_devices_dt));
>
> /* setup early console here as well */
> shmobile_setup_console();
> @@ -1080,8 +1087,8 @@ void __init sh7372_add_standard_devices_dt(void)
> /* clocks are setup late during boot in the case of DT */
> sh7372_clock_init();
>
> - platform_add_devices(sh7372_early_devices,
> - ARRAY_SIZE(sh7372_early_devices));
> + platform_add_devices(sh7372_early_devices_dt,
> + ARRAY_SIZE(sh7372_early_devices_dt));
>
> of_platform_populate(NULL, of_default_bus_match_table,
> sh7372_auxdata_lookup, NULL);
> --
> 1.7.10.4
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH 2/2] ARM: SH-Mobile: sh73a0: Add CPU Hotplug
From: Bastian Hecht @ 2012-12-17 11:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <50CB6B14.1070500@gmail.com>
Hi Rob,
2012/12/14 Rob Herring <robherring2@gmail.com>:
> On 12/14/2012 09:33 AM, Bastian Hecht wrote:
>> Hi,
>>
>> 2012/12/14 Rob Herring <robherring2@gmail.com>:
>>> On 12/06/2012 06:08 AM, Bastian Hecht wrote:
>>>> From: Bastian Hecht <hechtb@gmail.com>
>>>>
>>>> Add the capability to add and remove CPUs on the fly.
>>>> The Cortex-A9 offers the possibility to take single cores out of the
>>>> MP Core. We add this capabilty taking care that caches are kept
>>>> coherent. For the actual shutdown via a WFI instruction, a code snippet
>>>> from the omap2 code tree is copied. Thanks for that! For verifying the
>>>> shutdown we rely on the internal SH73A0 Power Status Register
>>>> PSTR.
>>>>
>>>> Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
>>>> ---
>>>> arch/arm/mach-shmobile/headsmp-sh73a0.S | 46 ++++++++++++++++++++++++++
>>>> arch/arm/mach-shmobile/include/mach/common.h | 1 +
>>>> arch/arm/mach-shmobile/smp-sh73a0.c | 41 +++++++++++++++++++----
>>>> 3 files changed, 82 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-shmobile/headsmp-sh73a0.S b/arch/arm/mach-shmobile/headsmp-sh73a0.S
>>>> index bec4c0d..be463a3 100644
>>>> --- a/arch/arm/mach-shmobile/headsmp-sh73a0.S
>>>> +++ b/arch/arm/mach-shmobile/headsmp-sh73a0.S
>>>> @@ -23,6 +23,52 @@
>>>> #include <linux/init.h>
>>>> #include <asm/memory.h>
>>>>
>>>> +/* Taken from arch/arm/mach-omap2/sleep44xx.S. Thanks! */
>>>> +ENTRY(sh73a0_do_wfi)
>>>> + stmfd sp!, {lr}
>>>
>>> Why does the lr need to be pushed to the stack?
>>
>> Yes I must admit this is paradox - we never return but prepare to do
>> so... In the OMAP code they've got a lead out code in case the WFI
>> doesn't succeed. I see no reason how this could ever happen here but
>> to take a safe route I've decided to keep the mechanism to be able to
>> return and spit out an error message back in the C code.
>
> It's not clear to me that OMAP needed this either. The lr value would
> have to get lost during wfi.
>
>>>> +
>>>> + /*
>>>> + * Execute an ISB instruction to ensure that all of the
>>>> + * CP15 register changes have been committed.
>>>> + */
>>>> + isb
>>>
>>> Generally writes to cp15 registers that need an isb already do so.
>>
>> Ok nice, I'll check that and throw it out.
>>
>>>> +
>>>> + /*
>>>> + * Execute a barrier instruction to ensure that all cache,
>>>> + * TLB and branch predictor maintenance operations issued
>>>> + * by any CPU in the cluster have completed.
>>>> + */
>>>> + dsb
>>>> + dmb
>>>
>>> A dsb is a superset of a dmb, so you should not need both.
>>
>> Same here.
>>
>>>> +
>>>> + /*
>>>> + * Execute a WFI instruction and wait until the
>>>> + * STANDBYWFI output is asserted to indicate that the
>>>> + * CPU is in idle and low power state. CPU can specualatively
>>>> + * prefetch the instructions so add NOPs after WFI. Sixteen
>>>> + * NOPs as per Cortex-A9 pipeline.
>>>
>>> Why do you care what is prefetched? You're never coming back here, right?
>>
>> We can jump back to the paradox top. The idea seems to be to have a
>> clean pipeline in case the WFI doesn't succeed.
>> The thing about this whole code snippet is: I saw no reason to
>> reinvent the wheel and tinker on my own solution when there is code
>> that does the job in a clean way. I thought this could maybe be moved
>> to a general ARM code base when more people rely on it.
>>
>
> Blindly copying code is reinventing the wheel. You are making it appear
> that this is needed, but don't seem to have any reason why other than
> OMAP did it. If it is in fact needed, then it should be common.
>
> Use cpu_do_idle or figure out and explain why you can't. It's important
> to know that if we do go and consolidate this code later.
Yes after thinking about it a bit, I must completely agree to you.
Copying OMAP's code was done because of hesitation and uncertainty
that I could miss something and brings the drawbacks that you
mentioned. It's much straighter to go with cpu_do_idle.
Thanks!
Bastian
> Rob
>
^ permalink raw reply
* Re: [PATCH 2/2] net: sh_eth: Add Device Tree support
From: Bastian Hecht @ 2012-12-17 11:17 UTC (permalink / raw)
To: linux-sh
In-Reply-To: <1355666066-25649-3-git-send-email-hechtb+renesas@gmail.com>
Hi Nobuhiro,
2012/12/17 Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>:
> Hi, Bastian.
>
> I already sent a patch which support OF of sh-eth.
> http://patchwork.ozlabs.org/patch/191713/.
Oh great! Yes this looks much better than mine, thanks for pointing it out.
> And please CC netdev.
I omitted netdev as the patch was not intended for merge yet.
Thanks,
Bastian
> Best regards,
> Nobuhiro
>
> On Sun, Dec 16, 2012 at 10:54 PM, Bastian Hecht <hechtb@gmail.com> wrote:
>> We add support for Device Tree probing in order to support the Armadillo
>> DT testcase.
>>
>> Until we figure out what portions of the platform config are board
>> dependent and should be used as OF properties and what portions are only
>> SoC related and can be stored in the driver itself, I have attached all
>> platform configurations to the OF match in the driver.
>>
>> Not-yet-signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
>> ---
>> drivers/net/ethernet/renesas/sh_eth.c | 72 ++++++++++++++++++++++++++++++++-
>> 1 file changed, 71 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
>> index c8bfea0..1fbbf69 100644
>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
>> @@ -31,6 +31,8 @@
>> #include <linux/platform_device.h>
>> #include <linux/mdio-bitbang.h>
>> #include <linux/netdevice.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> #include <linux/phy.h>
>> #include <linux/cache.h>
>> #include <linux/io.h>
>> @@ -2347,6 +2349,63 @@ static const struct net_device_ops sh_eth_netdev_ops = {
>> .ndo_change_mtu = eth_change_mtu,
>> };
>>
>> +#ifdef CONFIG_OF
>> +struct sh_eth_soc_config {
>> + int phy;
>> + phy_interface_t phy_interface;
>> + unsigned edmac_endian:1;
>> + unsigned register_type:2;
>> +};
>> +
>> +static struct sh_eth_soc_config sh_eth_r8a7740_config = {
>> + .phy = 0x00,
>> + .phy_interface = PHY_INTERFACE_MODE_MII,
>> + .edmac_endian = EDMAC_LITTLE_ENDIAN,
>> + .register_type = SH_ETH_REG_GIGABIT,
>> +};
>> +
>> +static const struct of_device_id sh_eth_match[] = {
>> + { .compatible = "renesas,sh-eth-r8a7740",
>> + .data = &sh_eth_r8a7740_config },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, sh_eth_match);
>> +
>> +static struct sh_eth_plat_data *sh_eth_parse_dt(struct device *dev)
>> +{
>> + const struct of_device_id *match;
>> + struct sh_eth_plat_data *pd;
>> + struct sh_eth_soc_config *config;
>> +
>> + match = of_match_device(sh_eth_match, dev);
>> + if (match)
>> + config = (struct sh_eth_soc_config *)match->data;
>> + else {
>> + dev_err(dev, "%s: no OF configuration attached\n", __func__);
>> + return NULL;
>> + }
>> +
>> + pd = devm_kzalloc(dev, sizeof(struct sh_eth_plat_data), GFP_KERNEL);
>> + if (!pd) {
>> + dev_err(dev, "failed to allocate setup data\n");
>> + return NULL;
>> + }
>> +
>> + pd->phy = config->phy;
>> + pd->phy_interface = config->phy_interface;
>> + pd->edmac_endian = config->edmac_endian;
>> + pd->register_type = config->register_type;
>> +
>> + return pd;
>> +}
>> +#else
>> +static struct sh_eth_plat_data *sh_eth_parse_dt(struct device *dev)
>> +{
>> + return NULL;
>> +}
>> +#define sh_eth_match NULL
>> +#endif
>> +
>> static int sh_eth_drv_probe(struct platform_device *pdev)
>> {
>> int ret, devno = 0;
>> @@ -2403,7 +2462,17 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
>> pm_runtime_enable(&pdev->dev);
>> pm_runtime_resume(&pdev->dev);
>>
>> - pd = (struct sh_eth_plat_data *)(pdev->dev.platform_data);
>> + if (pdev->dev.of_node)
>> + pd = sh_eth_parse_dt(&pdev->dev);
>> + else
>> + pd = pdev->dev.platform_data;
>> +
>> + if (!pd) {
>> + dev_err(&pdev->dev, "failed to obtain device info\n");
>> + ret = -ENXIO;
>> + goto out_release;
>> + }
>> +
>> /* get PHY ID */
>> mdp->phy_id = pd->phy;
>> mdp->phy_interface = pd->phy_interface;
>> @@ -2533,6 +2602,7 @@ static struct platform_driver sh_eth_driver = {
>> .driver = {
>> .name = CARDNAME,
>> .pm = &sh_eth_dev_pm_ops,
>> + .of_match_table = sh_eth_match,
>> },
>> };
>>
>> --
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Nobuhiro Iwamatsu
^ permalink raw reply
* Re: [PATCH 0/2] Preliminary Armadillo DT support
From: Bastian Hecht @ 2012-12-17 11:23 UTC (permalink / raw)
To: linux-sh
In-Reply-To: <1355666066-25649-1-git-send-email-hechtb+renesas@gmail.com>
Hi,
>
> Preliminary Armadillo800eva DT support itself is very good.
> But I guess these patches kill off "current" Armadillo800eva users who are using many devices ?
>
> How about this ?
> 1) add new file like board-armadillo800eva-dt.c for DT support
> 2) keep adding new DT suuport on board-armadillo800eva-dt.c
> 3) If board-armadillo800eva-dt.c has full-support,
> remove old board-armadillo800eva.c
>
Sounds like a plan!
Yes this looks like the way Simon has made up his kzm9g reference
implementation. So I suppose we want this extra board file in the
mainline until 3) happens?
Cheers,
Bastian
^ permalink raw reply
* Re: [PATCH 3/4 v2] spi: sh-msiof: Add device tree parsing to driver
From: Bastian Hecht @ 2012-12-17 11:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CABMQnV+CSEKwq=4kQYUDXOuARLA_WgvnkJEME_VeYmAv3PLXBg@mail.gmail.com>
Hi Nobuhiro,
2012/12/17 Nobuhiro Iwamatsu <iwamatsu@nigauri.org>:
> Hi, Bastian.
>
> On Wed, Dec 12, 2012 at 8:54 PM, Bastian Hecht <hechtb@gmail.com> wrote:
>> From: Bastian Hecht <hechtb@gmail.com>
>>
>> This adds the capability to retrieve setup data from the device tree
>> node. The usage of platform data is still available.
>>
>> Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
>> ---
>> v2:
>> - renamed property "chip_select" to "num-cs"
>> - renamed property "tx_fifo_size" to "renesas,tx-fifo-size"
>> - renamed property "rx_fifo_size" to "renesas,rx-fifo-size"
>>
>> drivers/spi/spi-sh-msiof.c | 56 +++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 55 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
>> index 96358d0..8b40d08 100644
>> --- a/drivers/spi/spi-sh-msiof.c
>> +++ b/drivers/spi/spi-sh-msiof.c
>> @@ -20,6 +20,7 @@
>> #include <linux/io.h>
>> #include <linux/kernel.h>
>> #include <linux/module.h>
>> +#include <linux/of.h>
>> #include <linux/platform_device.h>
>> #include <linux/pm_runtime.h>
>>
>> @@ -592,6 +593,37 @@ static u32 sh_msiof_spi_txrx_word(struct spi_device *spi, unsigned nsecs,
>> return 0;
>> }
>>
>> +#ifdef CONFIG_OF
>> +static struct sh_msiof_spi_info *sh_msiof_spi_parse_dt(struct device *dev)
>> +{
>> + struct sh_msiof_spi_info *info;
>> + struct device_node *np = dev->of_node;
>> + u32 num_cs = 0;
>> +
>> + info = devm_kzalloc(dev, sizeof(struct sh_msiof_spi_info), GFP_KERNEL);
>> + if (!info) {
>> + dev_err(dev, "failed to allocate setup data\n");
>> + return NULL;
>> + }
>> +
>> + /* Parse the MSIOF properties */
>> + of_property_read_u32(np, "num-cs", &num_cs);
>> + of_property_read_u32(np, "renesas,tx-fifo-size",
>> + &info->tx_fifo_override);
>> + of_property_read_u32(np, "renesas,rx-fifo-size",
>> + &info->rx_fifo_override);
>> +
>> + info->num_chipselect = num_cs;
>> +
>> + return info;
>> +}
>> +#else
>> +static struct sh_msiof_spi_info *sh_msiof_spi_parse_dt(struct device *dev)
>> +{
>> + return NULL;
>> +}
>> +#endif
>> +
>> static int sh_msiof_spi_probe(struct platform_device *pdev)
>> {
>> struct resource *r;
>> @@ -610,7 +642,17 @@ static int sh_msiof_spi_probe(struct platform_device *pdev)
>> p = spi_master_get_devdata(master);
>>
>> platform_set_drvdata(pdev, p);
>> - p->info = pdev->dev.platform_data;
>> + if (pdev->dev.of_node)
>> + p->info = sh_msiof_spi_parse_dt(&pdev->dev);
>> + else
>> + p->info = pdev->dev.platform_data;
>> +
>> + if (!p->info) {
>> + dev_err(&pdev->dev, "failed to obtain device info\n");
>> + ret = -ENXIO;
>> + goto err1;
>> + }
>> +
>> init_completion(&p->done);
>>
>> p->clk = clk_get(&pdev->dev, NULL);
>> @@ -715,6 +757,17 @@ static int sh_msiof_spi_runtime_nop(struct device *dev)
>> return 0;
>> }
>>
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id sh_msiof_match[] = {
>> + { .compatible = "renesas,sh-msiof", },
>> + { .compatible = "renesas,sh-mobile-msiof", },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, sh_msiof_match);
>> +#else
>> +#define sh_msiof_match NULL
>> +#endif
>
> You can remove ifdef if you use of_match_ptr().
>
>> +
>> static struct dev_pm_ops sh_msiof_spi_dev_pm_ops = {
>> .runtime_suspend = sh_msiof_spi_runtime_nop,
>> .runtime_resume = sh_msiof_spi_runtime_nop,
>> @@ -727,6 +780,7 @@ static struct platform_driver sh_msiof_spi_drv = {
>> .name = "spi_sh_msiof",
>> .owner = THIS_MODULE,
>> .pm = &sh_msiof_spi_dev_pm_ops,
>> + .of_match_table = sh_msiof_match,
>
> You can use of_match_ptr(sh_msiof_match).
>
Ok very nice, will do so.
Thanks,
Bastian
>> },
>> };
>> module_platform_driver(sh_msiof_spi_drv);
>> --
>> 1.7.9.5
>
> Best regards,
> Nobuhiro
>
> --
> Nobuhiro Iwamatsu
> iwamatsu at {nigauri.org / debian.org}
> GPG ID: 40AD1FA6
^ permalink raw reply
* [PATCH v2 6/7] ARM: mackerel: support booting with or without DT
From: Guennadi Liakhovetski @ 2012-12-17 12:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1355503531-7276-7-git-send-email-g.liakhovetski@gmx.de>
This patch adds dynamic switching to booting either with or without DT.
So far only a part of the board initialisation can be done via DT. Devices,
that still need platform data are kept that way. Devices, that can be
initialised from DT will not be supplied from the platform data, if a DT
image is detected.
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
Hi Simon
As suggested by you we drop patch #3/7 "ARM: sh7372: support mixed DT and
board code interrupt controller init" and as suggested by Grant we drop
patch #4/7 "ARM: sh7372: add clock lookup entries for DT-based devices"
(you can drop it from your soc5 branch too now.) As a result we update
this patch as follows:
v2:
1. use a lookup table to still be able to use non-DT clock names (thanks
to Grant for pointing out)
2. use sh7372_init_irq in the non-DT case (thanks to Simon)
Patches 1, 2, 5 and 7 don't change.
Thanks
Guennadi
arch/arm/mach-shmobile/board-mackerel.c | 100 +++++++++++++++++++++++++-----
1 files changed, 83 insertions(+), 17 deletions(-)
diff --git a/arch/arm/mach-shmobile/board-mackerel.c b/arch/arm/mach-shmobile/board-mackerel.c
index 39b8f2e..6d13b3a 100644
--- a/arch/arm/mach-shmobile/board-mackerel.c
+++ b/arch/arm/mach-shmobile/board-mackerel.c
@@ -1326,7 +1326,6 @@ static struct platform_device mackerel_camera = {
static struct platform_device *mackerel_devices[] __initdata = {
&nor_flash_device,
- &smc911x_device,
&lcdc_device,
&usbhs0_device,
&usbhs1_device,
@@ -1335,17 +1334,21 @@ static struct platform_device *mackerel_devices[] __initdata = {
&fsi_ak4643_device,
&fsi_hdmi_device,
&nand_flash_device,
+ &ceu_device,
+ &mackerel_camera,
+ &hdmi_device,
+ &hdmi_lcdc_device,
+ &meram_device,
+};
+
+static struct platform_device *mackerel_devices_dt[] __initdata = {
+ &smc911x_device,
&sdhi0_device,
#if !defined(CONFIG_MMC_SH_MMCIF) && !defined(CONFIG_MMC_SH_MMCIF_MODULE)
&sdhi1_device,
#endif
&sdhi2_device,
&sh_mmcif_device,
- &ceu_device,
- &mackerel_camera,
- &hdmi_device,
- &hdmi_lcdc_device,
- &meram_device,
};
/* Keypad Initialization */
@@ -1404,6 +1407,41 @@ static struct i2c_board_info i2c1_devices[] = {
},
};
+static int mackerel_i2c_bus_notify(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct device *dev = data;
+
+ /* We're only interested in 1 event: when the adapter is added */
+ if (action != BUS_NOTIFY_ADD_DEVICE ||
+ strcmp(dev_name(dev->parent), "i2c-sh_mobile.0"))
+ return NOTIFY_DONE;
+
+ i2c_new_device(to_i2c_adapter(dev), &i2c0_devices[1]);
+
+ return NOTIFY_OK;
+}
+
+static struct notifier_block mackerel_i2c_notifier = {
+ .notifier_call = mackerel_i2c_bus_notify,
+};
+
+/*
+ * Auxdata required until real OF clocks are provided
+ */
+struct of_dev_auxdata mackerel_auxdata_lookup[] __initdata = {
+ OF_DEV_AUXDATA("renesas,rmobile-iic", 0xfff20000, "i2c-sh_mobile.0", NULL),
+ OF_DEV_AUXDATA("renesas,rmobile-iic", 0xe6c20000, "i2c-sh_mobile.1", NULL),
+ OF_DEV_AUXDATA("renesas,rmobile-iic", 0xfff30000, "i2c-sh_mobile.2", NULL),
+ OF_DEV_AUXDATA("renesas,rmobile-iic", 0xe6d20000, "i2c-sh_mobile.3", NULL),
+ OF_DEV_AUXDATA("renesas,rmobile-iic", 0xe6d30000, "i2c-sh_mobile.4", NULL),
+ OF_DEV_AUXDATA("renesas,shmobile-sdhi", 0xe6850000, "sh_mobile_sdhi.0", NULL),
+ OF_DEV_AUXDATA("renesas,shmobile-sdhi", 0xe6860000, "sh_mobile_sdhi.1", NULL),
+ OF_DEV_AUXDATA("renesas,shmobile-sdhi", 0xe6870000, "sh_mobile_sdhi.2", NULL),
+ OF_DEV_AUXDATA("renesas,sh-mmcif", 0xe6bd0000, "sh_mmcif.0", NULL),
+ {},
+};
+
#define GPIO_PORT9CR IOMEM(0xE6051009)
#define GPIO_PORT10CR IOMEM(0xE605100A)
#define GPIO_PORT167CR IOMEM(0xE60520A7)
@@ -1420,22 +1458,26 @@ static void __init mackerel_init(void)
{ "A3SP", &usbhs0_device, },
{ "A3SP", &usbhs1_device, },
{ "A3SP", &nand_flash_device, },
+ { "A4R", &ceu_device, },
+ };
+ struct pm_domain_device domain_devices_dt[] = {
{ "A3SP", &sh_mmcif_device, },
{ "A3SP", &sdhi0_device, },
#if !defined(CONFIG_MMC_SH_MMCIF) && !defined(CONFIG_MMC_SH_MMCIF_MODULE)
{ "A3SP", &sdhi1_device, },
#endif
{ "A3SP", &sdhi2_device, },
- { "A4R", &ceu_device, },
};
u32 srcr4;
struct clk *clk;
- regulator_register_always_on(0, "fixed-1.8V", fixed1v8_power_consumers,
- ARRAY_SIZE(fixed1v8_power_consumers), 1800000);
- regulator_register_always_on(1, "fixed-3.3V", fixed3v3_power_consumers,
- ARRAY_SIZE(fixed3v3_power_consumers), 3300000);
- regulator_register_fixed(2, dummy_supplies, ARRAY_SIZE(dummy_supplies));
+ if (!of_have_populated_dt()) {
+ regulator_register_always_on(0, "fixed-1.8V", fixed1v8_power_consumers,
+ ARRAY_SIZE(fixed1v8_power_consumers), 1800000);
+ regulator_register_always_on(1, "fixed-3.3V", fixed3v3_power_consumers,
+ ARRAY_SIZE(fixed3v3_power_consumers), 3300000);
+ regulator_register_fixed(2, dummy_supplies, ARRAY_SIZE(dummy_supplies));
+ }
/* External clock source */
clk_set_rate(&sh7372_dv_clki_clk, 27000000);
@@ -1633,22 +1675,36 @@ static void __init mackerel_init(void)
udelay(50);
__raw_writel(srcr4 & ~(1 << 13), SRCR4);
- i2c_register_board_info(0, i2c0_devices,
- ARRAY_SIZE(i2c0_devices));
- i2c_register_board_info(1, i2c1_devices,
- ARRAY_SIZE(i2c1_devices));
+ if (!of_have_populated_dt()) {
+ i2c_register_board_info(0, i2c0_devices,
+ ARRAY_SIZE(i2c0_devices));
+ i2c_register_board_info(1, i2c1_devices,
+ ARRAY_SIZE(i2c1_devices));
+ } else {
+ bus_register_notifier(&i2c_bus_type,
+ &mackerel_i2c_notifier);
+ }
sh7372_add_standard_devices();
platform_add_devices(mackerel_devices, ARRAY_SIZE(mackerel_devices));
+ if (!of_have_populated_dt())
+ platform_add_devices(mackerel_devices_dt,
+ ARRAY_SIZE(mackerel_devices_dt));
rmobile_add_devices_to_domains(domain_devices,
ARRAY_SIZE(domain_devices));
+ if (!of_have_populated_dt())
+ rmobile_add_devices_to_domains(domain_devices_dt,
+ ARRAY_SIZE(domain_devices_dt));
hdmi_init_pm_clock();
sh7372_pm_init();
pm_clk_add(&fsi_device.dev, "spu2");
pm_clk_add(&hdmi_lcdc_device.dev, "hdmi");
+
+ of_platform_populate(NULL, of_default_bus_match_table,
+ mackerel_auxdata_lookup, NULL);
}
static const char *mackerel_boards_compat_dt[] __initdata = {
@@ -1659,10 +1715,20 @@ static const char *mackerel_boards_compat_dt[] __initdata = {
DT_MACHINE_START(MACKEREL_DT, "mackerel")
.map_io = sh7372_map_io,
.init_early = sh7372_add_early_devices,
+ .init_irq = sh7372_init_irq_of,
+ .handle_irq = shmobile_handle_irq_intc,
+ .init_machine = mackerel_init,
+ .init_late = sh7372_pm_init_late,
+ .timer = &shmobile_timer,
+ .dt_compat = mackerel_boards_compat_dt,
+MACHINE_END
+
+MACHINE_START(MACKEREL, "mackerel")
+ .map_io = sh7372_map_io,
+ .init_early = sh7372_add_early_devices,
.init_irq = sh7372_init_irq,
.handle_irq = shmobile_handle_irq_intc,
.init_machine = mackerel_init,
.init_late = sh7372_pm_init_late,
.timer = &shmobile_timer,
- .dt_compat = mackerel_boards_compat_dt,
MACHINE_END
--
1.7.2.5
^ permalink raw reply related
* Re: [PATCH 6/7] ARM: mackerel: support booting with or without DT
From: Grant Likely @ 2012-12-17 16:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <Pine.LNX.4.64.1212162224380.20059@axis700.grange>
On Sun, 16 Dec 2012 22:36:56 +0100 (CET), Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Hi Grant
>
> On Sun, 16 Dec 2012, Grant Likely wrote:
>
> > On Fri, 14 Dec 2012 17:45:30 +0100, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > > This patch adds dynamic switching to booting either with or without DT.
> > > So far only a part of the board initialisation can be done via DT. Devices,
> > > that still need platform data are kept that way. Devices, that can be
> > > initialised from DT will not be supplied from the platform data, if a DT
> > > image is detected.
> > >
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > ---
> > >
> > > +static int mackerel_i2c_bus_notify(struct notifier_block *nb,
> > > + unsigned long action, void *data)
> > > +{
> > > + struct device *dev = data;
> > > +
> > > + if (action != BUS_NOTIFY_ADD_DEVICE ||
> > > + strcmp(dev_name(dev->parent), "fff20000.i2c"))
> > > + return NOTIFY_DONE;
> > > +
> > > + i2c_new_device(to_i2c_adapter(dev), &i2c0_devices[1]);
> > > +
> > > + return NOTIFY_OK;
> > > +}
> > > +
> > > +static struct notifier_block mackerel_i2c_notifier = {
> > > + .notifier_call = mackerel_i2c_bus_notify,
> > > +};
> >
> > This looks dodgy. It appears that the purpose of this hook is to create
> > the tca6408-keys device because it has some platform data. However,
> > this hook will try to create the device every time BUS_NOTIFY_ADD_DEVICE
> > happens on the fff20000.i2c bus *including* when the tca6408-keys device
> > gets added.
>
> I think, this is only called once, when the i2c adapter device is added in
> i2c_register_adapter(). I2C client devices have these adapter devices as
> their parents, and those devices have "i2c-%d" as their names. Since all
> client devices get destroyed when the adapter is unbound, the above should
> be safe.
Take another look. The way the bus notifiers work is they get called once for every device registered with the given bus type. That means i2c_client objects, not i3c
>
> > The correct approach when you need to add specific platform data is to
> > still put the device into the device tree and make the notifier look for
> > that specific device. Then the platform data pointer can be attached at
> > BUS_NOTIFY_ADD_DEVICE time.
> >
> > However, it doesn't look like you need a notifier at all. From what I
> > can see the tca6408-keys device does have platform data, but it is all
> > simple (no callback pointers). You should instead encode the platform
> > data into device tree properties and extract them from the driver.
>
> Yes, this is the ultimate goal. But the purpose of this patch series is to
> move whatever devices are possible right _now_ to DT. Ultimately all of
> them should be migrated. So, yes, we could try to begin with tca6408,
> because the above hack is certainly the ugliest one in this series, but in
> principle this is just one of several devices, that we have to keep in
> platform data for now and aim at removing these temporary hacks as soon as
> respective drivers implement sufficient DT support.
>
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
--
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.
^ permalink raw reply
* Re: [PATCH 6/7] ARM: mackerel: support booting with or without DT
From: Guennadi Liakhovetski @ 2012-12-17 16:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121217163852.C57FD3E0BDD@localhost>
Hi Grant
On Mon, 17 Dec 2012, Grant Likely wrote:
> On Sun, 16 Dec 2012 22:36:56 +0100 (CET), Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Hi Grant
> >
> > On Sun, 16 Dec 2012, Grant Likely wrote:
> >
> > > On Fri, 14 Dec 2012 17:45:30 +0100, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > > > This patch adds dynamic switching to booting either with or without DT.
> > > > So far only a part of the board initialisation can be done via DT. Devices,
> > > > that still need platform data are kept that way. Devices, that can be
> > > > initialised from DT will not be supplied from the platform data, if a DT
> > > > image is detected.
> > > >
> > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > > ---
> > > >
> > > > +static int mackerel_i2c_bus_notify(struct notifier_block *nb,
> > > > + unsigned long action, void *data)
> > > > +{
> > > > + struct device *dev = data;
> > > > +
> > > > + if (action != BUS_NOTIFY_ADD_DEVICE ||
> > > > + strcmp(dev_name(dev->parent), "fff20000.i2c"))
> > > > + return NOTIFY_DONE;
> > > > +
> > > > + i2c_new_device(to_i2c_adapter(dev), &i2c0_devices[1]);
> > > > +
> > > > + return NOTIFY_OK;
> > > > +}
> > > > +
> > > > +static struct notifier_block mackerel_i2c_notifier = {
> > > > + .notifier_call = mackerel_i2c_bus_notify,
> > > > +};
> > >
> > > This looks dodgy. It appears that the purpose of this hook is to create
> > > the tca6408-keys device because it has some platform data. However,
> > > this hook will try to create the device every time BUS_NOTIFY_ADD_DEVICE
> > > happens on the fff20000.i2c bus *including* when the tca6408-keys device
> > > gets added.
> >
> > I think, this is only called once, when the i2c adapter device is added in
> > i2c_register_adapter(). I2C client devices have these adapter devices as
> > their parents, and those devices have "i2c-%d" as their names. Since all
> > client devices get destroyed when the adapter is unbound, the above should
> > be safe.
>
> Take another look. The way the bus notifiers work is they get called
> once for every device registered with the given bus type. That means
> i2c_client objects, not i3c
I did, and I also tested. Both I2C clients and I2C adapters are placed on
the i2c_bus_type. So, yes, you're right, the above notifier will be called
upon registration of each adapter and client. But, the string comparison
in the "if" above will only match for the I2C adapter, so, no recursion.
Thanks
Guennadi
> > > The correct approach when you need to add specific platform data is to
> > > still put the device into the device tree and make the notifier look for
> > > that specific device. Then the platform data pointer can be attached at
> > > BUS_NOTIFY_ADD_DEVICE time.
> > >
> > > However, it doesn't look like you need a notifier at all. From what I
> > > can see the tca6408-keys device does have platform data, but it is all
> > > simple (no callback pointers). You should instead encode the platform
> > > data into device tree properties and extract them from the driver.
> >
> > Yes, this is the ultimate goal. But the purpose of this patch series is to
> > move whatever devices are possible right _now_ to DT. Ultimately all of
> > them should be migrated. So, yes, we could try to begin with tca6408,
> > because the above hack is certainly the ugliest one in this series, but in
> > principle this is just one of several devices, that we have to keep in
> > platform data for now and aim at removing these temporary hacks as soon as
> > respective drivers implement sufficient DT support.
> >
> > Thanks
> > Guennadi
> > ---
> > Guennadi Liakhovetski, Ph.D.
> > Freelance Open-Source Software Developer
> > http://www.open-technology.de/
>
> --
> Grant Likely, B.Sc, P.Eng.
> Secret Lab Technologies, Ltd.
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH 6/7] ARM: mackerel: support booting with or without DT
From: Grant Likely @ 2012-12-17 16:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <Pine.LNX.4.64.1212162224380.20059@axis700.grange>
On Sun, 16 Dec 2012 22:36:56 +0100 (CET), Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Hi Grant
>
> On Sun, 16 Dec 2012, Grant Likely wrote:
>
> > On Fri, 14 Dec 2012 17:45:30 +0100, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > > This patch adds dynamic switching to booting either with or without DT.
> > > So far only a part of the board initialisation can be done via DT. Devices,
> > > that still need platform data are kept that way. Devices, that can be
> > > initialised from DT will not be supplied from the platform data, if a DT
> > > image is detected.
> > >
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > ---
> > >
> > > +static int mackerel_i2c_bus_notify(struct notifier_block *nb,
> > > + unsigned long action, void *data)
> > > +{
> > > + struct device *dev = data;
> > > +
> > > + if (action != BUS_NOTIFY_ADD_DEVICE ||
> > > + strcmp(dev_name(dev->parent), "fff20000.i2c"))
> > > + return NOTIFY_DONE;
> > > +
> > > + i2c_new_device(to_i2c_adapter(dev), &i2c0_devices[1]);
> > > +
> > > + return NOTIFY_OK;
> > > +}
> > > +
> > > +static struct notifier_block mackerel_i2c_notifier = {
> > > + .notifier_call = mackerel_i2c_bus_notify,
> > > +};
> >
> > This looks dodgy. It appears that the purpose of this hook is to create
> > the tca6408-keys device because it has some platform data. However,
> > this hook will try to create the device every time BUS_NOTIFY_ADD_DEVICE
> > happens on the fff20000.i2c bus *including* when the tca6408-keys device
> > gets added.
>
> I think, this is only called once, when the i2c adapter device is added in
> i2c_register_adapter(). I2C client devices have these adapter devices as
> their parents, and those devices have "i2c-%d" as their names. Since all
> client devices get destroyed when the adapter is unbound, the above should
> be safe.
Take another look. The way the bus notifiers work is they get called
once for every device registered that has the given bus type. In this
case, that means i2c_bus_type, which also means every i2c_client object
registration. Also, the point of i2c_new_device() is that it creates a
new i2c_client object and registers it.... trigger a new call to this
notifier... which calls i2c_new_device again!
>
> > The correct approach when you need to add specific platform data is to
> > still put the device into the device tree and make the notifier look for
> > that specific device. Then the platform data pointer can be attached at
> > BUS_NOTIFY_ADD_DEVICE time.
> >
> > However, it doesn't look like you need a notifier at all. From what I
> > can see the tca6408-keys device does have platform data, but it is all
> > simple (no callback pointers). You should instead encode the platform
> > data into device tree properties and extract them from the driver.
>
> Yes, this is the ultimate goal. But the purpose of this patch series is to
> move whatever devices are possible right _now_ to DT. Ultimately all of
> them should be migrated. So, yes, we could try to begin with tca6408,
> because the above hack is certainly the ugliest one in this series, but in
> principle this is just one of several devices, that we have to keep in
> platform data for now and aim at removing these temporary hacks as soon as
> respective drivers implement sufficient DT support.
Understood. However I think you're going about it the long way around.
Instead of cherry picking some devices to put into the device tree, you
should put all of them into the DT, even if the driver doesn't have
bindings for them yet. If you really prefer the temporary hack approach
then you should do two things:
1) for platform devices, use AUXDATA to hook up platform_data pointers
when the devices are instantiated from the device tree.
2) for everything else, use 1 notifier per bus_type to hook up
platform_data on the BUS_NOTIFY_ADD_DEVICE event, but instead of the
dodgy code used above, use the dev->of_node->full_name property to
figure out which device has gotten registered.
It would be convenient to have AUXDATA for non-platform device
registrations, but nobody has hooked that up just yet, and it is really
a temporary measure until clock bindings are fully in place.
g.
^ permalink raw reply
* Re: [PATCH v2 6/7] ARM: mackerel: support booting with or without DT
From: Grant Likely @ 2012-12-17 17:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <Pine.LNX.4.64.1212171332520.24007@axis700.grange>
On Mon, 17 Dec 2012 13:40:45 +0100 (CET), Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> This patch adds dynamic switching to booting either with or without DT.
> So far only a part of the board initialisation can be done via DT. Devices,
> that still need platform data are kept that way. Devices, that can be
> initialised from DT will not be supplied from the platform data, if a DT
> image is detected.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>
> static const char *mackerel_boards_compat_dt[] __initdata = {
> @@ -1659,10 +1715,20 @@ static const char *mackerel_boards_compat_dt[] __initdata = {
> DT_MACHINE_START(MACKEREL_DT, "mackerel")
> .map_io = sh7372_map_io,
> .init_early = sh7372_add_early_devices,
> + .init_irq = sh7372_init_irq_of,
> + .handle_irq = shmobile_handle_irq_intc,
> + .init_machine = mackerel_init,
> + .init_late = sh7372_pm_init_late,
> + .timer = &shmobile_timer,
> + .dt_compat = mackerel_boards_compat_dt,
> +MACHINE_END
> +
> +MACHINE_START(MACKEREL, "mackerel")
> + .map_io = sh7372_map_io,
> + .init_early = sh7372_add_early_devices,
> .init_irq = sh7372_init_irq,
> .handle_irq = shmobile_handle_irq_intc,
> .init_machine = mackerel_init,
> .init_late = sh7372_pm_init_late,
> .timer = &shmobile_timer,
> - .dt_compat = mackerel_boards_compat_dt,
> MACHINE_END
MACHINE_START() handle's the DT case just fine. You shouldn't need
separate MACHINE_START() and DT_MACHINE_START() stanzas. Please merge. I
see that there was some discussion around sh7372_init_irq_of(), but I
it is better to have the single function handle the OF/non-OF case
gracefully rather than duplicating MACHINE definitions.
g.
^ 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