[PATCH] [media] staging: allow omap4iss to be modular

[Tony Lindgren]

* Laurent Pinchart <laurent.pinchart at ideasonboard.com> [140613 03:30]:
> Hi Tony,
> 
> On Friday 13 June 2014 00:53:25 Tony Lindgren wrote:
> > * Laurent Pinchart <laurent.pinchart at ideasonboard.com> [140612 23:48]:
> > > On Thursday 12 June 2014 22:30:44 Tony Lindgren wrote:
> > > > 1. They live in separate hardware modules that can be clocked separately
> > > 
> > > Actually I don't think that's true. The CSI2 PHY is part of the camera
> > > device, with all its registers but the one above in the camera device
> > > register space. For some weird reason a couple of bits were pushed to the
> > > control module, but that doesn't make the CSI2 PHY itself a separate
> > > device.
> > 
> > Yes they are separate. Anything in the system control module is
> > a separate hardware module from the other devices. So in this case
> > the CSI2 PHY is part of the system control module, not the camera
> > module.
> 
> Section 8.2.3 ("ISS CSI2 PHY") of the OMAP4460 TRM (revision AA) documents the 
> CSI2 PHY is being part of the ISS, with three PHY registers in the ISS 
> register space (not counting the PHY interrupt and status bits in several 
> other ISS registers) and one register in the system control module register 
> space. It's far from clear which power domain(s) is (are) involved.

OK I see. The register in the system control module just contains some
pin and clock related resources for the phy.
 
> > > > 2. Doing a read-back to flush a posted write in one hardware module most
> > > >    likely won't flush the write to other and that can lead into hard to
> > > >    find mysterious bugs
> > > 
> > > The OMAP4 ISS driver can just read back the CAMERA_RX register, can't it ?
> > 
> > Right, but you would have to do readbacks both from the phy register and
> > camera register to ensure writes get written. It's best to keep the
> > logic completely separate especially considering that they can be
> > clocked separately.
> > 
> > > > 3. If we ever have a common system control module driver, we need to
> > > >    rewrite all the system control module register tinkering in the
> > > >    drivers
> > > 
> > > Sure, but that's already the case today, as the OMAP4 ISS driver already
> > > accesses the control module register directly. I won't make that worse :-)
> > 
> > Well it's in staging for a reason :)
> > 
> > > > So it's best to try to use an existing framework for it. That avoids
> > > > tons of pain later on ;)
> > > 
> > > I agree, but I don't think the PHY framework would be the right
> > > abstraction. As explained above the CSI2 PHY is part of the OMAP4 ISS, so
> > > modeling its single control module register as a PHY would be a hack.
> > 
> > Well that register belongs to the system control module, not the
> > camera module. It's not like the camera IO space is out of registers
> > or something! :)
> 
> The PHY has 3 registers in the ISS I/O space and one register in the control 
> module I/O space. I have no idea why they've split it that way. The clock 
> enable bits are especially "interested", the source clock (CAM_PHY_CTRL_FCLK) 
> comes from the ISS as documented in section 8.1.1 ("ISS Integration"), is 
> gated by the control module (the gated clock is called CTRLCLK) and then goes 
> back to the ISS CSI2 PHY (it's mentioned in the CSI2 PHY "REGISTER1" 
> documentation).
> 
> > We're already handling similar control module phy cases, see for
> > example drivers/phy/phy-omap-control.c. Maybe you have most of the
> > code already there?
> 
> I'm afraid not. For PHYs that are in the system control module that solution 
> is perfectly fine, but the CSI2 PHY isn't (or at least not all of it).
> 
> I would be fine with writing a separate PHY driver if the PHY was completely 
> separate. As the documentation doesn't make it clear which part of the 
> hardware belongs to which module, matching the software implementation with an 
> unknown hardware implementation would be pretty difficult :-)

Yeah it seems the phy driver would still have to use the pin resources
in the system control module.
 
> If you have a couple of minutes to spare and can look at the CSI2 PHY 
> documentation in the TRM, you might be more successful than me figuring out 
> how the hardware is implemented.

Took a look and it seems the phy is split into two parts. So probably
using the syscon mapping for the register in scm are is a good start.
At least then there's some protection from drivers tinkering directly
with the system control modules.

Maybe s	ee what drivers/regulator/pbias-regulator.c is doing with
syscon to see if that works? Moving that to some phy driver later on
should be trivial if needed :)

Regards,

Tony