| Message ID | 20240212174822.77734-7-christophe.kerello@foss.st.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add MP25 FMC2 support | expand |
On 12/02/2024 18:48, Christophe Kerello wrote: > The FMC2 revision 2 supports security and isolation compliant with > the Resource Isolation Framework (RIF). From RIF point of view, > the FMC2 is composed of several independent resources, listed below, > which can be assigned to different security and compartment domains: > - 0: Common FMC_CFGR register. > - 1: EBI controller for Chip Select 1. > - 2: EBI controller for Chip Select 2. > - 3: EBI controller for Chip Select 3. > - 4: EBI controller for Chip Select 4. > - 5: NAND controller. > > regmap_update_bits(ebi->regmap, reg, mask, setup ? mask : 0); > > return 0; > @@ -990,6 +1023,107 @@ static const struct stm32_fmc2_prop stm32_fmc2_child_props[] = { > }, > }; > > +static int stm32_fmc2_ebi_check_rif(struct stm32_fmc2_ebi *ebi, u32 resource) > +{ > + u32 seccfgr, cidcfgr, semcr; > + int cid; > + > + if (ebi->majrev < FMC2_VERR_MAJREV_2) > + return 0; > + > + if (resource >= FMC2_MAX_RESOURCES) > + return -EINVAL; > + > + regmap_read(ebi->regmap, FMC2_SECCFGR, &seccfgr); No checking of read value? > + if (seccfgr & BIT(resource)) { Then on read failure this is random stack junk. > + if (resource) > + dev_err(ebi->dev, "resource %d is configured as secure\n", > + resource); > + > + return -EACCES; > + } > + > + regmap_read(ebi->regmap, FMC2_CIDCFGR(resource), &cidcfgr); > + if (!(cidcfgr & FMC2_CIDCFGR_CFEN)) > + /* CID filtering is turned off: access granted */ > + return 0; > + > + if (!(cidcfgr & FMC2_CIDCFGR_SEMEN)) { > + /* Static CID mode */ > + cid = FIELD_GET(FMC2_CIDCFGR_SCID, cidcfgr); > + if (cid != FMC2_CID1) { > + if (resource) > + dev_err(ebi->dev, "static CID%d set for resource %d\n", > + cid, resource); > + > + return -EACCES; > + } > + > + return 0; > + } > + > + /* Pass-list with semaphore mode */ > + if (!(cidcfgr & FMC2_CIDCFGR_SEMWLC1)) { > + if (resource) > + dev_err(ebi->dev, "CID1 is block-listed for resource %d\n", > + resource); > + > + return -EACCES; > + } > + > + regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr); > + if (!(semcr & FMC2_SEMCR_SEM_MUTEX)) { > + regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource), > + FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX); > + regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr); > + } > + > + cid = FIELD_GET(FMC2_SEMCR_SEMCID, semcr); > + if (cid != FMC2_CID1) { > + if (resource) > + dev_err(ebi->dev, "resource %d is already used by CID%d\n", > + resource, cid); > + > + return -EACCES; > + } > + > + ebi->sem_taken |= BIT(resource); > + > + return 0; > +} > + > +static void stm32_fmc2_ebi_put_sems(struct stm32_fmc2_ebi *ebi) > +{ > + unsigned int resource; > + > + if (ebi->majrev < FMC2_VERR_MAJREV_2) > + return; > + > + for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) { > + if (!(ebi->sem_taken & BIT(resource))) > + continue; > + > + regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource), > + FMC2_SEMCR_SEM_MUTEX, 0); > + } > +} > + > +static void stm32_fmc2_ebi_get_sems(struct stm32_fmc2_ebi *ebi) > +{ > + unsigned int resource; > + > + if (ebi->majrev < FMC2_VERR_MAJREV_2) > + return; > + > + for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) { > + if (!(ebi->sem_taken & BIT(resource))) > + continue; > + > + regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource), > + FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX); > + } > +} > + > static int stm32_fmc2_ebi_parse_prop(struct stm32_fmc2_ebi *ebi, > struct device_node *dev_node, > const struct stm32_fmc2_prop *prop, > @@ -1057,6 +1191,9 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi) > unsigned int cs; > > for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) { > + if (!(ebi->bank_assigned & BIT(cs))) > + continue; > + > regmap_read(ebi->regmap, FMC2_BCR(cs), &ebi->bcr[cs]); > regmap_read(ebi->regmap, FMC2_BTR(cs), &ebi->btr[cs]); > regmap_read(ebi->regmap, FMC2_BWTR(cs), &ebi->bwtr[cs]); > @@ -1064,7 +1201,7 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi) > > if (ebi->majrev < FMC2_VERR_MAJREV_2) > regmap_read(ebi->regmap, FMC2_PCSCNTR, &ebi->pcscntr); > - else > + else if (ebi->access_granted) > regmap_read(ebi->regmap, FMC2_CFGR, &ebi->cfgr); > } > > @@ -1073,6 +1210,9 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi) > unsigned int cs; > > for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) { > + if (!(ebi->bank_assigned & BIT(cs))) > + continue; > + > regmap_write(ebi->regmap, FMC2_BCR(cs), ebi->bcr[cs]); > regmap_write(ebi->regmap, FMC2_BTR(cs), ebi->btr[cs]); > regmap_write(ebi->regmap, FMC2_BWTR(cs), ebi->bwtr[cs]); > @@ -1080,7 +1220,7 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi) > > if (ebi->majrev < FMC2_VERR_MAJREV_2) > regmap_write(ebi->regmap, FMC2_PCSCNTR, ebi->pcscntr); > - else > + else if (ebi->access_granted) > regmap_write(ebi->regmap, FMC2_CFGR, ebi->cfgr); So this is kind of half-allowed-half-not. How is it supposed to work with !access_granted? You configure some registers but some not. So will it work or not? If yes, why even needing to write to FMC2_CFGR! > } > > @@ -1124,7 +1264,8 @@ static void stm32_fmc2_ebi_enable(struct stm32_fmc2_ebi *ebi) > u32 mask = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_BCR1_FMC2EN : > FMC2_CFGR_FMC2EN; > > - regmap_update_bits(ebi->regmap, reg, mask, mask); > + if (ebi->access_granted) > + regmap_update_bits(ebi->regmap, reg, mask, mask); > } > > static void stm32_fmc2_ebi_disable(struct stm32_fmc2_ebi *ebi) > @@ -1133,7 +1274,8 @@ static void stm32_fmc2_ebi_disable(struct stm32_fmc2_ebi *ebi) > u32 mask = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_BCR1_FMC2EN : > FMC2_CFGR_FMC2EN; > > - regmap_update_bits(ebi->regmap, reg, mask, 0); > + if (ebi->access_granted) > + regmap_update_bits(ebi->regmap, reg, mask, 0); > } > > static int stm32_fmc2_ebi_setup_cs(struct stm32_fmc2_ebi *ebi, > @@ -1190,6 +1332,13 @@ static int stm32_fmc2_ebi_parse_dt(struct stm32_fmc2_ebi *ebi) > return -EINVAL; > } > > + ret = stm32_fmc2_ebi_check_rif(ebi, bank + 1); > + if (ret) { > + dev_err(dev, "bank access failed: %d\n", bank); > + of_node_put(child); > + return ret; > + } > + > if (bank < FMC2_MAX_EBI_CE) { > ret = stm32_fmc2_ebi_setup_cs(ebi, child, bank); > if (ret) { > @@ -1261,6 +1410,23 @@ static int stm32_fmc2_ebi_probe(struct platform_device *pdev) > regmap_read(ebi->regmap, FMC2_VERR, &verr); > ebi->majrev = FIELD_GET(FMC2_VERR_MAJREV, verr); > > + /* Check if CFGR register can be modified */ > + ret = stm32_fmc2_ebi_check_rif(ebi, 0); > + if (!ret) > + ebi->access_granted = true; I don't understand why you need to store it. If access is not granted, what else is to do for this driver? Why even probing it? Why enabling clocks and keep everything running if it cannot work? > + > + /* In case of CFGR is secure, just check that the FMC2 is enabled */ > + if (!ebi->access_granted) { This is just "else", isn't it? > + u32 sr; > + > + regmap_read(ebi->regmap, FMC2_SR, &sr); > + if (sr & FMC2_SR_ISOST) { > + dev_err(dev, "FMC2 is not ready to be used.\n"); > + ret = -EACCES; > + goto err_release; > + } > + } > + > ret = stm32_fmc2_ebi_parse_dt(ebi); > Best regards, Krzysztof
On 2/13/24 08:52, Krzysztof Kozlowski wrote: > On 12/02/2024 18:48, Christophe Kerello wrote: >> The FMC2 revision 2 supports security and isolation compliant with >> the Resource Isolation Framework (RIF). From RIF point of view, >> the FMC2 is composed of several independent resources, listed below, >> which can be assigned to different security and compartment domains: >> - 0: Common FMC_CFGR register. >> - 1: EBI controller for Chip Select 1. >> - 2: EBI controller for Chip Select 2. >> - 3: EBI controller for Chip Select 3. >> - 4: EBI controller for Chip Select 4. >> - 5: NAND controller. >> > > >> regmap_update_bits(ebi->regmap, reg, mask, setup ? mask : 0); >> >> return 0; >> @@ -990,6 +1023,107 @@ static const struct stm32_fmc2_prop stm32_fmc2_child_props[] = { >> }, >> }; >> >> +static int stm32_fmc2_ebi_check_rif(struct stm32_fmc2_ebi *ebi, u32 resource) >> +{ >> + u32 seccfgr, cidcfgr, semcr; >> + int cid; >> + >> + if (ebi->majrev < FMC2_VERR_MAJREV_2) >> + return 0; >> + >> + if (resource >= FMC2_MAX_RESOURCES) >> + return -EINVAL; >> + >> + regmap_read(ebi->regmap, FMC2_SECCFGR, &seccfgr); Hi Krzysztof, > > No checking of read value? > No, it should never failed. >> + if (seccfgr & BIT(resource)) { > > Then on read failure this is random stack junk. > >> + if (resource) >> + dev_err(ebi->dev, "resource %d is configured as secure\n", >> + resource); >> + >> + return -EACCES; >> + } >> + >> + regmap_read(ebi->regmap, FMC2_CIDCFGR(resource), &cidcfgr); >> + if (!(cidcfgr & FMC2_CIDCFGR_CFEN)) >> + /* CID filtering is turned off: access granted */ >> + return 0; >> + >> + if (!(cidcfgr & FMC2_CIDCFGR_SEMEN)) { >> + /* Static CID mode */ >> + cid = FIELD_GET(FMC2_CIDCFGR_SCID, cidcfgr); >> + if (cid != FMC2_CID1) { >> + if (resource) >> + dev_err(ebi->dev, "static CID%d set for resource %d\n", >> + cid, resource); >> + >> + return -EACCES; >> + } >> + >> + return 0; >> + } >> + >> + /* Pass-list with semaphore mode */ >> + if (!(cidcfgr & FMC2_CIDCFGR_SEMWLC1)) { >> + if (resource) >> + dev_err(ebi->dev, "CID1 is block-listed for resource %d\n", >> + resource); >> + >> + return -EACCES; >> + } >> + >> + regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr); >> + if (!(semcr & FMC2_SEMCR_SEM_MUTEX)) { >> + regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource), >> + FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX); >> + regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr); >> + } >> + >> + cid = FIELD_GET(FMC2_SEMCR_SEMCID, semcr); >> + if (cid != FMC2_CID1) { >> + if (resource) >> + dev_err(ebi->dev, "resource %d is already used by CID%d\n", >> + resource, cid); >> + >> + return -EACCES; >> + } >> + >> + ebi->sem_taken |= BIT(resource); >> + >> + return 0; >> +} >> + >> +static void stm32_fmc2_ebi_put_sems(struct stm32_fmc2_ebi *ebi) >> +{ >> + unsigned int resource; >> + >> + if (ebi->majrev < FMC2_VERR_MAJREV_2) >> + return; >> + >> + for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) { >> + if (!(ebi->sem_taken & BIT(resource))) >> + continue; >> + >> + regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource), >> + FMC2_SEMCR_SEM_MUTEX, 0); >> + } >> +} >> + >> +static void stm32_fmc2_ebi_get_sems(struct stm32_fmc2_ebi *ebi) >> +{ >> + unsigned int resource; >> + >> + if (ebi->majrev < FMC2_VERR_MAJREV_2) >> + return; >> + >> + for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) { >> + if (!(ebi->sem_taken & BIT(resource))) >> + continue; >> + >> + regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource), >> + FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX); >> + } >> +} >> + >> static int stm32_fmc2_ebi_parse_prop(struct stm32_fmc2_ebi *ebi, >> struct device_node *dev_node, >> const struct stm32_fmc2_prop *prop, >> @@ -1057,6 +1191,9 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi) >> unsigned int cs; >> >> for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) { >> + if (!(ebi->bank_assigned & BIT(cs))) >> + continue; >> + >> regmap_read(ebi->regmap, FMC2_BCR(cs), &ebi->bcr[cs]); >> regmap_read(ebi->regmap, FMC2_BTR(cs), &ebi->btr[cs]); >> regmap_read(ebi->regmap, FMC2_BWTR(cs), &ebi->bwtr[cs]); >> @@ -1064,7 +1201,7 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi) >> >> if (ebi->majrev < FMC2_VERR_MAJREV_2) >> regmap_read(ebi->regmap, FMC2_PCSCNTR, &ebi->pcscntr); >> - else >> + else if (ebi->access_granted) >> regmap_read(ebi->regmap, FMC2_CFGR, &ebi->cfgr); >> } >> >> @@ -1073,6 +1210,9 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi) >> unsigned int cs; >> >> for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) { >> + if (!(ebi->bank_assigned & BIT(cs))) >> + continue; >> + >> regmap_write(ebi->regmap, FMC2_BCR(cs), ebi->bcr[cs]); >> regmap_write(ebi->regmap, FMC2_BTR(cs), ebi->btr[cs]); >> regmap_write(ebi->regmap, FMC2_BWTR(cs), ebi->bwtr[cs]); >> @@ -1080,7 +1220,7 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi) >> >> if (ebi->majrev < FMC2_VERR_MAJREV_2) >> regmap_write(ebi->regmap, FMC2_PCSCNTR, ebi->pcscntr); >> - else >> + else if (ebi->access_granted) >> regmap_write(ebi->regmap, FMC2_CFGR, ebi->cfgr); > > So this is kind of half-allowed-half-not. How is it supposed to work > with !access_granted? You configure some registers but some not. So will > it work or not? If yes, why even needing to write to FMC2_CFGR! > This register is considered as one resource and can be protected. If a companion (like optee_os) has configured this resource as secure, it means that the driver can not write into this register, and this register will be handled by the companion. If this register is let as non secure, the driver can handle this ressource. >> } >> >> @@ -1124,7 +1264,8 @@ static void stm32_fmc2_ebi_enable(struct stm32_fmc2_ebi *ebi) >> u32 mask = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_BCR1_FMC2EN : >> FMC2_CFGR_FMC2EN; >> >> - regmap_update_bits(ebi->regmap, reg, mask, mask); >> + if (ebi->access_granted) >> + regmap_update_bits(ebi->regmap, reg, mask, mask); >> } >> >> static void stm32_fmc2_ebi_disable(struct stm32_fmc2_ebi *ebi) >> @@ -1133,7 +1274,8 @@ static void stm32_fmc2_ebi_disable(struct stm32_fmc2_ebi *ebi) >> u32 mask = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_BCR1_FMC2EN : >> FMC2_CFGR_FMC2EN; >> >> - regmap_update_bits(ebi->regmap, reg, mask, 0); >> + if (ebi->access_granted) >> + regmap_update_bits(ebi->regmap, reg, mask, 0); >> } >> >> static int stm32_fmc2_ebi_setup_cs(struct stm32_fmc2_ebi *ebi, >> @@ -1190,6 +1332,13 @@ static int stm32_fmc2_ebi_parse_dt(struct stm32_fmc2_ebi *ebi) >> return -EINVAL; >> } >> >> + ret = stm32_fmc2_ebi_check_rif(ebi, bank + 1); >> + if (ret) { >> + dev_err(dev, "bank access failed: %d\n", bank); >> + of_node_put(child); >> + return ret; >> + } >> + >> if (bank < FMC2_MAX_EBI_CE) { >> ret = stm32_fmc2_ebi_setup_cs(ebi, child, bank); >> if (ret) { >> @@ -1261,6 +1410,23 @@ static int stm32_fmc2_ebi_probe(struct platform_device *pdev) >> regmap_read(ebi->regmap, FMC2_VERR, &verr); >> ebi->majrev = FIELD_GET(FMC2_VERR_MAJREV, verr); >> >> + /* Check if CFGR register can be modified */ >> + ret = stm32_fmc2_ebi_check_rif(ebi, 0); >> + if (!ret) >> + ebi->access_granted = true; > > I don't understand why you need to store it. If access is not granted, > what else is to do for this driver? Why even probing it? Why enabling > clocks and keep everything running if it cannot work? > CFGR register contains the bit that is enabling the IP. CFGR register can be set to secure when all the others ressources can be set to non secure. If CFGR register is secured, then we check that the IP has been enabled by the companion. If it is the case, PSRAM controller or NAND controller set as non secure can be used. And, if CFGR register is secured and the IP is not enabled, the probe of the driver fails. >> + >> + /* In case of CFGR is secure, just check that the FMC2 is enabled */ >> + if (!ebi->access_granted) { > > This is just "else", isn't it? Yes, can be "else". Regards, Christophe Kerello. > >> + u32 sr; >> + >> + regmap_read(ebi->regmap, FMC2_SR, &sr); >> + if (sr & FMC2_SR_ISOST) { >> + dev_err(dev, "FMC2 is not ready to be used.\n"); >> + ret = -EACCES; >> + goto err_release; >> + } >> + } >> + >> ret = stm32_fmc2_ebi_parse_dt(ebi); > >> > > Best regards, > Krzysztof >
On 13/02/2024 14:15, Christophe Kerello wrote: >>> + >>> + if (ebi->majrev < FMC2_VERR_MAJREV_2) >>> + return 0; >>> + >>> + if (resource >= FMC2_MAX_RESOURCES) >>> + return -EINVAL; >>> + >>> + regmap_read(ebi->regmap, FMC2_SECCFGR, &seccfgr); > > Hi Krzysztof, > >> >> No checking of read value? >> > > No, it should never failed. And you tested that neither smatch, sparse nor Coverity report here warnings? > >>> + if (seccfgr & BIT(resource)) { >> >> Then on read failure this is random stack junk. >> >>> + if (resource) >>> + dev_err(ebi->dev, "resource %d is configured as secure\n", >>> + resource); >>> + >>> + return -EACCES; >>> + } >>> + >>> + regmap_read(ebi->regmap, FMC2_CIDCFGR(resource), &cidcfgr); >>> + if (!(cidcfgr & FMC2_CIDCFGR_CFEN)) >>> + /* CID filtering is turned off: access granted */ >>> + return 0; >>> + >>> + if (!(cidcfgr & FMC2_CIDCFGR_SEMEN)) { >>> + /* Static CID mode */ >>> + cid = FIELD_GET(FMC2_CIDCFGR_SCID, cidcfgr); >>> + if (cid != FMC2_CID1) { >>> + if (resource) >>> + dev_err(ebi->dev, "static CID%d set for resource %d\n", >>> + cid, resource); >>> + >>> + return -EACCES; >>> + } >>> + >>> + return 0; >>> + } >>> + >>> + /* Pass-list with semaphore mode */ >>> + if (!(cidcfgr & FMC2_CIDCFGR_SEMWLC1)) { >>> + if (resource) >>> + dev_err(ebi->dev, "CID1 is block-listed for resource %d\n", >>> + resource); >>> + >>> + return -EACCES; >>> + } >>> + >>> + regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr); >>> + if (!(semcr & FMC2_SEMCR_SEM_MUTEX)) { >>> + regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource), >>> + FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX); >>> + regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr); >>> + } >>> + >>> + cid = FIELD_GET(FMC2_SEMCR_SEMCID, semcr); >>> + if (cid != FMC2_CID1) { >>> + if (resource) >>> + dev_err(ebi->dev, "resource %d is already used by CID%d\n", >>> + resource, cid); >>> + >>> + return -EACCES; >>> + } >>> + >>> + ebi->sem_taken |= BIT(resource); >>> + >>> + return 0; >>> +} >>> + >>> +static void stm32_fmc2_ebi_put_sems(struct stm32_fmc2_ebi *ebi) >>> +{ >>> + unsigned int resource; >>> + >>> + if (ebi->majrev < FMC2_VERR_MAJREV_2) >>> + return; >>> + >>> + for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) { >>> + if (!(ebi->sem_taken & BIT(resource))) >>> + continue; >>> + >>> + regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource), >>> + FMC2_SEMCR_SEM_MUTEX, 0); >>> + } >>> +} >>> + >>> +static void stm32_fmc2_ebi_get_sems(struct stm32_fmc2_ebi *ebi) >>> +{ >>> + unsigned int resource; >>> + >>> + if (ebi->majrev < FMC2_VERR_MAJREV_2) >>> + return; >>> + >>> + for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) { >>> + if (!(ebi->sem_taken & BIT(resource))) >>> + continue; >>> + >>> + regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource), >>> + FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX); >>> + } >>> +} >>> + >>> static int stm32_fmc2_ebi_parse_prop(struct stm32_fmc2_ebi *ebi, >>> struct device_node *dev_node, >>> const struct stm32_fmc2_prop *prop, >>> @@ -1057,6 +1191,9 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi) >>> unsigned int cs; >>> >>> for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) { >>> + if (!(ebi->bank_assigned & BIT(cs))) >>> + continue; >>> + >>> regmap_read(ebi->regmap, FMC2_BCR(cs), &ebi->bcr[cs]); >>> regmap_read(ebi->regmap, FMC2_BTR(cs), &ebi->btr[cs]); >>> regmap_read(ebi->regmap, FMC2_BWTR(cs), &ebi->bwtr[cs]); >>> @@ -1064,7 +1201,7 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi) >>> >>> if (ebi->majrev < FMC2_VERR_MAJREV_2) >>> regmap_read(ebi->regmap, FMC2_PCSCNTR, &ebi->pcscntr); >>> - else >>> + else if (ebi->access_granted) >>> regmap_read(ebi->regmap, FMC2_CFGR, &ebi->cfgr); >>> } >>> >>> @@ -1073,6 +1210,9 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi) >>> unsigned int cs; >>> >>> for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) { >>> + if (!(ebi->bank_assigned & BIT(cs))) >>> + continue; >>> + >>> regmap_write(ebi->regmap, FMC2_BCR(cs), ebi->bcr[cs]); >>> regmap_write(ebi->regmap, FMC2_BTR(cs), ebi->btr[cs]); >>> regmap_write(ebi->regmap, FMC2_BWTR(cs), ebi->bwtr[cs]); >>> @@ -1080,7 +1220,7 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi) >>> >>> if (ebi->majrev < FMC2_VERR_MAJREV_2) >>> regmap_write(ebi->regmap, FMC2_PCSCNTR, ebi->pcscntr); >>> - else >>> + else if (ebi->access_granted) >>> regmap_write(ebi->regmap, FMC2_CFGR, ebi->cfgr); >> >> So this is kind of half-allowed-half-not. How is it supposed to work >> with !access_granted? You configure some registers but some not. So will >> it work or not? If yes, why even needing to write to FMC2_CFGR! >> > > This register is considered as one resource and can be protected. If a > companion (like optee_os) has configured this resource as secure, it > means that the driver can not write into this register, and this > register will be handled by the companion. If this register is let as > non secure, the driver can handle this ressource. So this is not an error? Other places print errors and return -EACCESS, so that's a bit confusing me. Best regards, Krzysztof
On 2/14/24 11:07, Krzysztof Kozlowski wrote: > On 13/02/2024 14:15, Christophe Kerello wrote: >>>> + >>>> + if (ebi->majrev < FMC2_VERR_MAJREV_2) >>>> + return 0; >>>> + >>>> + if (resource >= FMC2_MAX_RESOURCES) >>>> + return -EINVAL; >>>> + >>>> + regmap_read(ebi->regmap, FMC2_SECCFGR, &seccfgr); >> >> Hi Krzysztof, >> >>> >>> No checking of read value? >>> >> >> No, it should never failed. > > And you tested that neither smatch, sparse nor Coverity report here > warnings? > Hi Krzysztof, There is a lot of driver in the Kernel that are using same implementation, and I am surprised to not have had this comment 4 years ago when the driver was introduced. So, how should I proceed? Shall I initialize all local variables used by regmap_read? Or shall I check the return value of regmap_read? And, as there is a lot of regmap_read call in this driver, shall I fix them in a separate patch? >> >>>> + if (seccfgr & BIT(resource)) { >>> >>> Then on read failure this is random stack junk. >>> >>>> + if (resource) >>>> + dev_err(ebi->dev, "resource %d is configured as secure\n", >>>> + resource); >>>> + >>>> + return -EACCES; >>>> + } >>>> + >>>> + regmap_read(ebi->regmap, FMC2_CIDCFGR(resource), &cidcfgr); >>>> + if (!(cidcfgr & FMC2_CIDCFGR_CFEN)) >>>> + /* CID filtering is turned off: access granted */ >>>> + return 0; >>>> + >>>> + if (!(cidcfgr & FMC2_CIDCFGR_SEMEN)) { >>>> + /* Static CID mode */ >>>> + cid = FIELD_GET(FMC2_CIDCFGR_SCID, cidcfgr); >>>> + if (cid != FMC2_CID1) { >>>> + if (resource) >>>> + dev_err(ebi->dev, "static CID%d set for resource %d\n", >>>> + cid, resource); >>>> + >>>> + return -EACCES; >>>> + } >>>> + >>>> + return 0; >>>> + } >>>> + >>>> + /* Pass-list with semaphore mode */ >>>> + if (!(cidcfgr & FMC2_CIDCFGR_SEMWLC1)) { >>>> + if (resource) >>>> + dev_err(ebi->dev, "CID1 is block-listed for resource %d\n", >>>> + resource); >>>> + >>>> + return -EACCES; >>>> + } >>>> + >>>> + regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr); >>>> + if (!(semcr & FMC2_SEMCR_SEM_MUTEX)) { >>>> + regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource), >>>> + FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX); >>>> + regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr); >>>> + } >>>> + >>>> + cid = FIELD_GET(FMC2_SEMCR_SEMCID, semcr); >>>> + if (cid != FMC2_CID1) { >>>> + if (resource) >>>> + dev_err(ebi->dev, "resource %d is already used by CID%d\n", >>>> + resource, cid); >>>> + >>>> + return -EACCES; >>>> + } >>>> + >>>> + ebi->sem_taken |= BIT(resource); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static void stm32_fmc2_ebi_put_sems(struct stm32_fmc2_ebi *ebi) >>>> +{ >>>> + unsigned int resource; >>>> + >>>> + if (ebi->majrev < FMC2_VERR_MAJREV_2) >>>> + return; >>>> + >>>> + for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) { >>>> + if (!(ebi->sem_taken & BIT(resource))) >>>> + continue; >>>> + >>>> + regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource), >>>> + FMC2_SEMCR_SEM_MUTEX, 0); >>>> + } >>>> +} >>>> + >>>> +static void stm32_fmc2_ebi_get_sems(struct stm32_fmc2_ebi *ebi) >>>> +{ >>>> + unsigned int resource; >>>> + >>>> + if (ebi->majrev < FMC2_VERR_MAJREV_2) >>>> + return; >>>> + >>>> + for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) { >>>> + if (!(ebi->sem_taken & BIT(resource))) >>>> + continue; >>>> + >>>> + regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource), >>>> + FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX); >>>> + } >>>> +} >>>> + >>>> static int stm32_fmc2_ebi_parse_prop(struct stm32_fmc2_ebi *ebi, >>>> struct device_node *dev_node, >>>> const struct stm32_fmc2_prop *prop, >>>> @@ -1057,6 +1191,9 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi) >>>> unsigned int cs; >>>> >>>> for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) { >>>> + if (!(ebi->bank_assigned & BIT(cs))) >>>> + continue; >>>> + >>>> regmap_read(ebi->regmap, FMC2_BCR(cs), &ebi->bcr[cs]); >>>> regmap_read(ebi->regmap, FMC2_BTR(cs), &ebi->btr[cs]); >>>> regmap_read(ebi->regmap, FMC2_BWTR(cs), &ebi->bwtr[cs]); >>>> @@ -1064,7 +1201,7 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi) >>>> >>>> if (ebi->majrev < FMC2_VERR_MAJREV_2) >>>> regmap_read(ebi->regmap, FMC2_PCSCNTR, &ebi->pcscntr); >>>> - else >>>> + else if (ebi->access_granted) >>>> regmap_read(ebi->regmap, FMC2_CFGR, &ebi->cfgr); >>>> } >>>> >>>> @@ -1073,6 +1210,9 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi) >>>> unsigned int cs; >>>> >>>> for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) { >>>> + if (!(ebi->bank_assigned & BIT(cs))) >>>> + continue; >>>> + >>>> regmap_write(ebi->regmap, FMC2_BCR(cs), ebi->bcr[cs]); >>>> regmap_write(ebi->regmap, FMC2_BTR(cs), ebi->btr[cs]); >>>> regmap_write(ebi->regmap, FMC2_BWTR(cs), ebi->bwtr[cs]); >>>> @@ -1080,7 +1220,7 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi) >>>> >>>> if (ebi->majrev < FMC2_VERR_MAJREV_2) >>>> regmap_write(ebi->regmap, FMC2_PCSCNTR, ebi->pcscntr); >>>> - else >>>> + else if (ebi->access_granted) >>>> regmap_write(ebi->regmap, FMC2_CFGR, ebi->cfgr); >>> >>> So this is kind of half-allowed-half-not. How is it supposed to work >>> with !access_granted? You configure some registers but some not. So will >>> it work or not? If yes, why even needing to write to FMC2_CFGR! >>> >> >> This register is considered as one resource and can be protected. If a >> companion (like optee_os) has configured this resource as secure, it >> means that the driver can not write into this register, and this >> register will be handled by the companion. If this register is let as >> non secure, the driver can handle this ressource. > > So this is not an error? Other places print errors and return -EACCESS, > so that's a bit confusing me. > It is not an error. We are saving registers values to restore them on low power cases. If registers are set as secure, it is the responsibility of the companion to restore them when it is the responsibility of the non secure driver to restore them if they are configured as non secure. Regards, Christophe Kerello. > > Best regards, > Krzysztof >
On 15/02/2024 10:00, Christophe Kerello wrote: > > > On 2/14/24 11:07, Krzysztof Kozlowski wrote: >> On 13/02/2024 14:15, Christophe Kerello wrote: >>>>> + >>>>> + if (ebi->majrev < FMC2_VERR_MAJREV_2) >>>>> + return 0; >>>>> + >>>>> + if (resource >= FMC2_MAX_RESOURCES) >>>>> + return -EINVAL; >>>>> + >>>>> + regmap_read(ebi->regmap, FMC2_SECCFGR, &seccfgr); >>> >>> Hi Krzysztof, >>> >>>> >>>> No checking of read value? >>>> >>> >>> No, it should never failed. >> >> And you tested that neither smatch, sparse nor Coverity report here >> warnings? >> > > Hi Krzysztof, > > There is a lot of driver in the Kernel that are using same > implementation, and I am surprised to not have had this comment 4 years > ago when the driver was introduced. Really? Care to give some pointers? Heh, I don't know what to respond to it. Either you say that my comment is incorrect or you say that it's okay to sneak poor code if no one notices? We can argue on the first, whether my comment is reasonable or not. But if you claim that previous poor choice of code is argument of bringing more of such poor choices, then we are done here. It's the oldest argument: someone did it that way, so I can do the same. Nope. > > So, how should I proceed? Shall I initialize all local variables used by > regmap_read? Or shall I check the return value of regmap_read? > And, as there is a lot of regmap_read call in this driver, shall I fix > them in a separate patch? regmap operations, depending on the regmap used, can fail. Most of the errors are result of static configuration, e.g. alignment, regmap in cache mode etc. Then certain regmap implementations can produce errors, which is not a static condition but dynamic. You have neither error checking nor value initialization. You risk here to have quite tricky to find, unnoticeable bugs, if there any mistake leading to regmap errors. Indeed neither smatch nor sparse report this as error currently, but maybe that's their limitation? Best regards, Krzysztof
On 2/15/24 19:56, Krzysztof Kozlowski wrote: > On 15/02/2024 10:00, Christophe Kerello wrote: >> >> >> On 2/14/24 11:07, Krzysztof Kozlowski wrote: >>> On 13/02/2024 14:15, Christophe Kerello wrote: >>>>>> + >>>>>> + if (ebi->majrev < FMC2_VERR_MAJREV_2) >>>>>> + return 0; >>>>>> + >>>>>> + if (resource >= FMC2_MAX_RESOURCES) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + regmap_read(ebi->regmap, FMC2_SECCFGR, &seccfgr); >>>> >>>> Hi Krzysztof, >>>> >>>>> >>>>> No checking of read value? >>>>> >>>> >>>> No, it should never failed. >>> >>> And you tested that neither smatch, sparse nor Coverity report here >>> warnings? >>> >> >> Hi Krzysztof, >> >> There is a lot of driver in the Kernel that are using same >> implementation, and I am surprised to not have had this comment 4 years >> ago when the driver was introduced. > > Really? Care to give some pointers? Heh, I don't know what to respond to > it. Either you say that my comment is incorrect or you say that it's > okay to sneak poor code if no one notices? We can argue on the first, > whether my comment is reasonable or not. But if you claim that previous > poor choice of code is argument of bringing more of such poor choices, > then we are done here. It's the oldest argument: someone did it that > way, so I can do the same. Nope. > >> >> So, how should I proceed? Shall I initialize all local variables used by >> regmap_read? Or shall I check the return value of regmap_read? >> And, as there is a lot of regmap_read call in this driver, shall I fix >> them in a separate patch? > > regmap operations, depending on the regmap used, can fail. Most of the > errors are result of static configuration, e.g. alignment, regmap in > cache mode etc. Then certain regmap implementations can produce errors, > which is not a static condition but dynamic. > > You have neither error checking nor value initialization. You risk here > to have quite tricky to find, unnoticeable bugs, if there any mistake > leading to regmap errors. > > Indeed neither smatch nor sparse report this as error currently, but > maybe that's their limitation? > Hi Krzysztof, I will check the return value of regmap_read API. Regards, Christophe Kerello. > > Best regards, > Krzysztof >
diff --git a/drivers/memory/stm32-fmc2-ebi.c b/drivers/memory/stm32-fmc2-ebi.c index 066722274a45..04248c15832f 100644 --- a/drivers/memory/stm32-fmc2-ebi.c +++ b/drivers/memory/stm32-fmc2-ebi.c @@ -21,8 +21,14 @@ #define FMC2_BTR(x) ((x) * 0x8 + FMC2_BTR1) #define FMC2_PCSCNTR 0x20 #define FMC2_CFGR 0x20 +#define FMC2_SR 0x84 #define FMC2_BWTR1 0x104 #define FMC2_BWTR(x) ((x) * 0x8 + FMC2_BWTR1) +#define FMC2_SECCFGR 0x300 +#define FMC2_CIDCFGR0 0x30c +#define FMC2_CIDCFGR(x) ((x) * 0x8 + FMC2_CIDCFGR0) +#define FMC2_SEMCR0 0x310 +#define FMC2_SEMCR(x) ((x) * 0x8 + FMC2_SEMCR0) #define FMC2_VERR 0x3f4 /* Register: FMC2_BCR1 */ @@ -66,12 +72,27 @@ #define FMC2_CFGR_CCLKEN BIT(20) #define FMC2_CFGR_FMC2EN BIT(31) +/* Register: FMC2_SR */ +#define FMC2_SR_ISOST GENMASK(1, 0) + +/* Register: FMC2_CIDCFGR */ +#define FMC2_CIDCFGR_CFEN BIT(0) +#define FMC2_CIDCFGR_SEMEN BIT(1) +#define FMC2_CIDCFGR_SCID GENMASK(6, 4) +#define FMC2_CIDCFGR_SEMWLC1 BIT(17) + +/* Register: FMC2_SEMCR */ +#define FMC2_SEMCR_SEM_MUTEX BIT(0) +#define FMC2_SEMCR_SEMCID GENMASK(7, 5) + /* Register: FMC2_VERR */ #define FMC2_VERR_MAJREV GENMASK(7, 4) #define FMC2_VERR_MAJREV_2 2 #define FMC2_MAX_EBI_CE 4 #define FMC2_MAX_BANKS 5 +#define FMC2_MAX_RESOURCES 6 +#define FMC2_CID1 1 #define FMC2_BCR_CPSIZE_0 0x0 #define FMC2_BCR_CPSIZE_128 0x1 @@ -167,7 +188,9 @@ struct stm32_fmc2_ebi { struct regmap *regmap; const struct stm32_fmc2_ebi_data *data; u8 bank_assigned; + u8 sem_taken; u8 majrev; + bool access_granted; u32 bcr[FMC2_MAX_EBI_CE]; u32 btr[FMC2_MAX_EBI_CE]; @@ -733,6 +756,11 @@ static int stm32_fmc2_ebi_set_clk_period(struct stm32_fmc2_ebi *ebi, u32 reg = FMC2_BTR(cs); u32 mask = FMC2_BTR_CLKDIV; + if (!ebi->access_granted) { + dev_err(ebi->dev, "CFGR access forbidden\n"); + return -EACCES; + } + if (ebi->majrev >= FMC2_VERR_MAJREV_2) { u32 cfgr; @@ -822,6 +850,11 @@ static int stm32_fmc2_ebi_set_cclk(struct stm32_fmc2_ebi *ebi, u32 mask = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_BCR1_CCLKEN : FMC2_CFGR_CCLKEN; + if (!ebi->access_granted) { + dev_err(ebi->dev, "CFGR access forbidden\n"); + return -EACCES; + } + regmap_update_bits(ebi->regmap, reg, mask, setup ? mask : 0); return 0; @@ -990,6 +1023,107 @@ static const struct stm32_fmc2_prop stm32_fmc2_child_props[] = { }, }; +static int stm32_fmc2_ebi_check_rif(struct stm32_fmc2_ebi *ebi, u32 resource) +{ + u32 seccfgr, cidcfgr, semcr; + int cid; + + if (ebi->majrev < FMC2_VERR_MAJREV_2) + return 0; + + if (resource >= FMC2_MAX_RESOURCES) + return -EINVAL; + + regmap_read(ebi->regmap, FMC2_SECCFGR, &seccfgr); + if (seccfgr & BIT(resource)) { + if (resource) + dev_err(ebi->dev, "resource %d is configured as secure\n", + resource); + + return -EACCES; + } + + regmap_read(ebi->regmap, FMC2_CIDCFGR(resource), &cidcfgr); + if (!(cidcfgr & FMC2_CIDCFGR_CFEN)) + /* CID filtering is turned off: access granted */ + return 0; + + if (!(cidcfgr & FMC2_CIDCFGR_SEMEN)) { + /* Static CID mode */ + cid = FIELD_GET(FMC2_CIDCFGR_SCID, cidcfgr); + if (cid != FMC2_CID1) { + if (resource) + dev_err(ebi->dev, "static CID%d set for resource %d\n", + cid, resource); + + return -EACCES; + } + + return 0; + } + + /* Pass-list with semaphore mode */ + if (!(cidcfgr & FMC2_CIDCFGR_SEMWLC1)) { + if (resource) + dev_err(ebi->dev, "CID1 is block-listed for resource %d\n", + resource); + + return -EACCES; + } + + regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr); + if (!(semcr & FMC2_SEMCR_SEM_MUTEX)) { + regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource), + FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX); + regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr); + } + + cid = FIELD_GET(FMC2_SEMCR_SEMCID, semcr); + if (cid != FMC2_CID1) { + if (resource) + dev_err(ebi->dev, "resource %d is already used by CID%d\n", + resource, cid); + + return -EACCES; + } + + ebi->sem_taken |= BIT(resource); + + return 0; +} + +static void stm32_fmc2_ebi_put_sems(struct stm32_fmc2_ebi *ebi) +{ + unsigned int resource; + + if (ebi->majrev < FMC2_VERR_MAJREV_2) + return; + + for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) { + if (!(ebi->sem_taken & BIT(resource))) + continue; + + regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource), + FMC2_SEMCR_SEM_MUTEX, 0); + } +} + +static void stm32_fmc2_ebi_get_sems(struct stm32_fmc2_ebi *ebi) +{ + unsigned int resource; + + if (ebi->majrev < FMC2_VERR_MAJREV_2) + return; + + for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) { + if (!(ebi->sem_taken & BIT(resource))) + continue; + + regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource), + FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX); + } +} + static int stm32_fmc2_ebi_parse_prop(struct stm32_fmc2_ebi *ebi, struct device_node *dev_node, const struct stm32_fmc2_prop *prop, @@ -1057,6 +1191,9 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi) unsigned int cs; for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) { + if (!(ebi->bank_assigned & BIT(cs))) + continue; + regmap_read(ebi->regmap, FMC2_BCR(cs), &ebi->bcr[cs]); regmap_read(ebi->regmap, FMC2_BTR(cs), &ebi->btr[cs]); regmap_read(ebi->regmap, FMC2_BWTR(cs), &ebi->bwtr[cs]); @@ -1064,7 +1201,7 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi) if (ebi->majrev < FMC2_VERR_MAJREV_2) regmap_read(ebi->regmap, FMC2_PCSCNTR, &ebi->pcscntr); - else + else if (ebi->access_granted) regmap_read(ebi->regmap, FMC2_CFGR, &ebi->cfgr); } @@ -1073,6 +1210,9 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi) unsigned int cs; for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) { + if (!(ebi->bank_assigned & BIT(cs))) + continue; + regmap_write(ebi->regmap, FMC2_BCR(cs), ebi->bcr[cs]); regmap_write(ebi->regmap, FMC2_BTR(cs), ebi->btr[cs]); regmap_write(ebi->regmap, FMC2_BWTR(cs), ebi->bwtr[cs]); @@ -1080,7 +1220,7 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi) if (ebi->majrev < FMC2_VERR_MAJREV_2) regmap_write(ebi->regmap, FMC2_PCSCNTR, ebi->pcscntr); - else + else if (ebi->access_granted) regmap_write(ebi->regmap, FMC2_CFGR, ebi->cfgr); } @@ -1124,7 +1264,8 @@ static void stm32_fmc2_ebi_enable(struct stm32_fmc2_ebi *ebi) u32 mask = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_BCR1_FMC2EN : FMC2_CFGR_FMC2EN; - regmap_update_bits(ebi->regmap, reg, mask, mask); + if (ebi->access_granted) + regmap_update_bits(ebi->regmap, reg, mask, mask); } static void stm32_fmc2_ebi_disable(struct stm32_fmc2_ebi *ebi) @@ -1133,7 +1274,8 @@ static void stm32_fmc2_ebi_disable(struct stm32_fmc2_ebi *ebi) u32 mask = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_BCR1_FMC2EN : FMC2_CFGR_FMC2EN; - regmap_update_bits(ebi->regmap, reg, mask, 0); + if (ebi->access_granted) + regmap_update_bits(ebi->regmap, reg, mask, 0); } static int stm32_fmc2_ebi_setup_cs(struct stm32_fmc2_ebi *ebi, @@ -1190,6 +1332,13 @@ static int stm32_fmc2_ebi_parse_dt(struct stm32_fmc2_ebi *ebi) return -EINVAL; } + ret = stm32_fmc2_ebi_check_rif(ebi, bank + 1); + if (ret) { + dev_err(dev, "bank access failed: %d\n", bank); + of_node_put(child); + return ret; + } + if (bank < FMC2_MAX_EBI_CE) { ret = stm32_fmc2_ebi_setup_cs(ebi, child, bank); if (ret) { @@ -1261,6 +1410,23 @@ static int stm32_fmc2_ebi_probe(struct platform_device *pdev) regmap_read(ebi->regmap, FMC2_VERR, &verr); ebi->majrev = FIELD_GET(FMC2_VERR_MAJREV, verr); + /* Check if CFGR register can be modified */ + ret = stm32_fmc2_ebi_check_rif(ebi, 0); + if (!ret) + ebi->access_granted = true; + + /* In case of CFGR is secure, just check that the FMC2 is enabled */ + if (!ebi->access_granted) { + u32 sr; + + regmap_read(ebi->regmap, FMC2_SR, &sr); + if (sr & FMC2_SR_ISOST) { + dev_err(dev, "FMC2 is not ready to be used.\n"); + ret = -EACCES; + goto err_release; + } + } + ret = stm32_fmc2_ebi_parse_dt(ebi); if (ret) goto err_release; @@ -1273,6 +1439,7 @@ static int stm32_fmc2_ebi_probe(struct platform_device *pdev) err_release: stm32_fmc2_ebi_disable_banks(ebi); stm32_fmc2_ebi_disable(ebi); + stm32_fmc2_ebi_put_sems(ebi); clk_disable_unprepare(ebi->clk); return ret; @@ -1285,6 +1452,7 @@ static void stm32_fmc2_ebi_remove(struct platform_device *pdev) of_platform_depopulate(&pdev->dev); stm32_fmc2_ebi_disable_banks(ebi); stm32_fmc2_ebi_disable(ebi); + stm32_fmc2_ebi_put_sems(ebi); clk_disable_unprepare(ebi->clk); } @@ -1293,6 +1461,7 @@ static int __maybe_unused stm32_fmc2_ebi_suspend(struct device *dev) struct stm32_fmc2_ebi *ebi = dev_get_drvdata(dev); stm32_fmc2_ebi_disable(ebi); + stm32_fmc2_ebi_put_sems(ebi); clk_disable_unprepare(ebi->clk); pinctrl_pm_select_sleep_state(dev); @@ -1310,6 +1479,7 @@ static int __maybe_unused stm32_fmc2_ebi_resume(struct device *dev) if (ret) return ret; + stm32_fmc2_ebi_get_sems(ebi); stm32_fmc2_ebi_set_setup(ebi); stm32_fmc2_ebi_enable(ebi);
The FMC2 revision 2 supports security and isolation compliant with the Resource Isolation Framework (RIF). From RIF point of view, the FMC2 is composed of several independent resources, listed below, which can be assigned to different security and compartment domains: - 0: Common FMC_CFGR register. - 1: EBI controller for Chip Select 1. - 2: EBI controller for Chip Select 2. - 3: EBI controller for Chip Select 3. - 4: EBI controller for Chip Select 4. - 5: NAND controller. Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com> --- drivers/memory/stm32-fmc2-ebi.c | 178 +++++++++++++++++++++++++++++++- 1 file changed, 174 insertions(+), 4 deletions(-)