Patchwork [1/2] ARM i.MX51: setup mipi

login
register
mail settings
Submitter Sascha Hauer
Date June 5, 2012, 9:30 a.m.
Message ID <1338888630-25930-2-git-send-email-s.hauer@pengutronix.de>
Download mbox | patch
Permalink /patch/163055/
State New
Headers show

Comments

Sascha Hauer - June 5, 2012, 9:30 a.m.
The mipi unit has to be brought to a sane default state. This
unit is not present on i.MX53, so we add the setup here instead
of the ipu driver.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/mach-imx/mm-imx5.c |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
Shawn Guo - June 7, 2012, 2:59 a.m.
Hi Sascha,

On Tue, Jun 05, 2012 at 11:30:29AM +0200, Sascha Hauer wrote:
> The mipi unit has to be brought to a sane default state. This
> unit is not present on i.MX53, so we add the setup here instead
> of the ipu driver.
> 
Can't we do some detection in IPU driver to have the setup done in
IPU driver?  To me, this (as well as the reset in patch #2) is not
really the material for soc specific initialization.  If I'm running
a system without IPU driver enabled, why do I have to go through these
setup at all?

Regards,
Shawn

> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  arch/arm/mach-imx/mm-imx5.c |   21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c
> index feeee17..a4cb441 100644
> --- a/arch/arm/mach-imx/mm-imx5.c
> +++ b/arch/arm/mach-imx/mm-imx5.c
> @@ -194,6 +194,26 @@ void __init imx50_soc_init(void)
>  					ARRAY_SIZE(imx50_audmux_res));
>  }
>  
> +/*
> + * The MIPI HSC unit has been removed from the i.MX51 Reference Manual by
> + * the Freescale marketing division. However this did not remove the
> + * hardware from the chip which still needs to be configured for proper
> + * IPU support.
> + */
> +static void __init imx51_ipu_mipi_setup(void)
> +{
> +	void __iomem *hsc_addr;
> +
> +	hsc_addr = MX51_IO_ADDRESS(MX51_MIPI_HSC_BASE_ADDR);
> +
> +	/* setup MIPI module to legacy mode */
> +	__raw_writel(0xf00, hsc_addr);
> +
> +	/* CSI mode: reserved; DI control mode: legacy (from Freescale BSP) */
> +	__raw_writel(__raw_readl(hsc_addr + 0x800) | 0x30ff,
> +		hsc_addr + 0x800);
> +}
> +
>  void __init imx51_soc_init(void)
>  {
>  	/* i.mx51 has the i.mx31 type gpio */
> @@ -212,6 +232,7 @@ void __init imx51_soc_init(void)
>  	/* i.mx51 has the i.mx31 type audmux */
>  	platform_device_register_simple("imx31-audmux", 0, imx51_audmux_res,
>  					ARRAY_SIZE(imx51_audmux_res));
> +	imx51_ipu_mipi_setup();
>  }
>  
>  void __init imx53_soc_init(void)
> -- 
> 1.7.10
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
David Jander - June 7, 2012, 6:13 a.m.
On Thu, 7 Jun 2012 10:59:51 +0800
Shawn Guo <shawn.guo@linaro.org> wrote:

