Groups > linux.kernel > #1280843 > unrolled thread

[PATCH v3 00/18] * SUBJECT HERE *

Started by	Noam Camus <noamc@ezchip.com>
First post	2015-12-01 14:10 +0100
Last post	2015-12-08 14:10 +0100
Articles	12 — 4 participants

Back to article view | Back to linux.kernel

  [PATCH v3 00/18] *** SUBJECT HERE *** Noam Camus <noamc@ezchip.com> - 2015-12-01 14:10 +0100
    Re: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips Marc Zyngier <marc.zyngier@arm.com> - 2015-12-01 14:30 +0100
      RE: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips Noam Camus <noamc@ezchip.com> - 2015-12-02 16:10 +0100
        Re: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips Marc Zyngier <marc.zyngier@arm.com> - 2015-12-03 19:40 +0100
          RE: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips Noam Camus <noamc@ezchip.com> - 2015-12-07 12:20 +0100
      Re: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2015-12-11 09:00 +0100
    Re: [PATCH v3 01/18] Documentation: Add EZchip vendor to binding list Daniel Lezcano <daniel.lezcano@linaro.org> - 2015-12-04 09:10 +0100
      Re: [PATCH v3 01/18] Documentation: Add EZchip vendor to binding list Noam Camus <noamc@ezchip.com> - 2015-12-04 12:50 +0100
    Re: [PATCH v3 03/18] clocksource: Add NPS400 timers driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2015-12-04 10:20 +0100
      Re: [PATCH v3 03/18] clocksource: Add NPS400 timers driver Noam Camus <noamc@ezchip.com> - 2015-12-04 13:30 +0100
        Re: [PATCH v3 03/18] clocksource: Add NPS400 timers driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2015-12-04 14:00 +0100
      RE: [PATCH v3 03/18] clocksource: Add NPS400 timers driver Noam Camus <noamc@ezchip.com> - 2015-12-08 14:10 +0100

#1280843 — [PATCH v3 00/18] * SUBJECT HERE *

From	Noam Camus <noamc@ezchip.com>
Date	2015-12-01 14:10 +0100
Subject	[PATCH v3 00/18] * SUBJECT HERE *
Message-ID	<qASQW-1Dr-33@gated-at.bofh.it>

From: Noam Camus <noamc@ezchip.com>

v3:
1)	irqchip: use MACROS instead of structures to decribe
	registers.
2)	clocksource: use 32bit counter and avoid 2 halfs read
	of 64bit dance.

v2:
1)	Remove out of tree platform include path
2)	Move atomic/bitop/cmpxchg for platform to end.
	Remove macro duplication.
	Fix some bad implementation.
3)	define cpu_relax_lowlatency() for platform.
4)	rename init_irq_cpu() to init_per_cpu()
	reorder call to init_per_cpu() for secondary
	use it instead of init_cpu_smp().
5)	set res_service to call stext
6)	fix build failure for CTOP_AUX_BASE at assembly code
7)	Use ilog2 for mtm_init_nat()
8)	Add CLKSRC_NPS option to Kconfig
	change nps_clksrc_read() to be more readable.

General summay:
This set introduce new platform to ARC architecture.
Platform name called "eznps" for working with EZchip NPS400
Network Proccessor.
NPS400 is targeted to service "fast path" network applications.

NPS400 got mesh of 256 extended ARC cores (AKA CTOP), each core
got 16 HW threads. This is basically SMT core where at any point of
time only one HW thread is active.
Each core have HW scheduler that round robin between eligible HW
threads. Totaly, kernel sees 4096 CPUs which I belive is a high record.
There is no cache coherency between cores so generic user applications
and kernel do not use D$.

Cores got special memory mappings for huge pages (8MB).
Mapping is static and should provide application enough memory without
any "TLB miss". This mapping is on top of TLB mapping.

This is a basic set that will later be followed with additional
set of patches with all advanced features.

Many thanks to all people helping to make this happen.

Regards,
Noam Camus

Noam Camus (17):
  Documentation: Add EZchip vendor to binding list
  ARC: [plat-eznps] define IPI_IRQ
  clocksource: Add NPS400 timers driver
  irqchip: add nps Internal and external irqchips
  ARC: Set vmalloc size from configuration
  ARC: rwlock: disable interrupts in !LLSC variant
  ARC: rename smp operation init_irq_cpu() to init_per_cpu()
  ARC: Mark secondary cpu online only after all HW setup is done
  ARC: add CONFIG_CLKSRC_OF support to time_init()
  ARC: [plat-eznps] Add eznps board defconfig and dts
  ARC: [plat-eznps] Add eznps platform
  ARC: [plat-eznps] Use dedicated user stack top
  ARC: [plat-eznps] Use dedicated atomic/bitops/cmpxchg
  ARC: [plat-eznps] Use dedicated SMP barriers
  ARC: [plat-eznps] Use dedicated identity auxiliary register.
  ARC: [plat-eznps] Use dedicated COMMAND_LINE_SIZE
  ARC: Add eznps platform to Kconfig and Makefile

