UART changes for ns8250 uart on denverton Intel SoC
Needs ReviewPublic
Actions

Authored by stevek on Apr 15 2023, 8:40 PM.

Details

Reviewers

imp
cperciva
jhb

Summary

The uart chip on denverton is ns8250 compatible, except denverton
requires different uart bus param settings after uart driver is
initialised.

Added a new dnv8250 uart class.

Adjust pre-scalar to get fuart(= baudrate * ps) close to uart clock
freq. Derive mul & div to fraction the uart clock freq to equal fuart.

Added quirk to not call ns8250_init during bus_probe call, to skip
ns8250_param.

Submitted by: Lakshman Likith Nudurupati <lnlakshman@juniper.net>
Obtained from: Juniper Networks, Inc.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 50925
Build 47816: arc lint + arc unit

Event Timeline

stevek created this revision.Apr 15 2023, 8:40 PM

Herald added a subscriber: imp. · View Herald TranscriptApr 15 2023, 8:40 PM

stevek requested review of this revision.Apr 15 2023, 8:40 PM

Harbormaster completed remote builds in B50925: Diff 120375.Apr 15 2023, 8:40 PM

stevek added reviewers: imp, cperciva, jhb.Apr 18 2023, 4:11 PM

jhb added inline comments.Apr 18 2023, 9:44 PM

sys/dev/uart/uart_bus_pci.c
77	A more typical approach for quirks in other drivers would be to extend this structure with a new `quirks` member and to set those here rather than having a separate table for each quirk.
302	I would be tempted to make the decision of which class to use a new quirk so you can still stick with a single table.
311	This would be `id->quirks`, etc.
sys/dev/uart/uart_dev_dnv8250.c
63	You don't need this type at all. Just use `struct ns8250_softc` directly as the softc.
sys/dev/uart/uart_dev_ns8250.c
840
sys/modules/uart/Makefile
19	Seems like this needs to be paid with the above .if? Either way, I would maybe do this change as a separate commit perhaps or just leave it out?
32	It would seem more readable if you instead made this .if ${MACHINE_CPUARCH} == "i386" \|\| ${MACHINE_CPUARCH} == "amd64"

stevek added inline comments.Apr 19 2023, 7:19 AM

sys/dev/uart/uart_bus_pci.c
77	True. Makes sense to me.
sys/dev/uart/uart_dev_dnv8250.c
63	Yes, you are right, I'm not sure why the original author decided to do it that way. Probably was copy/pasting from some other driver.
sys/modules/uart/Makefile
19	Actually, now that I look, I'm not sure why this change is needed at all. The original should have been fine. I'll remove it.
32	We use the :N form a lot in Juniper, as it is cheaper than the above that you mention. It's the form that sjg suggests, which is why it's there.

given how small the dnv8250 driver is, I'm surprised it's not just a simple quirk for the normal driver like half a dozen others.. Seems like a lot of extra mechanism for not a lot of gain.

In D39587#903566, @imp wrote:

given how small the dnv8250 driver is, I'm surprised it's not just a simple quirk for the normal driver like half a dozen others.. Seems like a lot of extra mechanism for not a lot of gain.

The pre-scalar and clock gen registers are Intel specific additions, from what I understand, so I don't see how a quirk would make sense.

I could see adding a function pointer to the ns8250 softc structure to avoid the need for copying the ns8250_bus_param. The call to ns8250_param() in ns8250_bus_param() could then just call through the function pointer.

The LCR register handling in ns8250_param() can be split off into its own function that dnv8250.c could then call and not duplicate that part.

Also, could probably drop the "8250" part of the uart_dev_dnv8250.c file name (change it to uart_dev_dnv.c)

In D39587#903566, @imp wrote:

given how small the dnv8250 driver is, I'm surprised it's not just a simple quirk for the normal driver like half a dozen others.. Seems like a lot of extra mechanism for not a lot of gain.

I thought about that, but I feel like the code to handle the quirk would be ugly compared to the current approach of just adding a new uart_param method.

sys/dev/uart/uart_dev_dnv8250.c
67	This can be static.

In D39587#903619, @stevek wrote:

The pre-scalar and clock gen registers are Intel specific additions, from what I understand, so I don't see how a quirk would make sense.

I could see adding a function pointer to the ns8250 softc structure to avoid the need for copying the ns8250_bus_param. The call to ns8250_param() in ns8250_bus_param() could then just call through the function pointer.

The LCR register handling in ns8250_param() can be split off into its own function that dnv8250.c could then call and not duplicate that part.

Also, could probably drop the "8250" part of the uart_dev_dnv8250.c file name (change it to uart_dev_dnv.c)

I think it make sense to keep 8250 as it's definitely an 8250-style UART. Is the custom logic here really confined to Denverton chipsets though and not something that might be present on other chipsets? Given the proliferation of Intel chipset names that are all nearly identical from a software point of view, it seems unlikely for this to really be specific to just one (or is Denverton a reference to a family of chipsets like Atom is for CPUs?)

In D39587#904360, @jhb wrote:

I think it make sense to keep 8250 as it's definitely an 8250-style UART. Is the custom logic here really confined to Denverton chipsets though and not something that might be present on other chipsets? Given the proliferation of Intel chipset names that are all nearly identical from a software point of view, it seems unlikely for this to really be specific to just one (or is Denverton a reference to a family of chipsets like Atom is for CPUs?)

Denverton is a chipset in the Atom family.

It looks like in Linux the driver source file is called 8250_mid.c and it supports this specific UART functionality for 4 different chipsets from what I can see: Penwell, Tangier, Cedar Fork, Denverton
I believe that "MID" was referring to the "Mobile Internet Devices" platform, which included a number of other chips besides the chipsets mentioned above.

Do you think it would make sense to follow the naming and call it uart_dev_mid.c or something similar to that?

In D39587#904380, @stevek wrote:

In D39587#904360, @jhb wrote:

I think it make sense to keep 8250 as it's definitely an 8250-style UART. Is the custom logic here really confined to Denverton chipsets though and not something that might be present on other chipsets? Given the proliferation of Intel chipset names that are all nearly identical from a software point of view, it seems unlikely for this to really be specific to just one (or is Denverton a reference to a family of chipsets like Atom is for CPUs?)

Denverton is a chipset in the Atom family.

It looks like in Linux the driver source file is called 8250_mid.c and it supports this specific UART functionality for 4 different chipsets from what I can see: Penwell, Tangier, Cedar Fork, Denverton
I believe that "MID" was referring to the "Mobile Internet Devices" platform, which included a number of other chips besides the chipsets mentioned above.

Do you think it would make sense to follow the naming and call it uart_dev_mid.c or something similar to that?