Re: [PATCH v2 8/9] ARM: OMAP: iommu: add device tree support

2012-10-03 Thread Omar Ramirez Luna 
Hi Matt,

On 2 October 2012 16:25, Matt Porter mpor...@ti.com wrote:
...
 I can see why you went this path with the iommu driver as it already had
 some integration code present here. I have some concerns going forward
 about how this should be long-term. Take any platform booting only with
 DT populated, we'd like to avoid having to use this approach where
 platform private APIs are called via pdata. In fact, it's going to makes
 thing ugly to carry any sort of pdata for a driver that's otherwise
 driven exclusively from DT.

 For AM335x, I can implement this approach, but it means adding some
 pruss specific integration code just to have it create the pdata for
 reset_name and assert/deassert.

Yes I agree, it looks a bit ugly for devices that have more than one
reset line name too, but right now there is no other way to keep the
reset names and also provide flexibility to the driver to control them
in a given order.

 From reading all the threads on hardresets and OMAP, it seems we may not
 be able to come up with a generic OMAP handler for these resets and
 that's really reflected in the fact that this API exists. So given that,
 it reasons that OMAP isn't the only one needing a reset API for drivers.
 I'm thinking that (as trivial as it may seem), this support may need to
 move to a reset subsystem such that drivers have a clean way to access
 reset resources in an SoC.

Well, there was a point where the OMAP hwmod code contained the reset
code and at least for me it was working fine, with iommu and ipu
processor, just with a minor misleading warning print... however then
this code got stripped out and since there appeared to be people
needing to handle their reset lines in unknown ways it was advised
that everybody should implement their own reset functions, but in my
case I could reuse most of the disabled code at the expense of almost
duplicating _enable (omap_hwmod) function while waiting all the reset
line users started asking for changes.

 I'm curious if you or others have thought about where this needs to go
 next.

I haven't planned for a reset subsystem or a more generic
implementation, although I have been looking for a way to avoid using
the pdata function pointers.

 When I first thought about a reset subsystem it seemed to trivial
 to bother with but looking at the reasoning behind the power_seq
 subsystem, it seems to have similar justification to get this machine
 specific logic out of the platform code and under standardized control
 of the driver.

IMHO, the reset code even if made a subsystem or a library, would
still need to interface with machine specific code and definitions
(say omap_hwmod), so I don't see much difference in making the
omap_device reset handlers as exported symbols for the time being,
with acceptance of the owners of omap_device code.

And then if power_seq makes it to mainline perhaps extending it to
handle deassert, assert and softreset events, but then again this
would have the same hooks to omap_hwmod which is the one with the prcm
information.

Regards,

Omar
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 8/9] ARM: OMAP: iommu: add device tree support

2012-10-02 Thread Matt Porter 
On Wed, Sep 12, 2012 at 02:45:51PM -0500, Omar Ramirez Luna wrote:
 Adapt driver to use DT if provided.

Hi Omar,

I'm interested in making use of the assert/deassert APIs you exposed in
this series on AM335x for the pruss hwmod which has one hardreset
line. I have the same situation where I need to get it deasserted before
I do any runtime PM. See my comments below...

 +static int __devinit
 +iommu_add_platform_data_from_dt(struct platform_device *pdev)
 +{
 + struct iommu_platform_data *pdata;
 + struct device_node *node = pdev-dev.of_node;
 + struct omap_hwmod *oh;
 + struct omap_mmu_dev_attr *a;
 + int ret = 0;
 +
 + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
 + if (!pdata)
 + return -ENOMEM;
 +
 + of_property_read_string(node, ti,hwmods, pdata-name);
 + oh = omap_hwmod_lookup(pdata-name);
 + if (!oh) {
 + dev_err(pdev-dev, Cannot lookup hwmod '%s'\n, pdata-name);
 + ret = -ENODEV;
 + goto out;
 + }
 +
 + a = (struct omap_mmu_dev_attr *)oh-dev_attr;
 + pdata-nr_tlb_entries = a-nr_tlb_entries;
 + pdata-da_start = a-da_start;
 + pdata-da_end = a-da_end;
 +
 + if (oh-rst_lines_cnt == 1) {
 + pdata-reset_name = oh-rst_lines-name;
 + pdata-assert_reset = omap_device_assert_hardreset;
 + pdata-deassert_reset = omap_device_deassert_hardreset;
 + }
 +
 + ret = platform_device_add_data(pdev, pdata, sizeof(*pdata));
 + if (ret)
 + dev_err(pdev-dev, Cannot add pdata for %s\n, pdata-name);
 +
 +out:
 + kfree(pdata);
 +
 + return ret;
 +}

I can see why you went this path with the iommu driver as it already had
some integration code present here. I have some concerns going forward
about how this should be long-term. Take any platform booting only with
DT populated, we'd like to avoid having to use this approach where
platform private APIs are called via pdata. In fact, it's going to makes
thing ugly to carry any sort of pdata for a driver that's otherwise
driven exclusively from DT.

For AM335x, I can implement this approach, but it means adding some
pruss specific integration code just to have it create the pdata for
reset_name and assert/deassert.

From reading all the threads on hardresets and OMAP, it seems we may not
be able to come up with a generic OMAP handler for these resets and
that's really reflected in the fact that this API exists. So given that,
it reasons that OMAP isn't the only one needing a reset API for drivers.
I'm thinking that (as trivial as it may seem), this support may need to
move to a reset subsystem such that drivers have a clean way to access
reset resources in an SoC.