Tal Zilcer (1):
  ARC: [plat-eznps] Use dedicated cpu_relax()

 Documentation/devicetree/bindings/arc/eznps.txt    |    7 +
 .../interrupt-controller/ezchip,nps400-ic.txt      |   17 ++
 .../bindings/timer/ezchip,nps400-timer.txt         |   11 +
 .../devicetree/bindings/vendor-prefixes.txt        |    1 +
 MAINTAINERS                                        |    6 +
 arch/arc/Kconfig                                   |    9 +
 arch/arc/Makefile                                  |    5 +
 arch/arc/boot/dts/eznps.dts                        |   76 ++++++
 arch/arc/configs/nps_defconfig                     |   85 +++++++
 arch/arc/include/asm/atomic.h                      |   79 ++++++-
 arch/arc/include/asm/barrier.h                     |    8 +
 arch/arc/include/asm/bitops.h                      |   54 +++++
 arch/arc/include/asm/cmpxchg.h                     |   87 ++++++--
 arch/arc/include/asm/entry-compact.h               |    8 +
 arch/arc/include/asm/irq.h                         |    4 +
 arch/arc/include/asm/pgtable.h                     |    2 +-
 arch/arc/include/asm/processor.h                   |   36 +++-
 arch/arc/include/asm/setup.h                       |    4 +
 arch/arc/include/asm/smp.h                         |    4 +-
 arch/arc/include/asm/spinlock.h                    |   14 +
 arch/arc/kernel/ctx_sw.c                           |   13 +
 arch/arc/kernel/irq.c                              |    4 +-
 arch/arc/kernel/mcip.c                             |    2 +-
 arch/arc/kernel/smp.c                              |   14 +-
 arch/arc/kernel/time.c                             |    4 +
 arch/arc/mm/tlb.c                                  |   12 +
 arch/arc/plat-eznps/Kconfig                        |   34 +++
 arch/arc/plat-eznps/Makefile                       |    7 +
 arch/arc/plat-eznps/entry.S                        |   75 ++++++
 arch/arc/plat-eznps/include/plat/ctop.h            |  250 ++++++++++++++++++++
 arch/arc/plat-eznps/include/plat/mtm.h             |   60 +++++
 arch/arc/plat-eznps/include/plat/smp.h             |   26 ++
 arch/arc/plat-eznps/mtm.c                          |  133 +++++++++++
 arch/arc/plat-eznps/platform.c                     |   27 ++
 arch/arc/plat-eznps/smp.c                          |  149 ++++++++++++
 drivers/clocksource/Kconfig                        |    7 +
 drivers/clocksource/Makefile                       |    1 +
 drivers/clocksource/timer-nps.c                    |   63 +++++
 drivers/irqchip/Makefile                           |    1 +
 drivers/irqchip/irq-eznps.c                        |  213 +++++++++++++++++
 40 files changed, 1576 insertions(+), 36 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arc/eznps.txt
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ezchip,nps400-ic.txt
 create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
 create mode 100644 arch/arc/boot/dts/eznps.dts
 create mode 100644 arch/arc/configs/nps_defconfig
 create mode 100644 arch/arc/plat-eznps/Kconfig
 create mode 100644 arch/arc/plat-eznps/Makefile
 create mode 100644 arch/arc/plat-eznps/entry.S
 create mode 100644 arch/arc/plat-eznps/include/plat/ctop.h
 create mode 100644 arch/arc/plat-eznps/include/plat/mtm.h
 create mode 100644 arch/arc/plat-eznps/include/plat/smp.h
 create mode 100644 arch/arc/plat-eznps/mtm.c
 create mode 100644 arch/arc/plat-eznps/platform.c
 create mode 100644 arch/arc/plat-eznps/smp.c
 create mode 100644 drivers/clocksource/timer-nps.c
 create mode 100644 drivers/irqchip/irq-eznps.c

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1280864 — Re: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips

From	Marc Zyngier <marc.zyngier@arm.com>
Date	2015-12-01 14:30 +0100
Subject	Re: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips
Message-ID	<qATah-1KL-15@gated-at.bofh.it>
In reply to	#1280843

On 01/12/15 13:02, Noam Camus wrote:
> From: Noam Camus <noamc@ezchip.com>
> 
> Adding EZchip NPS400 support.
> NPS internal interrupts are internally handled at
> Multi Thread Manager (MTM) that is signaled for deactivating
> an interrupt.
> External interrupts is handled also at Global Interrupt
> Controller (GIC) e.g. serial and network devices.
> 
> Signed-off-by: Noam Camus <noamc@ezchip.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  .../interrupt-controller/ezchip,nps400-ic.txt      |   17 ++
>  drivers/irqchip/Makefile                           |    1 +
>  drivers/irqchip/irq-eznps.c                        |  213 ++++++++++++++++++++
>  3 files changed, 231 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ezchip,nps400-ic.txt
>  create mode 100644 drivers/irqchip/irq-eznps.c
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/ezchip,nps400-ic.txt b/Documentation/devicetree/bindings/interrupt-controller/ezchip,nps400-ic.txt
> new file mode 100644
> index 0000000..888b2b9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/ezchip,nps400-ic.txt
> @@ -0,0 +1,17 @@
> +EZchip NPS Interrupt Controller
> +
> +Required properties:
> +
> +- compatible : should be "ezchip,nps400-ic"
> +- interrupt-controller : Identifies the node as an interrupt controller
> +- #interrupt-cells : Specifies the number of cells needed to encode an
> +  interrupt source. The value shall be 1.

So you never have to encode the interrupt trigger type? Do you only
support edge or level?

> +
> +
> +Example:
> +
> +intc: interrupt-controller {
> +	compatible = "ezchip,nps400-ic";
> +	interrupt-controller;
> +	#interrupt-cells = <1>;
> +};
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 177f78f..b95b954 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -55,3 +55,4 @@ obj-$(CONFIG_RENESAS_H8S_INTC)		+= irq-renesas-h8s.o
>  obj-$(CONFIG_ARCH_SA1100)		+= irq-sa11x0.o
>  obj-$(CONFIG_INGENIC_IRQ)		+= irq-ingenic.o
>  obj-$(CONFIG_IMX_GPCV2)			+= irq-imx-gpcv2.o
> +obj-$(CONFIG_ARC_PLAT_EZNPS)		+= irq-eznps.o
> diff --git a/drivers/irqchip/irq-eznps.c b/drivers/irqchip/irq-eznps.c
> new file mode 100644
> index 0000000..bb8d547
> --- /dev/null
> +++ b/drivers/irqchip/irq-eznps.c
> @@ -0,0 +1,213 @@
> +/*
> + * Copyright(c) 2015 EZchip Technologies.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * The full GNU General Public License is included in this distribution in
> + * the file called "COPYING".
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/irqdomain.h>
> +#include <linux/irqchip.h>
> +#include <asm/irq.h>
> +#include <plat/mtm.h>
> +
> +#define NPS_MSU_EN_CFG	0x80	/* MSU Enable Configuration Register */
> +#define MSU_EN		BIT(0)  /* MSU block enable */
> +#define IPI_EN		BIT(16) /* Enable service of incoming IPI Messages */
> +#define GIM0_EN		BIT(17) /* Enable service of incoming GIM 0 messages */
> +#define GIM1_EN		BIT(18) /* Enable service of incoming GIM 1 messages */
> +
> +#define NPS_GIM_P_POL	0x110	/* Peripheral interrupts source polarity */
> +#define NPS_GIM_P_SENS	0x114	/* Peripheral interrupts sensitivity */
> +#define GIM_UART	BIT(7)
> +#define GIM_LAN_TX	(BIT(10) | BIT(25))
> +#define GIM_LAN_RX	(BIT(11) | BIT(26))
> +#define GIM_PERIPH_ALL	(GIM_UART | GIM_LAN_TX | GIM_LAN_RX)
> +
> +#define NPS_GIM_P_DST10	0x13A	/* Peripheral Interrupt Destination (LAN RX) */
> +#define NPS_GIM_P_DST11	0x13B	/* Peripheral Interrupt Destination (LAN TX) */
> +#define NPS_GIM_P_DST25	0x149	/* Peripheral Interrupt Destination (LAN RX) */
> +#define NPS_GIM_P_DST26	0x14A	/* Peripheral Interrupt Destination (LAN TX) */
> +#define DST_IS		BIT(26) /* Interrupt select for line 7 */
> +
> +#define NPS_GIM_P_EN	0x100	/* Peripheral interrupts source enable */
> +#define NPS_GIM_P_BLK	0x118	/* Peripheral interrupts blocking for sources */

