On 16/06/26 06:40PM, Jonathan Cameron wrote:
On Tue, 9 Jun 2026 16:28:33 +0530
Shrihari E S <[email protected]> wrote:
Plumb the 'uio_capable' flag to CXL HDM decoder capability and
control register interfaces. The UIO bit in the capability register
is now set for CXL Type3 devices and ports when UIO support is
advertised via 'x-uio-svc' property.
Per CXL 4.0 specification Section 8.2.4.20.7, the decoder control
UIO bit is validated against the advertised capability during
HDM decoder commit operations.
Additionally, replace hardcoded HDM decoder control write masks
with spec-aligned 'WRMASK' definitions.
I think we should be modifying that depending on whether it is uio_capable
rather than saying it is 'writeable' but 'not writetable' if not UIO capable.
A few other comments inline.
Hi Jonathan,
Got your point, will change that in the next version of the patch.
Thanks,
Signed-off-by: Shrihari E S <[email protected]>
Signed-off-by: Dongjoo Seo <[email protected]>
---
+/* CXL 4.0 specification 8.4.2.20.7 */
+#define CXL_HDM_DECODER_CTRL_WRMASK ( \
+ (0xfu << 0) | /* IG */ \
+ (0xfu << 4) | /* IW */ \
+ BIT(8) | /* LOCK_ON_COMMIT */ \
+ BIT(9) | /* COMMIT */ \
+ BIT(12) | /* TYPE */ \
+ BIT(13) | /* BI */ \
+ BIT(14) | /* UIO */ \
+ (0xfu << 16) | /* UIG */ \
+ (0xfu << 20) | /* UIW */ \
+ (0xfu << 24)) /* ISP */
Given the FIELD() macro defines appropriate masks, can we build this as
R_CXL_HDM_DECODER_0_CTRL_IG_MASK |
R_CXL_HDM_DECODER_0_CTRL_IW_MASK |
etc
That will make it both self documenting and ensure it aligns with the field
defines. If we get a bug at least it will only be in one place :)
Sure, will add those Fields instead of hardcoding.
+
/* CXL r3.1 Section 8.2.4.20.1 CXL HDM Decoder Capability Register */
int cxl_decoder_count_enc(int count)
{
@@ -305,7 +318,7 @@ static void ras_init_common(uint32_t *reg_state, uint32_t
*write_msk)
}
static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
- enum reg_type type, bool bi)
+ enum reg_type type, bool bi, bool uio)
{
int decoder_count = CXL_HDM_DECODER_COUNT;
int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
@@ -325,9 +338,12 @@ static void hdm_init_common(uint32_t *reg_state, uint32_t
*write_msk,
ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, 3_6_12_WAY, 0);
ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, 16_WAY, 0);
}
- ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, UIO, 0);
+ ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, UIO,
+ (type == CXL2_TYPE3_DEVICE || CXL2_UPSTREAM_PORT) && uio);
ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
- UIO_DECODER_COUNT, 0);
+ UIO_DECODER_COUNT,
+ (type == CXL2_TYPE3_DEVICE || CXL2_UPSTREAM_PORT) && uio ?
+ decoder_count : 0);
ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, MEMDATA_NXM_CAP,
0);
ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
SUPPORTED_COHERENCY_MODEL,
@@ -341,7 +357,8 @@ static void hdm_init_common(uint32_t *reg_state, uint32_t
*write_msk,
write_msk[R_CXL_HDM_DECODER0_BASE_HI + i * hdm_inc] = 0xffffffff;
write_msk[R_CXL_HDM_DECODER0_SIZE_LO + i * hdm_inc] = 0xf0000000;
write_msk[R_CXL_HDM_DECODER0_SIZE_HI + i * hdm_inc] = 0xffffffff;
- write_msk[R_CXL_HDM_DECODER0_CTRL + i * hdm_inc] = 0x13ff;
+ write_msk[R_CXL_HDM_DECODER0_CTRL + i * hdm_inc] =
+ CXL_HDM_DECODER_CTRL_WRMASK;
I think file at least mostly aligns these long line wraps as
write_msk[R_CXL_HDM_DECODER0_CTRL + i * hdm_inc] =
CXL_HDM_DECODER_CTRL_WRMASK;
Sure, will rectify it.
Given i think this needs to be dependent on whether the device is uio_capable
maybe just bring the mask defines suggested above down here, and modify the
UIO bit to fit whether it should be writeable.
Yeah, got it. Will check the UIO capability first and apply the mask. Thanks for
pointing it out.
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index b7ad437cbc..4cdadc3e10 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -590,6 +590,11 @@ static void hdm_decoder_commit(CXLType3Dev *ct3d, int
which)
/* TODO: Sanity checks that the decoder is possible */
ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0);
ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
+ if (ct3d->uio_capable) {
+ ct3d->uio_enabled = FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, UIO);
+ } else {
+ ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, UIO, 0);
If it's not supported, shouldn't the write mask stop it being written? I think
that would be cleaner than overriding any write to 0 here.
yes, understood. Will do that in the next version of the patch.
