[1/5] soc: mediatek: Add infracfg misc driver support

Commit Message

Sascha Hauer May 11, 2015, 7:23 p.m. UTC

This adds support for some miscellaneous bits of the infracfg controller.
The mtk_infracfg_set/clear_bus_protection functions are necessary for
the scpsys power domain driver to handle the bus protection bits which
are contained in the infacfg register space.

Signed-off-by: Sascha Hauer <[e-mail]>
---
 drivers/soc/mediatek/Kconfig        |  9 +++++
 drivers/soc/mediatek/Makefile       |  1 +
 drivers/soc/mediatek/mtk-infracfg.c | 80 +++++++++++++++++++++++++++++++++++++
 3 files changed, 90 insertions(+)
 create mode 100644 drivers/soc/mediatek/mtk-infracfg.c

Comments

Sascha Hauer May 12, 2015, 7:12 a.m. UTC | #1

On Mon, May 11, 2015 at 09:23:22PM +0200, Sascha Hauer wrote:
> This adds support for some miscellaneous bits of the infracfg controller.
> The mtk_infracfg_set/clear_bus_protection functions are necessary for
> the scpsys power domain driver to handle the bus protection bits which
> are contained in the infacfg register space.
> 
> Signed-off-by: Sascha Hauer <[e-mail]>
> ---
>  drivers/soc/mediatek/Kconfig        |  9 +++++
>  drivers/soc/mediatek/Makefile       |  1 +
>  drivers/soc/mediatek/mtk-infracfg.c | 80 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 90 insertions(+)
>  create mode 100644 drivers/soc/mediatek/mtk-infracfg.c
> 
> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> index bcdb22d..6fae66f 100644
> --- a/drivers/soc/mediatek/Kconfig
> +++ b/drivers/soc/mediatek/Kconfig
> @@ -9,3 +9,12 @@ config MTK_PMIC_WRAP
>  	  Say yes here to add support for MediaTek PMIC Wrapper found
>  	  on different MediaTek SoCs. The PMIC wrapper is a proprietary
>  	  hardware to connect the PMIC.
> +
> +config MTK_INFRACFG
> +	tristate "MediaTek INFRACFG Support"
> +	depends on ARCH_MEDIATEK
> +	select REGMAP
> +	help
> +	  Say yes here to add support for the MediaTek INFRACFG controller. The
> +	  INFRACFG controller contains various infrastructure registers not
> +	  directly associated to any device.
> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> index ecaf4de..ce39119 100644
> --- a/drivers/soc/mediatek/Makefile
> +++ b/drivers/soc/mediatek/Makefile
> @@ -1 +1,2 @@
>  obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
> +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
> diff --git a/drivers/soc/mediatek/mtk-infracfg.c b/drivers/soc/mediatek/mtk-infracfg.c
> new file mode 100644
> index 0000000..b3ebfae
> --- /dev/null
> +++ b/drivers/soc/mediatek/mtk-infracfg.c
> @@ -0,0 +1,80 @@
> +#include <linux/regmap.h>
> +#include <linux/export.h>
> +#include <linux/jiffies.h>
> +#include <linux/soc/mediatek/infracfg.h>
> +#include <asm/processor.h>
> +
> +#define INFRA_TOPAXI_PROTECTEN		0x0220
> +#define INFRA_TOPAXI_PROTECTSTA1	0x0228
> +
> +/**
> + * mtk_infracfg_set_bus_protection - enable bus protection
> + * @regmap: The infracfg regmap
> + * @mask: The mask containing the protection bits to be enabled.
> + *
> + * This function enables the bus protection bits for disabled power
> + * domains so that the system does not hanf when some unit accesses the
> + * bus while in power down.
> + */
> +int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask)
> +{
> +	unsigned long expired;
> +	u32 val;
> +	int ret;
> +
> +	regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask, mask);
> +
> +	expired = jiffies + HZ;
> +
> +	while (1) {
> +		ret = regmap_read(infracfg, INFRA_TOPAXI_PROTECTSTA1, &val);
> +		if (ret)
> +			return ret;
> +
> +		if ((val & mask) == mask)
> +			break;
> +
> +		cpu_relax();
> +		if (time_after(jiffies, expired))
> +			return -EIO;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(mtk_infracfg_set_bus_protection);
> +
> +/**
> + * mtk_infracfg_clear_bus_protection - disable bus protection
> + * @regmap: The infracfg regmap
> + * @mask: The mask containing the protection bits to be disabled.
> + *
> + * This function disables the bus protection bits previously enabled with
> + * mtk_infracfg_set_bus_protection.
> + */
> +int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask)
> +{
> +	unsigned long expired;
> +	int ret;
> +
> +	regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask, 0);
> +
> +	expired = jiffies + HZ;
> +
> +	while (1) {
> +		u32 val;
> +
> +		ret = regmap_read(infracfg, INFRA_TOPAXI_PROTECTSTA1, &val);
> +		if (ret)
> +			return ret;
> +
> +		if (!(val & mask))
> +			break;
> +
> +		cpu_relax();
> +		if (time_after(jiffies, expired))
> +			return -EIO;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(mtk_infracfg_clear_bus_protection);

Forgot to git add include/linux/soc/mediatek/infracfg.h, this file
contains:

#ifndef __SOC_MEDIATEK_INFRACFG_H
#define __SOC_MEDIATEK_INFRACFG_H

#define MT8173_TOP_AXI_PROT_EN_MCI_M2		BIT(0)
#define MT8173_TOP_AXI_PROT_EN_MM_M0		BIT(1)
#define MT8173_TOP_AXI_PROT_EN_MM_M1		BIT(2)
#define MT8173_TOP_AXI_PROT_EN_MMAPB_S		BIT(6)
#define MT8173_TOP_AXI_PROT_EN_L2C_M2		BIT(9)
#define MT8173_TOP_AXI_PROT_EN_L2SS_SMI		BIT(11)
#define MT8173_TOP_AXI_PROT_EN_L2SS_ADD		BIT(12)
#define MT8173_TOP_AXI_PROT_EN_CCI_M2		BIT(13)
#define MT8173_TOP_AXI_PROT_EN_MFG_S		BIT(14)
#define MT8173_TOP_AXI_PROT_EN_PERI_M0		BIT(15)
#define MT8173_TOP_AXI_PROT_EN_PERI_M1		BIT(16)
#define MT8173_TOP_AXI_PROT_EN_DEBUGSYS		BIT(17)
#define MT8173_TOP_AXI_PROT_EN_CQ_DMA		BIT(18)
#define MT8173_TOP_AXI_PROT_EN_GCPU		BIT(19)
#define MT8173_TOP_AXI_PROT_EN_IOMMU		BIT(20)
#define MT8173_TOP_AXI_PROT_EN_MFG_M0		BIT(21)
#define MT8173_TOP_AXI_PROT_EN_MFG_M1		BIT(22)
#define MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT	BIT(23)

int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask);
int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask);

