* Re: [PATCH 5/6] tty: serial: Add linflexuart driver for S32V234
From: Will Deacon @ 2019-08-08 17:17 UTC (permalink / raw)
To: Stefan-gabriel Mirea
Cc: corbet@lwn.net, robh+dt@kernel.org, mark.rutland@arm.com,
gregkh@linuxfoundation.org, catalin.marinas@arm.com,
shawnguo@kernel.org, Leo Li, jslaby@suse.com,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org, linux-serial@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Cosmin Stefan Stoica,
Larisa Ileana Grigore
In-Reply-To: <VI1PR0402MB28635661A4A294EC6F01095EDFD70@VI1PR0402MB2863.eurprd04.prod.outlook.com>
Hi,
On Thu, Aug 08, 2019 at 12:47:00PM +0000, Stefan-gabriel Mirea wrote:
> On 8/8/2019 11:08 AM, Will Deacon wrote:
> > On Fri, Aug 02, 2019 at 07:47:23PM +0000, Stefan-gabriel Mirea wrote:
> >> + linflex,<addr>
> >> + Use early console provided by Freescale LinFlex UART
> >> + serial driver for NXP S32V234 SoCs. A valid base
> >> + address must be provided, and the serial port must
> >> + already be setup and configured.
> >
> > Why isn't earlycon= sufficient for this?
>
> "earlycon=" is not actually supported. I will fix this in the next
> version by adding a /chosen/stdout-path to the dts. The compatible
> string provided to OF_EARLYCON_DECLARE will also be changed from
> "fsl,s32v234-linflexuart" to "fsl,s32-linflexuart" to match the one in
> the device tree nodes. I missed this after importing a rename from our
> codebase.
>
> Should I remove this addition from kernel-parameters.txt after that?
Yes, if you can use earlycon instead, then you can drop your custom option
entirely and therefore there's no need to document it either.
Will
^ permalink raw reply
* Re: [PATCH v2 4/6] irqchip/irq-pruss-intc: Add helper functions to configure internal mapping
From: David Lechner @ 2019-08-08 17:09 UTC (permalink / raw)
To: Suman Anna, Marc Zyngier, Thomas Gleixner, Jason Cooper
Cc: Rob Herring, Tony Lindgren, Andrew F. Davis, Roger Quadros,
Lokesh Vutla, Grygorii Strashko, Sekhar Nori, Murali Karicheri,
devicetree, linux-omap, linux-arm-kernel, linux-kernel
In-Reply-To: <6c17875e-496d-1277-278f-239d3a9d8ca2@ti.com>
On 8/2/19 4:26 PM, Suman Anna wrote:
> Point is different applications might use mapping differently as per
> their firmware and driver/application design and their split across one
> or more PRUs (design by contract). And we need to set this up at runtime
> when the application driver is getting run. We will have either the Soft
> UART or the Ethernet running at a time depending on the end goal desired
>
>> I have an idea that we can use multiple struct irq_domains to make
>> this work in the existing IRQ framework, but it would be helpful to
>> know more about the bigger picture first.
>
> Yeah, would be great if there is a way this can be solved without having
> to introduce additional API.
>
Here is what I came up with to use existing IRQ APIs to implement event mapping.
Basically it is the same as my previous suggestion [1], with the addition of
multiple IRQ domains.
The idea is that each external interrupt controller (or DMA controller, etc.)
that is connected to the PRUSS interrupt controller is considered an interrupt
domain. One of the objections to my previous patch was that we could only have
one IRQ descriptor per event. Now we can have one descriptor per event per
domain.
I am still proposing that we use the interrupt-cells and identical vendor
resource data structures in the PRU firmware be used to provide the mapping
information. (As a side note, I still think it is important to include EVTSEL
on AM18xx in order to fully describe the event.)
The bindings will have N = 4 cells (or N = 5 when EVTSEL is required to fully
describe the event):
Cell 0: The PRUSS event number, e.g. 0 to 64 for most PRUSSs
Cell 1: The EVTSEL value (omitted when N == 4), e.g. 0, 1 or
TI_PRUSS_INTC_EVTSEL_ANY if the event is the same for all EVTSEL
values. On AM18xx, external events will all require 0 or 1 while
system events will always be TI_PRUSS_INTC_EVTSEL_ANY.
Cell N-3: The channel that the event gets mapped to, e.g. 0 to 9
Cell N-2: The host that the channel gets mapped to, e.g. 0 to 9
Cell N-1: The interrupt domain, e.g. TI_PRUSS_INTC_DOMAIN_PRU or
TI_PRUSS_INTC_DOMAIN_MCU
The TI_PRUSS_INTC_DOMAIN_* values are just arbitrary numbers assigned to the
possible domains. For example, on AM18xx and AM33xx, there are just two domains,
the PRU domain for host 0 and host 1 and the MCU domain for host 2 thru 9.
Looking at the AM65xx manual, it looks like it would have 4 domains, the PRU
domain, the RTU PRU domain, the MCU domain and a task manager domain. (And I
suppose that domains could even be more granular if needed, e.g. we could drop
the arbitrary domain number and treat each host interrupt/event as an interrupt
domain, then there would be an IRQ descriptor per PRU INTC event per host.)
The AM18xx example I have been using will look like this in the device tree:
interrupts = <63 TI_PRUSS_INTC_EVTSEL_ANY 0 0 TI_PRUSS_INTC_DOMAIN_PRU>,
<62 TI_PRUSS_INTC_EVTSEL_ANY 2 2 TI_PRUSS_INTC_DOMAIN_MCU>;
To keep parsing simple, the PRU firmware can include vendor resources that have
essentially the same format as the device tree bindings. For example:
enum {
/* IRQ descriptor without EVTSEL */
TI_PRU_VENDOR_RESOURCE_IRQ = RSC_VENDOR_START,
/* IRQ descriptor with EVTSEL */
TI_PRU_VENDOR_RESOURCE_IRQ2,
};
struct ti_pru_vendor_resource_irq {
__le32 event;
__le32 channel;
__le32 host;
__le32 domain;
};
struct ti_pru_vendor_resource_irq2 {
__le32 event;
__le32 evt_sel;
__le32 channel;
__le32 host;
__le32 domain;
};
Then we can provide a vendor resource hook in the remoteproc driver to handle
these resources:
static int ti_pru_rproc_handle_rsc(struct rproc *rproc, u32 rsc_type, void *rsc,
int offset, int avail)
{
struct ti_pru_data *pru = rproc->priv;
struct irq_fwspec fwspec;
unsigned int virq;
switch (rsc_type) {
case TI_PRU_VENDOR_RESOURCE_IRQ:
{
struct ti_pru_vendor_resource_irq *rsc_irq = rsc;
fwspec.fwnode = pru->intc_fwnode;
fwspec.param[0] = le32_to_cpu(rsc_irq->event);
fwspec.param[1] = le32_to_cpu(rsc_irq->channel);
fwspec.param[2] = le32_to_cpu(rsc_irq->host);
fwspec.param[3] = le32_to_cpu(rsc_irq->domain);
fwspec.param_count = 4;
}
break;
case TI_PRU_VENDOR_RESOURCE_IRQ2:
{
struct ti_pru_vendor_resource_irq2 *rsc_irq2 = rsc;
fwspec.fwnode = pru->intc_fwnode;
fwspec.param[0] = le32_to_cpu(rsc_irq2->event);
fwspec.param[1] = le32_to_cpu(rsc_irq2->evt_sel);
fwspec.param[2] = le32_to_cpu(rsc_irq2->channel);
fwspec.param[3] = le32_to_cpu(rsc_irq2->host);
fwspec.param[4] = le32_to_cpu(rsc_irq2->domain);
fwspec.param_count = 5;
break;
}
default:
return RSC_IGNORED;
}
virq = irq_create_fwspec_mapping(&fwspec);
if (!virq)
return -EINVAL;
/* TODO: save virq (and other metadata) for later use */
return RSC_HANDLED;
}
static const struct rproc_ops ti_pru_rproc_ops = {
.start = ti_pru_rproc_start,
.stop = ti_pru_rproc_stop,
.kick = ti_pru_rproc_kick,
.da_to_va = ti_pru_rproc_da_to_va,
.handle_rsc = ti_pru_rproc_handle_rsc,
};
The handle_rsc callback is called for each resource when the PRU is booted.
The function irq_create_fwspec_mapping() causes the IRQ to be mapped in
hardware. From what I understand from the previous discussions, this is exactly
when we want this to happen.
This patch applies on top of "irqchip/irq-pruss-intc: Add a PRUSS irqchip driver
for PRUSS interrupts", "irqchip/irq-pruss-intc: Add support for shared and
invalid interrupts" and "irqchip/irq-pruss-intc: Implement irq_{get,set}
_irqchip_state ops" from [PATCH v2 0/6] "Add TI PRUSS Local Interrupt Controller
IRQChip driver" [2].
A working copy along with some remoteproc and rpmsg hacks can be found on my
GitHub [3].
[1]: https://lore.kernel.org/lkml/fb2bdb7b-4d4d-508f-722a-554888280145@lechnology.com/
[2]: https://lore.kernel.org/lkml/20190731224149.11153-1-s-anna@ti.com/
[3]: https://github.com/dlech/linux/commits/pruss-2019-08-08
Signed-off-by: Suman Anna <s-anna@ti.com>
Signed-off-by: Andrew F. Davis <afd@ti.com>
Signed-off-by: Roger Quadros <rogerq@ti.com>
Signed-off-by: David Lechner <david@lechnology.com>
---
drivers/irqchip/irq-pruss-intc.c | 387 +++++++++++++++++-
.../interrupt-controller/ti-pruss.h | 27 ++
2 files changed, 396 insertions(+), 18 deletions(-)
create mode 100644 include/dt-bindings/interrupt-controller/ti-pruss.h
diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c
index c1fd6c09f2f2..da4349df08c3 100644
--- a/drivers/irqchip/irq-pruss-intc.c
+++ b/drivers/irqchip/irq-pruss-intc.c
@@ -5,6 +5,8 @@
* Copyright (C) 2016-2019 Texas Instruments Incorporated - http://www.ti.com/
* Andrew F. Davis <afd@ti.com>
* Suman Anna <s-anna@ti.com>
+ *
+ * Copyright (C) 2019 David Lechner <david@lechnology.com>
*/
#include <linux/interrupt.h>
@@ -14,6 +16,14 @@
#include <linux/module.h>
#include <linux/of_device.h>
#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+
+#include <dt-bindings/interrupt-controller/ti-pruss.h>
+
+/* The number of possible interrupt domains, see TI_PRUSS_INTC_DOMAIN_* in
+ * dt-bindings/interrupt-controller/ti-pruss.h
+ */
+#define NUM_TI_PRUSS_INTC_DOMAIN 5
/*
* Number of host interrupts reaching the main MPU sub-system. Note that this
@@ -25,6 +35,12 @@
/* minimum starting host interrupt number for MPU */
#define MIN_PRU_HOST_INT 2
+/* maximum number of host interrupts */
+#define MAX_PRU_HOST_INT 10
+
+/* maximum number of interrupt channels */
+#define MAX_PRU_CHANNELS 10
+
/* maximum number of system events */
#define MAX_PRU_SYS_EVENTS 64
@@ -57,27 +73,83 @@
#define PRU_INTC_HINLR(x) (0x1100 + (x) * 4)
#define PRU_INTC_HIER 0x1500
+/* CMR register bit-field macros */
+#define CMR_EVT_MAP_MASK 0xf
+#define CMR_EVT_MAP_BITS 8
+#define CMR_EVT_PER_REG 4
+
+/* HMR register bit-field macros */
+#define HMR_CH_MAP_MASK 0xf
+#define HMR_CH_MAP_BITS 8
+#define HMR_CH_PER_REG 4
+
/* HIPIR register bit-fields */
#define INTC_HIPIR_NONE_HINT 0x80000000
+/**
+ * struct pruss_intc_hwirq_data - additional metadata associated with a PRU
+ * system event
+ * @evtsel: The event select index (AM18xx only)
+ * @channel: The PRU INTC channel that the system event should be mapped to
+ * @host: The PRU INTC host that the channel should be mapped to
+ */
+struct pruss_intc_hwirq_data {
+ u8 evtsel;
+ u8 channel;
+ u8 host;
+};
+
+/**
+ * struct pruss_intc_map_record - keeps track of actual mapping state
+ * @value: The currently mapped value (evtsel, channel or host)
+ * @ref_count: Keeps track of number of current users of this resource
+ */
+struct pruss_intc_map_record {
+ u8 value;
+ u8 ref_count;
+};
+
+/**
+ * struct pruss_intc_domain - information specific to an external IRQ domain
+ * @hwirq_data: Table of additional mapping data received from device tree
+ * or PRU firmware
+ * @domain: irq domain
+ * @intc: the interrupt controller
+ * @id: Unique domain identifier (from device tree bindings)
+ */
+struct pruss_intc_domain {
+ struct pruss_intc_hwirq_data hwirq_data[MAX_PRU_SYS_EVENTS];
+ struct irq_domain *domain;
+ struct pruss_intc *intc;
+ u32 id;
+};
+
/**
* struct pruss_intc - PRUSS interrupt controller structure
+ * @domain: External interrupt domains
+ * @evtsel: Tracks the current state of CFGCHIP3[3].PRUSSEVTSEL (AM18xx only)
+ * @event_channel: Tracks the current state of system event to channel mappings
+ * @channel_host: Tracks the current state of channel to host mappings
* @irqs: kernel irq numbers corresponding to PRUSS host interrupts
* @base: base virtual address of INTC register space
* @irqchip: irq chip for this interrupt controller
- * @domain: irq domain for this interrupt controller
* @lock: mutex to serialize access to INTC
* @shared_intr: bit-map denoting if the MPU host interrupt is shared
* @invalid_intr: bit-map denoting if host interrupt is not connected to MPU
+ * @has_evtsel: indicates that the chip has an event select mux
*/
struct pruss_intc {
+ struct pruss_intc_domain domain[NUM_ISA_INTERRUPTS];
+ struct pruss_intc_map_record evtsel;
+ struct pruss_intc_map_record event_channel[MAX_PRU_SYS_EVENTS];
+ struct pruss_intc_map_record channel_host[MAX_PRU_CHANNELS];
unsigned int irqs[MAX_NUM_HOST_IRQS];
void __iomem *base;
struct irq_chip *irqchip;
- struct irq_domain *domain;
struct mutex lock; /* PRUSS INTC lock */
u16 shared_intr;
u16 invalid_intr;
+ bool has_evtsel;
};
static inline u32 pruss_intc_read_reg(struct pruss_intc *intc, unsigned int reg)
@@ -105,6 +177,172 @@ static int pruss_intc_check_write(struct pruss_intc *intc, unsigned int reg,
return 0;
}
+/**
+ * pruss_intc_map() - configure the PRUSS INTC
+ * @domain: pru intc domain pointer
+ * @hwirq: the system event number
+ *
+ * Configures the PRUSS INTC with the provided configuration from the one
+ * parsed in the xlate function. Any existing event to channel mappings or
+ * channel to host interrupt mappings are checked to make sure there are no
+ * conflicting configuration between both the PRU cores.
+ *
+ * Returns 0 on success, or a suitable error code otherwise
+ */
+static int pruss_intc_map(struct pruss_intc_domain *domain, unsigned long hwirq)
+{
+ struct pruss_intc *intc = domain->intc;
+ struct device* dev = intc->irqchip->parent_device;
+ u32 val;
+ int idx, ret;
+ u8 evtsel, ch, host;
+
+ if (hwirq >= MAX_PRU_SYS_EVENTS)
+ return -EINVAL;
+
+ mutex_lock(&intc->lock);
+
+ evtsel = domain->hwirq_data[hwirq].evtsel;
+ ch = domain->hwirq_data[hwirq].channel;
+ host = domain->hwirq_data[hwirq].host;
+
+ if (intc->has_evtsel && intc->evtsel.ref_count > 0 &&
+ intc->evtsel.value != evtsel) {
+ dev_err(dev, "event %lu (req. evtsel %d) already assigned to evtsel %d\n",
+ hwirq, evtsel, intc->evtsel.value);
+ ret = -EBUSY;
+ goto unlock;
+ }
+
+ /* check if sysevent already assigned */
+ if (intc->event_channel[hwirq].ref_count > 0 &&
+ intc->event_channel[hwirq].value != ch) {
+ dev_err(dev, "event %lu (req. channel %d) already assigned to channel %d\n",
+ hwirq, ch, intc->event_channel[hwirq].value);
+ ret = -EBUSY;
+ goto unlock;
+ }
+
+ /* check if channel already assigned */
+ if (intc->channel_host[ch].ref_count > 0 &&
+ intc->channel_host[ch].value != host) {
+ dev_err(dev, "channel %d (req. host %d) already assigned to host %d\n",
+ ch, host, intc->channel_host[ch].value);
+ ret = -EBUSY;
+ goto unlock;
+ }
+
+ if (++intc->evtsel.ref_count == 1) {
+ intc->evtsel.value = evtsel;
+
+ /* TODO: need to implement CFGCHIP3[3].PRUSSEVTSEL */
+ }
+
+ if (++intc->event_channel[hwirq].ref_count == 1) {
+ intc->event_channel[hwirq].value = ch;
+
+ /*
+ * configure channel map registers - each register holds map
+ * info for 4 events, with each event occupying the lower nibble
+ * in a register byte address in little-endian fashion
+ */
+ idx = hwirq / CMR_EVT_PER_REG;
+
+ val = pruss_intc_read_reg(intc, PRU_INTC_CMR(idx));
+ val &= ~(CMR_EVT_MAP_MASK <<
+ ((hwirq % CMR_EVT_PER_REG) * CMR_EVT_MAP_BITS));
+ val |= ch << ((hwirq % CMR_EVT_PER_REG) * CMR_EVT_MAP_BITS);
+ pruss_intc_write_reg(intc, PRU_INTC_CMR(idx), val);
+
+ dev_dbg(dev, "SYSEV%lu -> CH%d (CMR%d 0x%08x)\n", hwirq, ch,
+ idx, pruss_intc_read_reg(intc, PRU_INTC_CMR(idx)));
+
+ /* clear and enable system event */
+ pruss_intc_write_reg(intc, PRU_INTC_SICR, hwirq);
+ pruss_intc_write_reg(intc, PRU_INTC_EISR, hwirq);
+ }
+
+ if (++intc->channel_host[ch].ref_count == 1) {
+ intc->channel_host[ch].value = host;
+
+ /*
+ * set host map registers - each register holds map info for
+ * 4 channels, with each channel occupying the lower nibble in
+ * a register byte address in little-endian fashion
+ */
+ idx = ch / HMR_CH_PER_REG;
+
+ val = pruss_intc_read_reg(intc, PRU_INTC_HMR(idx));
+ val &= ~(HMR_CH_MAP_MASK <<
+ ((ch % HMR_CH_PER_REG) * HMR_CH_MAP_BITS));
+ val |= host << ((ch % HMR_CH_PER_REG) * HMR_CH_MAP_BITS);
+ pruss_intc_write_reg(intc, PRU_INTC_HMR(idx), val);
+
+ dev_dbg(dev, "CH%d -> HOST%d (HMR%d 0x%08x)\n", ch, host, idx,
+ pruss_intc_read_reg(intc, PRU_INTC_HMR(idx)));
+
+ /* enable host interrupts */
+ pruss_intc_write_reg(intc, PRU_INTC_HIEISR, host);
+ }
+
+ dev_info(dev, "mapped system_event = %lu channel = %d host = %d domain = %u\n",
+ hwirq, ch, host, domain->id);
+
+ /* global interrupt enable */
+ pruss_intc_write_reg(intc, PRU_INTC_GER, 1);
+
+ mutex_unlock(&intc->lock);
+ return 0;
+
+unlock:
+ mutex_unlock(&intc->lock);
+ return ret;
+}
+
+/**
+ * pruss_intc_unmap() - unconfigure the PRUSS INTC
+ * @domain: pru intc domain pointer
+ * @hwirq: the system event number
+ *
+ * Undo whatever was done in pruss_intc_map() for a PRU core.
+ * Mappings are reference counted, so resources are only disabled when there
+ * are no longer any users.
+ */
+static void pruss_intc_unmap(struct pruss_intc_domain *domain, unsigned long hwirq)
+{
+ struct pruss_intc *intc = domain->intc;
+ struct device* dev = intc->irqchip->parent_device;
+ u8 ch, host;
+
+ if (hwirq >= MAX_PRU_SYS_EVENTS)
+ return;
+
+ mutex_lock(&intc->lock);
+
+ ch = intc->event_channel[hwirq].value;
+ host = intc->channel_host[ch].value;
+
+ if (--intc->channel_host[ch].ref_count == 0) {
+ /* disable host interrupts */
+ pruss_intc_write_reg(intc, PRU_INTC_HIDISR, host);
+ }
+
+ if (--intc->event_channel[hwirq].ref_count == 0) {
+ /* disable system events */
+ pruss_intc_write_reg(intc, PRU_INTC_EICR, hwirq);
+ /* clear any pending status */
+ pruss_intc_write_reg(intc, PRU_INTC_SICR, hwirq);
+ }
+
+ if (intc->has_evtsel)
+ intc->evtsel.ref_count--;
+
+ dev_info(dev, "unmapped system_event = %lu channel = %d host = %d\n",
+ hwirq, ch, host);
+
+ mutex_unlock(&intc->lock);
+}
+
static void pruss_intc_init(struct pruss_intc *intc)
{
int i;
@@ -198,10 +436,83 @@ static int pruss_intc_irq_set_irqchip_state(struct irq_data *data,
return pruss_intc_check_write(intc, PRU_INTC_SICR, data->hwirq);
}
+static int pruss_intc_irq_domain_select(struct irq_domain *d,
+ struct irq_fwspec *fwspec,
+ enum irq_domain_bus_token bus_token)
+{
+ struct pruss_intc_domain *domain = d->host_data;
+ int num_cells = domain->intc->has_evtsel ? 5 : 4;
+ u32 domain_id;
+
+ if (!fwspec || fwspec->fwnode != domain->domain->fwnode)
+ return 0;
+
+ if (bus_token != DOMAIN_BUS_ANY && bus_token != domain->domain->bus_token)
+ return 0;
+
+ if (WARN_ON_ONCE(fwspec->param_count != num_cells))
+ return 0;
+
+ domain_id = fwspec->param[fwspec->param_count - 1];
+ if (domain_id != domain->id)
+ return 0;
+
+ return 1;
+}
+
+static int
+pruss_intc_irq_domain_xlate(struct irq_domain *d, struct device_node *node,
+ const u32 *intspec, unsigned int intsize,
+ unsigned long *out_hwirq, unsigned int *out_type)
+{
+ struct pruss_intc_domain *domain = d->host_data;
+ struct pruss_intc *intc = domain->intc;
+ int num_cells = intc->has_evtsel ? 5 : 4;
+ u32 sys_event, channel, host, domain_id;
+ u32 evtsel = 0;
+
+ if (WARN_ON_ONCE(intsize != num_cells))
+ return -EINVAL;
+
+ sys_event = intspec[0];
+ if (sys_event >= MAX_PRU_SYS_EVENTS)
+ return -EINVAL;
+
+ if (intc->has_evtsel)
+ evtsel = intspec[1];
+
+ channel = intspec[intsize - 3];
+ if (channel >= MAX_PRU_CHANNELS)
+ return -EINVAL;
+
+ host = intspec[intsize - 2];
+ if (host >= MAX_PRU_HOST_INT)
+ return -EINVAL;
+
+ domain_id = intspec[intsize - 1];
+ if (domain_id != domain->id)
+ return -EINVAL;
+
+ domain->hwirq_data[sys_event].evtsel = evtsel;
+ domain->hwirq_data[sys_event].channel = channel;
+ domain->hwirq_data[sys_event].host = host;
+
+ *out_hwirq = sys_event;
+ *out_type = IRQ_TYPE_NONE;
+
+ return 0;
+}
+
static int pruss_intc_irq_domain_map(struct irq_domain *d, unsigned int virq,
irq_hw_number_t hw)
{
- struct pruss_intc *intc = d->host_data;
+ struct pruss_intc_domain *domain = d->host_data;
+ struct pruss_intc *intc = domain->intc;
+ int err;
+
+ err = pruss_intc_map(domain, hw);
+ if (err < 0)
+ return err;
irq_set_chip_data(virq, intc);
irq_set_chip_and_handler(virq, intc->irqchip, handle_level_irq);
@@ -211,12 +522,17 @@ static int pruss_intc_irq_domain_map(struct irq_domain *d, unsigned int virq,
static void pruss_intc_irq_domain_unmap(struct irq_domain *d, unsigned int virq)
{
+ struct pruss_intc_domain *domain = d->host_data;
+ unsigned long hwirq = irqd_to_hwirq(irq_get_irq_data(virq));
+
irq_set_chip_and_handler(virq, NULL, NULL);
irq_set_chip_data(virq, NULL);
+ pruss_intc_unmap(domain, hwirq);
}
static const struct irq_domain_ops pruss_intc_irq_domain_ops = {
- .xlate = irq_domain_xlate_onecell,
+ .select = pruss_intc_irq_domain_select,
+ .xlate = pruss_intc_irq_domain_xlate,
.map = pruss_intc_irq_domain_map,
.unmap = pruss_intc_irq_domain_unmap,
};
@@ -245,7 +561,8 @@ static void pruss_intc_irq_handler(struct irq_desc *desc)
hipir = pruss_intc_read_reg(intc, PRU_INTC_HIPIR(i));
while (!(hipir & INTC_HIPIR_NONE_HINT)) {
hwirq = hipir & GENMASK(9, 0);
- virq = irq_linear_revmap(intc->domain, hwirq);
+ virq = irq_linear_revmap(
+ intc->domain[TI_PRUSS_INTC_DOMAIN_MCU].domain, hwirq);
/*
* NOTE: manually ACK any system events that do not have a
@@ -272,7 +589,8 @@ static int pruss_intc_probe(struct platform_device *pdev)
struct pruss_intc *intc;
struct resource *res;
struct irq_chip *irqchip;
- int i, irq, count;
+ int i, err, irq, count;
+ u32 num_cells;
u8 temp_intr[MAX_NUM_HOST_IRQS] = { 0 };
intc = devm_kzalloc(dev, sizeof(*intc), GFP_KERNEL);
@@ -323,13 +641,22 @@ static int pruss_intc_probe(struct platform_device *pdev)
}
}
+ err = of_property_read_u32(dev->of_node, "#interrupt-cells", &num_cells);
+ if (!err && num_cells == 5)
+ intc->has_evtsel = true;
+
mutex_init(&intc->lock);
+ pm_runtime_enable(dev);
+ pm_runtime_get_sync(dev);
+
pruss_intc_init(intc);
irqchip = devm_kzalloc(dev, sizeof(*irqchip), GFP_KERNEL);
- if (!irqchip)
- return -ENOMEM;
+ if (!irqchip) {
+ err = -ENOMEM;
+ goto fail_alloc;
+ }
irqchip->irq_ack = pruss_intc_irq_ack;
irqchip->irq_mask = pruss_intc_irq_mask;
@@ -338,14 +665,24 @@ static int pruss_intc_probe(struct platform_device *pdev)
irqchip->irq_release_resources = pruss_intc_irq_relres;
irqchip->irq_get_irqchip_state = pruss_intc_irq_get_irqchip_state;
irqchip->irq_set_irqchip_state = pruss_intc_irq_set_irqchip_state;
+ irqchip->parent_device = dev;
irqchip->name = dev_name(dev);
intc->irqchip = irqchip;
- /* always 64 events */
- intc->domain = irq_domain_add_linear(dev->of_node, MAX_PRU_SYS_EVENTS,
- &pruss_intc_irq_domain_ops, intc);
- if (!intc->domain)
- return -ENOMEM;
+ for (i = 0; i < NUM_TI_PRUSS_INTC_DOMAIN; i++) {
+ intc->domain[i].intc = intc;
+ intc->domain[i].id = i;
+ /* always 64 events */
+ intc->domain[i].domain = irq_domain_add_linear(dev->of_node,
+ MAX_PRU_SYS_EVENTS, &pruss_intc_irq_domain_ops,
+ &intc->domain[i]);
+ if (!intc->domain[i].domain) {
+ while (--i >= 0)
+ irq_domain_remove(intc->domain[i].domain);
+ err = -ENOMEM;
+ goto fail_alloc;
+ }
+ }
for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
irq = platform_get_irq_byname(pdev, irq_names[i]);
@@ -356,6 +693,7 @@ static int pruss_intc_probe(struct platform_device *pdev)
dev_err(dev, "platform_get_irq_byname failed for %s : %d\n",
irq_names[i], irq);
+ err = irq;
goto fail_irq;
}
@@ -372,13 +710,20 @@ static int pruss_intc_probe(struct platform_device *pdev)
irq_set_chained_handler_and_data(intc->irqs[i], NULL,
NULL);
}
- irq_domain_remove(intc->domain);
- return irq;
+ for (i = 0; i < NUM_TI_PRUSS_INTC_DOMAIN; i++)
+ irq_domain_remove(intc->domain[i].domain);
+
+fail_alloc:
+ pm_runtime_put(dev);
+ pm_runtime_disable(dev);
+
+ return err;
}
static int pruss_intc_remove(struct platform_device *pdev)
{
struct pruss_intc *intc = platform_get_drvdata(pdev);
+ struct device *dev = &pdev->dev;
unsigned int hwirq;
int i;
@@ -388,9 +733,15 @@ static int pruss_intc_remove(struct platform_device *pdev)
NULL);
}
- for (hwirq = 0; hwirq < MAX_PRU_SYS_EVENTS; hwirq++)
- irq_dispose_mapping(irq_find_mapping(intc->domain, hwirq));
- irq_domain_remove(intc->domain);
+ for (i = 0; i < NUM_TI_PRUSS_INTC_DOMAIN; i++) {
+ for (hwirq = 0; hwirq < MAX_PRU_SYS_EVENTS; hwirq++)
+ irq_dispose_mapping(irq_find_mapping(
+ intc->domain[i].domain, hwirq));
+ irq_domain_remove(intc->domain[i].domain);
+ }
+
+ pm_runtime_put(dev);
+ pm_runtime_disable(dev);
return 0;
}
diff --git a/include/dt-bindings/interrupt-controller/ti-pruss.h b/include/dt-bindings/interrupt-controller/ti-pruss.h
new file mode 100644
index 000000000000..326a68c31bce
--- /dev/null
+++ b/include/dt-bindings/interrupt-controller/ti-pruss.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+/*
+ * This header provides constants for the Texas Instruments Programmable
+ * Realtime Unit Subsystem (PRUSS) interrupt controller.
+ */
+
+#ifndef _DT_BINDINGS_INTERRUPT_CONTROLLER_TI_PRUSS_H
+#define _DT_BINDINGS_INTERRUPT_CONTROLLER_TI_PRUSS_H
+
+/* interrupt specifier for optional cell 1 */
+
+#define TI_PRUSS_INTC_EVTSEL_ANY 0xffffffff
+
+/* interrupt specifier for cell #interrupt-cells - 1 */
+
+/* host interrupt is connected to PRU cores, e.g. host events 0 and 1 */
+#define TI_PRUSS_INTC_DOMAIN_PRU 0
+/* host interrupt is connected to MCU's interrupt controller */
+#define TI_PRUSS_INTC_DOMAIN_MCU 1
+/* host interrupt is connected to DSP's interrupt controller */
+#define TI_PRUSS_INTC_DOMAIN_DSP 2
+/* host interrupt is connected to the auxillary PRU cores */
+#define TI_PRUSS_INTC_DOMAIN_RTU_PRU 3
+/* host interrupt is connected to the task managers */
+#define TI_PRUSS_INTC_DOMAIN_TASK 4
+
+#endif /* _DT_BINDINGS_INTERRUPT_CONTROLLER_TI_PRUSS_H */
--
2.17.1
^ permalink raw reply related
* Re: [V1, 1/2] media: dt-bindings: media: i2c: Add bindings for ov8856
From: Sakari Ailus @ 2019-08-08 16:54 UTC (permalink / raw)
To: dongchun.zhu
Cc: mark.rutland, devicetree, drinkcat, srv_heupstream, shengnan.wang,
Ben Kao, tfiga, louis.kuo, sj.huang, robh+dt, linux-mediatek,
matthias.bgg, bingbu.cao, mchehab, linux-arm-kernel, linux-media
In-Reply-To: <20190808092215.5608-2-dongchun.zhu@mediatek.com>
Hi Dongchun,
(Cc'ing Ben, too.)
On Thu, Aug 08, 2019 at 05:22:14PM +0800, dongchun.zhu@mediatek.com wrote:
> From: Dongchun Zhu <dongchun.zhu@mediatek.com>
>
> Add device tree binding documentation for the ov8856 camera sensor.
>
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> ---
> .../devicetree/bindings/media/i2c/ov8856.txt | 41 ++++++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 42 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.txt
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov8856.txt b/Documentation/devicetree/bindings/media/i2c/ov8856.txt
> new file mode 100644
> index 0000000..96b10e0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.txt
> @@ -0,0 +1,41 @@
> +* Omnivision OV8856 MIPI CSI-2 sensor
> +
> +Required Properties:
> +- compatible: shall be "ovti,ov8856"
> +- clocks: reference to the xvclk input clock
> +- clock-names: shall be "xvclk"
Could you add "clock-frequency" property here, please, and specify the
upper and lower limits?
> +- avdd-supply: Analog voltage supply, 2.8 volts
> +- dovdd-supply: Digital I/O voltage supply, 1.8 volts
> +- dvdd-supply: Digital core voltage supply, 1.2 volts
> +- reset-gpios: Low active reset gpio
> +
> +The device node shall contain one 'port' child node with an
> +'endpoint' subnode for its digital output video port,
> +in accordance with the video interface bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +The endpoint optional property 'data-lanes' shall be "<0 1 3 4>".
If you don't support lane reordering, then monotonically incrementing lane
numbers from 1 onwards are recommended.
Please also make the property mandatory.
> +
> +Example:
> +&i2c7 {
> + ov8856: camera-sensor@10 {
> + compatible = "ovti,ov8856";
> + reg = <0x10>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&clk_24m_cam>;
> +
> + clocks = <&cru SCLK_TESTCLKOUT1>;
> + clock-names = "xvclk";
> +
> + avdd-supply = <&mt6358_vcama2_reg>;
> + dvdd-supply = <&mt6358_vcamd_reg>;
> + dovdd-supply = <&mt6358_vcamio_reg>;
> + reset-gpios = <&gpio2 3 GPIO_ACTIVE_LOW>;
> +
> + port {
> + wcam_out: endpoint {
> + remote-endpoint = <&mipi_in_wcam>;
> + data-lanes = <0 1 3 4>;
> + };
> + };
> + };
> +};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 783569e..7746c6b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11889,6 +11889,7 @@ L: linux-media@vger.kernel.org
> T: git git://linuxtv.org/media_tree.git
> S: Maintained
> F: drivers/media/i2c/ov8856.c
> +F: Documentation/devicetree/bindings/media/i2c/ov8856.txt
>
> OMNIVISION OV9650 SENSOR DRIVER
> M: Sakari Ailus <sakari.ailus@linux.intel.com>
--
Regards,
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply
* Re: [PATCH v2 1/4] dt-bindings: soundwire: add slave bindings
From: Srinivas Kandagatla @ 2019-08-08 16:48 UTC (permalink / raw)
To: Pierre-Louis Bossart, vkoul, broonie
Cc: bgoswami, plai, robh+dt, devicetree, lgirdwood, alsa-devel,
linux-kernel
In-Reply-To: <d346b2af-f285-4c53-b706-46a129ab7951@linux.intel.com>
On 08/08/2019 16:58, Pierre-Louis Bossart wrote:
>
>> +++ b/Documentation/devicetree/bindings/soundwire/slave.txt
>> @@ -0,0 +1,46 @@
>> +SoundWire slave device bindings.
>> +
>> +SoundWire is a 2-pin multi-drop interface with data and clock line.
>> +It facilitates development of low cost, efficient, high performance
>> systems.
>> +
>> +SoundWire slave devices:
>> +Every SoundWire controller node can contain zero or more child nodes
>> +representing slave devices on the bus. Every SoundWire slave device is
>> +uniquely determined by the enumeration address containing 5 fields:
>> +SoundWire Version, Instance ID, Manufacturer ID, Part ID and Class ID
>> +for a device. Addition to below required properties, child nodes can
>> +have device specific bindings.
>
> In case the controller supports multiple links, what's the encoding then?
> in the MIPI DisCo spec there is a linkId field in the _ADR encoding that
> helps identify which link the Slave device is connected to
> >> +
>> +Required property for SoundWire child node if it is present:
>> +- compatible: "sdwVER,MFD,PID,CID". The textual representation of
>> + SoundWire Enumeration address comprising SoundWire
>> + Version, Manufacturer ID, Part ID and Class ID,
>> + shall be in lower-case hexadecimal with leading
>> + zeroes suppressed.
>> + Version number '0x10' represents SoundWire 1.0
>> + Version number '0x11' represents SoundWire 1.1
>> + ex: "sdw10,0217,2010,0"
>> +
>> +- sdw-instance-id: Should be ('Instance ID') from SoundWire
>> + Enumeration Address. Instance ID is for the cases
>> + where multiple Devices of the same type or Class
>> + are attached to the bus.
>
> so it is actually required if you have a single Slave device? Or is it
> only required when you have more than 1 device of the same type?
>
This is mandatory for any slave device!
> FWIW in the MIPI DisCo spec we kept the instanceID as part of the _ADR,
> so it's implicitly mandatory (and ignored by the bus if there is only
> one device of the same time)
>
>> +
>> +SoundWire example for Qualcomm's SoundWire controller:
>> +
>> +soundwire@c2d0000 {
>> + compatible = "qcom,soundwire-v1.5.0"
>> + reg = <0x0c2d0000 0x2000>;
>> +
>> + spkr_left:wsa8810-left{
>> + compatible = "sdw10,0217,2010,0";
>> + sdw-instance-id = <1>;
>> + ...
>> + };
>> +
>> + spkr_right:wsa8810-right{
>> + compatible = "sdw10,0217,2010,0";
>> + sdw-instance-id = <2>;
>
> Isn't the MIPI encoding reported in the Dev_ID0..5 registers 0-based?
>
>> + ...
>> + };
>> +};
>>
>
> And now that I think of it, wouldn't it be simpler for everyone if we
> aligned on that MIPI DisCo public spec? e.g. you'd have one property
> with a 64-bit number that follows the MIPI spec. No special encoding
> necessary for device tree cases, your DT blob would use this:
Thanks for the suggestion, adding 64 device bits as compatible string
should take care of linkID too. I will give that a go!
>
> soundwire@c2d0000 {
> compatible = "qcom,soundwire-v1.5.0"
> reg = <0x0c2d0000 0x2000>;
>
> spkr_left:wsa8810-left{
> compatible = "sdw00 00 10 02 17 20 10 00"
> }
>
> spkr_right:wsa8810-right{
> compatible = "sdw0000100217201100"
> }
> }
>
> We could use parentheses if it makes people happier, but the information
> from the MIPI DisCo spec can be used as is, and provide a means for spec
> changes via reserved bits.
^ permalink raw reply
* Re: [PATCH v3 1/4] firmware: qcom_scm-64: Add atomic version of qcom_scm_call
From: Will Deacon @ 2019-08-08 16:30 UTC (permalink / raw)
To: Vivek Gautam
Cc: Bjorn Andersson,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-arm-msm, Will Deacon, open list, David Brown,
list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>,,
robh+dt, Andy Gross, Robin Murphy
In-Reply-To: <CAFp+6iHhh9749dAV4YDeE_0w1nCiftecTBedW4Rf0aiaOJsN2A@mail.gmail.com>
On Thu, Aug 08, 2019 at 05:05:21PM +0530, Vivek Gautam wrote:
> On Tue, Aug 6, 2019 at 3:58 AM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> > Would you be able to respin this patch, so that we could unblock the
> > introduction of the display nodes in the various device?
>
> Will pointed [1] to the restructuring of arm-smmu to support
> implementation specific details.
> That hasn't been posted yet, and I haven't yet been able to work on that either.
> I will be happy to respin this series with the comments addressed if
> Will is okay to pull changes to unblock sdm845 devices. :)
>
> [1] https://lore.kernel.org/patchwork/patch/1087457/
Just checked with Robin, and he's planning to post something tomorrow.
Will
^ permalink raw reply
* Re: [alsa-devel] [PATCH v2 4/4] ASoC: codecs: add wsa881x amplifier support
From: Pierre-Louis Bossart @ 2019-08-08 16:29 UTC (permalink / raw)
To: Srinivas Kandagatla, vkoul, broonie
Cc: devicetree, alsa-devel, bgoswami, plai, linux-kernel, lgirdwood,
robh+dt
In-Reply-To: <32516aae-8a43-6a74-c564-92dea8ff6e53@linaro.org>
>>> +/* 4 ports */
>>> +static struct sdw_dpn_prop wsa_sink_dpn_prop[WSA881X_MAX_SWR_PORTS] = {
>>> + {
>>> + /* DAC */
>>> + .num = 1,
>>> + .type = SDW_DPN_SIMPLE,
>>
>> IIRC we added the REDUCED type in SoundWire 1.1 to cover the PDM case
>> with channel packing (or was it grouping) used by Qualcomm. I am not
>> sure the SIMPLE type works?
> grouping I guess.
>
> This is a simplified data port as there is no DPn_OffsetCtrl2 register
> implemented.
ok, for the REDUCED type it's required to have BlockPackingMode and
OffsetCtrl2, so it does not apply here. Thanks for confirming.
^ permalink raw reply
* Re: [PATCH] arm64: dts: allwinner: a64: Drop PMU node
From: Maxime Ripard @ 2019-08-08 16:26 UTC (permalink / raw)
To: Vasily Khoruzhick
Cc: Mark Rutland, devicetree, Jared D . McNeill, Chen-Yu Tsai,
Rob Herring, Harald Geyer, Robin Murphy, arm-linux
In-Reply-To: <CA+E=qVdh3MHMsEC9XKe5-7O8fGTHFh76WLOgVf+PZPv7c4JE9w@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 2652 bytes --]
On Wed, Aug 07, 2019 at 10:36:08AM -0700, Vasily Khoruzhick wrote:
> On Wed, Aug 7, 2019 at 4:56 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Tue, Aug 06, 2019 at 07:39:26PM -0700, Vasily Khoruzhick wrote:
> > > On Tue, Aug 6, 2019 at 2:14 PM Robin Murphy <robin.murphy@arm.com> wrote:
> > > >
> > > > On 2019-08-06 9:52 pm, Vasily Khoruzhick wrote:
> > > > > On Tue, Aug 6, 2019 at 1:19 PM Harald Geyer <harald@ccbib.org> wrote:
> > > > >>
> > > > >> Vasily Khoruzhick writes:
> > > > >>> On Tue, Aug 6, 2019 at 7:35 AM Robin Murphy <robin.murphy@arm.com> wrote:
> > > > >>>>
> > > > >>>> On 06/08/2019 15:01, Vasily Khoruzhick wrote:
> > > > >>>>> Looks like PMU in A64 is broken, it generates no interrupts at all and
> > > > >>>>> as result 'perf top' shows no events.
> > > > >>>>
> > > > >>>> Does something like 'perf stat sleep 1' at least count cycles correctly?
> > > > >>>> It could well just be that the interrupt numbers are wrong...
> > > > >>>
> > > > >>> Looks like it does, at least result looks plausible:
> > > > >>
> > > > >> I'm using perf stat regularly (cache benchmarks) and it works fine.
> > > > >>
> > > > >> Unfortunately I wasn't aware that perf stat is a poor test for
> > > > >> the interrupts part of the node, when I added it. So I'm not too
> > > > >> surprised I got it wrong.
> > > > >>
> > > > >> However, it would be unfortunate if the node got removed completely,
> > > > >> because perf stat would not work anymore. Maybe we can only remove
> > > > >> the interrupts or just fix them even if the HW doesn't work?
> > > > >
> > > > > I'm not familiar with PMU driver. Is it possible to get it working
> > > > > without interrupts?
> > > >
> > > > Yup - you get a grumpy message from the driver, it will refuse sampling
> > > > events (the ones which weren't working anyway), and if you measure
> > > > anything for long enough that a counter overflows you'll get wonky
> > > > results. But for counting hardware events over relatively short periods
> > > > it'll still do the job.
> > >
> > > I tried to drop interrupts completely from the node but 'perf top' is
> > > still broken. Though now in different way: it complains "cycles: PMU
> > > Hardware doesn't support sampling/overflow-interrupts. Try 'perf
> > > stat'"
> >
> > I have no idea if that's the culprit, but what is the state of the
> > 0x09010000 register?
>
> What register is that and how do I check it?
It's in the CPUX Configuration block, and the bits are labelled as CPU
Debug Reset.
And if you have busybox, you can use devmem.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [alsa-devel] [PATCH v2 4/4] ASoC: codecs: add wsa881x amplifier support
From: Srinivas Kandagatla @ 2019-08-08 16:20 UTC (permalink / raw)
To: Pierre-Louis Bossart, vkoul, broonie
Cc: devicetree, alsa-devel, bgoswami, linux-kernel, plai, lgirdwood,
robh+dt
In-Reply-To: <3ad15652-9d6c-11e4-7cc3-0f076c6841bb@linux.intel.com>
Thanks for taking time to review,
On 08/08/2019 16:18, Pierre-Louis Bossart wrote:
>
>> +/* 4 ports */
>> +static struct sdw_dpn_prop wsa_sink_dpn_prop[WSA881X_MAX_SWR_PORTS] = {
>> + {
>> + /* DAC */
>> + .num = 1,
>> + .type = SDW_DPN_SIMPLE,
>
> IIRC we added the REDUCED type in SoundWire 1.1 to cover the PDM case
> with channel packing (or was it grouping) used by Qualcomm. I am not
> sure the SIMPLE type works?
grouping I guess.
This is a simplified data port as there is no DPn_OffsetCtrl2 register
implemented.
Having said below channel count looks incorrect, i should fix that.
>
>> + .min_ch = 1,
>> + .max_ch = 8,
>> + .simple_ch_prep_sm = true,
>> + }, {
>> + /* COMP */
>> + .num = 2,
>> + .type = SDW_DPN_SIMPLE,
>> + .min_ch = 1,
>> + .max_ch = 8,
>> + .simple_ch_prep_sm = true,
>> + }, {
>> + /* BOOST */
>> + .num = 3,
>> + .type = SDW_DPN_SIMPLE,
>> + .min_ch = 1,
>> + .max_ch = 8,
>> + .simple_ch_prep_sm = true,
>> + }, {
>> + /* VISENSE */
>> + .num = 4,
>> + .type = SDW_DPN_SIMPLE,
>> + .min_ch = 1,
>> + .max_ch = 8,
>> + .simple_ch_prep_sm = true,
>> + }
>> +};
>
>> +static int wsa881x_update_status(struct sdw_slave *slave,
>> + enum sdw_slave_status status)
>> +{
>> + struct wsa881x_priv *wsa881x = dev_get_drvdata(&slave->dev);
>> +
>> + if (status == SDW_SLAVE_ATTACHED) {
>
> there is an ambiguity here, the Slave can be attached but as device0
> (not enumerated). We should check dev_num > 0
>
Thanks for point that! will add a check!
>> + if (!wsa881x->regmap) {
>> + wsa881x->regmap = devm_regmap_init_sdw(slave,
>> + &wsa881x_regmap_config);
>> + if (IS_ERR(wsa881x->regmap)) {
>> + dev_err(&slave->dev, "regmap_init failed\n");
>> + return PTR_ERR(wsa881x->regmap);
>> + }
>> + }
>> +
>> + return snd_soc_register_component(&slave->dev,
>> + &wsa881x_component_drv,
>> + NULL, 0);
>> + } else if (status == SDW_SLAVE_UNATTACHED) {
>> + snd_soc_unregister_component(&slave->dev);
>
> the update_status() is supposed to be called based on bus events, it'd
> be very odd to register/unregister the component itself dynamically. In
> our existing Realtek-based solutions the register_component() is called
> in the probe function (and unregister_component() in remove). We do the
> inits when the Slave becomes attached but the component is already
> registered.
>
looks less intrusive! I will give that a try!
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +
>> +static int wsa881x_remove(struct sdw_slave *sdw)
>> +{
>> + return 0;
>> +}
>> +
>> +static const struct sdw_device_id wsa881x_slave_id[] = {
>> + SDW_SLAVE_ENTRY(0x0217, 0x2010, 0),
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(sdw, wsa881x_slave_id);
>> +
>> +static struct sdw_driver wsa881x_codec_driver = {
>> + .probe = wsa881x_probe,
>> + .remove = wsa881x_remove,
>
> is this needed since you do nothing in that function?
yes, it can be removed! will do that in next version.
--srini
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs
From: Stephen Boyd @ 2019-08-08 16:20 UTC (permalink / raw)
To: andy.gross, david.brown, linux-arm-msm
Cc: linux-kernel, linux-pm, bjorn.andersson, evgreen, dianders,
rnayak, ilina, lsrao, mkshah, devicetree, Mahesh Sivasubramanian
In-Reply-To: <20190808061228.16573-2-mkshah@codeaurora.org>
Quoting Maulik Shah (2019-08-07 23:12:27)
> Add device binding documentation for Qualcomm Technology Inc's (QTI)
> SoC sleep stats driver. The driver is used for displaying SoC sleep
> statistic maintained by Always On Processor or Resource Power Manager.
>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
Your SoB chain is odd. The author is Mahesh? Otherwise, use the
Co-Developed-by tag.
> ---
> .../bindings/soc/qcom/soc-sleep-stats.txt | 36 +++++++++++++++++++
> 1 file changed, 36 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt
>
> diff --git a/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt
> new file mode 100644
> index 000000000000..ee40687ded34
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt
> @@ -0,0 +1,36 @@
> +* SoC Sleep Stats
> +
> +Always On Processor/Resource Power Manager maintains statistics of the SoC
> +sleep modes involving lowering or powering down of the backbone rails - Cx
What is a 'backbone' rail?
> +and Mx and the oscillator clock, XO.
Drop the comma? XO is the oscillator clock.
> +
> +Statistics includes SoC sleep mode type, number of times low power mode were
> +entered, time of last entry, time of last exit and accumulated sleep duration.
> +SoC Sleep Stats driver provides sysfs interface to display this information.
Can this document be YAML? Then it can be validated.
> +
> +PROPERTIES
> +
> +- compatible:
> + Usage: required
> + Value type: <string>
> + Definition: Should be "qcom,rpmh-sleep-stats" or "qcom,rpm-sleep-stats".
> +
> +- reg:
> + Usage: required
> + Value type: <prop-encoded-array>
> + Definition: The base address on the Always On Processor or Resource Power
> + Manager from where the stats are read.
> +
> +EXAMPLE 1:
> +
> + rpmh_sleep_stats: soc-sleep-stats@c3f0000 {
> + compatible = "qcom,rpmh-sleep-stats";
> + reg = <0 0xc3f0000 0 0x400>;
Is this memory region in DDR? Or some specific IMEM location? I wonder
if it would be better to just have a pointer from the RPM node to this
memory region and then populate some stats if so.
> + };
> +
^ permalink raw reply
* Re: [PATCH v7 4/6] clk: mvebu: ap806: Fix clock name for the cluster
From: Stephen Boyd @ 2019-08-08 16:12 UTC (permalink / raw)
To: Mike Turquette, linux-clk, linux-kernel
Cc: Rob Herring, devicetree, Jason Cooper, Andrew Lunn,
Sebastian Hesselbarth, Gregory CLEMENT, Thomas Petazzoni,
linux-arm-kernel, Antoine Tenart, Maxime Chevallier
In-Reply-To: <20190710134346.30239-5-gregory.clement@bootlin.com>
Quoting Gregory CLEMENT (2019-07-10 06:43:44)
> Actually, the clocks exposed for the cluster are not the CPU clocks, but
> the PLL clock used as entry clock for the CPU clocks. The CPU clock will
> be managed by a driver submitting in the following patches.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> ---
Applied to clk-next
^ permalink raw reply
* Re: [PATCH v7 3/6] clk: mvebu: add CPU clock driver for Armada 7K/8K
From: Stephen Boyd @ 2019-08-08 16:12 UTC (permalink / raw)
To: Mike Turquette, linux-clk, linux-kernel
Cc: Rob Herring, devicetree, Jason Cooper, Andrew Lunn,
Sebastian Hesselbarth, Gregory CLEMENT, Thomas Petazzoni,
linux-arm-kernel, Antoine Tenart, Maxime Chevallier
In-Reply-To: <20190710134346.30239-4-gregory.clement@bootlin.com>
Quoting Gregory CLEMENT (2019-07-10 06:43:43)
> The CPU frequency is managed at the AP level for the Armada 7K/8K. The
> CPU frequency is modified by cluster: the CPUs of the same cluster have
> the same frequency.
>
> This patch adds the clock driver that will be used by CPUFreq, it is
> based on the work of Omri Itach <omrii@marvell.com>.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> ---
Applied to clk-next
^ permalink raw reply
* Re: [PATCH v7 2/6] clk: mvebu: add helper file for Armada AP and CP clocks
From: Stephen Boyd @ 2019-08-08 16:12 UTC (permalink / raw)
To: Mike Turquette, linux-clk, linux-kernel
Cc: Rob Herring, devicetree, Jason Cooper, Andrew Lunn,
Sebastian Hesselbarth, Gregory CLEMENT, Thomas Petazzoni,
linux-arm-kernel, Antoine Tenart, Maxime Chevallier
In-Reply-To: <20190710134346.30239-3-gregory.clement@bootlin.com>
Quoting Gregory CLEMENT (2019-07-10 06:43:42)
> Clock drivers for Armada AP and Armada CP use the same function to
> generate unique clock name. A third drivers is coming with the same
> need, so it's time to move this function in a common file.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> ---
Applied to clk-next
^ permalink raw reply
* Re: [PATCH v7 1/6] dt-bindings: ap806: add the cluster clock node in the syscon file
From: Stephen Boyd @ 2019-08-08 16:12 UTC (permalink / raw)
To: Mike Turquette, linux-clk, linux-kernel
Cc: Rob Herring, devicetree, Jason Cooper, Andrew Lunn,
Sebastian Hesselbarth, Gregory CLEMENT, Thomas Petazzoni,
linux-arm-kernel, Antoine Tenart, Maxime Chevallier
In-Reply-To: <20190710134346.30239-2-gregory.clement@bootlin.com>
Quoting Gregory CLEMENT (2019-07-10 06:43:41)
> Document the device tree binding for the cluster clock controllers found
> in the Armada 7K/8K SoCs.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> ---
Applied to clk-next
^ permalink raw reply
* Re: [PATCH] of/platform: Clean up a return type in of_link_property()
From: Frank Rowand @ 2019-08-08 16:09 UTC (permalink / raw)
To: Dan Carpenter, Rob Herring, Saravana Kannan
Cc: Anton Vorontsov, Colin Cross, Tony Luck, devicetree,
kernel-janitors
In-Reply-To: <20190808103207.GA30506@mwanda>
On 8/8/19 3:32 AM, Dan Carpenter wrote:
> This function is supposed to return zero on success and negative
> error codes on failure but currently it returns true on failure. The
> caller only checks for zero and non-zero so this mixup doesn't cause any
> runtime issues.
>
> Fixes: 690ff7881b26 ("of/platform: Add functional dependency link from DT bindings")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> drivers/of/platform.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 21838226d68a..86fb8ab8c012 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -625,7 +625,7 @@ static const struct supplier_bindings bindings[] = {
> { },
> };
>
> -static bool of_link_property(struct device *dev, struct device_node *con_np,
> +static int of_link_property(struct device *dev, struct device_node *con_np,
> const char *prop)
> {
> struct device_node *phandle;
>
Hi Dan,
Thanks for catching this.
Another patch was submitted to fix this just before your patch.
-Frank
^ permalink raw reply
* Re: [PATCH] of/platform: fix compilation warning of of_link_property()
From: Frank Rowand @ 2019-08-08 16:09 UTC (permalink / raw)
To: Anders Roxell, robh+dt; +Cc: devicetree, linux-kernel
In-Reply-To: <20190808141818.22724-1-anders.roxell@linaro.org>
On 8/8/19 7:18 AM, Anders Roxell wrote:
> GCC warns that a negative integer can be returned but the
> of_link_property() function should return a boolean.
>
> ../drivers/of/platform.c: In function ‘of_link_property’:
> ../drivers/of/platform.c:650:18: warning: ?: using integer constants in boolean context [-Wint-in-bool-context]
> return done ? 0 : -ENODEV;
>
> Rework so function of_link_property() return an integer instead of a boolean.
>
> Fixes: 690ff7881b26 ("of/platform: Add functional dependency link from DT bindings")
> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> ---
> drivers/of/platform.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 21838226d68a..86fb8ab8c012 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -625,7 +625,7 @@ static const struct supplier_bindings bindings[] = {
> { },
> };
>
> -static bool of_link_property(struct device *dev, struct device_node *con_np,
> +static int of_link_property(struct device *dev, struct device_node *con_np,
> const char *prop)
> {
> struct device_node *phandle;
>
Hi Anders,
Thanks for catching this.
Another patch was submitted to fix this just before your patch.
-Frank
^ permalink raw reply
* Re: [PATCH v2 1/4] dt-bindings: soundwire: add slave bindings
From: Pierre-Louis Bossart @ 2019-08-08 15:58 UTC (permalink / raw)
To: Srinivas Kandagatla, vkoul, broonie
Cc: bgoswami, plai, robh+dt, devicetree, lgirdwood, alsa-devel,
linux-kernel
In-Reply-To: <20190808144504.24823-2-srinivas.kandagatla@linaro.org>
> +++ b/Documentation/devicetree/bindings/soundwire/slave.txt
> @@ -0,0 +1,46 @@
> +SoundWire slave device bindings.
> +
> +SoundWire is a 2-pin multi-drop interface with data and clock line.
> +It facilitates development of low cost, efficient, high performance systems.
> +
> +SoundWire slave devices:
> +Every SoundWire controller node can contain zero or more child nodes
> +representing slave devices on the bus. Every SoundWire slave device is
> +uniquely determined by the enumeration address containing 5 fields:
> +SoundWire Version, Instance ID, Manufacturer ID, Part ID and Class ID
> +for a device. Addition to below required properties, child nodes can
> +have device specific bindings.
In case the controller supports multiple links, what's the encoding then?
in the MIPI DisCo spec there is a linkId field in the _ADR encoding that
helps identify which link the Slave device is connected to
> +
> +Required property for SoundWire child node if it is present:
> +- compatible: "sdwVER,MFD,PID,CID". The textual representation of
> + SoundWire Enumeration address comprising SoundWire
> + Version, Manufacturer ID, Part ID and Class ID,
> + shall be in lower-case hexadecimal with leading
> + zeroes suppressed.
> + Version number '0x10' represents SoundWire 1.0
> + Version number '0x11' represents SoundWire 1.1
> + ex: "sdw10,0217,2010,0"
> +
> +- sdw-instance-id: Should be ('Instance ID') from SoundWire
> + Enumeration Address. Instance ID is for the cases
> + where multiple Devices of the same type or Class
> + are attached to the bus.
so it is actually required if you have a single Slave device? Or is it
only required when you have more than 1 device of the same type?
FWIW in the MIPI DisCo spec we kept the instanceID as part of the _ADR,
so it's implicitly mandatory (and ignored by the bus if there is only
one device of the same time)
> +
> +SoundWire example for Qualcomm's SoundWire controller:
> +
> +soundwire@c2d0000 {
> + compatible = "qcom,soundwire-v1.5.0"
> + reg = <0x0c2d0000 0x2000>;
> +
> + spkr_left:wsa8810-left{
> + compatible = "sdw10,0217,2010,0";
> + sdw-instance-id = <1>;
> + ...
> + };
> +
> + spkr_right:wsa8810-right{
> + compatible = "sdw10,0217,2010,0";
> + sdw-instance-id = <2>;
Isn't the MIPI encoding reported in the Dev_ID0..5 registers 0-based?
> + ...
> + };
> +};
>
And now that I think of it, wouldn't it be simpler for everyone if we
aligned on that MIPI DisCo public spec? e.g. you'd have one property
with a 64-bit number that follows the MIPI spec. No special encoding
necessary for device tree cases, your DT blob would use this:
soundwire@c2d0000 {
compatible = "qcom,soundwire-v1.5.0"
reg = <0x0c2d0000 0x2000>;
spkr_left:wsa8810-left{
compatible = "sdw0000100217201000"
}
spkr_right:wsa8810-right{
compatible = "sdw0000100217201100"
}
}
We could use parentheses if it makes people happier, but the information
from the MIPI DisCo spec can be used as is, and provide a means for spec
changes via reserved bits.
^ permalink raw reply
* Re: [PATCH v2 08/15] net: phy: adin: make RMII fifo depth configurable
From: Andrew Lunn @ 2019-08-08 15:42 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190808123026.17382-9-alexandru.ardelean@analog.com>
On Thu, Aug 08, 2019 at 03:30:19PM +0300, Alexandru Ardelean wrote:
> The FIFO depth can be configured for the RMII mode. This change adds
> support for doing this via device-tree (or ACPI).
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v2 07/15] net: phy: adin: make RGMII internal delays configurable
From: Andrew Lunn @ 2019-08-08 15:40 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190808123026.17382-8-alexandru.ardelean@analog.com>
On Thu, Aug 08, 2019 at 03:30:18PM +0300, Alexandru Ardelean wrote:
> The internal delays for the RGMII are configurable for both RX & TX. This
> change adds support for configuring them via device-tree (or ACPI).
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v2 06/15] net: phy: adin: configure RGMII/RMII/MII modes on config
From: Andrew Lunn @ 2019-08-08 15:38 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190808123026.17382-7-alexandru.ardelean@analog.com>
On Thu, Aug 08, 2019 at 03:30:17PM +0300, Alexandru Ardelean wrote:
> The ADIN1300 chip supports RGMII, RMII & MII modes. Default (if
> unconfigured) is RGMII.
> This change adds support for configuring these modes via the device
> registers.
>
> For RGMII with internal delays (modes RGMII_ID,RGMII_TXID, RGMII_RXID),
> the default delay is 2 ns. This can be configurable and will be done in
> a subsequent change.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH 0/6] hwspinlock: allow sharing of hwspinlocks
From: Bjorn Andersson @ 2019-08-08 15:37 UTC (permalink / raw)
To: Fabien DESSENNE
Cc: Suman Anna, Ohad Ben-Cohen, Rob Herring, Mark Rutland,
Maxime Coquelin, Alexandre TORGUE, Jonathan Corbet,
linux-remoteproc@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org, linux-doc@vger.kernel.org,
Benjamin GAIGNARD
In-Reply-To: <d8d4a172-ec18-a758-d994-8e05bb6a1f48@st.com>
On Thu 08 Aug 05:52 PDT 2019, Fabien DESSENNE wrote:
>
> On 07/08/2019 6:19 PM, Suman Anna wrote:
> > Hi Fabien,
> >
> > On 8/7/19 3:39 AM, Fabien DESSENNE wrote:
> >> Hi
> >>
> >> On 06/08/2019 11:30 PM, Suman Anna wrote:
> >>> On 8/6/19 1:21 PM, Bjorn Andersson wrote:
> >>>> On Tue 06 Aug 10:38 PDT 2019, Suman Anna wrote:
> >>>>
> >>>>> Hi Fabien,
> >>>>>
> >>>>> On 8/5/19 12:46 PM, Bjorn Andersson wrote:
> >>>> I agree that we shouldn't specify this property in DT - if anything it
> >>>> should be a variant of the API.
> >>
> >> If we decide to add a 'shared' API, then, what about the generic regmap
> >> driver?
> >>
> >> In the context of above example1, this would require to update the
> >> regmap driver.
> >>
> >> But would this be acceptable for any driver using syscon/regmap?
> >>
> >>
> >> I think it is better to keep the existing API (modifying it so it always
> >> allows
> >>
> >> hwlocks sharing, so no need for bindings update) than adding another API.
> > For your usecases, you would definitely need the syscon/regmap behavior
> > to be shared right. Whether we introduce a 'shared' API or an
> > 'exclusive' API and change the current API behavior to shared, it is
> > definitely a case-by-case usage scenario for the existing drivers and
> > usage right. The main contention point is what to do with the
> > unprotected usecases like Bjorn originally pointed out.
>
> OK, I see : the hwspinlock framework does not offer any lock protection
> with the RAW/IN_ATOMIC modes.
> This is an issue if several different 'local' drivers try to get a
> shared lock in the same time.
> And this is a personal problem since I need to use shared locks in
> ...atomic mode.
>
Why can't you use HWLOCK_IRQSTATE in this mode?
> I have tried to see how it is possible to put a constraint on the
> callers, just like this is documented for the RAW mode which is:
> "Caution: If the mode is HWLOCK_RAW, that means user must protect
> the routine
> of getting hardware lock with mutex or spinlock.."
> I do not think that it is acceptable to ask several drivers to share a
> common mutex/spinlock for shared locks.
No it's not.
> But I think about another option: the driver implementing the trylock
> ops may offer such protection. This is the case if the driver returns
> "busy" if the lock is already taken, not only by the remote processor,
> but also by the local host.
>
I think it's typical for hwspinlock hardware to not be able to
distinguish between different clients within Linux, so we would need to
wrap the usage in some construct that ensures mutual exclusion in Linux
- like a spinlock...
> So what do you think about adding such a documentation note :
> "Caution : the HWLOCK_RAW / HWLOCK_IN_ATOMIC modes shall not be used
> with shared locks unless the hwspinlock driver supports local lock
> protection"
>
But having local lock protection in the hwspinlock driver would defeat
the purpose of HWLOCK_RAW.
Also this kind of warning will at best be consumed by the client driver
authors, it will not be read by the dts authors.
Regards,
Bjorn
> Optionally, we may add a "local_lock_protection" flag in the
> hwspinlock_device struct, set by the driver before it calls
> hwspin_lock_register().
> This flag can then be checked by hwspinlock core to allow/deny use of
> shared locks in the raw/atomic modes.
>
> Let me know what you think about it.
>
> BR
>
> Fabien
>
> >
> > regards
> > Suman
> >
> >>
> >>
> >>>>> If you are sharing a hwlock on the Linux side, surely your driver should
> >>>>> be aware that it is a shared lock. The tag can be set during the first
> >>>>> request API, and you look through both tags when giving out a handle.
> >>>>>
> >>>> Why would the driver need to know about it?
> >>> Just the semantics if we were to support single user vs multiple users
> >>> on Linux-side to even get a handle. Your point is that this may be moot
> >>> since we have protection anyway other than the raw cases. But we need to
> >>> be able to have the same API work across all cases.
> >>>
> >>> So far, it had mostly been that there would be one user on Linux
> >>> competing with other equivalent peer entities on different processors.
> >>> It is not common to have multiple users since these protection schemes
> >>> are usually needed only at the lowest levels of a stack, so the
> >>> exclusive handle stuff had been sufficient.
> >>>
> >>>>> Obviously, the hwspin_lock_request() API usage semantics always had the
> >>>>> implied additional need for communicating the lock id to the other peer
> >>>>> entity, so a realistic usage is most always the specific API variant. I
> >>>>> doubt this API would be of much use for the shared driver usage. This
> >>>>> also implies that the client user does not care about specifying a lock
> >>>>> in DT.
> >>>>>
> >>>> Afaict if the lock are shared then there shouldn't be a problem with
> >>>> some clients using the request API and others request_specific(). As any
> >>>> collisions would simply mean that there are more contention on the lock.
> >>>>
> >>>> With the current exclusive model that is not possible and the success of
> >>>> the request_specific will depend on probe order.
> >>>>
> >>>> But perhaps it should be explicitly prohibited to use both APIs on the
> >>>> same hwspinlock instance?
> >>> Yeah, they are meant to be complimentary usage, though I doubt we will
> >>> ever have any realistic users for the generic API if we haven't had a
> >>> usage so far. I had posted a concept of reserved locks long back [1] to
> >>> keep away certain locks from the generic requestor, but dropped it since
> >>> we did not have an actual use-case needing it.
> >>>
> >>> regards
> >>> Suman
> >>>
> >>> [1] https://lwn.net/Articles/611944/
^ permalink raw reply
* Re: [PATCH v2 2/2] spi: npcm-fiu: add NPCM FIU controller driver
From: Tomer Maimon @ 2019-08-08 15:37 UTC (permalink / raw)
To: Mark Brown
Cc: Rob Herring, Mark Rutland, Vignesh Raghavendra, bbrezillon,
Avi Fishman, Tali Perry, Patrick Venture, Nancy Yuen,
Benjamin Fair, linux-spi, devicetree, OpenBMC Maillist,
Linux Kernel Mailing List
In-Reply-To: <20190808132740.GG3795@sirena.co.uk>
[-- Attachment #1: Type: text/plain, Size: 1102 bytes --]
Hi Mark,
Thanks for the prompt reply.
On Thu, 8 Aug 2019 at 16:27, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Aug 08, 2019 at 04:14:48PM +0300, Tomer Maimon wrote:
>
> > + ctrl->mode_bits = SPI_RX_DUAL | SPI_RX_QUAD
> > + | SPI_TX_DUAL | SPI_TX_QUAD;
> > + ctrl->setup = npcm_fiu_setup;
>
> I'm not seeing where we implement dual or quad modes in the driver?
> There's some
>
>
Do you mean you do not see where it is implemented in the NPCM FIU driver?
for example in our driver we modify the access type (singe, dual or quad)
according the op->addr.buswidth
for example in the npcm_fiu_set_drd function.
regmap_update_bits(fiu->regmap, NPCM_FIU_DRD_CFG,
NPCM_FIU_DRD_CFG_ACCTYPE,
ilog2(op->addr.buswidth) <<
NPCM_FIU_DRD_ACCTYPE_SHIFT);
we also modify it in the UMA R/W functions.
> + dev_info(dev, "NPCM %s probe succeed\n", fiu->info->name);
>
> Just remove this, it makes the log more verbose but doesn't really add
> any information.
>
I will remove it.
Thanks,
Tomer
[-- Attachment #2: Type: text/html, Size: 2223 bytes --]
^ permalink raw reply
* Re: [PATCH v2 05/15] net: phy: adin: add {write,read}_mmd hooks
From: Andrew Lunn @ 2019-08-08 15:35 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190808123026.17382-6-alexandru.ardelean@analog.com>
On Thu, Aug 08, 2019 at 03:30:16PM +0300, Alexandru Ardelean wrote:
> Both ADIN1200 & ADIN1300 support Clause 45 access.
> The Extended Management Interface (EMI) registers are accessible via both
> Clause 45 (at register MDIO_MMD_VEND1) and using Clause 22.
>
> However, the Clause 22 MMD access operations differ from the implementation
> in the kernel, in the sense that it uses registers ExtRegPtr (0x10) &
> ExtRegData (0x11) to access Clause 45 & EMI registers.
It is not that they differ from what the kernel supports. Its that
they differ from what the Standard says they should use. These
registers are defined in 802.3, part of C22, and this hardware
implements the standard incorrectly.
Andrew
^ permalink raw reply
* Re: [PATCH v2 2/2] spi: npcm-fiu: add NPCM FIU controller driver
From: Boris Brezillon @ 2019-08-08 15:32 UTC (permalink / raw)
To: Tomer Maimon
Cc: broonie, robh+dt, mark.rutland, vigneshr, bbrezillon,
avifishman70, tali.perry1, venture, yuenn, benjaminfair,
linux-spi, devicetree, openbmc, linux-kernel
In-Reply-To: <20190808131448.349161-3-tmaimon77@gmail.com>
On Thu, 8 Aug 2019 16:14:48 +0300
Tomer Maimon <tmaimon77@gmail.com> wrote:
> +
> +static const struct spi_controller_mem_ops npcm_fiu_mem_ops = {
> + .exec_op = npcm_fiu_exec_op,
No npcm_supports_op()? That's suspicious, especially after looking at
the npcm_fiu_exec_op() (and the functions called from there) where the
requested ->buswidth seems to be completely ignored...
> + .dirmap_create = npcm_fiu_dirmap_create,
> + .dirmap_read = npcm_fiu_direct_read,
> + .dirmap_write = npcm_fiu_direct_write,
> +};
> +
^ permalink raw reply
* Re: [PATCH v2 04/15] net: phy: adin: add support for interrupts
From: Andrew Lunn @ 2019-08-08 15:25 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190808123026.17382-5-alexandru.ardelean@analog.com>
On Thu, Aug 08, 2019 at 03:30:15PM +0300, Alexandru Ardelean wrote:
> This change adds support for enabling PHY interrupts that can be used by
> the PHY framework to get signal for link/speed/auto-negotiation changes.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v2 03/15] net: phy: adin: hook genphy_{suspend,resume} into the driver
From: Andrew Lunn @ 2019-08-08 15:24 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190808123026.17382-4-alexandru.ardelean@analog.com>
On Thu, Aug 08, 2019 at 03:30:14PM +0300, Alexandru Ardelean wrote:
> The chip supports standard suspend/resume via BMCR reg.
> Hook these functions into the `adin` driver.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
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