> Hi Sascha,
> 
> On Tue, Jun 05, 2012 at 11:30:29AM +0200, Sascha Hauer wrote:
> > The mipi unit has to be brought to a sane default state. This
> > unit is not present on i.MX53, so we add the setup here instead
> > of the ipu driver.
> > 
> Can't we do some detection in IPU driver to have the setup done in
> IPU driver?  To me, this (as well as the reset in patch #2) is not
> really the material for soc specific initialization.  If I'm running
> a system without IPU driver enabled, why do I have to go through these
> setup at all?

I would consider it part of SoC setup for the simple reason, that this
peripheral officially does not even exist (although it is physically there in
the chip and unfortunately _needs_ to be initialized). There is no
documentation of it anywhere (besides old, obsolete and unreleased manuals).
If one wanted to implement an IPU driver later on, there would be no way of
knowing that this part must be done first. We should take the chance that
someone happens to have this "knowledge" and do this (further harmless)
initialization in SoC setup, so that it is dealt with and won't get in the way
later. There is no worse driver developer nightmare than incomplete
documentation causing your otherwise correct driver to just not work.
OTOH, if this was an officially supported peripheral, it should have it's own
driver and not be part of the IPU driver either.

Best regards,
Shawn Guo - June 7, 2012, 7:10 a.m.
On 7 June 2012 14:13, David Jander <david.jander@protonic.nl> wrote:
> I would consider it part of SoC setup for the simple reason, that this
> peripheral officially does not even exist (although it is physically there in
> the chip and unfortunately _needs_ to be initialized). There is no
> documentation of it anywhere (besides old, obsolete and unreleased manuals).
> If one wanted to implement an IPU driver later on, there would be no way of
> knowing that this part must be done first. We should take the chance that
> someone happens to have this "knowledge" and do this (further harmless)
> initialization in SoC setup, so that it is dealt with and won't get in the way
> later. There is no worse driver developer nightmare than incomplete
> documentation causing your otherwise correct driver to just not work.
> OTOH, if this was an officially supported peripheral, it should have it's own
> driver and not be part of the IPU driver either.
>
That does not necessarily mean imx51_soc_init() is the right place for
that initialization, simply because not every single boot of imx51
needs that initialization.

I can probably accept the mipi initialization changes if you are saying
imx51_soc_init is not the right place for it either, but at the moment
there is no better place than imx51_soc_init().

But why can't we do ipu reset (the second patch) in ipu driver then?

Regards,
Shawn
David Jander - June 7, 2012, 7:55 a.m.
On Thu, 7 Jun 2012 15:10:05 +0800
Shawn Guo <shawn.guo@linaro.org> wrote:

> On 7 June 2012 14:13, David Jander <david.jander@protonic.nl> wrote:
> > I would consider it part of SoC setup for the simple reason, that this
> > peripheral officially does not even exist (although it is physically there in
> > the chip and unfortunately _needs_ to be initialized). There is no
> > documentation of it anywhere (besides old, obsolete and unreleased manuals).
> > If one wanted to implement an IPU driver later on, there would be no way of
> > knowing that this part must be done first. We should take the chance that
> > someone happens to have this "knowledge" and do this (further harmless)
> > initialization in SoC setup, so that it is dealt with and won't get in the way
> > later. There is no worse driver developer nightmare than incomplete
> > documentation causing your otherwise correct driver to just not work.
> > OTOH, if this was an officially supported peripheral, it should have it's own
> > driver and not be part of the IPU driver either.
> >
> That does not necessarily mean imx51_soc_init() is the right place for
> that initialization, simply because not every single boot of imx51
> needs that initialization.

I agree, but I couldn't come up with a better place.

> I can probably accept the mipi initialization changes if you are saying
> imx51_soc_init is not the right place for it either, but at the moment
> there is no better place than imx51_soc_init().

Sounds good. Yes, can we agree on that?

> But why can't we do ipu reset (the second patch) in ipu driver then?

No idea... I don't want to preempt Sascha here.
Just my guess: The system reset controller could potentially be used by other
peripherals, and in that case it would need to be a separate driver with
locking for register access. Doing it here seems a safe way of avoiding
concurrent access to the register of the SRC?
Same reason as above?

Best regards,
Sascha Hauer - June 7, 2012, 8:43 a.m.
On Thu, Jun 07, 2012 at 03:10:05PM +0800, Shawn Guo wrote:
> On 7 June 2012 14:13, David Jander <david.jander@protonic.nl> wrote:
> > I would consider it part of SoC setup for the simple reason, that this
> > peripheral officially does not even exist (although it is physically there in
> > the chip and unfortunately _needs_ to be initialized). There is no
> > documentation of it anywhere (besides old, obsolete and unreleased manuals).
> > If one wanted to implement an IPU driver later on, there would be no way of
> > knowing that this part must be done first. We should take the chance that
> > someone happens to have this "knowledge" and do this (further harmless)
> > initialization in SoC setup, so that it is dealt with and won't get in the way
> > later. There is no worse driver developer nightmare than incomplete
> > documentation causing your otherwise correct driver to just not work.
> > OTOH, if this was an officially supported peripheral, it should have it's own
> > driver and not be part of the IPU driver either.
> >
> That does not necessarily mean imx51_soc_init() is the right place for
> that initialization, simply because not every single boot of imx51
> needs that initialization.

Since it doesn't really add to the boot time I think it won't be a
problem.

> 
> I can probably accept the mipi initialization changes if you are saying
> imx51_soc_init is not the right place for it either, but at the moment
> there is no better place than imx51_soc_init().
> 
> But why can't we do ipu reset (the second patch) in ipu driver then?

Ok, we might have to come up with something better. I originally had the
IPU reset in the IPU driver (copied from the FSL BSP). There are two
problems with this:

- The IPU reset is external to the IPU which means that we have to
  distinguish between SoCs.
- The IPU reset takes quite a long time (about 300ms). Doing this in
  the IPU driver means that the kernel boot delays for this time. Doing
  this in early init means that the 300ms can be spent bringing the rest
  of the system up.

As said, there are more clean ways to do this without having the two
mentioned problems, I'm just not sure whether it's worth it to put
significant amount of work into this.

Sascha
Shawn Guo - June 7, 2012, 1:37 p.m.
On Thu, Jun 07, 2012 at 10:43:36AM +0200, Sascha Hauer wrote:
> On Thu, Jun 07, 2012 at 03:10:05PM +0800, Shawn Guo wrote:
> > But why can't we do ipu reset (the second patch) in ipu driver then?
> 
> Ok, we might have to come up with something better. I originally had the
> IPU reset in the IPU driver (copied from the FSL BSP). There are two
> problems with this:
> 
> - The IPU reset is external to the IPU which means that we have to
>   distinguish between SoCs.
> - The IPU reset takes quite a long time (about 300ms). Doing this in
>   the IPU driver means that the kernel boot delays for this time. Doing
>   this in early init means that the 300ms can be spent bringing the rest
>   of the system up.
> 
> As said, there are more clean ways to do this without having the two
> mentioned problems, I'm just not sure whether it's worth it to put
> significant amount of work into this.
> 
IMO, it's worth the effort.  Otherwise, we will end up with having
such reset functions for IPU, VPU and GPU (maybe more on later SoC)
over individual SoC initialization functions.  Well, VPU and GPU may
be less concerned by the mainline tree, but from FSL perspective, we
have to care about them.

Also, how will you support that for imx6q, considering we already have
src.c created as a little "driver" for imx6q System Reset Controller
(SRC)?  I feel like that we should extend this little "driver" to
support what you need for IPU reset here.
Sascha Hauer - June 9, 2012, 11:32 a.m.
On Thu, Jun 07, 2012 at 09:37:35PM +0800, Shawn Guo wrote:
> On Thu, Jun 07, 2012 at 10:43:36AM +0200, Sascha Hauer wrote:
> > On Thu, Jun 07, 2012 at 03:10:05PM +0800, Shawn Guo wrote:
> > > But why can't we do ipu reset (the second patch) in ipu driver then?
> > 
> > Ok, we might have to come up with something better. I originally had the
> > IPU reset in the IPU driver (copied from the FSL BSP). There are two
> > problems with this:
> > 
> > - The IPU reset is external to the IPU which means that we have to
> >   distinguish between SoCs.
> > - The IPU reset takes quite a long time (about 300ms). Doing this in
> >   the IPU driver means that the kernel boot delays for this time. Doing
> >   this in early init means that the 300ms can be spent bringing the rest
> >   of the system up.
> > 
> > As said, there are more clean ways to do this without having the two
> > mentioned problems, I'm just not sure whether it's worth it to put
> > significant amount of work into this.
> > 
> IMO, it's worth the effort.  Otherwise, we will end up with having
> such reset functions for IPU, VPU and GPU (maybe more on later SoC)
> over individual SoC initialization functions.  Well, VPU and GPU may
> be less concerned by the mainline tree, but from FSL perspective, we
> have to care about them.
> 
> Also, how will you support that for imx6q, considering we already have
> src.c created as a little "driver" for imx6q System Reset Controller
> (SRC)?  I feel like that we should extend this little "driver" to
> support what you need for IPU reset here.

Do you have an idea what kind of API such a reset unit should export?
Are you thinking about some kind of reset_me(struct device *dev) or
what else?

Sascha
Shawn Guo - June 11, 2012, 9:41 a.m.
On Sat, Jun 09, 2012 at 01:32:20PM +0200, Sascha Hauer wrote:
> Do you have an idea what kind of API such a reset unit should export?
> Are you thinking about some kind of reset_me(struct device *dev) or
> what else?
> 
Yeah, that works for me.  We can have sub-nodes under SRC node in device
tree representing reset register and bit for those peripherals, and have
a phandle like "fsl,reset-handler" under peripheral nodes pointing to
the sub-nodes.

Patch

diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c
index feeee17..a4cb441 100644
--- a/arch/arm/mach-imx/mm-imx5.c
+++ b/arch/arm/mach-imx/mm-imx5.c
@@ -194,6 +194,26 @@  void __init imx50_soc_init(void)
 					ARRAY_SIZE(imx50_audmux_res));
 }
 
