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

Code Review to add semifive blocks in freedom-metal-next #366

Open
wants to merge 2 commits into
base: freedom-metal-next
Choose a base branch
from

Conversation

aidenschoi
Copy link

Issue Description
This PR is for not to merge into sifive/freedom-metal. It is just for code review.
In sesame project, it needs to add a new custom IP driver code. I followed making rules of sifive-blocks to add semifive-blocks for a new custom IP driver.
Please check whether it is proper and which I missed something in this code.

semifive-blocks
core.dts has the following custom devices.

		L49: synopsys_i2c@e0000000 {
			clocks = <&L39>;
			compatible = "synopsys_i2c_v2_02a_standard";
			interrupt-parent = <&L3>;
			interrupts = <47 48 49 50 51 52 53 54 55 56 57 58>;
			reg = <0x0 0xe0000000 0x0 0x1000>;
			reg-names = "control";
		};
		L50: synopsys_i2c@e0001000 {
			clocks = <&L39>;
			compatible = "synopsys_i2c_v2_02a_standard";
			interrupt-parent = <&L3>;
			interrupts = <59 60 61 62 63 64 65 66 67 68 69 70>;
			reg = <0x0 0xe0001000 0x0 0x1000>;
			reg-names = "control";
		};

To support Synopsys I2C, semifive-blocks is composed as follows.

freedom-metal/semifive-blocks/
├── metal
│   └── drivers
│       └── i2c-designware.h
├── src
│   └── drivers
│       └── i2c-designware.c
└── templates
    ├── MANIFEST.ini
    └── metal
        ├── platform
        │   └── metal_platform_synopsys_i2c_v2_02a_standard.h.j2
        └── private
            └── metal_private_synopsys_i2c_v2_02a_stadard.h.j2

i2c-designware.c and header files are not implemented yet. I added them to show how to make the software structure for new custom IP. Now they are empty. I'll fill a driver code for Synopsys i2c in them.
We will add more custom IP drivers later in semifive-blocks as an above frame.

Test
It makes metal_platform/private_synopsys_i2c_v2_02a_stadard.h files well by built with template files.

@aidenschoi
Copy link
Author

@keith-packard @nategraff-sifive Can you please review this PR?

Copy link
Contributor

@keith-packard keith-packard left a comment

Choose a reason for hiding this comment

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

It looks like you've identified a bug in the templates for interrupt_handlers.[ch]. Can you explain the problem and split those changes out into a separate patch?

@aidenschoi
Copy link
Author

Hi Keith,

There are 2 commits.
The first one is to fix duplicated interrupt handler names issue in jinja2. I requested code review to Nate before. But I didn't get feedback about this yet.
And 2nd one is for adding semifive-blocks for new custom IP driver.
The above 2 commits are not for a merge to freedom-metal-next, just for code review. So I think it doesn't need to separate.
Do you have any concern about software structure for semifive-block of 2nd one?

Thanks,
Aiden.

@keith-packard
Copy link
Contributor

Nate has left SiFive, so it's falling on me to go back over these issues and figure out what's going on. I'll take a look and see if I can understand what the underlying issue is.

Copy link
Contributor

@keith-packard keith-packard left a comment

Choose a reason for hiding this comment

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

Ok, this looks reasonable. Can you provide a DTS which causes a problem so I can review that along with your solution?

@aidenschoi
Copy link
Author

I don't know how to share file with you by PR in github. I'll send dts file by email to you. Please check that.

@e-puerto e-puerto added the DNM Do Not Merge label Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DNM Do Not Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants