From: Thierry Reding <[email protected]>
In order to fully make use of the !IOMMU_API stub functions, make the
struct iommu_fwspec always available so that users of the stubs can keep
using the structure's internals without causing compile failures.
Signed-off-by: Thierry Reding <[email protected]>
---
Hi Joerg,
this is a rebased patch extracted from an ancient series that never
ended up getting applied:
https://lore.kernel.org/all/[email protected]/
You had already acked this particular patch, so maybe you can pick this
up. I've seen at least two discussions where this was brought up again,
so I figured it'd be worth sending this out again because it can help
remove a number of #ifdef blocks throughout the kernel.
include/linux/iommu.h | 39 +++++++++++++++++++--------------------
1 file changed, 19 insertions(+), 20 deletions(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index a325532aeab5..e3295c45d18f 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -173,6 +173,25 @@ enum iommu_dev_features {
#define IOMMU_PASID_INVALID (-1U)
+/**
+ * struct iommu_fwspec - per-device IOMMU instance data
+ * @ops: ops for this device's IOMMU
+ * @iommu_fwnode: firmware handle for this device's IOMMU
+ * @flags: IOMMU_FWSPEC_* flags
+ * @num_ids: number of associated device IDs
+ * @ids: IDs which this device may present to the IOMMU
+ */
+struct iommu_fwspec {
+ const struct iommu_ops *ops;
+ struct fwnode_handle *iommu_fwnode;
+ u32 flags;
+ unsigned int num_ids;
+ u32 ids[];
+};
+
+/* ATS is supported */
+#define IOMMU_FWSPEC_PCI_RC_ATS (1 << 0)
+
#ifdef CONFIG_IOMMU_API
/**
@@ -598,25 +617,6 @@ extern struct iommu_group *generic_device_group(struct device *dev);
/* FSL-MC device grouping function */
struct iommu_group *fsl_mc_device_group(struct device *dev);
-/**
- * struct iommu_fwspec - per-device IOMMU instance data
- * @ops: ops for this device's IOMMU
- * @iommu_fwnode: firmware handle for this device's IOMMU
- * @flags: IOMMU_FWSPEC_* flags
- * @num_ids: number of associated device IDs
- * @ids: IDs which this device may present to the IOMMU
- */
-struct iommu_fwspec {
- const struct iommu_ops *ops;
- struct fwnode_handle *iommu_fwnode;
- u32 flags;
- unsigned int num_ids;
- u32 ids[];
-};
-
-/* ATS is supported */
-#define IOMMU_FWSPEC_PCI_RC_ATS (1 << 0)
-
/**
* struct iommu_sva - handle to a device-mm bond
*/
@@ -680,7 +680,6 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group);
struct iommu_ops {};
struct iommu_group {};
-struct iommu_fwspec {};
struct iommu_device {};
struct iommu_fault_param {};
struct iommu_iotlb_gather {};
--
2.37.3
On Tue, 18 Oct 2022 at 12:51, Thierry Reding <[email protected]> wrote:
>
> From: Thierry Reding <[email protected]>
>
> In order to fully make use of the !IOMMU_API stub functions, make the
> struct iommu_fwspec always available so that users of the stubs can keep
> using the structure's internals without causing compile failures.
>
> Signed-off-by: Thierry Reding <[email protected]>
Reviewed-by: Ulf Hansson <[email protected]>
> ---
> Hi Joerg,
>
> this is a rebased patch extracted from an ancient series that never
> ended up getting applied:
>
> https://lore.kernel.org/all/[email protected]/
>
> You had already acked this particular patch, so maybe you can pick this
> up. I've seen at least two discussions where this was brought up again,
> so I figured it'd be worth sending this out again because it can help
> remove a number of #ifdef blocks throughout the kernel.
Yes, this would certainly help to improve the code. To me, it looks
like the current stub functions, like dev_iommu_fwspec_get() for
example, aren't really useful without $subject patch.
Note that, I have a pending patch for mmc that would benefit from
this. To prevent me from delaying that, an easy way forward, assuming
there are no objections of course, would be to send this for 6.1-rc.
>
> include/linux/iommu.h | 39 +++++++++++++++++++--------------------
> 1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index a325532aeab5..e3295c45d18f 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -173,6 +173,25 @@ enum iommu_dev_features {
>
> #define IOMMU_PASID_INVALID (-1U)
>
> +/**
> + * struct iommu_fwspec - per-device IOMMU instance data
> + * @ops: ops for this device's IOMMU
> + * @iommu_fwnode: firmware handle for this device's IOMMU
> + * @flags: IOMMU_FWSPEC_* flags
> + * @num_ids: number of associated device IDs
> + * @ids: IDs which this device may present to the IOMMU
> + */
> +struct iommu_fwspec {
> + const struct iommu_ops *ops;
> + struct fwnode_handle *iommu_fwnode;
> + u32 flags;
> + unsigned int num_ids;
> + u32 ids[];
> +};
> +
> +/* ATS is supported */
> +#define IOMMU_FWSPEC_PCI_RC_ATS (1 << 0)
> +
> #ifdef CONFIG_IOMMU_API
>
> /**
> @@ -598,25 +617,6 @@ extern struct iommu_group *generic_device_group(struct device *dev);
> /* FSL-MC device grouping function */
> struct iommu_group *fsl_mc_device_group(struct device *dev);
>
> -/**
> - * struct iommu_fwspec - per-device IOMMU instance data
> - * @ops: ops for this device's IOMMU
> - * @iommu_fwnode: firmware handle for this device's IOMMU
> - * @flags: IOMMU_FWSPEC_* flags
> - * @num_ids: number of associated device IDs
> - * @ids: IDs which this device may present to the IOMMU
> - */
> -struct iommu_fwspec {
> - const struct iommu_ops *ops;
> - struct fwnode_handle *iommu_fwnode;
> - u32 flags;
> - unsigned int num_ids;
> - u32 ids[];
> -};
> -
> -/* ATS is supported */
> -#define IOMMU_FWSPEC_PCI_RC_ATS (1 << 0)
> -
> /**
> * struct iommu_sva - handle to a device-mm bond
> */
> @@ -680,7 +680,6 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group);
>
> struct iommu_ops {};
> struct iommu_group {};
> -struct iommu_fwspec {};
> struct iommu_device {};
> struct iommu_fault_param {};
> struct iommu_iotlb_gather {};
> --
> 2.37.3
>
Kind regards
Uffe
On Thu, Oct 20, 2022 at 01:32:41PM +0200, Ulf Hansson wrote:
> On Tue, 18 Oct 2022 at 12:51, Thierry Reding <[email protected]> wrote:
> >
> > From: Thierry Reding <[email protected]>
> >
> > In order to fully make use of the !IOMMU_API stub functions, make the
> > struct iommu_fwspec always available so that users of the stubs can keep
> > using the structure's internals without causing compile failures.
> >
> > Signed-off-by: Thierry Reding <[email protected]>
>
> Reviewed-by: Ulf Hansson <[email protected]>
>
> > ---
> > Hi Joerg,
> >
> > this is a rebased patch extracted from an ancient series that never
> > ended up getting applied:
> >
> > https://lore.kernel.org/all/[email protected]/
> >
> > You had already acked this particular patch, so maybe you can pick this
> > up. I've seen at least two discussions where this was brought up again,
> > so I figured it'd be worth sending this out again because it can help
> > remove a number of #ifdef blocks throughout the kernel.
>
> Yes, this would certainly help to improve the code. To me, it looks
> like the current stub functions, like dev_iommu_fwspec_get() for
> example, aren't really useful without $subject patch.
>
> Note that, I have a pending patch for mmc that would benefit from
> this. To prevent me from delaying that, an easy way forward, assuming
> there are no objections of course, would be to send this for 6.1-rc.
Adding Prathamesh for visibility. Another alternative would be to
prepend this to Prathamesh's series with an Acked-by from Joerg.
Joerg, any preference on how to move forward with this?
Thierry
>
> >
> > include/linux/iommu.h | 39 +++++++++++++++++++--------------------
> > 1 file changed, 19 insertions(+), 20 deletions(-)
> >
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index a325532aeab5..e3295c45d18f 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -173,6 +173,25 @@ enum iommu_dev_features {
> >
> > #define IOMMU_PASID_INVALID (-1U)
> >
> > +/**
> > + * struct iommu_fwspec - per-device IOMMU instance data
> > + * @ops: ops for this device's IOMMU
> > + * @iommu_fwnode: firmware handle for this device's IOMMU
> > + * @flags: IOMMU_FWSPEC_* flags
> > + * @num_ids: number of associated device IDs
> > + * @ids: IDs which this device may present to the IOMMU
> > + */
> > +struct iommu_fwspec {
> > + const struct iommu_ops *ops;
> > + struct fwnode_handle *iommu_fwnode;
> > + u32 flags;
> > + unsigned int num_ids;
> > + u32 ids[];
> > +};
> > +
> > +/* ATS is supported */
> > +#define IOMMU_FWSPEC_PCI_RC_ATS (1 << 0)
> > +
> > #ifdef CONFIG_IOMMU_API
> >
> > /**
> > @@ -598,25 +617,6 @@ extern struct iommu_group *generic_device_group(struct device *dev);
> > /* FSL-MC device grouping function */
> > struct iommu_group *fsl_mc_device_group(struct device *dev);
> >
> > -/**
> > - * struct iommu_fwspec - per-device IOMMU instance data
> > - * @ops: ops for this device's IOMMU
> > - * @iommu_fwnode: firmware handle for this device's IOMMU
> > - * @flags: IOMMU_FWSPEC_* flags
> > - * @num_ids: number of associated device IDs
> > - * @ids: IDs which this device may present to the IOMMU
> > - */
> > -struct iommu_fwspec {
> > - const struct iommu_ops *ops;
> > - struct fwnode_handle *iommu_fwnode;
> > - u32 flags;
> > - unsigned int num_ids;
> > - u32 ids[];
> > -};
> > -
> > -/* ATS is supported */
> > -#define IOMMU_FWSPEC_PCI_RC_ATS (1 << 0)
> > -
> > /**
> > * struct iommu_sva - handle to a device-mm bond
> > */
> > @@ -680,7 +680,6 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group);
> >
> > struct iommu_ops {};
> > struct iommu_group {};
> > -struct iommu_fwspec {};
> > struct iommu_device {};
> > struct iommu_fault_param {};
> > struct iommu_iotlb_gather {};
> > --
> > 2.37.3
> >
>
> Kind regards
> Uffe
On Thu, 27 Oct 2022 at 11:11, Thierry Reding <[email protected]> wrote:
>
> On Thu, Oct 20, 2022 at 01:32:41PM +0200, Ulf Hansson wrote:
> > On Tue, 18 Oct 2022 at 12:51, Thierry Reding <[email protected]> wrote:
> > >
> > > From: Thierry Reding <[email protected]>
> > >
> > > In order to fully make use of the !IOMMU_API stub functions, make the
> > > struct iommu_fwspec always available so that users of the stubs can keep
> > > using the structure's internals without causing compile failures.
> > >
> > > Signed-off-by: Thierry Reding <[email protected]>
> >
> > Reviewed-by: Ulf Hansson <[email protected]>
> >
> > > ---
> > > Hi Joerg,
> > >
> > > this is a rebased patch extracted from an ancient series that never
> > > ended up getting applied:
> > >
> > > https://lore.kernel.org/all/[email protected]/
> > >
> > > You had already acked this particular patch, so maybe you can pick this
> > > up. I've seen at least two discussions where this was brought up again,
> > > so I figured it'd be worth sending this out again because it can help
> > > remove a number of #ifdef blocks throughout the kernel.
> >
> > Yes, this would certainly help to improve the code. To me, it looks
> > like the current stub functions, like dev_iommu_fwspec_get() for
> > example, aren't really useful without $subject patch.
> >
> > Note that, I have a pending patch for mmc that would benefit from
> > this. To prevent me from delaying that, an easy way forward, assuming
> > there are no objections of course, would be to send this for 6.1-rc.
>
> Adding Prathamesh for visibility. Another alternative would be to
> prepend this to Prathamesh's series with an Acked-by from Joerg.
Good idea!
I will then be awaiting a new version from Prathamesh's series, that
includes $subject patch too.
>
> Joerg, any preference on how to move forward with this?
>
> Thierry
>
Kind regards
Uffe
> >
> > >
> > > include/linux/iommu.h | 39 +++++++++++++++++++--------------------
> > > 1 file changed, 19 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > > index a325532aeab5..e3295c45d18f 100644
> > > --- a/include/linux/iommu.h
> > > +++ b/include/linux/iommu.h
> > > @@ -173,6 +173,25 @@ enum iommu_dev_features {
> > >
> > > #define IOMMU_PASID_INVALID (-1U)
> > >
> > > +/**
> > > + * struct iommu_fwspec - per-device IOMMU instance data
> > > + * @ops: ops for this device's IOMMU
> > > + * @iommu_fwnode: firmware handle for this device's IOMMU
> > > + * @flags: IOMMU_FWSPEC_* flags
> > > + * @num_ids: number of associated device IDs
> > > + * @ids: IDs which this device may present to the IOMMU
> > > + */
> > > +struct iommu_fwspec {
> > > + const struct iommu_ops *ops;
> > > + struct fwnode_handle *iommu_fwnode;
> > > + u32 flags;
> > > + unsigned int num_ids;
> > > + u32 ids[];
> > > +};
> > > +
> > > +/* ATS is supported */
> > > +#define IOMMU_FWSPEC_PCI_RC_ATS (1 << 0)
> > > +
> > > #ifdef CONFIG_IOMMU_API
> > >
> > > /**
> > > @@ -598,25 +617,6 @@ extern struct iommu_group *generic_device_group(struct device *dev);
> > > /* FSL-MC device grouping function */
> > > struct iommu_group *fsl_mc_device_group(struct device *dev);
> > >
> > > -/**
> > > - * struct iommu_fwspec - per-device IOMMU instance data
> > > - * @ops: ops for this device's IOMMU
> > > - * @iommu_fwnode: firmware handle for this device's IOMMU
> > > - * @flags: IOMMU_FWSPEC_* flags
> > > - * @num_ids: number of associated device IDs
> > > - * @ids: IDs which this device may present to the IOMMU
> > > - */
> > > -struct iommu_fwspec {
> > > - const struct iommu_ops *ops;
> > > - struct fwnode_handle *iommu_fwnode;
> > > - u32 flags;
> > > - unsigned int num_ids;
> > > - u32 ids[];
> > > -};
> > > -
> > > -/* ATS is supported */
> > > -#define IOMMU_FWSPEC_PCI_RC_ATS (1 << 0)
> > > -
> > > /**
> > > * struct iommu_sva - handle to a device-mm bond
> > > */
> > > @@ -680,7 +680,6 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group);
> > >
> > > struct iommu_ops {};
> > > struct iommu_group {};
> > > -struct iommu_fwspec {};
> > > struct iommu_device {};
> > > struct iommu_fault_param {};
> > > struct iommu_iotlb_gather {};
> > > --
> > > 2.37.3
> > >
> >
> > Kind regards
> > Uffe
This dummy implementation is useful to avoid a dependency on the
IOMMU_API Kconfig symbol in drivers that can optionally use the IOMMU
API.
In order to fully use this, also move the struct iommu_fwspec definition
out of the IOMMU_API protected region.
Signed-off-by: Thierry Reding <[email protected]>
Reviewed-by: Ulf Hansson <[email protected]>
---
include/linux/iommu.h | 39 +++++++++++++++++++--------------------
1 file changed, 19 insertions(+), 20 deletions(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index ea30f00dc145..afa829bc4356 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -173,6 +173,25 @@ enum iommu_dev_features {
#define IOMMU_PASID_INVALID (-1U)
+/**
+ * struct iommu_fwspec - per-device IOMMU instance data
+ * @ops: ops for this device's IOMMU
+ * @iommu_fwnode: firmware handle for this device's IOMMU
+ * @flags: IOMMU_FWSPEC_* flags
+ * @num_ids: number of associated device IDs
+ * @ids: IDs which this device may present to the IOMMU
+ */
+struct iommu_fwspec {
+ const struct iommu_ops *ops;
+ struct fwnode_handle *iommu_fwnode;
+ u32 flags;
+ unsigned int num_ids;
+ u32 ids[];
+};
+
+/* ATS is supported */
+#define IOMMU_FWSPEC_PCI_RC_ATS (1 << 0)
+
#ifdef CONFIG_IOMMU_API
/**
@@ -600,25 +619,6 @@ extern struct iommu_group *generic_device_group(struct device *dev);
/* FSL-MC device grouping function */
struct iommu_group *fsl_mc_device_group(struct device *dev);
-/**
- * struct iommu_fwspec - per-device IOMMU instance data
- * @ops: ops for this device's IOMMU
- * @iommu_fwnode: firmware handle for this device's IOMMU
- * @flags: IOMMU_FWSPEC_* flags
- * @num_ids: number of associated device IDs
- * @ids: IDs which this device may present to the IOMMU
- */
-struct iommu_fwspec {
- const struct iommu_ops *ops;
- struct fwnode_handle *iommu_fwnode;
- u32 flags;
- unsigned int num_ids;
- u32 ids[];
-};
-
-/* ATS is supported */
-#define IOMMU_FWSPEC_PCI_RC_ATS (1 << 0)
-
/**
* struct iommu_sva - handle to a device-mm bond
*/
@@ -682,7 +682,6 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group);
struct iommu_ops {};
struct iommu_group {};
-struct iommu_fwspec {};
struct iommu_device {};
struct iommu_fault_param {};
struct iommu_iotlb_gather {};
--
2.17.1
SMMU clients are supposed to program stream ID from
their respective address spaces instead of MC override.
Define NVQUIRK_PROGRAM_STREAMID and use it to program
SMMU stream ID from the SDMMC client address space.
Signed-off-by: Aniruddha TVS Rao <[email protected]>
Signed-off-by: Prathamesh Shete <[email protected]>
Acked-by: Adrian Hunter <[email protected]>
Acked-by: Thierry Reding <[email protected]>
---
drivers/mmc/host/sdhci-tegra.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index a6c5bbae77b4..e44060cceb68 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -25,6 +25,8 @@
#include <linux/mmc/slot-gpio.h>
#include <linux/gpio/consumer.h>
#include <linux/ktime.h>
+#include <linux/iommu.h>
+#include <linux/bitops.h>
#include <soc/tegra/common.h>
@@ -94,6 +96,8 @@
#define SDHCI_TEGRA_AUTO_CAL_STATUS 0x1ec
#define SDHCI_TEGRA_AUTO_CAL_ACTIVE BIT(31)
+#define SDHCI_TEGRA_CIF2AXI_CTRL_0 0x1fc
+
#define NVQUIRK_FORCE_SDHCI_SPEC_200 BIT(0)
#define NVQUIRK_ENABLE_BLOCK_GAP_DET BIT(1)
#define NVQUIRK_ENABLE_SDHCI_SPEC_300 BIT(2)
@@ -121,6 +125,7 @@
#define NVQUIRK_HAS_TMCLK BIT(10)
#define NVQUIRK_HAS_ANDROID_GPT_SECTOR BIT(11)
+#define NVQUIRK_PROGRAM_STREAMID BIT(12)
/* SDMMC CQE Base Address for Tegra Host Ver 4.1 and Higher */
#define SDHCI_TEGRA_CQE_BASE_ADDR 0xF000
@@ -177,6 +182,7 @@ struct sdhci_tegra {
bool enable_hwcq;
unsigned long curr_clk_rate;
u8 tuned_tap_delay;
+ u32 streamid;
};
static u16 tegra_sdhci_readw(struct sdhci_host *host, int reg)
@@ -1564,6 +1570,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra234 = {
NVQUIRK_DIS_CARD_CLK_CONFIG_TAP |
NVQUIRK_ENABLE_SDR50 |
NVQUIRK_ENABLE_SDR104 |
+ NVQUIRK_PROGRAM_STREAMID |
NVQUIRK_HAS_TMCLK,
.min_tap_delay = 95,
.max_tap_delay = 111,
@@ -1630,6 +1637,29 @@ static int sdhci_tegra_add_host(struct sdhci_host *host)
return ret;
}
+/* Program MC streamID for DMA transfers */
+static void program_stream_id(struct device *dev)
+{
+ struct sdhci_host *host = dev_get_drvdata(dev);
+ struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+ struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
+ struct iommu_fwspec *fwspec;
+
+ if (tegra_host->soc_data->nvquirks & NVQUIRK_PROGRAM_STREAMID) {
+ fwspec = dev_iommu_fwspec_get(dev);
+ if (!fwspec) {
+ dev_warn(mmc_dev(host->mmc),
+ "iommu fwspec is NULL, continue without stream ID\n");
+ } else {
+ tegra_host->streamid = fwspec->ids[0] & 0xff;
+ tegra_sdhci_writel(host, tegra_host->streamid |
+ FIELD_PREP(GENMASK(15, 8),
+ tegra_host->streamid),
+ SDHCI_TEGRA_CIF2AXI_CTRL_0);
+ }
+ }
+}
+
static int sdhci_tegra_probe(struct platform_device *pdev)
{
const struct sdhci_tegra_soc_data *soc_data;
@@ -1775,6 +1805,8 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
if (rc)
goto err_add_host;
+ program_stream_id(&pdev->dev);
+
return 0;
err_add_host:
@@ -1871,6 +1903,8 @@ static int sdhci_tegra_resume(struct device *dev)
if (ret)
return ret;
+ program_stream_id(dev);
+
ret = sdhci_resume_host(host);
if (ret)
goto disable_clk;
--
2.17.1
In case of error condition to avoid system crash
Tegra SDMMC controller requires CMD and DAT resets
issued together. SDHCI controller FSM goes into
bad state due to rapid SD card hot-plug event.
Issuing reset on the CMD FSM before DATA FSM results
in kernel panic, hence add support to issue CMD and
DAT resets together.
This is applicable to Tegra186 and later chips.
Signed-off-by: Aniruddha TVS Rao <[email protected]>
Signed-off-by: Prathamesh Shete <[email protected]>
Acked-by: Adrian Hunter <[email protected]>
Acked-by: Thierry Reding <[email protected]>
---
drivers/mmc/host/sdhci-tegra.c | 3 ++-
drivers/mmc/host/sdhci.c | 5 +++++
drivers/mmc/host/sdhci.h | 2 ++
3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index e44060cceb68..7cb3bf34a176 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -1531,7 +1531,8 @@ static const struct sdhci_pltfm_data sdhci_tegra186_pdata = {
SDHCI_QUIRK_NO_HISPD_BIT |
SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC |
SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
- .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
+ .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
+ SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER,
.ops = &tegra186_sdhci_ops,
};
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 2b5dda521b0e..8512a69f1aae 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -270,6 +270,11 @@ enum sdhci_reset_reason {
static void sdhci_reset_for_reason(struct sdhci_host *host, enum sdhci_reset_reason reason)
{
+ if (host->quirks2 &
+ SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER) {
+ sdhci_do_reset(host, SDHCI_RESET_CMD | SDHCI_RESET_DATA);
+ return;
+ }
switch (reason) {
case SDHCI_RESET_FOR_INIT:
sdhci_do_reset(host, SDHCI_RESET_CMD | SDHCI_RESET_DATA);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index d750c464bd1e..6a5766774b05 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -478,6 +478,8 @@ struct sdhci_host {
* block count.
*/
#define SDHCI_QUIRK2_USE_32BIT_BLK_CNT (1<<18)
+/* Issue CMD and DATA reset together */
+#define SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER (1<<19)
int irq; /* Device IRQ */
void __iomem *ioaddr; /* Mapped address */
--
2.17.1
Create new SoC data structure for Tegra234 platforms.
Additional features, tap value configurations are added/
updated for Tegra234 platform hence separate Tegra194 and
Tegra234 SoC data.
Signed-off-by: Aniruddha Tvs Rao <[email protected]>
Signed-off-by: Prathamesh Shete <[email protected]>
Acked-by: Adrian Hunter <[email protected]>
Acked-by: Thierry Reding <[email protected]>
---
drivers/mmc/host/sdhci-tegra.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 2d2d8260c681..a6c5bbae77b4 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -1556,7 +1556,21 @@ static const struct sdhci_tegra_soc_data soc_data_tegra194 = {
.max_tap_delay = 139,
};
+static const struct sdhci_tegra_soc_data soc_data_tegra234 = {
+ .pdata = &sdhci_tegra186_pdata,
+ .dma_mask = DMA_BIT_MASK(39),
+ .nvquirks = NVQUIRK_NEEDS_PAD_CONTROL |
+ NVQUIRK_HAS_PADCALIB |
+ NVQUIRK_DIS_CARD_CLK_CONFIG_TAP |
+ NVQUIRK_ENABLE_SDR50 |
+ NVQUIRK_ENABLE_SDR104 |
+ NVQUIRK_HAS_TMCLK,
+ .min_tap_delay = 95,
+ .max_tap_delay = 111,
+};
+
static const struct of_device_id sdhci_tegra_dt_match[] = {
+ { .compatible = "nvidia,tegra234-sdhci", .data = &soc_data_tegra234 },
{ .compatible = "nvidia,tegra194-sdhci", .data = &soc_data_tegra194 },
{ .compatible = "nvidia,tegra186-sdhci", .data = &soc_data_tegra186 },
{ .compatible = "nvidia,tegra210-sdhci", .data = &soc_data_tegra210 },
--
2.17.1
On Fri, 28 Oct 2022 at 15:02, Prathamesh Shete <[email protected]> wrote:
>
> This dummy implementation is useful to avoid a dependency on the
> IOMMU_API Kconfig symbol in drivers that can optionally use the IOMMU
> API.
>
> In order to fully use this, also move the struct iommu_fwspec definition
> out of the IOMMU_API protected region.
>
> Signed-off-by: Thierry Reding <[email protected]>
> Reviewed-by: Ulf Hansson <[email protected]>
It seems like you claimed authorship of the patch [1], that was
authored by Thierry and updated the commit message header.
Can you please restore the patch to its original that was posted by Thierry?
Kind regards
Uffe
[1]
https://lore.kernel.org/linux-mmc/[email protected]/
> ---
> include/linux/iommu.h | 39 +++++++++++++++++++--------------------
> 1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index ea30f00dc145..afa829bc4356 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -173,6 +173,25 @@ enum iommu_dev_features {
>
> #define IOMMU_PASID_INVALID (-1U)
>
> +/**
> + * struct iommu_fwspec - per-device IOMMU instance data
> + * @ops: ops for this device's IOMMU
> + * @iommu_fwnode: firmware handle for this device's IOMMU
> + * @flags: IOMMU_FWSPEC_* flags
> + * @num_ids: number of associated device IDs
> + * @ids: IDs which this device may present to the IOMMU
> + */
> +struct iommu_fwspec {
> + const struct iommu_ops *ops;
> + struct fwnode_handle *iommu_fwnode;
> + u32 flags;
> + unsigned int num_ids;
> + u32 ids[];
> +};
> +
> +/* ATS is supported */
> +#define IOMMU_FWSPEC_PCI_RC_ATS (1 << 0)
> +
> #ifdef CONFIG_IOMMU_API
>
> /**
> @@ -600,25 +619,6 @@ extern struct iommu_group *generic_device_group(struct device *dev);
> /* FSL-MC device grouping function */
> struct iommu_group *fsl_mc_device_group(struct device *dev);
>
> -/**
> - * struct iommu_fwspec - per-device IOMMU instance data
> - * @ops: ops for this device's IOMMU
> - * @iommu_fwnode: firmware handle for this device's IOMMU
> - * @flags: IOMMU_FWSPEC_* flags
> - * @num_ids: number of associated device IDs
> - * @ids: IDs which this device may present to the IOMMU
> - */
> -struct iommu_fwspec {
> - const struct iommu_ops *ops;
> - struct fwnode_handle *iommu_fwnode;
> - u32 flags;
> - unsigned int num_ids;
> - u32 ids[];
> -};
> -
> -/* ATS is supported */
> -#define IOMMU_FWSPEC_PCI_RC_ATS (1 << 0)
> -
> /**
> * struct iommu_sva - handle to a device-mm bond
> */
> @@ -682,7 +682,6 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group);
>
> struct iommu_ops {};
> struct iommu_group {};
> -struct iommu_fwspec {};
> struct iommu_device {};
> struct iommu_fault_param {};
> struct iommu_iotlb_gather {};
> --
> 2.17.1
>
In order to fully make use of the !IOMMU_API stub functions, make the
struct iommu_fwspec always available so that users of the stubs can keep
using the structure's internals without causing compile failures.
Signed-off-by: Thierry Reding <[email protected]>
Reviewed-by: Ulf Hansson <[email protected]>
---
include/linux/iommu.h | 39 +++++++++++++++++++--------------------
1 file changed, 19 insertions(+), 20 deletions(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index ea30f00dc145..afa829bc4356 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -173,6 +173,25 @@ enum iommu_dev_features {
#define IOMMU_PASID_INVALID (-1U)
+/**
+ * struct iommu_fwspec - per-device IOMMU instance data
+ * @ops: ops for this device's IOMMU
+ * @iommu_fwnode: firmware handle for this device's IOMMU
+ * @flags: IOMMU_FWSPEC_* flags
+ * @num_ids: number of associated device IDs
+ * @ids: IDs which this device may present to the IOMMU
+ */
+struct iommu_fwspec {
+ const struct iommu_ops *ops;
+ struct fwnode_handle *iommu_fwnode;
+ u32 flags;
+ unsigned int num_ids;
+ u32 ids[];
+};
+
+/* ATS is supported */
+#define IOMMU_FWSPEC_PCI_RC_ATS (1 << 0)
+
#ifdef CONFIG_IOMMU_API
/**
@@ -600,25 +619,6 @@ extern struct iommu_group *generic_device_group(struct device *dev);
/* FSL-MC device grouping function */
struct iommu_group *fsl_mc_device_group(struct device *dev);
-/**
- * struct iommu_fwspec - per-device IOMMU instance data
- * @ops: ops for this device's IOMMU
- * @iommu_fwnode: firmware handle for this device's IOMMU
- * @flags: IOMMU_FWSPEC_* flags
- * @num_ids: number of associated device IDs
- * @ids: IDs which this device may present to the IOMMU
- */
-struct iommu_fwspec {
- const struct iommu_ops *ops;
- struct fwnode_handle *iommu_fwnode;
- u32 flags;
- unsigned int num_ids;
- u32 ids[];
-};
-
-/* ATS is supported */
-#define IOMMU_FWSPEC_PCI_RC_ATS (1 << 0)
-
/**
* struct iommu_sva - handle to a device-mm bond
*/
@@ -682,7 +682,6 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group);
struct iommu_ops {};
struct iommu_group {};
-struct iommu_fwspec {};
struct iommu_device {};
struct iommu_fault_param {};
struct iommu_iotlb_gather {};
--
2.17.1
SMMU clients are supposed to program stream ID from
their respective address spaces instead of MC override.
Define NVQUIRK_PROGRAM_STREAMID and use it to program
SMMU stream ID from the SDMMC client address space.
Signed-off-by: Aniruddha TVS Rao <[email protected]>
Signed-off-by: Prathamesh Shete <[email protected]>
Acked-by: Adrian Hunter <[email protected]>
Acked-by: Thierry Reding <[email protected]>
---
drivers/mmc/host/sdhci-tegra.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index a6c5bbae77b4..e44060cceb68 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -25,6 +25,8 @@
#include <linux/mmc/slot-gpio.h>
#include <linux/gpio/consumer.h>
#include <linux/ktime.h>
+#include <linux/iommu.h>
+#include <linux/bitops.h>
#include <soc/tegra/common.h>
@@ -94,6 +96,8 @@
#define SDHCI_TEGRA_AUTO_CAL_STATUS 0x1ec
#define SDHCI_TEGRA_AUTO_CAL_ACTIVE BIT(31)
+#define SDHCI_TEGRA_CIF2AXI_CTRL_0 0x1fc
+
#define NVQUIRK_FORCE_SDHCI_SPEC_200 BIT(0)
#define NVQUIRK_ENABLE_BLOCK_GAP_DET BIT(1)
#define NVQUIRK_ENABLE_SDHCI_SPEC_300 BIT(2)
@@ -121,6 +125,7 @@
#define NVQUIRK_HAS_TMCLK BIT(10)
#define NVQUIRK_HAS_ANDROID_GPT_SECTOR BIT(11)
+#define NVQUIRK_PROGRAM_STREAMID BIT(12)
/* SDMMC CQE Base Address for Tegra Host Ver 4.1 and Higher */
#define SDHCI_TEGRA_CQE_BASE_ADDR 0xF000
@@ -177,6 +182,7 @@ struct sdhci_tegra {
bool enable_hwcq;
unsigned long curr_clk_rate;
u8 tuned_tap_delay;
+ u32 streamid;
};
static u16 tegra_sdhci_readw(struct sdhci_host *host, int reg)
@@ -1564,6 +1570,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra234 = {
NVQUIRK_DIS_CARD_CLK_CONFIG_TAP |
NVQUIRK_ENABLE_SDR50 |
NVQUIRK_ENABLE_SDR104 |
+ NVQUIRK_PROGRAM_STREAMID |
NVQUIRK_HAS_TMCLK,
.min_tap_delay = 95,
.max_tap_delay = 111,
@@ -1630,6 +1637,29 @@ static int sdhci_tegra_add_host(struct sdhci_host *host)
return ret;
}
+/* Program MC streamID for DMA transfers */
+static void program_stream_id(struct device *dev)
+{
+ struct sdhci_host *host = dev_get_drvdata(dev);
+ struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+ struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
+ struct iommu_fwspec *fwspec;
+
+ if (tegra_host->soc_data->nvquirks & NVQUIRK_PROGRAM_STREAMID) {
+ fwspec = dev_iommu_fwspec_get(dev);
+ if (!fwspec) {
+ dev_warn(mmc_dev(host->mmc),
+ "iommu fwspec is NULL, continue without stream ID\n");
+ } else {
+ tegra_host->streamid = fwspec->ids[0] & 0xff;
+ tegra_sdhci_writel(host, tegra_host->streamid |
+ FIELD_PREP(GENMASK(15, 8),
+ tegra_host->streamid),
+ SDHCI_TEGRA_CIF2AXI_CTRL_0);
+ }
+ }
+}
+
static int sdhci_tegra_probe(struct platform_device *pdev)
{
const struct sdhci_tegra_soc_data *soc_data;
@@ -1775,6 +1805,8 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
if (rc)
goto err_add_host;
+ program_stream_id(&pdev->dev);
+
return 0;
err_add_host:
@@ -1871,6 +1903,8 @@ static int sdhci_tegra_resume(struct device *dev)
if (ret)
return ret;
+ program_stream_id(dev);
+
ret = sdhci_resume_host(host);
if (ret)
goto disable_clk;
--
2.17.1
Create new SoC data structure for Tegra234 platforms.
Additional features, tap value configurations are added/
updated for Tegra234 platform hence separate Tegra194 and
Tegra234 SoC data.
Signed-off-by: Aniruddha Tvs Rao <[email protected]>
Signed-off-by: Prathamesh Shete <[email protected]>
Acked-by: Adrian Hunter <[email protected]>
Acked-by: Thierry Reding <[email protected]>
---
drivers/mmc/host/sdhci-tegra.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 2d2d8260c681..a6c5bbae77b4 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -1556,7 +1556,21 @@ static const struct sdhci_tegra_soc_data soc_data_tegra194 = {
.max_tap_delay = 139,
};
+static const struct sdhci_tegra_soc_data soc_data_tegra234 = {
+ .pdata = &sdhci_tegra186_pdata,
+ .dma_mask = DMA_BIT_MASK(39),
+ .nvquirks = NVQUIRK_NEEDS_PAD_CONTROL |
+ NVQUIRK_HAS_PADCALIB |
+ NVQUIRK_DIS_CARD_CLK_CONFIG_TAP |
+ NVQUIRK_ENABLE_SDR50 |
+ NVQUIRK_ENABLE_SDR104 |
+ NVQUIRK_HAS_TMCLK,
+ .min_tap_delay = 95,
+ .max_tap_delay = 111,
+};
+
static const struct of_device_id sdhci_tegra_dt_match[] = {
+ { .compatible = "nvidia,tegra234-sdhci", .data = &soc_data_tegra234 },
{ .compatible = "nvidia,tegra194-sdhci", .data = &soc_data_tegra194 },
{ .compatible = "nvidia,tegra186-sdhci", .data = &soc_data_tegra186 },
{ .compatible = "nvidia,tegra210-sdhci", .data = &soc_data_tegra210 },
--
2.17.1
In case of error condition to avoid system crash
Tegra SDMMC controller requires CMD and DAT resets
issued together. SDHCI controller FSM goes into
bad state due to rapid SD card hot-plug event.
Issuing reset on the CMD FSM before DATA FSM results
in kernel panic, hence add support to issue CMD and
DAT resets together.
This is applicable to Tegra186 and later chips.
Signed-off-by: Aniruddha TVS Rao <[email protected]>
Signed-off-by: Prathamesh Shete <[email protected]>
Acked-by: Adrian Hunter <[email protected]>
Acked-by: Thierry Reding <[email protected]>
---
drivers/mmc/host/sdhci-tegra.c | 3 ++-
drivers/mmc/host/sdhci.c | 5 +++++
drivers/mmc/host/sdhci.h | 2 ++
3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index e44060cceb68..7cb3bf34a176 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -1531,7 +1531,8 @@ static const struct sdhci_pltfm_data sdhci_tegra186_pdata = {
SDHCI_QUIRK_NO_HISPD_BIT |
SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC |
SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
- .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
+ .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
+ SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER,
.ops = &tegra186_sdhci_ops,
};
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 2b5dda521b0e..8512a69f1aae 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -270,6 +270,11 @@ enum sdhci_reset_reason {
static void sdhci_reset_for_reason(struct sdhci_host *host, enum sdhci_reset_reason reason)
{
+ if (host->quirks2 &
+ SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER) {
+ sdhci_do_reset(host, SDHCI_RESET_CMD | SDHCI_RESET_DATA);
+ return;
+ }
switch (reason) {
case SDHCI_RESET_FOR_INIT:
sdhci_do_reset(host, SDHCI_RESET_CMD | SDHCI_RESET_DATA);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index d750c464bd1e..6a5766774b05 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -478,6 +478,8 @@ struct sdhci_host {
* block count.
*/
#define SDHCI_QUIRK2_USE_32BIT_BLK_CNT (1<<18)
+/* Issue CMD and DATA reset together */
+#define SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER (1<<19)
int irq; /* Device IRQ */
void __iomem *ioaddr; /* Mapped address */
--
2.17.1
On Thu, 3 Nov 2022 at 05:39, Prathamesh Shete <[email protected]> wrote:
>
> In order to fully make use of the !IOMMU_API stub functions, make the
> struct iommu_fwspec always available so that users of the stubs can keep
> using the structure's internals without causing compile failures.
>
> Signed-off-by: Thierry Reding <[email protected]>
> Reviewed-by: Ulf Hansson <[email protected]>
Joerg, Will, Robin - may I have an ack from some of you for $subject
patch, so I can funnel it via my mmc tree for v6.2?
Kind regards
Uffe
> ---
> include/linux/iommu.h | 39 +++++++++++++++++++--------------------
> 1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index ea30f00dc145..afa829bc4356 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -173,6 +173,25 @@ enum iommu_dev_features {
>
> #define IOMMU_PASID_INVALID (-1U)
>
> +/**
> + * struct iommu_fwspec - per-device IOMMU instance data
> + * @ops: ops for this device's IOMMU
> + * @iommu_fwnode: firmware handle for this device's IOMMU
> + * @flags: IOMMU_FWSPEC_* flags
> + * @num_ids: number of associated device IDs
> + * @ids: IDs which this device may present to the IOMMU
> + */
> +struct iommu_fwspec {
> + const struct iommu_ops *ops;
> + struct fwnode_handle *iommu_fwnode;
> + u32 flags;
> + unsigned int num_ids;
> + u32 ids[];
> +};
> +
> +/* ATS is supported */
> +#define IOMMU_FWSPEC_PCI_RC_ATS (1 << 0)
> +
> #ifdef CONFIG_IOMMU_API
>
> /**
> @@ -600,25 +619,6 @@ extern struct iommu_group *generic_device_group(struct device *dev);
> /* FSL-MC device grouping function */
> struct iommu_group *fsl_mc_device_group(struct device *dev);
>
> -/**
> - * struct iommu_fwspec - per-device IOMMU instance data
> - * @ops: ops for this device's IOMMU
> - * @iommu_fwnode: firmware handle for this device's IOMMU
> - * @flags: IOMMU_FWSPEC_* flags
> - * @num_ids: number of associated device IDs
> - * @ids: IDs which this device may present to the IOMMU
> - */
> -struct iommu_fwspec {
> - const struct iommu_ops *ops;
> - struct fwnode_handle *iommu_fwnode;
> - u32 flags;
> - unsigned int num_ids;
> - u32 ids[];
> -};
> -
> -/* ATS is supported */
> -#define IOMMU_FWSPEC_PCI_RC_ATS (1 << 0)
> -
> /**
> * struct iommu_sva - handle to a device-mm bond
> */
> @@ -682,7 +682,6 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group);
>
> struct iommu_ops {};
> struct iommu_group {};
> -struct iommu_fwspec {};
> struct iommu_device {};
> struct iommu_fault_param {};
> struct iommu_iotlb_gather {};
> --
> 2.17.1
>
On 2022-11-03 04:38, Prathamesh Shete wrote:
> SMMU clients are supposed to program stream ID from
> their respective address spaces instead of MC override.
> Define NVQUIRK_PROGRAM_STREAMID and use it to program
> SMMU stream ID from the SDMMC client address space.
>
> Signed-off-by: Aniruddha TVS Rao <[email protected]>
> Signed-off-by: Prathamesh Shete <[email protected]>
> Acked-by: Adrian Hunter <[email protected]>
> Acked-by: Thierry Reding <[email protected]>
> ---
> drivers/mmc/host/sdhci-tegra.c | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> index a6c5bbae77b4..e44060cceb68 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -25,6 +25,8 @@
> #include <linux/mmc/slot-gpio.h>
> #include <linux/gpio/consumer.h>
> #include <linux/ktime.h>
> +#include <linux/iommu.h>
> +#include <linux/bitops.h>
>
> #include <soc/tegra/common.h>
>
> @@ -94,6 +96,8 @@
> #define SDHCI_TEGRA_AUTO_CAL_STATUS 0x1ec
> #define SDHCI_TEGRA_AUTO_CAL_ACTIVE BIT(31)
>
> +#define SDHCI_TEGRA_CIF2AXI_CTRL_0 0x1fc
> +
> #define NVQUIRK_FORCE_SDHCI_SPEC_200 BIT(0)
> #define NVQUIRK_ENABLE_BLOCK_GAP_DET BIT(1)
> #define NVQUIRK_ENABLE_SDHCI_SPEC_300 BIT(2)
> @@ -121,6 +125,7 @@
> #define NVQUIRK_HAS_TMCLK BIT(10)
>
> #define NVQUIRK_HAS_ANDROID_GPT_SECTOR BIT(11)
> +#define NVQUIRK_PROGRAM_STREAMID BIT(12)
>
> /* SDMMC CQE Base Address for Tegra Host Ver 4.1 and Higher */
> #define SDHCI_TEGRA_CQE_BASE_ADDR 0xF000
> @@ -177,6 +182,7 @@ struct sdhci_tegra {
> bool enable_hwcq;
> unsigned long curr_clk_rate;
> u8 tuned_tap_delay;
> + u32 streamid;
> };
>
> static u16 tegra_sdhci_readw(struct sdhci_host *host, int reg)
> @@ -1564,6 +1570,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra234 = {
> NVQUIRK_DIS_CARD_CLK_CONFIG_TAP |
> NVQUIRK_ENABLE_SDR50 |
> NVQUIRK_ENABLE_SDR104 |
> + NVQUIRK_PROGRAM_STREAMID |
> NVQUIRK_HAS_TMCLK,
> .min_tap_delay = 95,
> .max_tap_delay = 111,
> @@ -1630,6 +1637,29 @@ static int sdhci_tegra_add_host(struct sdhci_host *host)
> return ret;
> }
>
> +/* Program MC streamID for DMA transfers */
> +static void program_stream_id(struct device *dev)
> +{
> + struct sdhci_host *host = dev_get_drvdata(dev);
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
> + struct iommu_fwspec *fwspec;
> +
> + if (tegra_host->soc_data->nvquirks & NVQUIRK_PROGRAM_STREAMID) {
> + fwspec = dev_iommu_fwspec_get(dev);
> + if (!fwspec) {
> + dev_warn(mmc_dev(host->mmc),
> + "iommu fwspec is NULL, continue without stream ID\n");
Not sure that really warrants a warning - if there's no IOMMU driver
present then StreamIDs are going to be irrelevant anyway. And repeating
the same warning on every PM resume seems even less useful.
> + } else {
> + tegra_host->streamid = fwspec->ids[0] & 0xff;
> + tegra_sdhci_writel(host, tegra_host->streamid |
Why bother storing streamid in tegra_host if it's never consumed from
anywhere other than the same place it's assigned?
Robin.
> + FIELD_PREP(GENMASK(15, 8),
> + tegra_host->streamid),
> + SDHCI_TEGRA_CIF2AXI_CTRL_0);
> + }
> + }
> +}
> +
> static int sdhci_tegra_probe(struct platform_device *pdev)
> {
> const struct sdhci_tegra_soc_data *soc_data;
> @@ -1775,6 +1805,8 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
> if (rc)
> goto err_add_host;
>
> + program_stream_id(&pdev->dev);
> +
> return 0;
>
> err_add_host:
> @@ -1871,6 +1903,8 @@ static int sdhci_tegra_resume(struct device *dev)
> if (ret)
> return ret;
>
> + program_stream_id(dev);
> +
> ret = sdhci_resume_host(host);
> if (ret)
> goto disable_clk;
On 2022-11-03 04:38, Prathamesh Shete wrote:
> In order to fully make use of the !IOMMU_API stub functions, make the
> struct iommu_fwspec always available so that users of the stubs can keep
> using the structure's internals without causing compile failures.
I'm really in two minds about this... fwspecs are an internal detail of
the IOMMU API that are meant to be private between individual drivers
and firmware code, so anything poking at them arguably does and should
depend on CONFIG_IOMMU_API. It looks like the stub for
dev_iommu_fwspec_get() was only added for the sake of one driver that
was misusing it where it really wanted device_iommu_mapped(), and has
since been fixed, so if anything my preference would be to remove that
stub :/
I don't technically have much objection to this patch in isolation, but
what I don't like is the direction of travel it implies. I see the
anti-pattern is only spread across Tegra drivers, making Tegra-specific
assumptions, so in my view the best answer would be to abstract that
fwpsec dependency into a single Tegra-specific helper, which would
better represent the nature of what's really going on here.
Thanks,
Robin.
> Signed-off-by: Thierry Reding <[email protected]>
> Reviewed-by: Ulf Hansson <[email protected]>
> ---
> include/linux/iommu.h | 39 +++++++++++++++++++--------------------
> 1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index ea30f00dc145..afa829bc4356 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -173,6 +173,25 @@ enum iommu_dev_features {
>
> #define IOMMU_PASID_INVALID (-1U)
>
> +/**
> + * struct iommu_fwspec - per-device IOMMU instance data
> + * @ops: ops for this device's IOMMU
> + * @iommu_fwnode: firmware handle for this device's IOMMU
> + * @flags: IOMMU_FWSPEC_* flags
> + * @num_ids: number of associated device IDs
> + * @ids: IDs which this device may present to the IOMMU
> + */
> +struct iommu_fwspec {
> + const struct iommu_ops *ops;
> + struct fwnode_handle *iommu_fwnode;
> + u32 flags;
> + unsigned int num_ids;
> + u32 ids[];
> +};
> +
> +/* ATS is supported */
> +#define IOMMU_FWSPEC_PCI_RC_ATS (1 << 0)
> +
> #ifdef CONFIG_IOMMU_API
>
> /**
> @@ -600,25 +619,6 @@ extern struct iommu_group *generic_device_group(struct device *dev);
> /* FSL-MC device grouping function */
> struct iommu_group *fsl_mc_device_group(struct device *dev);
>
> -/**
> - * struct iommu_fwspec - per-device IOMMU instance data
> - * @ops: ops for this device's IOMMU
> - * @iommu_fwnode: firmware handle for this device's IOMMU
> - * @flags: IOMMU_FWSPEC_* flags
> - * @num_ids: number of associated device IDs
> - * @ids: IDs which this device may present to the IOMMU
> - */
> -struct iommu_fwspec {
> - const struct iommu_ops *ops;
> - struct fwnode_handle *iommu_fwnode;
> - u32 flags;
> - unsigned int num_ids;
> - u32 ids[];
> -};
> -
> -/* ATS is supported */
> -#define IOMMU_FWSPEC_PCI_RC_ATS (1 << 0)
> -
> /**
> * struct iommu_sva - handle to a device-mm bond
> */
> @@ -682,7 +682,6 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group);
>
> struct iommu_ops {};
> struct iommu_group {};
> -struct iommu_fwspec {};
> struct iommu_device {};
> struct iommu_fault_param {};
> struct iommu_iotlb_gather {};
On Thu, Oct 27, 2022 at 11:11:24AM +0200, Thierry Reding wrote:
> Adding Prathamesh for visibility. Another alternative would be to
> prepend this to Prathamesh's series with an Acked-by from Joerg.
>
> Joerg, any preference on how to move forward with this?
Sorry, missed the discussion until now. Adding this patch to
Prathamesh's series is fine with me. Feel free to add my
Acked-by: Joerg Roedel <[email protected]>
On Thu, Nov 03, 2022 at 12:23:20PM +0000, Robin Murphy wrote:
> On 2022-11-03 04:38, Prathamesh Shete wrote:
> > In order to fully make use of the !IOMMU_API stub functions, make the
> > struct iommu_fwspec always available so that users of the stubs can keep
> > using the structure's internals without causing compile failures.
>
> I'm really in two minds about this... fwspecs are an internal detail of the
> IOMMU API that are meant to be private between individual drivers and
> firmware code, so anything poking at them arguably does and should depend on
> CONFIG_IOMMU_API. It looks like the stub for dev_iommu_fwspec_get() was only
> added for the sake of one driver that was misusing it where it really wanted
> device_iommu_mapped(), and has since been fixed, so if anything my
> preference would be to remove that stub :/
Tegra has been using this type of weak dependency on IOMMU_API mainly in
order to allow building without the IOMMU support on some old platforms
where people may actually care about the kernel size (Tegra20 systems
were sometimes severely constrained and don't have anything that we'd
call an IOMMU today).
We have similar stubs in place for most other major subsystems in order
to allow code to simply compile out if the subsystem is disabled, which
is quite convenient for sharing code between platforms that may want a
given feature and other platforms that may not want it, without causing
too much of a hassle with compile-testing.
> I don't technically have much objection to this patch in isolation, but what
> I don't like is the direction of travel it implies. I see the anti-pattern
> is only spread across Tegra drivers, making Tegra-specific assumptions, so
> in my view the best answer would be to abstract that fwpsec dependency into
> a single Tegra-specific helper, which would better represent the nature of
> what's really going on here.
I don't see how this is an anti-pattern. It might not be common for
drivers to need to reach into iommu_fwspec, so that might indeed be
specific to Tegra (for whatever reason our IP seems to want extra
flexibility), but the general pattern of using stubs is wide-spread,
so I don't see why IOMMU_API would need to be special.
Of course we could get rid of the stubs and make these hard
dependencies, but I suspect some people may then get mad that they can
no longer disable the IOMMU dependency.
Thierry
On Thu, 3 Nov 2022 at 15:01, Thierry Reding <[email protected]> wrote:
>
> On Thu, Nov 03, 2022 at 12:23:20PM +0000, Robin Murphy wrote:
> > On 2022-11-03 04:38, Prathamesh Shete wrote:
> > > In order to fully make use of the !IOMMU_API stub functions, make the
> > > struct iommu_fwspec always available so that users of the stubs can keep
> > > using the structure's internals without causing compile failures.
> >
> > I'm really in two minds about this... fwspecs are an internal detail of the
> > IOMMU API that are meant to be private between individual drivers and
> > firmware code, so anything poking at them arguably does and should depend on
> > CONFIG_IOMMU_API. It looks like the stub for dev_iommu_fwspec_get() was only
> > added for the sake of one driver that was misusing it where it really wanted
> > device_iommu_mapped(), and has since been fixed, so if anything my
> > preference would be to remove that stub :/
>
> Tegra has been using this type of weak dependency on IOMMU_API mainly in
> order to allow building without the IOMMU support on some old platforms
> where people may actually care about the kernel size (Tegra20 systems
> were sometimes severely constrained and don't have anything that we'd
> call an IOMMU today).
>
> We have similar stubs in place for most other major subsystems in order
> to allow code to simply compile out if the subsystem is disabled, which
> is quite convenient for sharing code between platforms that may want a
> given feature and other platforms that may not want it, without causing
> too much of a hassle with compile-testing.
I agree with the above.
Moreover, the stubs make the code more portable/scalable and so it
becomes easier to maintain.
>
> > I don't technically have much objection to this patch in isolation, but what
> > I don't like is the direction of travel it implies. I see the anti-pattern
> > is only spread across Tegra drivers, making Tegra-specific assumptions, so
> > in my view the best answer would be to abstract that fwpsec dependency into
> > a single Tegra-specific helper, which would better represent the nature of
> > what's really going on here.
>
> I don't see how this is an anti-pattern. It might not be common for
> drivers to need to reach into iommu_fwspec, so that might indeed be
> specific to Tegra (for whatever reason our IP seems to want extra
> flexibility), but the general pattern of using stubs is wide-spread,
> so I don't see why IOMMU_API would need to be special.
Again, I agree.
Moreover, a "git grep CONFIG_IOMMU_API" indicates that the problem
isn't specific to Tegra. The "#ifdef CONFIG_IOMMU_API" seems to be
sprinkled across the kernel. I think it would be nice if we could
improve the situation. So far, using stubs along with what the
$subject patch proposes, seems to me to be the best approach.
[...]
Kind regards
Uffe
On 2022-11-03 14:55, Ulf Hansson wrote:
> On Thu, 3 Nov 2022 at 15:01, Thierry Reding <[email protected]> wrote:
>>
>> On Thu, Nov 03, 2022 at 12:23:20PM +0000, Robin Murphy wrote:
>>> On 2022-11-03 04:38, Prathamesh Shete wrote:
>>>> In order to fully make use of the !IOMMU_API stub functions, make the
>>>> struct iommu_fwspec always available so that users of the stubs can keep
>>>> using the structure's internals without causing compile failures.
>>>
>>> I'm really in two minds about this... fwspecs are an internal detail of the
>>> IOMMU API that are meant to be private between individual drivers and
>>> firmware code, so anything poking at them arguably does and should depend on
>>> CONFIG_IOMMU_API. It looks like the stub for dev_iommu_fwspec_get() was only
>>> added for the sake of one driver that was misusing it where it really wanted
>>> device_iommu_mapped(), and has since been fixed, so if anything my
>>> preference would be to remove that stub :/
>>
>> Tegra has been using this type of weak dependency on IOMMU_API mainly in
>> order to allow building without the IOMMU support on some old platforms
>> where people may actually care about the kernel size (Tegra20 systems
>> were sometimes severely constrained and don't have anything that we'd
>> call an IOMMU today).
>>
>> We have similar stubs in place for most other major subsystems in order
>> to allow code to simply compile out if the subsystem is disabled, which
>> is quite convenient for sharing code between platforms that may want a
>> given feature and other platforms that may not want it, without causing
>> too much of a hassle with compile-testing.
>
> I agree with the above.
>
> Moreover, the stubs make the code more portable/scalable and so it
> becomes easier to maintain.
Are you suggesting that having the same thing open-coded slightly
differently (with bugs) in 8 different places is somehow more
maintainable than abstracting it into a single centralised implementation?
Is it "easier to maintain" when already seemingly every thing I try to
clean up or refactor in the IOMMU API at the moment is stymied by
finding Tegra drivers doing unexpected (and often questionable) things?
Is it "more scalable" to make it even easier for people to copy
questionable code without a second thought, leaving API maintainers to
play an ever-expanding game of whack-a-mole to clean it up? No. No it
chuffing well isn't :(
>>> I don't technically have much objection to this patch in isolation, but what
>>> I don't like is the direction of travel it implies. I see the anti-pattern
>>> is only spread across Tegra drivers, making Tegra-specific assumptions, so
>>> in my view the best answer would be to abstract that fwpsec dependency into
>>> a single Tegra-specific helper, which would better represent the nature of
>>> what's really going on here.
>>
>> I don't see how this is an anti-pattern. It might not be common for
>> drivers to need to reach into iommu_fwspec, so that might indeed be
>> specific to Tegra (for whatever reason our IP seems to want extra
>> flexibility), but the general pattern of using stubs is wide-spread,
>> so I don't see why IOMMU_API would need to be special.
>
> Again, I agree.
The anti-pattern is reaching into some other driver's private data
assuming a particular format, with zero indication of the huge degree of
assumption involved, and half the time not even checking that what's
being dereferenced is valid.
> Moreover, a "git grep CONFIG_IOMMU_API" indicates that the problem
> isn't specific to Tegra. The "#ifdef CONFIG_IOMMU_API" seems to be
> sprinkled across the kernel. I think it would be nice if we could
> improve the situation. So far, using stubs along with what the
> $subject patch proposes, seems to me to be the best approach.
Yes, there is plenty of code through the tree that is only relevant to
the IOMMU API and would be a complete waste of space without it, that is
not the point in question here. Grep for dev_iommu_fwspec_get; outside
drivers/iommu, the only users are IOMMU-API-specific parts of ACPI code,
as intended, plus 8 random Tegra drivers.
Now, there does happen to be a tacit contract between the ACPI IORT code
and the Arm SMMU drivers for how SMMU StreamIDs are encoded in their
respective fwspecs, but it was never intended for wider consumption. If
Tegra drivers want to have a special relationship with arm-smmu then
fair enough, but they can do the same as MSM and formalise it somewhere
that the SMMU driver maintainers are at least aware of, rather than
holding the whole generic IOMMU API hostage.
Since apparently it wasn't clear, what I was proposing is a driver
helper at least something like this:
int tegra_arm_smmu_streamid(struct device *dev)
{
#ifdef CONFIG_IOMMU_API
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev)
if (fwspec && fwspec->num_ids == 1)
return fwspec->ids[0] & 0xffff;
#endif
return -EINVAL;
}
Now THAT is scalable and maintainable; any number of random drivers can
call it without any preconditions, it's a lot clearer what's going on,
and I won't have to swear profusely while herding patches through half a
dozen different trees if, when my ops rework gets to the point of
refactoring iommu_fwspec with dev_iommu, it ends up changing anything
significant.
Thanks,
Robin.
On Thu, Nov 03, 2022 at 05:35:19PM +0000, Robin Murphy wrote:
> On 2022-11-03 14:55, Ulf Hansson wrote:
> > On Thu, 3 Nov 2022 at 15:01, Thierry Reding <[email protected]> wrote:
> > >
> > > On Thu, Nov 03, 2022 at 12:23:20PM +0000, Robin Murphy wrote:
> > > > On 2022-11-03 04:38, Prathamesh Shete wrote:
> > > > > In order to fully make use of the !IOMMU_API stub functions, make the
> > > > > struct iommu_fwspec always available so that users of the stubs can keep
> > > > > using the structure's internals without causing compile failures.
> > > >
> > > > I'm really in two minds about this... fwspecs are an internal detail of the
> > > > IOMMU API that are meant to be private between individual drivers and
> > > > firmware code, so anything poking at them arguably does and should depend on
> > > > CONFIG_IOMMU_API. It looks like the stub for dev_iommu_fwspec_get() was only
> > > > added for the sake of one driver that was misusing it where it really wanted
> > > > device_iommu_mapped(), and has since been fixed, so if anything my
> > > > preference would be to remove that stub :/
> > >
> > > Tegra has been using this type of weak dependency on IOMMU_API mainly in
> > > order to allow building without the IOMMU support on some old platforms
> > > where people may actually care about the kernel size (Tegra20 systems
> > > were sometimes severely constrained and don't have anything that we'd
> > > call an IOMMU today).
> > >
> > > We have similar stubs in place for most other major subsystems in order
> > > to allow code to simply compile out if the subsystem is disabled, which
> > > is quite convenient for sharing code between platforms that may want a
> > > given feature and other platforms that may not want it, without causing
> > > too much of a hassle with compile-testing.
> >
> > I agree with the above.
> >
> > Moreover, the stubs make the code more portable/scalable and so it
> > becomes easier to maintain.
>
> Are you suggesting that having the same thing open-coded slightly
> differently (with bugs) in 8 different places is somehow more maintainable
> than abstracting it into a single centralised implementation?
>
> Is it "easier to maintain" when already seemingly every thing I try to clean
> up or refactor in the IOMMU API at the moment is stymied by finding Tegra
> drivers doing unexpected (and often questionable) things? Is it "more
> scalable" to make it even easier for people to copy questionable code
> without a second thought, leaving API maintainers to play an ever-expanding
> game of whack-a-mole to clean it up? No. No it chuffing well isn't :(
>
> > > > I don't technically have much objection to this patch in isolation, but what
> > > > I don't like is the direction of travel it implies. I see the anti-pattern
> > > > is only spread across Tegra drivers, making Tegra-specific assumptions, so
> > > > in my view the best answer would be to abstract that fwpsec dependency into
> > > > a single Tegra-specific helper, which would better represent the nature of
> > > > what's really going on here.
> > >
> > > I don't see how this is an anti-pattern. It might not be common for
> > > drivers to need to reach into iommu_fwspec, so that might indeed be
> > > specific to Tegra (for whatever reason our IP seems to want extra
> > > flexibility), but the general pattern of using stubs is wide-spread,
> > > so I don't see why IOMMU_API would need to be special.
> >
> > Again, I agree.
>
> The anti-pattern is reaching into some other driver's private data assuming
> a particular format, with zero indication of the huge degree of assumption
> involved, and half the time not even checking that what's being dereferenced
> is valid.
If this is really driver private data that nobody else should be
accessing, then this certainly is lacking documentation. And quite
frankly, perhaps it should really be hidden from other drivers in
that case.
Remember the reason why we want to make this change is because we can
already get access to all this data if we depend on IOMMU_API, while all
the rest is also already available for !IOMMU_API configurations. This
change removes that inconsistency.
> > Moreover, a "git grep CONFIG_IOMMU_API" indicates that the problem
> > isn't specific to Tegra. The "#ifdef CONFIG_IOMMU_API" seems to be
> > sprinkled across the kernel. I think it would be nice if we could
> > improve the situation. So far, using stubs along with what the
> > $subject patch proposes, seems to me to be the best approach.
>
> Yes, there is plenty of code through the tree that is only relevant to the
> IOMMU API and would be a complete waste of space without it, that is not the
> point in question here. Grep for dev_iommu_fwspec_get; outside
> drivers/iommu, the only users are IOMMU-API-specific parts of ACPI code, as
> intended, plus 8 random Tegra drivers.
>
> Now, there does happen to be a tacit contract between the ACPI IORT code and
> the Arm SMMU drivers for how SMMU StreamIDs are encoded in their respective
> fwspecs, but it was never intended for wider consumption.
Again, if iommu_fwspec and its accessors were somehow hidden, or if it
was documented that this was not meant for wider consumption, then
perhaps we wouldn't have started using it in the first place.
> If Tegra drivers
> want to have a special relationship with arm-smmu then fair enough, but they
> can do the same as MSM and formalise it somewhere that the SMMU driver
> maintainers are at least aware of, rather than holding the whole generic
> IOMMU API hostage.
>
> Since apparently it wasn't clear, what I was proposing is a driver helper at
> least something like this:
>
> int tegra_arm_smmu_streamid(struct device *dev)
> {
> #ifdef CONFIG_IOMMU_API
> struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev)
>
> if (fwspec && fwspec->num_ids == 1)
> return fwspec->ids[0] & 0xffff;
> #endif
> return -EINVAL;
> }
>
> Now THAT is scalable and maintainable; any number of random drivers can call
> it without any preconditions, it's a lot clearer what's going on, and I
> won't have to swear profusely while herding patches through half a dozen
> different trees if, when my ops rework gets to the point of refactoring
> iommu_fwspec with dev_iommu, it ends up changing anything significant.
I don't have any objection to making that change. It's not like we're
doing all of this just for fun. We need to do this in order for the
hardware to work correctly. If the above is an acceptable way to do this
and preferable to using the more generic API then we can move ahead with
that.
In order to keep things moving, do we want to merge the change as-is for
now and then subsequently do a pass over all Tegra drivers and use a
common function for stream ID access? Or do you want me to make that
change prior to getting the SDHCI series merged?
Thierry
On Thu, 3 Nov 2022 at 18:35, Robin Murphy <[email protected]> wrote:
>
> On 2022-11-03 14:55, Ulf Hansson wrote:
> > On Thu, 3 Nov 2022 at 15:01, Thierry Reding <[email protected]> wrote:
> >>
> >> On Thu, Nov 03, 2022 at 12:23:20PM +0000, Robin Murphy wrote:
> >>> On 2022-11-03 04:38, Prathamesh Shete wrote:
> >>>> In order to fully make use of the !IOMMU_API stub functions, make the
> >>>> struct iommu_fwspec always available so that users of the stubs can keep
> >>>> using the structure's internals without causing compile failures.
> >>>
> >>> I'm really in two minds about this... fwspecs are an internal detail of the
> >>> IOMMU API that are meant to be private between individual drivers and
> >>> firmware code, so anything poking at them arguably does and should depend on
> >>> CONFIG_IOMMU_API. It looks like the stub for dev_iommu_fwspec_get() was only
> >>> added for the sake of one driver that was misusing it where it really wanted
> >>> device_iommu_mapped(), and has since been fixed, so if anything my
> >>> preference would be to remove that stub :/
> >>
> >> Tegra has been using this type of weak dependency on IOMMU_API mainly in
> >> order to allow building without the IOMMU support on some old platforms
> >> where people may actually care about the kernel size (Tegra20 systems
> >> were sometimes severely constrained and don't have anything that we'd
> >> call an IOMMU today).
> >>
> >> We have similar stubs in place for most other major subsystems in order
> >> to allow code to simply compile out if the subsystem is disabled, which
> >> is quite convenient for sharing code between platforms that may want a
> >> given feature and other platforms that may not want it, without causing
> >> too much of a hassle with compile-testing.
> >
> > I agree with the above.
> >
> > Moreover, the stubs make the code more portable/scalable and so it
> > becomes easier to maintain.
>
> Are you suggesting that having the same thing open-coded slightly
> differently (with bugs) in 8 different places is somehow more
> maintainable than abstracting it into a single centralised implementation?
>
> Is it "easier to maintain" when already seemingly every thing I try to
> clean up or refactor in the IOMMU API at the moment is stymied by
> finding Tegra drivers doing unexpected (and often questionable) things?
> Is it "more scalable" to make it even easier for people to copy
> questionable code without a second thought, leaving API maintainers to
> play an ever-expanding game of whack-a-mole to clean it up? No. No it
> chuffing well isn't :(
Ohh, I wasn't aware of these kinds of issues for the IOMMU interface.
Abusing interfaces is an orthogonal problem to what I was suggesting
to solve here. The main problem I was trying to address was to prevent
sprinkling subsystems/drivers with "#ifdefs" all over the place, as
that doesn't scale.
>
> >>> I don't technically have much objection to this patch in isolation, but what
> >>> I don't like is the direction of travel it implies. I see the anti-pattern
> >>> is only spread across Tegra drivers, making Tegra-specific assumptions, so
> >>> in my view the best answer would be to abstract that fwpsec dependency into
> >>> a single Tegra-specific helper, which would better represent the nature of
> >>> what's really going on here.
> >>
> >> I don't see how this is an anti-pattern. It might not be common for
> >> drivers to need to reach into iommu_fwspec, so that might indeed be
> >> specific to Tegra (for whatever reason our IP seems to want extra
> >> flexibility), but the general pattern of using stubs is wide-spread,
> >> so I don't see why IOMMU_API would need to be special.
> >
> > Again, I agree.
>
> The anti-pattern is reaching into some other driver's private data
> assuming a particular format, with zero indication of the huge degree of
> assumption involved, and half the time not even checking that what's
> being dereferenced is valid.
I see.
>
> > Moreover, a "git grep CONFIG_IOMMU_API" indicates that the problem
> > isn't specific to Tegra. The "#ifdef CONFIG_IOMMU_API" seems to be
> > sprinkled across the kernel. I think it would be nice if we could
> > improve the situation. So far, using stubs along with what the
> > $subject patch proposes, seems to me to be the best approach.
>
> Yes, there is plenty of code through the tree that is only relevant to
> the IOMMU API and would be a complete waste of space without it, that is
> not the point in question here. Grep for dev_iommu_fwspec_get; outside
> drivers/iommu, the only users are IOMMU-API-specific parts of ACPI code,
> as intended, plus 8 random Tegra drivers.
>
> Now, there does happen to be a tacit contract between the ACPI IORT code
> and the Arm SMMU drivers for how SMMU StreamIDs are encoded in their
> respective fwspecs, but it was never intended for wider consumption. If
> Tegra drivers want to have a special relationship with arm-smmu then
> fair enough, but they can do the same as MSM and formalise it somewhere
> that the SMMU driver maintainers are at least aware of, rather than
> holding the whole generic IOMMU API hostage.
Thanks for clarifying this. I certainly understand your concern better now.
>
> Since apparently it wasn't clear, what I was proposing is a driver
> helper at least something like this:
>
> int tegra_arm_smmu_streamid(struct device *dev)
> {
> #ifdef CONFIG_IOMMU_API
> struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev)
>
> if (fwspec && fwspec->num_ids == 1)
> return fwspec->ids[0] & 0xffff;
> #endif
> return -EINVAL;
> }
>
> Now THAT is scalable and maintainable; any number of random drivers can
> call it without any preconditions, it's a lot clearer what's going on,
> and I won't have to swear profusely while herding patches through half a
> dozen different trees if, when my ops rework gets to the point of
> refactoring iommu_fwspec with dev_iommu, it ends up changing anything
> significant.
It sure sounds like we need another level of abstraction for iommus,
to avoid the interface from being abused. Perhaps something along the
lines of what we have for clocks (providers and consumers use
different interfaces).
Anyway, to simplify future potential rework in this direction, I can
agree and understand your points.
What you propose above, with one or a few Tegra specific helper
functions, seems like the best we can do for now.
Kind regards
Uffe
On Thu, Nov 03, 2022 at 05:35:19PM +0000, Robin Murphy wrote:
[...]
> Now, there does happen to be a tacit contract between the ACPI IORT code and
> the Arm SMMU drivers for how SMMU StreamIDs are encoded in their respective
> fwspecs, but it was never intended for wider consumption. If Tegra drivers
> want to have a special relationship with arm-smmu then fair enough, but they
> can do the same as MSM and formalise it somewhere that the SMMU driver
> maintainers are at least aware of, rather than holding the whole generic
> IOMMU API hostage.
Are you talking about qcom_adrena_smmu_is_gpu_device()? That's the only
place I can find where MSM uses iommu_fwspec directly and in a "special"
way.
> Since apparently it wasn't clear, what I was proposing is a driver helper at
> least something like this:
>
> int tegra_arm_smmu_streamid(struct device *dev)
> {
> #ifdef CONFIG_IOMMU_API
> struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev)
>
> if (fwspec && fwspec->num_ids == 1)
> return fwspec->ids[0] & 0xffff;
> #endif
> return -EINVAL;
> }
We actually also use this mechanism on devices that predate the ARM
SMMU, so it'd need to be even more generic. Also, since we need to
access this from a wide range of subsystems, it'd need to be in a
centralized place. Do you think iommu.h would be acceptable for this?
How about if I also add a comment to struct iommu_fwspec about the
intended use?
Thierry
On Fri, Nov 04, 2022 at 03:35:32PM +0100, Ulf Hansson wrote:
> On Thu, 3 Nov 2022 at 18:35, Robin Murphy <[email protected]> wrote:
> >
> > On 2022-11-03 14:55, Ulf Hansson wrote:
> > > On Thu, 3 Nov 2022 at 15:01, Thierry Reding <[email protected]> wrote:
> > >>
> > >> On Thu, Nov 03, 2022 at 12:23:20PM +0000, Robin Murphy wrote:
> > >>> On 2022-11-03 04:38, Prathamesh Shete wrote:
> > >>>> In order to fully make use of the !IOMMU_API stub functions, make the
> > >>>> struct iommu_fwspec always available so that users of the stubs can keep
> > >>>> using the structure's internals without causing compile failures.
> > >>>
> > >>> I'm really in two minds about this... fwspecs are an internal detail of the
> > >>> IOMMU API that are meant to be private between individual drivers and
> > >>> firmware code, so anything poking at them arguably does and should depend on
> > >>> CONFIG_IOMMU_API. It looks like the stub for dev_iommu_fwspec_get() was only
> > >>> added for the sake of one driver that was misusing it where it really wanted
> > >>> device_iommu_mapped(), and has since been fixed, so if anything my
> > >>> preference would be to remove that stub :/
> > >>
> > >> Tegra has been using this type of weak dependency on IOMMU_API mainly in
> > >> order to allow building without the IOMMU support on some old platforms
> > >> where people may actually care about the kernel size (Tegra20 systems
> > >> were sometimes severely constrained and don't have anything that we'd
> > >> call an IOMMU today).
> > >>
> > >> We have similar stubs in place for most other major subsystems in order
> > >> to allow code to simply compile out if the subsystem is disabled, which
> > >> is quite convenient for sharing code between platforms that may want a
> > >> given feature and other platforms that may not want it, without causing
> > >> too much of a hassle with compile-testing.
> > >
> > > I agree with the above.
> > >
> > > Moreover, the stubs make the code more portable/scalable and so it
> > > becomes easier to maintain.
> >
> > Are you suggesting that having the same thing open-coded slightly
> > differently (with bugs) in 8 different places is somehow more
> > maintainable than abstracting it into a single centralised implementation?
> >
> > Is it "easier to maintain" when already seemingly every thing I try to
> > clean up or refactor in the IOMMU API at the moment is stymied by
> > finding Tegra drivers doing unexpected (and often questionable) things?
> > Is it "more scalable" to make it even easier for people to copy
> > questionable code without a second thought, leaving API maintainers to
> > play an ever-expanding game of whack-a-mole to clean it up? No. No it
> > chuffing well isn't :(
>
> Ohh, I wasn't aware of these kinds of issues for the IOMMU interface.
>
> Abusing interfaces is an orthogonal problem to what I was suggesting
> to solve here. The main problem I was trying to address was to prevent
> sprinkling subsystems/drivers with "#ifdefs" all over the place, as
> that doesn't scale.
>
> >
> > >>> I don't technically have much objection to this patch in isolation, but what
> > >>> I don't like is the direction of travel it implies. I see the anti-pattern
> > >>> is only spread across Tegra drivers, making Tegra-specific assumptions, so
> > >>> in my view the best answer would be to abstract that fwpsec dependency into
> > >>> a single Tegra-specific helper, which would better represent the nature of
> > >>> what's really going on here.
> > >>
> > >> I don't see how this is an anti-pattern. It might not be common for
> > >> drivers to need to reach into iommu_fwspec, so that might indeed be
> > >> specific to Tegra (for whatever reason our IP seems to want extra
> > >> flexibility), but the general pattern of using stubs is wide-spread,
> > >> so I don't see why IOMMU_API would need to be special.
> > >
> > > Again, I agree.
> >
> > The anti-pattern is reaching into some other driver's private data
> > assuming a particular format, with zero indication of the huge degree of
> > assumption involved, and half the time not even checking that what's
> > being dereferenced is valid.
>
> I see.
>
> >
> > > Moreover, a "git grep CONFIG_IOMMU_API" indicates that the problem
> > > isn't specific to Tegra. The "#ifdef CONFIG_IOMMU_API" seems to be
> > > sprinkled across the kernel. I think it would be nice if we could
> > > improve the situation. So far, using stubs along with what the
> > > $subject patch proposes, seems to me to be the best approach.
> >
> > Yes, there is plenty of code through the tree that is only relevant to
> > the IOMMU API and would be a complete waste of space without it, that is
> > not the point in question here. Grep for dev_iommu_fwspec_get; outside
> > drivers/iommu, the only users are IOMMU-API-specific parts of ACPI code,
> > as intended, plus 8 random Tegra drivers.
> >
> > Now, there does happen to be a tacit contract between the ACPI IORT code
> > and the Arm SMMU drivers for how SMMU StreamIDs are encoded in their
> > respective fwspecs, but it was never intended for wider consumption. If
> > Tegra drivers want to have a special relationship with arm-smmu then
> > fair enough, but they can do the same as MSM and formalise it somewhere
> > that the SMMU driver maintainers are at least aware of, rather than
> > holding the whole generic IOMMU API hostage.
>
> Thanks for clarifying this. I certainly understand your concern better now.
>
> >
> > Since apparently it wasn't clear, what I was proposing is a driver
> > helper at least something like this:
> >
> > int tegra_arm_smmu_streamid(struct device *dev)
> > {
> > #ifdef CONFIG_IOMMU_API
> > struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev)
> >
> > if (fwspec && fwspec->num_ids == 1)
> > return fwspec->ids[0] & 0xffff;
> > #endif
> > return -EINVAL;
> > }
> >
> > Now THAT is scalable and maintainable; any number of random drivers can
> > call it without any preconditions, it's a lot clearer what's going on,
> > and I won't have to swear profusely while herding patches through half a
> > dozen different trees if, when my ops rework gets to the point of
> > refactoring iommu_fwspec with dev_iommu, it ends up changing anything
> > significant.
>
> It sure sounds like we need another level of abstraction for iommus,
> to avoid the interface from being abused. Perhaps something along the
> lines of what we have for clocks (providers and consumers use
> different interfaces).
Yeah, I was thinking along the same lines. It seems a bit odd to me that
Tegra would be the only chip that ever needs access to the stream IDs
outside of the IOMMU driver), so a generic function that would allow a
device to retrieve its stream ID seems like it would be useful.
We have a few cases where a device can have multiple stream IDs where we
currently force them all to be the same for simplicity. One case where
this can happen is when a device is both a DMA engine and a DMA-capable
microcontroller in one, where the microcontroller can then use a stream
ID separate from that of the DMA engine.
We have another case where a device has multiple contexts for isolation
where things are a bit trickier, but we already have an implementation
using multiple logical devices and iommu-map to take care of that, so it
basically boils down to the above use-case.
> Anyway, to simplify future potential rework in this direction, I can
> agree and understand your points.
>
> What you propose above, with one or a few Tegra specific helper
> functions, seems like the best we can do for now.
If this is really a Tegra-specific need, I guess we can start with a
Tegra-specific helper. We could always generalize from that at a later
point.
Thierry