Page MenuHomeFreeBSD

Allwinner A10/A20 i2c
ClosedPublic

Authored by manu_bidouilliste.com on Jan 10 2016, 4:50 AM.
Tags
Referenced Files
Unknown Object (File)
Fri, Mar 22, 7:04 PM
Unknown Object (File)
Dec 31 2023, 8:18 AM
Unknown Object (File)
Dec 31 2023, 8:18 AM
Unknown Object (File)
Dec 31 2023, 8:18 AM
Unknown Object (File)
Dec 31 2023, 8:17 AM
Unknown Object (File)
Dec 31 2023, 8:17 AM
Unknown Object (File)
Dec 31 2023, 8:17 AM
Unknown Object (File)
Dec 31 2023, 8:17 AM
Subscribers

Details

Summary

Add Allwinner i2c (TWI) driver.
Allwinner chips use the TWSI Marvell controller already supported but with different register addresses.
This patch splits the marvell driver into driver and i2c driver methods.

Test Plan

Apply patch, build kernel and verify that i2c is up. On A20 hardware the AXP209 PMU is at address 0x34 :
$ i2c -s
Scanning I2C devices on /dev/iic0: 34

I don't have Marvell hardware so I only tested that the kernel for dreamplug and sheevaplug compiled.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

manu_bidouilliste.com retitled this revision from to Allwinner A10/A20 i2c.
manu_bidouilliste.com updated this object.
manu_bidouilliste.com edited the test plan for this revision. (Show Details)
manu_bidouilliste.com set the repository for this revision to rS FreeBSD src repository - subversion.
manu_bidouilliste.com added a project: ARM.
sys/arm/allwinner/a10_i2c.c
68–72 ↗(On Diff #12086)

stylistically, this should be more like:

if (ofw_bus...("allwinner...") == 0)
return (ENXIO);

sc->is_allwinner = 1;

sys/arm/allwinner/files.allwinner
16–17 ↗(On Diff #12086)

not entirely sure I like this layout, but we don't have to fix this for the commit.

sys/arm/mv/mv_twsi.c
137–141 ↗(On Diff #12086)

same comments as before. Except is_allwinner will be zero because the softc is zerod when first allocated.

220–263 ↗(On Diff #12086)

Why do we do this here and not in the allwinner driver? Alternatively, why do we do this here at all? Shouldn't the iic bus code attach the children? or is the MV FDT different :( If so, there should be a comment here because this code doesn't make sense to me at all.

sys/arm/mv/twsi.c
376 ↗(On Diff #12086)

Not commenting on all of thse, but perhaps it would be better to create twsi_data and twsi_status register offsets in the softc.

sys/arm/mv/mv_twsi.c
220–263 ↗(On Diff #12086)

This is the original code from twsi.c I haven't touched this part.

manu_bidouilliste.com edited edge metadata.

Move register offset into the softc struct.

Rename driver to iichb.
Add bus and ofw interface methods.

Now i2c kernel device drivers are probed and attached correctly.

Update patch due to the last commit on mv_twsi.c and a10_clk.c on HEAD.

Update diff to add change introduce into rev 295567
Also now that A10 kernel config is into -HEAD, add iic and iicbus into it.

I think the driver should be moved to be, e.g. sys/dev/iicbus/twsi/.

sys/arm/allwinner/a10_i2c.c
71–77 ↗(On Diff #13251)

This should be moved to the attach function, and have the magic numbers changed to macros.

147–169 ↗(On Diff #13251)

I think it would make sense to create a base class for the common code, then subclass them here. You can see an example in the arm64 GIC driver. The code is under sys/arm64/arm64/ in gic.c, gic_fdt.c, and gic_acpi.c.

I think the driver should be moved to be, e.g. sys/dev/iicbus/twsi/.

Do you mean all the drivers or the common function ? (the ones currently in arm/mv/twsi.c)

Convert twsi to a driver class and move generic function to dev/iicbus/twsi/
Add defines for allwinner and marvell controller reg offset.
Move reg offset assignment to attach function.
Add twsi driver to files.arm since it's used in more that one SoC.

While converting to class the driver name changed from iichb so iic child device couldn't attach.
Fix that in this diff.

Update diff since the twsi device is now in -HEAD

sys/dev/iicbus/twsi/a10_twsi.c
34 ↗(On Diff #13303)

Needed?

45 ↗(On Diff #13303)

Needed?

50 ↗(On Diff #13303)

Sorting. Should be after the other iicbus headers.

54–62 ↗(On Diff #13303)

#define<tab>TWI...

85–100 ↗(On Diff #13303)

This looks to be common.

108–116 ↗(On Diff #13303)

This looks like it could be moved into a common attach function.

134–140 ↗(On Diff #13303)

Also common. I'd create twsi_attach, then call it from here, and where this code is in the Marvell attachment.

sys/dev/iicbus/twsi/mv_twsi.c
234 ↗(On Diff #13303)

This is unneeded, and may overflow dname. OF_getprop already includes the null terminator.

261 ↗(On Diff #13303)

And here

sys/dev/iicbus/twsi/twsi.c
226 ↗(On Diff #13303)

Could this (and the functions below) be static? It doesn't seem to be used outside this file.

manu_bidouilliste.com added inline comments.
sys/dev/iicbus/twsi/twsi.c
226 ↗(On Diff #13303)

Now that the device is a class yes, I'll change that.

manu_bidouilliste.com marked an inline comment as done.

Update diff to take care of andrew comments.

andrew added a reviewer: andrew.
This revision is now accepted and ready to land.Feb 15 2016, 3:04 PM
This revision was automatically updated to reflect the committed changes.