Page MenuHomeFreeBSD
Differential D4168

i.MX6 IPU drivers
ClosedPublic
Actions

Authored by gonzo on Nov 15 2015, 7:43 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Dec 28, 1:30 PM
Unknown Object (File)
Sun, Dec 22, 6:26 AM
Unknown Object (File)
Sun, Dec 22, 6:26 AM
Unknown Object (File)
Fri, Dec 20, 10:22 PM
Unknown Object (File)
Fri, Dec 20, 9:57 PM
Unknown Object (File)
Fri, Dec 20, 9:22 PM
Unknown Object (File)
Thu, Dec 12, 2:34 PM
Unknown Object (File)
Sat, Dec 7, 9:54 AM
View All Files
Subscribers
andrew
imp

Details

Reviewers
• ian
Commits
rS292574: Add i.MX 6 IPU driver and enable it in IMX6 config
rS292565: Add CCM functions to enable HDMI framer and IPU units (video controller)
Summary

This diff contains driver for IPU and HDMI framer.
Most of patch is self-contained. I had to add OF_device_register_xref so when usign reference to i2c device in hdmi node we can get actual iic device. not iicbus.

Driver simplifies actual structure of IPU. iMX6 can have up to two IPUs (IPU1 amd IPU2), each of them with two display interfaces (DI0, DI1) Driver assumes that display is on DI0 at IPU1, it's something we'll need to address later.

Driver also assumes that pixel clock is initialized by U-Boot for default 1026x768 mode. In order to use other modes we need to set pixel clock accordingly and for this we need more sophisticated clock control framework.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

gonzo updated this revision to Diff 10207.Nov 15 2015, 7:43 PM
gonzo retitled this revision from to i.MX6 IPU and HDMI drivers.
gonzo updated this object.
gonzo edited the test plan for this revision. (Show Details)
gonzo added a reviewer: • ian.
gonzo set the repository for this revision to rS FreeBSD src repository - subversion.
Herald added subscribers: andrew, imp. · View Herald TranscriptNov 15 2015, 7:43 PM
andrew added a comment.Nov 15 2015, 9:50 PM

There are many magic numbers, and not many comments explaining what is happening. There are also style(9) issues.

Would it pay to split the review into two, one for ipu, one for hdmi?

Is imx_hdmi_sc needed? It seems like all calls to imx_hdmi_read_1 and imx_hdmi_write_1 already have access to a softc to pass to RD1 or WR1.

sys/arm/freescale/imx/imx6_ccm.c
367
void
imx_ccm_hdmi_enable(void)
sys/arm/freescale/imx/imx6_ccmreg.h
59–67

This indentation looks different than above. Is it correct?

sys/arm/freescale/imx/imx6_hdmi.c
114 ↗(On Diff #10207)

Missing newline.

115 ↗(On Diff #10207)

Why imx_hdmi_read_1 and not RD1?

124 ↗(On Diff #10207)

Missing newline after void.

125 ↗(On Diff #10207)

Extra indentation.

127 ↗(On Diff #10207)

Why 0xFF?

286 ↗(On Diff #10207)

static void\n

323 ↗(On Diff #10207)

What's special about 45250000?

325–369 ↗(On Diff #10207)

Many magic values

sys/arm/freescale/imx/imx6_ipu.c
719–735

What are all these magic numbers for?

903

if (err != 0) {

911

Ditto

949–950

Shouldn't err be set?

953–954

And here.

957

Is this needed? I don't see how err could be non-zero here.

983

Extra space after the (

gonzo mentioned this in D4174: i.MX6 HDMI framer driver.Nov 16 2015, 4:06 AM

Andrew, I cleaned up HDMI driver per your suggestions and moved it to separate revision: D4174. I'll upload updated IPU driver here later.

gonzo updated this revision to Diff 10752.Dec 4 2015, 10:35 PM
gonzo retitled this revision from i.MX6 IPU and HDMI drivers to i.MX6 IPU drivers.
gonzo removed rS FreeBSD src repository - subversion as the repository for this revision.

Update for IPU part of the driver:

  • style clean-up
  • magic numbers scrubbing
gonzo updated this revision to Diff 10806.Dec 6 2015, 12:33 AM

Remove no-op IPU_READ4s that were used for debug purpose

• ian added inline comments.Dec 13 2015, 12:12 AM
sys/arm/freescale/imx/imx6_ipu.c
64

the freebsd idiom for this is #define EDID_DEBUG_not (although I haven't seen it as much recently as in years past)

195

the DI_COUNTER and DI_SYNC things seem strangely indented, as if they're subordinate to some missing IPU_DI_something register definitions.

421

I think if offset were defined as u_int the compiler would have a better chance of converting these divide ops into shifting and masking.

426

this check should probably be before the 1 << len. Also, this function is called from all those macros above, right? so len > 32 is a programming error, not a runtime error? if so, KASSERT is better.

and likewise in ​ipu_ch_param_get_value() below

571

a lot of this arithmetic is missing the spaces between operators throughout all these functions

609

should this be "microcode"?

749

s/User/Use/

1026

could just pass BUS_DMA_ZERO to bus_dmamem_alloc()

1168

why do we need this have_ipu thing enforcing that only one instance can exist? I don't see any global or static vars or anything that imply there can be only one.

gonzo added inline comments.Dec 18 2015, 1:23 AM
sys/arm/freescale/imx/imx6_ipu.c
1168

Because if there are more than one IPU instance they all handle HDMI connect event and create framebuffer. This will be handled later when I add support for multiple video paths.

gonzo updated this revision to Diff 11403.Dec 18 2015, 1:24 AM

Address latest round of review comments

• ian accepted this revision.Dec 21 2015, 7:22 PM
• ian edited edge metadata.
This revision is now accepted and ready to land.Dec 21 2015, 7:22 PM
Closed by commit rS292565: Add CCM functions to enable HDMI framer and IPU units (video controller) (authored by gonzo). · Explain WhyDec 21 2015, 8:17 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
sys/
arm/
conf/
6 lines
freescale/
imx/
5 lines
37 lines
16 lines
1211 lines
2 lines

Diff 10806

View Options

sys/arm/conf/IMX6

Loading...
View Options

sys/arm/freescale/imx/files.imx6

Loading...
View Options

sys/arm/freescale/imx/imx6_ccm.c

Loading...
View Options

sys/arm/freescale/imx/imx6_ccmreg.h

Loading...
View Options

sys/arm/freescale/imx/imx6_ipu.c

Loading...
View Options

sys/arm/freescale/imx/imx_ccmvar.h

Loading...