+/*
+ * The MIPI HSC unit has been removed from the i.MX51 Reference Manual by
+ * the Freescale marketing division. However this did not remove the
+ * hardware from the chip which still needs to be configured for proper
+ * IPU support.
+ */
+static void __init imx51_ipu_mipi_setup(void)
+{
+	void __iomem *hsc_addr;
+
+	hsc_addr = MX51_IO_ADDRESS(MX51_MIPI_HSC_BASE_ADDR);
+
+	/* setup MIPI module to legacy mode */
+	__raw_writel(0xf00, hsc_addr);
+
+	/* CSI mode: reserved; DI control mode: legacy (from Freescale BSP) */
+	__raw_writel(__raw_readl(hsc_addr + 0x800) | 0x30ff,
+		hsc_addr + 0x800);
+}
+
 void __init imx51_soc_init(void)
 {
 	/* i.mx51 has the i.mx31 type gpio */
@@ -212,6 +232,7 @@  void __init imx51_soc_init(void)
 	/* i.mx51 has the i.mx31 type audmux */
 	platform_device_register_simple("imx31-audmux", 0, imx51_audmux_res,
 					ARRAY_SIZE(imx51_audmux_res));
+	imx51_ipu_mipi_setup();
 }
 
 void __init imx53_soc_init(void)