Re: [Patch v4 01/12] microblaze: irqchip: Move intc driver to irqchip
Hi Michal, Zubair, On Fri, Sep 02, 2016 at 12:27:54PM +0200, Michal Simek wrote: > On 2.9.2016 12:06, Zubair Lutfullah Kakakhel wrote: > > On 09/02/2016 07:25 AM, Michal Simek wrote: ... > >> Also there is another copy of this driver in the tree which was using > >> old ppc405 and ppc440 xilinx platforms. > >> > >> arch/powerpc/include/asm/xilinx_intc.h > >> arch/powerpc/sysdev/xilinx_intc.c > >> > >> These should be also removed by moving this driver to generic folder. > > > > I didn't know about that drivers existence. > > > > This patch series already touches microblaze, mips and irqchip. > > Both microblaze and mips platforms using this driver are little-endian. > > MB is big ending too and as you see there is big endian support in the > driver already. > > > > > Adding a big-endian powerpc driver + platform to the mix is going to > > complicate the series further and make it super hard to synchronize > > various subsystems, test stuff, and then move the drivers without > > breakage. The whole point of Linus' push to move drivers out of arch/ is to reduce code duplication and create more robust drivers. > > I'd highly recommend letting this move happen. And then the powerpc > > driver can transition over time to this driver. I've seen this argument before, and despite everyone's best intentions, it never happens. :( We have linux-next, 0-day and other resources to test these sorts of changes and catch errors before they hit mainline. Let's take our time and do it right. thx, Jason.
Re: [Patch v4 01/12] microblaze: irqchip: Move intc driver to irqchip
Hi Michal, On Fri, Sep 02, 2016 at 02:06:40PM +0200, Michal Simek wrote: > On 2.9.2016 13:46, Zubair Lutfullah Kakakhel wrote: > > On 09/02/2016 11:27 AM, Michal Simek wrote: > >> On 2.9.2016 12:06, Zubair Lutfullah Kakakhel wrote: > >>> On 09/02/2016 07:25 AM, Michal Simek wrote: > >>>> On 1.9.2016 18:50, Zubair Lutfullah Kakakhel wrote: > >>>>> V1 -> V2 > >>>>> > >>>>> Renamed irq-xilinx to irq-axi-intc > >>>>> Renamed CONFIG_XILINX_INTC to CONFIG_XILINX_AXI_INTC > >>>> > >>>> > >>>> I see that this was suggested by Jason Cooper but using axi name > >>>> here is > >>>> not correct. > >>>> There is xps-intc name which is the name used on old OPB hardware > >>>> designs. It means this driver can be still used only on system which > >>>> uses it. > >>> > >>> Wouldn't axi-intc be more suitable moving forwards? > >>> The IP block is now known as axi intc for 5 years as far as I can tell. > >>> > >>> Searching "axi intc" online results in the right docs for current and > >>> future platforms. Please add links to the relevant docs in the comments of the code. > >> > >> yes but we still should support older platform and it is more then this. > >> This is soft-IP core and in future when there is new bus then IP will > >> just change bus interface, etc. > > > > That makes sense. I'll rename the driver to irq-xps-intc.c > > and CONFIG_XILINX_XPS_INTC > > > > Please shout now if anybody has issues with this. > > XPS was shortcut for design tools. You had CONFIG_XILINX_INTC which is > IMHO the best name you can have. Michal, thanks for the background info! Zubair, any problem with CONFIG_XILINX_INTC/irq-xilinx-intc.c ? thx, Jason.
Re: [PATCH 1/9] microblaze: irqchip: Move intc driver to irqchip
Hi Zubair, On Mon, Aug 15, 2016 at 02:55:27PM +0100, Zubair Lutfullah Kakakhel wrote: > The Xilinx AXI Interrupt Controller IP block is used by the MIPS > based xilfpga platform. > > Move the interrupt controller code out of arch/microblaze so that > it can be used by everyone > > Signed-off-by: Zubair Lutfullah Kakakhel > --- > arch/microblaze/Kconfig | 1 + > arch/microblaze/kernel/Makefile | 2 +- > drivers/irqchip/Kconfig | 4 > drivers/irqchip/Makefile | 1 + > arch/microblaze/kernel/intc.c => drivers/irqchip/irq-xilinx.c | 0 > 5 files changed, 7 insertions(+), 1 deletion(-) > rename arch/microblaze/kernel/intc.c => drivers/irqchip/irq-xilinx.c (100%) When you send the next version, please disable rename detection. The driver looks pretty straight forward, but I'd like to take the opportunity to clean up the abstraction around read and write. As well as making it easier for everyone to review on-list. thx, Jason.
Re: [PATCH 8/9] MIPS: xilfpga: Add DT node for AXI emaclite
Hi Zubair,
On Mon, Aug 15, 2016 at 02:55:34PM +0100, Zubair Lutfullah Kakakhel wrote:
> The xilfpga platform has a Xilinx AXI emaclite block.
>
> Add the DT node to use it.
>
> Signed-off-by: Zubair Lutfullah Kakakhel
> ---
> arch/mips/boot/dts/xilfpga/nexys4ddr.dts | 27 +++
> 1 file changed, 27 insertions(+)
>
> diff --git a/arch/mips/boot/dts/xilfpga/nexys4ddr.dts
> b/arch/mips/boot/dts/xilfpga/nexys4ddr.dts
> index 3658e21..58bc62f 100644
> --- a/arch/mips/boot/dts/xilfpga/nexys4ddr.dts
> +++ b/arch/mips/boot/dts/xilfpga/nexys4ddr.dts
> @@ -42,6 +42,33 @@
> xlnx,tri-default = <0x>;
> } ;
>
> + axi_ethernetlite: ethernet@10e0 {
> + compatible = "xlnx,xps-ethernetlite-3.00.a";
This one also isn't documented.
> + device_type = "network";
> + interrupt-parent = <&axi_intc>;
> + interrupts = <1>;
> + local-mac-address = [08 86 4C 0D F7 09];
I'm pretty sure you don't want this in the mainline dts file.
thx,
Jason.
> + phy-handle = <&phy0>;
> + reg = <0x10e0 0x1>;
> + xlnx,duplex = <0x1>;
> + xlnx,include-global-buffers = <0x1>;
> + xlnx,include-internal-loopback = <0x0>;
> + xlnx,include-mdio = <0x1>;
> + xlnx,instance = "axi_ethernetlite_inst";
> + xlnx,rx-ping-pong = <0x1>;
> + xlnx,s-axi-id-width = <0x1>;
> + xlnx,tx-ping-pong = <0x1>;
> + xlnx,use-internal = <0x0>;
> + mdio {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + phy0: phy@1 {
> + device_type = "ethernet-phy";
> + reg = <1>;
> + };
> + };
> + };
> +
> axi_uart16550: serial@1040 {
> compatible = "ns16550a";
> reg = <0x1040 0x1>;
> --
> 1.9.1
>
Re: [PATCH 4/9] MIPS: xilfpga: Use Xilinx AXI Interrupt Controller
Hi Zubair,
On Mon, Aug 15, 2016 at 02:55:30PM +0100, Zubair Lutfullah Kakakhel wrote:
> IRQs from peripherals such as i2c/uart/ethernet come via
> the AXI Interrupt controller.
>
> Select it in Kconfig for xilfpga and add the DT node
>
> Signed-off-by: Zubair Lutfullah Kakakhel
> ---
> arch/mips/Kconfig| 1 +
> arch/mips/boot/dts/xilfpga/nexys4ddr.dts | 12
> 2 files changed, 13 insertions(+)
>
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index 2638856..42ecf40 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -426,6 +426,7 @@ config MACH_XILFPGA
> select SYS_SUPPORTS_ZBOOT_UART16550
> select USE_OF
> select USE_GENERIC_EARLY_PRINTK_8250
> + select XILINX_IRQ
> help
> This enables support for the IMG University Program MIPSfpga platform.
Please split dt changes from code changes.
> diff --git a/arch/mips/boot/dts/xilfpga/nexys4ddr.dts
> b/arch/mips/boot/dts/xilfpga/nexys4ddr.dts
> index 48d2112..8db660b 100644
> --- a/arch/mips/boot/dts/xilfpga/nexys4ddr.dts
> +++ b/arch/mips/boot/dts/xilfpga/nexys4ddr.dts
> @@ -17,6 +17,18 @@
> compatible = "mti,cpu-interrupt-controller";
> };
>
> + axi_intc: interrupt-controller@1020 {
> + #interrupt-cells = <1>;
> + compatible = "xlnx,xps-intc-1.00.a";
This compatible string isn't documented, mind adding it? Please make
sure to Cc the devicetree maintainers on it.
thx,
Jason.
> + interrupt-controller;
> + reg = <0x1020 0x1>;
> + xlnx,kind-of-intr = <0x0>;
> + xlnx,num-intr-inputs = <0x6>;
> +
> + interrupt-parent = <&cpuintc>;
> + interrupts = <6>;
> + };
> +
> axi_gpio: gpio@1060 {
> #gpio-cells = <1>;
> compatible = "xlnx,xps-gpio-1.00.a";
> --
> 1.9.1
>
Re: [PATCH 1/9] microblaze: irqchip: Move intc driver to irqchip
Hi Zubair, On Mon, Aug 15, 2016 at 02:55:27PM +0100, Zubair Lutfullah Kakakhel wrote: > The Xilinx AXI Interrupt Controller IP block is used by the MIPS > based xilfpga platform. > > Move the interrupt controller code out of arch/microblaze so that > it can be used by everyone > > Signed-off-by: Zubair Lutfullah Kakakhel > --- > arch/microblaze/Kconfig | 1 + > arch/microblaze/kernel/Makefile | 2 +- > drivers/irqchip/Kconfig | 4 > drivers/irqchip/Makefile | 1 + > arch/microblaze/kernel/intc.c => drivers/irqchip/irq-xilinx.c | 0 > 5 files changed, 7 insertions(+), 1 deletion(-) > rename arch/microblaze/kernel/intc.c => drivers/irqchip/irq-xilinx.c (100%) How about irq-axi-intc.c? > diff --git a/arch/microblaze/Kconfig b/arch/microblaze/Kconfig > index 86f6572..198e921 100644 > --- a/arch/microblaze/Kconfig > +++ b/arch/microblaze/Kconfig > @@ -27,6 +27,7 @@ config MICROBLAZE > select HAVE_MEMBLOCK_NODE_MAP > select HAVE_OPROFILE > select IRQ_DOMAIN > + select XILINX_IRQ > select MODULES_USE_ELF_RELA > select OF > select OF_EARLY_FLATTREE > diff --git a/arch/microblaze/kernel/Makefile b/arch/microblaze/kernel/Makefile > index f08baca..e098381 100644 > --- a/arch/microblaze/kernel/Makefile > +++ b/arch/microblaze/kernel/Makefile > @@ -15,7 +15,7 @@ endif > extra-y := head.o vmlinux.lds > > obj-y += dma.o exceptions.o \ > - hw_exception_handler.o intc.o irq.o \ > + hw_exception_handler.o irq.o \ > platform.o process.o prom.o ptrace.o \ > reset.o setup.o signal.o sys_microblaze.o timer.o traps.o unwind.o > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index 7f87289..b5e40ee 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -203,6 +203,10 @@ config XTENSA_MX > bool > select IRQ_DOMAIN > > +config XILINX_IRQ XILINX_AXI_INTC ? thx, Jason. > + bool > + select IRQ_DOMAIN > + > config IRQ_CROSSBAR > bool > help > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index 4c203b6..2bd833d 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -51,6 +51,7 @@ obj-$(CONFIG_TB10X_IRQC)+= irq-tb10x.o > obj-$(CONFIG_TS4800_IRQ) += irq-ts4800.o > obj-$(CONFIG_XTENSA) += irq-xtensa-pic.o > obj-$(CONFIG_XTENSA_MX) += irq-xtensa-mx.o > +obj-$(CONFIG_XILINX_IRQ) += irq-xilinx.o > obj-$(CONFIG_IRQ_CROSSBAR) += irq-crossbar.o > obj-$(CONFIG_SOC_VF610) += irq-vf610-mscm-ir.o > obj-$(CONFIG_BCM6345_L1_IRQ) += irq-bcm6345-l1.o > diff --git a/arch/microblaze/kernel/intc.c b/drivers/irqchip/irq-xilinx.c > similarity index 100% > rename from arch/microblaze/kernel/intc.c > rename to drivers/irqchip/irq-xilinx.c > -- > 1.9.1 >
Re: [PATCH v2 1/3] net: mvneta: introduce compatible string "marvell, armada-xp-neta"
On Thu, Jun 25, 2015 at 11:13:23AM +0200, Simon Guinot wrote: > On Fri, Jun 19, 2015 at 02:32:53PM +0200, Simon Guinot wrote: > > On Wed, Jun 17, 2015 at 05:01:12PM +0000, Jason Cooper wrote: > > > On Wed, Jun 17, 2015 at 05:15:28PM +0200, Gregory CLEMENT wrote: > > > > On 17/06/2015 17:12, Gregory CLEMENT wrote: > > > > > On 17/06/2015 15:19, Simon Guinot wrote: > > > > >> The mvneta driver supports the Ethernet IP found in the Armada 370, > > > > >> XP, > > > > >> 380 and 385 SoCs. Since at least one more hardware feature is > > > > >> available > > > > >> for the Armada XP SoCs then a way to identify them is needed. > > > > >> > > > > >> This patch introduces a new compatible string > > > > >> "marvell,armada-xp-neta". > > > > > > > > > > Let's be future proof by going further. I would like to have an > > > > > compatible string > > > > > for each SoC even if we currently we don't use them. > > > > > > I disagree with this. We can't predict what incosistencies we'll > > > discover in > > > the future. We should only assign new compatible strings based on known > > > IP > > > variations when we discover them. This seems fraught with demons since we > > > can't predict the scope of affected IP blocks (some steppings of one SoC, > > > three > > > SoCs plus two steppings of a fourth, etc) > > > > > > imho, the 'future-proofing' lies in being specific as to the naming of the > > > compatible strings against known hardware variations at the time. > > > > So, should I add more compatible strings or not ? > > How do you want me to handle this ? Did you reach an agreement ? Sorry, this slipped off my radar. Probably EBKAC. :) I'm still of the opinion that future-proofing equates to guessing. It has the advantage of, if we guess correctly, things are easier down the road when we discover differences between similar IP blocks. However, if we guess incorrectly, then we have a mess on our hands. iow, this proposal fails poorly. I've no problem breaking DT compatibility when it's determined that we made a mistake (or mistakes) in the past. See the irqchip rework that Marc did a few cycles ago. The difference here is that we know better. We *know* that dtbs are upgraded with the kernel. We *know* that no one is shipping products with dtbs in ROMs. So what are we really trying to protect against? thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] net: mvneta: introduce compatible string "marvell, armada-xp-neta"
Hey Thomas, On Wed, Jun 17, 2015 at 10:43:12PM +0200, Thomas Petazzoni wrote: > On Wed, 17 Jun 2015 17:01:12 +0000, Jason Cooper wrote: > > > I disagree with this. We can't predict what incosistencies we'll discover > > in > > the future. We should only assign new compatible strings based on known IP > > variations when we discover them. This seems fraught with demons since we > > can't predict the scope of affected IP blocks (some steppings of one SoC, > > three > > SoCs plus two steppings of a fourth, etc) > > > > imho, the 'future-proofing' lies in being specific as to the naming of the > > compatible strings against known hardware variations at the time. > > Except that this clearly doesn't work, and the case raised by Simon is > a perfect illustration of why planning ahead is beneficial. Odd, I'd use that as an example of the process working. ;-) we have everyone using 'armada-370-neta' for a given block. We discovered that the original IP block (on the 370s) had a limitation (no hw checksum for greater than 1600 bytes). A newer version of the IP block (XP) doesn't have the limitation. So we change the driver to honor the limit for the 370 compatible string. We create a new compatible string for xp where the block doesn't have the limitation. How did the process fail? > We already had the issue several times on mvebu platforms, so it > should really become the rule to have one compatible string specific > to the actual SoC in the list of compatible strings. Sorry, I'm just not a fan of guessing. But I'll fall back to the DT maintainers on this one. if they are ok with it, then I'll drop my objection. > Not doing so requires breaking DT backward compatibility more often, so > wanting DT backward compatibility and not wanting to plan ahead is a > bit antagonist. I'm not seeing where backwards compatibility was broken? A device with an old dtb booting a newer kernel gets a bugfix. In the case of an XP board with an old dtb (armada-370-neta), the hardware still works, but not optimally. Upgrading the dtb will enable hw checksumming for jumbo packets. thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] net: mvneta: introduce compatible string "marvell, armada-xp-neta"
Hi Gregory, On Wed, Jun 17, 2015 at 05:15:28PM +0200, Gregory CLEMENT wrote: > On 17/06/2015 17:12, Gregory CLEMENT wrote: > > On 17/06/2015 15:19, Simon Guinot wrote: > >> The mvneta driver supports the Ethernet IP found in the Armada 370, XP, > >> 380 and 385 SoCs. Since at least one more hardware feature is available > >> for the Armada XP SoCs then a way to identify them is needed. > >> > >> This patch introduces a new compatible string "marvell,armada-xp-neta". > > > > Let's be future proof by going further. I would like to have an compatible > > string > > for each SoC even if we currently we don't use them. I disagree with this. We can't predict what incosistencies we'll discover in the future. We should only assign new compatible strings based on known IP variations when we discover them. This seems fraught with demons since we can't predict the scope of affected IP blocks (some steppings of one SoC, three SoCs plus two steppings of a fourth, etc) imho, the 'future-proofing' lies in being specific as to the naming of the compatible strings against known hardware variations at the time. thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] net: mvneta: introduce tx_csum_limit property
Thomas, Simon, On Tue, Jun 16, 2015 at 12:00:34PM +0200, Thomas Petazzoni wrote: > On Mon, 15 Jun 2015 17:54:41 +0200, Simon Guinot wrote: > > > The current armada-370-neta would limit the HW checksumming features to > > > packets smaller than 1600 bytes, while a new armada-xp-neta would not > > > have this limit. > > > > This was also my first idea. But by doing this, we take the risk of > > losing the HW checksumming feature with jumbo frames on some currently > > working Armada XP setups. This may happen for example if a user is able > > to update the kernel but not the on-board DTB. In order to fix a feature > > on a SoC, we are breaking the DTB-kernel compatibility for the very same > > feature on an another SoC. I am not sure it is OK. Frankly, this isn't a realistic scenario. We've said from day one that if the dtb is provided with the board that it needs to be updateable. For exactly these kinds of situations. Also, Thomas' assessment is correct, everyone we've ever spoken to is keeping the dtb in sync with the kernel. As long as a board with the old dtb boots a newer kernel without crashing, then it's fine. afaict in this situation, the updated driver should limit HW checksumming for packets >1600 bytes if the compatible string is 'armada-370-neta'. Regardless of actual SoC underneath. If the driver gets 'armada-xp-neta' then there is no checksum limit. Users with an Armada XP SoC and an old dtb will need to upgrade the dtb in order to make use of HW checksumming on jumbo packets with newer kernels. Seems sane to me. thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