Are these the interrupts the peripherals are using? If yes, they really
have nothing to do here...

> +
> +/* Messaging and Scheduling Unit:
> + * Provides message management for a CPU cluster.
> + */
> +static void __init eznps_configure_msu(void)
> +{
> +	int cpu;
> +	u32 value = MSU_EN | IPI_EN | GIM0_EN | GIM1_EN;
> +
> +	/* Enable IPI and GIM messages on all clusters */
> +	for (cpu = 0 ; cpu < eznps_max_cpus; cpu += eznps_cpus_per_cluster)
> +		iowrite32be(value,
> +			    nps_host_reg(cpu, NPS_MSU_BLKID, NPS_MSU_EN_CFG));
> +}
> +
> +/* Global Interrupt Manager:
> + * Configures and manages up to 64 interrupts from peripherals,
> + * 16 interrupts from CPUs (virtual interrupts) and ECC interrupts.
> + * Receives the interrupts and transmits them to relevant CPU.
> + */
> +static void __init eznps_configure_gim(void)
> +{
> +	u32 reg_addr, reg_val;
> +
> +	/* IRQ polarity, low or high level, negative or positive edge */
> +	reg_addr = nps_host_reg_non_cl(NPS_GIM_BLKID, NPS_GIM_P_POL);
> +	reg_val = ioread32be(reg_addr);
> +	reg_val &= ~GIM_PERIPH_ALL;
> +	iowrite32be(reg_val, reg_addr);
> +
> +	/* IRQ type level or edge */
> +	reg_addr = nps_host_reg_non_cl(NPS_GIM_BLKID, NPS_GIM_P_SENS);
> +	reg_val = ioread32be(reg_addr);
> +	reg_val |= GIM_LAN_TX;
> +	iowrite32be(reg_val, reg_addr);
> +
> +	/* GIM interrupt select type for debug LAN interrupts (both sides) */
> +	reg_val = DST_IS;
> +	reg_addr = nps_host_reg_non_cl(NPS_GIM_BLKID, NPS_GIM_P_DST10);
> +	iowrite32be(reg_val, reg_addr);
> +	reg_addr = nps_host_reg_non_cl(NPS_GIM_BLKID, NPS_GIM_P_DST11);
> +	iowrite32be(reg_val, reg_addr);
> +	reg_addr = nps_host_reg_non_cl(NPS_GIM_BLKID, NPS_GIM_P_DST25);
> +	iowrite32be(reg_val, reg_addr);
> +	reg_addr = nps_host_reg_non_cl(NPS_GIM_BLKID, NPS_GIM_P_DST26);
> +	iowrite32be(reg_val, reg_addr);
> +
> +	/* CTOP IRQ lines should be defined as blocking in GIM */
> +	iowrite32be(GIM_PERIPH_ALL,
> +		    nps_host_reg_non_cl(NPS_GIM_BLKID, NPS_GIM_P_BLK));
> +
> +	/* Enable CTOP IRQ lines in GIM */
> +	iowrite32be(GIM_PERIPH_ALL,
> +		    nps_host_reg_non_cl(NPS_GIM_BLKID, NPS_GIM_P_EN));
> +}
> +
> +/*
> + * NPS400 core includes a Interrupt Controller (IC) support.
> + * All cores can deactivate level irqs at first level control
> + * at cores mesh layer called MTM.
> + * For devices out side chip e.g. uart, network there is another
> + * level called Global Interrupt Manager (GIM).
> + * This second level can control level and edge interrupt.
> + *
> + * NOTE: AUX_IENABLE and CTOP_AUX_IACK are auxiliary registers
> + * with private HW copy per CPU.
> + */
> +
> +static void nps400_irq_mask(struct irq_data *data)
> +{
> +	unsigned int ienb;
> +
> +	ienb = read_aux_reg(AUX_IENABLE);
> +	ienb &= ~(1 << data->hwirq);
> +	write_aux_reg(AUX_IENABLE, ienb);
> +}
> +
> +static void nps400_irq_unmask(struct irq_data *data)
> +{
> +	unsigned int ienb;
> +
> +	ienb = read_aux_reg(AUX_IENABLE);
> +	ienb |= (1 << data->hwirq);
> +	write_aux_reg(AUX_IENABLE, ienb);
> +}
> +
> +static void nps400_irq_eoi_global(struct irq_data *data)
> +{
> +	write_aux_reg(CTOP_AUX_IACK, 1 << data->hwirq);
> +
> +	/* Don't ack before all device access is done */
> +	mb();
> +
> +	__asm__ __volatile__ (
> +	"       .word %0\n"
> +	:
> +	: "i"(CTOP_INST_RSPI_GIC_0_R12)
> +	: "memory");

Silly question: why cannot you just write the actual instruction instead
of shoving the instruction like this? Also, .inst would be more
appropriate...

> +}
> +
> +static void nps400_irq_eoi(struct irq_data *data)
> +{
> +	write_aux_reg(CTOP_AUX_IACK, 1 << data->hwirq);
> +}
> +
> +
> +static struct irq_chip nps400_irq_chip_fasteoi = {
> +	.name		= "NPS400 IC Global",
> +	.irq_mask	= nps400_irq_mask,
> +	.irq_unmask	= nps400_irq_unmask,
> +	.irq_eoi	= nps400_irq_eoi_global,
> +};
> +
> +static struct irq_chip nps400_irq_chip_percpu = {
> +	.name		= "NPS400 IC",
> +	.irq_mask	= nps400_irq_mask,
> +	.irq_unmask	= nps400_irq_unmask,
> +	.irq_eoi	= nps400_irq_eoi,
> +};
> +
> +static int nps400_irq_map(struct irq_domain *d, unsigned int irq,
> +			  irq_hw_number_t hw)
> +{
> +	switch (irq) {
> +	case TIMER0_IRQ:
> +#if defined(CONFIG_SMP)
> +	case IPI_IRQ:
> +#endif
> +		irq_set_chip_and_handler(irq, &nps400_irq_chip_percpu,
> +					 handle_percpu_irq);
> +	break;
> +	default:
> +		irq_set_chip_and_handler(irq, &nps400_irq_chip_fasteoi,
> +					 handle_fasteoi_irq);
> +	break;
> +	}

No. This is just wrong. Either you get per interrupt information from
the device tree to configure the interrupt the right way, or you have
different interrupt controllers for each device.

But using the Linux irq number is always wrong. You should only consider
the hwirq.

> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops nps400_irq_ops = {
> +	.xlate = irq_domain_xlate_onecell,
> +	.map = nps400_irq_map,
> +};
> +
> +static struct irq_domain *nps400_root_domain;
> +
> +static int __init nps400_of_init(struct device_node *node,
> +				 struct device_node *parent)
> +{
> +	if (parent)
> +		panic("DeviceTree incore ic not a root irq controller\n");
> +
> +	eznps_configure_msu();
> +	eznps_configure_gim();
> +
> +	nps400_root_domain = irq_domain_add_legacy(node, NR_CPU_IRQS, 0, 0,
> +						   &nps400_irq_ops, NULL);

And that's why you can get away with the above horror. Don't use legacy
domains. This stuff is by no mean legacy.

> +
> +	if (!nps400_root_domain)
> +		panic("nps400 root irq domain not avail\n");
> +
> +	/* with this we don't need to export nps400_root_domain */
> +	irq_set_default_host(nps400_root_domain);
> +
> +	return 0;
> +}
> +IRQCHIP_DECLARE(ezchip_nps400_ic, "ezchip,nps400-ic", nps400_of_init);
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1281884 — RE: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips

From	Noam Camus <noamc@ezchip.com>
Date	2015-12-02 16:10 +0100
Subject	RE: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips
Message-ID	<qBhcB-nj-15@gated-at.bofh.it>
In reply to	#1280864

>From: Marc Zyngier [mailto:marc.zyngier@arm.com] 
>Sent: Tuesday, December 01, 2015 3:29 PM

> +  interrupt source. The value shall be 1.

>So you never have to encode the interrupt trigger type? Do you only support edge or level?
I Always use level sensitive.

> +
> +#define NPS_GIM_P_EN	0x100	/* Peripheral interrupts source enable */
> +#define NPS_GIM_P_BLK	0x118	/* Peripheral interrupts blocking for sources */

>>Are these the interrupts the peripherals are using? If yes, they really have nothing to do here...
I will move this from here 
>> +	__asm__ __volatile__ (
>> +	"       .word %0\n"
>> +	:
>> +	: "i"(CTOP_INST_RSPI_GIC_0_R12)
>> +	: "memory");

>Silly question: why cannot you just write the actual instruction instead of shoving the instruction like this? Also, .inst would be more appropriate...
[Noam Camus] Since this is instruction that yet is not part of up-streamed binutils of ARC.  Now ARC maintainer can build our kernel with generic ARC toolchain. 

>> +static int nps400_irq_map(struct irq_domain *d, unsigned int irq,
>> +			  irq_hw_number_t hw)
>> +{
>> +	switch (irq) {
>> +	case TIMER0_IRQ:
>> +#if defined(CONFIG_SMP)
>> +	case IPI_IRQ:
>> +#endif
>> +		irq_set_chip_and_handler(irq, &nps400_irq_chip_percpu,
>> +					 handle_percpu_irq);
>> +	break;
>> +	default:
>> +		irq_set_chip_and_handler(irq, &nps400_irq_chip_fasteoi,
>> +					 handle_fasteoi_irq);
>> +	break;
>> +	}

>No. This is just wrong. Either you get per interrupt information from the device tree to configure the interrupt the right way, or you have different interrupt controllers for each device.
I am not sure how you want me to get it from DTB? Please refer to some reference.

>But using the Linux irq number is always wrong. You should only consider the hwirq.
I will change

> +
> +	nps400_root_domain = irq_domain_add_legacy(node, NR_CPU_IRQS, 0, 0,
> +						   &nps400_irq_ops, NULL);

>And that's why you can get away with the above horror. Don't use legacy domains. This stuff is by no mean legacy.
So what is my alternative here?

-Noam
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283248 — Re: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips

From	Marc Zyngier <marc.zyngier@arm.com>
Date	2015-12-03 19:40 +0100
Subject	Re: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips
Message-ID	<qBGXo-j5-5@gated-at.bofh.it>
In reply to	#1281884

Hi Noam,

On 02/12/15 15:08, Noam Camus wrote:
>> From: Marc Zyngier [mailto:marc.zyngier@arm.com] 
>> Sent: Tuesday, December 01, 2015 3:29 PM
> 
>> +  interrupt source. The value shall be 1.
> 
>> So you never have to encode the interrupt trigger type? Do you only support edge or level?
> I Always use level sensitive.
> 
>> +
>> +#define NPS_GIM_P_EN	0x100	/* Peripheral interrupts source enable */
>> +#define NPS_GIM_P_BLK	0x118	/* Peripheral interrupts blocking for sources */
> 
>>> Are these the interrupts the peripherals are using? If yes, they really have nothing to do here...
> I will move this from here 
>>> +	__asm__ __volatile__ (
>>> +	"       .word %0\n"
>>> +	:
>>> +	: "i"(CTOP_INST_RSPI_GIC_0_R12)
>>> +	: "memory");
> 
>> Silly question: why cannot you just write the actual instruction
>> instead of shoving the instruction like this? Also, .inst would be
>> more appropriate...
> [Noam Camus] Since this is instruction that yet is not part of
> up-streamed binutils of ARC.  Now ARC maintainer can build our kernel
> with generic ARC toolchain.

OK. If you decide to carry on using this, I'd still recommend using
.inst instead of .word, so that you can get a proper disassembly.

>>> +static int nps400_irq_map(struct irq_domain *d, unsigned int irq,
>>> +			  irq_hw_number_t hw)
>>> +{
>>> +	switch (irq) {
>>> +	case TIMER0_IRQ:
>>> +#if defined(CONFIG_SMP)
>>> +	case IPI_IRQ:
>>> +#endif
>>> +		irq_set_chip_and_handler(irq, &nps400_irq_chip_percpu,
>>> +					 handle_percpu_irq);
>>> +	break;
>>> +	default:
>>> +		irq_set_chip_and_handler(irq, &nps400_irq_chip_fasteoi,
>>> +					 handle_fasteoi_irq);
>>> +	break;
>>> +	}
> 
>> No. This is just wrong. Either you get per interrupt information
>> from the device tree to configure the interrupt the right way, or
>> you have different interrupt controllers for each device.
> I am not sure how you want me to get it from DTB? Please refer to
> some reference.

Here, you are assuming that 'irq' is a hardware number, while there is
no reason why it should be (it only works because you are using legacy
domains, more on that later).

Your switch/case statement should be based on the 'hw' parameter,
because that is your HW IRQ number. the irq parameter can be completely
random, and will eventually be once you fix the rest of the driver.

Also, can you always tell the per-cpu property of your interrupt based
on its number? If you can, then it is fine.

>> But using the Linux irq number is always wrong. You should only consider the hwirq.
> I will change
> 
>> +
>> +	nps400_root_domain = irq_domain_add_legacy(node, NR_CPU_IRQS, 0, 0,
>> +						   &nps400_irq_ops, NULL);
> 
>> And that's why you can get away with the above horror. Don't use
>> legacy domains. This stuff is by no mean legacy.
> So what is my alternative here?

Your alternative is to use irq_domain_add_linear, for example, and to
make sure that you always refer to the hw number when manipulating the
HW. You will quickly notice that the Linux IRQ number has nothing to do
with the HW one, and you'll be able to quickly iron out the bugs.

Looking forward to reviewing your next version.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1285215 — RE: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips

From	Noam Camus <noamc@ezchip.com>
Date	2015-12-07 12:20 +0100
Subject	RE: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips
Message-ID	<qD1ZM-3Ks-7@gated-at.bofh.it>
In reply to	#1283248

From: Marc Zyngier [mailto:marc.zyngier@arm.com] 
Sent: Thursday, December 03, 2015 8:34 PM

>>> Silly question: why cannot you just write the actual instruction 
>>> instead of shoving the instruction like this? Also, .inst would be 
>>> more appropriate...
>> [Noam Camus] Since this is instruction that yet is not part of 
>> up-streamed binutils of ARC.  Now ARC maintainer can build our kernel 
>> with generic ARC toolchain.

>OK. If you decide to carry on using this, I'd still recommend using .inst instead of .word, so that you can get a proper disassembly.
Seem to me that ".inst" is ARM Machine directive.

>Also, can you always tell the per-cpu property of your interrupt based on its number? If you can, then it is fine.
Yes I can, and I am using this knowledge.

>Your alternative is to use irq_domain_add_linear, for example, and to make sure that you always refer to the hw number when manipulating the HW. You will quickly notice that the Linux IRQ number has nothing to do with the HW one, and you'll be able to quickly iron out the bugs.

Indeed when I moved from legacy to linear map I had to call irq_create_mapping() for my percpu IRQs and that's it.
Thanks

-Noam
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1289285 — Re: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips

From	Vineet Gupta <Vineet.Gupta1@synopsys.com>
Date	2015-12-11 09:00 +0100
Subject	Re: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips
Message-ID	<qEqMq-19l-9@gated-at.bofh.it>
In reply to	#1280864

On Tuesday 01 December 2015 06:59 PM, Marc Zyngier wrote:
>> +static int nps400_irq_map(struct irq_domain *d, unsigned int irq,
>> > +			  irq_hw_number_t hw)
>> > +{
>> > +	switch (irq) {
>> > +	case TIMER0_IRQ:
>> > +#if defined(CONFIG_SMP)
>> > +	case IPI_IRQ:
>> > +#endif
>> > +		irq_set_chip_and_handler(irq, &nps400_irq_chip_percpu,
>> > +					 handle_percpu_irq);
>> > +	break;
>> > +	default:
>> > +		irq_set_chip_and_handler(irq, &nps400_irq_chip_fasteoi,
>> > +					 handle_fasteoi_irq);
>> > +	break;
>> > +	}
> No. This is just wrong. Either you get per interrupt information from
> the device tree to configure the interrupt the right way, or you have
> different interrupt controllers for each device.
> 
> But using the Linux irq number is always wrong. You should only consider
> the hwirq.

The source is this incorrectness is ARC core intc code which also does the same
thing and we get away with it because of the legacy domain usage.

I'll fix that up.

Thx,
-Vineet
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283622 — Re: [PATCH v3 01/18] Documentation: Add EZchip vendor to binding list

From	Daniel Lezcano <daniel.lezcano@linaro.org>
Date	2015-12-04 09:10 +0100
Subject	Re: [PATCH v3 01/18] Documentation: Add EZchip vendor to binding list
Message-ID	<qBTBg-7g-13@gated-at.bofh.it>
In reply to	#1280843

On 12/01/2015 02:02 PM, Noam Camus wrote:
> From: Noam Camus <noamc@ezchip.com>
>
> Add EZchip to vendor prefixes list.
> EZchip introduce the NPS platform for the ARC architecture.
>
> Signed-off-by: Noam Camus <noamc@ezchip.com>
> Acked-by: Rob Herring <robh+dt@kernel.org>
> Cc: Pawel Moll <pawel.moll@arm.com>
> ---
>   Documentation/devicetree/bindings/arc/eznps.txt    |    7 +++++++
>   .../devicetree/bindings/vendor-prefixes.txt        |    1 +
>   2 files changed, 8 insertions(+), 0 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/arc/eznps.txt
>
> diff --git a/Documentation/devicetree/bindings/arc/eznps.txt b/Documentation/devicetree/bindings/arc/eznps.txt
> new file mode 100644
> index 0000000..f8b5e9b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arc/eznps.txt
> @@ -0,0 +1,7 @@
> +EZchip NPS Network Proccessor Platforms Device Tree Bindings

Processor

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283723 — Re: [PATCH v3 01/18] Documentation: Add EZchip vendor to binding list

From	Noam Camus <noamc@ezchip.com>
Date	2015-12-04 12:50 +0100
Subject	Re: [PATCH v3 01/18] Documentation: Add EZchip vendor to binding list
Message-ID	<qBX29-279-1@gated-at.bofh.it>
In reply to	#1283622

>From: Daniel Lezcano <daniel.lezcano@linaro.org>
>Sent: Friday, December 4, 2015 10:07 AM
>>
>> diff --git a/Documentation/devicetree/bindings/arc/eznps.txt b/Documentation/devicetree/bindings/arc/eznps.txt
>> new file mode 100644
>> index 0000000..f8b5e9b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arc/eznps.txt
>> @@ -0,0 +1,7 @@
>> +EZchip NPS Network Proccessor Platforms Device Tree Bindings
>
>Processor
Thanks
--
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283669 — Re: [PATCH v3 03/18] clocksource: Add NPS400 timers driver

From	Daniel Lezcano <daniel.lezcano@linaro.org>
Date	2015-12-04 10:20 +0100
Subject	Re: [PATCH v3 03/18] clocksource: Add NPS400 timers driver
Message-ID	<qBUH1-Lu-13@gated-at.bofh.it>
In reply to	#1280843

On 12/01/2015 02:02 PM, Noam Camus wrote:
> From: Noam Camus <noamc@ezchip.com>
>
> Add internal tick generator which is shared by all cores.
> Each cluster of cores view it through dedicated address.
> This is used for SMP system where all CPUs synced by same
> clock source.
>
> Signed-off-by: Noam Camus <noamc@ezchip.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: John Stultz <john.stultz@linaro.org>
> Acked-by: Vineet Gupta <vgupta@synopsys.com>

[ ... ]

> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 56bd16e..20969b0 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -46,6 +46,7 @@ obj-$(CONFIG_CLKSRC_QCOM)	+= qcom-timer.o
>   obj-$(CONFIG_MTK_TIMER)		+= mtk_timer.o
>   obj-$(CONFIG_CLKSRC_PISTACHIO)	+= time-pistachio.o
>   obj-$(CONFIG_CLKSRC_TI_32K)	+= timer-ti-32k.o
> +obj-$(CONFIG_ARC_PLAT_EZNPS)	+= timer-nps.o

CONFIG_CLKSRC_NPS

>
>   obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
>   obj-$(CONFIG_ARM_GLOBAL_TIMER)		+= arm_global_timer.o
> diff --git a/drivers/clocksource/timer-nps.c b/drivers/clocksource/timer-nps.c
> new file mode 100644
> index 0000000..ef8f287
> --- /dev/null
> +++ b/drivers/clocksource/timer-nps.c
> @@ -0,0 +1,63 @@
> +/*
> + * Copyright(c) 2015 EZchip Technologies.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * The full GNU General Public License is included in this distribution in
> + * the file called "COPYING".
> + */
> +
> +#include <linux/clocksource.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <plat/ctop.h>

Why do you need this header ? nps_host_reg ?

We prevent to include headers from <plat> in the drivers directory. You 
should find a way to get rid of it.

> +#define NPS_MSU_TICK_LOW	0xC8
> +#define NPS_CLUSTER_OFFSET	8
> +#define NPS_CLUSTER_NUM		16
> +
> +static void *nps_msu_reg_low_addr[NPS_CLUSTER_NUM] __read_mostly;

Perhaps a small optimization...

static DEFINE_PER_CPU_READ_MOSTLY(void __iomem *, baseaddr);

static cycle_t nps_clksrc_read(struct clocksource *clksrc)
{
	void __iomem *base = per_cpu(baseaddr, raw_smp_processor_id());

	return (cycle_t)ioread32be(base);
}

and in the init function:

for_each_cpu(cpu) {
	per_cpu(baseaddr, cpu) = nps_host_reg(cpu,
					NPS_MSU_BLKID,
					NPS_MSU_TICK_LOW
}

> +static cycle_t nps_clksrc_read(struct clocksource *clksrc)
> +{
> +	int cluster = raw_smp_processor_id() >> NPS_CLUSTER_OFFSET;
> +
> +	return (cycle_t)ioread32be(nps_msu_reg_low_addr[cluster]);

AFAICT, there is a memory barrier with ioread32be, are you really sure 
we have to use it in this code path ?

> +}
> +
> +static struct clocksource nps_counter = {
> +	.name	= "EZnps-tick",
> +	.rating = 301,
> +	.read   = nps_clksrc_read,
> +	.mask   = CLOCKSOURCE_MASK(32),
> +	.flags  = CLOCK_SOURCE_IS_CONTINUOUS,
> +};
> +
> +static void __init nps_setup_clocksource(struct device_node *node)
> +{
> +	struct clocksource *clksrc = &nps_counter;
> +	unsigned long rate, dt_root;
> +	int ret, cluster;
> +
> +	for (cluster = 0; cluster < NPS_CLUSTER_NUM; cluster++)
> +		nps_msu_reg_low_addr[cluster] =
> +			nps_host_reg((cluster << NPS_CLUSTER_OFFSET),
> +				 NPS_MSU_BLKID, NPS_MSU_TICK_LOW);

> +
> +	dt_root = of_get_flat_dt_root();
> +	rate = (u32)of_get_flat_dt_prop(dt_root, "clock-frequency", NULL);

I don't get why this is done this way. The Kconfig option help says the 
clocksource rate is 1GHz but in the DT the clock is 88MHz.

It would be cleaner to define a fixed clock and then add a phandle in 
the DT.

	timer_clk: timer_clk {
		#clock-cells = <0>;
		compatible = "fixed-clock";
		clock-frequency = <123456789>;
	};

	timer {
		compatible = "ezchip,nps400-timer";
		clocks = <&timer_clk>;
	}

That will result in the same code than the other drivers.

	clk = of_clk_get(np, 0);
	if (IS_ERR(clk)) {
		pr_err("%s: invalid clock\n", np->full_name);
		return;
	}

  	rate = clk_get_rate(clk);


> +	ret = clocksource_register_hz(clksrc, rate);
> +	if (ret)
> +		pr_err("Couldn't register clock source.\n");
> +}
> +
> +CLOCKSOURCE_OF_DECLARE(nps_400, "ezchip,nps400-timer",
> +		       nps_setup_clocksource);
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283776 — Re: [PATCH v3 03/18] clocksource: Add NPS400 timers driver

From	Noam Camus <noamc@ezchip.com>
Date	2015-12-04 13:30 +0100
Subject	Re: [PATCH v3 03/18] clocksource: Add NPS400 timers driver
Message-ID	<qBXES-2B5-7@gated-at.bofh.it>
In reply to	#1283669

>From: Daniel Lezcano <daniel.lezcano@linaro.org>
>Sent: Friday, December 4, 2015 11:13 AM
>> +obj-$(CONFIG_ARC_PLAT_EZNPS) += timer-nps.o

>CONFIG_CLKSRC_NPS
I wish this driver to be build only for this specific ARC platform.
This clock source is embedded in our SoC.
It is not meant to be built for any other architecture.
This is why below I include header from our ARC platform to avoid code duplicity.

>> +#include <plat/ctop.h>

>Why do you need this header ? nps_host_reg ?
Correct we use common code from our platform.

>We prevent to include headers from <plat> in the drivers directory. You
>should find a way to get rid of it.

The Only way I can think of is code duplicity and I prefer not to.
I see some include to asm/mach headers in clocksource, what is the difference?
Could you suggest a better place for me to place my header file.

>> +static void *nps_msu_reg_low_addr[NPS_CLUSTER_NUM] __read_mostly;
>
>Perhaps a small optimization...
Thanks

>static DEFINE_PER_CPU_READ_MOSTLY(void __iomem *, baseaddr);

>static cycle_t nps_clksrc_read(struct clocksource *clksrc)
>{
>       void __iomem *base = per_cpu(baseaddr, raw_smp_processor_id());
>
 >       return (cycle_t)ioread32be(base);
>}

>and in the init function:

>for_each_cpu(cpu) {
>       per_cpu(baseaddr, cpu) = nps_host_reg(cpu,
>                                      NPS_MSU_BLKID,
>                                    NPS_MSU_TICK_LOW
>}
Thanks again


>> +static cycle_t nps_clksrc_read(struct clocksource *clksrc)
>> +{
>> +     int cluster = raw_smp_processor_id() >> NPS_CLUSTER_OFFSET;
>> +
>> +     return (cycle_t)ioread32be(nps_msu_reg_low_addr[cluster]);

>,AFAICT, there is a memory barrier with ioread32be, are you really sure
>we have to use it in this code path ?
Are you saying to remove use of ioread32be?
What should I use instead?

>> +
>> +     dt_root = of_get_flat_dt_root();
>> +     rate = (u32)of_get_flat_dt_prop(dt_root, "clock-frequency", NULL);

>I don't get why this is done this way. The Kconfig option help says the
>clocksource rate is 1GHz but in the DT the clock is 88MHz.
It says that clock source is up to 1GHz

>It would be cleaner to define a fixed clock and then add a phandle in
>the DT.

>        timer_clk: timer_clk {
>               #clock-cells = <0>;
>                compatible = "fixed-clock";
>               clock-frequency = <123456789>;
>        };

>        timer {
>               compatible = "ezchip,nps400-timer";
>                clocks = <&timer_clk>;
>       }

>That will result in the same code than the other drivers.

>      clk = of_clk_get(np, 0);
>     if (IS_ERR(clk)) {
>              pr_err("%s: invalid clock\n", np->full_name);
>               return;
>        }

>      rate = clk_get_rate(clk);
Once again thanks I will fix this.

-Noam
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283798 — Re: [PATCH v3 03/18] clocksource: Add NPS400 timers driver

From	Daniel Lezcano <daniel.lezcano@linaro.org>
Date	2015-12-04 14:00 +0100
Subject	Re: [PATCH v3 03/18] clocksource: Add NPS400 timers driver
Message-ID	<qBY7U-2M0-25@gated-at.bofh.it>
In reply to	#1283776

On 12/04/2015 01:26 PM, Noam Camus wrote:
>> From: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Sent: Friday, December 4, 2015 11:13 AM
>>> +obj-$(CONFIG_ARC_PLAT_EZNPS) += timer-nps.o
>
>> CONFIG_CLKSRC_NPS
> I wish this driver to be build only for this specific ARC platform.
> This clock source is embedded in our SoC.

I understand but we are removing all platform specific Kconfig options 
from the drivers. If CONFIG_CLKSRC_NPS is selected by 
CONFIG_ARC_PLAT_EZNPS only, the result is the same.

By the way, it is probable the "if COMPILE_TEST" option is added later 
to the option. We are trying to have the drivers to be compilable on 
different platforms in order to increase the compilation test coverage. 
It is useful for instance when we touch a common code which impact all 
the drivers, we don't need the specific platform to compile on it and we 
minimize a cross compile toolchain usage. Hence compilation failure on 
ARC will be detected sooner, before the next merge window happens.

For this reason, having common code in the driver is important and 
including a platform breaks the current effort.

> It is not meant to be built for any other architecture.
> This is why below I include header from our ARC platform to avoid code duplicity.
>
>>> +#include <plat/ctop.h>
>
>> Why do you need this header ? nps_host_reg ?
> Correct we use common code from our platform.
>
>> We prevent to include headers from <plat> in the drivers directory. You
>> should find a way to get rid of it.
>
> The Only way I can think of is code duplicity and I prefer not to.
> I see some include to asm/mach headers in clocksource, what is the difference?

The difference is <asm> includes are architecture dependent and could be 
acceptable if there is no choice (all arm timers use the same 
functionality from these headers), while <plat> includes are platform 
specific, so restricted to a specific platform.

> Could you suggest a better place for me to place my header file.

Yes, perhaps:

include/soc/nps
drivers/soc/nps

>>> +static void *nps_msu_reg_low_addr[NPS_CLUSTER_NUM] __read_mostly;
>>
>> Perhaps a small optimization...
> Thanks
>
>> static DEFINE_PER_CPU_READ_MOSTLY(void __iomem *, baseaddr);
>
>> static cycle_t nps_clksrc_read(struct clocksource *clksrc)
>> {
>>        void __iomem *base = per_cpu(baseaddr, raw_smp_processor_id());
>>
>   >       return (cycle_t)ioread32be(base);
>> }
>
>> and in the init function:
>
>> for_each_cpu(cpu) {
>>        per_cpu(baseaddr, cpu) = nps_host_reg(cpu,
>>                                       NPS_MSU_BLKID,
>>                                     NPS_MSU_TICK_LOW
>> }
> Thanks again
>
>
>>> +static cycle_t nps_clksrc_read(struct clocksource *clksrc)
>>> +{
>>> +     int cluster = raw_smp_processor_id() >> NPS_CLUSTER_OFFSET;
>>> +
>>> +     return (cycle_t)ioread32be(nps_msu_reg_low_addr[cluster]);
>
>> ,AFAICT, there is a memory barrier with ioread32be, are you really sure
>> we have to use it in this code path ?
> Are you saying to remove use of ioread32be?
> What should I use instead?

Never mind, I looked at the arm's ioread32be definition where there is a 
memory barrier. With this architecture ioread32be uses the generic one 
without the memory barrier (well, yes it uses one in the raw_readl asm 
implementation but we can't do anything).

>>> +
>>> +     dt_root = of_get_flat_dt_root();
>>> +     rate = (u32)of_get_flat_dt_prop(dt_root, "clock-frequency", NULL);
>
>> I don't get why this is done this way. The Kconfig option help says the
>> clocksource rate is 1GHz but in the DT the clock is 88MHz.
> It says that clock source is up to 1GHz

ah ok.

>> It would be cleaner to define a fixed clock and then add a phandle in
>> the DT.
>
>>         timer_clk: timer_clk {
>>                #clock-cells = <0>;
>>                 compatible = "fixed-clock";
>>                clock-frequency = <123456789>;
>>         };
>
>>         timer {
>>                compatible = "ezchip,nps400-timer";
>>                 clocks = <&timer_clk>;
>>        }
>
>> That will result in the same code than the other drivers.
>
>>       clk = of_clk_get(np, 0);
>>      if (IS_ERR(clk)) {
>>               pr_err("%s: invalid clock\n", np->full_name);
>>                return;
>>         }
>
>>       rate = clk_get_rate(clk);
> Once again thanks I will fix this.
>
> -Noam
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1286452 — RE: [PATCH v3 03/18] clocksource: Add NPS400 timers driver

From	Noam Camus <noamc@ezchip.com>
Date	2015-12-08 14:10 +0100
Subject	RE: [PATCH v3 03/18] clocksource: Add NPS400 timers driver
Message-ID	<qDqbN-2Nk-33@gated-at.bofh.it>
In reply to	#1283669

RnJvbTogRGFuaWVsIExlemNhbm8gW21haWx0bzpkYW5pZWwubGV6Y2Fub0BsaW5hcm8ub3JnXSAN
ClNlbnQ6IEZyaWRheSwgRGVjZW1iZXIgMDQsIDIwMTUgMTE6MTMgQU0NCg0KDQo+SXQgd291bGQg
YmUgY2xlYW5lciB0byBkZWZpbmUgYSBmaXhlZCBjbG9jayBhbmQgdGhlbiBhZGQgYSBwaGFuZGxl
IGluIHRoZSBEVC4NCg0KPgl0aW1lcl9jbGs6IHRpbWVyX2NsayB7DQo+CQkjY2xvY2stY2VsbHMg
PSA8MD47DQo+CQljb21wYXRpYmxlID0gImZpeGVkLWNsb2NrIjsNCj4JCWNsb2NrLWZyZXF1ZW5j
eSA9IDwxMjM0NTY3ODk+Ow0KPgl9Ow0KDQo+CXRpbWVyIHsNCj4JCWNvbXBhdGlibGUgPSAiZXpj
aGlwLG5wczQwMC10aW1lciI7DQo+CQljbG9ja3MgPSA8JnRpbWVyX2Nsaz47DQo+CX0NCg0KPlRo
YXQgd2lsbCByZXN1bHQgaW4gdGhlIHNhbWUgY29kZSB0aGFuIHRoZSBvdGhlciBkcml2ZXJzLg0K
DQo+CWNsayA9IG9mX2Nsa19nZXQobnAsIDApOw0KPglpZiAoSVNfRVJSKGNsaykpIHsNCj4JCXBy
X2VycigiJXM6IGludmFsaWQgY2xvY2tcbiIsIG5wLT5mdWxsX25hbWUpOw0KPgkJcmV0dXJuOw0K
Pgl9DQoNCj4gIAlyYXRlID0gY2xrX2dldF9yYXRlKGNsayk7DQoNClNlZW0gbGlrZSBteSBkcml2
ZXIgY29tZXMgYmVmb3JlICJmaXhlZC1jbG9jayIgZHJpdmVyIHNvIEkgZmFpbGVkIHNpbmNlIEkg
Z290IG5vIGNsb2Nrc291cmNlIHByb3ZpZGVyLg0KSG93IGNhbiB0aGlzIGJlIHNvbHZlZD8NCg0K
LU5vYW0NCg==
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

csiph-web

[PATCH v3 00/18] *** SUBJECT HERE ***

Contents

#1280843 — [PATCH v3 00/18] *** SUBJECT HERE ***

#1280864 — Re: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips

#1281884 — RE: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips

#1283248 — Re: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips

#1285215 — RE: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips

#1289285 — Re: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips

#1283622 — Re: [PATCH v3 01/18] Documentation: Add EZchip vendor to binding list

#1283723 — Re: [PATCH v3 01/18] Documentation: Add EZchip vendor to binding list

#1283669 — Re: [PATCH v3 03/18] clocksource: Add NPS400 timers driver

#1283776 — Re: [PATCH v3 03/18] clocksource: Add NPS400 timers driver

#1283798 — Re: [PATCH v3 03/18] clocksource: Add NPS400 timers driver

#1286452 — RE: [PATCH v3 03/18] clocksource: Add NPS400 timers driver

[PATCH v3 00/18] * SUBJECT HERE *

#1280843 — [PATCH v3 00/18] * SUBJECT HERE *