Page MenuHomeFreeBSD

Add ACPI attachment for the Vybrid VF610 I2C controller
Changes PlannedPublic

Authored by greg_unrelenting.technology on Tue, May 19, 2:33 PM.

Details

Reviewers
mw
manu
dgr_semihalf.com
Group Reviewers
arm64
Summary

This expands on D24361, splitting FDT into a separate file and adding ACPI, to support the SolidRun LX2160A CEX7 module (e.g. on the SolidRun HoneyComb LX2K workstation board).

Ignore the files.arm64, I can fix that after D24361 and stuff lands (note: I don't like the SOC_NXP_LS1046A option, that's overly specific, this kind of hardware is common to the whole layerscape family)

Test Plan

Tested on: SolidRun LX2160A CEX7, by Dan Kotowski on freebsd-arm@

i2c0: <Vybrid Family Inter-Integrated Circuit (I2C)> iomem 0x2000000-0x200ffff irq 7 on acpi0
iicbus0: <Philips I2C bus (ACPI-hinted)> on i2c0
iicbus0: <unknown card> at addr 0x77
iic0: <I2C generic I/O> on iicbus0
i2c1: <Vybrid Family Inter-Integrated Circuit (I2C)> iomem 0x2040000-0x204ffff irq 8 on acpi0
iicbus1: <Philips I2C bus (ACPI-hinted)> on i2c1
iic1: <I2C generic I/O> on iicbus1

seems to detect all numbers as present though o_0 that's probably not good

Scanning I2C devices on /dev/iic0: 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f 30 31 32 33 34 35 36 37 38 39 3a 3b 3c 3d 3e 3f 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f 50 51 52 53 54 55 56 57 58 59 5a 5b 5c 5d 5e 5f 60 61 62 63 64 65 66 67 68 69 6a 6b 6c 6d 6e 6f 70 71 72 73 74 75 76 77 78 79 7a 7b 7c 7d 7e

Maybe setting the freq would help..

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

greg_unrelenting.technology edited the test plan for this revision. (Show Details)Tue, May 19, 4:43 PM
greg_unrelenting.technology edited the summary of this revision. (Show Details)
greg_unrelenting.technology edited the test plan for this revision. (Show Details)
greg_unrelenting.technology planned changes to this revision.Sun, May 24, 12:44 PM

Looks like this needs to wait until there's some way to get the clock without devicetree (I guess Linux runs with fdt+acpi both to make it work..)

manu added a comment.Sun, May 24, 12:59 PM

Looks like this needs to wait until there's some way to get the clock without devicetree (I guess Linux runs with fdt+acpi both to make it work..)

That don't make sense to run in fdt+acpi, I don't even think that it is possible in Linux.

mw added a comment.Sun, May 24, 10:34 PM
In D24917#550060, @manu wrote:

Looks like this needs to wait until there's some way to get the clock without devicetree (I guess Linux runs with fdt+acpi both to make it work..)

That don't make sense to run in fdt+acpi, I don't even think that it is possible in Linux.

Clock subsystem is out of OS control when running with Linux + ACPI and IMO this should be true for the FreeBSD as well. In such case we rely on firmware to manage the power/clocking stuff.

In D24917#550242, @mw wrote:
In D24917#550060, @manu wrote:

Looks like this needs to wait until there's some way to get the clock without devicetree (I guess Linux runs with fdt+acpi both to make it work..)

That don't make sense to run in fdt+acpi, I don't even think that it is possible in Linux.

Clock subsystem is out of OS control when running with Linux + ACPI and IMO this should be true for the FreeBSD as well. In such case we rely on firmware to manage the power/clocking stuff.

Well then how does the driver for NXP0001

https://github.com/torvalds/linux/blob/bef7b2a7be28638770972ab2709adf11d601c11a/drivers/i2c/busses/i2c-imx.c#L258

unconditionally require a platform clock in probe?

https://github.com/torvalds/linux/blob/bef7b2a7be28638770972ab2709adf11d601c11a/drivers/i2c/busses/i2c-imx.c#L1160-L1166

Linux's acpi_apd integration seems to be really deep, I guess these devices kind of become proper OFW platform members

mw added a comment.Mon, May 25, 1:55 AM
In D24917#550242, @mw wrote:
In D24917#550060, @manu wrote:

Looks like this needs to wait until there's some way to get the clock without devicetree (I guess Linux runs with fdt+acpi both to make it work..)

That don't make sense to run in fdt+acpi, I don't even think that it is possible in Linux.

Clock subsystem is out of OS control when running with Linux + ACPI and IMO this should be true for the FreeBSD as well. In such case we rely on firmware to manage the power/clocking stuff.

Well then how does the driver for NXP0001

https://github.com/torvalds/linux/blob/bef7b2a7be28638770972ab2709adf11d601c11a/drivers/i2c/busses/i2c-imx.c#L258

unconditionally require a platform clock in probe?

https://github.com/torvalds/linux/blob/bef7b2a7be28638770972ab2709adf11d601c11a/drivers/i2c/busses/i2c-imx.c#L1160-L1166

Linux's acpi_apd integration seems to be really deep, I guess these devices kind of become proper OFW platform members

drivers/acpi/acpi_apd.c gives an option to mock a real clock with a fixed-rate one - acpi_apd_setup() routine registers such fixed clock in the controllers platform data. This way, the clk_get can return it without error, avoiding the OFW path (the section under if (dev && dev->of_node) { is omitted.

As you can see, this way of adding fixed clock is used for SPI/I2C/UART controllers, where you need to calculate the output rate. As mentioned before, the actual control over enabling the clocks in HW is done in firmware.

The method used in Linux is IMO awful - a central place with platform devices description only for specifying a clock rate. I know SoCs, which use same controller in various variants, but feed them with the different frequency. Therefore I think a proper way would be either add 'clock-frequency' to the _DSD package of the controller in DSDT or hardcode a frequency in the driver when working with ACPI - this is what the Linux effectively does.

Therefore I think a proper way would be either add 'clock-frequency' to the _DSD package of the controller in DSDT

It does have that:

Name (_DSD, Package () {
  ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
  Package () {
     Package () {"clock-frequency", DEFAULT_PLAT_FREQ}, //This is device specific data, Need to see how to pass clk stuff
  }
})

So that's cool I guess, I'll just need to finally work on DSD access.

What confused me the most was i2c_get_div_val which does calculations with both sc->freq, which *is* the i2c device's clock-frequency, AND a frequency of a clock device! That made me think the frequency from the clock is something very dynamic.