[2/8] soc: ti: add initial PRM driver with reset control support

Commit Message

Tero Kristo Aug. 7, 2019, 7:48 a.m. UTC

Add initial PRM (Power and Reset Management) driver for TI OMAP class
SoCs. Initially this driver only supports reset control, but can be
extended to support rest of the functionality, like powerdomain
control, PRCM irq support etc.

Signed-off-by: Tero Kristo <[e-mail]>
---
 arch/arm/mach-omap2/Kconfig |   1 +
 drivers/soc/ti/Makefile     |   1 +
 drivers/soc/ti/omap_prm.c   | 216 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 218 insertions(+)
 create mode 100644 drivers/soc/ti/omap_prm.c

Comments

J, KEERTHY Aug. 8, 2019, 5:26 a.m. UTC | #1

On 07/08/19 1:18 PM, Tero Kristo wrote:
> Add initial PRM (Power and Reset Management) driver for TI OMAP class
> SoCs. Initially this driver only supports reset control, but can be
> extended to support rest of the functionality, like powerdomain
> control, PRCM irq support etc.
> 
> Signed-off-by: Tero Kristo <[e-mail]>
> ---
>   arch/arm/mach-omap2/Kconfig |   1 +
>   drivers/soc/ti/Makefile     |   1 +
>   drivers/soc/ti/omap_prm.c   | 216 ++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 218 insertions(+)
>   create mode 100644 drivers/soc/ti/omap_prm.c
> 
> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
> index fdb6743..42ad063 100644
> --- a/arch/arm/mach-omap2/Kconfig
> +++ b/arch/arm/mach-omap2/Kconfig
> @@ -109,6 +109,7 @@ config ARCH_OMAP2PLUS
>   	select TI_SYSC
>   	select OMAP_IRQCHIP
>   	select CLKSRC_TI_32K
> +	select RESET_CONTROLLER
>   	help
>   	  Systems based on OMAP2, OMAP3, OMAP4 or OMAP5
>   
> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
> index b3868d3..788b5cd 100644
> --- a/drivers/soc/ti/Makefile
> +++ b/drivers/soc/ti/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS)	+= knav_qmss.o
>   knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o
>   obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA)	+= knav_dma.o
>   obj-$(CONFIG_AMX3_PM)			+= pm33xx.o
> +obj-$(CONFIG_ARCH_OMAP2PLUS)		+= omap_prm.o
>   obj-$(CONFIG_WKUP_M3_IPC)		+= wkup_m3_ipc.o
>   obj-$(CONFIG_TI_SCI_PM_DOMAINS)		+= ti_sci_pm_domains.o
>   obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN)	+= ti_sci_inta_msi.o
> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
> new file mode 100644
> index 0000000..7c89eb8
> --- /dev/null
> +++ b/drivers/soc/ti/omap_prm.c
> @@ -0,0 +1,216 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * OMAP2+ PRM driver
> + *
> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> + *	Tero Kristo <[e-mail]>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/delay.h>
> +
> +struct omap_rst_map {
> +	s8 rst;
> +	s8 st;
> +};
> +
> +struct omap_prm_data {
> +	u32 base;
> +	const char *name;
> +	u16 pwstctrl;
> +	u16 pwstst;
> +	u16 rstctl;
> +	u16 rstst;
> +	struct omap_rst_map *rstmap;
> +	u8 flags;
> +};
> +
> +struct omap_prm {
> +	const struct omap_prm_data *data;
> +	void __iomem *base;
> +};
> +
> +struct omap_reset_data {
> +	struct reset_controller_dev rcdev;
> +	struct omap_prm *prm;
> +};
> +
> +#define to_omap_reset_data(p) container_of((p), struct omap_reset_data, rcdev)
> +
> +#define OMAP_MAX_RESETS		8
> +#define OMAP_RESET_MAX_WAIT	10000
> +
> +#define OMAP_PRM_NO_RSTST	BIT(0)
> +
> +static const struct of_device_id omap_prm_id_table[] = {
> +	{ },
> +};

This table is blank and we are doing of_match_device against it.

> +
> +static int omap_reset_status(struct reset_controller_dev *rcdev,
> +			     unsigned long id)
> +{
> +	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
> +	u32 v;
> +
> +	v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
> +	v &= 1 << id;
> +	v >>= id;
> +
> +	return v;
> +}
> +
> +static int omap_reset_assert(struct reset_controller_dev *rcdev,
> +			     unsigned long id)
> +{
> +	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
> +	u32 v;
> +
> +	/* assert the reset control line */
> +	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
> +	v |= 1 << id;
> +	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
> +
> +	return 0;
> +}
> +
> +static int omap_reset_get_st_bit(struct omap_reset_data *reset,
> +				 unsigned long id)
> +{
> +	struct omap_rst_map *map = reset->prm->data->rstmap;
> +
> +	while (map && map->rst >= 0) {
> +		if (map->rst == id)
> +			return map->st;
> +
> +		map++;
> +	}
> +
> +	return id;
> +}
> +
> +/*
> + * Note that status will not change until clocks are on, and clocks cannot be
> + * enabled until reset is deasserted. Consumer drivers must check status
> + * separately after enabling clocks.
> + */
> +static int omap_reset_deassert(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
> +	u32 v;
> +	int st_bit = id;
> +	bool has_rstst;
> +
> +	/* check the current status to avoid de-asserting the line twice */
> +	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
> +	if (!(v & BIT(id)))
> +		return -EEXIST;
> +
> +	has_rstst = !(reset->prm->data->flags & OMAP_PRM_NO_RSTST);
> +
> +	if (has_rstst) {
> +		st_bit = omap_reset_get_st_bit(reset, id);
> +
> +		/* Clear the reset status by writing 1 to the status bit */
> +		v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
> +		v |= 1 << st_bit;
> +		writel_relaxed(v, reset->prm->base + reset->prm->data->rstst);
> +	}
> +
> +	/* de-assert the reset control line */
> +	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
> +	v &= ~(1 << id);
> +	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
> +
> +	return 0;
> +}
> +
> +static const struct reset_control_ops omap_reset_ops = {
> +	.assert		= omap_reset_assert,
> +	.deassert	= omap_reset_deassert,
> +	.status		= omap_reset_status,
> +};
> +
> +static int omap_prm_reset_probe(struct platform_device *pdev,
> +				struct omap_prm *prm)
> +{
> +	struct omap_reset_data *reset;
> +
> +	/*
> +	 * Check if we have resets. If either rstctl or rstst is
> +	 * non-zero, we have reset registers in place. Additionally
> +	 * the flag OMAP_PRM_NO_RSTST implies that we have resets.
> +	 */
> +	if (!prm->data->rstctl && !prm->data->rstst &&
> +	    !(prm->data->flags & OMAP_PRM_NO_RSTST))
> +		return 0;
> +
> +	reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
> +	if (!reset)
> +		return -ENOMEM;
> +
> +	reset->rcdev.owner = THIS_MODULE;
> +	reset->rcdev.ops = &omap_reset_ops;
> +	reset->rcdev.of_node = pdev->dev.of_node;
> +	reset->rcdev.nr_resets = OMAP_MAX_RESETS;
> +
> +	reset->prm = prm;
> +
> +	return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
> +}
> +
> +static int omap_prm_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	const struct omap_prm_data *data;
> +	struct omap_prm *prm;
> +	const struct of_device_id *match;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +
> +	match = of_match_device(omap_prm_id_table, &pdev->dev);
> +	if (!match)
> +		return -ENOTSUPP;
> +
> +	prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL);
> +	if (!prm)
> +		return -ENOMEM;
> +
> +	data = match->data;
> +
> +	while (data->base != res->start) {
> +		if (!data->base)
> +			return -EINVAL;
> +		data++;
> +	}
> +
> +	prm->data = data;
> +
> +	prm->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (!prm->base)
> +		return -ENOMEM;
> +
> +	return omap_prm_reset_probe(pdev, prm);
> +}
> +
> +static struct platform_driver omap_prm_driver = {
> +	.probe = omap_prm_probe,
> +	.driver = {
> +		.name		= KBUILD_MODNAME,
> +		.of_match_table	= omap_prm_id_table,
> +	},
> +};
> +builtin_platform_driver(omap_prm_driver);
> +
> +MODULE_ALIAS("platform:prm");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("omap2+ prm driver");

It is a builtin_platform_driver so do we need the MODULE*?

>

Tero Kristo Aug. 19, 2019, 9:32 a.m. UTC | #2