#endif /* __SOC_MEDIATEK_INFRACFG_H */

Paul Bolle May 12, 2015, 9:24 a.m. UTC | #2

On Mon, 2015-05-11 at 21:23 +0200, Sascha Hauer wrote:
> --- a/drivers/soc/mediatek/Kconfig
> +++ b/drivers/soc/mediatek/Kconfig

> +config MTK_INFRACFG
> +	tristate "MediaTek INFRACFG Support"
> +	depends on ARCH_MEDIATEK
> +	select REGMAP
> +	help
> +	  Say yes here to add support for the MediaTek INFRACFG controller. The
> +	  INFRACFG controller contains various infrastructure registers not
> +	  directly associated to any device.

> --- a/drivers/soc/mediatek/Makefile
> +++ b/drivers/soc/mediatek/Makefile

> +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o

> --- /dev/null
> +++ b/drivers/soc/mediatek/mtk-infracfg.c

> +EXPORT_SYMBOL_GPL(mtk_infracfg_set_bus_protection);

> +EXPORT_SYMBOL_GPL(mtk_infracfg_clear_bus_protection);

As I understand it, if both MTK_INFRACFG and MTK_SCPSYS (see 3/5) are m
we will get mtk-infracfg.ko and mtk-scpsys.ko (see 3/5). And loading
mtk-scpsys.ko will trigger loading mtk-infracfg.ko, right?