I'm curious if you or others have thought about where this needs to go
next. When I first thought about a reset subsystem it seemed to trivial
to bother with but looking at the reasoning behind the power_seq
subsystem, it seems to have similar justification to get this machine
specific logic out of the platform code and under standardized control
of the driver. We have resources that are manipulated outside of the IP
block but need to be controlled at the driver level and probably should
have a common driver API that isn't OMAP specific and tied to pdata.

-Matt
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 8/9] ARM: OMAP: iommu: add device tree support

2012-09-12 Thread Omar Ramirez Luna 
Adapt driver to use DT if provided.

Signed-off-by: Omar Ramirez Luna omar.l...@linaro.org
---
 .../devicetree/bindings/arm/omap/iommu.txt |   10 +++
 arch/arm/mach-omap2/omap-iommu.c   |4 ++
 drivers/iommu/omap-iommu.c |   65 +++-
 3 files changed, 78 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/arm/omap/iommu.txt

diff --git a/Documentation/devicetree/bindings/arm/omap/iommu.txt 
b/Documentation/devicetree/bindings/arm/omap/iommu.txt
new file mode 100644
index 000..2bb780f
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/omap/iommu.txt
@@ -0,0 +1,10 @@
+* MMU (Memory Management Unit)
+
+MMU present in OMAP subsystems.
+
+Required properties:
+ compatible : should be ti,omap3-iommu for OMAP3 MMUs
+ compatible : should be ti,omap4-iommu for OMAP4 MMUs
+
+Optional properties:
+ None
diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
index e8f88dc..4b7912b 100644
--- a/arch/arm/mach-omap2/omap-iommu.c
+++ b/arch/arm/mach-omap2/omap-iommu.c
@@ -11,6 +11,7 @@
  */
 
 #include linux/module.h
+#include linux/of.h
 #include linux/platform_device.h
 #include linux/err.h
 #include linux/slab.h
@@ -27,6 +28,9 @@ static int __init omap_iommu_dev_init(struct omap_hwmod *oh, 
void *unused)
struct omap_mmu_dev_attr *a = (struct omap_mmu_dev_attr *)oh-dev_attr;
static int i;
 
+   if (of_have_populated_dt())
+   return 0;
+
pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
if (!pdata)
return -ENOMEM;
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 3f8eb04..fbfec44 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -21,12 +21,15 @@
 #include linux/mutex.h
 #include linux/spinlock.h
 #include linux/pm_runtime.h
+#include linux/of.h
 
 #include asm/cacheflush.h
 
 #include plat/iommu.h
 
 #include plat/iopgtable.h
+#include plat/omap_device.h
+#include plat/omap_hwmod.h
 
 #define for_each_iotlb_cr(obj, n, __i, cr) \
for (__i = 0;   \
@@ -909,13 +912,63 @@ static void omap_iommu_detach(struct omap_iommu *obj)
 /*
  * OMAP Device MMU(IOMMU) detection
  */
+static int __devinit
+iommu_add_platform_data_from_dt(struct platform_device *pdev)
+{
+   struct iommu_platform_data *pdata;
+   struct device_node *node = pdev-dev.of_node;
+   struct omap_hwmod *oh;
+   struct omap_mmu_dev_attr *a;
+   int ret = 0;
+
+   pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+   if (!pdata)
+   return -ENOMEM;
+
+   of_property_read_string(node, ti,hwmods, pdata-name);
+   oh = omap_hwmod_lookup(pdata-name);
+   if (!oh) {
+   dev_err(pdev-dev, Cannot lookup hwmod '%s'\n, pdata-name);
+   ret = -ENODEV;
+   goto out;
+   }
+
+   a = (struct omap_mmu_dev_attr *)oh-dev_attr;
+   pdata-nr_tlb_entries = a-nr_tlb_entries;
+   pdata-da_start = a-da_start;
+   pdata-da_end = a-da_end;
+
+   if (oh-rst_lines_cnt == 1) {
+   pdata-reset_name = oh-rst_lines-name;
+   pdata-assert_reset = omap_device_assert_hardreset;
+   pdata-deassert_reset = omap_device_deassert_hardreset;
+   }
+
+   ret = platform_device_add_data(pdev, pdata, sizeof(*pdata));
+   if (ret)
+   dev_err(pdev-dev, Cannot add pdata for %s\n, pdata-name);
+
+out:
+   kfree(pdata);
+
+   return ret;
+}
+
 static int __devinit omap_iommu_probe(struct platform_device *pdev)
 {
int err = -ENODEV;
int irq;
struct omap_iommu *obj;
struct resource *res;
-   struct iommu_platform_data *pdata = pdev-dev.platform_data;
+   struct iommu_platform_data *pdata;
+
+   if (of_have_populated_dt()) {
+   err = iommu_add_platform_data_from_dt(pdev);
+   if (err)
+   return err;
+   }
+
+   pdata = pdev-dev.platform_data;
 
obj = kzalloc(sizeof(*obj), GFP_KERNEL);
if (!obj)
@@ -1024,12 +1077,22 @@ static const struct dev_pm_ops iommu_pm_ops = {
   NULL)
 };
 
+#if defined(CONFIG_OF)
+static const struct of_device_id omap_iommu_of_match[] = {
+   { .compatible = ti,omap3-iommu },
+   { .compatible = ti,omap4-iommu },
+   { },
+};
+MODULE_DEVICE_TABLE(of, omap_iommu_of_match);
+#endif
+
 static struct platform_driver omap_iommu_driver = {
.probe  = omap_iommu_probe,
.remove = __devexit_p(omap_iommu_remove),
.driver = {
.name   = omap-iommu,
.pm = iommu_pm_ops,
+   .of_match_table = of_match_ptr(omap_iommu_of_match),
},
 };
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More