ARM-software / tf-issues

Issue tracking for the ARM Trusted Firmware project
37 stars 16 forks source link

GIC v3 function gicd_set_ipriorityr funtion is buggy ! need update like above #640

Open mariusxp opened 6 years ago

mariusxp commented 6 years ago

void gicd_set_ipriorityr(unsigned int base, unsigned int id, unsigned int pri) { //read/modify/write 8 bits unsigned n = id >> IPRIORITYR_SHIFT; // select the byte within the word unsigned int bit_shift = (id & 0x3) << 3; unsigned int prior_val = GET_REG_WORD_VAL(base + GICD_IPRIORITYR + (n << 2)); prior_val &= ~(GIC_PRI_MASK << bit_shift); prior_val |= ((pri & GIC_PRI_MASK) << bit_shift); SET_REG_WORD(base + GICD_IPRIORITYR + (n << 2), prior_val); }

ghost commented 6 years ago

Why do you need to do the read, mask and write manually? The register is R/W according to ARM Generic Interrupt Controller Architecture Specification (IHI 0069C), so just writing a byte should work.

mariusxp commented 6 years ago

please read with care the specs: For interrupt ID m, when DIV and MOD are the integer division and modulo operations: • The corresponding GICD_IPRIORITYR number, n, is given by n = m DIV 4. • The offset of the required GICD_IPRIORITYR register is (0x400 + (4*n)). • The byte offset of the required Priority field in this register is m MOD 4, where: — Byte offset 0 refers to register bits [7:0]. — Byte offset 1 refers to register bits [15:8]. — Byte offset 2 refers to register bits [23:16]. — Byte offset 3 refers to register bits [31:24].

it is not so easy you pointed it

ghost commented 6 years ago

Well, but that only means that for interrupt ID m the corresponding address is GICD_IPRIORITYR + id, just explained in a really convoluted way. That explanation considers each register as a 32-bit register. If you consider them as 8-bit registers then it is just like I have explained.

soby-mathew commented 6 years ago

• The corresponding GICD_IPRIORITYR number, n, is given by n = m DIV 4. • The offset of the required GICD_IPRIORITYR register is (0x400 + (4*n)). • The byte offset of the required Priority field in this register is m MOD 4, where: — Byte offset 0 refers to register bits [7:0]. — Byte offset 1 refers to register bits [15:8]. — Byte offset 2 refers to register bits [23:16]. — Byte offset 3 refers to register bits [31:24].

Yes, its is a bitfield of 4 fields and each field is accessible as a byte. The read-modify-write sequence will not work in a multi core system unless there can be global locking scheme shared across multiple software stacks.

void gicd_set_ipriorityr(uintptr_t base, unsigned int id, unsigned int pri)
{
    uint8_t val = pri & GIC_PRI_MASK;
    mmio_write_8(base + GICD_IPRIORITYR + id, val);
}

Please let us know if you see any corruption of priority values with that implementation.