But since this file has no MODULE_LICENSE() that should generate a
warning and taint the kernel. (I haven't tested this. Please correct me
if I'm overlooking something here.)

Thanks,


Paul Bolle

Sascha Hauer May 12, 2015, 1:26 p.m. UTC | #3

Hi Paul,

On Tue, May 12, 2015 at 11:24:31AM +0200, Paul Bolle wrote:
> On Mon, 2015-05-11 at 21:23 +0200, Sascha Hauer wrote:
> > --- a/drivers/soc/mediatek/Kconfig
> > +++ b/drivers/soc/mediatek/Kconfig
> 
> > +config MTK_INFRACFG
> > +	tristate "MediaTek INFRACFG Support"
> > +	depends on ARCH_MEDIATEK
> > +	select REGMAP
> > +	help
> > +	  Say yes here to add support for the MediaTek INFRACFG controller. The
> > +	  INFRACFG controller contains various infrastructure registers not
> > +	  directly associated to any device.
> 
> > --- a/drivers/soc/mediatek/Makefile
> > +++ b/drivers/soc/mediatek/Makefile
> 
> > +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
> 
> > --- /dev/null
> > +++ b/drivers/soc/mediatek/mtk-infracfg.c
> 
> > +EXPORT_SYMBOL_GPL(mtk_infracfg_set_bus_protection);
> 
> > +EXPORT_SYMBOL_GPL(mtk_infracfg_clear_bus_protection);
> 
> As I understand it, if both MTK_INFRACFG and MTK_SCPSYS (see 3/5) are m
> we will get mtk-infracfg.ko and mtk-scpsys.ko (see 3/5). And loading
> mtk-scpsys.ko will trigger loading mtk-infracfg.ko, right?
> 
> But since this file has no MODULE_LICENSE() that should generate a
> warning and taint the kernel. (I haven't tested this. Please correct me
> if I'm overlooking something here.)

No, you're absolutely right here. It turned out though that the power
domain specific functions are not exported to modules and also there is
no unregistration code for power domains, so compiling this code as
modules doesn't work anyway. I'll change the tristate to bool.

Sascha

Daniel Kurtz May 15, 2015, 2:17 p.m. UTC | #4

Hi Sascha,

On Tue, May 12, 2015 at 3:23 AM, Sascha Hauer <[e-mail]> wrote:
> This adds support for some miscellaneous bits of the infracfg controller.
> The mtk_infracfg_set/clear_bus_protection functions are necessary for
> the scpsys power domain driver to handle the bus protection bits which
> are contained in the infacfg register space.
>
> Signed-off-by: Sascha Hauer <[e-mail]>
> ---
>  drivers/soc/mediatek/Kconfig        |  9 +++++
>  drivers/soc/mediatek/Makefile       |  1 +
>  drivers/soc/mediatek/mtk-infracfg.c | 80 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 90 insertions(+)
>  create mode 100644 drivers/soc/mediatek/mtk-infracfg.c
>
> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> index bcdb22d..6fae66f 100644
> --- a/drivers/soc/mediatek/Kconfig
> +++ b/drivers/soc/mediatek/Kconfig
> @@ -9,3 +9,12 @@ config MTK_PMIC_WRAP
>           Say yes here to add support for MediaTek PMIC Wrapper found
>           on different MediaTek SoCs. The PMIC wrapper is a proprietary
>           hardware to connect the PMIC.
> +
> +config MTK_INFRACFG

nit: Could you alphabetize these config options - so this one before
MTK_PMIC_WRAP

> +       tristate "MediaTek INFRACFG Support"
> +       depends on ARCH_MEDIATEK

I've seen several drivers like this now:

  depends on ARCH_MEDIATEK || COMPILE_TEST


> +       select REGMAP
> +       help
> +         Say yes here to add support for the MediaTek INFRACFG controller. The
> +         INFRACFG controller contains various infrastructure registers not
> +         directly associated to any device.
> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> index ecaf4de..ce39119 100644
> --- a/drivers/soc/mediatek/Makefile
> +++ b/drivers/soc/mediatek/Makefile
> @@ -1 +1,2 @@
>  obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
> +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o

alphabetize here, too.

> diff --git a/drivers/soc/mediatek/mtk-infracfg.c b/drivers/soc/mediatek/mtk-infracfg.c
> new file mode 100644
> index 0000000..b3ebfae
> --- /dev/null
> +++ b/drivers/soc/mediatek/mtk-infracfg.c
> @@ -0,0 +1,80 @@
> +#include <linux/regmap.h>
> +#include <linux/export.h>
> +#include <linux/jiffies.h>
> +#include <linux/soc/mediatek/infracfg.h>
> +#include <asm/processor.h>

and... alphabetize headers here.

I'm not sure if people care, but I find it makes it much easier to
merge/add things later if these lists are already sorted.
Same "please alphabetize" comments for the mtk-scpsys patch, so I
won't repeat them.

> +
> +#define INFRA_TOPAXI_PROTECTEN         0x0220
> +#define INFRA_TOPAXI_PROTECTSTA1       0x0228
> +
> +/**
> + * mtk_infracfg_set_bus_protection - enable bus protection
> + * @regmap: The infracfg regmap
> + * @mask: The mask containing the protection bits to be enabled.
> + *
> + * This function enables the bus protection bits for disabled power
> + * domains so that the system does not hanf when some unit accesses the
> + * bus while in power down.
> + */
> +int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask)
> +{
> +       unsigned long expired;
> +       u32 val;
> +       int ret;
> +
> +       regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask, mask);
> +
> +       expired = jiffies + HZ;
> +
> +       while (1) {
> +               ret = regmap_read(infracfg, INFRA_TOPAXI_PROTECTSTA1, &val);
> +               if (ret)
> +                       return ret;
> +
> +               if ((val & mask) == mask)
> +                       break;
> +
> +               cpu_relax();
> +               if (time_after(jiffies, expired))
> +                       return -EIO;

I think we should check for timeout first, and then cpu_relax() if
there is still time left (here and in
mtk_infracfg_clear_bus_protection()).  Otherwise we end up doing one
final cpu_relax() without rechecking the register we are polling
(again, I have the same comment for the timeout loops in mtk-scpsys).

Also, shouldn't we return -ETIMEOUT if we timeout?

Thanks!
-Dan

> +       }
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(mtk_infracfg_set_bus_protection);
> +
> +/**
> + * mtk_infracfg_clear_bus_protection - disable bus protection
> + * @regmap: The infracfg regmap
> + * @mask: The mask containing the protection bits to be disabled.
> + *
> + * This function disables the bus protection bits previously enabled with
> + * mtk_infracfg_set_bus_protection.
> + */
> +int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask)
> +{
> +       unsigned long expired;
> +       int ret;
> +
> +       regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask, 0);
> +
> +       expired = jiffies + HZ;
> +
> +       while (1) {
> +               u32 val;
> +
> +               ret = regmap_read(infracfg, INFRA_TOPAXI_PROTECTSTA1, &val);
> +               if (ret)
> +                       return ret;
> +
> +               if (!(val & mask))
> +                       break;
> +
> +               cpu_relax();
> +               if (time_after(jiffies, expired))
> +                       return -EIO;
> +       }
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(mtk_infracfg_clear_bus_protection);
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Sascha Hauer May 18, 2015, 8:16 a.m. UTC | #5

