For historical reasons TI's platforms (AM335x and OMAP4) support on FreeBSD used custom-made DTS
files. In order to be compatible with upstream (TI) and re-use all DTSes for extension shields on Beaglebone Black we need to move to using vendor's DTS files.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
This is first revision of the migration to TI's DTS patch. Changes are not backward-compatible with old DTS due to different tree organization structure (more details below). Patch includes following changes:
- simplebus_attach_children helper routine to add sub-buses to simple bus. e.g. PWM subsytem is organized as bus with two children eHRPW and eCAP. This functions enumerate child nodes, sets simplebus-compatible device info and create device instances. All bus-related resource/interrupt-releated requests are proxied through sub-bus up to simplebus driver.
- uc_regshft field added to UART class structure. TI's DTS doe not have reg-shift property which should be 2 and default value in current uart driver is 0. uc_regshft is used as a fallback if value is not provided by DTS
- Rewritten TI/AM335x drivers: ADC, CPSW, EDMA3, GPIO, I2C, pinmux, System Control, PRCM, PMIC
Some drivers required only minor changes like fixing compatible strings but several of them were rewritten quite a lot
dmtimer: Instead of one device with 7 timers there are now multiple timer devices. Disabled PPS supprt until I figure out how to get it back properly.
GPIO: Instead of one GPIO device with 4 banks now there are 4 devices, each one controls its own pin bank. Which is possible major PITA for existing setups.
PWMSS: converted to bus with two child devices ecasX and ehrpwmX
pinux/sc driver was split in two parts: pinmux and system control
USBSS: usb converted to bus with two musb devices
cpsw: Had to do some hackish resource management because Linux's way of resource handling and description doesn't quite match FreeBSD's abstraction.
Also to identify device module number ti,hwmod is used in somewhat hackish way.
Comment Actions
- The PRU driver needs to be changed as well, but the vendor DTS don't contain the PRU description block.
- The WDT driver should work ok.
- The mailbox driver probably needs to be changed to use ti,omap4-mailbox as the compatible string.
| sys/arm/ti/am335x/am335x_dmtimer.c | ||
|---|---|---|
| 637 ↗ | (On Diff #4427) | release mem, irq resources acquired earlier. likewise in ENXIO case a few lines above. |
| sys/arm/ti/am335x/am335x_pmic.c | ||
| 118 ↗ | (On Diff #4427) | I think "<< 1" would make it more clear that this was adjusting the iic bus address for the low-order write bit. More to the point: why do we need this? There is no other caller of iicbus_get_addr() in the system right now that does this kind of adjustment to the return value. |
| sys/arm/ti/cpsw/if_cpsw.c | ||
| 577 ↗ | (On Diff #4427) | fdt32_to_cpu() seems wrong after OF_getencprop(). |
| 592 ↗ | (On Diff #4427) | "node node" |
| sys/arm/ti/ti_hwmods.c | ||
| 65 ↗ | (On Diff #4427) | OF_getproplen() + malloc + OF_getprop() = OF_getprop_alloc() (with elsz 1, retval is str len), free using M_OFWPROP mem type. (really we should hide that under a OF_freeprop()) |
| sys/arm/ti/ti_pinmux.c | ||
| 469 ↗ | (On Diff #4427) | probably not the right name anymore |
| 498 ↗ | (On Diff #4427) | timer interface? |
| 504 ↗ | (On Diff #4427) | this doesn't seem right, all the pinmux data should now be in the pinctrl,single format and there should be no need for all the old code we had for doing things based on name strings. |
| sys/arm/ti/ti_pinmux.h | ||
| 20 ↗ | (On Diff #4427) | if this file is new, it shouldn't be 4-clause with Ben's copyright |
| sys/arm/ti/ti_scm.c | ||
| 95 ↗ | (On Diff #4427) | should this be "ti,am33xx-controlmodule"? is there anything left in this device at all? |
| sys/arm/ti/ti_pinmux.h | ||
|---|---|---|
| 20 ↗ | (On Diff #4427) | It's not exactly new. It's part of old ti_scm module with minor changes. TI DTS has separate devices for pinmux and system control modules. |
| sys/arm/ti/ti_scm.c | ||
| 95 ↗ | (On Diff #4427) | This driver is shared with OMAP4 so probably more generic syscon is preferrable. I'll know when I have clear understanding what has to be done for OMAP4. The only function device provides is access to control registers window that enable/disable various devices. |
Comment Actions
I have just looked at some general interrupt parts of GPIO device to minimalize a little bit changes which would come with new interrupt framework.
| sys/arm/ti/ti_gpio.c | ||
|---|---|---|
| 558 ↗ | (On Diff #4427) | This comment is obsolete now. |
| 574 ↗ | (On Diff #4427) | I would prefer to delete this function and move the content to device attach routine now. Further, testing sc_irq_res for NULL is done already there. |
| 596 ↗ | (On Diff #4427) | I would prefer to delete this function and move the content to device detach routine now. Further, testing sc_irq_hdl for NULL is sufficient. |
| 697 ↗ | (On Diff #4427) | Does it mean that bank interrupts are not optional any more? There was RF_OPTIONAL flag for all banks except the first one before. |
Comment Actions
New revision of diff includes following changes:
- Fixes for issues pointed out by reviewers
- OMAP4 portion of TI support migrated to vendor's FDT
- TDS files in boot/fdt/dts/arm are wrappers over TI's DTS files in gnu/. They also include freebsd-specific parts like console node for pandaboard and pruss node for BBB
Comment Actions
Nice work. Please use -U99999 for diffs next time.
We can test this patch in next one or two days, on Panda and BBB.
| sys/arm/ti/aintc.c | ||
|---|---|---|
| 95–96 ↗ | (On Diff #4646) | ofw_bus_search_compatible() ? |
| sys/arm/ti/am335x/am335x_pwmss.c | ||
| 73–75 ↗ | (On Diff #4646) | Do you have it verified? The child nodes have memory and interrupt properties defined, but this driver doesn't pass any of resource manager bus method calls down to its parent. Is not a subclassing simplebus proper solution for this? |
| 156 ↗ | (On Diff #4646) | This call is superfluous. dinfo is initialized in simplebus_add_device() call. |
| sys/arm/ti/am335x/am335x_usbss.c | ||
| 189 ↗ | (On Diff #4646) | Same as in pwmss case. |
| 215–220 ↗ | (On Diff #4646) | Same as in pwmss case. |
| sys/dev/fdt/fdt_common.c | ||
| 60 ↗ | (On Diff #4646) | Why 32? |
| sys/dev/ofw/ofw_bus_subr.c | ||
| 101 ↗ | (On Diff #4646) | This function is not used yet. |
Comment Actions
Firstly, thank you for your hard work.
Can you please update you patch for current? Here are diffs which does not apply:
BBB summary: It looks that everything works well. However I've got the following error:
ti_scm0: <TI Control Module> mem 0x44e10000-0x44e107fb on ofwbus0
....
ti_scm1: <TI Control Module> mem 0x44e10000-0x44e107ff on simplebus0
device_attach: ti_scm1 attach returned 6
That's due to that only one instance of ti_scm driver is allowed but two of them are defined in dts files.
PANDABOARD summary: It's almost well but I do not get login prompt. I've got the followint errors:
unsupported trigger/polarity configuration 0x a
unsupported trigger/polarity configuration 0x a
unsupported trigger/polarity configuration 0x 8
The trigger/polarity config should be processed better in gic_decode_fdt(). Michal already has a patch needed for his tegra:
l2cache0: cannot allocate IRQ, not using interrupt
I'm not sure if logging it is needed when L2 cache can work without it.
ti_pinmux0: <TI Pinmux Module> mem 0x4a100040-0x4a1001d5 on simplebus0
ti_pinmux1: <TI Pinmux Module> mem 0x4a31e040-0x4a31e077 on simplebus0
device_attach: ti_pinmux1 attach returned 6
It's similar to ti_scm case. Only one instance is allowed. Maybe this is the problem that I do not get login prompt.
ehci0: <TI OMAP USB 2.0 controller> mem 0x4a064c00-0x4a064fff irq 109 on omap_uhh0
ehci0: Starting TI EHCI USB Controller
ehci0: PHY reset operation timed out
| sys/arm/ti/ti_common.c | ||
|---|---|---|
| 74 ↗ | (On Diff #4646) | gic_decode_fdt not defined here |
| sys/boot/fdt/dts/arm/pandaboard-common.dtsi | ||
| 39 ↗ | (On Diff #4646) | global-timer? |
Comment Actions
Well, here are PANDABOARD boot logs if you are interested.
- kernel without patch
- kernel with patch
| sys/arm/ti/aintc.c | ||
|---|---|---|
| 95–96 ↗ | (On Diff #4646) | Yes, it's cleaner. Switched to it in new revision |
| sys/arm/ti/am335x/am335x_pwmss.c | ||
| 73–75 ↗ | (On Diff #4646) | Good catch. I overlooked subcalssing part. Fixed in new revision. |
| sys/boot/fdt/dts/arm/pandaboard-common.dtsi | ||
| 39 ↗ | (On Diff #4646) | Fixed in new revision |
| sys/dev/fdt/fdt_common.c | ||
| 60 ↗ | (On Diff #4646) | It's from specification: |
| sys/dev/ofw/ofw_bus_subr.c | ||
| 101 ↗ | (On Diff #4646) | Removed in new revision |
Comment Actions
This problem can be fixed by https://reviews.freebsd.org/D2345
unsupported trigger/polarity configuration 0x a
unsupported trigger/polarity configuration 0x a
unsupported trigger/polarity configuration 0x 8
Comment Actions
BBB summary: It looks that everything works well. However I've got the following error:
ti_scm0: <TI Control Module> mem 0x44e10000-0x44e107fb on ofwbus0
....
ti_scm1: <TI Control Module> mem 0x44e10000-0x44e107ff on simplebus0
device_attach: ti_scm1 attach returned 6
It's misleading indeed. I moved the check for multiple scm devices to probe so the second instance does not generate error any more
That's due to that only one instance of ti_scm driver is allowed but two of them are defined in dts files.
PANDABOARD summary: It's almost well but I do not get login prompt. I've got the followint errors:
unsupported trigger/polarity configuration 0x a
unsupported trigger/polarity configuration 0x a
unsupported trigger/polarity configuration 0x 8The trigger/polarity config should be processed better in gic_decode_fdt(). Michal already has a patch needed for his tegra:
I'd like to fix it as a separate issue once the patch is merged. It doesn't seem to affect existing functionality
l2cache0: cannot allocate IRQ, not using interrupt
> I'm not sure if logging it is needed when L2 cache can work without it.
It's already in mainline, but probably you're right and it shouldn't be a warning but for now I'll keep it out of scope of this review
ti_pinmux0: <TI Pinmux Module> mem 0x4a100040-0x4a1001d5 on simplebus0
ti_pinmux1: <TI Pinmux Module> mem 0x4a31e040-0x4a31e077 on simplebus0
device_attach: ti_pinmux1 attach returned 6It's similar to ti_scm case. Only one instance is allowed. Maybe this is the problem that I do not get login prompt.
Fix the same as for SCM - perform check in probe. There should be no problem with having multiple pinctrl instances but it doesn't work for current model. We have pinmux data built in as part of kernel and gpio driver modifies pins config directly. I believe that it should be fixed but I do not want to combine unrelated fixes in this patch.
ehci0: <TI OMAP USB 2.0 controller> mem 0x4a064c00-0x4a064fff irq 109 on omap_uhh0
ehci0: Starting TI EHCI USB Controller
ehci0: PHY reset operation timed out
There was unconditional reset for non-connected port. Was fixed in new revision.
Comment Actions
The lack of login prompt is due to change of serial port device in system. /etc/ttys should use /dev/ttyu2 for serial console
Comment Actions
New version tested on beaglebone and pandaboard again. It looks that everything works (except already mentioned things you want to fix later). However, I just booted to multiuser and checked over dmesgs.