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

* Laurent Pinchart [off-list ref] [140613 03:30]:

Hi Tony,

On Friday 13 June 2014 00:53:25 Tony Lindgren wrote:

quoted

* Laurent Pinchart [off-list ref] [140612 23:48]:

quoted

On Thursday 12 June 2014 22:30:44 Tony Lindgren wrote:

quoted

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.
 

quoted

quoted

quoted

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.

quoted

quoted

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 :)

quoted

quoted

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).

quoted

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