Hi Daniel,

On Fri, May 15, 2015 at 10:17:33PM +0800, Daniel Kurtz wrote:
> Hi Sascha,
> 
> On Tue, May 12, 2015 at 3:23 AM, Sascha Hauer <[e-mail]> wrote:
> > This adds support for some miscellaneous bits of the infracfg controller.
> > The mtk_infracfg_set/clear_bus_protection functions are necessary for
> > the scpsys power domain driver to handle the bus protection bits which
> > are contained in the infacfg register space.
> >
> > Signed-off-by: Sascha Hauer <[e-mail]>
> > ---
> >  drivers/soc/mediatek/Kconfig        |  9 +++++
> >  drivers/soc/mediatek/Makefile       |  1 +
> >  drivers/soc/mediatek/mtk-infracfg.c | 80 +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 90 insertions(+)
> >  create mode 100644 drivers/soc/mediatek/mtk-infracfg.c
> >
> > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> > index bcdb22d..6fae66f 100644
> > --- a/drivers/soc/mediatek/Kconfig
> > +++ b/drivers/soc/mediatek/Kconfig
> > @@ -9,3 +9,12 @@ config MTK_PMIC_WRAP
> >           Say yes here to add support for MediaTek PMIC Wrapper found
> >           on different MediaTek SoCs. The PMIC wrapper is a proprietary
> >           hardware to connect the PMIC.
> > +
> > +config MTK_INFRACFG
> 
> nit: Could you alphabetize these config options - so this one before
> MTK_PMIC_WRAP
> 
> > +       tristate "MediaTek INFRACFG Support"
> > +       depends on ARCH_MEDIATEK
> 
> I've seen several drivers like this now:
> 
>   depends on ARCH_MEDIATEK || COMPILE_TEST
> 
> 
> > +       select REGMAP
> > +       help
> > +         Say yes here to add support for the MediaTek INFRACFG controller. The
> > +         INFRACFG controller contains various infrastructure registers not
> > +         directly associated to any device.
> > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> > index ecaf4de..ce39119 100644
> > --- a/drivers/soc/mediatek/Makefile
> > +++ b/drivers/soc/mediatek/Makefile
> > @@ -1 +1,2 @@
> >  obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
> > +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
> 
> alphabetize here, too.
> 
> > diff --git a/drivers/soc/mediatek/mtk-infracfg.c b/drivers/soc/mediatek/mtk-infracfg.c
> > new file mode 100644
> > index 0000000..b3ebfae
> > --- /dev/null
> > +++ b/drivers/soc/mediatek/mtk-infracfg.c
> > @@ -0,0 +1,80 @@
> > +#include <linux/regmap.h>
> > +#include <linux/export.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/soc/mediatek/infracfg.h>
> > +#include <asm/processor.h>
> 
> and... alphabetize headers here.
> 
> I'm not sure if people care, but I find it makes it much easier to
> merge/add things later if these lists are already sorted.
> Same "please alphabetize" comments for the mtk-scpsys patch, so I
> won't repeat them.
> 
> > +
> > +#define INFRA_TOPAXI_PROTECTEN         0x0220
> > +#define INFRA_TOPAXI_PROTECTSTA1       0x0228
> > +
> > +/**
> > + * mtk_infracfg_set_bus_protection - enable bus protection
> > + * @regmap: The infracfg regmap
> > + * @mask: The mask containing the protection bits to be enabled.
> > + *
> > + * This function enables the bus protection bits for disabled power
> > + * domains so that the system does not hanf when some unit accesses the
> > + * bus while in power down.
> > + */
> > +int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask)
> > +{
> > +       unsigned long expired;
> > +       u32 val;
> > +       int ret;
> > +
> > +       regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask, mask);
> > +
> > +       expired = jiffies + HZ;
> > +
> > +       while (1) {
> > +               ret = regmap_read(infracfg, INFRA_TOPAXI_PROTECTSTA1, &val);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               if ((val & mask) == mask)
> > +                       break;
> > +
> > +               cpu_relax();
> > +               if (time_after(jiffies, expired))
> > +                       return -EIO;
> 
> I think we should check for timeout first, and then cpu_relax() if
> there is still time left (here and in
> mtk_infracfg_clear_bus_protection()).  Otherwise we end up doing one
> final cpu_relax() without rechecking the register we are polling
> (again, I have the same comment for the timeout loops in mtk-scpsys).

I think cpu_relax() delays execution in the order of microseconds (I
don't actually know, just a guess), so if the timeout is a second the
order doesn't really matter. What can happen though is an interrupt
after the (val & mask) test but before the timeout check. So to be
truly correct we have to repeat the (val & mask) test after the
time_after() check. Is that what you want?

> 
> Also, shouldn't we return -ETIMEOUT if we timeout?

I dunno. Probably the operation operation timed out because of an IO
error. I'll change it to -ETIMEDOUT.

Sascha

Daniel Kurtz May 19, 2015, 6:54 a.m. UTC | #6

On Mon, May 18, 2015 at 4:16 PM, Sascha Hauer <[e-mail]> wrote:
> Hi Daniel,
>
> On Fri, May 15, 2015 at 10:17:33PM +0800, Daniel Kurtz wrote:
>> Hi Sascha,
>>
>> On Tue, May 12, 2015 at 3:23 AM, Sascha Hauer <[e-mail]> wrote:
>> > This adds support for some miscellaneous bits of the infracfg controller.
>> > The mtk_infracfg_set/clear_bus_protection functions are necessary for
>> > the scpsys power domain driver to handle the bus protection bits which
>> > are contained in the infacfg register space.
>> >
>> > Signed-off-by: Sascha Hauer <[e-mail]>
>> > ---
>> >  drivers/soc/mediatek/Kconfig        |  9 +++++
>> >  drivers/soc/mediatek/Makefile       |  1 +
>> >  drivers/soc/mediatek/mtk-infracfg.c | 80 +++++++++++++++++++++++++++++++++++++
>> >  3 files changed, 90 insertions(+)
>> >  create mode 100644 drivers/soc/mediatek/mtk-infracfg.c
>> >
>> > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
>> > index bcdb22d..6fae66f 100644
>> > --- a/drivers/soc/mediatek/Kconfig
>> > +++ b/drivers/soc/mediatek/Kconfig
>> > @@ -9,3 +9,12 @@ config MTK_PMIC_WRAP
>> >           Say yes here to add support for MediaTek PMIC Wrapper found
>> >           on different MediaTek SoCs. The PMIC wrapper is a proprietary
>> >           hardware to connect the PMIC.
>> > +
>> > +config MTK_INFRACFG
>>
>> nit: Could you alphabetize these config options - so this one before
>> MTK_PMIC_WRAP
>>
>> > +       tristate "MediaTek INFRACFG Support"
>> > +       depends on ARCH_MEDIATEK
>>
>> I've seen several drivers like this now:
>>
>>   depends on ARCH_MEDIATEK || COMPILE_TEST
>>
>>
>> > +       select REGMAP
>> > +       help
>> > +         Say yes here to add support for the MediaTek INFRACFG controller. The
>> > +         INFRACFG controller contains various infrastructure registers not
>> > +         directly associated to any device.
>> > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
>> > index ecaf4de..ce39119 100644
>> > --- a/drivers/soc/mediatek/Makefile
>> > +++ b/drivers/soc/mediatek/Makefile
>> > @@ -1 +1,2 @@
>> >  obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
>> > +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
>>
>> alphabetize here, too.
>>
>> > diff --git a/drivers/soc/mediatek/mtk-infracfg.c b/drivers/soc/mediatek/mtk-infracfg.c
>> > new file mode 100644
>> > index 0000000..b3ebfae
>> > --- /dev/null
>> > +++ b/drivers/soc/mediatek/mtk-infracfg.c
>> > @@ -0,0 +1,80 @@
>> > +#include <linux/regmap.h>
>> > +#include <linux/export.h>
>> > +#include <linux/jiffies.h>
>> > +#include <linux/soc/mediatek/infracfg.h>
>> > +#include <asm/processor.h>
>>
>> and... alphabetize headers here.
>>
>> I'm not sure if people care, but I find it makes it much easier to
>> merge/add things later if these lists are already sorted.
>> Same "please alphabetize" comments for the mtk-scpsys patch, so I
>> won't repeat them.
>>
>> > +
>> > +#define INFRA_TOPAXI_PROTECTEN         0x0220
>> > +#define INFRA_TOPAXI_PROTECTSTA1       0x0228
>> > +
>> > +/**
>> > + * mtk_infracfg_set_bus_protection - enable bus protection
>> > + * @regmap: The infracfg regmap
>> > + * @mask: The mask containing the protection bits to be enabled.
>> > + *
>> > + * This function enables the bus protection bits for disabled power
>> > + * domains so that the system does not hanf when some unit accesses the
>> > + * bus while in power down.
>> > + */
>> > +int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask)
>> > +{
>> > +       unsigned long expired;
>> > +       u32 val;
>> > +       int ret;
>> > +
>> > +       regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask, mask);
>> > +
>> > +       expired = jiffies + HZ;
>> > +
>> > +       while (1) {
>> > +               ret = regmap_read(infracfg, INFRA_TOPAXI_PROTECTSTA1, &val);
>> > +               if (ret)
>> > +                       return ret;
>> > +
>> > +               if ((val & mask) == mask)
>> > +                       break;
>> > +
>> > +               cpu_relax();
>> > +               if (time_after(jiffies, expired))
>> > +                       return -EIO;
>>
>> I think we should check for timeout first, and then cpu_relax() if
>> there is still time left (here and in
>> mtk_infracfg_clear_bus_protection()).  Otherwise we end up doing one
>> final cpu_relax() without rechecking the register we are polling
>> (again, I have the same comment for the timeout loops in mtk-scpsys).
>
> I think cpu_relax() delays execution in the order of microseconds (I
> don't actually know, just a guess), so if the timeout is a second the
> order doesn't really matter. What can happen though is an interrupt
> after the (val & mask) test but before the timeout check. So to be
> truly correct we have to repeat the (val & mask) test after the
> time_after() check. Is that what you want?

I'm not following, why would you need to repeat (val & mask) test
after time_after?
What does an interrupt have to do with it?
Can you show a code snippet with what exactly you are proposing?

-Dan

>> Also, shouldn't we return -ETIMEOUT if we timeout?
>
> I dunno. Probably the operation operation timed out because of an IO
> error. I'll change it to -ETIMEDOUT.
>
> Sascha
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: [phone]    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |

Sascha Hauer May 19, 2015, 7:45 a.m. UTC | #7

On Tue, May 19, 2015 at 02:54:41PM +0800, Daniel Kurtz wrote:
> >> > +       while (1) {
> >> > +               ret = regmap_read(infracfg, INFRA_TOPAXI_PROTECTSTA1, &val);
> >> > +               if (ret)
> >> > +                       return ret;
> >> > +
> >> > +               if ((val & mask) == mask)
> >> > +                       break;
> >> > +
> >> > +               cpu_relax();
> >> > +               if (time_after(jiffies, expired))
> >> > +                       return -EIO;
> >>
> >> I think we should check for timeout first, and then cpu_relax() if
> >> there is still time left (here and in
> >> mtk_infracfg_clear_bus_protection()).  Otherwise we end up doing one
> >> final cpu_relax() without rechecking the register we are polling
> >> (again, I have the same comment for the timeout loops in mtk-scpsys).
> >
> > I think cpu_relax() delays execution in the order of microseconds (I
> > don't actually know, just a guess), so if the timeout is a second the
> > order doesn't really matter. What can happen though is an interrupt
> > after the (val & mask) test but before the timeout check. So to be
> > truly correct we have to repeat the (val & mask) test after the
> > time_after() check. Is that what you want?
> 
> I'm not following, why would you need to repeat (val & mask) test
> after time_after?
> What does an interrupt have to do with it?
> Can you show a code snippet with what exactly you are proposing?

Consider you have this timeout loop:

	while (1) {
		if (success())
			break;

		if (time_after(jiffies, expired))
			return -ETIMEDOUT;
	}

Now when an interupt comes in between success() and time_after() then it
can happen that the delay caused by the interrupt makes the code timeout
even though success() might have become true in the meantime. So to be
correct you have to:

	while (1) {
		if (success())
			break;

		if (time_after(jiffies, expired)) {
			if (success())
				break;
			return -ETIMEDOUT;
	}

Or, if you don't want to repeat the termination condition:

	bool timeout = false;

	while (1) {
		if (success())
			break;

		if (timeout)
			return -ETIMEDOUT;

		if (time_after(jiffies, expired))
			timeout = true;
	}

Anyway, with the timeout of one second used here this is all academic.

Sascha

Daniel Kurtz May 19, 2015, 10:39 a.m. UTC | #8

On Tue, May 19, 2015 at 3:45 PM, Sascha Hauer <[e-mail]> wrote:
> On Tue, May 19, 2015 at 02:54:41PM +0800, Daniel Kurtz wrote:
>> >> > +       while (1) {
>> >> > +               ret = regmap_read(infracfg, INFRA_TOPAXI_PROTECTSTA1, &val);
>> >> > +               if (ret)
>> >> > +                       return ret;
>> >> > +
>> >> > +               if ((val & mask) == mask)
>> >> > +                       break;
>> >> > +
>> >> > +               cpu_relax();
>> >> > +               if (time_after(jiffies, expired))
>> >> > +                       return -EIO;
>> >>
>> >> I think we should check for timeout first, and then cpu_relax() if
>> >> there is still time left (here and in
>> >> mtk_infracfg_clear_bus_protection()).  Otherwise we end up doing one
>> >> final cpu_relax() without rechecking the register we are polling
>> >> (again, I have the same comment for the timeout loops in mtk-scpsys).
>> >
>> > I think cpu_relax() delays execution in the order of microseconds (I
>> > don't actually know, just a guess), so if the timeout is a second the
>> > order doesn't really matter. What can happen though is an interrupt
>> > after the (val & mask) test but before the timeout check. So to be
>> > truly correct we have to repeat the (val & mask) test after the
>> > time_after() check. Is that what you want?
>>
>> I'm not following, why would you need to repeat (val & mask) test
>> after time_after?
>> What does an interrupt have to do with it?
>> Can you show a code snippet with what exactly you are proposing?
>
> Consider you have this timeout loop:
>
>         while (1) {
>                 if (success())
>                         break;
>
>                 if (time_after(jiffies, expired))
>                         return -ETIMEDOUT;
>         }
>
> Now when an interupt comes in between success() and time_after() then it
> can happen that the delay caused by the interrupt makes the code timeout
> even though success() might have become true in the meantime. So to be
> correct you have to:

I agree - I was confused because you only mentioned repeating the
"(val & mask) test", not re-reading the register, which is the
important bit.
For other drivers, I've seen "wait_for()" macros, like below, which do
exactly as you suggest above:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/iommu/rockchip-iommu.c#n110

>
>         while (1) {
>                 if (success())
>                         break;
>
>                 if (time_after(jiffies, expired)) {
>                         if (success())
>                                 break;
>                         return -ETIMEDOUT;
>         }
>
> Or, if you don't want to repeat the termination condition:
>
>         bool timeout = false;
>
>         while (1) {
>                 if (success())
>                         break;
>
>                 if (timeout)
>                         return -ETIMEDOUT;
>
>                 if (time_after(jiffies, expired))
>                         timeout = true;
>         }
>
> Anyway, with the timeout of one second used here this is all academic.

I totally agree that this is academic for the loops here and in
SCPSYS, where the timeout is arbitrary and long.

-Dan

>
> Sascha
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: [phone]    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |

Kevin Hilman May 26, 2015, 11:12 p.m. UTC | #9

Sascha Hauer <[e-mail]> writes:

> This adds support for some miscellaneous bits of the infracfg controller.
> The mtk_infracfg_set/clear_bus_protection functions are necessary for
> the scpsys power domain driver to handle the bus protection bits which
> are contained in the infacfg register space.
>
> Signed-off-by: Sascha Hauer <[e-mail]>

IMO, this feature still needs more documentation.  From an earlier
exchange with James Liao, he was able to get some more details from the
HW designer[1] which I think belong in comments somewhere in this
driver.

Kevin
 
[1] http://lists.infradead.org/pipermail/linux-mediatek/2015-March/000142.html

Sascha Hauer May 27, 2015, 7:33 a.m. UTC | #10

On Tue, May 26, 2015 at 04:12:06PM -0700, Kevin Hilman wrote:
> Sascha Hauer <[e-mail]> writes:
> 
> > This adds support for some miscellaneous bits of the infracfg controller.
> > The mtk_infracfg_set/clear_bus_protection functions are necessary for
> > the scpsys power domain driver to handle the bus protection bits which
> > are contained in the infacfg register space.
> >
> > Signed-off-by: Sascha Hauer <[e-mail]>
> 
> IMO, this feature still needs more documentation.  From an earlier
> exchange with James Liao, he was able to get some more details from the
> HW designer[1] which I think belong in comments somewhere in this
> driver.

Ok, I'll add something like this.

Sascha

Patch

diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
index bcdb22d..6fae66f 100644
--- a/drivers/soc/mediatek/Kconfig
+++ b/drivers/soc/mediatek/Kconfig
@@ -9,3 +9,12 @@  config MTK_PMIC_WRAP
 	  Say yes here to add support for MediaTek PMIC Wrapper found
 	  on different MediaTek SoCs. The PMIC wrapper is a proprietary
 	  hardware to connect the PMIC.
+
+config MTK_INFRACFG
+	tristate "MediaTek INFRACFG Support"
+	depends on ARCH_MEDIATEK
+	select REGMAP
+	help
+	  Say yes here to add support for the MediaTek INFRACFG controller. The
+	  INFRACFG controller contains various infrastructure registers not
+	  directly associated to any device.
diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
index ecaf4de..ce39119 100644
--- a/drivers/soc/mediatek/Makefile
+++ b/drivers/soc/mediatek/Makefile
@@ -1 +1,2 @@ 
 obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
+obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
diff --git a/drivers/soc/mediatek/mtk-infracfg.c b/drivers/soc/mediatek/mtk-infracfg.c
new file mode 100644
index 0000000..b3ebfae
--- /dev/null
+++ b/drivers/soc/mediatek/mtk-infracfg.c
@@ -0,0 +1,80 @@ 
+#include <linux/regmap.h>
+#include <linux/export.h>
+#include <linux/jiffies.h>
+#include <linux/soc/mediatek/infracfg.h>
+#include <asm/processor.h>
+
+#define INFRA_TOPAXI_PROTECTEN		0x0220
+#define INFRA_TOPAXI_PROTECTSTA1	0x0228
+
+/**
+ * mtk_infracfg_set_bus_protection - enable bus protection
+ * @regmap: The infracfg regmap
+ * @mask: The mask containing the protection bits to be enabled.
+ *
+ * This function enables the bus protection bits for disabled power
+ * domains so that the system does not hanf when some unit accesses the
+ * bus while in power down.
+ */
+int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask)
+{
+	unsigned long expired;
+	u32 val;
+	int ret;
+
+	regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask, mask);
+
+	expired = jiffies + HZ;
+
+	while (1) {
+		ret = regmap_read(infracfg, INFRA_TOPAXI_PROTECTSTA1, &val);
+		if (ret)
+			return ret;
+
+		if ((val & mask) == mask)
+			break;
+
+		cpu_relax();
+		if (time_after(jiffies, expired))
+			return -EIO;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mtk_infracfg_set_bus_protection);
+
+/**
+ * mtk_infracfg_clear_bus_protection - disable bus protection
+ * @regmap: The infracfg regmap
+ * @mask: The mask containing the protection bits to be disabled.
+ *
+ * This function disables the bus protection bits previously enabled with
+ * mtk_infracfg_set_bus_protection.
+ */
+int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask)
+{
+	unsigned long expired;
+	int ret;
+
+	regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask, 0);
+
+	expired = jiffies + HZ;
+
+	while (1) {
+		u32 val;
+
+		ret = regmap_read(infracfg, INFRA_TOPAXI_PROTECTSTA1, &val);
+		if (ret)
+			return ret;
+
+		if (!(val & mask))
+			break;
+
+		cpu_relax();
+		if (time_after(jiffies, expired))
+			return -EIO;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mtk_infracfg_clear_bus_protection);