* [PATCH] irqchip/ls-scfg-msi: update Layerscape SCFG MSI driver
From: Minghuan Lian @ 2016-10-25 12:39 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, devicetree
Cc: Jason Cooper, Roy Zang, Marc Zyngier, Stuart Yoder, Yang-Leo Li,
Minghuan Lian, Scott Wood, Mingkai Hu
1. The patch uses soc_device_match() to match the SoC family
and revision instead of DTS compatible, because compatible cannot
describe the SoC revision information.
2. The patch provides a new method to support Layerscape
SCFG MSI. It tries to assign a dedicated MSIR to every core.
When changing a MSI interrupt affinity, the MSI message
data will be changed to refer to a new MSIR that has
been associated with the core.
Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
---
The patch depends on https://patchwork.kernel.org/patch/9342915/
drivers/irqchip/irq-ls-scfg-msi.c | 444 +++++++++++++++++++++++++++++++-------
1 file changed, 367 insertions(+), 77 deletions(-)
diff --git a/drivers/irqchip/irq-ls-scfg-msi.c b/drivers/irqchip/irq-ls-scfg-msi.c
index 02cca74c..0245d8a 100644
--- a/drivers/irqchip/irq-ls-scfg-msi.c
+++ b/drivers/irqchip/irq-ls-scfg-msi.c
@@ -10,6 +10,7 @@
* published by the Free Software Foundation.
*/
+#include <linux/bitmap.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/msi.h>
@@ -17,23 +18,91 @@
#include <linux/irq.h>
#include <linux/irqchip/chained_irq.h>
#include <linux/irqdomain.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
#include <linux/of_pci.h>
#include <linux/of_platform.h>
#include <linux/spinlock.h>
+#include <linux/sys_soc.h>
-#define MSI_MAX_IRQS 32
-#define MSI_IBS_SHIFT 3
-#define MSIR 4
+#define LS_MSIR_NUM_MAX 4 /* MSIIR can index 4 MSI registers */
+#define IRQS_32_PER_MSIR 32
+#define IRQS_8_PER_MSIR 8
+
+#define MSIR_OFFSET(idx) ((idx) * 0x4)
+
+enum msi_affinity_flag {
+ MSI_GROUP_AFFINITY_FLAG,
+ MSI_AFFINITY_FLAG
+};
+
+struct ls_scfg_msi;
+struct ls_scfg_msi_ctrl;
+
+struct ls_scfg_msi_cfg {
+ u32 ibs_shift; /* Shift of interrupt bit select */
+ u32 msir_irqs; /* The irq number per MSIR */
+ u32 msir_base; /* The base address of MSIR */
+};
+
+struct ls_scfg_msir {
+ struct ls_scfg_msi_ctrl *ctrl;
+ void __iomem *addr;
+ int index;
+ int virq;
+};
+
+struct ls_scfg_msi_ctrl {
+ struct list_head list;
+ struct ls_scfg_msi *msi_data;
+ void __iomem *regs;
+ phys_addr_t msiir_addr;
+ enum msi_affinity_flag flag;
+ int irq_base;
+ spinlock_t lock;
+ struct ls_scfg_msir *msir;
+ unsigned long *bm;
+};
struct ls_scfg_msi {
- spinlock_t lock;
- struct platform_device *pdev;
- struct irq_domain *parent;
- struct irq_domain *msi_domain;
- void __iomem *regs;
- phys_addr_t msiir_addr;
- int irq;
- DECLARE_BITMAP(used, MSI_MAX_IRQS);
+ struct platform_device *pdev;
+ struct irq_domain *parent;
+ struct irq_domain *msi_domain;
+ struct list_head ctrl_list;
+ const struct ls_scfg_msi_cfg *cfg;
+ u32 cpu_num;
+};
+
+static struct ls_scfg_msi_cfg ls1021_msi_cfg = {
+ .ibs_shift = 3,
+ .msir_irqs = IRQS_32_PER_MSIR,
+ .msir_base = 0x4,
+};
+
+static struct ls_scfg_msi_cfg ls1043_rev11_msi_cfg = {
+ .ibs_shift = 2,
+ .msir_irqs = IRQS_8_PER_MSIR,
+ .msir_base = 0x10,
+};
+
+static struct ls_scfg_msi_cfg ls1046_msi_cfg = {
+ .ibs_shift = 2,
+ .msir_irqs = IRQS_32_PER_MSIR,
+ .msir_base = 0x4,
+};
+
+static struct soc_device_attribute soc_msi_matches[] = {
+ { .family = "QorIQ LS1021A",
+ .data = &ls1021_msi_cfg },
+ { .family = "QorIQ LS1012A",
+ .data = &ls1021_msi_cfg },
+ { .family = "QorIQ LS1043A", .revision = "1.0",
+ .data = &ls1021_msi_cfg },
+ { .family = "QorIQ LS1043A", .revision = "1.1",
+ .data = &ls1043_rev11_msi_cfg },
+ { .family = "QorIQ LS1046A",
+ .data = &ls1046_msi_cfg },
+ { },
};
static struct irq_chip ls_scfg_msi_irq_chip = {
@@ -49,19 +118,53 @@ struct ls_scfg_msi {
.chip = &ls_scfg_msi_irq_chip,
};
+static int ctrl_num;
+
+static irqreturn_t (*ls_scfg_msi_irq_handler)(int irq, void *arg);
+
static void ls_scfg_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
{
- struct ls_scfg_msi *msi_data = irq_data_get_irq_chip_data(data);
+ struct ls_scfg_msi_ctrl *ctrl = irq_data_get_irq_chip_data(data);
+ u32 ibs, srs;
- msg->address_hi = upper_32_bits(msi_data->msiir_addr);
- msg->address_lo = lower_32_bits(msi_data->msiir_addr);
- msg->data = data->hwirq << MSI_IBS_SHIFT;
+ msg->address_hi = upper_32_bits(ctrl->msiir_addr);
+ msg->address_lo = lower_32_bits(ctrl->msiir_addr);
+
+ ibs = data->hwirq - ctrl->irq_base;
+
+ srs = cpumask_first(irq_data_get_affinity_mask(data));
+ if (srs >= ctrl->msi_data->cpu_num)
+ srs = 0;
+
+ msg->data = ibs << ctrl->msi_data->cfg->ibs_shift | srs;
+
+ pr_debug("%s: ibs %d srs %d address0x%x-0x%x data 0x%x\n",
+ __func__, ibs, srs, msg->address_hi,
+ msg->address_lo, msg->data);
}
-static int ls_scfg_msi_set_affinity(struct irq_data *irq_data,
- const struct cpumask *mask, bool force)
+static int ls_scfg_msi_set_affinity(struct irq_data *data,
+ const struct cpumask *mask, bool force)
{
- return -EINVAL;
+ struct ls_scfg_msi_ctrl *ctrl = irq_data_get_irq_chip_data(data);
+ u32 cpu;
+
+ if (!force)
+ cpu = cpumask_any_and(mask, cpu_online_mask);
+ else
+ cpu = cpumask_first(mask);
+
+ if (cpu >= ctrl->msi_data->cpu_num)
+ return -EINVAL;
+
+ if (ctrl->msir[cpu].virq <= 0) {
+ pr_warn("cannot bind the irq to cpu%d\n", cpu);
+ return -EINVAL;
+ }
+
+ cpumask_copy(irq_data_get_affinity_mask(data), mask);
+
+ return IRQ_SET_MASK_OK_NOCOPY;
}
static struct irq_chip ls_scfg_msi_parent_chip = {
@@ -76,44 +179,57 @@ static int ls_scfg_msi_domain_irq_alloc(struct irq_domain *domain,
void *args)
{
struct ls_scfg_msi *msi_data = domain->host_data;
- int pos, err = 0;
+ static struct list_head *current_entry;
+ struct ls_scfg_msi_ctrl *ctrl;
+ int i, hwirq = -ENOMEM;
+
+ if (!current_entry || current_entry->next == &msi_data->ctrl_list)
+ current_entry = &msi_data->ctrl_list;
+
+ list_for_each_entry(ctrl, current_entry, list) {
+ spin_lock(&ctrl->lock);
+ hwirq = bitmap_find_free_region(ctrl->bm,
+ msi_data->cfg->msir_irqs,
+ order_base_2(nr_irqs));
+ spin_unlock(&ctrl->lock);
+
+ if (hwirq >= 0)
+ break;
+ }
- WARN_ON(nr_irqs != 1);
+ if (hwirq < 0)
+ return hwirq;
- spin_lock(&msi_data->lock);
- pos = find_first_zero_bit(msi_data->used, MSI_MAX_IRQS);
- if (pos < MSI_MAX_IRQS)
- __set_bit(pos, msi_data->used);
- else
- err = -ENOSPC;
- spin_unlock(&msi_data->lock);
+ hwirq = hwirq + ctrl->irq_base;
- if (err)
- return err;
+ for (i = 0; i < nr_irqs; i++)
+ irq_domain_set_info(domain, virq + i, hwirq + i,
+ &ls_scfg_msi_parent_chip, ctrl,
+ handle_simple_irq, NULL, NULL);
- irq_domain_set_info(domain, virq, pos,
- &ls_scfg_msi_parent_chip, msi_data,
- handle_simple_irq, NULL, NULL);
+ current_entry = &ctrl->list;
return 0;
}
static void ls_scfg_msi_domain_irq_free(struct irq_domain *domain,
- unsigned int virq, unsigned int nr_irqs)
+ unsigned int virq,
+ unsigned int nr_irqs)
{
struct irq_data *d = irq_domain_get_irq_data(domain, virq);
- struct ls_scfg_msi *msi_data = irq_data_get_irq_chip_data(d);
+ struct ls_scfg_msi_ctrl *ctrl = irq_data_get_irq_chip_data(d);
int pos;
- pos = d->hwirq;
- if (pos < 0 || pos >= MSI_MAX_IRQS) {
- pr_err("failed to teardown msi. Invalid hwirq %d\n", pos);
+ pos = d->hwirq - ctrl->irq_base;
+
+ if (pos < 0 || pos >= ctrl->msi_data->cfg->msir_irqs) {
+ pr_err("Failed to teardown msi. Invalid hwirq %d\n", pos);
return;
}
- spin_lock(&msi_data->lock);
- __clear_bit(pos, msi_data->used);
- spin_unlock(&msi_data->lock);
+ spin_lock(&ctrl->lock);
+ bitmap_release_region(ctrl->bm, pos, order_base_2(nr_irqs));
+ spin_unlock(&ctrl->lock);
}
static const struct irq_domain_ops ls_scfg_msi_domain_ops = {
@@ -121,29 +237,198 @@ static void ls_scfg_msi_domain_irq_free(struct irq_domain *domain,
.free = ls_scfg_msi_domain_irq_free,
};
-static void ls_scfg_msi_irq_handler(struct irq_desc *desc)
+static irqreturn_t ls_scfg_msi_irqs32_handler(int irq, void *arg)
{
- struct ls_scfg_msi *msi_data = irq_desc_get_handler_data(desc);
+ struct ls_scfg_msir *msir = arg;
+ struct ls_scfg_msi_ctrl *ctrl = msir->ctrl;
+ struct ls_scfg_msi *msi_data = ctrl->msi_data;
unsigned long val;
- int pos, virq;
+ int pos = 0, hwirq, virq;
+ irqreturn_t ret = IRQ_NONE;
- chained_irq_enter(irq_desc_get_chip(desc), desc);
+ val = ioread32be(msir->addr);
- val = ioread32be(msi_data->regs + MSIR);
- for_each_set_bit(pos, &val, MSI_MAX_IRQS) {
- virq = irq_find_mapping(msi_data->parent, (31 - pos));
- if (virq)
+ for_each_set_bit(pos, &val, IRQS_32_PER_MSIR) {
+ hwirq = (IRQS_32_PER_MSIR - 1 - pos) + ctrl->irq_base;
+ virq = irq_find_mapping(msi_data->parent, hwirq);
+ if (virq) {
generic_handle_irq(virq);
+ ret = IRQ_HANDLED;
+ }
+ }
+
+ return ret;
+}
+
+static irqreturn_t ls_scfg_msi_irqs8_handler(int irq, void *arg)
+{
+ struct ls_scfg_msir *msir = arg;
+ struct ls_scfg_msi_ctrl *ctrl = msir->ctrl;
+ struct ls_scfg_msi *msi_data = ctrl->msi_data;
+ unsigned long val;
+ int pos = 0, hwirq, virq;
+ irqreturn_t ret = IRQ_NONE;
+
+ val = ioread32be(msir->addr);
+ val = (val << (msir->index * 8)) & 0xff000000;
+
+ for_each_set_bit(pos, &val, IRQS_32_PER_MSIR) {
+ hwirq = (IRQS_32_PER_MSIR - 1 - pos) + ctrl->irq_base;
+ virq = irq_find_mapping(msi_data->parent, hwirq);
+ if (virq) {
+ generic_handle_irq(virq);
+ ret = IRQ_HANDLED;
+ }
+ }
+
+ return ret;
+}
+
+static void ls_scfg_msi_cascade(struct irq_desc *desc)
+{
+ struct ls_scfg_msir *msir = irq_desc_get_handler_data(desc);
+ struct irq_chip *chip = irq_desc_get_chip(desc);
+
+ chained_irq_enter(chip, desc);
+ ls_scfg_msi_irq_handler(desc->irq_data.irq, msir);
+ chained_irq_exit(chip, desc);
+}
+
+static int ls_scfg_msi_setup_hwirq(struct ls_scfg_msi_ctrl *ctrl,
+ struct device_node *node,
+ int index)
+{
+ struct ls_scfg_msir *msir = &ctrl->msir[index];
+ int ret;
+
+ msir->virq = of_irq_get(node, index);
+ if (msir->virq <= 0)
+ return -ENODEV;
+
+ msir->index = index;
+ msir->ctrl = ctrl;
+ msir->addr = ctrl->regs + ctrl->msi_data->cfg->msir_base +
+ MSIR_OFFSET(msir->index);
+
+ if (ctrl->flag == MSI_GROUP_AFFINITY_FLAG) {
+ ret = request_irq(msir->virq, ls_scfg_msi_irq_handler,
+ IRQF_NO_THREAD, "MSI-GROUP", msir);
+ if (ret) {
+ pr_err("failed to request irq %d\n", msir->virq);
+ msir->virq = 0;
+ return -ENODEV;
+ }
+ } else {
+ irq_set_chained_handler(msir->virq, ls_scfg_msi_cascade);
+ irq_set_handler_data(msir->virq, msir);
+ irq_set_affinity(msir->virq, get_cpu_mask(index));
+ }
+
+ return 0;
+}
+
+static void ls_scfg_msi_ctrl_remove(struct ls_scfg_msi_ctrl *ctrl)
+{
+ struct ls_scfg_msir *msir;
+ int i;
+
+ if (!ctrl)
+ return;
+
+ if (ctrl->msir) {
+ for (i = 0; i < ctrl->msi_data->cpu_num; i++) {
+ msir = &ctrl->msir[i];
+
+ if (msir->virq <= 0)
+ continue;
+
+ if (ctrl->flag == MSI_GROUP_AFFINITY_FLAG)
+ free_irq(msir->virq, msir);
+ else
+ irq_set_chained_handler_and_data(msir->virq,
+ NULL, NULL);
+ }
+
+ kfree(ctrl->msir);
}
- chained_irq_exit(irq_desc_get_chip(desc), desc);
+ if (ctrl->regs)
+ iounmap(ctrl->regs);
+
+ kfree(ctrl->bm);
+ kfree(ctrl);
+}
+
+static int ls_scfg_msi_ctrl_probe(struct device_node *node,
+ struct ls_scfg_msi *msi_data)
+{
+ struct ls_scfg_msi_ctrl *ctrl;
+ struct resource res;
+ int err, irqs, i;
+
+ err = of_address_to_resource(node, 0, &res);
+ if (err) {
+ pr_warn("%s: no regs\n", node->full_name);
+ return -ENXIO;
+ }
+
+ ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
+ if (!ctrl)
+ return -ENOMEM;
+
+ ctrl->msi_data = msi_data;
+ ctrl->msiir_addr = res.start;
+ spin_lock_init(&ctrl->lock);
+
+ ctrl->regs = ioremap(res.start, resource_size(&res));
+ if (!ctrl->regs) {
+ pr_err("%s: unable to map registers\n", node->full_name);
+ err = -ENOMEM;
+ goto _err;
+ }
+
+ ctrl->msir = kcalloc(msi_data->cpu_num, sizeof(struct ls_scfg_msir),
+ GFP_KERNEL);
+ if (!ctrl->msir) {
+ err = -ENOMEM;
+ goto _err;
+ }
+
+ ctrl->bm = kcalloc(BITS_TO_LONGS(msi_data->cfg->msir_irqs),
+ sizeof(long), GFP_KERNEL);
+ if (!ctrl->bm) {
+ err = -ENOMEM;
+ goto _err;
+ }
+
+ ctrl->irq_base = msi_data->cfg->msir_irqs * ctrl_num;
+ ctrl_num++;
+
+ irqs = of_irq_count(node);
+ if (irqs >= msi_data->cpu_num)
+ ctrl->flag = MSI_AFFINITY_FLAG;
+ else
+ ctrl->flag = MSI_GROUP_AFFINITY_FLAG;
+
+ for (i = 0; i < msi_data->cpu_num; i++)
+ ls_scfg_msi_setup_hwirq(ctrl, node, i);
+
+ list_add_tail(&ctrl->list, &msi_data->ctrl_list);
+
+ return 0;
+
+_err:
+ ls_scfg_msi_ctrl_remove(ctrl);
+ pr_err("MSI: failed probing %s (%d)\n", node->full_name, err);
+ return err;
}
static int ls_scfg_msi_domains_init(struct ls_scfg_msi *msi_data)
{
/* Initialize MSI domain parent */
msi_data->parent = irq_domain_add_linear(NULL,
- MSI_MAX_IRQS,
+ msi_data->cfg->msir_irqs *
+ ctrl_num,
&ls_scfg_msi_domain_ops,
msi_data);
if (!msi_data->parent) {
@@ -167,51 +452,57 @@ static int ls_scfg_msi_domains_init(struct ls_scfg_msi *msi_data)
static int ls_scfg_msi_probe(struct platform_device *pdev)
{
struct ls_scfg_msi *msi_data;
- struct resource *res;
- int ret;
+ const struct soc_device_attribute *match;
+ struct device_node *child;
msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
if (!msi_data)
return -ENOMEM;
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- msi_data->regs = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(msi_data->regs)) {
- dev_err(&pdev->dev, "failed to initialize 'regs'\n");
- return PTR_ERR(msi_data->regs);
- }
- msi_data->msiir_addr = res->start;
-
- msi_data->irq = platform_get_irq(pdev, 0);
- if (msi_data->irq <= 0) {
- dev_err(&pdev->dev, "failed to get MSI irq\n");
- return -ENODEV;
- }
+ INIT_LIST_HEAD(&msi_data->ctrl_list);
msi_data->pdev = pdev;
- spin_lock_init(&msi_data->lock);
+ msi_data->cpu_num = num_possible_cpus();
+
+ match = soc_device_match(soc_msi_matches);
+ if (match)
+ msi_data->cfg = match->data;
+ else
+ msi_data->cfg = &ls1046_msi_cfg;
+
+ if (msi_data->cfg->msir_irqs == IRQS_8_PER_MSIR)
+ ls_scfg_msi_irq_handler = ls_scfg_msi_irqs8_handler;
+ else
+ ls_scfg_msi_irq_handler = ls_scfg_msi_irqs32_handler;
- ret = ls_scfg_msi_domains_init(msi_data);
- if (ret)
- return ret;
+ for_each_child_of_node(msi_data->pdev->dev.of_node, child)
+ ls_scfg_msi_ctrl_probe(child, msi_data);
- irq_set_chained_handler_and_data(msi_data->irq,
- ls_scfg_msi_irq_handler,
- msi_data);
+ ls_scfg_msi_domains_init(msi_data);
platform_set_drvdata(pdev, msi_data);
+ dev_info(&pdev->dev, "irqs:%dx%d ibs_shift:%d msir_base:0x%x\n",
+ msi_data->cfg->msir_irqs, ctrl_num,
+ msi_data->cfg->ibs_shift, msi_data->cfg->msir_base);
+
return 0;
}
static int ls_scfg_msi_remove(struct platform_device *pdev)
{
struct ls_scfg_msi *msi_data = platform_get_drvdata(pdev);
+ struct ls_scfg_msi_ctrl *ctrl, *temp;
- irq_set_chained_handler_and_data(msi_data->irq, NULL, NULL);
+ list_for_each_entry_safe(ctrl, temp, &msi_data->ctrl_list, list) {
+ list_move_tail(&ctrl->list, &msi_data->ctrl_list);
+ ls_scfg_msi_ctrl_remove(ctrl);
+ }
- irq_domain_remove(msi_data->msi_domain);
- irq_domain_remove(msi_data->parent);
+ if (msi_data->msi_domain)
+ irq_domain_remove(msi_data->msi_domain);
+ if (msi_data->parent)
+ irq_domain_remove(msi_data->parent);
platform_set_drvdata(pdev, NULL);
@@ -219,8 +510,7 @@ static int ls_scfg_msi_remove(struct platform_device *pdev)
}
static const struct of_device_id ls_scfg_msi_id[] = {
- { .compatible = "fsl,1s1021a-msi", },
- { .compatible = "fsl,1s1043a-msi", },
+ { .compatible = "fsl,ls-scfg-msi" },
{},
};
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v3 01/12] dt: arm: shmobile: add H3ULCB board DT bindings
From: Geert Uytterhoeven @ 2016-10-25 12:48 UTC (permalink / raw)
To: Vladimir Barinov
Cc: Simon Horman, Magnus Damm, Rob Herring, Mark Rutland,
devicetree@vger.kernel.org, Linux-Renesas
In-Reply-To: <1472637741-14624-1-git-send-email-vladimir.barinov@cogentembedded.com>
Hi Vladimir,
On Wed, Aug 31, 2016 at 12:02 PM, Vladimir Barinov
<vladimir.barinov@cogentembedded.com> wrote:
> Add H3ULCB Device tree bindings Documentation, listing it as a supported
> board.
>
> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Changes in version 2:
> - none
> Changes in version 3:
> - none
>
> Documentation/devicetree/bindings/arm/shmobile.txt | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/shmobile.txt b/Documentation/devicetree/bindings/arm/shmobile.txt
> index 5484c31d..2f0b716 100644
> --- a/Documentation/devicetree/bindings/arm/shmobile.txt
> +++ b/Documentation/devicetree/bindings/arm/shmobile.txt
> @@ -49,6 +49,8 @@ Boards:
> compatible = "renesas,genmai", "renesas,r7s72100"
> - Gose
> compatible = "renesas,gose", "renesas,r8a7793"
> + - H3ULCB (RTP0RC7795SKB00010S)
> + compatible = "renesas,h3ulcb", "renesas,r8a7795";
Should this have been
H3SK (R-Car Starter Kit Premier, RTP0RC7795SKB00010S)
compatible = "renesas,h3sk", "renesas,r8a7795";
instead?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Device driver - parsing GPIOs from tree
From: colin.helliwell-ZqAGQOxBGgIxoycv7ERPSQ @ 2016-10-25 12:50 UTC (permalink / raw)
To: devicetree-u79uwXL29TY76Z2rM5mHXA
I'm writing my own device driver for some GPIOs on my hardware. I’ve
successfully created the pins in the Device Tree, and in my driver I’ve been
able to find the pins in the device tree using 'of_get_named_gpio_flags()'
when they are at a top level (e.g. ‘test-name’ shown below).
But for neatness I would prefer to group them hierarchically e.g.
rfctrl: rfctrl {
compatible = "wg2xx-rfctrl";
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_rfctrl>;
status = "okay";
test-name = <&gpio4 25 0>;
jn {
jnProgN = <&gpio4 26 0>;
};
alt {
altGpio0 = <&gpio4 27 0 >;
altGpio1 = <&gpio4 28 0 >;
altGpio2 = <&gpio4 29 0 >;
altGpio3 = <&gpio4 30 0 >;
};
};
Is there a way to search within the subtree ("jn" or "alt"), or to descend
into it and then use 'of_get_named_gpio_flags()’?
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH/RFT v2 09/17] regulator: fixed: Add over current event
From: Axel Haslam @ 2016-10-25 12:55 UTC (permalink / raw)
To: Mark Brown
Cc: Greg KH, Johan Hovold, robh+dt, Sekhar Nori, Alan Stern,
Kevin Hilman, Sergei Shtylyov, David Lechner, manjunath.goudar,
Alexandre Bailon, linux-usb, devicetree, linux-kernel,
linux-arm-kernel
In-Reply-To: <20161024181937.GP17252@sirena.org.uk>
Hi Mark,
On Mon, Oct 24, 2016 at 8:19 PM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Oct 24, 2016 at 08:11:40PM +0200, Axel Haslam wrote:
>> On Mon, Oct 24, 2016 at 7:53 PM, Mark Brown <broonie@kernel.org> wrote:
>
>> > does it make sense to report this as a mode, we don't report other error
>> > conditions as modes but instead use REGULATOR_STATUS_ with the
>> > get_status() operation?
>
>> I used mode, because when the regulator toggles the overcurrent
>> line, it means that it has entered a current limited mode, at least the
>> regulator im looking at. ill change to STATUS
>
> That's not what regulator modes are - please look at the documentation
> for the defines here. They're about the quality of regulation.
To be able to use regulator to handle the overcurrent pin, i need to be able
to somehow retrieve the over current pin state from the regulator driver.
As i was trying your suggestion, i remembered why i thought i should use
mode instead of status: Status seems to be for internal regulator driver use,
there is no regulator_get_status, function and REGULATOR_STATUS_* are defined
in driver.h and not in consumer.h as REGULATOR_MODE_*
it seems that status is only used to print sysfs info.
Would you be ok if i allow consumers to get the status via a new
"regulator_get_status" call?
Regards
Axel.
^ permalink raw reply
* Re: [PATCH 1/6] dt/bindings: adjust bindings for Layerscape SCFG MSI
From: Robin Murphy @ 2016-10-25 13:01 UTC (permalink / raw)
To: Minghuan Lian, linux-arm-kernel, linux-kernel, devicetree
Cc: Marc Zyngier, Stuart Yoder, Yang-Leo Li, Scott Wood, Shawn Guo,
Mingkai Hu
In-Reply-To: <1477398945-22774-1-git-send-email-Minghuan.Lian@nxp.com>
On 25/10/16 13:35, Minghuan Lian wrote:
> 1. The different version of a SoC may have different MSI
> implementation. But compatible "fsl,<soc-name>-msi" can not describe
> the SoC version.
Can't it?
compatible = "fsl-ls1043a-rev11-msi";
Oh, I guess it can!
Joking aside, if there are multiple versions of a piece of hardware
which require *different* treatment by drivers, then it is obviously
wrong to use the same compatible string because *they are not compatible*.
> The MSI driver will use SoC match interface to get
> SoC type and version instead of compatible string. So all MSI node
> can use the common compatible "fsl,ls-scfg-msi" and the original
> compatible is unnecessary.
If there is some common level of functionality that *all* variants
support without the driver having to know which one is which, then there
might be some sense in having an additional common compatible to
represent that level of functionality, e.g.
compatible = "fsl-ls1043a-rev11-msi", "fsl,ls-scfg-msi";
But if, say, new variants turn out to have less functionality, rather
than more, then there's probably not much point, and we should stick to
specific, accurate, compatible strings.
DT is not specific to a kernel version, nor even to Linux. A string
which triggers some board-specific magic in a specific version of a
Linux driver does not describe the hardware.
Robin.
> 2. Layerscape SoCs may have one or several MSI controllers.
> In order to increase MSI interrupt number of a PCIe, the patch
> moves all MSI node into the parent node "msi-controller". So a
> PCIe can request MSI from all the MSI controllers.
>
> Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
> ---
> .../interrupt-controller/fsl,ls-scfg-msi.txt | 57 +++++++++++++++++++---
> 1 file changed, 49 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt
> index 9e38949..29f95fd 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt
> @@ -1,18 +1,28 @@
> * Freescale Layerscape SCFG PCIe MSI controller
>
> +Layerscape SoCs may have one or multiple MSI controllers.
> +Each MSI controller must be showed as a child node.
> +
> Required properties:
>
> -- compatible: should be "fsl,<soc-name>-msi" to identify
> - Layerscape PCIe MSI controller block such as:
> - "fsl,1s1021a-msi"
> - "fsl,1s1043a-msi"
> +- compatible: should be "fsl,ls-scfg-msi"
> +- #address-cells: must be 2
> +- #size-cells: must be 2
> +- ranges: allows valid 1:1 translation between child's address space and
> + parent's address space
> - msi-controller: indicates that this is a PCIe MSI controller node
> +
> +Required child node:
> +A child node must exist to represent the MSI controller.
> +The following are properties specific to those nodes:
> +
> - reg: physical base address of the controller and length of memory mapped.
> - interrupts: an interrupt to the parent interrupt controller.
>
> Optional properties:
> - interrupt-parent: the phandle to the parent interrupt controller.
>
> +Notes:
> This interrupt controller hardware is a second level interrupt controller that
> is hooked to a parent interrupt controller: e.g: ARM GIC for ARM-based
> platforms. If interrupt-parent is not provided, the default parent interrupt
> @@ -22,9 +32,40 @@ MSI controller node
>
> Examples:
>
> - msi1: msi-controller@1571000 {
> - compatible = "fsl,1s1043a-msi";
> - reg = <0x0 0x1571000 0x0 0x8>,
> + msi: msi-controller {
> + compatible = "fsl,ls-scfg-msi";
> + #address-cells = <2>;
> + #size-cells = <2>;
> + ranges;
> msi-controller;
> - interrupts = <0 116 0x4>;
> +
> + msi0@1580000 {
> + reg = <0x0 0x1580000 0x0 0x10000>;
> + interrupts = <0 116 0x4>,
> + <0 111 0x4>,
> + <0 112 0x4>,
> + <0 113 0x4>;
> + };
> +
> + msi1@1590000 {
> + reg = <0x0 0x1590000 0x0 0x10000>;
> + interrupts = <0 126 0x4>,
> + <0 121 0x4>,
> + <0 122 0x4>,
> + <0 123 0x4>;
> + };
> +
> + msi2@15a0000 {
> + reg = <0x0 0x15a0000 0x0 0x10000>;
> + interrupts = <0 160 0x4>,
> + <0 155 0x4>,
> + <0 156 0x4>,
> + <0 157 0x4>;
> + };
> + };
> +
> + pcie@3400000 {
> + ...
> + msi-parent = <&msi>;
> + ...
> };
>
^ permalink raw reply
* Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq
From: Jerome Brunet @ 2016-10-25 13:08 UTC (permalink / raw)
To: Marc Zyngier, Linus Walleij
Cc: Carlo Caione, Kevin Hilman, open list:ARM/Amlogic Meson...,
linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
Thomas Gleixner, Jason Cooper, Rob Herring, Catalin Marinas,
Will Deacon, Russell King
In-Reply-To: <ec7c6cc5-cf9a-9c75-4b07-e337064c7d6e@arm.com>
On Tue, 2016-10-25 at 11:38 +0100, Marc Zyngier wrote:
> >
> On 25/10/16 10:14, Linus Walleij wrote:
> >
> > On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylibre.c
> > om> wrote:
> >
> > >
> > > >
> > > > Isn't this usecase (also as described in the cover letter) a
> > > > textbook
> > > > example of when you should be using hierarchical irqdomain?
> > > >
> > > > Please check with Marc et al on hierarchical irqdomains.
> > >
> > > Linus,
> > > Do you mean I should create a new hierarchical irqdomains in each
> > > of
> > > the two pinctrl instances we have in these SoC, these domains
> > > being
> > > stacked on the one I just added for controller in irqchip ?
> > >
> > > I did not understand this is what you meant when I asked you the
> > > question at ELCE.
> >
> > Honestly, I do not understand when and where to properly use
> > hierarchical irqdomain, even after Marc's talk at ELC-E.
>
> I probably didn't do that good a job explaining it then. Let's try
> again. You want to use hierarchical domains when you want to describe
> an
> interrupt whose path traverses multiple controllers without ever
> being
> multiplexed with other signals. As long as you have this 1:1
> relationship between controllers, you can use them.
>
Linus, Marc,
The calculation is question here is meant to get the appropriate hwirq
number from a particular gpio (and deal with the gpios that can't
provide an irq at all).
If I look at other gpio drivers, many are doing exactly this kind of
calculation before calling 'irq_create_mapping' in the to_irq callback.
For example:
- pinctrl/nomadik/pinctrl-abx500.c
- pinctrl/samsung/pinctrl-exynos5440.c
Some can afford to create all the mappings in the probe and just call
'irq_find_mapping' (gpio/gpio_tegra.c) but this would not work here. We
have only 8 upstream irqs for 130+ pins, so only 8 mappings possible at
a time.
My understanding is that irqdomain provide a way to map hwirq to linux
virq (and back), not map gpio number to hwirq, right?
Even if I implement an another irqdomain at the gpio level, I would
still have to perform this kind of calculation, one way or the other.
> > Which is problematic since quite a few GPIO drivers now
> > need to use them.
> >
> > I will review his slides, in the meantime I would say: whatever
> > Marc ACKs is fine with me. I trust this guy 100%. So I guess I
> > could ask that he ACK the entire chain of patches
> > from GIC->specialchip->GPIO.
Actually this discussion go me thinking about another issue we have
with this hardware.
We are looking for a way to implement support for IRQ_TYPE_EDGE_BOTH
(needed for things like gpio-keys or mmc card detect).
The controller can do each edge but not both at the same time.
I'm thinking that implementing another irqdomain at the gpio level
would allow to properly check the pad level in the EOI callback then
set the next expected edge type accordingly (using
'irq_chip_set_type_parent')
Would it be acceptable ?
It looks a few other drivers deal with IRQ_TYPE_EDGE_BOTH in a similar
way (gpio/gpio-omap.c, gpio/gpio-dwapb.c)
> Man, I don't even trust myself... ;-)
>
> Thanks,
>
> M.
Thx
Regards
Jerome
^ permalink raw reply
* Re: [PATCH] irqchip/ls-scfg-msi: update Layerscape SCFG MSI driver
From: Marc Zyngier @ 2016-10-25 13:11 UTC (permalink / raw)
To: Minghuan Lian, linux-arm-kernel, linux-kernel, devicetree
Cc: Rob Herring, Jason Cooper, Roy Zang, Mingkai Hu, Stuart Yoder,
Yang-Leo Li, Scott Wood
In-Reply-To: <1477399162-22881-1-git-send-email-Minghuan.Lian@nxp.com>
On 25/10/16 13:39, Minghuan Lian wrote:
> 1. The patch uses soc_device_match() to match the SoC family
> and revision instead of DTS compatible, because compatible cannot
> describe the SoC revision information.
> 2. The patch provides a new method to support Layerscape
> SCFG MSI. It tries to assign a dedicated MSIR to every core.
> When changing a MSI interrupt affinity, the MSI message
> data will be changed to refer to a new MSIR that has
> been associated with the core.
>
> Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
> ---
> The patch depends on https://patchwork.kernel.org/patch/9342915/
What is the status of this patch? This is the first time I hear about
it, and I can't find it in -rc2.
>
> drivers/irqchip/irq-ls-scfg-msi.c | 444 +++++++++++++++++++++++++++++++-------
> 1 file changed, 367 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/irqchip/irq-ls-scfg-msi.c b/drivers/irqchip/irq-ls-scfg-msi.c
> index 02cca74c..0245d8a 100644
> --- a/drivers/irqchip/irq-ls-scfg-msi.c
> +++ b/drivers/irqchip/irq-ls-scfg-msi.c
> @@ -10,6 +10,7 @@
> * published by the Free Software Foundation.
> */
>
> +#include <linux/bitmap.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/msi.h>
> @@ -17,23 +18,91 @@
> #include <linux/irq.h>
> #include <linux/irqchip/chained_irq.h>
> #include <linux/irqdomain.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> #include <linux/of_pci.h>
> #include <linux/of_platform.h>
> #include <linux/spinlock.h>
> +#include <linux/sys_soc.h>
>
> -#define MSI_MAX_IRQS 32
> -#define MSI_IBS_SHIFT 3
> -#define MSIR 4
> +#define LS_MSIR_NUM_MAX 4 /* MSIIR can index 4 MSI registers */
> +#define IRQS_32_PER_MSIR 32
> +#define IRQS_8_PER_MSIR 8
> +
> +#define MSIR_OFFSET(idx) ((idx) * 0x4)
> +
> +enum msi_affinity_flag {
> + MSI_GROUP_AFFINITY_FLAG,
> + MSI_AFFINITY_FLAG
> +};
> +
> +struct ls_scfg_msi;
> +struct ls_scfg_msi_ctrl;
> +
> +struct ls_scfg_msi_cfg {
> + u32 ibs_shift; /* Shift of interrupt bit select */
> + u32 msir_irqs; /* The irq number per MSIR */
> + u32 msir_base; /* The base address of MSIR */
> +};
> +
> +struct ls_scfg_msir {
> + struct ls_scfg_msi_ctrl *ctrl;
> + void __iomem *addr;
> + int index;
> + int virq;
> +};
> +
> +struct ls_scfg_msi_ctrl {
> + struct list_head list;
> + struct ls_scfg_msi *msi_data;
> + void __iomem *regs;
> + phys_addr_t msiir_addr;
> + enum msi_affinity_flag flag;
> + int irq_base;
> + spinlock_t lock;
> + struct ls_scfg_msir *msir;
> + unsigned long *bm;
> +};
>
> struct ls_scfg_msi {
> - spinlock_t lock;
> - struct platform_device *pdev;
> - struct irq_domain *parent;
> - struct irq_domain *msi_domain;
> - void __iomem *regs;
> - phys_addr_t msiir_addr;
> - int irq;
> - DECLARE_BITMAP(used, MSI_MAX_IRQS);
> + struct platform_device *pdev;
> + struct irq_domain *parent;
> + struct irq_domain *msi_domain;
> + struct list_head ctrl_list;
> + const struct ls_scfg_msi_cfg *cfg;
> + u32 cpu_num;
> +};
> +
> +static struct ls_scfg_msi_cfg ls1021_msi_cfg = {
> + .ibs_shift = 3,
> + .msir_irqs = IRQS_32_PER_MSIR,
> + .msir_base = 0x4,
> +};
> +
> +static struct ls_scfg_msi_cfg ls1043_rev11_msi_cfg = {
> + .ibs_shift = 2,
> + .msir_irqs = IRQS_8_PER_MSIR,
> + .msir_base = 0x10,
> +};
> +
> +static struct ls_scfg_msi_cfg ls1046_msi_cfg = {
> + .ibs_shift = 2,
> + .msir_irqs = IRQS_32_PER_MSIR,
> + .msir_base = 0x4,
> +};
> +
> +static struct soc_device_attribute soc_msi_matches[] = {
> + { .family = "QorIQ LS1021A",
> + .data = &ls1021_msi_cfg },
> + { .family = "QorIQ LS1012A",
> + .data = &ls1021_msi_cfg },
> + { .family = "QorIQ LS1043A", .revision = "1.0",
> + .data = &ls1021_msi_cfg },
> + { .family = "QorIQ LS1043A", .revision = "1.1",
> + .data = &ls1043_rev11_msi_cfg },
> + { .family = "QorIQ LS1046A",
> + .data = &ls1046_msi_cfg },
> + { },
Right. So after spending about 5 years getting rid of board files, they
are now back, just spread over a zillion drivers? Why isn't that
described in DT?
> };
>
> static struct irq_chip ls_scfg_msi_irq_chip = {
> @@ -49,19 +118,53 @@ struct ls_scfg_msi {
> .chip = &ls_scfg_msi_irq_chip,
> };
>
> +static int ctrl_num;
> +
> +static irqreturn_t (*ls_scfg_msi_irq_handler)(int irq, void *arg);
> +
> static void ls_scfg_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
> {
> - struct ls_scfg_msi *msi_data = irq_data_get_irq_chip_data(data);
> + struct ls_scfg_msi_ctrl *ctrl = irq_data_get_irq_chip_data(data);
> + u32 ibs, srs;
>
> - msg->address_hi = upper_32_bits(msi_data->msiir_addr);
> - msg->address_lo = lower_32_bits(msi_data->msiir_addr);
> - msg->data = data->hwirq << MSI_IBS_SHIFT;
> + msg->address_hi = upper_32_bits(ctrl->msiir_addr);
> + msg->address_lo = lower_32_bits(ctrl->msiir_addr);
> +
> + ibs = data->hwirq - ctrl->irq_base;
> +
> + srs = cpumask_first(irq_data_get_affinity_mask(data));
> + if (srs >= ctrl->msi_data->cpu_num)
> + srs = 0;
> +
> + msg->data = ibs << ctrl->msi_data->cfg->ibs_shift | srs;
> +
> + pr_debug("%s: ibs %d srs %d address0x%x-0x%x data 0x%x\n",
> + __func__, ibs, srs, msg->address_hi,
> + msg->address_lo, msg->data);
> }
>
> -static int ls_scfg_msi_set_affinity(struct irq_data *irq_data,
> - const struct cpumask *mask, bool force)
> +static int ls_scfg_msi_set_affinity(struct irq_data *data,
> + const struct cpumask *mask, bool force)
> {
> - return -EINVAL;
> + struct ls_scfg_msi_ctrl *ctrl = irq_data_get_irq_chip_data(data);
> + u32 cpu;
> +
> + if (!force)
> + cpu = cpumask_any_and(mask, cpu_online_mask);
> + else
> + cpu = cpumask_first(mask);
> +
> + if (cpu >= ctrl->msi_data->cpu_num)
> + return -EINVAL;
> +
> + if (ctrl->msir[cpu].virq <= 0) {
> + pr_warn("cannot bind the irq to cpu%d\n", cpu);
> + return -EINVAL;
> + }
> +
> + cpumask_copy(irq_data_get_affinity_mask(data), mask);
> +
> + return IRQ_SET_MASK_OK_NOCOPY;
> }
>
> static struct irq_chip ls_scfg_msi_parent_chip = {
> @@ -76,44 +179,57 @@ static int ls_scfg_msi_domain_irq_alloc(struct irq_domain *domain,
> void *args)
> {
> struct ls_scfg_msi *msi_data = domain->host_data;
> - int pos, err = 0;
> + static struct list_head *current_entry;
> + struct ls_scfg_msi_ctrl *ctrl;
> + int i, hwirq = -ENOMEM;
> +
> + if (!current_entry || current_entry->next == &msi_data->ctrl_list)
> + current_entry = &msi_data->ctrl_list;
> +
> + list_for_each_entry(ctrl, current_entry, list) {
> + spin_lock(&ctrl->lock);
> + hwirq = bitmap_find_free_region(ctrl->bm,
> + msi_data->cfg->msir_irqs,
> + order_base_2(nr_irqs));
> + spin_unlock(&ctrl->lock);
> +
> + if (hwirq >= 0)
> + break;
> + }
>
> - WARN_ON(nr_irqs != 1);
> + if (hwirq < 0)
> + return hwirq;
>
> - spin_lock(&msi_data->lock);
> - pos = find_first_zero_bit(msi_data->used, MSI_MAX_IRQS);
> - if (pos < MSI_MAX_IRQS)
> - __set_bit(pos, msi_data->used);
> - else
> - err = -ENOSPC;
> - spin_unlock(&msi_data->lock);
> + hwirq = hwirq + ctrl->irq_base;
>
> - if (err)
> - return err;
> + for (i = 0; i < nr_irqs; i++)
> + irq_domain_set_info(domain, virq + i, hwirq + i,
> + &ls_scfg_msi_parent_chip, ctrl,
> + handle_simple_irq, NULL, NULL);
>
> - irq_domain_set_info(domain, virq, pos,
> - &ls_scfg_msi_parent_chip, msi_data,
> - handle_simple_irq, NULL, NULL);
> + current_entry = &ctrl->list;
>
> return 0;
> }
>
> static void ls_scfg_msi_domain_irq_free(struct irq_domain *domain,
> - unsigned int virq, unsigned int nr_irqs)
> + unsigned int virq,
> + unsigned int nr_irqs)
> {
> struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> - struct ls_scfg_msi *msi_data = irq_data_get_irq_chip_data(d);
> + struct ls_scfg_msi_ctrl *ctrl = irq_data_get_irq_chip_data(d);
> int pos;
>
> - pos = d->hwirq;
> - if (pos < 0 || pos >= MSI_MAX_IRQS) {
> - pr_err("failed to teardown msi. Invalid hwirq %d\n", pos);
> + pos = d->hwirq - ctrl->irq_base;
> +
> + if (pos < 0 || pos >= ctrl->msi_data->cfg->msir_irqs) {
> + pr_err("Failed to teardown msi. Invalid hwirq %d\n", pos);
> return;
> }
>
> - spin_lock(&msi_data->lock);
> - __clear_bit(pos, msi_data->used);
> - spin_unlock(&msi_data->lock);
> + spin_lock(&ctrl->lock);
> + bitmap_release_region(ctrl->bm, pos, order_base_2(nr_irqs));
> + spin_unlock(&ctrl->lock);
> }
>
> static const struct irq_domain_ops ls_scfg_msi_domain_ops = {
> @@ -121,29 +237,198 @@ static void ls_scfg_msi_domain_irq_free(struct irq_domain *domain,
> .free = ls_scfg_msi_domain_irq_free,
> };
>
> -static void ls_scfg_msi_irq_handler(struct irq_desc *desc)
> +static irqreturn_t ls_scfg_msi_irqs32_handler(int irq, void *arg)
> {
> - struct ls_scfg_msi *msi_data = irq_desc_get_handler_data(desc);
> + struct ls_scfg_msir *msir = arg;
> + struct ls_scfg_msi_ctrl *ctrl = msir->ctrl;
> + struct ls_scfg_msi *msi_data = ctrl->msi_data;
> unsigned long val;
> - int pos, virq;
> + int pos = 0, hwirq, virq;
> + irqreturn_t ret = IRQ_NONE;
>
> - chained_irq_enter(irq_desc_get_chip(desc), desc);
> + val = ioread32be(msir->addr);
>
> - val = ioread32be(msi_data->regs + MSIR);
> - for_each_set_bit(pos, &val, MSI_MAX_IRQS) {
> - virq = irq_find_mapping(msi_data->parent, (31 - pos));
> - if (virq)
> + for_each_set_bit(pos, &val, IRQS_32_PER_MSIR) {
> + hwirq = (IRQS_32_PER_MSIR - 1 - pos) + ctrl->irq_base;
> + virq = irq_find_mapping(msi_data->parent, hwirq);
> + if (virq) {
> generic_handle_irq(virq);
> + ret = IRQ_HANDLED;
> + }
> + }
> +
> + return ret;
> +}
NAK. This is not an interrupt handler. This is a flow handler.
> +
> +static irqreturn_t ls_scfg_msi_irqs8_handler(int irq, void *arg)
> +{
> + struct ls_scfg_msir *msir = arg;
> + struct ls_scfg_msi_ctrl *ctrl = msir->ctrl;
> + struct ls_scfg_msi *msi_data = ctrl->msi_data;
> + unsigned long val;
> + int pos = 0, hwirq, virq;
> + irqreturn_t ret = IRQ_NONE;
> +
> + val = ioread32be(msir->addr);
> + val = (val << (msir->index * 8)) & 0xff000000;
> +
> + for_each_set_bit(pos, &val, IRQS_32_PER_MSIR) {
> + hwirq = (IRQS_32_PER_MSIR - 1 - pos) + ctrl->irq_base;
> + virq = irq_find_mapping(msi_data->parent, hwirq);
> + if (virq) {
> + generic_handle_irq(virq);
> + ret = IRQ_HANDLED;
> + }
> + }
> +
> + return ret;
> +}
> +
> +static void ls_scfg_msi_cascade(struct irq_desc *desc)
> +{
> + struct ls_scfg_msir *msir = irq_desc_get_handler_data(desc);
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> +
> + chained_irq_enter(chip, desc);
> + ls_scfg_msi_irq_handler(desc->irq_data.irq, msir);
> + chained_irq_exit(chip, desc);
> +}
NAK.
> +
> +static int ls_scfg_msi_setup_hwirq(struct ls_scfg_msi_ctrl *ctrl,
> + struct device_node *node,
> + int index)
> +{
> + struct ls_scfg_msir *msir = &ctrl->msir[index];
> + int ret;
> +
> + msir->virq = of_irq_get(node, index);
> + if (msir->virq <= 0)
> + return -ENODEV;
> +
> + msir->index = index;
> + msir->ctrl = ctrl;
> + msir->addr = ctrl->regs + ctrl->msi_data->cfg->msir_base +
> + MSIR_OFFSET(msir->index);
> +
> + if (ctrl->flag == MSI_GROUP_AFFINITY_FLAG) {
> + ret = request_irq(msir->virq, ls_scfg_msi_irq_handler,
> + IRQF_NO_THREAD, "MSI-GROUP", msir);
> + if (ret) {
> + pr_err("failed to request irq %d\n", msir->virq);
> + msir->virq = 0;
> + return -ENODEV;
> + }
> + } else {
> + irq_set_chained_handler(msir->virq, ls_scfg_msi_cascade);
> + irq_set_handler_data(msir->virq, msir);
> + irq_set_affinity(msir->virq, get_cpu_mask(index));
> + }
> +
> + return 0;
> +}
> +
> +static void ls_scfg_msi_ctrl_remove(struct ls_scfg_msi_ctrl *ctrl)
> +{
> + struct ls_scfg_msir *msir;
> + int i;
> +
> + if (!ctrl)
> + return;
> +
> + if (ctrl->msir) {
> + for (i = 0; i < ctrl->msi_data->cpu_num; i++) {
> + msir = &ctrl->msir[i];
> +
> + if (msir->virq <= 0)
> + continue;
> +
> + if (ctrl->flag == MSI_GROUP_AFFINITY_FLAG)
> + free_irq(msir->virq, msir);
> + else
> + irq_set_chained_handler_and_data(msir->virq,
> + NULL, NULL);
> + }
> +
> + kfree(ctrl->msir);
> }
>
> - chained_irq_exit(irq_desc_get_chip(desc), desc);
> + if (ctrl->regs)
> + iounmap(ctrl->regs);
> +
> + kfree(ctrl->bm);
> + kfree(ctrl);
> +}
> +
> +static int ls_scfg_msi_ctrl_probe(struct device_node *node,
> + struct ls_scfg_msi *msi_data)
> +{
> + struct ls_scfg_msi_ctrl *ctrl;
> + struct resource res;
> + int err, irqs, i;
> +
> + err = of_address_to_resource(node, 0, &res);
> + if (err) {
> + pr_warn("%s: no regs\n", node->full_name);
> + return -ENXIO;
> + }
> +
> + ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
> + if (!ctrl)
> + return -ENOMEM;
> +
> + ctrl->msi_data = msi_data;
> + ctrl->msiir_addr = res.start;
> + spin_lock_init(&ctrl->lock);
> +
> + ctrl->regs = ioremap(res.start, resource_size(&res));
> + if (!ctrl->regs) {
> + pr_err("%s: unable to map registers\n", node->full_name);
> + err = -ENOMEM;
> + goto _err;
> + }
> +
> + ctrl->msir = kcalloc(msi_data->cpu_num, sizeof(struct ls_scfg_msir),
> + GFP_KERNEL);
> + if (!ctrl->msir) {
> + err = -ENOMEM;
> + goto _err;
> + }
> +
> + ctrl->bm = kcalloc(BITS_TO_LONGS(msi_data->cfg->msir_irqs),
> + sizeof(long), GFP_KERNEL);
> + if (!ctrl->bm) {
> + err = -ENOMEM;
> + goto _err;
> + }
> +
> + ctrl->irq_base = msi_data->cfg->msir_irqs * ctrl_num;
> + ctrl_num++;
> +
> + irqs = of_irq_count(node);
> + if (irqs >= msi_data->cpu_num)
> + ctrl->flag = MSI_AFFINITY_FLAG;
> + else
> + ctrl->flag = MSI_GROUP_AFFINITY_FLAG;
> +
> + for (i = 0; i < msi_data->cpu_num; i++)
> + ls_scfg_msi_setup_hwirq(ctrl, node, i);
> +
> + list_add_tail(&ctrl->list, &msi_data->ctrl_list);
> +
> + return 0;
> +
> +_err:
> + ls_scfg_msi_ctrl_remove(ctrl);
> + pr_err("MSI: failed probing %s (%d)\n", node->full_name, err);
> + return err;
> }
>
> static int ls_scfg_msi_domains_init(struct ls_scfg_msi *msi_data)
> {
> /* Initialize MSI domain parent */
> msi_data->parent = irq_domain_add_linear(NULL,
> - MSI_MAX_IRQS,
> + msi_data->cfg->msir_irqs *
> + ctrl_num,
> &ls_scfg_msi_domain_ops,
> msi_data);
> if (!msi_data->parent) {
> @@ -167,51 +452,57 @@ static int ls_scfg_msi_domains_init(struct ls_scfg_msi *msi_data)
> static int ls_scfg_msi_probe(struct platform_device *pdev)
> {
> struct ls_scfg_msi *msi_data;
> - struct resource *res;
> - int ret;
> + const struct soc_device_attribute *match;
> + struct device_node *child;
>
> msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
> if (!msi_data)
> return -ENOMEM;
>
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - msi_data->regs = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(msi_data->regs)) {
> - dev_err(&pdev->dev, "failed to initialize 'regs'\n");
> - return PTR_ERR(msi_data->regs);
> - }
> - msi_data->msiir_addr = res->start;
> -
> - msi_data->irq = platform_get_irq(pdev, 0);
> - if (msi_data->irq <= 0) {
> - dev_err(&pdev->dev, "failed to get MSI irq\n");
> - return -ENODEV;
> - }
> + INIT_LIST_HEAD(&msi_data->ctrl_list);
>
> msi_data->pdev = pdev;
> - spin_lock_init(&msi_data->lock);
> + msi_data->cpu_num = num_possible_cpus();
> +
> + match = soc_device_match(soc_msi_matches);
> + if (match)
> + msi_data->cfg = match->data;
> + else
> + msi_data->cfg = &ls1046_msi_cfg;
> +
> + if (msi_data->cfg->msir_irqs == IRQS_8_PER_MSIR)
> + ls_scfg_msi_irq_handler = ls_scfg_msi_irqs8_handler;
> + else
> + ls_scfg_msi_irq_handler = ls_scfg_msi_irqs32_handler;
>
> - ret = ls_scfg_msi_domains_init(msi_data);
> - if (ret)
> - return ret;
> + for_each_child_of_node(msi_data->pdev->dev.of_node, child)
> + ls_scfg_msi_ctrl_probe(child, msi_data);
>
> - irq_set_chained_handler_and_data(msi_data->irq,
> - ls_scfg_msi_irq_handler,
> - msi_data);
> + ls_scfg_msi_domains_init(msi_data);
>
> platform_set_drvdata(pdev, msi_data);
>
> + dev_info(&pdev->dev, "irqs:%dx%d ibs_shift:%d msir_base:0x%x\n",
> + msi_data->cfg->msir_irqs, ctrl_num,
> + msi_data->cfg->ibs_shift, msi_data->cfg->msir_base);
> +
> return 0;
> }
>
> static int ls_scfg_msi_remove(struct platform_device *pdev)
> {
> struct ls_scfg_msi *msi_data = platform_get_drvdata(pdev);
> + struct ls_scfg_msi_ctrl *ctrl, *temp;
>
> - irq_set_chained_handler_and_data(msi_data->irq, NULL, NULL);
> + list_for_each_entry_safe(ctrl, temp, &msi_data->ctrl_list, list) {
> + list_move_tail(&ctrl->list, &msi_data->ctrl_list);
> + ls_scfg_msi_ctrl_remove(ctrl);
> + }
>
> - irq_domain_remove(msi_data->msi_domain);
> - irq_domain_remove(msi_data->parent);
> + if (msi_data->msi_domain)
> + irq_domain_remove(msi_data->msi_domain);
> + if (msi_data->parent)
> + irq_domain_remove(msi_data->parent);
>
> platform_set_drvdata(pdev, NULL);
>
> @@ -219,8 +510,7 @@ static int ls_scfg_msi_remove(struct platform_device *pdev)
> }
>
> static const struct of_device_id ls_scfg_msi_id[] = {
> - { .compatible = "fsl,1s1021a-msi", },
> - { .compatible = "fsl,1s1043a-msi", },
> + { .compatible = "fsl,ls-scfg-msi" },
And now you've broken all the previous DTs that previously existed.
What's the plan?
> {},
> };
>
>
Overall, I hate this patch. It is broken from an irqchip PoV, it creates
a new range of board files, just even less maintainable, it breaks
compatibility with previous DTs.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH v7 0/2] tango NFC support + bindings doc
From: Marc Gonzalez @ 2016-10-25 13:14 UTC (permalink / raw)
To: linux-mtd, Boris Brezillon, Richard Weinberger
Cc: Mark Rutland, DT, Rob Herring, Sebastian Frias, Mason
Hello,
This patch series adds support for the NAND Flash controller
on recent Tango chips, along with the bindings documentation.
Marc Gonzalez (2):
mtd: nand: add tango NFC dt bindings doc
mtd: nand: add tango NAND flash controller support
Documentation/devicetree/bindings/mtd/tango-nand.txt | 38 ++
drivers/mtd/nand/Kconfig | 6 +
drivers/mtd/nand/Makefile | 1 +
drivers/mtd/nand/tango_nand.c | 654 +++++++++++++++++++++++++++++
4 files changed, 699 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mtd/tango-nand.txt
create mode 100644 drivers/mtd/nand/tango_nand.c
Regards.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply
* [PATCH v7 1/2] mtd: nand: add tango NFC dt bindings doc
From: Marc Gonzalez @ 2016-10-25 13:15 UTC (permalink / raw)
To: linux-mtd, Boris Brezillon, Richard Weinberger
Cc: Mark Rutland, DT, Rob Herring, Sebastian Frias, Mason
In-Reply-To: <580F5AD2.9020909@sigmadesigns.com>
Add the tango NAND Flash Controller dt bindings documentation.
Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
Documentation/devicetree/bindings/mtd/tango-nand.txt | 38 ++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/Documentation/devicetree/bindings/mtd/tango-nand.txt b/Documentation/devicetree/bindings/mtd/tango-nand.txt
new file mode 100644
index 000000000000..3cbf95d6595a
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/tango-nand.txt
@@ -0,0 +1,38 @@
+Sigma Designs Tango4 NAND Flash Controller (NFC)
+
+Required properties:
+
+- compatible: "sigma,smp8758-nand"
+- reg: address/size of nfc_reg, nfc_mem, and pbus_reg
+- dmas: reference to the DMA channel used by the controller
+- dma-names: "nfc_sbox"
+- clocks: reference to the system clock
+- #address-cells: <1>
+- #size-cells: <0>
+
+Children nodes represent the available NAND chips.
+See Documentation/devicetree/bindings/mtd/nand.txt for generic bindings.
+
+Example:
+
+ nand: nand@2c000 {
+ compatible = "sigma,smp8758-nand";
+ reg = <0x2c000 0x30 0x2d000 0x800 0x20000 0x1000>;
+ dmas = <&dma0 3>;
+ dma-names = "nfc_sbox";
+ clocks = <&clkgen SYS_CLK>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ nand@0 {
+ reg = <0>; /* CS0 */
+ nand-ecc-strength = <14>;
+ nand-ecc-step-size = <1024>;
+ };
+
+ nand@1 {
+ reg = <1>; /* CS1 */
+ nand-ecc-strength = <14>;
+ nand-ecc-step-size = <1024>;
+ };
+ };
--
2.9.0
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related
* [PATCH v7 2/2] mtd: nand: add tango NAND flash controller support
From: Marc Gonzalez @ 2016-10-25 13:16 UTC (permalink / raw)
To: linux-mtd, Boris Brezillon, Richard Weinberger
Cc: Mark Rutland, DT, Rob Herring, Sebastian Frias, Mason
In-Reply-To: <580F5AD2.9020909@sigmadesigns.com>
This driver supports the NAND Flash controller embedded in recent
Tango chips, such as SMP8758 and SMP8759.
Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
drivers/mtd/nand/Kconfig | 6 +
drivers/mtd/nand/Makefile | 1 +
drivers/mtd/nand/tango_nand.c | 654 ++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 661 insertions(+)
diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 7b7a887b4709..844ab20a23a4 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -205,6 +205,12 @@ config MTD_NAND_S3C2410_CLKSTOP
when the is NAND chip selected or released, but will save
approximately 5mA of power when there is nothing happening.
+config MTD_NAND_TANGO
+ tristate "NAND Flash support for Tango chips"
+ depends on ARCH_TANGO
+ help
+ Enables the NAND Flash controller on Tango chips.
+
config MTD_NAND_DISKONCHIP
tristate "DiskOnChip 2000, Millennium and Millennium Plus (NAND reimplementation)"
depends on HAS_IOMEM
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index cafde6f3d957..4904ad3614fb 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_MTD_NAND_DENALI_DT) += denali_dt.o
obj-$(CONFIG_MTD_NAND_AU1550) += au1550nd.o
obj-$(CONFIG_MTD_NAND_BF5XX) += bf5xx_nand.o
obj-$(CONFIG_MTD_NAND_S3C2410) += s3c2410.o
+obj-$(CONFIG_MTD_NAND_TANGO) += tango_nand.o
obj-$(CONFIG_MTD_NAND_DAVINCI) += davinci_nand.o
obj-$(CONFIG_MTD_NAND_DISKONCHIP) += diskonchip.o
obj-$(CONFIG_MTD_NAND_DOCG4) += docg4.o
diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c
new file mode 100644
index 000000000000..74e39a92771c
--- /dev/null
+++ b/drivers/mtd/nand/tango_nand.c
@@ -0,0 +1,654 @@
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/clk.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/mtd/nand.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
+#include <linux/platform_device.h>
+
+/* Offsets relative to chip->base */
+#define PBUS_CMD 0
+#define PBUS_ADDR 4
+#define PBUS_DATA 8
+
+/* Offsets relative to reg_base */
+#define NFC_STATUS 0x00
+#define NFC_FLASH_CMD 0x04
+#define NFC_DEVICE_CFG 0x08
+#define NFC_TIMING1 0x0c
+#define NFC_TIMING2 0x10
+#define NFC_XFER_CFG 0x14
+#define NFC_PKT_0_CFG 0x18
+#define NFC_PKT_N_CFG 0x1c
+#define NFC_BB_CFG 0x20
+#define NFC_ADDR_PAGE 0x24
+#define NFC_ADDR_OFFSET 0x28
+#define NFC_XFER_STATUS 0x2c
+
+/* NFC_STATUS values */
+#define CMD_READY BIT(31)
+
+/* NFC_FLASH_CMD values */
+#define NFC_READ 1
+#define NFC_WRITE 2
+
+/* NFC_XFER_STATUS values */
+#define PAGE_IS_EMPTY BIT(16)
+
+/* Offsets relative to mem_base */
+#define METADATA 0x000
+#define ERROR_REPORT 0x1c0
+
+/*
+ * Error reports are split in two bytes:
+ * byte 0 for the first packet in the page (PKT_0)
+ * byte 1 for other packets in the page (PKT_N, for N > 0)
+ * ERR_COUNT_PKT_N is the max error count over all but the first packet.
+ */
+#define DECODE_OK_PKT_0(v) (v & BIT(7))
+#define DECODE_OK_PKT_N(v) (v & BIT(15))
+#define ERR_COUNT_PKT_0(v) ((v >> 0) & 0x3f)
+#define ERR_COUNT_PKT_N(v) ((v >> 8) & 0x3f)
+
+/* Offsets relative to pbus_base */
+#define PBUS_CS_CTRL 0x83c
+#define PBUS_PAD_MODE 0x8f0
+
+/* PBUS_CS_CTRL values */
+#define PBUS_IORDY BIT(31)
+
+/*
+ * PBUS_PAD_MODE values
+ * In raw mode, the driver communicates directly with the NAND chips.
+ * In NFC mode, the NAND Flash controller manages the communication.
+ * We use NFC mode for read and write; raw mode for everything else.
+ */
+#define MODE_RAW 0
+#define MODE_NFC BIT(31)
+
+#define METADATA_SIZE 4
+#define BBM_SIZE 6
+#define FIELD_ORDER 15
+
+#define MAX_CS 4
+
+struct tango_nfc {
+ struct nand_hw_control hw;
+ void __iomem *reg_base;
+ void __iomem *mem_base;
+ void __iomem *pbus_base;
+ struct tango_chip *chips[MAX_CS];
+ struct dma_chan *chan;
+ int freq_kHz;
+};
+
+#define to_tango_nfc(ptr) container_of(ptr, struct tango_nfc, hw)
+
+struct tango_chip {
+ struct nand_chip nand_chip;
+ void __iomem *base;
+ u32 timing1;
+ u32 timing2;
+ u32 xfer_cfg;
+ u32 pkt_0_cfg;
+ u32 pkt_n_cfg;
+ u32 bb_cfg;
+};
+
+#define to_tango_chip(ptr) container_of(ptr, struct tango_chip, nand_chip)
+
+#define XFER_CFG(cs, page_count, steps, metadata_size) \
+ ((cs) << 24 | (page_count) << 16 | (steps) << 8 | (metadata_size))
+
+#define PKT_CFG(size, strength) ((size) << 16 | (strength))
+
+#define BB_CFG(bb_offset, bb_size) ((bb_offset) << 16 | (bb_size))
+
+#define TIMING(t0, t1, t2, t3) ((t0) << 24 | (t1) << 16 | (t2) << 8 | (t3))
+
+static void tango_cmd_ctrl(struct mtd_info *mtd, int dat, unsigned int ctrl)
+{
+ struct tango_chip *tchip = to_tango_chip(mtd_to_nand(mtd));
+
+ if (ctrl & NAND_CLE)
+ writeb_relaxed(dat, tchip->base + PBUS_CMD);
+
+ if (ctrl & NAND_ALE)
+ writeb_relaxed(dat, tchip->base + PBUS_ADDR);
+}
+
+static int tango_dev_ready(struct mtd_info *mtd)
+{
+ struct nand_chip *chip = mtd_to_nand(mtd);
+ struct tango_nfc *nfc = to_tango_nfc(chip->controller);
+
+ return readl_relaxed(nfc->pbus_base + PBUS_CS_CTRL) & PBUS_IORDY;
+}
+
+static uint8_t tango_read_byte(struct mtd_info *mtd)
+{
+ struct tango_chip *tchip = to_tango_chip(mtd_to_nand(mtd));
+
+ return readb_relaxed(tchip->base + PBUS_DATA);
+}
+
+static void tango_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
+{
+ struct tango_chip *tchip = to_tango_chip(mtd_to_nand(mtd));
+
+ ioread8_rep(tchip->base + PBUS_DATA, buf, len);
+}
+
+static void tango_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
+{
+ struct tango_chip *tchip = to_tango_chip(mtd_to_nand(mtd));
+
+ iowrite8_rep(tchip->base + PBUS_DATA, buf, len);
+}
+
+static void tango_select_chip(struct mtd_info *mtd, int idx)
+{
+ struct nand_chip *chip = mtd_to_nand(mtd);
+ struct tango_nfc *nfc = to_tango_nfc(chip->controller);
+ struct tango_chip *tchip = to_tango_chip(chip);
+
+ if (idx < 0)
+ return; /* No "chip unselect" function */
+
+ writel_relaxed(tchip->timing1, nfc->reg_base + NFC_TIMING1);
+ writel_relaxed(tchip->timing2, nfc->reg_base + NFC_TIMING2);
+ writel_relaxed(tchip->xfer_cfg, nfc->reg_base + NFC_XFER_CFG);
+ writel_relaxed(tchip->pkt_0_cfg, nfc->reg_base + NFC_PKT_0_CFG);
+ writel_relaxed(tchip->pkt_n_cfg, nfc->reg_base + NFC_PKT_N_CFG);
+ writel_relaxed(tchip->bb_cfg, nfc->reg_base + NFC_BB_CFG);
+}
+
+/*
+ * The controller does not check for bitflips in erased pages,
+ * therefore software must check instead.
+ */
+static int check_erased_page(struct nand_chip *chip, u8 *buf)
+{
+ u8 *meta = chip->oob_poi + BBM_SIZE;
+ u8 *ecc = chip->oob_poi + BBM_SIZE + METADATA_SIZE;
+ const int ecc_size = chip->ecc.bytes;
+ const int pkt_size = chip->ecc.size;
+ int i, res, meta_len, bitflips = 0;
+
+ for (i = 0; i < chip->ecc.steps; ++i) {
+ meta_len = i ? 0 : METADATA_SIZE;
+ res = nand_check_erased_ecc_chunk(buf, pkt_size, ecc, ecc_size,
+ meta, meta_len, chip->ecc.strength);
+ if (res < 0)
+ chip->mtd.ecc_stats.failed++;
+
+ bitflips = max(res, bitflips);
+ buf += pkt_size;
+ ecc += ecc_size;
+ }
+
+ return bitflips;
+}
+
+static int decode_error_report(struct tango_nfc *nfc)
+{
+ u32 status, res;
+
+ status = readl_relaxed(nfc->reg_base + NFC_XFER_STATUS);
+ if (status & PAGE_IS_EMPTY)
+ return 0;
+
+ res = readl_relaxed(nfc->mem_base + ERROR_REPORT);
+
+ if (DECODE_OK_PKT_0(res) && DECODE_OK_PKT_N(res))
+ return max(ERR_COUNT_PKT_0(res), ERR_COUNT_PKT_N(res));
+
+ return -EBADMSG;
+}
+
+static void tango_dma_callback(void *arg)
+{
+ complete(arg);
+}
+
+static int do_dma(struct tango_nfc *nfc, int dir, int cmd,
+ const void *buf, int len, int page)
+{
+ void __iomem *addr = nfc->reg_base + NFC_STATUS;
+ struct dma_chan *chan = nfc->chan;
+ struct dma_async_tx_descriptor *desc;
+ struct scatterlist sg;
+ struct completion tx_done;
+ int err = -EIO;
+ u32 res, val;
+
+ sg_init_one(&sg, buf, len);
+ if (dma_map_sg(chan->device->dev, &sg, 1, dir) != 1)
+ return -EIO;
+
+ desc = dmaengine_prep_slave_sg(chan, &sg, 1, dir, DMA_PREP_INTERRUPT);
+ if (!desc)
+ goto dma_unmap;
+
+ desc->callback = tango_dma_callback;
+ desc->callback_param = &tx_done;
+ init_completion(&tx_done);
+
+ writel_relaxed(MODE_NFC, nfc->pbus_base + PBUS_PAD_MODE);
+
+ writel_relaxed(page, nfc->reg_base + NFC_ADDR_PAGE);
+ writel_relaxed(0, nfc->reg_base + NFC_ADDR_OFFSET);
+ writel_relaxed(cmd, nfc->reg_base + NFC_FLASH_CMD);
+
+ dmaengine_submit(desc);
+ dma_async_issue_pending(chan);
+
+ res = wait_for_completion_timeout(&tx_done, HZ);
+ if (res > 0)
+ err = readl_poll_timeout(addr, val, val & CMD_READY, 0, 1000);
+
+ writel_relaxed(MODE_RAW, nfc->pbus_base + PBUS_PAD_MODE);
+
+dma_unmap:
+ dma_unmap_sg(chan->device->dev, &sg, 1, dir);
+
+ return err;
+}
+
+static int tango_read_page(struct mtd_info *mtd, struct nand_chip *chip,
+ uint8_t *buf, int oob_required, int page)
+{
+ struct tango_nfc *nfc = to_tango_nfc(chip->controller);
+ int err, res, len = mtd->writesize;
+
+ if (oob_required)
+ chip->ecc.read_oob(mtd, chip, page);
+
+ err = do_dma(nfc, DMA_FROM_DEVICE, NFC_READ, buf, len, page);
+ if (err)
+ return err;
+
+ res = decode_error_report(nfc);
+ if (res < 0) {
+ chip->ecc.read_oob_raw(mtd, chip, page);
+ res = check_erased_page(chip, buf);
+ }
+
+ return res;
+}
+
+static int tango_write_page(struct mtd_info *mtd, struct nand_chip *chip,
+ const uint8_t *buf, int oob_required, int page)
+{
+ struct tango_nfc *nfc = to_tango_nfc(chip->controller);
+ int err, len = mtd->writesize;
+
+ /* Calling tango_write_oob() would send PAGEPROG twice */
+ if (oob_required)
+ return -ENOTSUPP;
+
+ writel_relaxed(0xffffffff, nfc->mem_base + METADATA);
+ err = do_dma(nfc, DMA_TO_DEVICE, NFC_WRITE, buf, len, page);
+ if (err)
+ return err;
+
+ return 0;
+}
+
+static void aux_read(struct nand_chip *chip, u8 **buf, int len, int *pos)
+{
+ *pos += len;
+
+ if (*buf == NULL) {
+ /* skip over "len" bytes */
+ chip->cmdfunc(&chip->mtd, NAND_CMD_RNDOUT, *pos, -1);
+ } else {
+ tango_read_buf(&chip->mtd, *buf, len);
+ *buf += len;
+ }
+}
+
+static void aux_write(struct nand_chip *chip, const u8 **buf, int len, int *pos)
+{
+ *pos += len;
+
+ if (*buf == NULL) {
+ /* skip over "len" bytes */
+ chip->cmdfunc(&chip->mtd, NAND_CMD_SEQIN, *pos, -1);
+ } else {
+ tango_write_buf(&chip->mtd, *buf, len);
+ *buf += len;
+ }
+}
+
+/*
+ * Physical page layout (not drawn to scale)
+ *
+ * NB: Bad Block Marker area splits PKT_N in two (N1, N2).
+ *
+ * +---+-----------------+-------+-----+-----------+-----+----+-------+
+ * | M | PKT_0 | ECC_0 | ... | N1 | BBM | N2 | ECC_N |
+ * +---+-----------------+-------+-----+-----------+-----+----+-------+
+ *
+ * Logical page layout:
+ *
+ * +-----+---+-------+-----+-------+
+ * oob = | BBM | M | ECC_0 | ... | ECC_N |
+ * +-----+---+-------+-----+-------+
+ *
+ * +-----------------+-----+-----------------+
+ * buf = | PKT_0 | ... | PKT_N |
+ * +-----------------+-----+-----------------+
+ */
+static int raw_read(struct nand_chip *chip, u8 *buf, u8 *oob)
+{
+ u8 *oob_orig = oob;
+ const int page_size = chip->mtd.writesize;
+ const int ecc_size = chip->ecc.bytes;
+ const int pkt_size = chip->ecc.size;
+ int pos = 0; /* position within physical page */
+ int rem = page_size; /* bytes remaining until BBM area */
+
+ if (oob != NULL)
+ oob += BBM_SIZE;
+
+ aux_read(chip, &oob, METADATA_SIZE, &pos);
+
+ while (rem > pkt_size) {
+ aux_read(chip, &buf, pkt_size, &pos);
+ aux_read(chip, &oob, ecc_size, &pos);
+ rem = page_size - pos;
+ }
+
+ aux_read(chip, &buf, rem, &pos);
+ aux_read(chip, &oob_orig, BBM_SIZE, &pos);
+ aux_read(chip, &buf, pkt_size - rem, &pos);
+ aux_read(chip, &oob, ecc_size, &pos);
+
+ return 0;
+}
+
+static int raw_write(struct nand_chip *chip, const u8 *buf, const u8 *oob)
+{
+ const u8 *oob_orig = oob;
+ const int page_size = chip->mtd.writesize;
+ const int ecc_size = chip->ecc.bytes;
+ const int pkt_size = chip->ecc.size;
+ int pos = 0; /* position within physical page */
+ int rem = page_size; /* bytes remaining until BBM area */
+
+ if (oob != NULL)
+ oob += BBM_SIZE;
+
+ aux_write(chip, &oob, METADATA_SIZE, &pos);
+
+ while (rem > pkt_size) {
+ aux_write(chip, &buf, pkt_size, &pos);
+ aux_write(chip, &oob, ecc_size, &pos);
+ rem = page_size - pos;
+ }
+
+ aux_write(chip, &buf, rem, &pos);
+ aux_write(chip, &oob_orig, BBM_SIZE, &pos);
+ aux_write(chip, &buf, pkt_size - rem, &pos);
+ aux_write(chip, &oob, ecc_size, &pos);
+
+ return 0;
+}
+
+static int tango_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
+ uint8_t *buf, int oob_required, int page)
+{
+ return raw_read(chip, buf, chip->oob_poi);
+}
+
+static int tango_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
+ const uint8_t *buf, int oob_required, int page)
+{
+ return raw_write(chip, buf, chip->oob_poi);
+}
+
+static int tango_read_oob(struct mtd_info *mtd, struct nand_chip *chip, int page)
+{
+ chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
+ return raw_read(chip, NULL, chip->oob_poi);
+}
+
+static int tango_write_oob(struct mtd_info *mtd, struct nand_chip *chip, int page)
+{
+ chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0, page);
+ raw_write(chip, NULL, chip->oob_poi);
+ chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
+ chip->waitfunc(mtd, chip);
+ return 0;
+}
+
+static int oob_ecc(struct mtd_info *mtd, int idx, struct mtd_oob_region *res)
+{
+ struct nand_chip *chip = mtd_to_nand(mtd);
+ struct nand_ecc_ctrl *ecc = &chip->ecc;
+
+ if (idx >= ecc->steps)
+ return -ERANGE;
+
+ res->offset = BBM_SIZE + METADATA_SIZE + ecc->bytes * idx;
+ res->length = ecc->bytes;
+
+ return 0;
+}
+
+static int oob_free(struct mtd_info *mtd, int idx, struct mtd_oob_region *res)
+{
+ return -ERANGE; /* no free space in spare area */
+}
+
+static const struct mtd_ooblayout_ops tango_nand_ooblayout_ops = {
+ .ecc = oob_ecc,
+ .free = oob_free,
+};
+
+static u32 to_ticks(int kHz, int ps)
+{
+ return DIV_ROUND_UP_ULL((u64)kHz * ps, NSEC_PER_SEC);
+}
+
+static int tango_set_timings(struct mtd_info *mtd,
+ const struct nand_data_interface *conf, bool check_only)
+{
+ const struct nand_sdr_timings *sdr = nand_get_sdr_timings(conf);
+ struct nand_chip *chip = mtd_to_nand(mtd);
+ struct tango_nfc *nfc = to_tango_nfc(chip->controller);
+ struct tango_chip *tchip = to_tango_chip(chip);
+ u32 Trdy, Textw, Twc, Twpw, Tacc, Thold, Trpw, Textr;
+ int kHz = nfc->freq_kHz;
+
+ if (IS_ERR(sdr))
+ return PTR_ERR(sdr);
+
+ if (check_only)
+ return 0;
+
+ Trdy = to_ticks(kHz, sdr->tCEA_max - sdr->tREA_max);
+ Textw = to_ticks(kHz, sdr->tWB_max);
+ Twc = to_ticks(kHz, sdr->tWC_min);
+ Twpw = to_ticks(kHz, sdr->tWC_min - sdr->tWP_min);
+
+ Tacc = to_ticks(kHz, sdr->tREA_max);
+ Thold = to_ticks(kHz, sdr->tREH_min);
+ Trpw = to_ticks(kHz, sdr->tRC_min - sdr->tREH_min);
+ Textr = to_ticks(kHz, sdr->tRHZ_max);
+
+ tchip->timing1 = TIMING(Trdy, Textw, Twc, Twpw);
+ tchip->timing2 = TIMING(Tacc, Thold, Trpw, Textr);
+
+ return 0;
+}
+
+static int chip_init(struct device *dev, struct device_node *np)
+{
+ u32 cs;
+ int err, res;
+ struct mtd_info *mtd;
+ struct nand_chip *chip;
+ struct tango_chip *tchip;
+ struct nand_ecc_ctrl *ecc;
+ struct tango_nfc *nfc = dev_get_drvdata(dev);
+
+ tchip = devm_kzalloc(dev, sizeof(*tchip), GFP_KERNEL);
+ if (!tchip)
+ return -ENOMEM;
+
+ res = of_property_count_u32_elems(np, "reg");
+ if (res < 0)
+ return res;
+
+ if (res != 1)
+ return -ENOTSUPP; /* Multi-CS chips are not supported */
+
+ err = of_property_read_u32_index(np, "reg", 0, &cs);
+ if (err)
+ return err;
+
+ if (cs >= MAX_CS)
+ return -EINVAL;
+
+ chip = &tchip->nand_chip;
+ ecc = &chip->ecc;
+ mtd = &chip->mtd;
+
+ chip->read_byte = tango_read_byte;
+ chip->write_buf = tango_write_buf;
+ chip->read_buf = tango_read_buf;
+ chip->select_chip = tango_select_chip;
+ chip->cmd_ctrl = tango_cmd_ctrl;
+ chip->dev_ready = tango_dev_ready;
+ chip->setup_data_interface = tango_set_timings;
+ chip->options = NAND_USE_BOUNCE_BUFFER
+ | NAND_NO_SUBPAGE_WRITE
+ | NAND_WAIT_TCCS;
+ chip->controller = &nfc->hw;
+ tchip->base = nfc->pbus_base + (cs * 256);
+
+ nand_set_flash_node(chip, np);
+ mtd_set_ooblayout(mtd, &tango_nand_ooblayout_ops);
+ mtd->dev.parent = dev;
+
+ err = nand_scan_ident(mtd, 1, NULL);
+ if (err)
+ return err;
+
+ ecc->mode = NAND_ECC_HW;
+ ecc->algo = NAND_ECC_BCH;
+ ecc->bytes = DIV_ROUND_UP(ecc->strength * FIELD_ORDER, BITS_PER_BYTE);
+
+ ecc->read_page_raw = tango_read_page_raw;
+ ecc->write_page_raw = tango_write_page_raw;
+ ecc->read_page = tango_read_page;
+ ecc->write_page = tango_write_page;
+ ecc->read_oob = tango_read_oob;
+ ecc->write_oob = tango_write_oob;
+
+ err = nand_scan_tail(mtd);
+ if (err)
+ return err;
+
+ tchip->xfer_cfg = XFER_CFG(cs, 1, ecc->steps, METADATA_SIZE);
+ tchip->pkt_0_cfg = PKT_CFG(ecc->size + METADATA_SIZE, ecc->strength);
+ tchip->pkt_n_cfg = PKT_CFG(ecc->size, ecc->strength);
+ tchip->bb_cfg = BB_CFG(mtd->writesize, BBM_SIZE);
+
+ err = mtd_device_register(mtd, NULL, 0);
+ if (err)
+ return err;
+
+ nfc->chips[cs] = tchip;
+
+ return 0;
+}
+
+static int tango_nand_remove(struct platform_device *pdev)
+{
+ int cs;
+ struct tango_nfc *nfc = platform_get_drvdata(pdev);
+
+ dma_release_channel(nfc->chan);
+
+ for (cs = 0; cs < MAX_CS; ++cs) {
+ if (nfc->chips[cs] != NULL)
+ nand_release(&nfc->chips[cs]->nand_chip.mtd);
+ }
+
+ return 0;
+}
+
+static int tango_nand_probe(struct platform_device *pdev)
+{
+ int err;
+ struct clk *clk;
+ struct resource *res;
+ struct tango_nfc *nfc;
+ struct device_node *np;
+
+ nfc = devm_kzalloc(&pdev->dev, sizeof(*nfc), GFP_KERNEL);
+ if (!nfc)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ nfc->reg_base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(nfc->reg_base))
+ return PTR_ERR(nfc->reg_base);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ nfc->mem_base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(nfc->mem_base))
+ return PTR_ERR(nfc->mem_base);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+ nfc->pbus_base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(nfc->pbus_base))
+ return PTR_ERR(nfc->pbus_base);
+
+ clk = clk_get(&pdev->dev, NULL);
+ if (IS_ERR(clk))
+ return PTR_ERR(clk);
+
+ nfc->chan = dma_request_chan(&pdev->dev, "nfc_sbox");
+ if (IS_ERR(nfc->chan))
+ return PTR_ERR(nfc->chan);
+
+ platform_set_drvdata(pdev, nfc);
+ nand_hw_control_init(&nfc->hw);
+ nfc->freq_kHz = clk_get_rate(clk) / 1000;
+
+ for_each_child_of_node(pdev->dev.of_node, np) {
+ err = chip_init(&pdev->dev, np);
+ if (err) {
+ tango_nand_remove(pdev);
+ return err;
+ }
+ }
+
+ return 0;
+}
+
+static const struct of_device_id tango_nand_ids[] = {
+ { .compatible = "sigma,smp8758-nand" },
+ { /* sentinel */ }
+};
+
+static struct platform_driver tango_nand_driver = {
+ .probe = tango_nand_probe,
+ .remove = tango_nand_remove,
+ .driver = {
+ .name = "tango-nand",
+ .of_match_table = tango_nand_ids,
+ },
+};
+
+module_platform_driver(tango_nand_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Sigma Designs");
+MODULE_DESCRIPTION("Tango4 NAND Flash controller driver");
--
2.9.0
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related
* Re: [PATCH v7 1/2] mtd: nand: add tango NFC dt bindings doc
From: Boris Brezillon @ 2016-10-25 13:25 UTC (permalink / raw)
To: Marc Gonzalez
Cc: linux-mtd, Richard Weinberger, DT, Rob Herring, Mark Rutland,
Mason, Sebastian Frias
In-Reply-To: <580F5B06.6030608-y1yR0Z3OICC7zZZRDBGcUA@public.gmane.org>
On Tue, 25 Oct 2016 15:15:50 +0200
Marc Gonzalez <marc_gonzalez-y1yR0Z3OICC7zZZRDBGcUA@public.gmane.org> wrote:
> Add the tango NAND Flash Controller dt bindings documentation.
>
> Signed-off-by: Marc Gonzalez <marc_gonzalez-y1yR0Z3OICC7zZZRDBGcUA@public.gmane.org>
> ---
> Documentation/devicetree/bindings/mtd/tango-nand.txt | 38 ++++++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mtd/tango-nand.txt b/Documentation/devicetree/bindings/mtd/tango-nand.txt
> new file mode 100644
> index 000000000000..3cbf95d6595a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/tango-nand.txt
> @@ -0,0 +1,38 @@
> +Sigma Designs Tango4 NAND Flash Controller (NFC)
> +
> +Required properties:
> +
> +- compatible: "sigma,smp8758-nand"
> +- reg: address/size of nfc_reg, nfc_mem, and pbus_reg
> +- dmas: reference to the DMA channel used by the controller
> +- dma-names: "nfc_sbox"
> +- clocks: reference to the system clock
> +- #address-cells: <1>
> +- #size-cells: <0>
> +
> +Children nodes represent the available NAND chips.
> +See Documentation/devicetree/bindings/mtd/nand.txt for generic bindings.
> +
> +Example:
> +
> + nand: nand@2c000 {
nandc: nand-controller@2c000 {
No need to resend a new version for that (I can fix it when applying),
unless I find other things in patch 2.
> + compatible = "sigma,smp8758-nand";
> + reg = <0x2c000 0x30 0x2d000 0x800 0x20000 0x1000>;
> + dmas = <&dma0 3>;
> + dma-names = "nfc_sbox";
> + clocks = <&clkgen SYS_CLK>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + nand@0 {
> + reg = <0>; /* CS0 */
> + nand-ecc-strength = <14>;
> + nand-ecc-step-size = <1024>;
> + };
> +
> + nand@1 {
> + reg = <1>; /* CS1 */
> + nand-ecc-strength = <14>;
> + nand-ecc-step-size = <1024>;
> + };
> + };
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] irqchip/ls-scfg-msi: update Layerscape SCFG MSI driver
From: Mark Rutland @ 2016-10-25 13:28 UTC (permalink / raw)
To: Minghuan Lian
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Jason Cooper, Roy Zang,
Marc Zyngier, Stuart Yoder, Yang-Leo Li, Scott Wood, Mingkai Hu
In-Reply-To: <1477399162-22881-1-git-send-email-Minghuan.Lian-3arQi8VN3Tc@public.gmane.org>
On Tue, Oct 25, 2016 at 08:39:22PM +0800, Minghuan Lian wrote:
> 1. The patch uses soc_device_match() to match the SoC family
> and revision instead of DTS compatible, because compatible cannot
> describe the SoC revision information.
What difference do you care about? If it affects the programming model
of the device, it should be described in the compatible string, or in
properties on the device node.
Matching the SoC is a bit aof a warning sign. If there's information
that we need, but don't currently have, please extend the binding to
provide it.
> +struct ls_scfg_msi_cfg {
> + u32 ibs_shift; /* Shift of interrupt bit select */
> + u32 msir_irqs; /* The irq number per MSIR */
> + u32 msir_base; /* The base address of MSIR */
> +};
> +static struct ls_scfg_msi_cfg ls1021_msi_cfg = {
> + .ibs_shift = 3,
> + .msir_irqs = IRQS_32_PER_MSIR,
> + .msir_base = 0x4,
> +};
> +
> +static struct ls_scfg_msi_cfg ls1043_rev11_msi_cfg = {
> + .ibs_shift = 2,
> + .msir_irqs = IRQS_8_PER_MSIR,
> + .msir_base = 0x10,
> +};
> +
> +static struct ls_scfg_msi_cfg ls1046_msi_cfg = {
> + .ibs_shift = 2,
> + .msir_irqs = IRQS_32_PER_MSIR,
> + .msir_base = 0x4,
> +};
> +
> +static struct soc_device_attribute soc_msi_matches[] = {
> + { .family = "QorIQ LS1021A",
> + .data = &ls1021_msi_cfg },
> + { .family = "QorIQ LS1012A",
> + .data = &ls1021_msi_cfg },
> + { .family = "QorIQ LS1043A", .revision = "1.0",
> + .data = &ls1021_msi_cfg },
> + { .family = "QorIQ LS1043A", .revision = "1.1",
> + .data = &ls1043_rev11_msi_cfg },
> + { .family = "QorIQ LS1046A",
> + .data = &ls1046_msi_cfg },
> + { },
If the base address of a register differs in this manner, then these
aren't "compatible", and should have separate strings.
Either that, or the binding should mandate a property to describe this.
You can *add* a new string, for which that property is mandatory, if you
believe you will see greate variation here in future.
> static const struct of_device_id ls_scfg_msi_id[] = {
> - { .compatible = "fsl,1s1021a-msi", },
> - { .compatible = "fsl,1s1043a-msi", },
> + { .compatible = "fsl,ls-scfg-msi" },
> {},
NAK. Breaking support for existing DTBs is not ok.
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq
From: Marc Zyngier @ 2016-10-25 13:38 UTC (permalink / raw)
To: Jerome Brunet, Linus Walleij
Cc: Carlo Caione, Kevin Hilman, open list:ARM/Amlogic Meson...,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Thomas Gleixner, Jason Cooper, Rob Herring, Catalin Marinas,
Will Deacon, Russell King
In-Reply-To: <1477400900.2482.51.camel-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
On 25/10/16 14:08, Jerome Brunet wrote:
> On Tue, 2016-10-25 at 11:38 +0100, Marc Zyngier wrote:
>>>
>> On 25/10/16 10:14, Linus Walleij wrote:
>>>
>>> On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylibre.c
>>> om> wrote:
>>>
>>>>
>>>>>
>>>>> Isn't this usecase (also as described in the cover letter) a
>>>>> textbook
>>>>> example of when you should be using hierarchical irqdomain?
>>>>>
>>>>> Please check with Marc et al on hierarchical irqdomains.
>>>>
>>>> Linus,
>>>> Do you mean I should create a new hierarchical irqdomains in each
>>>> of
>>>> the two pinctrl instances we have in these SoC, these domains
>>>> being
>>>> stacked on the one I just added for controller in irqchip ?
>>>>
>>>> I did not understand this is what you meant when I asked you the
>>>> question at ELCE.
>>>
>>> Honestly, I do not understand when and where to properly use
>>> hierarchical irqdomain, even after Marc's talk at ELC-E.
>>
>> I probably didn't do that good a job explaining it then. Let's try
>> again. You want to use hierarchical domains when you want to describe
>> an
>> interrupt whose path traverses multiple controllers without ever
>> being
>> multiplexed with other signals. As long as you have this 1:1
>> relationship between controllers, you can use them.
>>
>
> Linus, Marc,
>
> The calculation is question here is meant to get the appropriate hwirq
> number from a particular gpio (and deal with the gpios that can't
> provide an irq at all).
>
> If I look at other gpio drivers, many are doing exactly this kind of
> calculation before calling 'irq_create_mapping' in the to_irq callback.
> For example:
> - pinctrl/nomadik/pinctrl-abx500.c
> - pinctrl/samsung/pinctrl-exynos5440.c
>
> Some can afford to create all the mappings in the probe and just call
> 'irq_find_mapping' (gpio/gpio_tegra.c) but this would not work here. We
> have only 8 upstream irqs for 130+ pins, so only 8 mappings possible at
> a time.
>
> My understanding is that irqdomain provide a way to map hwirq to linux
> virq (and back), not map gpio number to hwirq, right?
But why are those number different? Why don't you use the same
namespace? If gpio == hwirq, all your problems are already solved. If
you don't find the mapping in the irqdomain, then there is no irq, end
of story. What am I missing?
>
> Even if I implement an another irqdomain at the gpio level, I would
> still have to perform this kind of calculation, one way or the other.
>
>>> Which is problematic since quite a few GPIO drivers now
>>> need to use them.
>>>
>>> I will review his slides, in the meantime I would say: whatever
>>> Marc ACKs is fine with me. I trust this guy 100%. So I guess I
>>> could ask that he ACK the entire chain of patches
>>> from GIC->specialchip->GPIO.
>
> Actually this discussion go me thinking about another issue we have
> with this hardware.
> We are looking for a way to implement support for IRQ_TYPE_EDGE_BOTH
> (needed for things like gpio-keys or mmc card detect).
> The controller can do each edge but not both at the same time.
> I'm thinking that implementing another irqdomain at the gpio level
> would allow to properly check the pad level in the EOI callback then
> set the next expected edge type accordingly (using
> 'irq_chip_set_type_parent')
>
> Would it be acceptable ?
I really don't see what another irqdomain brings to the table. This is
not a separate piece of HW, so the hwirq:irq mapping is still the same.
I fail to see what the benefit is.
> It looks a few other drivers deal with IRQ_TYPE_EDGE_BOTH in a similar
> way (gpio/gpio-omap.c, gpio/gpio-dwapb.c)
Being already done doesn't make it reliable. If the line goes low
between latching the rising edge and reprogramming the trigger, you've
lost at least *two* interrupts (the falling edge and the following
rising edge).
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [RFC v2] da850: DDR2/mDDR memory controller driver
From: Bartosz Golaszewski @ 2016-10-25 13:41 UTC (permalink / raw)
To: Kevin Hilman, Michael Turquette, Sekhar Nori, Rob Herring,
Frank Rowand, Mark Rutland, Peter Ujfalusi, Russell King
Cc: LKML, arm-soc, linux-drm, linux-devicetree, Jyri Sarha,
Tomi Valkeinen, David Airlie, Laurent Pinchart,
Bartosz Golaszewski
This is a follow-up for the series[1] adding new bus and memory drivers
to better support the TI LCD controller on the da850-lcdk board.
The general consensus of the discussion that followed was that DT is
not the right tool for this kind of SoC performance tweaks.
In order to avoid committing to stable DT bindings, we only introduce
two common properties (compatible and reg) while the configuration
register values are hard-coded for each board (currently only lcdk).
In the future, once linux gets a proper framework for performance
knobs, we'll convert this driver to using the better solution.
I'm sending a single patch this time as RFC to get some reviews and
see it the approach is viewed as correct.
[1] https://lkml.org/lkml/2016/10/17/613
v1 -> v2:
- changed the compatible string to make it more descriptive
- changed the DT bindings description to describe the device, not the
driver's functionalities
- switched to using of_machine_is_compatible() instead of handcoding
the same functionality
- used platform_get_resource() instead of ioremapping registers by hand
Bartosz Golaszewski (1):
ARM: memory: da8xx-ddrctl: new driver
.../memory-controllers/ti-da8xx-ddrctl.txt | 20 +++
drivers/memory/Kconfig | 8 +
drivers/memory/Makefile | 1 +
drivers/memory/da8xx-ddrctl.c | 175 +++++++++++++++++++++
4 files changed, 204 insertions(+)
create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
create mode 100644 drivers/memory/da8xx-ddrctl.c
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [RFC v2] ARM: memory: da8xx-ddrctl: new driver
From: Bartosz Golaszewski @ 2016-10-25 13:41 UTC (permalink / raw)
To: Kevin Hilman, Michael Turquette, Sekhar Nori, Rob Herring,
Frank Rowand, Mark Rutland, Peter Ujfalusi, Russell King
Cc: LKML, arm-soc, linux-drm, linux-devicetree, Jyri Sarha,
Tomi Valkeinen, David Airlie, Laurent Pinchart,
Bartosz Golaszewski
In-Reply-To: <1477402876-22472-1-git-send-email-bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
Create a new driver for the da8xx DDR2/mDDR controller and implement
support for writing to the Peripheral Bus Burst Priority Register.
Signed-off-by: Bartosz Golaszewski <bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
.../memory-controllers/ti-da8xx-ddrctl.txt | 20 +++
drivers/memory/Kconfig | 8 +
drivers/memory/Makefile | 1 +
drivers/memory/da8xx-ddrctl.c | 175 +++++++++++++++++++++
4 files changed, 204 insertions(+)
create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
create mode 100644 drivers/memory/da8xx-ddrctl.c
diff --git a/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
new file mode 100644
index 0000000..7e271dd
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
@@ -0,0 +1,20 @@
+* Device tree bindings for Texas Instruments da8xx DDR2/mDDR memory controller
+
+The DDR2/mDDR memory controller present on Texas Instruments da8xx SoCs features
+a set of registers which allow to tweak the controller's behavior.
+
+Documentation:
+OMAP-L138 (DA850) - http://www.ti.com/lit/ug/spruh82c/spruh82c.pdf
+
+Required properties:
+
+- compatible: "ti,da850-ddr-controller" - for da850 SoC based boards
+- reg: a tuple containing the base address of the memory
+ controller and the size of the memory area to map
+
+Example for da850 shown below.
+
+ddrctl {
+ compatible = "ti,da850-ddr-controller";
+ reg = <0xB0000000 0x100>;
+};
diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
index 4b4c0c3..ec80e35 100644
--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -134,6 +134,14 @@ config MTK_SMI
mainly help enable/disable iommu and control the power domain and
clocks for each local arbiter.
+config DA8XX_DDRCTL
+ bool "Texas Instruments da8xx DDR2/mDDR driver"
+ depends on ARCH_DAVINCI_DA8XX
+ help
+ This driver is for the DDR2/mDDR Memory Controller present on
+ Texas Instruments da8xx SoCs. It's used to tweak various memory
+ controller configuration options.
+
source "drivers/memory/samsung/Kconfig"
source "drivers/memory/tegra/Kconfig"
diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
index b20ae38..e88097fb 100644
--- a/drivers/memory/Makefile
+++ b/drivers/memory/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o
obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o
obj-$(CONFIG_JZ4780_NEMC) += jz4780-nemc.o
obj-$(CONFIG_MTK_SMI) += mtk-smi.o
+obj-$(CONFIG_DA8XX_DDRCTL) += da8xx-ddrctl.o
obj-$(CONFIG_SAMSUNG_MC) += samsung/
obj-$(CONFIG_TEGRA_MC) += tegra/
diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c
new file mode 100644
index 0000000..66022df
--- /dev/null
+++ b/drivers/memory/da8xx-ddrctl.c
@@ -0,0 +1,175 @@
+/*
+ * TI da8xx DDR2/mDDR controller driver
+ *
+ * Copyright (C) 2016 BayLibre SAS
+ *
+ * Author:
+ * Bartosz Golaszewski <bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_fdt.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+
+/*
+ * REVISIT: Linux doesn't have a good framework for the kind of performance
+ * knobs this driver controls. We can't use device tree properties as it deals
+ * with hardware configuration rather than description. We also don't want to
+ * commit to maintaining some random sysfs attributes.
+ *
+ * For now we just hardcode the register values for the boards that need
+ * some changes (as is the case for the LCD controller on da850-lcdk - the
+ * first board we support here). When linux gets an appropriate framework,
+ * we'll easily convert the driver to it.
+ */
+
+struct da8xx_ddrctl_config_knob {
+ const char *name;
+ u32 reg;
+ u32 mask;
+ u32 offset;
+};
+
+static const struct da8xx_ddrctl_config_knob da8xx_ddrctl_knobs[] = {
+ {
+ .name = "da850-pbbpr",
+ .reg = 0x20,
+ .mask = 0xffffff00,
+ .offset = 0,
+ },
+};
+
+struct da8xx_ddrctl_setting {
+ const char *name;
+ u32 val;
+};
+
+struct da8xx_ddrctl_board_settings {
+ const char *board;
+ const struct da8xx_ddrctl_setting *settings;
+};
+
+static const struct da8xx_ddrctl_setting da850_lcdk_ddrctl_settings[] = {
+ {
+ .name = "da850-pbbpr",
+ .val = 0x20,
+ },
+ { }
+};
+
+static const struct da8xx_ddrctl_board_settings da8xx_ddrctl_board_confs[] = {
+ {
+ .board = "ti,da850-lcdk",
+ .settings = da850_lcdk_ddrctl_settings,
+ },
+};
+
+static const struct da8xx_ddrctl_config_knob *
+da8xx_ddrctl_match_knob(const struct da8xx_ddrctl_setting *setting)
+{
+ const struct da8xx_ddrctl_config_knob *knob;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(da8xx_ddrctl_knobs); i++) {
+ knob = &da8xx_ddrctl_knobs[i];
+
+ if (strcmp(knob->name, setting->name) == 0)
+ return knob;
+ }
+
+ return NULL;
+}
+
+static const struct da8xx_ddrctl_setting *da8xx_ddrctl_get_board_settings(void)
+{
+ const struct da8xx_ddrctl_board_settings *board_settings;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(da8xx_ddrctl_board_confs); i++) {
+ board_settings = &da8xx_ddrctl_board_confs[0];
+
+ if (of_machine_is_compatible(board_settings->board))
+ return board_settings->settings;
+ }
+
+ return NULL;
+}
+
+static int da8xx_ddrctl_probe(struct platform_device *pdev)
+{
+ const struct da8xx_ddrctl_config_knob *knob;
+ const struct da8xx_ddrctl_setting *setting;
+ struct device_node *node;
+ struct resource *res;
+ void __iomem *ddrctl;
+ struct device *dev;
+ u32 reg;
+
+ dev = &pdev->dev;
+ node = dev->of_node;
+
+ setting = da8xx_ddrctl_get_board_settings();
+ if (!setting) {
+ dev_err(dev, "no settings for board '%s'\n",
+ of_flat_dt_get_machine_name());
+ return -EINVAL;
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ ddrctl = devm_ioremap_resource(dev, res);
+ if (IS_ERR(ddrctl)) {
+ dev_err(dev, "unable to map memory controller registers\n");
+ return PTR_ERR(ddrctl);
+ }
+
+ for (; setting->name; setting++) {
+ knob = da8xx_ddrctl_match_knob(setting);
+ if (!knob) {
+ dev_warn(dev,
+ "no such config option: %s\n", setting->name);
+ continue;
+ }
+
+ if (knob->reg > (res->end - res->start - sizeof(u32))) {
+ dev_warn(dev,
+ "register offset of '%s' exceeds mapped memory size\n",
+ knob->name);
+ continue;
+ }
+
+ reg = __raw_readl(ddrctl + knob->reg);
+ reg &= knob->mask;
+ reg |= setting->val << knob->offset;
+
+ dev_dbg(dev, "writing 0x%08x to %s\n", reg, setting->name);
+
+ __raw_writel(reg, ddrctl + knob->reg);
+ }
+
+ return 0;
+}
+
+static const struct of_device_id da8xx_ddrctl_of_match[] = {
+ { .compatible = "ti,da850-ddr-controller", },
+ { },
+};
+
+static struct platform_driver da8xx_ddrctl_driver = {
+ .probe = da8xx_ddrctl_probe,
+ .driver = {
+ .name = "da850-ddr-controller",
+ .of_match_table = da8xx_ddrctl_of_match,
+ },
+};
+module_platform_driver(da8xx_ddrctl_driver);
+
+MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>");
+MODULE_DESCRIPTION("TI da8xx DDR2/mDDR controller driver");
+MODULE_LICENSE("GPL v2");
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH v3 3/6] dt-bindings: pinctrl: Deprecate sunxi pinctrl bindings
From: Maxime Ripard @ 2016-10-25 13:41 UTC (permalink / raw)
To: Linus Walleij
Cc: Chen-Yu Tsai, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Rob Herring
In-Reply-To: <CACRpkdYGYPhPZQmgeLA-K-RQKZMLy1qHY6P7LS6SKsFC=d=MHg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1403 bytes --]
Hi Linus,
On Tue, Oct 25, 2016 at 02:07:27PM +0200, Linus Walleij wrote:
> On Mon, Oct 24, 2016 at 9:49 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
>
> > However, it looks like the first patch from this serie is missing from
> > your tree, is there a reason for that?
>
> No can you point it out?
Sure:
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-October/462500.html
> > Also, in order to preserve bisectability, could you create an
> > immutable branch for those sunxi patches so that I can merge the DT
> > bits?
>
> It's too late because they are already in the devel branch
> and mixed up with merged of *other* immutable stuff.
Hmmmm, ok.
> However I think it is plain wrong to try to keep any bisectability
> between the kernel at large and arch/*/boot/dts/*, because
> the DTS stuff is supposed to at some point be maintained outside
> of the kernel and for all OSes, they simply shouldn't be sync:ed.
Yes, in the case of a new driver that needs to be introduced, I
definitely get your point.
However, during a conversion like we're doing here, this is not ideal,
because it essentially means that you will not have a branch that
works, at all.
I'll hold off on those patches until 4.11 then.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH] ARM: sti: stih410-clocks: Add PROC_STFE as a critical clock
From: Peter Griffin @ 2016-10-25 13:49 UTC (permalink / raw)
To: Lee Jones
Cc: linux-arm-kernel, linux-kernel, kernel, patrice.chotard,
devicetree
In-Reply-To: <20161025100141.GH8574@dell>
Hi Lee,
On Tue, 25 Oct 2016, Lee Jones wrote:
> On Tue, 25 Oct 2016, Peter Griffin wrote:
>
> > Hi Lee,
> >
> > On Tue, 25 Oct 2016, Lee Jones wrote:
> >
> > > On Mon, 24 Oct 2016, Peter Griffin wrote:
> > >
> > > > Hi Lee,
> > > >
> > > > On Mon, 24 Oct 2016, Lee Jones wrote:
> > > > > On Tue, 18 Oct 2016, Peter Griffin wrote:
> > > > >
> > > > > > Once the ST frontend demux HW IP has been enabled, the clock can't
> > > > > > be disabled otherwise the system will hang and the board will
> > > > > > be unserviceable.
> > > > > >
> > > > > > To allow balanced clock enable/disable calls in the driver we use
> > > > > > the critical clock infrastructure to take an extra reference on the
> > > > > > clock so the clock will never actually be disabled.
> > > > >
> > > > > This is an abuse of the critical-clocks framework, and is exactly the
> > > > > type of hack I promised the clk guys I'd try to prevent.
> > > >
> > > > I expect the best way to do this would be to write some documentation on the
> > > > clock-critical DT binding and/or CRITICAL_CLK flag. The only documentation I can
> > > > find currently is with the initial patch series [1] and the comment in
> > > > clk-provider.h of
> > > >
> > > > #define CLK_IS_CRITICAL BIT(11) /* do not gate, ever */
> > > >
> > > > Or the patch decription
> > > >
> > > > "Critical clocks are those which must not be gated, else undefined
> > > > or catastrophic failure would occur. Here we have chosen to
> > > > ensure the prepare/enable counts are correctly incremented, so as
> > > > not to confuse users with enabled clocks with no visible users."
> > > >
> > > > Which is the functionality I want for this clock.
> > >
> > > No, that's not the functionality you want.
> >
> > Yes it is :)
>
> No, it's the functionally that is most convenient.
Nope, the solution you proposed already exists upstream so changing it is not
convienient, it is IMO the correct thing to do and the current upstream solution
is the hack.
Allowing the common CCF to disable this clock during clk_disable_unused is
wrong, and can lead to catastrophic failure which requires a reboot.
IMO Linux needs to be resilient to whatever state the hardware could be left in
by previous software. It can't assume it will be OK to disable this clock, we
*know* there is a HW bug, we *know* that disabling the clock can brick the
board.
So I stick with my initial assertion, if this clock is enabled when Linux is
booted it *should not* be disabled.
>
> > > You want for the clock not
> > > to be RE-gated (big difference). Currently, the STFE clock will never
> > > be gated, even when a) it's not used and b) can actually be disabled.
> > > You're needlessly wasting power here.
> >
> > IMO it is *never* safe for Linux to gate this clock, as you have no idea
> > on the state of the hardware from the primary and secondary bootloaders.
>
> You can say that with any of the clocks on this platform.
This is a odd thing to say. I'm not aware of any other clocks which
will render the platform unusable that aren't already listed as critical for the
STi platform.
>
> > If the clock is enabled when Linux boots, the Linux clock framework *needs*
> > to assume the hardware may have been used in previous boot stages, and it should
> > not attempt to disable the clock.
>
> None of the boot loaders we use do this.
But the Linux kernel isn't just used by us. It is not uncommon for STB
bootloaders to get information from the frontend as part of the boot process.
>
> I have never seen the STFE clock crash a platform.
I have, it leads to the same behaviour as turning off any of the other critical
clocks, the SoC is dead and the board needs to be rebooted.
>
> > > Also, in your use-case there is a visible user, and the prepare/enable
> > > counts will be correct.
> >
> > Your correct there is a visible user, but this is the same as CLK_EXT2F_A9 where
> > there are multiple users of the clock (SPI, I2C, UART etc).
>
> Right, but their clocks can not be turned off, ever. So all the boxes
> are ticked for criticalness. Not the case with your clock.
This clock also should never be turned off by Linux..ever. It is unsafe to do so.
Incidentally this isn't the only platform upstream where the requirement for a
clocks criticalness is dependent on whether it was enabled during boot. Take a
look in bcm/clk-bcm2835.c
/* If the clock wasn't actually enabled at boot, it's not
* critical.
*/
if (!(cprman_read(cprman, data->ctl_reg) & CM_ENABLE))
init.flags &= ~CLK_IS_CRITICAL;
>
> > > > > If this, or
> > > > > any other IP has some quirks (i.e. once enabled, if this clock is
> > > > > subsequently disabled it will have a catastrophic effect on the
> > > > > platform), then they should be worked around in the driver.
> > > > >
> > > > > The correct thing to do here is craft a clk-keep-on flag and ensure it
> > > > > is set to true for the effected platform(s)' platform data.
> > > >
> > > > I'm always wary of creating a driver specific flag, especially when its
> > > > purpose is to do the same thing as an existing mechanism provided by the
> > > > subsystem of not gating the clock.
> > >
> > > Using existing sub-system supplied mechanisms in the way they were not
> > > intended is sub-optimal (read "hacky").
> >
> > I think the scope of this flag has been defined in a very narrow way, which is
> > making you want to suggest lots of hacks in the client driver.
>
> The scope was narrowed intentionally, buy the maintainers. I am
> merely reflecting their views.
What is your recollection of their views? LIke I said before this should be
documented somewhere.
> If you really wish to contend these,
> you should take it up with them.
>
> > > > I can see a couple of problems with what you propose:
> > > >
> > > > 1) You have to put the clk-keep-on flag in every driver which consumes the
> > > > clock. IMO it is much better to have this knowledge in the SoC's
> > > > clock driver so every consumer of the clock automatically benefits.
> > >
> > > That would also be fine(ish). The issue is that this problem is
> > > board specific, so the platform clock driver would have to contain
> > > board level knowledge.
> >
> > It is not board specific. It is a SoC HW bug, so by definition present on all
> > boards which have this SoC.
>
> Okay SoC specific. Is there a SoC specific clock driver?
STi platform clocks are mainly described in DT in stih407/10-clock.dsti, which is SoC
specific, and binds with code in drivers/clk/st. So the marking of a clock as
critical is done on a SoC basis which is what I want.
A quick grep of CLK_IGNORE_UNUSED or CLK_IS_CRITICAL, will show you where the
equivalent code for other platforms is located.
>
> > > Also, if you were to implement this, it would
> > > too mess up reference counting in the core.
>
> This still stands.
I don't understand what you mean here. The point of using CCF and the critical
flag is so the reference counting of the clock is held at 1.
>
> > > > 2) You don't benefit from the CLK_IS_CRITICAL reference counting logic in
> > > > clk.c. So then each driver has to also work around that to get sensible reference
> > > > counts. e.g.
> > > >
> > > > if (!__clk_is_enabled(clk) && pdata->clk-keep-on)
> > > > clk_enable(clk)
> > > >
> > > > Which seems to me to be fighting against the subsystem. Given that the only use of
> > > > _clk_is_enabled() outside drivers/clk is in an old arch/arm/mach-omap2/pm24xx.c
> > > > driver I suspect its use is frowned upon, and it shouldn't really be an EXPORTED_SYMBOL.
> > >
> > > In this instance, since the STFE clock is only used by this IP, I
> > > would choose to handle it in the driver.
> >
> > There are other IPs within this IP block for which upstream drivers don't yet exist.
>
> If they are many and various, we can discuss alternative solutions.
>
> What are they?
You have hardware for Merged Input Blocks, SWTS, TSDMA, Cable Card Stream
Convertor and Tag Counter which is currently unsupported upstream.
>
> > > This can be done using a
> > > single flag stored in pdata which should be fetched using
> > > of_match_device(). This way there is no need for any more API abuse;
> > > either by incorrectly identifying the STFE clock as critical OR
> > > invoking any internal __clk_*() calls.
> > >
> > > Enable the clock once in .probe(), which you already do.
> >
> > But these drivers are by default built as modules, so when you rmmod, and insmod the
> > driver you now have an ever increasing clock reference count. This is the
> > problem I was describing in the previous email, and why what you propose is a
> > bad idea.
>
> Then the variable would have to be exported.
I don't understand why if you had a choice you would choose to put the logic
there over the clock subsystem.
The reasoning is the same as with the I2C & SPI drivers. You don't want to have
to put a platform specific clocking knowledge into each driver, when it
could live centrally in the clock subsystem for the platform. You also don't want to
have to change the driver when a different SoC with the same IP has a different
clock tree.
Not only that CCF *has* to have the knowledge internal to the clock susbsytem so
that it doesn't disable the clock during the clk_disable_unused phase on boot.
>
> > > Then, whenever you do any power saving do:
> > >
> > > suspend()
> > > {
> > > if (!ddata->enable_clk_once)
> > > clk_disable(clk);
> > > }
> > >
> > > resume()
> > > {
> > > if (!ddata->enable_clk_once)
> > > clk_enable(clk);
> > > }
> > >
> > > However, looking at your driver, I think this point might even be
> > > moot, since you don't have any power saving. The only time you
> > > disable the clock is in the error path. Just replace that with a
> > > comment about the platform's unfortunate errata.
> >
> > This is exactly what I want to avoid doing. The driver already has these
> > hacks as it was waiting for the critical clock patches to land so I could remove
> > them and fix the problem properly.
> >
> > Much like you did with the I2C and SPI drivers, where you removed similar hacks
> > and added the clock to the critical clock list.
>
> Right, see above.
>
> > > > [1] https://lkml.org/lkml/2016/1/18/272
> > >
> > > I'm glad you mentioned this. Let's take a look:
> > >
> > > > Some platforms contain clocks which if gated, will cause undefined or
> > > > catastrophic behaviours. As such they are not to be turned off, ever.
> > >
> > > Not the case here.
> > >
> > > This clock *can* be gated and can be turned off *sometimes*.
> >
> > See above, if the clock is on when Linux boots it can never be assumed that it
> > is safe to disable it.
>
> See above.
>
> > > > Many of these such clocks do not have devices, thus device drivers
> > > > where clocks may be enabled and references taken to ensure they stay
> > > > enabled do not exist. Therefore, we must handle these such cases in
> > > > the core.
> > >
> > > This clock *does* have a driver and correct references *can* be
> > > taken.
> >
> > As above this is the same for CLK_EXT2F_A9, which has multiple users in the
> > kernel.
> >
> > The point is we don't wish to have the knowledge in the individual drivers that
> > this clock is critical and can destroy the system if it is disabled.
This point stlll stands.
> >
> > >
> > > [...]
> > >
> > > All I'm saying is, and it's the same thing I've said many times; these
> > > types of issues do not exhibit the same set of symptoms as a critical
> > > clock by definition. Critical clocks are those which references can
> > > not be taken by any other means.
> >
> > That is not how you are using it currently. See CLK_EXT2F_A9 and
> > CLK_TX_ICN_DMU.
So does this one.
> >
> > > Think of the critical clock
> > > framework as a mechanism to circumvent the requirement of writing a
> > > special driver which would *only* handle clocks i.e. an interconnect
> > > driver in the ST case.
> > >
> >
> > You didn't need a new flag for that. CLK_IGNORE_UNUSED already allowed you to not
> > write a specical driver which only handled for example an interconnect clock and
> > would ensure it wouldn't be gated by the disable_unused logic.
> >
> > IMO the point of the critical clock stuff, and what CLK_IGNORE_UNUSED *did not* allow
> > you to do is for situations like CLK_EXT2F_A9, CLK_TX_ICN_DMU and PROC_STFE where users
> > *do* exist in the kernel, but the SoC clock driver has *special* knowledge of the
> > clock tree on the SoC and knows that despite what the drivers are doing with
> > their enable/disable calls the clock should never be disabled.
> >
> > In fact out of what we currently mark as clock-critical on STi platform most
> > of the clocks could have been handled perfectly fine with the CLK_IGNORE_UNUSED
> > flag (apart from the fact that there is no way to set the flag from DT). It is
> > only CLK_EXT2F_A9 and CLK_TX_ICN_DMU which *do* have kernel users where the
> > extra functionality of critical clocks is required (we need the additional
> > reference taken by the clk framework to avoid the kernel drivers from disabling
> > the clock).
And most importantly this one.
>
> Probably best to take your special use-case up with the Clock
> Maintainers. As the author of the critical-clocks patch-set, I would
> say that your use-case does not tick all the boxes for use of the
> critical-clock mechanism, but at the end of the day, it really isn't
> my train-set.
>
I don't think there is anything special about it. If when Linux
boots the clock is enabled you shouldn't disable it, just like the other
critical clocks.
regards,
Peter.
^ permalink raw reply
* Re: [PATCH v9 1/4] soc: mediatek: Refine scpsys to support multiple platform
From: Matthias Brugger @ 2016-10-25 14:04 UTC (permalink / raw)
To: James Liao, Sascha Hauer
Cc: Rob Herring, Kevin Hilman, Daniel Kurtz,
srv_heupstream-NuS5LvNUpcJWk0Htik3J/w,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <1476953798-23263-2-git-send-email-jamesjj.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
Hi James,
On 10/20/2016 10:56 AM, James Liao wrote:
> -static int scpsys_probe(struct platform_device *pdev)
> +static void init_clks(struct platform_device *pdev, struct clk *clk[CLK_MAX])
I prefer struct clk **clk.
> +{
> + int i;
> +
> + for (i = CLK_NONE + 1; i < CLK_MAX; i++)
> + clk[i] = devm_clk_get(&pdev->dev, clk_names[i]);
> +}
> +
> +static struct scp *init_scp(struct platform_device *pdev,
> + const struct scp_domain_data *scp_domain_data, int num)
> {
> struct genpd_onecell_data *pd_data;
> struct resource *res;
> - int i, j, ret;
> + int i, j;
> struct scp *scp;
> - struct clk *clk[MT8173_CLK_MAX];
> + struct clk *clk[CLK_MAX];
should be *[CLK_MAX - 1] but I would prefer to define in the enum:
CLK_MAX = CLK_VENC_LT,
If you are ok with it, I can fix both of my comments when applying.
Regards,
Matthias
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2
From: Jean-Francois Moine @ 2016-10-25 14:14 UTC (permalink / raw)
To: Maxime Ripard
Cc: Dave Airlie, Liam Girdwood, Mark Brown, Rob Herring,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20161024140419.ah6hywf3b24pj3fv@lukather>
On Mon, 24 Oct 2016 16:04:19 +0200
Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi,
Hi Maxime,
> On Fri, Oct 21, 2016 at 09:26:18AM +0200, Jean-Francois Moine wrote:
> > Allwinner's recent SoCs, as A64, A83T and H3, contain a new display
> > engine, DE2.
> > This patch adds a DRM video driver for this device.
> >
> > Signed-off-by: Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org>
>
> Output from checkpatch:
> total: 0 errors, 20 warnings, 83 checks, 1799 lines checked
>
> > ---
> > .../bindings/display/sunxi/sunxi-de2.txt | 83 +++
> > drivers/gpu/drm/Kconfig | 2 +
> > drivers/gpu/drm/Makefile | 1 +
> > drivers/gpu/drm/sunxi/Kconfig | 21 +
> > drivers/gpu/drm/sunxi/Makefile | 7 +
> > drivers/gpu/drm/sunxi/de2_crtc.c | 475 +++++++++++++++++
> > drivers/gpu/drm/sunxi/de2_crtc.h | 63 +++
> > drivers/gpu/drm/sunxi/de2_de.c | 591 +++++++++++++++++++++
> > drivers/gpu/drm/sunxi/de2_drm.h | 47 ++
> > drivers/gpu/drm/sunxi/de2_drv.c | 378 +++++++++++++
> > drivers/gpu/drm/sunxi/de2_plane.c | 119 +++++
> > 11 files changed, 1787 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt
> > create mode 100644 drivers/gpu/drm/sunxi/Kconfig
> > create mode 100644 drivers/gpu/drm/sunxi/Makefile
> > create mode 100644 drivers/gpu/drm/sunxi/de2_crtc.c
> > create mode 100644 drivers/gpu/drm/sunxi/de2_crtc.h
> > create mode 100644 drivers/gpu/drm/sunxi/de2_de.c
> > create mode 100644 drivers/gpu/drm/sunxi/de2_drm.h
> > create mode 100644 drivers/gpu/drm/sunxi/de2_drv.c
> > create mode 100644 drivers/gpu/drm/sunxi/de2_plane.c
> >
> > diff --git a/Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt b/Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt
> > new file mode 100644
> > index 0000000..f9cd67a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt
> > @@ -0,0 +1,83 @@
> > +Allwinner sunxi Display Engine 2 subsystem
> > +==========================================
> > +
> > +The sunxi DE2 subsystem contains a display controller (DE2),
>
> sunxi is a made up name, and doesn't really mean anything. You can
> call it either sun8i (because it was introduced in that family).
OK.
> > +one or two LCD controllers (TCON) and their external interfaces.
> > +
> > +Display controller
> > +==================
> > +
> > +Required properties:
> > +
> > +- compatible: value should be one of the following
> > + "allwinner,sun8i-a83t-display-engine"
> > + "allwinner,sun8i-h3-display-engine"
> > +
> > +- clocks: must include clock specifiers corresponding to entries in the
> > + clock-names property.
> > +
> > +- clock-names: must contain
> > + "gate": for DE activation
> > + "clock": DE clock
>
> We've been calling them bus and mod.
I can understand "bus" (which is better than "apb"), but why "mod"?
> > +
> > +- resets: phandle to the reset of the device
> > +
> > +- ports: phandle's to the LCD ports
>
> Please use the OF graph.
These ports are references to the graph of nodes. See
http://www.kernelhub.org/?msg=911825&p=2
[snip]
> > diff --git a/drivers/gpu/drm/sunxi/de2_crtc.c b/drivers/gpu/drm/sunxi/de2_crtc.c
> > new file mode 100644
> > index 0000000..dae0fab
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sunxi/de2_crtc.c
> > @@ -0,0 +1,475 @@
> > +/*
> > + * Allwinner DRM driver - DE2 CRTC
> > + *
> > + * Copyright (C) 2016 Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + */
> > +
> > +#include <linux/component.h>
> > +#include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_atomic_helper.h>
> > +#include <asm/io.h>
> > +#include <linux/of_irq.h>
> > +
> > +#include "de2_drm.h"
> > +#include "de2_crtc.h"
> > +
> > +/* I/O map */
> > +
> > +struct tcon {
> > + u32 gctl;
> > +#define TCON_GCTL_TCON_En BIT(31)
> > + u32 gint0;
> > +#define TCON_GINT0_TCON1_Vb_Int_En BIT(30)
> > +#define TCON_GINT0_TCON1_Vb_Int_Flag BIT(14)
> > + u32 gint1;
> > + u32 dum0[13];
> > + u32 tcon0_ctl; /* 0x40 */
> > +#define TCON0_CTL_TCON_En BIT(31)
> > + u32 dum1[19];
> > + u32 tcon1_ctl; /* 0x90 */
> > +#define TCON1_CTL_TCON_En BIT(31)
> > +#define TCON1_CTL_Interlace_En BIT(20)
> > +#define TCON1_CTL_Start_Delay_SHIFT 4
> > +#define TCON1_CTL_Start_Delay_MASK GENMASK(8, 4)
> > + u32 basic0; /* XI/YI */
> > + u32 basic1; /* LS_XO/LS_YO */
> > + u32 basic2; /* XO/YO */
> > + u32 basic3; /* HT/HBP */
> > + u32 basic4; /* VT/VBP */
> > + u32 basic5; /* HSPW/VSPW */
> > + u32 dum2;
> > + u32 ps_sync; /* 0xb0 */
> > + u32 dum3[15];
> > + u32 io_pol; /* 0xf0 */
> > +#define TCON1_IO_POL_IO0_inv BIT(24)
> > +#define TCON1_IO_POL_IO1_inv BIT(25)
> > +#define TCON1_IO_POL_IO2_inv BIT(26)
> > + u32 io_tri;
> > + u32 dum4[2];
> > +
> > + u32 ceu_ctl; /* 0x100 */
> > +#define TCON_CEU_CTL_ceu_en BIT(31)
> > + u32 dum5[3];
> > + u32 ceu_rr;
> > + u32 ceu_rg;
> > + u32 ceu_rb;
> > + u32 ceu_rc;
> > + u32 ceu_gr;
> > + u32 ceu_gg;
> > + u32 ceu_gb;
> > + u32 ceu_gc;
> > + u32 ceu_br;
> > + u32 ceu_bg;
> > + u32 ceu_bb;
> > + u32 ceu_bc;
> > + u32 ceu_rv;
> > + u32 ceu_gv;
> > + u32 ceu_bv;
> > + u32 dum6[45];
> > +
> > + u32 mux_ctl; /* 0x200 */
> > + u32 dum7[63];
> > +
> > + u32 fill_ctl; /* 0x300 */
> > + u32 fill_start0;
> > + u32 fill_end0;
> > + u32 fill_data0;
> > +};
>
> Please use defines instead of the structures.
I think that structures are more readable.
> > +
> > +#define XY(x, y) (((x) << 16) | (y))
> > +
> > +#define tcon_read(base, member) \
> > + readl_relaxed(base + offsetof(struct tcon, member))
> > +#define tcon_write(base, member, data) \
> > + writel_relaxed(data, base + offsetof(struct tcon, member))
> > +
> > +/* vertical blank functions */
> > +static void de2_atomic_flush(struct drm_crtc *crtc,
> > + struct drm_crtc_state *old_state)
> > +{
> > + struct drm_pending_vblank_event *event = crtc->state->event;
> > +
> > + if (event) {
> > + crtc->state->event = NULL;
> > + spin_lock_irq(&crtc->dev->event_lock);
> > + if (drm_crtc_vblank_get(crtc) == 0)
> > + drm_crtc_arm_vblank_event(crtc, event);
> > + else
> > + drm_crtc_send_vblank_event(crtc, event);
> > + spin_unlock_irq(&crtc->dev->event_lock);
> > + }
> > +}
> > +
> > +static irqreturn_t de2_lcd_irq(int irq, void *dev_id)
> > +{
> > + struct lcd *lcd = (struct lcd *) dev_id;
> > + u32 isr;
> > +
> > + isr = tcon_read(lcd->mmio, gint0);
> > +
> > + drm_crtc_handle_vblank(&lcd->crtc);
> > +
> > + tcon_write(lcd->mmio, gint0, isr & ~TCON_GINT0_TCON1_Vb_Int_Flag);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +int de2_enable_vblank(struct drm_device *drm, unsigned crtc)
> > +{
> > + struct priv *priv = drm->dev_private;
> > + struct lcd *lcd = priv->lcds[crtc];
> > +
> > + tcon_write(lcd->mmio, gint0,
> > + tcon_read(lcd->mmio, gint0) |
> > + TCON_GINT0_TCON1_Vb_Int_En);
>
> That's a weird indentation
>
> > + return 0;
> > +}
> > +
> > +void de2_disable_vblank(struct drm_device *drm, unsigned crtc)
> > +{
> > + struct priv *priv = drm->dev_private;
> > + struct lcd *lcd = priv->lcds[crtc];
> > +
> > + tcon_write(lcd->mmio, gint0,
> > + tcon_read(lcd->mmio, gint0) &
> > + ~TCON_GINT0_TCON1_Vb_Int_En);
> > +}
> > +
> > +/* panel functions */
>
> Panel functions? In the CRTC driver?
Yes, dumb panel.
> > +static void de2_set_frame_timings(struct lcd *lcd)
> > +{
> > + struct drm_crtc *crtc = &lcd->crtc;
> > + const struct drm_display_mode *mode = &crtc->mode;
> > + int interlace = mode->flags & DRM_MODE_FLAG_INTERLACE ? 2 : 1;
> > + int start_delay;
> > + u32 data;
> > +
> > + data = XY(mode->hdisplay - 1, mode->vdisplay / interlace - 1);
> > + tcon_write(lcd->mmio, basic0, data);
> > + tcon_write(lcd->mmio, basic1, data);
> > + tcon_write(lcd->mmio, basic2, data);
> > + tcon_write(lcd->mmio, basic3,
> > + XY(mode->htotal - 1,
> > + mode->htotal - mode->hsync_start - 1));
> > + tcon_write(lcd->mmio, basic4,
> > + XY(mode->vtotal * (3 - interlace),
> > + mode->vtotal - mode->vsync_start - 1));
> > + tcon_write(lcd->mmio, basic5,
> > + XY(mode->hsync_end - mode->hsync_start - 1,
> > + mode->vsync_end - mode->vsync_start - 1));
> > +
> > + tcon_write(lcd->mmio, ps_sync, XY(1, 1));
> > +
> > + data = TCON1_IO_POL_IO2_inv;
> > + if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> > + data |= TCON1_IO_POL_IO0_inv;
> > + if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> > + data |= TCON1_IO_POL_IO1_inv;
> > + tcon_write(lcd->mmio, io_pol, data);
> > +
> > + tcon_write(lcd->mmio, ceu_ctl,
> > + tcon_read(lcd->mmio, ceu_ctl) & ~TCON_CEU_CTL_ceu_en);
> > +
> > + data = tcon_read(lcd->mmio, tcon1_ctl);
> > + if (interlace == 2)
> > + data |= TCON1_CTL_Interlace_En;
> > + else
> > + data &= ~TCON1_CTL_Interlace_En;
> > + tcon_write(lcd->mmio, tcon1_ctl, data);
> > +
> > + tcon_write(lcd->mmio, fill_ctl, 0);
> > + tcon_write(lcd->mmio, fill_start0, mode->vtotal + 1);
> > + tcon_write(lcd->mmio, fill_end0, mode->vtotal);
> > + tcon_write(lcd->mmio, fill_data0, 0);
> > +
> > + start_delay = (mode->vtotal - mode->vdisplay) / interlace - 5;
> > + if (start_delay > 31)
> > + start_delay = 31;
> > + data = tcon_read(lcd->mmio, tcon1_ctl);
> > + data &= ~TCON1_CTL_Start_Delay_MASK;
> > + data |= start_delay << TCON1_CTL_Start_Delay_SHIFT;
> > + tcon_write(lcd->mmio, tcon1_ctl, data);
> > +
> > + tcon_write(lcd->mmio, io_tri, 0x0fffffff);
> > +}
>
> Some comments here would be nice, there's a lot of non trivial things.
... and no documentation. I just set the values I saw in the I/O memory
when running the legacy driver.
> > +
> > +static void de2_crtc_enable(struct drm_crtc *crtc)
> > +{
> > + struct lcd *lcd = crtc_to_lcd(crtc);
> > + struct drm_display_mode *mode = &crtc->mode;
> > +
> > + DRM_DEBUG_DRIVER("\n");
>
> Log something useful, or don't.
This was useful when the driver was crashing only God knew where.
> > +
> > + clk_set_rate(lcd->clk, mode->clock * 1000);
> > +
> > + de2_set_frame_timings(lcd);
> > +
> > + tcon_write(lcd->mmio, tcon1_ctl,
> > + tcon_read(lcd->mmio, tcon1_ctl) | TCON1_CTL_TCON_En);
> > +
> > + de2_de_panel_init(lcd->priv, lcd->num, mode);
>
> panel_init in the CRTC enable? Shouldn't that be in the panel driver?
> or at least the encoder?
I will change to 'dumb panel'.
> > +
> > + drm_mode_debug_printmodeline(mode);
>
> This is already printed by the core.
Not at this time.
[snip]
> > +static int de2_lcd_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct device_node *np = dev->of_node, *tmp, *parent, *port;
> > + struct lcd *lcd;
> > + struct resource *res;
> > + int id, irq, ret;
> > +
> > + id = of_alias_get_id(np, "lcd");
> > + if (id < 0) {
> > + dev_err(dev, "no alias for lcd\n");
> > + id = 0;
> > + }
> > + lcd = devm_kzalloc(dev, sizeof *lcd, GFP_KERNEL);
> > + if (!lcd) {
> > + dev_err(dev, "failed to allocate private data\n");
> > + return -ENOMEM;
> > + }
> > + dev_set_drvdata(dev, lcd);
> > + lcd->dev = dev;
> > + lcd->num = id;
>
> What do you need this number for?
It permits access to the overlay planes in the DE2.
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res) {
> > + dev_err(dev, "failed to get memory resource\n");
> > + return -EINVAL;
> > + }
> > +
> > + lcd->mmio = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(lcd->mmio)) {
> > + dev_err(dev, "failed to map registers\n");
> > + return PTR_ERR(lcd->mmio);
> > + }
> > +
> > + snprintf(lcd->name, sizeof(lcd->name), "sunxi-lcd%d", id);
> > +
> > + /* possible CRTCs */
> > + parent = np;
> > + tmp = of_get_child_by_name(np, "ports");
> > + if (tmp)
> > + parent = tmp;
> > + port = of_get_child_by_name(parent, "port");
> > + of_node_put(tmp);
> > + if (!port) {
> > + dev_err(dev, "no port node\n");
> > + return -ENXIO;
> > + }
> > + lcd->crtc.port = port;
> > +
> > + lcd->gate = devm_clk_get(dev, "gate"); /* optional */
>
> Having some kind of error checking would still be nice.
And cancel the device creation?
> > +
> > + lcd->clk = devm_clk_get(dev, "clock");
> > + if (IS_ERR(lcd->clk)) {
> > + dev_err(dev, "video clock err %d\n", (int) PTR_ERR(lcd->clk));
> > + ret = PTR_ERR(lcd->clk);
> > + goto err;
> > + }
> > +
> > + lcd->rstc = devm_reset_control_get_optional(dev, NULL);
>
> Ditto.
>
> > +
> > + irq = irq_of_parse_and_map(np, 0);
> > + if (irq <= 0 || irq == NO_IRQ) {
> > + dev_err(dev, "unable to get irq lcd %d\n", id);
> > + ret = -EINVAL;
> > + goto err;
> > + }
>
> You can use platform_get_irq for that.
Right. Thanks.
> > +
> > + if (!IS_ERR(lcd->rstc)) {
> > + ret = reset_control_deassert(lcd->rstc);
> > + if (ret) {
> > + dev_err(dev, "reset deassert err %d\n", ret);
> > + goto err;
> > + }
> > + }
> > +
> > + if (!IS_ERR(lcd->gate)) {
> > + ret = clk_prepare_enable(lcd->gate);
> > + if (ret)
> > + goto err2;
> > + }
> > +
> > + ret = clk_prepare_enable(lcd->clk);
> > + if (ret)
> > + goto err2;
>
> Is there any reason not to do that in the enable / disable? Leaving
> clocks running while the device has no guarantee that it's going to be
> used seems like a waste of resources.
If the machine does not need video (network server, router..), it is simpler
to prevent the video driver to be loaded (DT, module black list...).
> > +
> > + de2_tcon_init(lcd);
> > +
> > + ret = devm_request_irq(dev, irq, de2_lcd_irq, 0,
> > + lcd->name, lcd);
> > + if (ret < 0) {
> > + dev_err(dev, "unable to request irq %d\n", irq);
> > + goto err2;
> > + }
> > +
> > + return component_add(dev, &de2_lcd_ops);
> > +
> > +err2:
> > + if (!IS_ERR_OR_NULL(lcd->rstc))
> > + reset_control_assert(lcd->rstc);
> > + clk_disable_unprepare(lcd->gate);
> > + clk_disable_unprepare(lcd->clk);
> > +err:
> > + of_node_put(lcd->crtc.port);
> > + return ret;
> > +}
> > +
> > +static int de2_lcd_remove(struct platform_device *pdev)
> > +{
> > + struct lcd *lcd = platform_get_drvdata(pdev);
> > +
> > + component_del(&pdev->dev, &de2_lcd_ops);
> > +
> > + if (!IS_ERR_OR_NULL(lcd->rstc))
> > + reset_control_assert(lcd->rstc);
> > + clk_disable_unprepare(lcd->gate);
> > + clk_disable_unprepare(lcd->clk);
> > + of_node_put(lcd->crtc.port);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id de2_lcd_ids[] = {
> > + { .compatible = "allwinner,sun8i-a83t-lcd", },
> > + { }
> > +};
> > +
> > +struct platform_driver de2_lcd_platform_driver = {
> > + .probe = de2_lcd_probe,
> > + .remove = de2_lcd_remove,
> > + .driver = {
> > + .name = "sunxi-de2-lcd",
> > + .of_match_table = of_match_ptr(de2_lcd_ids),
> > + },
> > +};
> > diff --git a/drivers/gpu/drm/sunxi/de2_crtc.h b/drivers/gpu/drm/sunxi/de2_crtc.h
> > new file mode 100644
> > index 0000000..efbe45d
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sunxi/de2_crtc.h
> > @@ -0,0 +1,63 @@
> > +#ifndef __DE2_CRTC_H__
> > +#define __DE2_CRTC_H__
> > +/*
> > + * Copyright (C) 2016 Jean-François Moine
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/reset.h>
> > +#include <drm/drm_plane_helper.h>
> > +
> > +struct priv;
> > +
> > +enum de2_plane2 {
> > + DE2_PRIMARY_PLANE,
> > + DE2_CURSOR_PLANE,
> > + DE2_VI_PLANE,
> > + DE2_N_PLANES,
> > +};
> > +struct lcd {
> > + void __iomem *mmio;
> > +
> > + struct device *dev;
> > + struct drm_crtc crtc;
> > + struct priv *priv; /* DRM/DE private data */
> > +
> > + short num; /* LCD number in hardware */
> > + short crtc_idx; /* CRTC index in drm */
> > +
> > + struct clk *clk;
> > + struct clk *gate;
> > + struct reset_control *rstc;
> > +
> > + char name[16];
> > +
> > + struct drm_pending_vblank_event *event;
> > +
> > + struct drm_plane planes[DE2_N_PLANES];
> > +};
> > +
> > +#define crtc_to_lcd(x) container_of(x, struct lcd, crtc)
> > +
> > +/* in de2_de.c */
> > +void de2_de_enable(struct priv *priv, int lcd_num);
> > +void de2_de_disable(struct priv *priv, int lcd_num);
> > +void de2_de_hw_init(struct priv *priv, int lcd_num);
> > +void de2_de_panel_init(struct priv *priv, int lcd_num,
> > + struct drm_display_mode *mode);
> > +void de2_de_plane_disable(struct priv *priv,
> > + int lcd_num, int plane_ix);
> > +void de2_de_plane_update(struct priv *priv,
> > + int lcd_num, int plane_ix,
> > + struct drm_plane_state *state,
> > + struct drm_plane_state *old_state);
>
> Does it need to be exported?
I don't understand the question.
> > +
> > +/* in de2_plane.c */
> > +int de2_plane_init(struct drm_device *drm, struct lcd *lcd);
> > +
> > +#endif /* __DE2_CRTC_H__ */
> > diff --git a/drivers/gpu/drm/sunxi/de2_de.c b/drivers/gpu/drm/sunxi/de2_de.c
> > new file mode 100644
> > index 0000000..0d8cb62
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sunxi/de2_de.c
> > @@ -0,0 +1,591 @@
> > +/*
> > + * ALLWINNER DRM driver - Display Engine 2
> > + *
> > + * Copyright (C) 2016 Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org>
> > + * Copyright (c) 2016 Allwinnertech Co., Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + */
> > +
> > +#include <asm/io.h>
> > +#include <drm/drm_gem_cma_helper.h>
> > +
> > +#include "de2_drm.h"
> > +#include "de2_crtc.h"
> > +
> > +static DEFINE_SPINLOCK(de_lock);
> > +
> > +#define DE_CLK_RATE_A83T 504000000 /* pll-de */
> > +#define DE_CLK_RATE_H3 432000000 /* de */
>
> This can be set in the DT.
That's what I had in first releases, but, recently, I saw a 'set
parent' code in a clock driver, because 'it is not the plan to expose'
some clocks in the DT. I am glad to move these clock rate settings
back to the DT.
> > +
> > +/* I/O map */
> > +
> > +#define DE_MOD_REG 0x0000 /* 1 bit per LCD */
> > +#define DE_GATE_REG 0x0004
> > +#define DE_RESET_REG 0x0008
> > +#define DE_DIV_REG 0x000c /* 4 bits per LCD */
> > +#define DE_SEL_REG 0x0010
> > +
> > +#define DE_MUX0_BASE 0x00100000
> > +#define DE_MUX1_BASE 0x00200000
> > +
> > +/* MUX registers (addr / MUX base) */
> > +#define DE_MUX_GLB_REGS 0x00000 /* global control */
> > +#define DE_MUX_BLD_REGS 0x01000 /* alpha blending */
> > +#define DE_MUX_CHAN_REGS 0x02000 /* VI/UI overlay channels */
> > +#define DE_MUX_CHAN_SZ 0x1000 /* size of a channel */
> > +#define DE_MUX_VSU_REGS 0x20000 /* VSU */
> > +#define DE_MUX_GSU1_REGS 0x30000 /* GSUs */
> > +#define DE_MUX_GSU2_REGS 0x40000
> > +#define DE_MUX_GSU3_REGS 0x50000
> > +#define DE_MUX_FCE_REGS 0xa0000 /* FCE */
> > +#define DE_MUX_BWS_REGS 0xa2000 /* BWS */
> > +#define DE_MUX_LTI_REGS 0xa4000 /* LTI */
> > +#define DE_MUX_PEAK_REGS 0xa6000 /* PEAK */
> > +#define DE_MUX_ASE_REGS 0xa8000 /* ASE */
> > +#define DE_MUX_FCC_REGS 0xaa000 /* FCC */
> > +#define DE_MUX_DCSC_REGS 0xb0000 /* DCSC/SMBL */
> > +
> > +/* global control */
> > +struct de_glb {
> > + u32 ctl;
> > +#define DE_MUX_GLB_CTL_rt_en BIT(0)
> > +#define DE_MUX_GLB_CTL_finish_irq_en BIT(4)
> > +#define DE_MUX_GLB_CTL_rtwb_port BIT(12)
> > + u32 status;
> > + u32 dbuff;
> > + u32 size;
> > +};
> > +
> > +/* alpha blending */
> > +struct de_bld {
> > + u32 fcolor_ctl; /* 00 */
> > + struct {
> > + u32 fcolor;
> > + u32 insize;
> > + u32 offset;
> > + u32 dum;
> > + } attr[4];
> > + u32 dum0[15]; /* (end of clear offset) */
> > + u32 route; /* 80 */
> > + u32 premultiply;
> > + u32 bkcolor;
> > + u32 output_size;
> > + u32 bld_mode[4];
> > + u32 dum1[4];
> > + u32 ck_ctl; /* b0 */
> > + u32 ck_cfg;
> > + u32 dum2[2];
> > + u32 ck_max[4]; /* c0 */
> > + u32 dum3[4];
> > + u32 ck_min[4]; /* e0 */
> > + u32 dum4[3];
> > + u32 out_ctl; /* fc */
> > +};
> > +
> > +/* VI channel */
> > +struct de_vi {
> > + struct {
> > + u32 attr;
> > +#define VI_CFG_ATTR_en BIT(0)
> > +#define VI_CFG_ATTR_fcolor_en BIT(4)
> > +#define VI_CFG_ATTR_fmt_SHIFT 8
> > +#define VI_CFG_ATTR_fmt_MASK GENMASK(12, 8)
> > +#define VI_CFG_ATTR_ui_sel BIT(15)
> > +#define VI_CFG_ATTR_top_down BIT(23)
> > + u32 size;
> > + u32 coord;
> > +#define VI_N_PLANES 3
> > + u32 pitch[VI_N_PLANES];
> > + u32 top_laddr[VI_N_PLANES];
> > + u32 bot_laddr[VI_N_PLANES];
> > + } cfg[4];
> > + u32 fcolor[4]; /* c0 */
> > + u32 top_haddr[VI_N_PLANES]; /* d0 */
> > + u32 bot_haddr[VI_N_PLANES]; /* dc */
> > + u32 ovl_size[2]; /* e8 */
> > + u32 hori[2]; /* f0 */
> > + u32 vert[2]; /* f8 */
> > +};
> > +
> > +/* UI channel */
> > +struct de_ui {
> > + struct {
> > + u32 attr;
> > +#define UI_CFG_ATTR_en BIT(0)
> > +#define UI_CFG_ATTR_alpmod_SHIFT 1
> > +#define UI_CFG_ATTR_alpmod_MASK GENMASK(2, 1)
> > +#define UI_CFG_ATTR_fcolor_en BIT(4)
> > +#define UI_CFG_ATTR_fmt_SHIFT 8
> > +#define UI_CFG_ATTR_fmt_MASK GENMASK(12, 8)
> > +#define UI_CFG_ATTR_top_down BIT(23)
> > +#define UI_CFG_ATTR_alpha_SHIFT 24
> > +#define UI_CFG_ATTR_alpha_MASK GENMASK(31, 24)
> > + u32 size;
> > + u32 coord;
> > + u32 pitch;
> > + u32 top_laddr;
> > + u32 bot_laddr;
> > + u32 fcolor;
> > + u32 dum;
> > + } cfg[4]; /* 00 */
> > + u32 top_haddr; /* 80 */
> > + u32 bot_haddr;
> > + u32 ovl_size; /* 88 */
> > +};
>
> Please use defines instead of the structures.
Juggling with data in arrays is painful when the offsets are defined by
defines, and structures are so much readable.
> > +
> > +/* coordinates and sizes */
> > +#define XY(x, y) (((y) << 16) | (x))
> > +#define WH(w, h) (((h - 1) << 16) | (w - 1))
> > +
> > +/* UI video formats */
> > +#define DE2_FORMAT_ARGB_8888 0
> > +#define DE2_FORMAT_BGRA_8888 3
> > +#define DE2_FORMAT_XRGB_8888 4
> > +#define DE2_FORMAT_RGB_888 8
> > +#define DE2_FORMAT_BGR_888 9
> > +
> > +/* VI video formats */
> > +#define DE2_FORMAT_YUV422_I_YVYU 1 /* Y-V-Y-U */
> > +#define DE2_FORMAT_YUV422_I_UYVY 2 /* U-Y-V-Y */
> > +#define DE2_FORMAT_YUV422_I_YUYV 3 /* Y-U-Y-V */
> > +#define DE2_FORMAT_YUV422_P 6 /* YYYY UU VV planar */
> > +#define DE2_FORMAT_YUV420_P 10 /* YYYY U V planar */
> > +
> > +#define glb_read(base, member) \
> > + readl_relaxed(base + offsetof(struct de_glb, member))
> > +#define glb_write(base, member, data) \
> > + writel_relaxed(data, base + offsetof(struct de_glb, member))
> > +#define bld_read(base, member) \
> > + readl_relaxed(base + offsetof(struct de_bld, member))
> > +#define bld_write(base, member, data) \
> > + writel_relaxed(data, base + offsetof(struct de_bld, member))
> > +#define ui_read(base, member) \
> > + readl_relaxed(base + offsetof(struct de_ui, member))
> > +#define ui_write(base, member, data) \
> > + writel_relaxed(data, base + offsetof(struct de_ui, member))
> > +#define vi_read(base, member) \
> > + readl_relaxed(base + offsetof(struct de_vi, member))
> > +#define vi_write(base, member, data) \
> > + writel_relaxed(data, base + offsetof(struct de_vi, member))
> > +
> > +static const struct {
> > + char chan;
> > + char layer;
> > + char pipe;
> > +} plane2layer[DE2_N_PLANES] = {
> > + [DE2_PRIMARY_PLANE] = {0, 0, 0},
> > + [DE2_CURSOR_PLANE] = {1, 0, 1},
> > + [DE2_VI_PLANE] = {0, 1, 0},
> > +};
>
> Comments?
This
primary plane is channel 0 (VI), layer 0, pipe 0
cursor plane is channel 1 (UI), layer 0, pipe 1
overlay plane is channel 0 (VI), layer 1, pipe 0
or the full explanation:
Constraints:
The VI channels can do RGB or YUV, while UI channels can do RGB
only.
The LCD 0 has 1 VI channel and 4 UI channels, while
LCD 1 has only 1 VI channel and 1 UI channel.
The cursor must go to a channel bigger than the primary channel,
otherwise it is not transparent.
First try:
Letting the primary plane (usually RGB) in the 2nd channel (UI),
as this is done in the legacy driver, asks for the cursor to go
to the next channel (UI), but this one does not exist in LCD1.
Retained layout:
So, we must use only 2 channels for the same behaviour on LCD0
(H3) and LCD1 (A83T)
The retained combination is:
- primary plane in the first channel (VI),
- cursor plane inthe 2nd channel (UI), and
- overlay plane in the 1st channel (VI).
Note that there could be 3 overlay planes (a channel has 4
layers), but I am not sure that the A83T or the H3 could
support 3 simultaneous video streams...
> > +static inline void de_write(struct priv *priv, int reg, u32 data)
> > +{
> > + writel_relaxed(data, priv->mmio + reg);
> > +}
> > +
> > +static inline u32 de_read(struct priv *priv, int reg)
> > +{
> > + return readl_relaxed(priv->mmio + reg);
> > +}
> > +
> > +static void de_lcd_select(struct priv *priv,
> > + int lcd_num,
> > + void __iomem *mux_o)
> > +{
> > + u32 data;
> > +
> > + /* select the LCD */
> > + data = de_read(priv, DE_SEL_REG);
> > + data &= ~1;
> > + de_write(priv, DE_SEL_REG, data);
> > +
> > + /* double register switch */
> > + glb_write(mux_o + DE_MUX_GLB_REGS, dbuff, 1);
> > +}
> > +
> > +void de2_de_plane_update(struct priv *priv,
> > + int lcd_num, int plane_ix,
> > + struct drm_plane_state *state,
> > + struct drm_plane_state *old_state)
> > +{
> > + struct drm_framebuffer *fb = state->fb;
> > + struct drm_gem_cma_object *gem;
> > + void __iomem *mux_o = priv->mmio;
> > + void __iomem *chan_o;
> > + u32 size = WH(state->crtc_w, state->crtc_h);
> > + u32 coord;
> > + u32 screen_size;
> > + u32 data, fcolor;
> > + u32 ui_sel, alpha_glob;
> > + int chan, layer, x, y;
> > + unsigned fmt;
> > + unsigned long flags;
> > +
> > + chan = plane2layer[plane_ix].chan;
> > + layer = plane2layer[plane_ix].layer;
> > +
> > + mux_o += (lcd_num == 0) ? DE_MUX0_BASE : DE_MUX1_BASE;
> > + chan_o = mux_o;
> > + chan_o += DE_MUX_CHAN_REGS + DE_MUX_CHAN_SZ * chan;
> > +
> > + x = state->crtc_x >= 0 ? state->crtc_x : 0;
> > + y = state->crtc_y >= 0 ? state->crtc_y : 0;
> > + coord = XY(x, y);
> > +
> > + /* handle the cursor move */
> > + if (plane_ix == DE2_CURSOR_PLANE
> > + && fb == old_state->fb) {
> > + spin_lock_irqsave(&de_lock, flags);
> > + de_lcd_select(priv, lcd_num, mux_o);
> > + if (chan == 0)
> > + vi_write(chan_o, cfg[layer].coord, coord);
> > + else
> > + ui_write(chan_o, cfg[layer].coord, coord);
> > + spin_unlock_irqrestore(&de_lock, flags);
> > + return;
> > + }
> > +
> > + gem = drm_fb_cma_get_gem_obj(fb, 0);
> > +
> > + ui_sel = alpha_glob = 0;
> > + switch (fb->pixel_format) {
> > + case DRM_FORMAT_ARGB8888:
> > + fmt = DE2_FORMAT_ARGB_8888;
> > + ui_sel = VI_CFG_ATTR_ui_sel;
> > + break;
> > + case DRM_FORMAT_BGRA8888:
> > + fmt = DE2_FORMAT_BGRA_8888;
> > + ui_sel = VI_CFG_ATTR_ui_sel;
> > + break;
> > + case DRM_FORMAT_XRGB8888:
> > + fmt = DE2_FORMAT_XRGB_8888;
> > + ui_sel = VI_CFG_ATTR_ui_sel;
> > + alpha_glob = (1 << UI_CFG_ATTR_alpmod_SHIFT) |
> > + (0xff << UI_CFG_ATTR_alpha_SHIFT);
> > + break;
> > + case DRM_FORMAT_RGB888:
> > + fmt = DE2_FORMAT_RGB_888;
> > + ui_sel = VI_CFG_ATTR_ui_sel;
> > + break;
> > + case DRM_FORMAT_BGR888:
> > + fmt = DE2_FORMAT_BGR_888;
> > + ui_sel = VI_CFG_ATTR_ui_sel;
> > + break;
> > + case DRM_FORMAT_YUYV:
> > + fmt = DE2_FORMAT_YUV422_I_YUYV;
> > + break;
> > + case DRM_FORMAT_YVYU:
> > + fmt = DE2_FORMAT_YUV422_I_YVYU;
> > + break;
> > + case DRM_FORMAT_YUV422:
> > + fmt = DE2_FORMAT_YUV422_P;
> > + break;
> > + case DRM_FORMAT_YUV420:
> > + fmt = DE2_FORMAT_YUV420_P;
> > + break;
> > + case DRM_FORMAT_UYVY:
> > + fmt = DE2_FORMAT_YUV422_I_UYVY;
> > + break;
> > + default:
> > + pr_err("format %.4s not yet treated\n",
> > + (char *) &fb->pixel_format);
> > + return;
> > + }
> > +
> > + spin_lock_irqsave(&de_lock, flags);
> > +
> > + screen_size = plane_ix == DE2_PRIMARY_PLANE ?
> > + size :
> > + glb_read(mux_o + DE_MUX_GLB_REGS, size);
> > +
> > + /* prepare the activation of alpha blending (1 bit per plane) */
> > + fcolor = bld_read(mux_o + DE_MUX_BLD_REGS, fcolor_ctl)
> > + | (0x100 << plane2layer[plane_ix].pipe);
> > +
> > + de_lcd_select(priv, lcd_num, mux_o);
> > +
> > + if (chan == 0) { /* VI channel */
> > + int i;
> > +
> > + data = VI_CFG_ATTR_en | (fmt << VI_CFG_ATTR_fmt_SHIFT) |
> > + ui_sel;
> > + vi_write(chan_o, cfg[layer].attr, data);
> > + vi_write(chan_o, cfg[layer].size, size);
> > + vi_write(chan_o, cfg[layer].coord, coord);
> > + for (i = 0; i < VI_N_PLANES; i++) {
> > + vi_write(chan_o, cfg[layer].pitch[i],
> > + fb->pitches[i] ? fb->pitches[i] :
> > + fb->pitches[0]);
> > + vi_write(chan_o, cfg[layer].top_laddr[i],
> > + gem->paddr + fb->offsets[i]);
> > + vi_write(chan_o, fcolor[layer], 0xff000000);
> > + }
> > + if (layer == 0)
> > + vi_write(chan_o, ovl_size[0], screen_size);
> > +
> > + } else { /* UI channel */
> > + data = UI_CFG_ATTR_en | (fmt << UI_CFG_ATTR_fmt_SHIFT) |
> > + alpha_glob;
> > + ui_write(chan_o, cfg[layer].attr, data);
> > + ui_write(chan_o, cfg[layer].size, size);
> > + ui_write(chan_o, cfg[layer].coord, coord);
> > + ui_write(chan_o, cfg[layer].pitch, fb->pitches[0]);
> > + ui_write(chan_o, cfg[layer].top_laddr,
> > + gem->paddr + fb->offsets[0]);
> > + if (layer == 0)
> > + ui_write(chan_o, ovl_size, screen_size);
> > + }
> > + bld_write(mux_o + DE_MUX_BLD_REGS, fcolor_ctl, fcolor);
> > +
> > + spin_unlock_irqrestore(&de_lock, flags);
> > +}
>
> Splitting that into functions would make it a bit more trivial and
> readable.
Not sure: there is a lot of common data and different I/O accesses.
> > +void de2_de_plane_disable(struct priv *priv,
> > + int lcd_num, int plane_ix)
> > +{
> > + void __iomem *mux_o = priv->mmio;
> > + void __iomem *chan_o;
> > + u32 fcolor;
> > + int chan, layer, chan_disable = 0;
> > + unsigned long flags;
> > +
> > + chan = plane2layer[plane_ix].chan;
> > + layer = plane2layer[plane_ix].layer;
> > +
> > + mux_o += (lcd_num == 0) ? DE_MUX0_BASE : DE_MUX1_BASE;
> > + chan_o = mux_o;
> > + chan_o += DE_MUX_CHAN_REGS + DE_MUX_CHAN_SZ * chan;
> > +
> > + /* (only 2 layers) */
> > + if (chan == 0) {
> > + if (vi_read(chan_o, cfg[1 - layer].attr) == 0)
> > + chan_disable = 1;
> > + } else {
> > + if (ui_read(chan_o, cfg[1 - layer].attr) == 0)
> > + chan_disable = 1;
> > + }
> > +
> > + spin_lock_irqsave(&de_lock, flags);
> > +
> > + fcolor = bld_read(mux_o + DE_MUX_BLD_REGS, fcolor_ctl);
> > +
> > + de_lcd_select(priv, lcd_num, mux_o);
> > +
> > + if (chan == 0)
> > + vi_write(chan_o, cfg[layer].attr, 0);
> > + else
> > + ui_write(chan_o, cfg[layer].attr, 0);
> > +
> > + if (chan_disable)
> > + bld_write(mux_o + DE_MUX_BLD_REGS, fcolor_ctl,
> > + fcolor & ~(0x100 << plane2layer[plane_ix].pipe));
> > +
> > + spin_unlock_irqrestore(&de_lock, flags);
> > +}
>
> Can't you just disable it?
Which 'it'? A layer must be disabled and it is not useful to let the
DE2 processor to scan a pipe (channel) without any layer.
> > +void de2_de_panel_init(struct priv *priv, int lcd_num,
> > + struct drm_display_mode *mode)
> > +{
> > + void __iomem *mux_o = priv->mmio;
> > + u32 size = WH(mode->hdisplay, mode->vdisplay);
> > + unsigned i;
> > + unsigned long flags;
> > +
> > + mux_o += (lcd_num == 0) ? DE_MUX0_BASE : DE_MUX1_BASE;
> > +
> > + DRM_DEBUG_DRIVER("%dx%d\n", mode->hdisplay, mode->vdisplay);
> > +
> > + spin_lock_irqsave(&de_lock, flags);
> > +
> > + de_lcd_select(priv, lcd_num, mux_o);
> > +
> > + glb_write(mux_o + DE_MUX_GLB_REGS, size, size);
> > +
> > + /* set alpha blending */
> > + for (i = 0; i < 4; i++) {
> > + bld_write(mux_o + DE_MUX_BLD_REGS, attr[i].fcolor, 0xff000000);
> > + bld_write(mux_o + DE_MUX_BLD_REGS, attr[i].insize, size);
> > + }
> > + bld_write(mux_o + DE_MUX_BLD_REGS, output_size, size);
> > + bld_write(mux_o + DE_MUX_BLD_REGS, out_ctl,
> > + mode->flags & DRM_MODE_FLAG_INTERLACE ? 2 : 0);
> > +
> > + spin_unlock_irqrestore(&de_lock, flags);
> > +}
> > +
> > +void de2_de_enable(struct priv *priv, int lcd_num)
> > +{
[snip]
> > +}
> > +
> > +void de2_de_disable(struct priv *priv, int lcd_num)
> > +{
> > + u32 data;
> > +
> > + data = ~(1 << lcd_num);
> > + de_write(priv, DE_MOD_REG,
> > + de_read(priv, DE_MOD_REG) & data);
> > + de_write(priv, DE_GATE_REG,
> > + de_read(priv, DE_GATE_REG) & data);
> > + de_write(priv, DE_RESET_REG,
> > + de_read(priv, DE_RESET_REG) & data);
> > +}
> > +
> > +int de2_de_init(struct priv *priv, struct device *dev)
> > +{
> > + struct resource *res;
> > + int ret;
> > +
> > + DRM_DEBUG_DRIVER("\n");
> > +
> > + res = platform_get_resource(to_platform_device(dev),
> > + IORESOURCE_MEM, 0);
> > + if (!res) {
> > + dev_err(dev, "failed to get memory resource\n");
> > + return -EINVAL;
> > + }
> > +
> > + priv->mmio = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(priv->mmio)) {
> > + dev_err(dev, "failed to map registers\n");
> > + return PTR_ERR(priv->mmio);
> > + }
> > +
> > + priv->gate = devm_clk_get(dev, "gate"); /* optional */
>
> Error checking
>
> > +
> > + priv->clk = devm_clk_get(dev, "clock");
> > + if (IS_ERR(priv->clk)) {
> > + dev_err(dev, "video clock err %d\n", (int) PTR_ERR(priv->clk));
> > + return PTR_ERR(priv->clk);
> > + }
> > +
> > + priv->rstc = devm_reset_control_get_optional(dev, NULL);
> > +
> > + if (!IS_ERR(priv->rstc)) {
> > + ret = reset_control_deassert(priv->rstc);
> > + if (ret) {
> > + dev_err(dev, "reset deassert err %d\n", ret);
> > + return ret;
> > + }
> > + }
> > +
> > + if (!IS_ERR(priv->gate)) {
> > + ret = clk_prepare_enable(priv->gate);
> > + if (ret)
> > + goto err_gate;
> > + }
> > +
> > + ret = clk_prepare_enable(priv->clk);
> > + if (ret)
> > + goto err_enable;
> > + if (priv->soc_type == SOC_A83T)
> > + clk_set_rate(priv->clk, DE_CLK_RATE_A83T);
> > + else
> > + clk_set_rate(priv->clk, DE_CLK_RATE_H3);
> > +
> > + /* set the A83T clock divider = 500 / 250 */
> > + if (priv->soc_type == SOC_A83T)
> > + de_write(priv, DE_DIV_REG,
> > + 0x00000011); /* div = 2 for both LCDs */
> > +
> > + return 0;
> > +
> > +err_enable:
> > + clk_disable_unprepare(priv->gate);
> > +err_gate:
> > + if (!IS_ERR(priv->rstc))
> > + reset_control_assert(priv->rstc);
> > + return ret;
> > +}
> > +
> > +void de2_de_cleanup(struct priv *priv)
> > +{
> > + clk_disable_unprepare(priv->clk);
> > + clk_disable_unprepare(priv->gate);
> > + if (!IS_ERR(priv->rstc))
> > + reset_control_assert(priv->rstc);
> > +}
[snip]
> > diff --git a/drivers/gpu/drm/sunxi/de2_drv.c b/drivers/gpu/drm/sunxi/de2_drv.c
> > new file mode 100644
> > index 0000000..5daa15c
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sunxi/de2_drv.c
[snip]
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +/*
> > + * Power management
> > + */
> > +static int de2_pm_suspend(struct device *dev)
> > +{
> > + struct drm_device *drm = dev_get_drvdata(dev);
> > +
> > + drm_kms_helper_poll_disable(drm);
> > + return 0;
> > +}
> > +
> > +static int de2_pm_resume(struct device *dev)
> > +{
> > + struct drm_device *drm = dev_get_drvdata(dev);
> > +
> > + drm_kms_helper_poll_enable(drm);
> > + return 0;
> > +}
> > +#endif
> > +
> > +static const struct dev_pm_ops de2_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(de2_pm_suspend, de2_pm_resume)
> > +};
>
> Why do you need that? How did you test it? There's no runtime_pm calls
> in your kernel.
That's not tested. I will remove this.
[snip]
> > +static struct platform_driver de2_drm_platform_driver = {
> > + .probe = de2_drm_probe,
> > + .remove = de2_drm_remove,
> > + .driver = {
> > + .name = DRIVER_NAME,
> > + .pm = &de2_pm_ops,
> > + .of_match_table = de2_drm_of_match,
> > + },
> > +};
> > +
> > +static int __init de2_drm_init(void)
> > +{
> > + int ret;
> > +
> > +/* uncomment to activate the drm traces at startup time */
> > +/* drm_debug = DRM_UT_CORE | DRM_UT_DRIVER | DRM_UT_KMS |
> > + DRM_UT_PRIME | DRM_UT_ATOMIC; */
>
> That's useless.
Right, but it seems that some people don't know how to debug a DRM
driver. This is only a reminder.
> > + DRM_DEBUG_DRIVER("\n");
> > +
> > + ret = platform_driver_register(&de2_lcd_platform_driver);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = platform_driver_register(&de2_drm_platform_driver);
> > + if (ret < 0)
> > + platform_driver_unregister(&de2_lcd_platform_driver);
> > +
> > + return ret;
> > +}
>
> And that really shouldn't be done that way.
May you explain?
> > +static void __exit de2_drm_fini(void)
> > +{
> > + platform_driver_unregister(&de2_lcd_platform_driver);
> > + platform_driver_unregister(&de2_drm_platform_driver);
> > +}
> > +
> > +module_init(de2_drm_init);
> > +module_exit(de2_drm_fini);
> > +
> > +MODULE_AUTHOR("Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org>");
> > +MODULE_DESCRIPTION("Allwinner DE2 DRM Driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/gpu/drm/sunxi/de2_plane.c b/drivers/gpu/drm/sunxi/de2_plane.c
> > new file mode 100644
> > index 0000000..b338684
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sunxi/de2_plane.c
> > @@ -0,0 +1,119 @@
> > +/*
> > + * Allwinner DRM driver - DE2 planes
> > + *
> > + * Copyright (C) 2016 Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + */
> > +
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_plane_helper.h>
> > +#include <drm/drm_crtc_helper.h>
> > +
> > +#include "de2_drm.h"
> > +#include "de2_crtc.h"
> > +
> > +/* plane formats */
> > +static const uint32_t ui_formats[] = {
> > + DRM_FORMAT_ARGB8888,
> > + DRM_FORMAT_BGRA8888,
> > + DRM_FORMAT_XRGB8888,
> > + DRM_FORMAT_RGB888,
> > + DRM_FORMAT_BGR888,
> > +};
> > +
> > +static const uint32_t vi_formats[] = {
> > + DRM_FORMAT_XRGB8888,
> > + DRM_FORMAT_YUYV,
> > + DRM_FORMAT_YVYU,
> > + DRM_FORMAT_YUV422,
> > + DRM_FORMAT_YUV420,
> > + DRM_FORMAT_UYVY,
> > + DRM_FORMAT_BGRA8888,
> > + DRM_FORMAT_RGB888,
> > + DRM_FORMAT_BGR888,
> > +};
[snip]
> > +static int de2_one_plane_init(struct drm_device *drm,
> > + struct drm_plane *plane,
> > + int type, int possible_crtcs,
> > + const uint32_t *formats,
> > + int nformats)
> > +{
> > + int ret;
> > +
> > + ret = drm_universal_plane_init(drm, plane, possible_crtcs,
> > + &plane_funcs,
> > + formats, nformats, type, NULL);
> > + if (ret >= 0)
> > + drm_plane_helper_add(plane, &plane_helper_funcs);
> > +
> > + return ret;
> > +}
> > +
> > +int de2_plane_init(struct drm_device *drm, struct lcd *lcd)
> > +{
> > + int ret, possible_crtcs = 1 << lcd->crtc_idx;
> > +
> > + ret = de2_one_plane_init(drm, &lcd->planes[DE2_PRIMARY_PLANE],
> > + DRM_PLANE_TYPE_PRIMARY, possible_crtcs,
> > + ui_formats, ARRAY_SIZE(ui_formats));
> > + if (ret >= 0)
> > + ret = de2_one_plane_init(drm, &lcd->planes[DE2_CURSOR_PLANE],
> > + DRM_PLANE_TYPE_CURSOR, possible_crtcs,
> > + ui_formats, ARRAY_SIZE(ui_formats));
>
> Nothing looks really special about that cursor plane. Any reasion not
> to make it an overlay?
As explained above (channel/layer/pipe plane definitions), the cursor
cannot go in a channel lower or equal to the one of the primary plane.
Then, it must be known and, so, have an explicit plane.
--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH 1/2 v6] iio: gyro: Add MPU-3050 device tree bindings
From: Linus Walleij @ 2016-10-25 14:15 UTC (permalink / raw)
To: Jonathan Cameron, linux-iio-u79uwXL29TY76Z2rM5mHXA
Cc: Linus Walleij, devicetree-u79uwXL29TY76Z2rM5mHXA, Peter Rosin
This adds device tree bindings for the MPU-3050 gyroscope. Since it
is the first set of bindings for a gyroscope, the folder for it
is also created.
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
ChangeLog v5->v6:
- No changes, just reposting.
ChangeLog v4->v5:
- No changes, just reposting.
ChangeLog v3->v4:
- No changes, just reposting. If we agree on these bindings and since
the dependent I2C bindings are merged, this patch should be merge
material now I guess.
ChangeLog v2->v3:
- Augmented device tree bindings to reflect Peter Rosins now merged
I2C gate bindings.
ChangeLog v1->v2:
- Not yet convered to use i2c-gate as I'm waiting for that
patch set to stabilize.
---
.../bindings/iio/gyroscope/invensense,mpu3050.txt | 46 ++++++++++++++++++++++
1 file changed, 46 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/gyroscope/invensense,mpu3050.txt
diff --git a/Documentation/devicetree/bindings/iio/gyroscope/invensense,mpu3050.txt b/Documentation/devicetree/bindings/iio/gyroscope/invensense,mpu3050.txt
new file mode 100644
index 000000000000..b0d3b59966bc
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/gyroscope/invensense,mpu3050.txt
@@ -0,0 +1,46 @@
+Invensense MPU-3050 Gyroscope device tree bindings
+
+Required properties:
+ - compatible : should be "invensense,mpu3050"
+ - reg : the I2C address of the sensor
+
+Optional properties:
+ - interrupt-parent : should be the phandle for the interrupt controller
+ - interrupts : interrupt mapping for the trigger interrupt from the
+ internal oscillator. The following IRQ modes are supported:
+ IRQ_TYPE_EDGE_RISING, IRQ_TYPE_EDGE_FALLING, IRQ_TYPE_LEVEL_HIGH and
+ IRQ_TYPE_LEVEL_LOW. The driver should detect and configure the hardware
+ for the desired interrupt type.
+ - vdd-supply : supply regulator for the main power voltage.
+ - vlogic-supply : supply regulator for the signal voltage.
+ - mount-matrix : see iio/mount-matrix.txt
+
+Optional subnodes:
+ - The MPU-3050 will pass through and forward the I2C signals from the
+ incoming I2C bus, alternatively drive traffic to a slave device (usually
+ an accelerometer) on its own initiative. Therefore is supports a subnode
+ i2c gate node. For details see: i2c/i2c-gate.txt
+
+Example:
+
+mpu3050@68 {
+ compatible = "invensense,mpu3050";
+ reg = <0x68>;
+ interrupt-parent = <&foo>;
+ interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
+ vdd-supply = <&bar>;
+ vlogic-supply = <&baz>;
+
+ /* External I2C interface */
+ i2c-gate {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ fnord@18 {
+ compatible = "fnord";
+ reg = <0x18>;
+ interrupt-parent = <&foo>;
+ interrupts = <13 IRQ_TYPE_EDGE_FALLING>;
+ };
+ };
+};
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2
From: Jean-Francois Moine @ 2016-10-25 14:16 UTC (permalink / raw)
To: Daniel Vetter
Cc: devicetree, alsa-devel, Liam Girdwood, Rob Herring, linux-sunxi,
Mark Brown, dri-devel, Maxime Ripard, linux-arm-kernel
In-Reply-To: <20161025064422.4jua6qmpr7zu3ijt@phenom.ffwll.local>
On Tue, 25 Oct 2016 08:44:22 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:
> > + /* start the subdevices */
> > + ret = component_bind_all(dev, drm);
> > + if (ret < 0)
> > + goto out2;
> > +
> > + ret = drm_dev_register(drm, 0);
>
> This needs to be the very last step in your driver load sequence.
> Kerneldoc explains why. Similar, but inverted for unloading:
> drm_dev_unregister is the very first thing you must call.
Thanks, and also for embedding the drm device.
--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq
From: Jerome Brunet @ 2016-10-25 14:22 UTC (permalink / raw)
To: Marc Zyngier, Linus Walleij
Cc: devicetree@vger.kernel.org, Jason Cooper, Kevin Hilman,
Will Deacon, linux-kernel@vger.kernel.org, Russell King,
linux-gpio@vger.kernel.org, Rob Herring, Catalin Marinas,
Carlo Caione, open list:ARM/Amlogic Meson..., Thomas Gleixner,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <dee26f0b-e175-f8a0-2b73-08e8e324841c@arm.com>
On Tue, 2016-10-25 at 14:38 +0100, Marc Zyngier wrote:
> On 25/10/16 14:08, Jerome Brunet wrote:
> >
> > On Tue, 2016-10-25 at 11:38 +0100, Marc Zyngier wrote:
> > >
> > > >
> > > >
> > > On 25/10/16 10:14, Linus Walleij wrote:
> > > >
> > > >
> > > > On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylib
> > > > re.c
> > > > om> wrote:
> > > >
> > > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > Isn't this usecase (also as described in the cover letter)
> > > > > > a
> > > > > > textbook
> > > > > > example of when you should be using hierarchical irqdomain?
> > > > > >
> > > > > > Please check with Marc et al on hierarchical irqdomains.
> > > > >
> > > > > Linus,
> > > > > Do you mean I should create a new hierarchical irqdomains in
> > > > > each
> > > > > of
> > > > > the two pinctrl instances we have in these SoC, these domains
> > > > > being
> > > > > stacked on the one I just added for controller in irqchip ?
> > > > >
> > > > > I did not understand this is what you meant when I asked you
> > > > > the
> > > > > question at ELCE.
> > > >
> > > > Honestly, I do not understand when and where to properly use
> > > > hierarchical irqdomain, even after Marc's talk at ELC-E.
> > >
> > > I probably didn't do that good a job explaining it then. Let's
> > > try
> > > again. You want to use hierarchical domains when you want to
> > > describe
> > > an
> > > interrupt whose path traverses multiple controllers without ever
> > > being
> > > multiplexed with other signals. As long as you have this 1:1
> > > relationship between controllers, you can use them.
> > >
> >
> > Linus, Marc,
> >
> > The calculation is question here is meant to get the appropriate
> > hwirq
> > number from a particular gpio (and deal with the gpios that can't
> > provide an irq at all).
> >
> > If I look at other gpio drivers, many are doing exactly this kind
> > of
> > calculation before calling 'irq_create_mapping' in the to_irq
> > callback.
> > For example:
> > - pinctrl/nomadik/pinctrl-abx500.c
> > - pinctrl/samsung/pinctrl-exynos5440.c
> >
> > Some can afford to create all the mappings in the probe and just
> > call
> > 'irq_find_mapping' (gpio/gpio_tegra.c) but this would not work
> > here. We
> > have only 8 upstream irqs for 130+ pins, so only 8 mappings
> > possible at
> > a time.
> >
> > My understanding is that irqdomain provide a way to map hwirq to
> > linux
> > virq (and back), not map gpio number to hwirq, right?
>
> But why are those number different? Why don't you use the same
> namespace? If gpio == hwirq, all your problems are already solved. If
> you don't find the mapping in the irqdomain, then there is no irq,
> end
> of story. What am I missing?
>
There is a few problems to guarantee that gpio == hwirq.
1. We have 2 instances of pinctrl, to guarantee that the linux gpio
number == hwirq, we would have to guarantee the order in which they are
probed. At least this my understanding
2. Inside each instance, we may have banks that can't have irq. We even
have a bank on meson8b which has a mixed state (the last pins don't
have irqs, while the first ones do). those banks/pins are still valid
gpios with gpio numbers. This introduce a shift between the gpio
numbering and the hwirq.
The point of this calculation is take the offset given to the 'to_irq'
callback, remove the gpio bank base number and add irq base number.
There is a few trick added to handled the case in 2.
In addition, to call 'irq_find_mapping', we would first have to create
the mapping, in the probe I suppose. This would call the allocate
callback of the domain, in which we can allocate only 8 interrupts.
That's why I create the mapping in the .to_irq callback.
> >
> >
> > Even if I implement an another irqdomain at the gpio level, I would
> > still have to perform this kind of calculation, one way or the
> > other.
> >
> > >
> > > >
> > > > Which is problematic since quite a few GPIO drivers now
> > > > need to use them.
> > > >
> > > > I will review his slides, in the meantime I would say: whatever
> > > > Marc ACKs is fine with me. I trust this guy 100%. So I guess I
> > > > could ask that he ACK the entire chain of patches
> > > > from GIC->specialchip->GPIO.
> >
> > Actually this discussion go me thinking about another issue we have
> > with this hardware.
> > We are looking for a way to implement support for
> > IRQ_TYPE_EDGE_BOTH
> > (needed for things like gpio-keys or mmc card detect).
> > The controller can do each edge but not both at the same time.
> > I'm thinking that implementing another irqdomain at the gpio level
> > would allow to properly check the pad level in the EOI callback
> > then
> > set the next expected edge type accordingly (using
> > 'irq_chip_set_type_parent')
> >
> > Would it be acceptable ?
>
> I really don't see what another irqdomain brings to the table. This
> is
> not a separate piece of HW, so the hwirq:irq mapping is still the
> same.
> I fail to see what the benefit is.
The separate piece of hw is the gpio itself.
The irq-controller (in irqchip) can't read the gpio state because it is
simply another device.
The domain I'm thinking about wouldn't do much, I reckon.
It would just register an irqchip which would only implement eoi.
For everything else, it would rely the parent controller.
From your explanation, I understood this is the purpose of hierarchy
domains, For each hw in the chain to handle only what it can, and rely
on its parent for the rest.
>
> >
> > It looks a few other drivers deal with IRQ_TYPE_EDGE_BOTH in a
> > similar
> > way (gpio/gpio-omap.c, gpio/gpio-dwapb.c)
>
> Being already done doesn't make it reliable. If the line goes low
> between latching the rising edge and reprogramming the trigger,
> you've
> lost at least *two* interrupts (the falling edge and the following
> rising edge).
If a 'usual' controller support IRQ_TYPE_EDGE_BOTH and the line goes
low between latching rising edge and handling the interrupt, wouldn't
you miss the falling edge anyway ? The signal is just going too fast
the HW to handle everything.
For the second rising edge, I disagree, it would not be lost, since we
would read the pad state, get a low level, and reprogram the controller
for another rising edge.
>
> Thanks,
>
> M.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH/RFT v2 09/17] regulator: fixed: Add over current event
From: Mark Brown @ 2016-10-25 14:33 UTC (permalink / raw)
To: Axel Haslam
Cc: Greg KH, Johan Hovold, robh+dt, Sekhar Nori, Alan Stern,
Kevin Hilman, Sergei Shtylyov, David Lechner, manjunath.goudar,
Alexandre Bailon, linux-usb, devicetree, linux-kernel,
linux-arm-kernel
In-Reply-To: <CAKXjFTOHcqRFtbXMXSTyUZ3P00eRMz1N3d62Y3NWVZJDGeScZA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 791 bytes --]
On Tue, Oct 25, 2016 at 02:55:48PM +0200, Axel Haslam wrote:
> To be able to use regulator to handle the overcurrent pin, i need to be able
> to somehow retrieve the over current pin state from the regulator driver.
What makes you say that, none of the existing users need this?
> As i was trying your suggestion, i remembered why i thought i should use
> mode instead of status: Status seems to be for internal regulator driver use,
> there is no regulator_get_status, function and REGULATOR_STATUS_* are defined
> in driver.h and not in consumer.h as REGULATOR_MODE_*
> Would you be ok if i allow consumers to get the status via a new
> "regulator_get_status" call?
What would they do with this information that they can't do with the
existing error notification?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* [PATCH 0/4] Add DT support for DA8xx
From: Alexandre Bailon @ 2016-10-25 14:39 UTC (permalink / raw)
To: khilman-rdvid1DuHRBWk0Htik3J/w, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
b-liu-l0cyMroinI0
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Alexandre Bailon
The purpose of this series is to add DT support to the da8xx USB OTG.
This series should apply and build without any issues but it has
some dependencies on "Add DT support for ohci-da8xx" series.
Without it, the phy init will fail and then the da8xx driver will also fail.
Alexandre Bailon (1):
ARM: dts: da850: Add the usb otg device node
Petr Kulhavy (3):
dt/bindings: Add binding for the DA8xx MUSB driver
usb: musb: core: added helper function for parsing DT
usb: musb: da8xx: Add DT support for the DA8xx driver
.../devicetree/bindings/usb/da8xx-usb.txt | 33 ++++++++++
arch/arm/boot/dts/da850-lcdk.dts | 8 +++
arch/arm/boot/dts/da850.dtsi | 15 +++++
drivers/usb/musb/da8xx.c | 76 +++++++++++++++++++---
drivers/usb/musb/musb_core.c | 19 ++++++
drivers/usb/musb/musb_core.h | 5 ++
6 files changed, 147 insertions(+), 9 deletions(-)
create mode 100644 Documentation/devicetree/bindings/usb/da8xx-usb.txt
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH 1/4] dt/bindings: Add binding for the DA8xx MUSB driver
From: Alexandre Bailon @ 2016-10-25 14:39 UTC (permalink / raw)
To: khilman-rdvid1DuHRBWk0Htik3J/w, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
b-liu-l0cyMroinI0
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Petr Kulhavy,
Alexandre Bailon
In-Reply-To: <1477406345-27192-1-git-send-email-abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
From: Petr Kulhavy <petr-Qh/3xLP0EvwAvxtiuMwx3w@public.gmane.org>
DT binding for the TI DA8xx/OMAP-L1x/AM17xx/AM18xx MUSB driver.
Signed-off-by: Petr Kulhavy <petr-Qh/3xLP0EvwAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
.../devicetree/bindings/usb/da8xx-usb.txt | 43 ++++++++++++++++++++++
1 file changed, 43 insertions(+)
create mode 100644 Documentation/devicetree/bindings/usb/da8xx-usb.txt
diff --git a/Documentation/devicetree/bindings/usb/da8xx-usb.txt b/Documentation/devicetree/bindings/usb/da8xx-usb.txt
new file mode 100644
index 0000000..5663d79
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/da8xx-usb.txt
@@ -0,0 +1,43 @@
+TI DA8xx MUSB
+~~~~~~~~~~~~~
+For DA8xx/OMAP-L1x/AM17xx/AM18xx platforms.
+
+Required properties:
+~~~~~~~~~~~~~~~~~~~~
+ - compatible : Should be set to "ti,da830-musb".
+
+ - reg: Offset and length of the USB controller register set.
+
+ - interrupts: The USB interrupt number.
+
+ - interrupt-names: Should be set to "mc".
+
+ - dr_mode: The USB operation mode. Should be one of "host", "peripheral" or "otg".
+
+ - phys: Phandle for the PHY device
+
+ - phy-names: Should be "usb-phy"
+
+Optional properties:
+~~~~~~~~~~~~~~~~~~~~
+ - vbus-supply: Phandle to a regulator providing the USB bus power.
+
+Example:
+ usb_phy: usb-phy {
+ compatible = "ti,da830-usb-phy";
+ #phy-cells = <0>;
+ status = "okay";
+ };
+ usb20: usb@1e00000 {
+ compatible = "ti,da830-musb";
+ reg = <0x00200000 0x10000>;
+ interrupts = <58>;
+ interrupt-names = "mc";
+
+ dr_mode = "host";
+ vbus-supply = <&usb_vbus>;
+ phys = <&usb_phy 0>;
+ phy-names = "usb-phy";
+
+ status = "okay";
+ };
--
2.7.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox