Page MenuHomeFreeBSD
Differential D48532

riscv: Add driver for the cvitek restart controller
AcceptedPublic
Actions

Authored by bnovkov on Jan 20 2025, 6:34 PM.
Tags
None
Referenced Files
F137682291: D48532.diff
Mon, Nov 24, 9:45 PM
Unknown Object (File)
Sun, Nov 23, 6:26 PM
Unknown Object (File)
Wed, Nov 12, 1:22 AM
Unknown Object (File)
Tue, Nov 11, 10:41 AM
Unknown Object (File)
Sat, Nov 8, 11:57 PM
Unknown Object (File)
Sat, Nov 1, 11:05 PM
Unknown Object (File)
Thu, Oct 30, 5:12 AM
Unknown Object (File)
Mon, Oct 27, 11:15 AM
View All 79 Files
Subscribers
imp
jrtc27
mhorne
riscv

Details

Reviewers
br
Summary

This patch adds support for the cvitek restart controller.
This controller is present on the Milk-V riscv SoCs.

Test Plan

Tested on a Milk-V Duo S board.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

bnovkov created this revision.Jan 20 2025, 6:34 PM
Herald added subscribers: riscv, imp. · View Herald TranscriptJan 20 2025, 6:34 PM
bnovkov requested review of this revision.Jan 20 2025, 6:34 PM
jrtc27 added a subscriber: jrtc27.Jan 20 2025, 6:44 PM
jrtc27 added inline comments.
sys/riscv/cvitek/cvitek_restart.c
118

Don't need to check this

br added inline comments.Jan 20 2025, 7:11 PM
sys/riscv/cvitek/cvitek_restart.c
43

sys headers first, then vm headers (if any), then machine headers, then dev

155

this should fit into line, no wrapping needed

bnovkov updated this revision to Diff 149682.Jan 21 2025, 7:04 PM

Address @br 's and @jrtc27 's comments:

  • remove redundant property checks
  • style fixes
bnovkov marked 3 inline comments as done.Jan 21 2025, 7:10 PM
br added inline comments.Jan 21 2025, 10:46 PM
sys/riscv/cvitek/cvitek_restart.c
47

why this header is needed?

mhorne added a subscriber: mhorne.Jan 22 2025, 2:48 PM
mhorne added inline comments.
sys/riscv/cvitek/cvitek_restart.c
77

You can't use a switch statement here; howto is a bitmask not an enum. You'll need to check each flag in an if case.

Off the top of my head it is possible to have RB_HALT and RB_POWEROFF both set (and possibly other cases).

imp added inline comments.Jan 22 2025, 3:06 PM
sys/riscv/cvitek/cvitek_restart.c
77

Yes. RB_POWEROFF and RB_POWERCYCLE will never be set at the same time.
I'm not sure what SHUTDOWN does: if it just stops the CPU, but leaves power applied, then it should be set when RB_HALT is set, but RB_POWERCYLE or RB_POWEROFF are not. If it does remove power, it should only be when RB_POWEROFF is set (no need to test for RB_HALT, that will also be set, but it will always be set).

bnovkov updated this revision to Diff 149965.Jan 25 2025, 6:18 PM

Properly handle howto bitmask.

bnovkov updated this revision to Diff 149966.Jan 25 2025, 6:21 PM

Minor style(9) fixes.

bnovkov marked an inline comment as done.Jan 25 2025, 6:24 PM
bnovkov added inline comments.
sys/riscv/cvitek/cvitek_restart.c
47

Thanks for catching this, I've removed the stray header.

77

You can't use a switch statement here; howto is a bitmask not an enum. You'll need to check each flag in an if case.

Off the top of my head it is possible to have RB_HALT and RB_POWEROFF both set (and possibly other cases).

Oh, was not aware of that, thanks!
It should be handled properly now.

Yes. RB_POWEROFF and RB_POWERCYCLE will never be set at the same time.
I'm not sure what SHUTDOWN does: if it just stops the CPU, but leaves power applied, then it should be set when RB_HALT is set, but RB_POWERCYLE or RB_POWEROFF are not. If it does remove power, it should only be when RB_POWEROFF is set (no need to test for RB_HALT, that will also be set, but it will always be set).

The manual is a bit vague and uses both terms interchangeably, but it appears that it does remove power.

br accepted this revision.Jan 30 2025, 8:34 AM
br added inline comments.
sys/riscv/cvitek/cvitek_restart.c
103

although this is correct, but to match style of other drivers, could we return success at the end of function unless some issue arise, i.e.

cvitek_restart_probe(device_t dev)
{

        if (!ofw_bus_status_okay(dev))
                return (ENXIO);

        if (!ofw_bus_is_compatible(dev, "cvitek,restart"))
                return (ENXIO);

         device_set_desc(dev, "Cvitek restart controller");

         return (BUS_PROBE_DEFAULT);
This revision is now accepted and ready to land.Jan 30 2025, 8:34 AM
bnovkov mentioned this in rGe07254d1483f: riscv: Add driver for the cvitek reboot controller.Aug 1 2025, 12:45 PM

Revision Contents
Changeset List

PathSize
sys/
riscv/
cvitek/
162 lines

Diff 149682

View Options

sys/riscv/cvitek/cvitek_restart.c

Loading...