| Message ID | 52845c9fdfdd7f38a694e7727f3eabbd10e9f8ee.1715147377.git.nicolinc@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Tegra241 (Grace) CMDQV Support (part 1/2) | expand |
On Tue, May 07, 2024 at 10:56:51PM -0700, Nicolin Chen wrote: > There is an existing arm_smmu_cmdq_build_sync_cmd() so the driver should > call it at all places other than going through arm_smmu_cmdq_build_cmd() > separately. This helps the following patch that adds a CS_NONE quirk for > tegra241-cmdqv driver. > > Note that this changes the type of CMD_SYNC in __arm_smmu_cmdq_skip_err, > for ARM_SMMU_OPT_MSIPOLL=true cases, from previously a non-MSI one to an > MSI one that is proven to still work by a hacking test: > nvme: Adding to iommu group 10 > nvme: --------hacking----------- > arm-smmu-v3: unexpected global error reported (0x00000001), > this could be serious > arm-smmu-v3: CMDQ error (cons 0x01000022): Illegal command > arm-smmu-v3: skipping command in error state: > arm-smmu-v3: 0x0000000000000000 > arm-smmu-v3: 0x0000000000000000 > nvme: -------recovered---------- > nvme nvme0: 72/0/0 default/read/poll queues > nvme0n1: p1 p2 Nice > @@ -350,20 +340,23 @@ static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct arm_smmu_device *smmu) > static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu, > struct arm_smmu_queue *q, u32 prod) > { > - struct arm_smmu_cmdq_ent ent = { > - .opcode = CMDQ_OP_CMD_SYNC, > - }; > + memset(cmd, 0, 1 << CMDQ_ENT_SZ_SHIFT); The command would also benifit from its own type someday :\ Maybe this should just be cmd[1] = 0 ? Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
On Sun, May 12, 2024 at 12:39:43PM -0300, Jason Gunthorpe wrote: > On Tue, May 07, 2024 at 10:56:51PM -0700, Nicolin Chen wrote: > > There is an existing arm_smmu_cmdq_build_sync_cmd() so the driver should > > call it at all places other than going through arm_smmu_cmdq_build_cmd() > > separately. This helps the following patch that adds a CS_NONE quirk for > > tegra241-cmdqv driver. > > > > Note that this changes the type of CMD_SYNC in __arm_smmu_cmdq_skip_err, > > for ARM_SMMU_OPT_MSIPOLL=true cases, from previously a non-MSI one to an > > MSI one that is proven to still work by a hacking test: > > nvme: Adding to iommu group 10 > > nvme: --------hacking----------- > > arm-smmu-v3: unexpected global error reported (0x00000001), > > this could be serious > > arm-smmu-v3: CMDQ error (cons 0x01000022): Illegal command > > arm-smmu-v3: skipping command in error state: > > arm-smmu-v3: 0x0000000000000000 > > arm-smmu-v3: 0x0000000000000000 > > nvme: -------recovered---------- > > nvme nvme0: 72/0/0 default/read/poll queues > > nvme0n1: p1 p2 > > Nice > > > @@ -350,20 +340,23 @@ static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct arm_smmu_device *smmu) > > static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu, > > struct arm_smmu_queue *q, u32 prod) > > { > > - struct arm_smmu_cmdq_ent ent = { > > - .opcode = CMDQ_OP_CMD_SYNC, > > - }; > > + memset(cmd, 0, 1 << CMDQ_ENT_SZ_SHIFT); > > The command would also benifit from its own type someday :\ > > Maybe this should just be cmd[1] = 0 ? Yes. I will add that. > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Thanks! Nicolin
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index 3d2962f6dbda..569bca1a8a1f 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -325,16 +325,6 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent) cmd[0] |= FIELD_PREP(CMDQ_RESUME_0_RESP, ent->resume.resp); cmd[1] |= FIELD_PREP(CMDQ_RESUME_1_STAG, ent->resume.stag); break; - case CMDQ_OP_CMD_SYNC: - if (ent->sync.msiaddr) { - cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_IRQ); - cmd[1] |= ent->sync.msiaddr & CMDQ_SYNC_1_MSIADDR_MASK; - } else { - cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_SEV); - } - cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSH, ARM_SMMU_SH_ISH); - cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSIATTR, ARM_SMMU_MEMATTR_OIWB); - break; default: return -ENOENT; } @@ -350,20 +340,23 @@ static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct arm_smmu_device *smmu) static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu, struct arm_smmu_queue *q, u32 prod) { - struct arm_smmu_cmdq_ent ent = { - .opcode = CMDQ_OP_CMD_SYNC, - }; + memset(cmd, 0, 1 << CMDQ_ENT_SZ_SHIFT); + cmd[0] = FIELD_PREP(CMDQ_0_OP, CMDQ_OP_CMD_SYNC) | + FIELD_PREP(CMDQ_SYNC_0_MSH, ARM_SMMU_SH_ISH) | + FIELD_PREP(CMDQ_SYNC_0_MSIATTR, ARM_SMMU_MEMATTR_OIWB); + + if (!(smmu->options & ARM_SMMU_OPT_MSIPOLL)) { + cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_SEV); + return; + } /* * Beware that Hi16xx adds an extra 32 bits of goodness to its MSI * payload, so the write will zero the entire command on that platform. */ - if (smmu->options & ARM_SMMU_OPT_MSIPOLL) { - ent.sync.msiaddr = q->base_dma + Q_IDX(&q->llq, prod) * - q->ent_dwords * 8; - } - - arm_smmu_cmdq_build_cmd(cmd, &ent); + cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_IRQ); + cmd[1] = (q->base_dma + Q_IDX(&q->llq, prod) * q->ent_dwords * 8) & + CMDQ_SYNC_1_MSIADDR_MASK; } void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu, @@ -380,9 +373,6 @@ void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu, u64 cmd[CMDQ_ENT_DWORDS]; u32 cons = readl_relaxed(q->cons_reg); u32 idx = FIELD_GET(CMDQ_CONS_ERR, cons); - struct arm_smmu_cmdq_ent cmd_sync = { - .opcode = CMDQ_OP_CMD_SYNC, - }; dev_err(smmu->dev, "CMDQ error (cons 0x%08x): %s\n", cons, idx < ARRAY_SIZE(cerror_str) ? cerror_str[idx] : "Unknown"); @@ -416,7 +406,7 @@ void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu, dev_err(smmu->dev, "\t0x%016llx\n", (unsigned long long)cmd[i]); /* Convert the erroneous command into a CMD_SYNC */ - arm_smmu_cmdq_build_cmd(cmd, &cmd_sync); + arm_smmu_cmdq_build_sync_cmd(cmd, smmu, q, cons); queue_write(Q_ENT(q, cons), cmd, q->ent_dwords); } diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h index 8e4fbf4f50f3..180c0b1e0658 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -512,9 +512,6 @@ struct arm_smmu_cmdq_ent { } resume; #define CMDQ_OP_CMD_SYNC 0x46 - struct { - u64 msiaddr; - } sync; }; };
There is an existing arm_smmu_cmdq_build_sync_cmd() so the driver should call it at all places other than going through arm_smmu_cmdq_build_cmd() separately. This helps the following patch that adds a CS_NONE quirk for tegra241-cmdqv driver. Note that this changes the type of CMD_SYNC in __arm_smmu_cmdq_skip_err, for ARM_SMMU_OPT_MSIPOLL=true cases, from previously a non-MSI one to an MSI one that is proven to still work by a hacking test: nvme: Adding to iommu group 10 nvme: --------hacking----------- arm-smmu-v3: unexpected global error reported (0x00000001), this could be serious arm-smmu-v3: CMDQ error (cons 0x01000022): Illegal command arm-smmu-v3: skipping command in error state: arm-smmu-v3: 0x0000000000000000 arm-smmu-v3: 0x0000000000000000 nvme: -------recovered---------- nvme nvme0: 72/0/0 default/read/poll queues nvme0n1: p1 p2 Suggested-by: Jason Gunthorpe <jgg@nvidia.com> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 36 ++++++++------------- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 -- 2 files changed, 13 insertions(+), 26 deletions(-)