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

Attempt to make Serial_HelloWorld compatible with SDCC 4.4.0 #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jz13138
Copy link

@jz13138 jz13138 commented May 12, 2024

@spth
Copy link
Contributor

spth commented May 12, 2024

I didn't try it, but from a quick look at the diff it looks ok to me, and should do what the title claims.

Two questions:

  • What is the motivation for the PFS154->PFS173 change?
  • Do we really need those asm __set1io and __set0io? Why not just use &= and |= with suitable masks?

@cpldcpu
Copy link
Member

cpldcpu commented May 12, 2024

Do we really need those asm __set1io and __set0io? Why not just use &= and |= with suitable masks?

I believe older versions of SDCC were not able to infer the set instruction for different memory types. Is that now all fixed?

@spth
Copy link
Contributor

spth commented May 12, 2024

Compiling that Serial_HelloWorld, which has both, it looks like there is no difference:

;	../include/serial.h: 28: __set0io(PA, SERIAL_TX_PIN);                // Send 0 on TX Pin
	set0.io	__pa, #7

vs.

;	../include/serial.h: 39: INTEN &= ~INTEN_TM2;                          // Disable TM2 (setup of 16 bit value txdata is non atomic)
	set0.io	__inten, #6

Philipp

P.S.: AFAIK, SDCC does use set0 and set1 for I/O. But it can't do so for general RAM: set0 and1 are only available for a part of the RAM, and which variable will be where in RAM is typically not known before link time.

@jz13138
Copy link
Author

jz13138 commented May 13, 2024

Thanks for your feedback.

* What is the motivation for the PFS154->PFS173 change?

I am just evaluating PFS173 at the moment. No need to change that here. Sorry for that.

* Do we really need those asm `__set1io` and `__set0io`? Why not just use `&=` and `|=` with suitable masks?

I was hoping that it will run faster.

However, in a different test case I experienced that
__set1io(ADCC, ADCC_PROCESS_CONTROL_BIT);
could start the ADC conversion whereas |= didn't.

@cpldcpu
Copy link
Member

cpldcpu commented May 24, 2024

Shall we merge this? Or what are the proposed changes? Changing to PFS154 as default?

@spth
Copy link
Contributor

spth commented May 24, 2024

I'd rather just make the minimal changes necessary as a first step: Replace the mov by mov.io, etc in the inline asm.
Second step: Eliminate inline asm where it doesn't really provide an advantage. For now, I don't see much point in the rest of changes in the pull request (PFS154 default, etc).

P.S.: I've now implemented what I proposed above.

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.

3 participants