Re: [PATCH 11/13] OMAP: create omap_devices for MPU, DSP, L3
Paul Walmsley writes: > Hi Kevin, > > one other thought. > > On Thu, 24 Jun 2010, Paul Walmsley wrote: > >> On Wed, 23 Jun 2010, Kevin Hilman wrote: >> >> > Create simple omap_devices for the main processors and busses. >> > >> > This is required to support the forth-coming device-based OPP >> > approach, where OPPs are managed and tracked at the device level. > > If these omap_devices are only to deal with OPPs, you might only need one > omap_device for the MPU subsystem for OMAP4, since I think both cores > receive the same clock. That might simplify this patch for the current > timeframe. Agreed. Again, I fall victim to my own last-minute changes. Up until yesterday I wasn't trying to deal with SMP, and decided to add it yesterday, clearly without a good (enough) understanding of the PER_CPU stuff for SMP. Will re-spin without the SMP and multiple-devices-in-class support. Kevin -- 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 11/13] OMAP: create omap_devices for MPU, DSP, L3
> -Original Message-
> From: Paul Walmsley [mailto:p...@pwsan.com]
> Sent: Thursday, June 24, 2010 12:50 PM
> To: Shilimkar, Santosh
> Cc: Kevin Hilman; linux-omap@vger.kernel.org
> Subject: RE: [PATCH 11/13] OMAP: create omap_devices for MPU, DSP, L3
>
> On Thu, 24 Jun 2010, Shilimkar, Santosh wrote:
>
> > > -Original Message-
> > > From: linux-omap-ow...@vger.kernel.org
> > > [mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of
> Paul
> > > Walmsley
> > > Sent: Thursday, June 24, 2010 11:57 AM
>
> > > > +struct device *omap_get_mpu_device(void)
> > > > +{
> > > > + mpu_dev = per_cpu(mpu_dev, smp_processor_id());
> > >
> > > Looks like this will trash the per-CPU mpu_dev on SMP. You'll need to
> > > rename one of these two mpu_devs.
> > >
> > > Also, won't the use of smp_processor_id() mean that this will just return
> > > the struct device * for the MPU that is running this code? So on a
> > > two-CPU system, it would be either CPU 0 or 1, randomly. I guess one
> > > solution would be to pass in the processor ID to omap_get_mpu_device().
> > >
> > MPUSS and both CPU's are clocked from same clock source, so above assumption
> > still work on OMAP4.
>
> If the point is to only return an omap_device corresponding to the MPU
> subsystem, then all the per_cpu() stuff is unnecessary and can be dropped,
> and the function should be so named (e.g., omap_get_mpuss_device()).
>
Indeed !!
Even otherwise the patch is not really initializing per-cpu device because
init(omap_hwmod_for_each_by_class("mpu", _init_omap_device, &mpu_dev);)
is called only on one CPU.
Regards,
Santosh
--
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 11/13] OMAP: create omap_devices for MPU, DSP, L3
On Thu, 24 Jun 2010, Shilimkar, Santosh wrote:
> > -Original Message-
> > From: linux-omap-ow...@vger.kernel.org
> > [mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of Paul
> > Walmsley
> > Sent: Thursday, June 24, 2010 11:57 AM
> > > +struct device *omap_get_mpu_device(void)
> > > +{
> > > + mpu_dev = per_cpu(mpu_dev, smp_processor_id());
> >
> > Looks like this will trash the per-CPU mpu_dev on SMP. You'll need to
> > rename one of these two mpu_devs.
> >
> > Also, won't the use of smp_processor_id() mean that this will just return
> > the struct device * for the MPU that is running this code? So on a
> > two-CPU system, it would be either CPU 0 or 1, randomly. I guess one
> > solution would be to pass in the processor ID to omap_get_mpu_device().
> >
> MPUSS and both CPU's are clocked from same clock source, so above assumption
> still work on OMAP4.
If the point is to only return an omap_device corresponding to the MPU
subsystem, then all the per_cpu() stuff is unnecessary and can be dropped,
and the function should be so named (e.g., omap_get_mpuss_device()).
- Paul
--
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 11/13] OMAP: create omap_devices for MPU, DSP, L3
> -Original Message-
> From: linux-omap-ow...@vger.kernel.org
> [mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of Paul
> Walmsley
> Sent: Thursday, June 24, 2010 11:57 AM
> To: Kevin Hilman
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH 11/13] OMAP: create omap_devices for MPU, DSP, L3
>
> Hi Kevin,
>
> some comments below -
>
> On Wed, 23 Jun 2010, Kevin Hilman wrote:
>
> > Create simple omap_devices for the main processors and busses.
> >
> > This is required to support the forth-coming device-based OPP
> > approach, where OPPs are managed and tracked at the device level.
> >
> > So that these primary devices are available for early PM initialization,
> > they are created as early platform_devices.
>
> The idea sounds good, but -
>
> >
> > Cc: Paul Walmsley
> > Signed-off-by: Kevin Hilman
> > ---
> > arch/arm/mach-omap2/devices.c|2 +
> > arch/arm/mach-omap2/io.c | 55
> > +-
> > arch/arm/plat-omap/include/plat/common.h |4 ++
> > 3 files changed, 60 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
> > index 03e6c9e..62920ac 100644
> > --- a/arch/arm/mach-omap2/devices.c
> > +++ b/arch/arm/mach-omap2/devices.c
> > @@ -15,6 +15,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -29,6 +30,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include "mux.h"
> >
> > diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
> > index 3cfb425..ecebbbf 100644
> > --- a/arch/arm/mach-omap2/io.c
> > +++ b/arch/arm/mach-omap2/io.c
> > @@ -45,7 +45,7 @@
> >
> > #include
> > #include "clockdomains.h"
> > -#include
> > +#include
> >
> > /*
> > * The machine specific code may provide the extra mapping besides the
> > @@ -313,6 +313,58 @@ static int __init _omap2_init_reprogram_sdrc(void)
> > return v;
> > }
> >
> > +static struct omap_device_pm_latency *pm_lats;
> > +
> > +static DEFINE_PER_CPU(struct device *, mpu_dev);
> > +static struct devicee *dsp_dev;
> > +static struct device *l3_dev;
> > +
> > +struct device *omap_get_mpu_device(void)
> > +{
> > + mpu_dev = per_cpu(mpu_dev, smp_processor_id());
>
> Looks like this will trash the per-CPU mpu_dev on SMP. You'll need to
> rename one of these two mpu_devs.
>
> Also, won't the use of smp_processor_id() mean that this will just return
> the struct device * for the MPU that is running this code? So on a
> two-CPU system, it would be either CPU 0 or 1, randomly. I guess one
> solution would be to pass in the processor ID to omap_get_mpu_device().
>
MPUSS and both CPU's are clocked from same clock source, so above assumption
still work on OMAP4.
> > + WARN_ON_ONCE(!mpu_dev);
> > + return mpu_dev;
> > +}
> > +
> > +struct device *omap_get_dsp_device(void)
> > +{
> > + WARN_ON_ONCE(!dsp_dev);
> > + return dsp_dev;
> > +}
> > +
> > +struct device *omap_get_l3_device(void)
> > +{
> > + WARN_ON_ONCE(!l3_dev);
> > + return l3_dev;
> > +}
> > +
> > +static int _init_omap_device(struct omap_hwmod *oh, void *user)
> > +{
> > + struct omap_device *od;
> > + const char *name = oh->name;
> > + struct device **new_dev = (struct device **)user;
> > +
> > + od = omap_device_build(name, 0, oh, NULL, 0, pm_lats, 0, true);
> > + if (WARN(IS_ERR(od), "Could not build omap_device for %s\n", name))
> > + return -ENODEV;
> > +
> > + *new_dev = &od->pdev.dev;
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Build omap_devices for processors and bus.
> > + */
> > +static void omap_init_processor_devices(void)
> > +{
> > + mpu_dev = per_cpu(mpu_dev, smp_processor_id());
>
> (same issues as above)
>
> > +
> > + omap_hwmod_for_each_by_class("mpu", _init_omap_device, &mpu_dev);
>
> This won't work on SMP - it will use the same mpu_dev variable for each
> MPU, so mpu_dev will contain the struct device * for the last MPU hwmod
> iterated over.
>
> Also, there is a hidden assumption that the hwmods of class "mpu" will be
> iterated over in the order that the per_cpu() variable expect
Re: [PATCH 11/13] OMAP: create omap_devices for MPU, DSP, L3
Hi Kevin, one other thought. On Thu, 24 Jun 2010, Paul Walmsley wrote: > On Wed, 23 Jun 2010, Kevin Hilman wrote: > > > Create simple omap_devices for the main processors and busses. > > > > This is required to support the forth-coming device-based OPP > > approach, where OPPs are managed and tracked at the device level. If these omap_devices are only to deal with OPPs, you might only need one omap_device for the MPU subsystem for OMAP4, since I think both cores receive the same clock. That might simplify this patch for the current timeframe. - Paul -- 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 11/13] OMAP: create omap_devices for MPU, DSP, L3
Hi Kevin,
some comments below -
On Wed, 23 Jun 2010, Kevin Hilman wrote:
> Create simple omap_devices for the main processors and busses.
>
> This is required to support the forth-coming device-based OPP
> approach, where OPPs are managed and tracked at the device level.
>
> So that these primary devices are available for early PM initialization,
> they are created as early platform_devices.
The idea sounds good, but -
>
> Cc: Paul Walmsley
> Signed-off-by: Kevin Hilman
> ---
> arch/arm/mach-omap2/devices.c|2 +
> arch/arm/mach-omap2/io.c | 55
> +-
> arch/arm/plat-omap/include/plat/common.h |4 ++
> 3 files changed, 60 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
> index 03e6c9e..62920ac 100644
> --- a/arch/arm/mach-omap2/devices.c
> +++ b/arch/arm/mach-omap2/devices.c
> @@ -15,6 +15,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -29,6 +30,7 @@
> #include
> #include
> #include
> +#include
>
> #include "mux.h"
>
> diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
> index 3cfb425..ecebbbf 100644
> --- a/arch/arm/mach-omap2/io.c
> +++ b/arch/arm/mach-omap2/io.c
> @@ -45,7 +45,7 @@
>
> #include
> #include "clockdomains.h"
> -#include
> +#include
>
> /*
> * The machine specific code may provide the extra mapping besides the
> @@ -313,6 +313,58 @@ static int __init _omap2_init_reprogram_sdrc(void)
> return v;
> }
>
> +static struct omap_device_pm_latency *pm_lats;
> +
> +static DEFINE_PER_CPU(struct device *, mpu_dev);
> +static struct devicee *dsp_dev;
> +static struct device *l3_dev;
> +
> +struct device *omap_get_mpu_device(void)
> +{
> + mpu_dev = per_cpu(mpu_dev, smp_processor_id());
Looks like this will trash the per-CPU mpu_dev on SMP. You'll need to
rename one of these two mpu_devs.
Also, won't the use of smp_processor_id() mean that this will just return
the struct device * for the MPU that is running this code? So on a
two-CPU system, it would be either CPU 0 or 1, randomly. I guess one
solution would be to pass in the processor ID to omap_get_mpu_device().
> + WARN_ON_ONCE(!mpu_dev);
> + return mpu_dev;
> +}
> +
> +struct device *omap_get_dsp_device(void)
> +{
> + WARN_ON_ONCE(!dsp_dev);
> + return dsp_dev;
> +}
> +
> +struct device *omap_get_l3_device(void)
> +{
> + WARN_ON_ONCE(!l3_dev);
> + return l3_dev;
> +}
> +
> +static int _init_omap_device(struct omap_hwmod *oh, void *user)
> +{
> + struct omap_device *od;
> + const char *name = oh->name;
> + struct device **new_dev = (struct device **)user;
> +
> + od = omap_device_build(name, 0, oh, NULL, 0, pm_lats, 0, true);
> + if (WARN(IS_ERR(od), "Could not build omap_device for %s\n", name))
> + return -ENODEV;
> +
> + *new_dev = &od->pdev.dev;
> +
> + return 0;
> +}
> +
> +/*
> + * Build omap_devices for processors and bus.
> + */
> +static void omap_init_processor_devices(void)
> +{
> + mpu_dev = per_cpu(mpu_dev, smp_processor_id());
(same issues as above)
> +
> + omap_hwmod_for_each_by_class("mpu", _init_omap_device, &mpu_dev);
This won't work on SMP - it will use the same mpu_dev variable for each
MPU, so mpu_dev will contain the struct device * for the last MPU hwmod
iterated over.
Also, there is a hidden assumption that the hwmods of class "mpu" will be
iterated over in the order that the per_cpu() variable expects...
> + omap_hwmod_for_each_by_class("dsp", _init_omap_device, &dsp_dev);
> + omap_hwmod_for_each_by_class("l3", _init_omap_device, &l3_dev);
If there is more than one hwmod in these classes, these will lose the
omap_device pointers for all but the final omap_hwmod iterated over.
It's probably easier to just call omap_hwmod_lookup()/_init_omap_device()
directly and forget about omap_hwmod_for_each_by_class() for the above
two cases - maybe all three...
> +}
> +
> void __init omap2_init_common_hw(struct omap_sdrc_params *sdrc_cs0,
>struct omap_sdrc_params *sdrc_cs1)
> {
> @@ -342,6 +394,7 @@ void __init omap2_init_common_hw(struct omap_sdrc_params
> *sdrc_cs0,
> omap_serial_early_init();
> if (cpu_is_omap24xx() || cpu_is_omap34xx()) /* FIXME: OMAP4 */
> omap_hwmod_late_init();
> + omap_init_processor_devices();
> omap_pm_if_init();
> if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
> omap2_sdrc_init(sdrc_cs0, sdrc_cs1);
> diff --git a/arch/arm/plat-omap/include/plat/common.h
> b/arch/arm/plat-omap/include/plat/common.h
> index d265018..0059dec 100644
> --- a/arch/arm/plat-omap/include/plat/common.h
> +++ b/arch/arm/plat-omap/include/plat/common.h
> @@ -87,4 +87,8 @@ void omap2_set_globals_uart(struct omap_globals *);
> } \
> })
>
> +struct device
[PATCH 11/13] OMAP: create omap_devices for MPU, DSP, L3
Create simple omap_devices for the main processors and busses.
This is required to support the forth-coming device-based OPP
approach, where OPPs are managed and tracked at the device level.
So that these primary devices are available for early PM initialization,
they are created as early platform_devices.
Cc: Paul Walmsley
Signed-off-by: Kevin Hilman
---
arch/arm/mach-omap2/devices.c|2 +
arch/arm/mach-omap2/io.c | 55 +-
arch/arm/plat-omap/include/plat/common.h |4 ++
3 files changed, 60 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
index 03e6c9e..62920ac 100644
--- a/arch/arm/mach-omap2/devices.c
+++ b/arch/arm/mach-omap2/devices.c
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
@@ -29,6 +30,7 @@
#include
#include
#include
+#include
#include "mux.h"
diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
index 3cfb425..ecebbbf 100644
--- a/arch/arm/mach-omap2/io.c
+++ b/arch/arm/mach-omap2/io.c
@@ -45,7 +45,7 @@
#include
#include "clockdomains.h"
-#include
+#include
/*
* The machine specific code may provide the extra mapping besides the
@@ -313,6 +313,58 @@ static int __init _omap2_init_reprogram_sdrc(void)
return v;
}
+static struct omap_device_pm_latency *pm_lats;
+
+static DEFINE_PER_CPU(struct device *, mpu_dev);
+static struct device *dsp_dev;
+static struct device *l3_dev;
+
+struct device *omap_get_mpu_device(void)
+{
+ mpu_dev = per_cpu(mpu_dev, smp_processor_id());
+ WARN_ON_ONCE(!mpu_dev);
+ return mpu_dev;
+}
+
+struct device *omap_get_dsp_device(void)
+{
+ WARN_ON_ONCE(!dsp_dev);
+ return dsp_dev;
+}
+
+struct device *omap_get_l3_device(void)
+{
+ WARN_ON_ONCE(!l3_dev);
+ return l3_dev;
+}
+
+static int _init_omap_device(struct omap_hwmod *oh, void *user)
+{
+ struct omap_device *od;
+ const char *name = oh->name;
+ struct device **new_dev = (struct device **)user;
+
+ od = omap_device_build(name, 0, oh, NULL, 0, pm_lats, 0, true);
+ if (WARN(IS_ERR(od), "Could not build omap_device for %s\n", name))
+ return -ENODEV;
+
+ *new_dev = &od->pdev.dev;
+
+ return 0;
+}
+
+/*
+ * Build omap_devices for processors and bus.
+ */
+static void omap_init_processor_devices(void)
+{
+ mpu_dev = per_cpu(mpu_dev, smp_processor_id());
+
+ omap_hwmod_for_each_by_class("mpu", _init_omap_device, &mpu_dev);
+ omap_hwmod_for_each_by_class("dsp", _init_omap_device, &dsp_dev);
+ omap_hwmod_for_each_by_class("l3", _init_omap_device, &l3_dev);
+}
+
void __init omap2_init_common_hw(struct omap_sdrc_params *sdrc_cs0,
struct omap_sdrc_params *sdrc_cs1)
{
@@ -342,6 +394,7 @@ void __init omap2_init_common_hw(struct omap_sdrc_params
*sdrc_cs0,
omap_serial_early_init();
if (cpu_is_omap24xx() || cpu_is_omap34xx()) /* FIXME: OMAP4 */
omap_hwmod_late_init();
+ omap_init_processor_devices();
omap_pm_if_init();
if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
omap2_sdrc_init(sdrc_cs0, sdrc_cs1);
diff --git a/arch/arm/plat-omap/include/plat/common.h
b/arch/arm/plat-omap/include/plat/common.h
index d265018..0059dec 100644
--- a/arch/arm/plat-omap/include/plat/common.h
+++ b/arch/arm/plat-omap/include/plat/common.h
@@ -87,4 +87,8 @@ void omap2_set_globals_uart(struct omap_globals *);
} \
})
+struct device *omap_get_mpu_device(void);
+struct device *omap_get_dsp_device(void);
+struct device *omap_get_l3_device(void);
+
#endif /* __ARCH_ARM_MACH_OMAP_COMMON_H */
--
1.7.0.2
--
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
