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

DEV9: various fixes for ATA Identify #10514

Merged
merged 1 commit into from
Jan 9, 2024
Merged

Conversation

AKuHAK
Copy link
Contributor

@AKuHAK AKuHAK commented Dec 30, 2023

UDMA mode now set correctly
MDMA0 mode now set correctly
fixed user addressable sectors limit

Description of Changes

In continuation for #10427 as hdd size still wasnt set properly everywhere.

Rationale behind Changes

Fix critical values in ATA Identify response.

Suggested Testing Steps

Can be tested with https://github.com/ps2homebrew/PS2HDDTester/releases . After changes it can properly run UDMA tests and reports correct disk size for large disks:
hddtester  ? _hddtester_20231230162102

Below attached ATA Identify response from one of the Sony original PS2 drives for the research purposes.
HDD_IDentify.bin.zip

@github-actions github-actions bot added the DEV9 label Dec 30, 2023
@AKuHAK
Copy link
Contributor Author

AKuHAK commented Dec 30, 2023

TBH whole HDDInfo should be rewritten in accordance to the provided actual hdd response and in accordance to the ATA-7 specs:
d1532v1r4b-ATA-ATAPI-7-1.pdf.zip
My vision is that all Obsolete and Retired values does not need any calculation or explanation and can be just filled with fixed values without attempts to calculate them. This will simplify the code readability a lot too. I can do that, but I am not sure if team will accept that (@TheLastRar ? )

pcsx2/DEV9/ATA/ATA_Info.cpp Outdated Show resolved Hide resolved
pcsx2/DEV9/ATA/ATA_Info.cpp Outdated Show resolved Hide resolved
@TheLastRar
Copy link
Contributor

TBH whole HDDInfo should be rewritten in accordance to the provided actual hdd response and in accordance to the ATA-7 specs: d1532v1r4b-ATA-ATAPI-7-1.pdf.zip My vision is that all Obsolete and Retired values does not need any calculation or explanation and can be just filled with fixed values without attempts to calculate them. This will simplify the code readability a lot too. I can do that, but I am not sure if team will accept that (@TheLastRar ? )

Looking at your provided HDD Identity dump, the original HDD reports more obsolete/retired fields then we do.

As for providing fixed values. you can where it makes sense to (i.e. fixed values for default CHS fields as we are always >8GB)

As far as I can see, we aren't missing anything required for ATAPI-7, And comparing our Identity to the dump, I don't think we aren't too far off that, The most significant thing to me seems to be feature support.

@AKuHAK
Copy link
Contributor Author

AKuHAK commented Jan 1, 2024

As far as I can see, we aren't missing anything required for ATAPI-7

Yes, currently pcsx2 correctly emulates everything (just MDMA modes doesnt work properly, cant figure out why). What I would like to say that the codebase is overcomplicated. CHS and sdma modes are retired for example.

Looking at your provided HDD Identity dump, the original HDD reports more obsolete/retired fields then we do.

Yes, because HDD itself is manufactured with ATAPI-6 standard (bit 6 in word 80), while ps2 itself supports ATAPI-7.

@AKuHAK AKuHAK marked this pull request as ready for review January 1, 2024 20:50
@TheLastRar
Copy link
Contributor

TheLastRar commented Jan 2, 2024

Yes, currently pcsx2 correctly emulates everything (just MDMA modes doesnt work properly, cant figure out why).

Found the issue, I can address that in a PR once this has been merged. I'll also remove SDMA support as original hardware lacks it

What I would like to say that the codebase is overcomplicated. CHS and sdma modes are retired for example.

I somewhat doubt that having CHS and SDMA support contribute much to code complexity here

I am hesitant to remove things that original hardware support, however, we don't implement read/writes in CHS mode and I've yet to see an issue report about anything needing it.

If you are dead set on removing CHS support then you can propose that as a separate standalone pr

@AKuHAK AKuHAK marked this pull request as draft January 3, 2024 09:47
@AKuHAK AKuHAK force-pushed the patch-1 branch 4 times, most recently from f4d0208 to 8307edd Compare January 3, 2024 20:10
@AKuHAK
Copy link
Contributor Author

AKuHAK commented Jan 3, 2024

Found the issue, I can address that in a PR once this has been merged. I'll also remove SDMA support as original hardware lacks it

Nice

@AKuHAK AKuHAK marked this pull request as ready for review January 3, 2024 20:14
@AKuHAK AKuHAK marked this pull request as draft January 4, 2024 07:08
@AKuHAK AKuHAK marked this pull request as ready for review January 5, 2024 07:56
@AKuHAK AKuHAK marked this pull request as draft January 5, 2024 21:56
@AKuHAK AKuHAK force-pushed the patch-1 branch 2 times, most recently from 72a3f94 to c05d10f Compare January 6, 2024 13:11
@AKuHAK AKuHAK marked this pull request as ready for review January 6, 2024 13:11
@AKuHAK AKuHAK marked this pull request as draft January 6, 2024 13:42
@AKuHAK AKuHAK marked this pull request as ready for review January 6, 2024 14:07
@AKuHAK AKuHAK marked this pull request as draft January 7, 2024 09:40
@AKuHAK AKuHAK marked this pull request as ready for review January 7, 2024 09:40
@AKuHAK AKuHAK marked this pull request as draft January 7, 2024 19:20
improved 48-bit addressing
UDMA mode now set correctly
MDMA0 mode now set correctly
fixed user addressable sectors limit
@AKuHAK AKuHAK marked this pull request as ready for review January 7, 2024 19:28
Copy link
Contributor

@TheLastRar TheLastRar left a comment

Choose a reason for hiding this comment

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

Noted Codacy issues are pre-existing

@refractionpcsx2 refractionpcsx2 merged commit 1a1fdf6 into PCSX2:master Jan 9, 2024
11 of 12 checks passed
@refractionpcsx2
Copy link
Member

Nice work folks

@AKuHAK AKuHAK deleted the patch-1 branch January 10, 2024 08:03
@TheLastRar TheLastRar mentioned this pull request Jan 10, 2024
This was referenced Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants