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

[Tero Kristo <t-kristo@ti.com>]

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 <t-kristo@ti.com>
>>>> ---
>>>>    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 <t-kristo@ti.com>
>>>> + */
>>>> +
>>>> +#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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel