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

feat(sio): fix implemented Hardware Divider #37

Merged
merged 15 commits into from
May 24, 2021
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions src/peripherals/reset.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { BasePeripheral, Peripheral } from './peripheral';

const RESET = 0x0; //Reset control.
const WDSEL = 0x4; //Watchdog select.
const RESET_DONE = 0x8; //Reset Done

export class RPReset extends BasePeripheral implements Peripheral {
private reset: number = 0;
private wdsel: number = 0;
private reset_done: number = 0x1ffffff;

readUint32(offset: number) {
switch (offset) {
case RESET:
return this.reset;
case WDSEL:
return this.wdsel;
case RESET_DONE:
return this.reset_done;
}
return super.readUint32(offset);
}

writeUint32(offset: number, value: number) {
switch (offset) {
case RESET:
this.reset = value & 0x1ffffff;
break;
case WDSEL:
this.wdsel = value & 0x1ffffff;
break;
default:
super.writeUint32(offset, value);
break;
}
}
}
3 changes: 2 additions & 1 deletion src/rp2040.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { RP2040SysCfg } from './peripherals/syscfg';
import { RPTimer } from './peripherals/timer';
import { RPUART } from './peripherals/uart';
import { RPSIO } from './sio';
import { RPReset } from './peripherals/reset';

