Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kernel/aarch64: Get RPi::InterruptController address and RPi::Timer address + IRQ number from the devicetree #25500

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

spholz
Copy link
Member

@spholz spholz commented Nov 27, 2024

This PR removes 2 hard-coded address offsets and 1 hard-coded interrupt number from the aarch64 code!

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Nov 27, 2024
@spholz spholz force-pushed the devicetree-mmio branch 2 times, most recently from 902f738 to 751568f Compare November 27, 2024 21:30
}

private:
Vector<u8, 4 * sizeof(u32)> m_raw;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FixedArray should fit a bit better here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use Vectors because FixedArrays don't have an inline capacity.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then maybe ByteBuffer?
Vector feels wrong, but works I guess

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I would have to use Detail::ByteBuffer. ::ByteBuffer is just an alias for Detail::ByteBuffer<32>.
An inline capacity of 32 seems wasteful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could move ByteBuffer out of Detail and change it to have a default value (Ali suggested that in the PR that introduced the current ByteBuffer as well: #7151 (comment))

I'm not sure if this would cause any problems. But we would have to replace all current occurrences of ByteBuffer with ByteBuffer<>, which probably would be annoying for cherry-picking ladybird commits I think (@nico)

}

private:
Vector<u8, 2 * sizeof(u32)> m_raw;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

@spholz spholz force-pushed the devicetree-mmio branch 2 times, most recently from 42b473d to a323bbf Compare November 28, 2024 18:18
Copy link
Contributor

@nico nico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't looked at everything yet, but here's a comment asking for how the dtb file was made:


TEST_CASE(address_translation)
{
auto fdt_file = TRY_OR_FAIL(Core::File::open("/usr/Tests/LibDeviceTree/address-translation.dtb"sv, Core::File::OpenMode::Read));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you say in the commit message how you created this .dtb file? (Ideally the command line invocation.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I manually wrote a dts file and compiled it with dtc -o address-translation.dtb address-translation.dts.
I could add the source file as well, but I wasn't sure if I should do that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also could just depend on dtc for building serenity and compile it from source, but I don't think we want more build dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just put the dtc in to the commit message

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like this?

Userland/Libraries/LibDeviceTree/DeviceTree.cpp Outdated Show resolved Hide resolved
This (confusingly named) function will probably never be used, since you
normally should know whether you are looking for a child or property.
This will be necessary in the next commit to avoid a name conflict.
The struct is in the DeviceTree namespace, so the prefix is redundant.
The class is in the DeviceTree namespace, so the prefix is redundant.
Calling it a 'View' is also somewhat misleading, as it is not simply a
cheap-to-copy reference to the underlying memory but is instead marked
as AK_MAKE_NONCOPYABLE.
@spholz spholz force-pushed the devicetree-mmio branch 2 times, most recently from 92de3d7 to 280a519 Compare November 29, 2024 19:25
I created this test file by running the following command:

    dtc -o address-translation.dtb <<EOF
    /dts-v1/;

    / {
        compatible = "serenity,address-translation-test";
        #address-cells = <2>;
        #size-cells = <1>;

        soc {
            compatible = "simple-bus";
            #address-cells = <1>;
            #size-cells = <1>;
            ranges = <0xa0000000 0xfe 0xd0000000 0x40000000>;

            usb@a0010000 {
                reg = <0xa0010000 0x100000>;
            };

            some-bus@b0000000 {
                compatible = "simple-bus";
                #address-cells = <2>;
                #size-cells = <2>;
                ranges = <0x02 0x00 0xb0000000 0x00 0x200000>;

                leds@200100000 {
                    reg = <0x02 0x100000 0x00 0x1000>;
                };
            };
        };
    };
    EOF
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 pr-needs-review PR needs review from a maintainer or community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants