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

Add support for Transmeta(TM) Crusoe(TM) CPU #238

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

Conversation

danielinux
Copy link

I am aware of contributing guidelines 1, but I thought this might be helpful if you don't have a Crusoe at hand.

Initial support for Transmeta Crusoe CPU. Tested on TM5800 @ 1GHz.
Still trying to figure out how to read cache topology/set peak performance, but at least now my CPU is detected.

Upstream:
cpufetch-upstream

With this PR:
cpufetch-tm

neofetch, for reference:
neofetch

Copy link

github-actions bot commented Apr 5, 2024

cpufetch does not accept pull requests, see the contributing guidelines for details

Copy link
Owner

@Dr-Noob Dr-Noob left a comment

Choose a reason for hiding this comment

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

Very quick review, I'll have a look at the peak performance issue later.

src/common/ascii.h Show resolved Hide resolved
src/common/cpu.c Outdated Show resolved Hide resolved
src/common/cpu.h Show resolved Hide resolved
src/common/cpu.h Show resolved Hide resolved
src/common/cpu.h Show resolved Hide resolved
bool SHA1;
bool SHA2;
bool CRC32;
#endif
Copy link
Owner

Choose a reason for hiding this comment

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

nit: Remove space

Copy link
Author

Choose a reason for hiding this comment

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

about these 4 nit: remove space comments in cpu.h: My editor removed the existing trailing spaces. I can revert those if you prefer to address them separately,

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, yes, that would be nice. I want to give this a format pass sometime soon.

src/x86/cpuid.c Outdated Show resolved Hide resolved
@@ -692,6 +695,7 @@ struct topology* get_topology_info(struct cpuInfo* cpu, struct cache* cach, int

switch(cpu->cpu_vendor) {
case CPU_VENDOR_INTEL:
case CPU_VENDOR_TRANSMETA:
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure of this. From what I recall, get_topology_from_apic is Intel only. It seems like it is getting the right number of cores but this is probably coming from get_topology_from_udev. Can you confirm? Just run with --verbose and it will show us how is it getting this information.

Copy link
Author

Choose a reason for hiding this comment

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

You are right. Any help with finding out how to retrieve topology is appreciated. I believe we could also assume cores=1 on this platform.

[WARNING]: Can't read features information from cpuid (needed level is 0x00000007, max is 0x00000001)
[WARNING]: Can't read frequency information from cpuid (needed level is 0x00000016, max is 0x00000001). Using udev
[WARNING]: Can't read topology information from cpuid (needed level is 0x00000001, max is 0x00000001)

@@ -767,6 +771,32 @@ struct topology* get_topology_info(struct cpuInfo* cpu, struct cache* cach, int
return topo;
}

struct cache* get_cache_info_transmeta(struct cache* cach) {
Copy link
Owner

Choose a reason for hiding this comment

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

Why can't we use get_cache_info_general. It does not work?

Copy link
Author

Choose a reason for hiding this comment

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

It failed to retrieve the cache size with that. I have eventually hardcoded the cache size to this CPU model.

src/x86/uarch.c Outdated Show resolved Hide resolved
@Dr-Noob
Copy link
Owner

Dr-Noob commented Jul 3, 2024

I don't see why you get 0 MFLOP/s in peak performance. You will have to go to get_peak_performance in src/x86/cpuid.c and debug what is going on. I would suggest printing the variable flops just after doing:

int64_t flops = topo->physical_cores * topo->sockets * (freq*1000000) * vpus;

I suspect some of those values might be zero.

@danielinux
Copy link
Author

Hey @Dr-Noob thanks a lot for the detailed review. Glad to see this is being considered. I'll be addressing your comments ASAP.

@danielinux
Copy link
Author

@Dr-Noob I've fixed the style/dead code comments. About retreiving topology and cache information, I'm not sure if you want to provide some guidance for me to move ahead. I might be able to provide you temporary access to the machine if you think this might be quicker. :)

@danielinux
Copy link
Author

also, let me know if you want me to rebase this on top of latest master

@Dr-Noob
Copy link
Owner

Dr-Noob commented Jul 7, 2024

@Dr-Noob I've fixed the style/dead code comments. About retreiving topology and cache information, I'm not sure if you want to provide some guidance for me to move ahead. I might be able to provide you temporary access to the machine if you think this might be quicker. :)

Thank you for the feedback and the offer, but for now I think we should be good if you just do it in your machine 👍

My general guidance is to use cpuid whenever possible. This pdf should be used as a reference. You can also look at the cpufetch documentation (and more specifically, x86 docs) which explains how cpuid is used for various detections in both Intel and AMD. You can have this as an example and to understand how it is done in general and then have a look at the transmeta pdf to see the particular details of this vendor.

@Dr-Noob
Copy link
Owner

Dr-Noob commented Jul 7, 2024

also, let me know if you want me to rebase this on top of latest master

I might commit some additional stuff in the upcoming days, so you could rebase just after this PR is ready to be merged.

@danielinux
Copy link
Author

thanks for your patience. I've been a few days AFK, I'll resume on this during the weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants