From: Suman Anna <s-anna@ti.com>
To: Tero Kristo <t-kristo@ti.com>, Keerthy <j-keerthy@ti.com>,
<ssantosh@kernel.org>, <linux-arm-kernel@lists.infradead.org>,
<linux-omap@vger.kernel.org>, <robh+dt@kernel.org>,
Philipp Zabel <p.zabel@pengutronix.de>
Cc: tony@atomide.com, devicetree@vger.kernel.org
Subject: Re: [PATCH 2/8] soc: ti: add initial PRM driver with reset control support
Date: Tue, 20 Aug 2019 11:47:37 -0500 [thread overview]
Message-ID: <a4196b73-63a0-f9d8-1c43-e6c4d1c1d6a4@ti.com> (raw)
In-Reply-To: <0f335aec-bfdf-345a-8dfb-dad70aef1af6@ti.com>
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 <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.
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-08-20 16:47 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-07 7:48 [PATCH 0/8] soc: ti: Add OMAP PRM driver Tero Kristo
2019-08-07 7:48 ` [PATCH 1/8] dt-bindings: omap: add new binding for PRM instances Tero Kristo
2019-08-08 4:35 ` Keerthy
2019-08-19 9:28 ` Tero Kristo
2019-08-19 21:28 ` Suman Anna
2019-08-20 7:45 ` Tero Kristo
2019-08-07 7:48 ` [PATCH 2/8] soc: ti: add initial PRM driver with reset control support Tero Kristo
2019-08-08 5:26 ` Keerthy
2019-08-19 9:32 ` Tero Kristo
2019-08-19 23:01 ` Suman Anna
2019-08-20 7:37 ` Tero Kristo
2019-08-20 16:47 ` Suman Anna [this message]
2019-08-21 15:10 ` Philipp Zabel
2019-08-21 15:45 ` Suman Anna
2019-08-21 18:15 ` Tero Kristo
2019-08-23 8:50 ` Philipp Zabel
2019-08-23 11:27 ` Tero Kristo
2019-08-07 7:48 ` [PATCH 3/8] soc: ti: omap-prm: poll for reset complete during de-assert Tero Kristo
2019-08-07 7:48 ` [PATCH 4/8] soc: ti: omap-prm: add support for denying idle for reset clockdomain Tero Kristo
2019-08-19 23:16 ` Suman Anna
2019-08-20 7:51 ` Tero Kristo
2019-08-07 7:48 ` [PATCH 5/8] soc: ti: omap-prm: add omap4 PRM data Tero Kristo
2019-08-08 5:30 ` Keerthy
2019-08-19 9:32 ` Tero Kristo
2019-08-19 23:08 ` Suman Anna
2019-08-20 7:52 ` Tero Kristo
2019-08-20 17:23 ` Suman Anna
2019-08-21 6:38 ` Tero Kristo
2019-08-07 7:48 ` [PATCH 6/8] soc: ti: omap_prm: add data for am33xx Tero Kristo
2019-08-19 23:11 ` Suman Anna
2019-08-20 18:48 ` Suman Anna
2019-08-21 7:23 ` Tero Kristo
2019-08-21 15:49 ` Suman Anna
2019-08-07 7:48 ` [PATCH 7/8] soc: ti: omap-prm: add dra7 PRM data Tero Kristo
2019-08-19 23:12 ` Suman Anna
2019-08-20 19:03 ` Suman Anna
2019-08-21 7:36 ` Tero Kristo
2019-08-07 7:48 ` [PATCH 8/8] ARM: OMAP2+: pdata-quirks: add PRM data for reset support Tero Kristo
2019-08-19 23:20 ` [PATCH 0/8] soc: ti: Add OMAP PRM driver Suman Anna
2019-08-20 7:54 ` Tero Kristo
2019-08-20 16:51 ` Suman Anna
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a4196b73-63a0-f9d8-1c43-e6c4d1c1d6a4@ti.com \
--to=s-anna@ti.com \
--cc=devicetree@vger.kernel.org \
--cc=j-keerthy@ti.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-omap@vger.kernel.org \
--cc=p.zabel@pengutronix.de \
--cc=robh+dt@kernel.org \
--cc=ssantosh@kernel.org \
--cc=t-kristo@ti.com \
--cc=tony@atomide.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).