On 08/08/2019 08:26, Keerthy wrote:
> 
> 
> On 07/08/19 1:18 PM, Tero Kristo wrote:
>> Add initial PRM (Power and Reset Management) driver for TI OMAP class
>> SoCs. Initially this driver only supports reset control, but can be
>> extended to support rest of the functionality, like powerdomain
>> control, PRCM irq support etc.
>>
>> Signed-off-by: Tero Kristo <[e-mail]>
>> ---
>>   arch/arm/mach-omap2/Kconfig |   1 +
>>   drivers/soc/ti/Makefile     |   1 +
>>   drivers/soc/ti/omap_prm.c   | 216 
>> ++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 218 insertions(+)
>>   create mode 100644 drivers/soc/ti/omap_prm.c
>>
>> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
>> index fdb6743..42ad063 100644
>> --- a/arch/arm/mach-omap2/Kconfig
>> +++ b/arch/arm/mach-omap2/Kconfig
>> @@ -109,6 +109,7 @@ config ARCH_OMAP2PLUS
>>       select TI_SYSC
>>       select OMAP_IRQCHIP
>>       select CLKSRC_TI_32K
>> +    select RESET_CONTROLLER
>>       help
>>         Systems based on OMAP2, OMAP3, OMAP4 or OMAP5
>> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
>> index b3868d3..788b5cd 100644
>> --- a/drivers/soc/ti/Makefile
>> +++ b/drivers/soc/ti/Makefile
>> @@ -6,6 +6,7 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS)    += knav_qmss.o
>>   knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o
>>   obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA)    += knav_dma.o
>>   obj-$(CONFIG_AMX3_PM)            += pm33xx.o
>> +obj-$(CONFIG_ARCH_OMAP2PLUS)        += omap_prm.o
>>   obj-$(CONFIG_WKUP_M3_IPC)        += wkup_m3_ipc.o
>>   obj-$(CONFIG_TI_SCI_PM_DOMAINS)        += ti_sci_pm_domains.o
>>   obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN)    += ti_sci_inta_msi.o
>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
>> new file mode 100644
>> index 0000000..7c89eb8
>> --- /dev/null
>> +++ b/drivers/soc/ti/omap_prm.c
>> @@ -0,0 +1,216 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * OMAP2+ PRM driver
>> + *
>> + * Copyright (C) 2019 Texas Instruments Incorporated - 
>> http://www.ti.com/
>> + *    Tero Kristo <[e-mail]>
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/device.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reset-controller.h>
>> +#include <linux/delay.h>
>> +
>> +struct omap_rst_map {
>> +    s8 rst;
>> +    s8 st;
>> +};
>> +
>> +struct omap_prm_data {
>> +    u32 base;
>> +    const char *name;
>> +    u16 pwstctrl;
>> +    u16 pwstst;
>> +    u16 rstctl;
>> +    u16 rstst;
>> +    struct omap_rst_map *rstmap;
>> +    u8 flags;
>> +};
>> +
>> +struct omap_prm {
>> +    const struct omap_prm_data *data;
>> +    void __iomem *base;
>> +};
>> +
>> +struct omap_reset_data {
>> +    struct reset_controller_dev rcdev;
>> +    struct omap_prm *prm;
>> +};
>> +
>> +#define to_omap_reset_data(p) container_of((p), struct 
>> omap_reset_data, rcdev)
>> +
>> +#define OMAP_MAX_RESETS        8
>> +#define OMAP_RESET_MAX_WAIT    10000
>> +
>> +#define OMAP_PRM_NO_RSTST    BIT(0)
>> +
>> +static const struct of_device_id omap_prm_id_table[] = {
>> +    { },
>> +};
> 
> This table is blank and we are doing of_match_device against it.

Yes, it gets populated with other patches in series, one entry per soc.

> 
>> +
>> +static int omap_reset_status(struct reset_controller_dev *rcdev,
>> +                 unsigned long id)
>> +{
>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>> +    u32 v;
>> +
>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
>> +    v &= 1 << id;
>> +    v >>= id;
>> +
>> +    return v;
>> +}
>> +
>> +static int omap_reset_assert(struct reset_controller_dev *rcdev,
>> +                 unsigned long id)
>> +{
>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>> +    u32 v;
>> +
>> +    /* assert the reset control line */
>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>> +    v |= 1 << id;
>> +    writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
>> +
>> +    return 0;
>> +}
>> +
>> +static int omap_reset_get_st_bit(struct omap_reset_data *reset,
>> +                 unsigned long id)
>> +{
>> +    struct omap_rst_map *map = reset->prm->data->rstmap;
>> +
>> +    while (map && map->rst >= 0) {
>> +        if (map->rst == id)
>> +            return map->st;
>> +
>> +        map++;
>> +    }
>> +
>> +    return id;
>> +}
>> +
>> +/*
>> + * Note that status will not change until clocks are on, and clocks 
>> cannot be
>> + * enabled until reset is deasserted. Consumer drivers must check status
>> + * separately after enabling clocks.
>> + */
>> +static int omap_reset_deassert(struct reset_controller_dev *rcdev,
>> +                   unsigned long id)
>> +{
>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>> +    u32 v;
>> +    int st_bit = id;
>> +    bool has_rstst;
>> +
>> +    /* check the current status to avoid de-asserting the line twice */
>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>> +    if (!(v & BIT(id)))
>> +        return -EEXIST;
>> +
>> +    has_rstst = !(reset->prm->data->flags & OMAP_PRM_NO_RSTST);
>> +
>> +    if (has_rstst) {
>> +        st_bit = omap_reset_get_st_bit(reset, id);
>> +
>> +        /* Clear the reset status by writing 1 to the status bit */
>> +        v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
>> +        v |= 1 << st_bit;
>> +        writel_relaxed(v, reset->prm->base + reset->prm->data->rstst);
>> +    }
>> +
>> +    /* de-assert the reset control line */
>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>> +    v &= ~(1 << id);
>> +    writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct reset_control_ops omap_reset_ops = {
>> +    .assert        = omap_reset_assert,
>> +    .deassert    = omap_reset_deassert,
>> +    .status        = omap_reset_status,
>> +};
>> +
>> +static int omap_prm_reset_probe(struct platform_device *pdev,
>> +                struct omap_prm *prm)
>> +{
>> +    struct omap_reset_data *reset;
>> +
>> +    /*
>> +     * Check if we have resets. If either rstctl or rstst is
>> +     * non-zero, we have reset registers in place. Additionally
>> +     * the flag OMAP_PRM_NO_RSTST implies that we have resets.
>> +     */
>> +    if (!prm->data->rstctl && !prm->data->rstst &&
>> +        !(prm->data->flags & OMAP_PRM_NO_RSTST))
>> +        return 0;
>> +
>> +    reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
>> +    if (!reset)
>> +        return -ENOMEM;
>> +
>> +    reset->rcdev.owner = THIS_MODULE;
>> +    reset->rcdev.ops = &omap_reset_ops;
>> +    reset->rcdev.of_node = pdev->dev.of_node;
>> +    reset->rcdev.nr_resets = OMAP_MAX_RESETS;
>> +
>> +    reset->prm = prm;
>> +
>> +    return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
>> +}
>> +
>> +static int omap_prm_probe(struct platform_device *pdev)
>> +{
>> +    struct resource *res;
>> +    const struct omap_prm_data *data;
>> +    struct omap_prm *prm;
>> +    const struct of_device_id *match;
>> +
>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +    if (!res)
>> +        return -ENODEV;
>> +
>> +    match = of_match_device(omap_prm_id_table, &pdev->dev);
>> +    if (!match)
>> +        return -ENOTSUPP;
>> +
>> +    prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL);
>> +    if (!prm)
>> +        return -ENOMEM;
>> +
>> +    data = match->data;
>> +
>> +    while (data->base != res->start) {
>> +        if (!data->base)
>> +            return -EINVAL;
>> +        data++;
>> +    }
>> +
>> +    prm->data = data;
>> +
>> +    prm->base = devm_ioremap_resource(&pdev->dev, res);
>> +    if (!prm->base)
>> +        return -ENOMEM;
>> +
>> +    return omap_prm_reset_probe(pdev, prm);
>> +}
>> +
>> +static struct platform_driver omap_prm_driver = {
>> +    .probe = omap_prm_probe,
>> +    .driver = {
>> +        .name        = KBUILD_MODNAME,
>> +        .of_match_table    = omap_prm_id_table,
>> +    },
>> +};
>> +builtin_platform_driver(omap_prm_driver);
>> +
>> +MODULE_ALIAS("platform:prm");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("omap2+ prm driver");
> 
> It is a builtin_platform_driver so do we need the MODULE*?

Well, thats debatable, however some existing drivers do introduce this 
even if they are builtin.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Suman Anna Aug. 19, 2019, 11:01 p.m. UTC | #3

Hi Tero,

