Re: [PATCH 1/5] soc: mediatek: Add SMI driver
Hi Arnd, Am Montag, den 09.03.2015, 22:56 +0100 schrieb Arnd Bergmann: > On Monday 09 March 2015 11:26:52 Yingjoe Chen wrote: > > On Fri, 2015-03-06 at 18:48 +0800, yong...@mediatek.com wrote: > > > From: Yong Wu > > > > > > This patch add SMI(Smart Multimedia Interface) driver. This driver is > > > responsible to enable/disable iommu and control the clocks of each > > > local arbiter. > > > > > > Signed-off-by: Yong Wu > > > --- > > > drivers/soc/mediatek/Kconfig | 7 ++ > > > drivers/soc/mediatek/Makefile | 1 + > > > drivers/soc/mediatek/mt8173-smi.c | 143 > > > ++ > > > include/linux/mtk-smi.h | 40 +++ > > > 4 files changed, 191 insertions(+) > > > create mode 100644 drivers/soc/mediatek/mt8173-smi.c > > > create mode 100644 include/linux/mtk-smi.h > > > > > > > Hi Arnd, Matthias, > > > > For the SMI driver, we can't find a better place, so we put it in > > drivers/soc/mediatek now. Please let us know if you have any suggestion > > or concern. Thanks > > From what I understand from your description, I think it would better > fit in drivers/iommu. Another option is drivers/memory, which I think > is where the respective Tegra driver ended up. > Note that this is not the IOMMU driver. M4U is the IOMMU unit. The SMI unit is more of a bridge control for the internal buses where you can control various properties for the masters on the bus. One notable property (and why this driver is part of the IOMMU series) is the control weather a master should go through the M4U unit for translation or bypass it and talk directly to external memory controller. Regards, Lucas -- Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ | ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/5] soc: mediatek: Add SMI driver
On Tuesday 10 March 2015 14:27:04 Yingjoe Chen wrote: > On Mon, 2015-03-09 at 22:56 +0100, Arnd Bergmann wrote: > > On Monday 09 March 2015 11:26:52 Yingjoe Chen wrote: > > > On Fri, 2015-03-06 at 18:48 +0800, yong...@mediatek.com wrote: > > > > From: Yong Wu > > > > > > > > This patch add SMI(Smart Multimedia Interface) driver. This driver > > > > is > > > > responsible to enable/disable iommu and control the clocks of each > > > > local arbiter. > > > > > > > > Signed-off-by: Yong Wu > > > > --- > > > > drivers/soc/mediatek/Kconfig | 7 ++ > > > > drivers/soc/mediatek/Makefile | 1 + > > > > drivers/soc/mediatek/mt8173-smi.c | 143 > > > > ++ > > > > include/linux/mtk-smi.h | 40 +++ > > > > 4 files changed, 191 insertions(+) > > > > create mode 100644 drivers/soc/mediatek/mt8173-smi.c > > > > create mode 100644 include/linux/mtk-smi.h > > > > > > > > > > Hi Arnd, Matthias, > > > > > > For the SMI driver, we can't find a better place, so we put it in > > > drivers/soc/mediatek now. Please let us know if you have any suggestion > > > or concern. Thanks > > > > From what I understand from your description, I think it would better > > fit in drivers/iommu. Another option is drivers/memory, which I think > > is where the respective Tegra driver ended up. > > Hi Arnd, > > The description above only describe what is implemented now. Besides > that, SMI HW also does: > > - Prioritize/arbitrate memory requests between different ports & larbs. > - Bandwidth limiter > - Performance monitor > > I think drivers/memory make more sense. I can't find info about > drivers/memory in MAINTAINERS. Can this go through ARM SoC tree if we > put SMI driver there? > Yes, that works for me. I'd like to see a review from Thierry Reding though, since he wrote the Tegra driver that is used for very similar hardware, and I want to make sure you two are on the same page with the internal APIs. Arnd ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/5] soc: mediatek: Add SMI driver
On Mon, 2015-03-09 at 22:56 +0100, Arnd Bergmann wrote: > On Monday 09 March 2015 11:26:52 Yingjoe Chen wrote: > > On Fri, 2015-03-06 at 18:48 +0800, yong...@mediatek.com wrote: > > > From: Yong Wu > > > > > > This patch add SMI(Smart Multimedia Interface) driver. This driver is > > > responsible to enable/disable iommu and control the clocks of each > > > local arbiter. > > > > > > Signed-off-by: Yong Wu > > > --- > > > drivers/soc/mediatek/Kconfig | 7 ++ > > > drivers/soc/mediatek/Makefile | 1 + > > > drivers/soc/mediatek/mt8173-smi.c | 143 > > > ++ > > > include/linux/mtk-smi.h | 40 +++ > > > 4 files changed, 191 insertions(+) > > > create mode 100644 drivers/soc/mediatek/mt8173-smi.c > > > create mode 100644 include/linux/mtk-smi.h > > > > > > > Hi Arnd, Matthias, > > > > For the SMI driver, we can't find a better place, so we put it in > > drivers/soc/mediatek now. Please let us know if you have any suggestion > > or concern. Thanks > > From what I understand from your description, I think it would better > fit in drivers/iommu. Another option is drivers/memory, which I think > is where the respective Tegra driver ended up. Hi Arnd, The description above only describe what is implemented now. Besides that, SMI HW also does: - Prioritize/arbitrate memory requests between different ports & larbs. - Bandwidth limiter - Performance monitor I think drivers/memory make more sense. I can't find info about drivers/memory in MAINTAINERS. Can this go through ARM SoC tree if we put SMI driver there? Joe.C ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/5] soc: mediatek: Add SMI driver
On Mon, 2015-03-09 at 18:59 +0100, Paul Bolle wrote: > Hi Yong, > > Yong Wu schreef op ma 09-03-2015 om 19:57 [+0800]: > > On Fri, 2015-03-06 at 12:30 +0100, Paul Bolle wrote: > > > On Fri, 2015-03-06 at 18:48 +0800, yong...@mediatek.com wrote: > > > > --- a/drivers/soc/mediatek/Kconfig > > > > +++ b/drivers/soc/mediatek/Kconfig > > > > @@ -20,3 +20,10 @@ config MT8173_PMIC_WRAP > > > > PMIC wrapper is a proprietary hardware in MT8173 to make > > > > communication protocols to access PMIC device. > > > > This driver implement access protocols for MT8173. > > > > + > > > > +config MTK_SMI > > > > +bool > > > > > > Nit: make this one tab instead of 8 spaces, please. > > > > > > > + help > > > > + Smi help enable/disable iommu in mt8173 and control the > > > > + clock of each local arbiter. > > > > + It should be true while MTK_IOMMU enable. > > > > > > I don't think anyone using the *config tools will ever see this text, as > > > there's no prompt. So you might as well make this a comment or drop it > > > altogether. > > > > > We could search it in the tool even though we don't see it. In next > > version, I will try to make it a comment. > > > Is this selected by more than just MTK_IOMMU (see 2/5)? If not, I think > > > MTK_SMI will be set and unset in lockstep with MTK_IOMMU. In other > > > words, you could as well use one Kconfig symbol. > > > > > if we disable MTK_IOMMU, the MTK_SMI also should be selected.That is > > because > > if the multimedia h/w want to work, the clock of the local arbiters always > > should be opened. > > This is a bit confusing, I'm afraid. Do you mean to say that it ought to > be possible for MTK_SMI to be 'y' even if MTK_IOMMU would be 'n'? The SMI can be configured to bypass IOMMU and send traffic directly to memory interface. It is possible to not use IOMMU and have display/MM to use continuous memory only. Besides MTK_IOMMU, we expect DRM, VDEC driver to select MTK_SMI as well. Joe.C ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/5] soc: mediatek: Add SMI driver
On Monday 09 March 2015 11:26:52 Yingjoe Chen wrote: > On Fri, 2015-03-06 at 18:48 +0800, yong...@mediatek.com wrote: > > From: Yong Wu > > > > This patch add SMI(Smart Multimedia Interface) driver. This driver is > > responsible to enable/disable iommu and control the clocks of each > > local arbiter. > > > > Signed-off-by: Yong Wu > > --- > > drivers/soc/mediatek/Kconfig | 7 ++ > > drivers/soc/mediatek/Makefile | 1 + > > drivers/soc/mediatek/mt8173-smi.c | 143 > > ++ > > include/linux/mtk-smi.h | 40 +++ > > 4 files changed, 191 insertions(+) > > create mode 100644 drivers/soc/mediatek/mt8173-smi.c > > create mode 100644 include/linux/mtk-smi.h > > > > Hi Arnd, Matthias, > > For the SMI driver, we can't find a better place, so we put it in > drivers/soc/mediatek now. Please let us know if you have any suggestion > or concern. Thanks >From what I understand from your description, I think it would better fit in drivers/iommu. Another option is drivers/memory, which I think is where the respective Tegra driver ended up. Arnd ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/5] soc: mediatek: Add SMI driver
On Monday 09 March 2015 18:59:37 Paul Bolle wrote: > Hi Yong, > > Yong Wu schreef op ma 09-03-2015 om 19:57 [+0800]: > > On Fri, 2015-03-06 at 12:30 +0100, Paul Bolle wrote: > > > On Fri, 2015-03-06 at 18:48 +0800, yong...@mediatek.com wrote: > > > > --- a/drivers/soc/mediatek/Kconfig > > > > +++ b/drivers/soc/mediatek/Kconfig > > > > @@ -20,3 +20,10 @@ config MT8173_PMIC_WRAP > > > > PMIC wrapper is a proprietary hardware in MT8173 to make > > > > communication protocols to access PMIC device. > > > > This driver implement access protocols for MT8173. > > > > + > > > > +config MTK_SMI > > > > +bool > > > > > > Nit: make this one tab instead of 8 spaces, please. > > > > > > > + help > > > > + Smi help enable/disable iommu in mt8173 and control the > > > > + clock of each local arbiter. > > > > + It should be true while MTK_IOMMU enable. > > > > > > I don't think anyone using the *config tools will ever see this text, as > > > there's no prompt. So you might as well make this a comment or drop it > > > altogether. > > > > > We could search it in the tool even though we don't see it. In next > > version, I will try to make it a comment. No, please leave it the way it was. I prefer having help texts even for silent options over using comments to do the same. Arnd ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/5] soc: mediatek: Add SMI driver
Hi Yong, Yong Wu schreef op ma 09-03-2015 om 19:57 [+0800]: > On Fri, 2015-03-06 at 12:30 +0100, Paul Bolle wrote: > > On Fri, 2015-03-06 at 18:48 +0800, yong...@mediatek.com wrote: > > > --- a/drivers/soc/mediatek/Kconfig > > > +++ b/drivers/soc/mediatek/Kconfig > > > @@ -20,3 +20,10 @@ config MT8173_PMIC_WRAP > > > PMIC wrapper is a proprietary hardware in MT8173 to make > > > communication protocols to access PMIC device. > > > This driver implement access protocols for MT8173. > > > + > > > +config MTK_SMI > > > +bool > > > > Nit: make this one tab instead of 8 spaces, please. > > > > > + help > > > + Smi help enable/disable iommu in mt8173 and control the > > > + clock of each local arbiter. > > > + It should be true while MTK_IOMMU enable. > > > > I don't think anyone using the *config tools will ever see this text, as > > there's no prompt. So you might as well make this a comment or drop it > > altogether. > > > We could search it in the tool even though we don't see it. In next > version, I will try to make it a comment. > > Is this selected by more than just MTK_IOMMU (see 2/5)? If not, I think > > MTK_SMI will be set and unset in lockstep with MTK_IOMMU. In other > > words, you could as well use one Kconfig symbol. > > > if we disable MTK_IOMMU, the MTK_SMI also should be selected.That is > because > if the multimedia h/w want to work, the clock of the local arbiters always > should be opened. This is a bit confusing, I'm afraid. Do you mean to say that it ought to be possible for MTK_SMI to be 'y' even if MTK_IOMMU would be 'n'? Paul Bolle ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/5] soc: mediatek: Add SMI driver
Dear Paul, Thanks very much for your suggestion. On Fri, 2015-03-06 at 12:30 +0100, Paul Bolle wrote: > On Fri, 2015-03-06 at 18:48 +0800, yong...@mediatek.com wrote: > > --- a/drivers/soc/mediatek/Kconfig > > +++ b/drivers/soc/mediatek/Kconfig > > @@ -20,3 +20,10 @@ config MT8173_PMIC_WRAP > > PMIC wrapper is a proprietary hardware in MT8173 to make > > communication protocols to access PMIC device. > > This driver implement access protocols for MT8173. > > + > > +config MTK_SMI > > +bool > > Nit: make this one tab instead of 8 spaces, please. > > > + help > > + Smi help enable/disable iommu in mt8173 and control the > > + clock of each local arbiter. > > + It should be true while MTK_IOMMU enable. > > I don't think anyone using the *config tools will ever see this text, as > there's no prompt. So you might as well make this a comment or drop it > altogether. > We could search it in the tool even though we don't see it. In next version, I will try to make it a comment. > Is this selected by more than just MTK_IOMMU (see 2/5)? If not, I think > MTK_SMI will be set and unset in lockstep with MTK_IOMMU. In other > words, you could as well use one Kconfig symbol. > > Thanks, if we disable MTK_IOMMU, the MTK_SMI also should be selected.That is because if the multimedia h/w want to work, the clock of the local arbiters always should be opened. > > Paul Bolle > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/5] soc: mediatek: Add SMI driver
Hi Yong,
On Fri, Mar 6, 2015 at 6:37 PM, wrote:
> From: Yong Wu
>
> This patch add SMI(Smart Multimedia Interface) driver. This driver
> is responsible to enable/disable iommu and control the clocks of each
> local arbiter.
High-level:
Is there more to the smi (or smi-larb) driver, or is it always just a
1:1 wrapper for a particular m4u consumer?
In other words, instead of a separate driver, is it possible to move
this functionality into the m4u driver and/or the m4u consumers
directly?
>
> Signed-off-by: Yong Wu
> ---
> drivers/soc/mediatek/Kconfig | 7 ++
> drivers/soc/mediatek/Makefile | 1 +
> drivers/soc/mediatek/mt8173-smi.c | 143
> ++
> include/linux/mtk-smi.h | 40 +++
> 4 files changed, 191 insertions(+)
> create mode 100644 drivers/soc/mediatek/mt8173-smi.c
> create mode 100644 include/linux/mtk-smi.h
>
> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> index 729f93e..27fb26c 100644
> --- a/drivers/soc/mediatek/Kconfig
> +++ b/drivers/soc/mediatek/Kconfig
> @@ -20,3 +20,10 @@ config MT8173_PMIC_WRAP
> PMIC wrapper is a proprietary hardware in MT8173 to make
> communication protocols to access PMIC device.
> This driver implement access protocols for MT8173.
> +
> +config MTK_SMI
> +bool
> + help
> + Smi help enable/disable iommu in mt8173 and control the
> + clock of each local arbiter.
> + It should be true while MTK_IOMMU enable.
> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> index 9b5709b..cdfe95c 100644
> --- a/drivers/soc/mediatek/Makefile
> +++ b/drivers/soc/mediatek/Makefile
> @@ -1,2 +1,3 @@
> obj-$(CONFIG_MT8135_PMIC_WRAP) += mt8135-pmic-wrap.o
> obj-$(CONFIG_MT8173_PMIC_WRAP) += mt8173-pmic-wrap.o
> +obj-$(CONFIG_MTK_SMI) += mt8173-smi.o
> diff --git a/drivers/soc/mediatek/mt8173-smi.c
> b/drivers/soc/mediatek/mt8173-smi.c
> new file mode 100644
> index 000..4e3fab9
> --- /dev/null
> +++ b/drivers/soc/mediatek/mt8173-smi.c
> @@ -0,0 +1,143 @@
> +/*
> + * Copyright (c) 2014-2015 MediaTek Inc.
> + * Author: Yong Wu
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define SMI_LARB_MMU_EN (0xf00)
> +#define F_SMI_MMU_EN(port) (1 << (port))
> +
> +struct mtk_smi_larb {
> + void __iomem *larb_base;
> + struct clk *larb_clk[3];/* each larb has 3 clk at most */
> +};
> +
> +static const char * const mtk_smi_clk_name[] = {
> + "larb_sub0", "larb_sub1", "larb_sub2"
> +};
The order and meaning of these clocks do not seem particularly important.
It seems a bit awkward to use these arbitrary names just so we can use
devm_clk_get() to get a variably sized array of clocks from .dts.
Can we eliminate the "clock-names" property, and just use a single
.dts proprety that lists an array of clocks?
Then you would also get an explicit clock count, and can remove the
NULL checking when iterating.
An example of a clock list without names is:
Clock list in .dts:
https://lkml.org/lkml/2014/11/17/115
Filling in the clocks from .dts:
https://lkml.org/lkml/2014/11/17/114
Unfortunately, those patches never made it out of list discussion into
a maintainer tree.
> +
> +static const struct of_device_id mtk_smi_of_ids[] = {
> + { .compatible = "mediatek,mt8173-smi-larb",
> + },
> + {}
> +};
I find it a bit redundant to call the struct "mtk_smi_larb", and then
to prepend "larb_" to all of the fields.
In fact this whole driver is a bit confusing because it isn't clear if
this is an "smi" driver (of which only larb control has been
implemented) or is this an "smi_larb" driver (and potentially there
are other smi drivers).
Perhaps we can just call this an "smi_larb" driver, rename this file
to mt8173-smi-larb.c, and then doing something like:
struct mtk_smi_larb {
void __iomem *base;
struct clk *clk[3];/* each smi_larb has at most 3 clocks */
};
static const struct of_device_id mtk_smi_larb_of_ids[] = {
{ .compatible = "mediatek,mt8173-smi-larb" },
{}
};
> +
> +int mtk_smi_larb_get(struct platform_device *plarbdev)
Is there any reason to use "struct platform_device" here instead of
just "struct device"?
> +{
> + struct mtk_smi_larb *larbpriv = dev_get_drvdata(&plarbdev->dev);
> + int i, ret = 0;
> +
> + for (i = 0; i < 3; i++)
> + if (larbpriv->
Re: [PATCH 1/5] soc: mediatek: Add SMI driver
On Fri, 2015-03-06 at 18:48 +0800, yong...@mediatek.com wrote: > From: Yong Wu > > This patch add SMI(Smart Multimedia Interface) driver. This driver is > responsible to enable/disable iommu and control the clocks of each > local arbiter. > > Signed-off-by: Yong Wu > --- > drivers/soc/mediatek/Kconfig | 7 ++ > drivers/soc/mediatek/Makefile | 1 + > drivers/soc/mediatek/mt8173-smi.c | 143 > ++ > include/linux/mtk-smi.h | 40 +++ > 4 files changed, 191 insertions(+) > create mode 100644 drivers/soc/mediatek/mt8173-smi.c > create mode 100644 include/linux/mtk-smi.h > Hi Arnd, Matthias, For the SMI driver, we can't find a better place, so we put it in drivers/soc/mediatek now. Please let us know if you have any suggestion or concern. Thanks You can find more description about SMI and how we use it in the cover letter: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/328451.html Joe.C ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/5] soc: mediatek: Add SMI driver
On Fri, 2015-03-06 at 18:48 +0800, yong...@mediatek.com wrote: > --- a/drivers/soc/mediatek/Kconfig > +++ b/drivers/soc/mediatek/Kconfig > @@ -20,3 +20,10 @@ config MT8173_PMIC_WRAP > PMIC wrapper is a proprietary hardware in MT8173 to make > communication protocols to access PMIC device. > This driver implement access protocols for MT8173. > + > +config MTK_SMI > +bool Nit: make this one tab instead of 8 spaces, please. > + help > + Smi help enable/disable iommu in mt8173 and control the > + clock of each local arbiter. > + It should be true while MTK_IOMMU enable. I don't think anyone using the *config tools will ever see this text, as there's no prompt. So you might as well make this a comment or drop it altogether. Is this selected by more than just MTK_IOMMU (see 2/5)? If not, I think MTK_SMI will be set and unset in lockstep with MTK_IOMMU. In other words, you could as well use one Kconfig symbol. Thanks, Paul Bolle ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
