| Message ID | 1338888630-25930-2-git-send-email-s.hauer@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
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
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,
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
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,
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
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.
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
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.
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)
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(+)