export const FLASH_START_ADDRESS = 0x10000000;
export const FLASH_END_ADDRESS = 0x14000000;
Expand Down Expand Up @@ -191,7 +192,7 @@ export class RP2040 {
0x40000: new UnimplementedPeripheral(this, 'SYSINFO_BASE'),
0x40004: new RP2040SysCfg(this, 'SYSCFG'),
0x40008: new UnimplementedPeripheral(this, 'CLOCKS_BASE'),
0x4000c: new UnimplementedPeripheral(this, 'RESETS_BASE'),
0x4000c: new RPReset(this, 'RESETS_BASE'),
0x40010: new UnimplementedPeripheral(this, 'PSM_BASE'),
0x40014: new UnimplementedPeripheral(this, 'IO_BANK0_BASE'),
0x40018: new UnimplementedPeripheral(this, 'IO_QSPI_BASE'),
Expand Down
82 changes: 82 additions & 0 deletions src/sio.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
import { RP2040, SIO_START_ADDRESS } from './rp2040';

import { GDBClient } from './utils/gdbclient';
import { ICortexTestDriver } from './utils/test-driver';
import { GDBTestDriver } from './utils/test-driver-gdb';
import { RP2040TestDriver } from './utils/test-driver-rp2040';

//HARDWARE DIVIDER
const SIO_DIV_UDIVIDEND = SIO_START_ADDRESS + 0x060; // Divider unsigned dividend
const SIO_DIV_UDIVISOR = SIO_START_ADDRESS + 0x064; // Divider unsigned divisor
const SIO_DIV_SDIVIDEND = SIO_START_ADDRESS + 0x068; // Divider signed dividend
const SIO_DIV_SDIVISOR = SIO_START_ADDRESS + 0x06c; // Divider signed divisor
const SIO_DIV_QUOTIENT = SIO_START_ADDRESS + 0x070; // Divider result quotient
const SIO_DIV_REMAINDER = SIO_START_ADDRESS + 0x074; //Divider result remainder
const SIO_DIV_CSR = SIO_START_ADDRESS + 0x078;

describe('RPSIO', () => {
let cpu: ICortexTestDriver;

beforeEach(async () => {
// if (process.env.TEST_GDB_SERVER) {
if (false) {
const client = new GDBClient();
await client.connect('127.0.0.1');
cpu = new GDBTestDriver(client);
await cpu.init();
} else {
cpu = new RP2040TestDriver(new RP2040());
}
});

afterEach(async () => {
await cpu.tearDown();
});

describe('Hardware Divider', () => {
it('should set perform a signed hardware divider 123456 / -321 = -384 R192', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think "REM 192" will be clearer than "R192" (same for the other test names).

Also, the word "set" (second word) seems not related here?

await cpu.writeUint32(SIO_DIV_SDIVIDEND, 123456);
expect(await cpu.readUint32(SIO_DIV_SDIVIDEND)).toEqual(123456);
await cpu.writeUint32(SIO_DIV_SDIVISOR, -321);
expect(await cpu.readUint32(SIO_DIV_CSR)).toEqual(3);
expect((await cpu.readUint32(SIO_DIV_SDIVISOR)) | 0).toEqual(-321);
expect((await cpu.readUint32(SIO_DIV_REMAINDER)) | 0).toEqual(192);
expect((await cpu.readUint32(SIO_DIV_QUOTIENT)) | 0).toEqual(-384);
expect(await cpu.readUint32(SIO_DIV_CSR)).toEqual(1);
});

it('should set perform an unsigned hardware divider 123456 / 321 = 384 R192', async () => {
await cpu.writeUint32(SIO_DIV_UDIVIDEND, 123456);
await cpu.writeUint32(SIO_DIV_UDIVISOR, 321);
expect(await cpu.readUint32(SIO_DIV_CSR)).toEqual(3);
expect(await cpu.readUint32(SIO_DIV_REMAINDER)).toEqual(192);
expect(await cpu.readUint32(SIO_DIV_QUOTIENT)).toEqual(384);
expect(await cpu.readUint32(SIO_DIV_CSR)).toEqual(1);
});

it('should perform a division, store the result, do another division then restore the previously stored result ', async () => {
await cpu.writeUint32(SIO_DIV_SDIVIDEND, 123456);
await cpu.writeUint32(SIO_DIV_SDIVISOR, -321);
const remainder = (await cpu.readUint32(SIO_DIV_REMAINDER)) | 0;
const quotient = (await cpu.readUint32(SIO_DIV_QUOTIENT)) | 0;
expect(remainder).toEqual(192);
expect(quotient).toEqual(-384);
await cpu.writeUint32(SIO_DIV_UDIVIDEND, 123);
await cpu.writeUint32(SIO_DIV_UDIVISOR, 7);
expect(await cpu.readUint32(SIO_DIV_REMAINDER)).toEqual(4);
expect(await cpu.readUint32(SIO_DIV_QUOTIENT)).toEqual(17);
await cpu.writeUint32(SIO_DIV_REMAINDER, remainder);
await cpu.writeUint32(SIO_DIV_QUOTIENT, quotient);
expect(await cpu.readUint32(SIO_DIV_CSR)).toEqual(3);
expect((await cpu.readUint32(SIO_DIV_REMAINDER)) | 0).toEqual(192);
expect((await cpu.readUint32(SIO_DIV_QUOTIENT)) | 0).toEqual(-384);
});

it('should set perform an unsigned division by zero 123456 / 0 = Infinity RNaN', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The result isn't Infinity / NaN (as the test name says), rather 0xffffffff and remainder 123456.

await cpu.writeUint32(SIO_DIV_UDIVIDEND, 123456);
await cpu.writeUint32(SIO_DIV_UDIVISOR, 0);
expect(await cpu.readUint32(SIO_DIV_REMAINDER)).toEqual(123456);
expect(await cpu.readUint32(SIO_DIV_QUOTIENT)).toEqual(0xffffffff);
});
});
});
79 changes: 79 additions & 0 deletions src/sio.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,25 @@ const GPIO_HI_OE_XOR = 0x04c; // QSPI output enable XOR

const GPIO_MASK = 0x3fffffff;

//HARDWARE DIVIDER
const DIV_UDIVIDEND = 0x060; // Divider unsigned dividend
const DIV_UDIVISOR = 0x064; // Divider unsigned divisor
const DIV_SDIVIDEND = 0x068; // Divider signed dividend
const DIV_SDIVISOR = 0x06c; // Divider signed divisor
const DIV_QUOTIENT = 0x070; // Divider result quotient
const DIV_REMAINDER = 0x074; //Divider result remainder
const DIV_CSR = 0x078;