On 8/19/19 4:32 AM, Tero Kristo wrote:
> On 08/08/2019 08:26, Keerthy wrote:
>>
>>
>> On 07/08/19 1:18 PM, Tero Kristo wrote:
>>> Add initial PRM (Power and Reset Management) driver for TI OMAP class
>>> SoCs. Initially this driver only supports reset control, but can be
>>> extended to support rest of the functionality, like powerdomain
>>> control, PRCM irq support etc.
>>>
>>> Signed-off-by: Tero Kristo <[e-mail]>
>>> ---
>>>   arch/arm/mach-omap2/Kconfig |   1 +
>>>   drivers/soc/ti/Makefile     |   1 +
>>>   drivers/soc/ti/omap_prm.c   | 216
>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 218 insertions(+)
>>>   create mode 100644 drivers/soc/ti/omap_prm.c
>>>
>>> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
>>> index fdb6743..42ad063 100644
>>> --- a/arch/arm/mach-omap2/Kconfig
>>> +++ b/arch/arm/mach-omap2/Kconfig
>>> @@ -109,6 +109,7 @@ config ARCH_OMAP2PLUS
>>>       select TI_SYSC
>>>       select OMAP_IRQCHIP
>>>       select CLKSRC_TI_32K
>>> +    select RESET_CONTROLLER

Use ARCH_HAS_RESET_CONTROLLER instead.

