Gaurav Kashyap <quic_gaurkash@xxxxxxxxxxx> writes: ... > + /* > + * When ICE is in standard (hwkm) mode, it supports HW wrapped > + * keys, and when it is in legacy mode, it only supports standard > + * (non HW wrapped) keys. > + * > + * Put ICE in standard mode, ICE defaults to legacy mode. > + * Legacy mode - ICE HWKM slave not supported. > + * Standard mode - ICE HWKM slave supported. > + * > + * Depending on the version of HWKM, it is controlled by different > + * registers in ICE. > + */ > + if (ice->hwkm_version >= 2) { > + val = qcom_ice_readl(ice, QCOM_ICE_REG_CONTROL); > + val = val & 0xFFFFFFFE; >From the code I understand that the last bit is used for setting the mode. Was wondering if it would make more sense to use ~BIT(0) or GENMASK to generate this value and #define this value to express the work it does. In this case, something like #define xx_SET_MODE_MASK (~BIT(0)) or use GENMASK This would make it easier for a person who is taking a look at the code for first time. This is just my perspective. If you do agree, you can change them at multiple places accross this patch, wherever magic numbers are used for val and masking the value. Regards, Kamlesh > + qcom_ice_writel(ice, val, QCOM_ICE_REG_CONTROL); > + } else { > + qcom_ice_writel(ice, 0x7, HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_CTL)); > + } > +} > + > +static void qcom_ice_hwkm_init(struct qcom_ice *ice) > +{ > + /* Disable CRC checks. This HWKM feature is not used. */ > + qcom_ice_writel(ice, 0x6, > + HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_CTL)); > + > + /* > + * Give register bank of the HWKM slave access to read and modify > + * the keyslots in ICE HWKM slave. Without this, trustzone will not > + * be able to program keys into ICE. > + */ > + qcom_ice_writel(ice, 0xFFFFFFFF, HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_0)); > + qcom_ice_writel(ice, 0xFFFFFFFF, HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_1)); > + qcom_ice_writel(ice, 0xFFFFFFFF, HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_2)); > + qcom_ice_writel(ice, 0xFFFFFFFF, HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_3)); > + qcom_ice_writel(ice, 0xFFFFFFFF, HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_4)); > + > + /* Clear HWKM response FIFO before doing anything */ > + qcom_ice_writel(ice, 0x8, HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BANKN_IRQ_STATUS)); > + ice->hwkm_init_complete = true; > +} ...