export class RPSIO {
gpioValue = 0;
gpioOutputEnable = 0;
qspiGpioValue = 0;
qspiGpioOutputEnable = 0;
divDividend = 0;
divDivisor = 1;
divQuotient = 0;
divRemainder = 0;
divCSR = 0;

constructor(private readonly rp2040: RP2040) {}

Expand Down Expand Up @@ -62,6 +76,21 @@ export class RPSIO {
case CPUID:
// Returns the current CPU core id (always 0 for now)
return 0;
case DIV_UDIVIDEND:
return this.divDividend >>> 0;
case DIV_SDIVIDEND:
return this.divDividend | 0;
case DIV_UDIVISOR:
return this.divDivisor >>> 0;
case DIV_SDIVISOR:
return this.divDivisor | 0;
case DIV_QUOTIENT:
this.divCSR &= ~0b10;
return this.divQuotient;
case DIV_REMAINDER:
return this.divRemainder;
case DIV_CSR:
return this.divCSR;
}
console.warn(`Read from invalid SIO address: ${offset.toString(16)}`);
return 0xffffffff;
Expand Down Expand Up @@ -117,6 +146,56 @@ export class RPSIO {
case GPIO_HI_OE_XOR:
this.qspiGpioOutputEnable ^= value & GPIO_MASK;
break;
case DIV_UDIVIDEND:
this.divDividend = value >>> 0;
this.divCSR = 0b10;
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the reason for updating divCSR twice in the same switch case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was waiting for You on this...
well the datasheet states that the CSR lsb is cleared at the beginning of the calculation and set once result is ready (after 8 cpu cycles).
On a single core this assignment does not make any sense, since I don't know how multicore will be implemented it could be something that will matter in case of concurrency against hardware divider.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point. When we get to multicore, we'll just execute one instruction at a time (so jumping back and forth between running core0 and core1 code).

In any case, JavaScript does not support multi-threading natively. The only way is using Web Workers (or worker threads on Node.js). But even then, the only way two threads can access the same memory is using a SharedArrayBuffer, so if we decide at some point to truly run this on two threads (and share the divider memory between them) we'll have to change all these variables to be stored inside a SharedArrayBuffer and refactor the the code, and I don't see it happening any time soon.

So to summarize, I think we can safely remove the this.divCSR = 0b10;

Choose a reason for hiding this comment

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

note that SIO dividers/interpreters are core local

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point to remember when we implemented multi-core. I noted this!

this.divQuotient = Math.trunc(this.divDividend / this.divDivisor) >>> 0;
this.divRemainder = Math.trunc(this.divDividend % this.divDivisor) >>> 0;
this.divCSR = 0b11;
this.rp2040.cycles += 8;
break;
case DIV_SDIVIDEND:
this.divDividend = value | 0;
this.divCSR = 0b10;
this.divQuotient = Math.trunc(this.divDividend / this.divDivisor) | 0;
this.divRemainder = Math.trunc(this.divDividend % this.divDivisor) | 0;
this.divCSR = 0b11;
this.rp2040.cycles += 8;
break;
case DIV_UDIVISOR:
this.divDivisor = value >>> 0;
this.divCSR = 0b10;
if (this.divDivisor == 0) {
this.divQuotient = 0xffffffff;
this.divRemainder = this.divDividend;
} else {
this.divQuotient = Math.trunc(this.divDividend / this.divDivisor) >>> 0;
this.divRemainder = Math.trunc(this.divDividend % this.divDivisor) >>> 0;
}
this.divCSR = 0b11;
this.rp2040.cycles += 8;
break;
case DIV_SDIVISOR:
this.divCSR = 0b10;
this.divDivisor = value | 0;
if (this.divDivisor == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest that we move this repetitive code (in DIV_UDIVIDEND, DIV_SDIVIDEND, DIV_UDIVISOR, and DIV_SDIVISOR) into a function that will do the actual division, e.g. this.updateDivider().

As far as I can tell, you don't need to worry about the sign of the result, because when it's read back into a register it'll always be internally converted to unsigned anyway.

this.divQuotient = 0xffffffff;
this.divRemainder = this.divDividend;
} else {
this.divQuotient = Math.trunc(this.divDividend / this.divDivisor) | 0;
this.divRemainder = Math.trunc(this.divDividend % this.divDivisor) | 0;
}
this.divCSR = 0b11;
this.rp2040.cycles += 8;
break;
case DIV_QUOTIENT:
this.divQuotient = value;
this.divCSR = 0b11;
break;
case DIV_REMAINDER:
this.divRemainder = value;
this.divCSR = 0b11;
break;
}
}
}