Differential D43746
Adding Support for QorIQ LX2160A I2C Controller Authored by pldrouin_gmail.com on Feb 5 2024, 4:00 AM. Tags Referenced Files
Details This is based on D24917, which was an attempt to provide I2C controller support for the LX2160A SOC, notably used by the SolidRun HoneyComb LX2K board. On systems that use the EDK2 UEFI firmware, the I2C controller's frequency divider gets properly configured at boot time, so there is no need for it to be reconfigured by the operating system. Basic testing SolidRun LX2160A CEX7 using the DK2 UEFI firmware in ACPI mode: Right after boot: After selecting MUX's channel 1 (MUX address is 0x77): TZ0 is still working. Have not witnessed interference between the thermal zone and the I2C controller driver so far: Retrying bus scanning:
Diff Detail
Event TimelineComment Actions I currently have it configured as a pair of modules. One weird thing I noticed is that I could kldload and kldunload them just fine when I was building the modules manually and before I included them in sys/modules/Makefile and I rebuilt the kernel. After adding them to sys/modules/Makefile, rebuilding and installing the kernel, I can only load them through /boot/loader.conf, otherwise it complains the modules are already loaded in the kernel, although kldstat -v and grep -R lx2160a_i2c /boot/kernel say otherwise. How can this be fixed? Thanks Comment Actions Ok I have found a few things to change. It looks like some extra delays might be required when the module is not run in DEBUG mode. Comment Actions Thanks, I will make these changes. I also have to tweak some delay that should apply when a start follows a reset. Comment Actions -Moved the source files to sys/dev/iicbus/controller/qoriq Note: lx2160a_i2c_fdt should not be loaded along with vf_i2c, as they share the same FDT ID. What is the best way to handle this? Comment Actions Removing unnecessary small delay in i2c_stop. It had been introduced earlier as an attempt to fix the issue which was resolved by using wait_for_nibb in i2c_start. Comment Actions Avoid use of "All rights reserved." in copyright notices? https://www.freebsd.org/internal/software-license/ reports: With the ratification of the Berne Convention in 2000, it became obsolete. As such, the FreeBSD project recommends that new code omit the phrase and encourages existing copyright holders to remove it. In 2018, the project updated its templates to remove it. Comment Actions The templates shown at https://www.freebsd.org/internal/software-license/ have a (nearly) blank line after the Copyright . . . line: * Copyright (c) [year] [your name] * * Redistribution and use in source and binary forms, with or without In general it is a good place to look at for comparisons. Comment Actions Removing the verification of IBSR_IBB in i2c_stop. It was most likely potentially problematic and should have not been used. Comment Actions Just basic style(9) review.
Comment Actions manu had written: "Also you should add the devices in sys/arm64/conf/std.nxp". Such was not done but there may be problems with doing as requested: I will note that the HoneyComb related EDK2-platforms is from the fork at: https://github.com/SolidRun/edk2-platforms/tree/de2ef829e9f69eef255617cb536d13fa03c20dcc which goes along with the forks: https://github.com/SolidRun/edk2/tree/11f2013a093172f1f8b49a746d39c1ef228e4760 SolidRun's EDK2 installation images are based on the forked context, not upstream EDK2 at this point. This alone may mean that manu's request is inappropriate. The detail below may also indicate such. Another thing I do not know is what other NXP related EDK2 versions that also have their own NXP0001 ACPI items are compatible vs. incompatible with what works for the lx2160a: Platform/NXP/LS1046aRdbPkg/AcpiTables/Dsdt/I2c.asl Those are from the fork. Upstream searching for NXP0001 did not find any matches (master branch). Upstream has the following base paths mentioning LX2160A: Platform/NXP/LX2160aRdbPkg/ But it is not the same as for SolidRun's EDK2 materials. (For example, the lack of a NXP0001 to find.) I do not know if this means naming here should indicate the solidrun or honeycomb context somehow instead of just referencing lx2160a. In other words, I'm not sure what specific alternative to suggest. More context: NXP has its own EDK2 forks as well: https://github.com/nxp-qoriq/uefi But the most recent update was something like 4 years ago. It was to edk2-platforms. https://github.com/nxp-qoriq/edk2-platforms has some NXP0001 material in its files: Platform/NXP/LX2160aRdbPkg/AcpiTables/Dsdt/I2c.asl Comment Actions Adding devices to sys/arm64/conf/std.nxp . The driver should work for LX2160A, LX2120A, LX2080A, LS1046A and LS1026A based on their respective reference manuals. The ACPI driver could be included in GENERIC. However as previously mentioned, I think including the FDT version along with vf_i2c could be problematic due to them sharing the same ID, so I commented out the lx2160a_i2c_fdt device. Comment Actions lx2160a_i2c_fdt and vf_i2c? I did start from Val Packett's modified/split vf_i2c driver, but even after disabling the handling of the clock divider register, the driver did not work properly. The way the registers are handled with the LX2160A controller seems to differ substantially. The vf_i2c driver is setting the IBAD register, which should not be changed to match the slave device address in the case of the LX2160A. The vf_i2c driver is not properly handling arbitration loss for it to work with the LX2160A controller. Also it is not handling the IBSR_IBB flag the way it is required by the LX2160A controller. Finally, vf_i2c is using a bunch of delays that are not needed for the LX2160A controller. Comment Actions So I looked at the VFXXX controller reference manual (https://www.nxp.com/docs/en/reference-manual/VFXXXRM.pdf), and the specs for the I2C controller look almost identical to the ones for the LX2160A I2C controller, except mostly for the clock divider table. Based on this information, I do not see why the new implementation could not work for the VFXXX controller. I would have to reintroduce the code by @dgr_semihalf.com to do the clock divider calculations for the devices that differ from the LX2160A though. Comment Actions The main issue I have for the FDT driver, is the following couple of lines in vf_i2c.c: if (sc->freq == 0) return vf610_div_table[nitems(vf610_div_table) - 1].reg_val; When sc->freq is 0, which occurs when the parent clock is not found, the divider defaults to the highest value. In the case of the LX2160A, the absence of parent clock is the only way I can think of to detect a controller where the clock divider value is set at boot time and should not be changed. @dgr_semihalf.com, is this highest divider value when there is no parent clock used by any other device? Comment Actions Based on the information I posted earlier, I think I could merge both drivers. For systems that neither have their I2C controller divider configured by the boot loader nor have the required system bus speed information in FDT to determine it, I would suggest passing a flag to the driver in loader.conf to make it use the slowest speed., Currently in vf_i2c, the highest driver's divider speed is selected by default unless the parent clock speed information is available. Otherwise I would need to add higher divider values to the table, slowing the I2C bus even more for the devices on all other systems that don't provide a parent clock speed. Does my suggestion appear reasonable? Thanks!
Comment Actions Merging the code with the existing driver in D43811. This differential is now obsolete. This comment was removed by pldrouin_gmail.com. This comment was removed by pldrouin_gmail.com.
Comment Actions In the bottom Submit area there is an "Add Action..." button in the top of the subarea. For the author, it should show an option for abandonment that you could select. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||