Messages in this thread | | | From | Zong Li <> | Date | Thu, 3 Aug 2023 11:37:16 +0800 | Subject | Re: [PATCH 01/11] RISC-V: drivers/iommu: Add RISC-V IOMMU - Ziommu support. |
| |
On Thu, Aug 3, 2023 at 4:15 AM Tomasz Jeznach <tjeznach@rivosinc.com> wrote: > > On Thu, Jul 27, 2023 at 7:42 PM Zong Li <zong.li@sifive.com> wrote: > > > > On Thu, Jul 20, 2023 at 3:34 AM Tomasz Jeznach <tjeznach@rivosinc.com> wrote: > > > > > > +static int riscv_iommu_platform_probe(struct platform_device *pdev) > > > +{ > > > + struct device *dev = &pdev->dev; > > > + struct riscv_iommu_device *iommu = NULL; > > > + struct resource *res = NULL; > > > + int ret = 0; > > > + > > > + iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL); > > > + if (!iommu) > > > + return -ENOMEM; > > > + > > > + iommu->dev = dev; > > > + dev_set_drvdata(dev, iommu); > > > + > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > + if (!res) { > > > + dev_err(dev, "could not find resource for register region\n"); > > > + return -EINVAL; > > > + } > > > + > > > + iommu->reg = devm_platform_get_and_ioremap_resource(pdev, 0, &res); > > > + if (IS_ERR(iommu->reg)) { > > > + ret = dev_err_probe(dev, PTR_ERR(iommu->reg), > > > + "could not map register region\n"); > > > + goto fail; > > > + }; > > > + > > > + iommu->reg_phys = res->start; > > > + > > > + ret = -ENODEV; > > > + > > > + /* Sanity check: Did we get the whole register space ? */ > > > + if ((res->end - res->start + 1) < RISCV_IOMMU_REG_SIZE) { > > > + dev_err(dev, "device region smaller than register file (0x%llx)\n", > > > + res->end - res->start); > > > + goto fail; > > > + } > > > > Could we assume that DT should be responsible for specifying the right size? > > > > This only to validate DT provided info and driver expected register > file size. Expectation is that DT will provide right size. > > > > > +static struct iommu_domain *riscv_iommu_domain_alloc(unsigned type) > > > +{ > > > + struct riscv_iommu_domain *domain; > > > + > > > + if (type != IOMMU_DOMAIN_IDENTITY && > > > + type != IOMMU_DOMAIN_BLOCKED) > > > + return NULL; > > > + > > > + domain = kzalloc(sizeof(*domain), GFP_KERNEL); > > > + if (!domain) > > > + return NULL; > > > + > > > + mutex_init(&domain->lock); > > > + INIT_LIST_HEAD(&domain->endpoints); > > > + > > > + domain->domain.ops = &riscv_iommu_domain_ops; > > > + domain->mode = RISCV_IOMMU_DC_FSC_MODE_BARE; > > > + domain->pscid = ida_alloc_range(&riscv_iommu_pscids, 1, > > > + RISCV_IOMMU_MAX_PSCID, GFP_KERNEL); > > > + > > > + printk("domain type %x alloc %u\n", type, domain->pscid); > > > + > > > > Could it uses pr_xxx instead of printk? > > > > Absolutely, fixed here and elsewhere. Also, used dev_dbg wherever applicable. > > > > + > > > +static int riscv_iommu_enable(struct riscv_iommu_device *iommu, unsigned requested_mode) > > > +{ > > > + struct device *dev = iommu->dev; > > > + u64 ddtp = 0; > > > + u64 ddtp_paddr = 0; > > > + unsigned mode = requested_mode; > > > + unsigned mode_readback = 0; > > > + > > > + ddtp = riscv_iommu_get_ddtp(iommu); > > > + if (ddtp & RISCV_IOMMU_DDTP_BUSY) > > > + return -EBUSY; > > > + > > > + /* Disallow state transtion from xLVL to xLVL. */ > > > + switch (FIELD_GET(RISCV_IOMMU_DDTP_MODE, ddtp)) { > > > + case RISCV_IOMMU_DDTP_MODE_BARE: > > > + case RISCV_IOMMU_DDTP_MODE_OFF: > > > + break; > > > + default: > > > + if ((mode != RISCV_IOMMU_DDTP_MODE_BARE) > > > + && (mode != RISCV_IOMMU_DDTP_MODE_OFF)) > > > + return -EINVAL; > > > + break; > > > + } > > > + > > > + retry: > > > > We need to consider the `iommu.passthrough` before we set up the mode > > in switch case, something like > > > > This function is only to execute configuration and set device directory mode. > Handling global iommu.passthrough policy is implemented in > riscv_iommu_init() call (patch #7).
Thanks. I saw that in patch #7.
> > Best, > - Tomasz
| |