>>>       help
>>>         Systems based on OMAP2, OMAP3, OMAP4 or OMAP5
>>> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
>>> index b3868d3..788b5cd 100644
>>> --- a/drivers/soc/ti/Makefile
>>> +++ b/drivers/soc/ti/Makefile
>>> @@ -6,6 +6,7 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS)    += knav_qmss.o
>>>   knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o
>>>   obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA)    += knav_dma.o
>>>   obj-$(CONFIG_AMX3_PM)            += pm33xx.o
>>> +obj-$(CONFIG_ARCH_OMAP2PLUS)        += omap_prm.o
>>>   obj-$(CONFIG_WKUP_M3_IPC)        += wkup_m3_ipc.o
>>>   obj-$(CONFIG_TI_SCI_PM_DOMAINS)        += ti_sci_pm_domains.o
>>>   obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN)    += ti_sci_inta_msi.o
>>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
>>> new file mode 100644
>>> index 0000000..7c89eb8
>>> --- /dev/null
>>> +++ b/drivers/soc/ti/omap_prm.c
>>> @@ -0,0 +1,216 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * OMAP2+ PRM driver
>>> + *
>>> + * Copyright (C) 2019 Texas Instruments Incorporated -
>>> http://www.ti.com/
>>> + *    Tero Kristo <[e-mail]>
>>> + */
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/device.h>
>>> +#include <linux/io.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/reset-controller.h>
>>> +#include <linux/delay.h>
>>> +
>>> +struct omap_rst_map {
>>> +    s8 rst;
>>> +    s8 st;
>>> +};
>>> +
>>> +struct omap_prm_data {
>>> +    u32 base;
>>> +    const char *name;
>>> +    u16 pwstctrl;
>>> +    u16 pwstst;
>>> +    u16 rstctl;

Minor nit, can you use rstctrl instead here so that it is in sync with
the other variables and with the register names used in TRM.

>>> +    u16 rstst;
>>> +    struct omap_rst_map *rstmap;
>>> +    u8 flags;
>>> +};
>>> +
>>> +struct omap_prm {
>>> +    const struct omap_prm_data *data;
>>> +    void __iomem *base;
>>> +};
>>> +
>>> +struct omap_reset_data {
>>> +    struct reset_controller_dev rcdev;
>>> +    struct omap_prm *prm;
>>> +};
>>> +
>>> +#define to_omap_reset_data(p) container_of((p), struct
>>> omap_reset_data, rcdev)
>>> +
>>> +#define OMAP_MAX_RESETS        8
>>> +#define OMAP_RESET_MAX_WAIT    10000
>>> +
>>> +#define OMAP_PRM_NO_RSTST    BIT(0)
>>> +
>>> +static const struct of_device_id omap_prm_id_table[] = {
>>> +    { },
>>> +};
>>
>> This table is blank and we are doing of_match_device against it.
> 
> Yes, it gets populated with other patches in series, one entry per soc.
> 
>>
>>> +
>>> +static int omap_reset_status(struct reset_controller_dev *rcdev,
>>> +                 unsigned long id)
>>> +{
>>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>>> +    u32 v;
>>> +
>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
>>> +    v &= 1 << id;
>>> +    v >>= id;
>>> +
>>> +    return v;
>>> +}
>>> +
>>> +static int omap_reset_assert(struct reset_controller_dev *rcdev,
>>> +                 unsigned long id)
>>> +{
>>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>>> +    u32 v;
>>> +
>>> +    /* assert the reset control line */
>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>>> +    v |= 1 << id;
>>> +    writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int omap_reset_get_st_bit(struct omap_reset_data *reset,
>>> +                 unsigned long id)
>>> +{
>>> +    struct omap_rst_map *map = reset->prm->data->rstmap;
>>> +
>>> +    while (map && map->rst >= 0) {
>>> +        if (map->rst == id)
>>> +            return map->st;
>>> +
>>> +        map++;
>>> +    }
>>> +
>>> +    return id;
>>> +}
>>> +
>>> +/*
>>> + * Note that status will not change until clocks are on, and clocks
>>> cannot be
>>> + * enabled until reset is deasserted. Consumer drivers must check
>>> status
>>> + * separately after enabling clocks.
>>> + */
>>> +static int omap_reset_deassert(struct reset_controller_dev *rcdev,
>>> +                   unsigned long id)
>>> +{
>>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>>> +    u32 v;
>>> +    int st_bit = id;

No need to initialize this, will always get overwritten below.

>>> +    bool has_rstst;
>>> +
>>> +    /* check the current status to avoid de-asserting the line twice */
>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>>> +    if (!(v & BIT(id)))
>>> +        return -EEXIST;
>>> +
>>> +    has_rstst = !(reset->prm->data->flags & OMAP_PRM_NO_RSTST);
>>> +
>>> +    if (has_rstst) {
>>> +        st_bit = omap_reset_get_st_bit(reset, id);
>>> +
>>> +        /* Clear the reset status by writing 1 to the status bit */
>>> +        v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
>>> +        v |= 1 << st_bit;
>>> +        writel_relaxed(v, reset->prm->base + reset->prm->data->rstst);
>>> +    }
>>> +
>>> +    /* de-assert the reset control line */
>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>>> +    v &= ~(1 << id);
>>> +    writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct reset_control_ops omap_reset_ops = {
>>> +    .assert        = omap_reset_assert,
>>> +    .deassert    = omap_reset_deassert,
>>> +    .status        = omap_reset_status,
>>> +};
>>> +
>>> +static int omap_prm_reset_probe(struct platform_device *pdev,
>>> +                struct omap_prm *prm)

Call this omap_prm_reset_init or something similar to avoid confusion.

>>> +{
>>> +    struct omap_reset_data *reset;
>>> +
>>> +    /*
>>> +     * Check if we have resets. If either rstctl or rstst is
>>> +     * non-zero, we have reset registers in place. Additionally
>>> +     * the flag OMAP_PRM_NO_RSTST implies that we have resets.
>>> +     */
>>> +    if (!prm->data->rstctl && !prm->data->rstst &&
>>> +        !(prm->data->flags & OMAP_PRM_NO_RSTST))
>>> +        return 0;
>>> +
>>> +    reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
>>> +    if (!reset)
>>> +        return -ENOMEM;
>>> +
>>> +    reset->rcdev.owner = THIS_MODULE;
>>> +    reset->rcdev.ops = &omap_reset_ops;
>>> +    reset->rcdev.of_node = pdev->dev.of_node;
>>> +    reset->rcdev.nr_resets = OMAP_MAX_RESETS;

Suggest adding a number of resets to prm->data, and using it so that we
don't even entertain any resets beyond the actual number of resets.

You actually seem to be using the bit-position directly in client data
instead of a reset number. I am not sure if this is accepted practice
with reset controllers, do you incur any memory wastage?

>>> +
>>> +    reset->prm = prm;
>>> +
>>> +    return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
>>> +}
>>> +
>>> +static int omap_prm_probe(struct platform_device *pdev)
>>> +{
>>> +    struct resource *res;
>>> +    const struct omap_prm_data *data;
>>> +    struct omap_prm *prm;
>>> +    const struct of_device_id *match;
>>> +
>>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +    if (!res)
>>> +        return -ENODEV;
>>> +
>>> +    match = of_match_device(omap_prm_id_table, &pdev->dev);
>>> +    if (!match)
>>> +        return -ENOTSUPP;

Use of_device_get_match_data() instead to do both match and get the
data. That can perhaps be the first block.

>>> +
>>> +    prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL);
>>> +    if (!prm)
>>> +        return -ENOMEM;

Perhaps move the allocate after the match check to streamline.

>>> +
>>> +    data = match->data;
>>> +
>>> +    while (data->base != res->start) {
>>> +        if (!data->base)
>>> +            return -EINVAL;
>>> +        data++;
>>> +    }
>>> +
>>> +    prm->data = data;
>>> +
>>> +    prm->base = devm_ioremap_resource(&pdev->dev, res);
>>> +    if (!prm->base)
>>> +        return -ENOMEM;
>>> +
>>> +    return omap_prm_reset_probe(pdev, prm);
>>> +}
>>> +
>>> +static struct platform_driver omap_prm_driver = {
>>> +    .probe = omap_prm_probe,
>>> +    .driver = {
>>> +        .name        = KBUILD_MODNAME,
>>> +        .of_match_table    = omap_prm_id_table,
>>> +    },
>>> +};
>>> +builtin_platform_driver(omap_prm_driver);
>>> +
>>> +MODULE_ALIAS("platform:prm");

Drop this and use MODULE_DEVICE_TABLE instead on omap_prm_id_table if
retaining, but I don't think these need to be defined.

regards
Suman

>>> +MODULE_LICENSE("GPL v2");
>>> +MODULE_DESCRIPTION("omap2+ prm driver");
>>
>> It is a builtin_platform_driver so do we need the MODULE*?
> 
> Well, thats debatable, however some existing drivers do introduce this
> even if they are builtin.
> 
> -Tero
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Tero Kristo Aug. 20, 2019, 7:37 a.m. UTC | #4

On 20.8.2019 2.01, Suman Anna wrote:
> Hi Tero,
> 
> On 8/19/19 4:32 AM, Tero Kristo wrote:
>> On 08/08/2019 08:26, Keerthy wrote:
>>>
>>>
>>> On 07/08/19 1:18 PM, Tero Kristo wrote:
>>>> Add initial PRM (Power and Reset Management) driver for TI OMAP class
>>>> SoCs. Initially this driver only supports reset control, but can be
>>>> extended to support rest of the functionality, like powerdomain
>>>> control, PRCM irq support etc.
>>>>
>>>> Signed-off-by: Tero Kristo <[e-mail]>
>>>> ---
>>>>    arch/arm/mach-omap2/Kconfig |   1 +
>>>>    drivers/soc/ti/Makefile     |   1 +
>>>>    drivers/soc/ti/omap_prm.c   | 216
>>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 218 insertions(+)
>>>>    create mode 100644 drivers/soc/ti/omap_prm.c
>>>>
>>>> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
>>>> index fdb6743..42ad063 100644
>>>> --- a/arch/arm/mach-omap2/Kconfig
>>>> +++ b/arch/arm/mach-omap2/Kconfig
>>>> @@ -109,6 +109,7 @@ config ARCH_OMAP2PLUS
>>>>        select TI_SYSC
>>>>        select OMAP_IRQCHIP
>>>>        select CLKSRC_TI_32K
>>>> +    select RESET_CONTROLLER
> 
> Use ARCH_HAS_RESET_CONTROLLER instead.

Ok.

> 
>>>>        help
>>>>          Systems based on OMAP2, OMAP3, OMAP4 or OMAP5
>>>> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
>>>> index b3868d3..788b5cd 100644
>>>> --- a/drivers/soc/ti/Makefile
>>>> +++ b/drivers/soc/ti/Makefile
>>>> @@ -6,6 +6,7 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS)    += knav_qmss.o
>>>>    knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o
>>>>    obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA)    += knav_dma.o
>>>>    obj-$(CONFIG_AMX3_PM)            += pm33xx.o
>>>> +obj-$(CONFIG_ARCH_OMAP2PLUS)        += omap_prm.o
>>>>    obj-$(CONFIG_WKUP_M3_IPC)        += wkup_m3_ipc.o
>>>>    obj-$(CONFIG_TI_SCI_PM_DOMAINS)        += ti_sci_pm_domains.o
>>>>    obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN)    += ti_sci_inta_msi.o
>>>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
>>>> new file mode 100644
>>>> index 0000000..7c89eb8
>>>> --- /dev/null
>>>> +++ b/drivers/soc/ti/omap_prm.c
>>>> @@ -0,0 +1,216 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * OMAP2+ PRM driver
>>>> + *
>>>> + * Copyright (C) 2019 Texas Instruments Incorporated -
>>>> http://www.ti.com/
>>>> + *    Tero Kristo <[e-mail]>
>>>> + */
>>>> +
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/reset-controller.h>
>>>> +#include <linux/delay.h>
>>>> +
>>>> +struct omap_rst_map {
>>>> +    s8 rst;
>>>> +    s8 st;
>>>> +};
>>>> +
>>>> +struct omap_prm_data {
>>>> +    u32 base;
>>>> +    const char *name;
>>>> +    u16 pwstctrl;
>>>> +    u16 pwstst;
>>>> +    u16 rstctl;
> 
> Minor nit, can you use rstctrl instead here so that it is in sync with
> the other variables and with the register names used in TRM.

Ok.

> 
>>>> +    u16 rstst;
>>>> +    struct omap_rst_map *rstmap;
>>>> +    u8 flags;
>>>> +};
>>>> +
>>>> +struct omap_prm {
>>>> +    const struct omap_prm_data *data;
>>>> +    void __iomem *base;
>>>> +};
>>>> +
>>>> +struct omap_reset_data {
>>>> +    struct reset_controller_dev rcdev;
>>>> +    struct omap_prm *prm;
>>>> +};
>>>> +
>>>> +#define to_omap_reset_data(p) container_of((p), struct
>>>> omap_reset_data, rcdev)
>>>> +
>>>> +#define OMAP_MAX_RESETS        8
>>>> +#define OMAP_RESET_MAX_WAIT    10000
>>>> +
>>>> +#define OMAP_PRM_NO_RSTST    BIT(0)
>>>> +
>>>> +static const struct of_device_id omap_prm_id_table[] = {
>>>> +    { },
>>>> +};
>>>
>>> This table is blank and we are doing of_match_device against it.
>>
>> Yes, it gets populated with other patches in series, one entry per soc.
>>
>>>
>>>> +
>>>> +static int omap_reset_status(struct reset_controller_dev *rcdev,
>>>> +                 unsigned long id)
>>>> +{
>>>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>>>> +    u32 v;
>>>> +
>>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
>>>> +    v &= 1 << id;
>>>> +    v >>= id;
>>>> +
>>>> +    return v;
>>>> +}
>>>> +
>>>> +static int omap_reset_assert(struct reset_controller_dev *rcdev,
>>>> +                 unsigned long id)
>>>> +{
>>>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>>>> +    u32 v;
>>>> +
>>>> +    /* assert the reset control line */
>>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>>>> +    v |= 1 << id;
>>>> +    writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int omap_reset_get_st_bit(struct omap_reset_data *reset,
>>>> +                 unsigned long id)
>>>> +{
>>>> +    struct omap_rst_map *map = reset->prm->data->rstmap;
>>>> +
>>>> +    while (map && map->rst >= 0) {
>>>> +        if (map->rst == id)
>>>> +            return map->st;
>>>> +
>>>> +        map++;
>>>> +    }
>>>> +
>>>> +    return id;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Note that status will not change until clocks are on, and clocks
>>>> cannot be
>>>> + * enabled until reset is deasserted. Consumer drivers must check
>>>> status
>>>> + * separately after enabling clocks.
>>>> + */
>>>> +static int omap_reset_deassert(struct reset_controller_dev *rcdev,
>>>> +                   unsigned long id)
>>>> +{
>>>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>>>> +    u32 v;
>>>> +    int st_bit = id;
> 
> No need to initialize this, will always get overwritten below.

Hmm right, must be a leftover from some earlier code.

> 
>>>> +    bool has_rstst;
>>>> +
>>>> +    /* check the current status to avoid de-asserting the line twice */
>>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>>>> +    if (!(v & BIT(id)))
>>>> +        return -EEXIST;
>>>> +
>>>> +    has_rstst = !(reset->prm->data->flags & OMAP_PRM_NO_RSTST);
>>>> +
>>>> +    if (has_rstst) {
>>>> +        st_bit = omap_reset_get_st_bit(reset, id);
>>>> +
>>>> +        /* Clear the reset status by writing 1 to the status bit */
>>>> +        v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
>>>> +        v |= 1 << st_bit;
>>>> +        writel_relaxed(v, reset->prm->base + reset->prm->data->rstst);
>>>> +    }
>>>> +
>>>> +    /* de-assert the reset control line */
>>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>>>> +    v &= ~(1 << id);
>>>> +    writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static const struct reset_control_ops omap_reset_ops = {
>>>> +    .assert        = omap_reset_assert,
>>>> +    .deassert    = omap_reset_deassert,
>>>> +    .status        = omap_reset_status,
>>>> +};
>>>> +
>>>> +static int omap_prm_reset_probe(struct platform_device *pdev,
>>>> +                struct omap_prm *prm)
> 
> Call this omap_prm_reset_init or something similar to avoid confusion.

Ok, can change this.

> 
>>>> +{
>>>> +    struct omap_reset_data *reset;
>>>> +
>>>> +    /*
>>>> +     * Check if we have resets. If either rstctl or rstst is
>>>> +     * non-zero, we have reset registers in place. Additionally
>>>> +     * the flag OMAP_PRM_NO_RSTST implies that we have resets.
>>>> +     */
>>>> +    if (!prm->data->rstctl && !prm->data->rstst &&
>>>> +        !(prm->data->flags & OMAP_PRM_NO_RSTST))
>>>> +        return 0;
>>>> +
>>>> +    reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
>>>> +    if (!reset)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    reset->rcdev.owner = THIS_MODULE;
>>>> +    reset->rcdev.ops = &omap_reset_ops;
>>>> +    reset->rcdev.of_node = pdev->dev.of_node;
>>>> +    reset->rcdev.nr_resets = OMAP_MAX_RESETS;
> 
> Suggest adding a number of resets to prm->data, and using it so that we
> don't even entertain any resets beyond the actual number of resets.

Hmm why bother? Accessing a stale reset bit will just cause access to a 
reserved bit in the reset register, doing basically nothing. Also, this 
would not work for am3/am4 wkup, as there is a single reset bit at an 
arbitrary position.

> 
> You actually seem to be using the bit-position directly in client data
> instead of a reset number. I am not sure if this is accepted practice
> with reset controllers, do you incur any memory wastage?

Reset numbering almost always seems to start from 0, I think the only 
exception to this is wkup_m3 on am3/am4. Introducing an additional 
arbitrary mapping for this doesn't seem to make any sense.

Also, resets are allocated on-need-basis, so it only allocates one 
instance for the reset control whatever the index is.

> 
>>>> +
>>>> +    reset->prm = prm;
>>>> +
>>>> +    return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
>>>> +}
>>>> +
>>>> +static int omap_prm_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct resource *res;
>>>> +    const struct omap_prm_data *data;
>>>> +    struct omap_prm *prm;
>>>> +    const struct of_device_id *match;
>>>> +
>>>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +    if (!res)
>>>> +        return -ENODEV;
>>>> +
>>>> +    match = of_match_device(omap_prm_id_table, &pdev->dev);
>>>> +    if (!match)
>>>> +        return -ENOTSUPP;
> 
> Use of_device_get_match_data() instead to do both match and get the
> data. That can perhaps be the first block.
> 
>>>> +
>>>> +    prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL);
>>>> +    if (!prm)
>>>> +        return -ENOMEM;
> 
> Perhaps move the allocate after the match check to streamline.

Ok, will check these two out.

> 
>>>> +
>>>> +    data = match->data;
>>>> +
>>>> +    while (data->base != res->start) {
>>>> +        if (!data->base)
>>>> +            return -EINVAL;
>>>> +        data++;
>>>> +    }
>>>> +
>>>> +    prm->data = data;
>>>> +
>>>> +    prm->base = devm_ioremap_resource(&pdev->dev, res);
>>>> +    if (!prm->base)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    return omap_prm_reset_probe(pdev, prm);
>>>> +}
>>>> +
>>>> +static struct platform_driver omap_prm_driver = {
>>>> +    .probe = omap_prm_probe,
>>>> +    .driver = {
>>>> +        .name        = KBUILD_MODNAME,
>>>> +        .of_match_table    = omap_prm_id_table,
>>>> +    },
>>>> +};
>>>> +builtin_platform_driver(omap_prm_driver);
>>>> +
>>>> +MODULE_ALIAS("platform:prm");
> 
> Drop this and use MODULE_DEVICE_TABLE instead on omap_prm_id_table if
> retaining, but I don't think these need to be defined.

Ok, will ditch them.

-Tero

> 
> regards
> Suman
> 
>>>> +MODULE_LICENSE("GPL v2");
>>>> +MODULE_DESCRIPTION("omap2+ prm driver");
>>>
>>> It is a builtin_platform_driver so do we need the MODULE*?
>>
>> Well, thats debatable, however some existing drivers do introduce this
>> even if they are builtin.
>>
>> -Tero
>> -- 
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Suman Anna Aug. 20, 2019, 4:47 p.m. UTC | #5

On 8/20/19 2:37 AM, Tero Kristo wrote:
> On 20.8.2019 2.01, Suman Anna wrote:
>> Hi Tero,
>>
>> On 8/19/19 4:32 AM, Tero Kristo wrote:
>>> On 08/08/2019 08:26, Keerthy wrote:
>>>>
>>>>
>>>> On 07/08/19 1:18 PM, Tero Kristo wrote:
>>>>> Add initial PRM (Power and Reset Management) driver for TI OMAP class
>>>>> SoCs. Initially this driver only supports reset control, but can be
>>>>> extended to support rest of the functionality, like powerdomain
>>>>> control, PRCM irq support etc.
>>>>>
>>>>> Signed-off-by: Tero Kristo <[e-mail]>
>>>>> ---
>>>>>    arch/arm/mach-omap2/Kconfig |   1 +
>>>>>    drivers/soc/ti/Makefile     |   1 +
>>>>>    drivers/soc/ti/omap_prm.c   | 216
>>>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>>>    3 files changed, 218 insertions(+)
>>>>>    create mode 100644 drivers/soc/ti/omap_prm.c
>>>>>
>>>>> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
>>>>> index fdb6743..42ad063 100644
>>>>> --- a/arch/arm/mach-omap2/Kconfig
>>>>> +++ b/arch/arm/mach-omap2/Kconfig
>>>>> @@ -109,6 +109,7 @@ config ARCH_OMAP2PLUS
>>>>>        select TI_SYSC
>>>>>        select OMAP_IRQCHIP
>>>>>        select CLKSRC_TI_32K
>>>>> +    select RESET_CONTROLLER
>>
>> Use ARCH_HAS_RESET_CONTROLLER instead.
> 
> Ok.
> 
>>
>>>>>        help
>>>>>          Systems based on OMAP2, OMAP3, OMAP4 or OMAP5
>>>>> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
>>>>> index b3868d3..788b5cd 100644
>>>>> --- a/drivers/soc/ti/Makefile
>>>>> +++ b/drivers/soc/ti/Makefile
>>>>> @@ -6,6 +6,7 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS)    +=
>>>>> knav_qmss.o
>>>>>    knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o
>>>>>    obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA)    += knav_dma.o
>>>>>    obj-$(CONFIG_AMX3_PM)            += pm33xx.o
>>>>> +obj-$(CONFIG_ARCH_OMAP2PLUS)        += omap_prm.o
>>>>>    obj-$(CONFIG_WKUP_M3_IPC)        += wkup_m3_ipc.o
>>>>>    obj-$(CONFIG_TI_SCI_PM_DOMAINS)        += ti_sci_pm_domains.o
>>>>>    obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN)    += ti_sci_inta_msi.o
>>>>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
>>>>> new file mode 100644
>>>>> index 0000000..7c89eb8
>>>>> --- /dev/null
>>>>> +++ b/drivers/soc/ti/omap_prm.c
>>>>> @@ -0,0 +1,216 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +/*
>>>>> + * OMAP2+ PRM driver
>>>>> + *
>>>>> + * Copyright (C) 2019 Texas Instruments Incorporated -
>>>>> http://www.ti.com/
>>>>> + *    Tero Kristo <[e-mail]>
>>>>> + */
>>>>> +
>>>>> +#include <linux/kernel.h>
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/device.h>
>>>>> +#include <linux/io.h>
>>>>> +#include <linux/of.h>
>>>>> +#include <linux/of_device.h>
>>>>> +#include <linux/platform_device.h>
>>>>> +#include <linux/reset-controller.h>
>>>>> +#include <linux/delay.h>
>>>>> +
>>>>> +struct omap_rst_map {
>>>>> +    s8 rst;
>>>>> +    s8 st;
>>>>> +};
>>>>> +
>>>>> +struct omap_prm_data {
>>>>> +    u32 base;
>>>>> +    const char *name;
>>>>> +    u16 pwstctrl;
>>>>> +    u16 pwstst;
>>>>> +    u16 rstctl;
>>
>> Minor nit, can you use rstctrl instead here so that it is in sync with
>> the other variables and with the register names used in TRM.
> 
> Ok.
> 
>>
>>>>> +    u16 rstst;
>>>>> +    struct omap_rst_map *rstmap;
>>>>> +    u8 flags;
>>>>> +};
>>>>> +
>>>>> +struct omap_prm {
>>>>> +    const struct omap_prm_data *data;
>>>>> +    void __iomem *base;
>>>>> +};
>>>>> +
>>>>> +struct omap_reset_data {
>>>>> +    struct reset_controller_dev rcdev;
>>>>> +    struct omap_prm *prm;
>>>>> +};
>>>>> +
>>>>> +#define to_omap_reset_data(p) container_of((p), struct
>>>>> omap_reset_data, rcdev)
>>>>> +
>>>>> +#define OMAP_MAX_RESETS        8
>>>>> +#define OMAP_RESET_MAX_WAIT    10000
>>>>> +
>>>>> +#define OMAP_PRM_NO_RSTST    BIT(0)
>>>>> +
>>>>> +static const struct of_device_id omap_prm_id_table[] = {
>>>>> +    { },
>>>>> +};
>>>>
>>>> This table is blank and we are doing of_match_device against it.
>>>
>>> Yes, it gets populated with other patches in series, one entry per soc.
>>>
>>>>
>>>>> +
>>>>> +static int omap_reset_status(struct reset_controller_dev *rcdev,
>>>>> +                 unsigned long id)
>>>>> +{
>>>>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>>>>> +    u32 v;
>>>>> +
>>>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
>>>>> +    v &= 1 << id;
>>>>> +    v >>= id;
>>>>> +
>>>>> +    return v;
>>>>> +}
>>>>> +
>>>>> +static int omap_reset_assert(struct reset_controller_dev *rcdev,
>>>>> +                 unsigned long id)
>>>>> +{
>>>>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>>>>> +    u32 v;
>>>>> +
>>>>> +    /* assert the reset control line */
>>>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>>>>> +    v |= 1 << id;
>>>>> +    writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static int omap_reset_get_st_bit(struct omap_reset_data *reset,
>>>>> +                 unsigned long id)
>>>>> +{
>>>>> +    struct omap_rst_map *map = reset->prm->data->rstmap;
>>>>> +
>>>>> +    while (map && map->rst >= 0) {
>>>>> +        if (map->rst == id)
>>>>> +            return map->st;
>>>>> +
>>>>> +        map++;
>>>>> +    }
>>>>> +
>>>>> +    return id;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * Note that status will not change until clocks are on, and clocks
>>>>> cannot be
>>>>> + * enabled until reset is deasserted. Consumer drivers must check
>>>>> status
>>>>> + * separately after enabling clocks.
>>>>> + */
>>>>> +static int omap_reset_deassert(struct reset_controller_dev *rcdev,
>>>>> +                   unsigned long id)
>>>>> +{
>>>>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>>>>> +    u32 v;
>>>>> +    int st_bit = id;
>>
>> No need to initialize this, will always get overwritten below.
> 
> Hmm right, must be a leftover from some earlier code.
> 
>>
>>>>> +    bool has_rstst;
>>>>> +
>>>>> +    /* check the current status to avoid de-asserting the line
>>>>> twice */
>>>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>>>>> +    if (!(v & BIT(id)))
>>>>> +        return -EEXIST;
>>>>> +
>>>>> +    has_rstst = !(reset->prm->data->flags & OMAP_PRM_NO_RSTST);
>>>>> +
>>>>> +    if (has_rstst) {
>>>>> +        st_bit = omap_reset_get_st_bit(reset, id);
>>>>> +
>>>>> +        /* Clear the reset status by writing 1 to the status bit */
>>>>> +        v = readl_relaxed(reset->prm->base +
>>>>> reset->prm->data->rstst);
>>>>> +        v |= 1 << st_bit;
>>>>> +        writel_relaxed(v, reset->prm->base +
>>>>> reset->prm->data->rstst);
>>>>> +    }
>>>>> +
>>>>> +    /* de-assert the reset control line */
>>>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>>>>> +    v &= ~(1 << id);
>>>>> +    writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct reset_control_ops omap_reset_ops = {
>>>>> +    .assert        = omap_reset_assert,
>>>>> +    .deassert    = omap_reset_deassert,
>>>>> +    .status        = omap_reset_status,
>>>>> +};
>>>>> +
>>>>> +static int omap_prm_reset_probe(struct platform_device *pdev,
>>>>> +                struct omap_prm *prm)
>>
>> Call this omap_prm_reset_init or something similar to avoid confusion.
> 
> Ok, can change this.
> 
>>
>>>>> +{
>>>>> +    struct omap_reset_data *reset;
>>>>> +
>>>>> +    /*
>>>>> +     * Check if we have resets. If either rstctl or rstst is
>>>>> +     * non-zero, we have reset registers in place. Additionally
>>>>> +     * the flag OMAP_PRM_NO_RSTST implies that we have resets.
>>>>> +     */
>>>>> +    if (!prm->data->rstctl && !prm->data->rstst &&
>>>>> +        !(prm->data->flags & OMAP_PRM_NO_RSTST))
>>>>> +        return 0;
>>>>> +
>>>>> +    reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
>>>>> +    if (!reset)
>>>>> +        return -ENOMEM;
>>>>> +
>>>>> +    reset->rcdev.owner = THIS_MODULE;
>>>>> +    reset->rcdev.ops = &omap_reset_ops;
>>>>> +    reset->rcdev.of_node = pdev->dev.of_node;
>>>>> +    reset->rcdev.nr_resets = OMAP_MAX_RESETS;
>>
>> Suggest adding a number of resets to prm->data, and using it so that we
>> don't even entertain any resets beyond the actual number of resets.
> 
> Hmm why bother? Accessing a stale reset bit will just cause access to a
> reserved bit in the reset register, doing basically nothing. Also, this
> would not work for am3/am4 wkup, as there is a single reset bit at an
> arbitrary position.

The generic convention seems to be defining a reset id value defined
from include/dt-bindings/reset/ that can be used to match between the
dt-nodes and the reset-controller driver.

Philipp,
Any comments?

regards
Suman

> 
>>
>> You actually seem to be using the bit-position directly in client data
>> instead of a reset number. I am not sure if this is accepted practice
>> with reset controllers, do you incur any memory wastage?
> 
> Reset numbering almost always seems to start from 0, I think the only
> exception to this is wkup_m3 on am3/am4. Introducing an additional
> arbitrary mapping for this doesn't seem to make any sense.
> 
> Also, resets are allocated on-need-basis, so it only allocates one
> instance for the reset control whatever the index is.
> 
>>
>>>>> +
>>>>> +    reset->prm = prm;
>>>>> +
>>>>> +    return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
>>>>> +}
>>>>> +
>>>>> +static int omap_prm_probe(struct platform_device *pdev)
>>>>> +{
>>>>> +    struct resource *res;
>>>>> +    const struct omap_prm_data *data;
>>>>> +    struct omap_prm *prm;
>>>>> +    const struct of_device_id *match;
>>>>> +
>>>>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>> +    if (!res)
>>>>> +        return -ENODEV;
>>>>> +
>>>>> +    match = of_match_device(omap_prm_id_table, &pdev->dev);
>>>>> +    if (!match)
>>>>> +        return -ENOTSUPP;
>>
>> Use of_device_get_match_data() instead to do both match and get the
>> data. That can perhaps be the first block.
>>
>>>>> +
>>>>> +    prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL);
>>>>> +    if (!prm)
>>>>> +        return -ENOMEM;
>>
>> Perhaps move the allocate after the match check to streamline.
> 
> Ok, will check these two out.
> 
>>
>>>>> +
>>>>> +    data = match->data;
>>>>> +
>>>>> +    while (data->base != res->start) {
>>>>> +        if (!data->base)
>>>>> +            return -EINVAL;
>>>>> +        data++;
>>>>> +    }
>>>>> +
>>>>> +    prm->data = data;
>>>>> +
>>>>> +    prm->base = devm_ioremap_resource(&pdev->dev, res);
>>>>> +    if (!prm->base)
>>>>> +        return -ENOMEM;
>>>>> +
>>>>> +    return omap_prm_reset_probe(pdev, prm);
>>>>> +}
>>>>> +
>>>>> +static struct platform_driver omap_prm_driver = {
>>>>> +    .probe = omap_prm_probe,
>>>>> +    .driver = {
>>>>> +        .name        = KBUILD_MODNAME,
>>>>> +        .of_match_table    = omap_prm_id_table,
>>>>> +    },
>>>>> +};
>>>>> +builtin_platform_driver(omap_prm_driver);
>>>>> +
>>>>> +MODULE_ALIAS("platform:prm");
>>
>> Drop this and use MODULE_DEVICE_TABLE instead on omap_prm_id_table if
>> retaining, but I don't think these need to be defined.
> 
> Ok, will ditch them.
> 
> -Tero
> 
>>
>> regards
>> Suman
>>
>>>>> +MODULE_LICENSE("GPL v2");
>>>>> +MODULE_DESCRIPTION("omap2+ prm driver");
>>>>
>>>> It is a builtin_platform_driver so do we need the MODULE*?
>>>
>>> Well, thats debatable, however some existing drivers do introduce this
>>> even if they are builtin.
>>>
>>> -Tero
>>> -- 
>>
> 
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Philipp Zabel Aug. 21, 2019, 3:10 p.m. UTC | #6

On Tue, 2019-08-20 at 11:47 -0500, Suman Anna wrote:
> On 8/20/19 2:37 AM, Tero Kristo wrote:
> > On 20.8.2019 2.01, Suman Anna wrote:
> > > Hi Tero,
> > > 
> > > On 8/19/19 4:32 AM, Tero Kristo wrote:
[...]
> > > > > > +{
> > > > > > +    struct omap_reset_data *reset;
> > > > > > +
> > > > > > +    /*
> > > > > > +     * Check if we have resets. If either rstctl or rstst is
> > > > > > +     * non-zero, we have reset registers in place. Additionally
> > > > > > +     * the flag OMAP_PRM_NO_RSTST implies that we have resets.
> > > > > > +     */
> > > > > > +    if (!prm->data->rstctl && !prm->data->rstst &&
> > > > > > +        !(prm->data->flags & OMAP_PRM_NO_RSTST))
> > > > > > +        return 0;
> > > > > > +
> > > > > > +    reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
> > > > > > +    if (!reset)
> > > > > > +        return -ENOMEM;
> > > > > > +
> > > > > > +    reset->rcdev.owner = THIS_MODULE;
> > > > > > +    reset->rcdev.ops = &omap_reset_ops;
> > > > > > +    reset->rcdev.of_node = pdev->dev.of_node;
> > > > > > +    reset->rcdev.nr_resets = OMAP_MAX_RESETS;
> > > 
> > > Suggest adding a number of resets to prm->data, and using it so that we
> > > don't even entertain any resets beyond the actual number of resets.
> > 
> > Hmm why bother? Accessing a stale reset bit will just cause access to a
> > reserved bit in the reset register, doing basically nothing. Also, this
> > would not work for am3/am4 wkup, as there is a single reset bit at an
> > arbitrary position.
> 
> The generic convention seems to be defining a reset id value defined
> from include/dt-bindings/reset/ that can be used to match between the
> dt-nodes and the reset-controller driver.
> 
> Philipp,
> Any comments?

Are there only reset bits and reserved bits in the range accessible by
[0..OMAP_MAX_RESETS] or are ther bits with another function as well?
If the latter is the case, I would prefer enumerating the resets in a
dt-bindings header, with the driver containing an enum -> reg/bit
position lookup table.

In general, assuming the device tree contains no errors, this should not
matter much, but I think it is nice if the reset driver, even with a
misconfigured device tree, can't write into arbitrary bit fields.

regards
Philipp

Suman Anna Aug. 21, 2019, 3:45 p.m. UTC | #7

On 8/21/19 10:10 AM, Philipp Zabel wrote:
> On Tue, 2019-08-20 at 11:47 -0500, Suman Anna wrote:
>> On 8/20/19 2:37 AM, Tero Kristo wrote:
>>> On 20.8.2019 2.01, Suman Anna wrote:
>>>> Hi Tero,
>>>>
>>>> On 8/19/19 4:32 AM, Tero Kristo wrote:
> [...]
>>>>>>> +{
>>>>>>> +    struct omap_reset_data *reset;
>>>>>>> +
>>>>>>> +    /*
>>>>>>> +     * Check if we have resets. If either rstctl or rstst is
>>>>>>> +     * non-zero, we have reset registers in place. Additionally
>>>>>>> +     * the flag OMAP_PRM_NO_RSTST implies that we have resets.
>>>>>>> +     */
>>>>>>> +    if (!prm->data->rstctl && !prm->data->rstst &&
>>>>>>> +        !(prm->data->flags & OMAP_PRM_NO_RSTST))
>>>>>>> +        return 0;
>>>>>>> +
>>>>>>> +    reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
>>>>>>> +    if (!reset)
>>>>>>> +        return -ENOMEM;
>>>>>>> +
>>>>>>> +    reset->rcdev.owner = THIS_MODULE;
>>>>>>> +    reset->rcdev.ops = &omap_reset_ops;
>>>>>>> +    reset->rcdev.of_node = pdev->dev.of_node;
>>>>>>> +    reset->rcdev.nr_resets = OMAP_MAX_RESETS;
>>>>
>>>> Suggest adding a number of resets to prm->data, and using it so that we
>>>> don't even entertain any resets beyond the actual number of resets.
>>>
>>> Hmm why bother? Accessing a stale reset bit will just cause access to a
>>> reserved bit in the reset register, doing basically nothing. Also, this
>>> would not work for am3/am4 wkup, as there is a single reset bit at an
>>> arbitrary position.
>>
>> The generic convention seems to be defining a reset id value defined
>> from include/dt-bindings/reset/ that can be used to match between the
>> dt-nodes and the reset-controller driver.
>>
>> Philipp,
>> Any comments?
> 
> Are there only reset bits and reserved bits in the range accessible by
> [0..OMAP_MAX_RESETS] or are ther bits with another function as well?

Thanks Philipp, these are just reset bits and reserved bits.

> If the latter is the case, I would prefer enumerating the resets in a
> dt-bindings header, with the driver containing an enum -> reg/bit
> position lookup table.
> 
> In general, assuming the device tree contains no errors, this should not
> matter much, but I think it is nice if the reset driver, even with a
> misconfigured device tree, can't write into arbitrary bit fields.

Tero,
Can you add a check for this if possible?

regards
Suman

Tero Kristo Aug. 21, 2019, 6:15 p.m. UTC | #8

On 21.8.2019 18.45, Suman Anna wrote:
> On 8/21/19 10:10 AM, Philipp Zabel wrote:
>> On Tue, 2019-08-20 at 11:47 -0500, Suman Anna wrote:
>>> On 8/20/19 2:37 AM, Tero Kristo wrote:
>>>> On 20.8.2019 2.01, Suman Anna wrote:
>>>>> Hi Tero,
>>>>>
>>>>> On 8/19/19 4:32 AM, Tero Kristo wrote:
>> [...]
>>>>>>>> +{
>>>>>>>> +    struct omap_reset_data *reset;
>>>>>>>> +
>>>>>>>> +    /*
>>>>>>>> +     * Check if we have resets. If either rstctl or rstst is
>>>>>>>> +     * non-zero, we have reset registers in place. Additionally
>>>>>>>> +     * the flag OMAP_PRM_NO_RSTST implies that we have resets.
>>>>>>>> +     */
>>>>>>>> +    if (!prm->data->rstctl && !prm->data->rstst &&
>>>>>>>> +        !(prm->data->flags & OMAP_PRM_NO_RSTST))
>>>>>>>> +        return 0;
>>>>>>>> +
>>>>>>>> +    reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
>>>>>>>> +    if (!reset)
>>>>>>>> +        return -ENOMEM;
>>>>>>>> +
>>>>>>>> +    reset->rcdev.owner = THIS_MODULE;
>>>>>>>> +    reset->rcdev.ops = &omap_reset_ops;
>>>>>>>> +    reset->rcdev.of_node = pdev->dev.of_node;
>>>>>>>> +    reset->rcdev.nr_resets = OMAP_MAX_RESETS;
>>>>>
>>>>> Suggest adding a number of resets to prm->data, and using it so that we
>>>>> don't even entertain any resets beyond the actual number of resets.
>>>>
>>>> Hmm why bother? Accessing a stale reset bit will just cause access to a
>>>> reserved bit in the reset register, doing basically nothing. Also, this
>>>> would not work for am3/am4 wkup, as there is a single reset bit at an
>>>> arbitrary position.
>>>
>>> The generic convention seems to be defining a reset id value defined
>>> from include/dt-bindings/reset/ that can be used to match between the
>>> dt-nodes and the reset-controller driver.
>>>
>>> Philipp,
>>> Any comments?
>>
>> Are there only reset bits and reserved bits in the range accessible by
>> [0..OMAP_MAX_RESETS] or are ther bits with another function as well?
> 
> Thanks Philipp, these are just reset bits and reserved bits.
> 
>> If the latter is the case, I would prefer enumerating the resets in a
>> dt-bindings header, with the driver containing an enum -> reg/bit
>> position lookup table.
>>
>> In general, assuming the device tree contains no errors, this should not
>> matter much, but I think it is nice if the reset driver, even with a
>> misconfigured device tree, can't write into arbitrary bit fields.
> 
> Tero,
> Can you add a check for this if possible?

Well, I can enforce the usage of reset bit mapping, which I have already 
implemented for some SoCs like am33xx. If the specific ID is not found, 
I can bail out. So, basically in this example requesting reset at index 
3 would succeed, but it would fail for any other ID; this would be 
direct HW bit mapping.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Philipp Zabel Aug. 23, 2019, 8:50 a.m. UTC | #9

On Wed, 2019-08-21 at 21:15 +0300, Tero Kristo wrote:
> On 21.8.2019 18.45, Suman Anna wrote:
> > On 8/21/19 10:10 AM, Philipp Zabel wrote:
[...]
> > > In general, assuming the device tree contains no errors, this should not
> > > matter much, but I think it is nice if the reset driver, even with a
> > > misconfigured device tree, can't write into arbitrary bit fields.
> > 
> > Tero,
> > Can you add a check for this if possible?
> 
> Well, I can enforce the usage of reset bit mapping, which I have already 
> implemented for some SoCs like am33xx. If the specific ID is not found, 
> I can bail out. So, basically in this example requesting reset at index 
> 3 would succeed, but it would fail for any other ID; this would be 
> direct HW bit mapping.

That should be fine.

regards
Philipp

Tero Kristo Aug. 23, 2019, 11:27 a.m. UTC | #10

On 23.8.2019 11.50, Philipp Zabel wrote:
> On Wed, 2019-08-21 at 21:15 +0300, Tero Kristo wrote:
>> On 21.8.2019 18.45, Suman Anna wrote:
>>> On 8/21/19 10:10 AM, Philipp Zabel wrote:
> [...]
>>>> In general, assuming the device tree contains no errors, this should not
>>>> matter much, but I think it is nice if the reset driver, even with a
>>>> misconfigured device tree, can't write into arbitrary bit fields.
>>>
>>> Tero,
>>> Can you add a check for this if possible?
>>
>> Well, I can enforce the usage of reset bit mapping, which I have already
>> implemented for some SoCs like am33xx. If the specific ID is not found,
>> I can bail out. So, basically in this example requesting reset at index
>> 3 would succeed, but it would fail for any other ID; this would be
>> direct HW bit mapping.
> 
> That should be fine.

Ok, I am re-working it like this locally right now.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Patch

diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index fdb6743..42ad063 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -109,6 +109,7 @@  config ARCH_OMAP2PLUS
 	select TI_SYSC
 	select OMAP_IRQCHIP
 	select CLKSRC_TI_32K
+	select RESET_CONTROLLER
 	help
 	  Systems based on OMAP2, OMAP3, OMAP4 or OMAP5
 
diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
index b3868d3..788b5cd 100644
--- a/drivers/soc/ti/Makefile
+++ b/drivers/soc/ti/Makefile
@@ -6,6 +6,7 @@  obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS)	+= knav_qmss.o
 knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o
 obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA)	+= knav_dma.o
 obj-$(CONFIG_AMX3_PM)			+= pm33xx.o
+obj-$(CONFIG_ARCH_OMAP2PLUS)		+= omap_prm.o
 obj-$(CONFIG_WKUP_M3_IPC)		+= wkup_m3_ipc.o
 obj-$(CONFIG_TI_SCI_PM_DOMAINS)		+= ti_sci_pm_domains.o
 obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN)	+= ti_sci_inta_msi.o
diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
new file mode 100644
index 0000000..7c89eb8
--- /dev/null
+++ b/drivers/soc/ti/omap_prm.c
@@ -0,0 +1,216 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * OMAP2+ PRM driver
+ *
+ * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
+ *	Tero Kristo <[e-mail]>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/delay.h>
+
+struct omap_rst_map {
+	s8 rst;
+	s8 st;
+};
+
+struct omap_prm_data {
+	u32 base;
+	const char *name;
+	u16 pwstctrl;
+	u16 pwstst;
+	u16 rstctl;
+	u16 rstst;
+	struct omap_rst_map *rstmap;
+	u8 flags;
+};
+
+struct omap_prm {
+	const struct omap_prm_data *data;
+	void __iomem *base;
+};
+
+struct omap_reset_data {
+	struct reset_controller_dev rcdev;
+	struct omap_prm *prm;
+};
+
+#define to_omap_reset_data(p) container_of((p), struct omap_reset_data, rcdev)
+
+#define OMAP_MAX_RESETS		8
+#define OMAP_RESET_MAX_WAIT	10000
+
+#define OMAP_PRM_NO_RSTST	BIT(0)
+
+static const struct of_device_id omap_prm_id_table[] = {
+	{ },
+};
+
+static int omap_reset_status(struct reset_controller_dev *rcdev,
+			     unsigned long id)
+{
+	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
+	u32 v;
+
+	v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
+	v &= 1 << id;
+	v >>= id;
+
+	return v;
+}
+
+static int omap_reset_assert(struct reset_controller_dev *rcdev,
+			     unsigned long id)
+{
+	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
+	u32 v;
+
+	/* assert the reset control line */
+	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
+	v |= 1 << id;
+	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
+
+	return 0;
+}
+
+static int omap_reset_get_st_bit(struct omap_reset_data *reset,
+				 unsigned long id)
+{
+	struct omap_rst_map *map = reset->prm->data->rstmap;
+
+	while (map && map->rst >= 0) {
+		if (map->rst == id)
+			return map->st;
+
+		map++;
+	}
+
+	return id;
+}
+
+/*
+ * Note that status will not change until clocks are on, and clocks cannot be
+ * enabled until reset is deasserted. Consumer drivers must check status
+ * separately after enabling clocks.
+ */
+static int omap_reset_deassert(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
+	u32 v;
+	int st_bit = id;
+	bool has_rstst;
+
+	/* check the current status to avoid de-asserting the line twice */
+	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
+	if (!(v & BIT(id)))
+		return -EEXIST;
+
+	has_rstst = !(reset->prm->data->flags & OMAP_PRM_NO_RSTST);
+
+	if (has_rstst) {
+		st_bit = omap_reset_get_st_bit(reset, id);
+
+		/* Clear the reset status by writing 1 to the status bit */
+		v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
+		v |= 1 << st_bit;
+		writel_relaxed(v, reset->prm->base + reset->prm->data->rstst);
+	}
+
+	/* de-assert the reset control line */
+	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
+	v &= ~(1 << id);
+	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
+
+	return 0;
+}
+
+static const struct reset_control_ops omap_reset_ops = {
+	.assert		= omap_reset_assert,
+	.deassert	= omap_reset_deassert,
+	.status		= omap_reset_status,
+};
+
+static int omap_prm_reset_probe(struct platform_device *pdev,
+				struct omap_prm *prm)
+{
+	struct omap_reset_data *reset;
+
+	/*
+	 * Check if we have resets. If either rstctl or rstst is
+	 * non-zero, we have reset registers in place. Additionally
+	 * the flag OMAP_PRM_NO_RSTST implies that we have resets.
+	 */
+	if (!prm->data->rstctl && !prm->data->rstst &&
+	    !(prm->data->flags & OMAP_PRM_NO_RSTST))
+		return 0;
+
+	reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
+	if (!reset)
+		return -ENOMEM;
+
+	reset->rcdev.owner = THIS_MODULE;
+	reset->rcdev.ops = &omap_reset_ops;
+	reset->rcdev.of_node = pdev->dev.of_node;
+	reset->rcdev.nr_resets = OMAP_MAX_RESETS;
+
+	reset->prm = prm;
+
+	return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
+}
+
+static int omap_prm_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	const struct omap_prm_data *data;
+	struct omap_prm *prm;
+	const struct of_device_id *match;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	match = of_match_device(omap_prm_id_table, &pdev->dev);
+	if (!match)
+		return -ENOTSUPP;
+
+	prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL);
+	if (!prm)
+		return -ENOMEM;
+
+	data = match->data;
+
+	while (data->base != res->start) {
+		if (!data->base)
+			return -EINVAL;
+		data++;
+	}
+
+	prm->data = data;
+
+	prm->base = devm_ioremap_resource(&pdev->dev, res);
+	if (!prm->base)
+		return -ENOMEM;
+
+	return omap_prm_reset_probe(pdev, prm);
+}
+
+static struct platform_driver omap_prm_driver = {
+	.probe = omap_prm_probe,
+	.driver = {
+		.name		= KBUILD_MODNAME,
+		.of_match_table	= omap_prm_id_table,
+	},
+};
+builtin_platform_driver(omap_prm_driver);
+
+MODULE_ALIAS("platform:prm");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("omap2+ prm driver");